|
@@ -0,0 +1,328 @@
|
|
|
|
|
+# Plugin Revision — v1.4.2 (Fresh-Eye Analytical Review)
|
|
|
|
|
+
|
|
|
|
|
+> **Status as of 1.4.3 (2026-05-12):** every in-scope finding below has been resolved in plugin version **1.4.3**. Each finding now carries an inline status marker — ✅ resolved, ⏸ deliberately deferred. See the README changelog for the corresponding code changes.
|
|
|
|
|
+
|
|
|
|
|
+**Scope:** Independent read-only analytical pass over the v1.4.2 codebase. Re-read all eight PHP files from scratch and looked for issues regardless of whether they'd appeared in earlier reviews. This time I deliberately compared paired code paths (the two import handlers, the two cap-checks, the listener-vs-bulk stamps) and walked through user scenarios that the previous reviews didn't exercise.
|
|
|
|
|
+
|
|
|
|
|
+**Reviewer date:** 2026-05-12
|
|
|
|
|
+**Target environment:** WordPress 6.9.4, WooCommerce 10.7.0 (HPOS enabled), PHP 7.2+.
|
|
|
|
|
+**Files reviewed:** all PHP under `includes/`, the bootstrap `studiou-wc-ord-print-statuses.php`, and the markdown docs.
|
|
|
|
|
+**Verification limitations:** Static review only. No live PHP/WP/WC available.
|
|
|
|
|
+
|
|
|
|
|
+Severity reflects user-visible or correctness impact, not implementation effort.
|
|
|
|
|
+
|
|
|
|
|
+---
|
|
|
|
|
+
|
|
|
|
|
+## 1. Critical
|
|
|
|
|
+
|
|
|
|
|
+*(None.)*
|
|
|
|
|
+
|
|
|
|
|
+---
|
|
|
|
|
+
|
|
|
|
|
+## 2. High
|
|
|
|
|
+
|
|
|
|
|
+### 2.1 `import_delivered_protocol` does not skip terminal-status orders — asymmetry with `import_inprint_protocol` — ✅ resolved in 1.4.3
|
|
|
|
|
+
|
|
|
|
|
+Resolution: added a `has_status(['cancelled', 'refunded', 'failed'])` guard with the same per-row error and `UtilsLog::log` call as the InPrint path. `completed` is intentionally excluded — re-running a delivered protocol on already-completed orders is a benign no-op.
|
|
|
|
|
+
|
|
|
|
|
+`class-db-manager.php:200-230` (InPrint) and `:238-258` (Delivered).
|
|
|
|
|
+
|
|
|
|
|
+The InPrint import explicitly skips orders in `cancelled`, `refunded`, `failed`, or `completed` states:
|
|
|
|
|
+
|
|
|
|
|
+```php
|
|
|
|
|
+if ($order->has_status(self::TERMINAL_STATUSES)) {
|
|
|
|
|
+ $errors[] = sprintf(__('Order %s is in a final state and was not updated.', …), $row['order_no']);
|
|
|
|
|
+ continue;
|
|
|
|
|
+}
|
|
|
|
|
+$order->update_status('in-print');
|
|
|
|
|
+```
|
|
|
|
|
+
|
|
|
|
|
+The Delivered import does not:
|
|
|
|
|
+
|
|
|
|
|
+```php
|
|
|
|
|
+$order = wc_get_order($row['order_no']);
|
|
|
|
|
+if (!$order) { /* ... */ continue; }
|
|
|
|
|
+// ← no terminal-status guard here
|
|
|
|
|
+$order->update_status('completed');
|
|
|
|
|
+$order->update_meta_data('external_ref_ord_delivered', current_time('mysql'));
|
|
|
|
|
+```
|
|
|
|
|
+
|
|
|
|
|
+**Concrete failure modes:**
|
|
|
|
|
+
|
|
|
|
|
+- A `cancelled` order that appears in the print shop's delivered CSV (operator error, stale data, ghost entry) is silently **reactivated to `completed`**.
|
|
|
|
|
+- A `refunded` order is reactivated to `completed`. The refund accounting in WC is now inconsistent with order state.
|
|
|
|
|
+- A `failed` order is reactivated to `completed`. Same problem.
|
|
|
|
|
+
|
|
|
|
|
+Whether the asymmetry is intentional cannot be inferred from the spec (`docs/instructions.txt` line 32-36 does not address terminal states). It looks accidental — the cancellation paths in `bulk_set_print_status` and `import_inprint_protocol` are clearly defensive, and `import_delivered_protocol` was implemented to a parallel template but the guard wasn't ported.
|
|
|
|
|
+
|
|
|
|
|
+**Suggested fix:** mirror the InPrint guard. Optionally skip `completed` from the list since `completed → completed` is a no-op transition anyway and operators may legitimately re-deliver the same protocol.
|
|
|
|
|
+
|
|
|
|
|
+### 2.2 Import handlers require `manage_woocommerce`, bulk actions require `edit_shop_orders` — shop managers cannot use the import screen — ✅ resolved in 1.4.3
|
|
|
|
|
+
|
|
|
|
|
+Resolution: both `add_submenu_page()` and the `current_user_can` check in `run_import()` now use `edit_shop_orders`. Shop Managers see and can use the Import Protocols screen, matching the bulk-action capability requirement.
|
|
|
|
|
+
|
|
|
|
|
+`class-import-manager.php:106` (`current_user_can('manage_woocommerce')`) vs `class-bulk-actions-manager.php:28` (`current_user_can('edit_shop_orders')`).
|
|
|
|
|
+
|
|
|
|
|
+WC's default capabilities:
|
|
|
|
|
+- The **Shop Manager** role grants `edit_shop_orders` (and most order-management caps) **but not** `manage_woocommerce`.
|
|
|
|
|
+- `manage_woocommerce` is the WC administrator cap, normally given only to the Administrator role.
|
|
|
|
|
+
|
|
|
|
|
+So a Studiou employee with the Shop Manager role can:
|
|
|
|
|
+- Run the three bulk actions (`current_user_can('edit_shop_orders')` ✓).
|
|
|
|
|
+- Edit the Print Information panel on the order edit screen (`current_user_can('edit_shop_order', $order_id) || 'edit_shop_orders'` ✓).
|
|
|
|
|
+
|
|
|
|
|
+But they cannot:
|
|
|
|
|
+- Open **WooCommerce → Import Protocols** (`manage_woocommerce` ✗) — `add_submenu_page` hides the page itself, and `wp_die` blocks the POST handler.
|
|
|
|
|
+
|
|
|
|
|
+For day-to-day print-shop ops this means every protocol import has to go through an administrator, which is almost certainly not intended. Either:
|
|
|
|
|
+
|
|
|
|
|
+- Lower the import requirement to `edit_shop_orders` to match the bulk-action requirement, **or**
|
|
|
|
|
+- Raise the bulk-action requirement to `manage_woocommerce` and document the policy intent.
|
|
|
|
|
+
|
|
|
|
|
+The first option is the operationally pragmatic one; the second would be the more locked-down choice.
|
|
|
|
|
+
|
|
|
|
|
+---
|
|
|
|
|
+
|
|
|
|
|
+## 3. Medium
|
|
|
|
|
+
|
|
|
|
|
+### 3.1 `table_exists($table)` doesn't escape MySQL LIKE wildcards in `$wpdb->prefix` — ✅ resolved in 1.4.3
|
|
|
|
|
+
|
|
|
|
|
+Resolution: `$wpdb->esc_like($table)` is applied before `$wpdb->prepare()`. The post-query equality check stays as a belt-and-braces safeguard.
|
|
|
|
|
+
|
|
|
|
|
+`class-db-manager.php:128-132`:
|
|
|
|
|
+
|
|
|
|
|
+```php
|
|
|
|
|
+private static function table_exists($table) {
|
|
|
|
|
+ global $wpdb;
|
|
|
|
|
+ $found = $wpdb->get_var($wpdb->prepare('SHOW TABLES LIKE %s', $table));
|
|
|
|
|
+ return $found === $table;
|
|
|
|
|
+}
|
|
|
|
|
+```
|
|
|
|
|
+
|
|
|
|
|
+`$wpdb->prepare(..., '%s')` quotes the string for SQL, but `SHOW TABLES LIKE` interprets the value as a LIKE pattern where `_` is a single-character wildcard and `%` is a multi-character wildcard. `$wpdb->prefix` is conventionally alphanumeric + underscore (e.g., `wp_`, `wp_test_`), so the underscores in the prefix become wildcards.
|
|
|
|
|
+
|
|
|
|
|
+For prefix `wp_test_`, the LIKE pattern `wp_test_wc_order_product_lookup` matches:
|
|
|
|
|
+
|
|
|
|
|
+- `wp_test_wc_order_product_lookup` ✓ (intended)
|
|
|
|
|
+- `wpXtestXwcXorderXproductXlookup` (each `_` matches any char)
|
|
|
|
|
+- `wp1test2wc3order4product5lookup`
|
|
|
|
|
+
|
|
|
|
|
+The strict `=== $table` comparison after the query mitigates false positives — if a wildcard match returns a different table name, the equality check rejects it. **So today this is safe**, but the safety relies on the post-query comparison rather than on the query itself returning at most the intended row. Two ways the post-comparison could miss:
|
|
|
|
|
+
|
|
|
|
|
+1. If two tables happen to match the pattern and one is `wp_test_wc_order_product_lookup` exactly, MySQL returns multiple rows; `$wpdb->get_var` returns the first one. Order is engine-dependent.
|
|
|
|
|
+2. If a future contributor moves `table_exists` to a context where multiple matches matter (e.g., a `tables_like` helper), the wildcard semantics will silently misbehave.
|
|
|
|
|
+
|
|
|
|
|
+**Suggested fix:**
|
|
|
|
|
+
|
|
|
|
|
+```php
|
|
|
|
|
+$found = $wpdb->get_var($wpdb->prepare('SHOW TABLES LIKE %s', $wpdb->esc_like($table)));
|
|
|
|
|
+```
|
|
|
|
|
+
|
|
|
|
|
+`$wpdb->esc_like()` escapes `%` and `_` for LIKE pattern use.
|
|
|
|
|
+
|
|
|
|
|
+### 3.2 `set_order_processing_status` is effectively dead code on modern WC — ✅ resolved in 1.4.3
|
|
|
|
|
+
|
|
|
|
|
+Resolution: removed the handler and its `woocommerce_payment_complete` hook registration. The corresponding translation entry in `docs/translations.txt` was removed too. If a future custom payment gateway is found to fire the action without first setting the status, the handler can be reintroduced with an explanatory comment.
|
|
|
|
|
+
|
|
|
|
|
+`class-order-status-manager.php:112-129`. The handler is hooked to `woocommerce_payment_complete`, which is fired by `WC_Order::payment_complete()`. WC's own implementation of `payment_complete()`:
|
|
|
|
|
+
|
|
|
|
|
+1. Determines the destination status based on order content (downloadable-only → `completed`, otherwise → `processing`).
|
|
|
|
|
+2. Sets the order to that status **before** firing the action.
|
|
|
|
|
+
|
|
|
|
|
+When our handler then runs:
|
|
|
|
|
+
|
|
|
|
|
+- If WC set `processing`, our `update_status('processing')` is a no-op (same-status, WC short-circuits inside `set_status`).
|
|
|
|
|
+- If WC set `completed` (downloadable-only orders), our `has_status(['completed', 'to-print', 'in-print'])` guard hits → we return.
|
|
|
|
|
+
|
|
|
|
|
+In both cases the handler does nothing. It would only matter if a custom payment gateway fires `woocommerce_payment_complete` without first setting the order status — which is non-standard and arguably the gateway's bug.
|
|
|
|
|
+
|
|
|
|
|
+Two options:
|
|
|
|
|
+
|
|
|
|
|
+- **Remove it.** The hook serves no purpose on a default WC installation.
|
|
|
|
|
+- **Keep it and document.** Add a comment explaining what scenario it defends against (legacy gateway behaviour from pre-WC-4.0 days?). Otherwise a future reader will be confused.
|
|
|
|
|
+
|
|
|
|
|
+The order note string (`"Payment received — order automatically set to processing by …"`) appears in `docs/translations.txt` and is shipped translated — if the code is removed, the translation entry should follow it.
|
|
|
|
|
+
|
|
|
|
|
+### 3.3 CSV injection — output values may trigger Excel formula execution — ✅ resolved in 1.4.3
|
|
|
|
|
+
|
|
|
|
|
+Resolution: `Studiou_DB_Manager::csv_escape()` is applied to every cell value in the PHP post-processing loop. Cells beginning with `=`, `+`, `-`, `@`, `\t`, or `\r` get a leading single quote — hidden by Excel/LibreOffice display, prevents formula evaluation.
|
|
|
|
|
+
|
|
|
|
|
+`class-bulk-actions-manager.php:113-115` writes CSV cells directly from the export-query result. CSV cells beginning with `=`, `+`, `-`, `@`, or `\t` are interpreted by Excel and LibreOffice Calc as formulas. If a customer's `billing_email`, product name, or category name happens to begin with one of these characters — accidentally or maliciously — the print shop opens the CSV in Excel and a formula executes.
|
|
|
|
|
+
|
|
|
|
|
+Realistic vectors:
|
|
|
|
|
+
|
|
|
|
|
+- A customer registers `=cmd|'/c calc'!A1` as their email. The export includes it as-is. Excel parses it as a formula.
|
|
|
|
|
+- A product category gets renamed to `=HYPERLINK("https://evil/", "click")` by a compromised admin or contractor. The export embeds it; opening the CSV silently navigates the cell-link target.
|
|
|
|
|
+
|
|
|
|
|
+The mitigation in WP-aware code is to prefix any cell value starting with a dangerous character with a leading single-quote (`'`) or with a leading `\t`-equivalent zero-width-width control. The OWASP-recommended pattern is the leading single-quote:
|
|
|
|
|
+
|
|
|
|
|
+```php
|
|
|
|
|
+private function csv_escape($value) {
|
|
|
|
|
+ $value = (string) $value;
|
|
|
|
|
+ if ($value !== '' && in_array($value[0], array('=', '+', '-', '@', "\t", "\r"), true)) {
|
|
|
|
|
+ return "'" . $value;
|
|
|
|
|
+ }
|
|
|
|
|
+ return $value;
|
|
|
|
|
+}
|
|
|
|
|
+```
|
|
|
|
|
+
|
|
|
|
|
+Apply this to every cell before passing it to `fputcsv`. Excel still shows the value (without the leading quote) but does not evaluate it.
|
|
|
|
|
+
|
|
|
|
|
+The risk is **low likelihood, real impact** — print shop spreadsheet users are a concrete attack surface for this class of vulnerability.
|
|
|
|
|
+
|
|
|
|
|
+### 3.4 `datetime_local_to_mysql` accepts structurally-valid but semantically-out-of-range values — ✅ resolved in 1.4.3
|
|
|
|
|
+
|
|
|
|
|
+Resolution: `checkdate()` validates the date components; explicit numeric range checks reject hour > 23, minute > 59, second > 59. Bogus values return empty string instead of being stored.
|
|
|
|
|
+
|
|
|
|
|
+`class-order-fields-manager.php:108-118`:
|
|
|
|
|
+
|
|
|
|
|
+```php
|
|
|
|
|
+if (preg_match('/^(\d{4}-\d{2}-\d{2})T(\d{2}:\d{2})(?::(\d{2}))?$/', $value, $m)) {
|
|
|
|
|
+ $seconds = isset($m[3]) ? $m[3] : '00';
|
|
|
|
|
+ return $m[1] . ' ' . $m[2] . ':' . $seconds;
|
|
|
|
|
+}
|
|
|
|
|
+```
|
|
|
|
|
+
|
|
|
|
|
+The regex matches `2026-13-45T25:99` as well as `2026-05-12T14:30`. The function then stores `2026-13-45 25:99:00` to postmeta. MySQL's behaviour for an out-of-range datetime depends on `sql_mode`:
|
|
|
|
|
+
|
|
|
|
|
+- Strict mode: rejects the value, raises an error.
|
|
|
|
|
+- Non-strict mode: silently coerces to `0000-00-00 00:00:00`.
|
|
|
|
|
+
|
|
|
|
|
+Browsers' native `<input type="datetime-local">` validate the value before submit, so this is unlikely to come from a normal browser flow. But:
|
|
|
|
|
+
|
|
|
|
|
+- A power user editing the form via DevTools, or
|
|
|
|
|
+- A script POSTing directly to the order edit URL,
|
|
|
|
|
+
|
|
|
|
|
+…can produce the bogus value. The downstream display (`mysql_to_datetime_local`) would reject the bogus stored value (the back-conversion regex would still match structurally but the data is meaningless) and show an empty field.
|
|
|
|
|
+
|
|
|
|
|
+**Suggested fix:** validate via `checkdate()` and seconds/minutes/hours range before composing the MySQL string. Defense-in-depth.
|
|
|
|
|
+
|
|
|
|
|
+---
|
|
|
|
|
+
|
|
|
|
|
+## 4. Low
|
|
|
|
|
+
|
|
|
|
|
+### 4.1 CSV uses `,` separator — Excel CS-locale users see single-column data — ⏸ documented, no code change
|
|
|
|
|
+
|
|
|
|
|
+Resolution path: a `sep=,\n` directive at the top of the file (the Microsoft proprietary extension Excel respects) would break the export-then-reimport workflow because CSV parsers would see `sep=,` as the first header row. The README "Known limitations" section now documents the workaround (use Excel's Text Import wizard or change Windows regional list-separator).
|
|
|
|
|
+
|
|
|
|
|
+The plugin exports with comma-separated values. The Czech edition of Excel defaults to `;` as the list separator (because comma is the decimal separator), so a Czech operator opening the file in Excel without changing the locale or pre-converting sees every row as one cell.
|
|
|
|
|
+
|
|
|
|
|
+LibreOffice Calc prompts for the separator on import; Excel-CS does not. Workarounds the operator can apply: rename the file extension to `.txt` and use Excel's text-import wizard, or change Windows regional settings. Neither is ideal.
|
|
|
|
|
+
|
|
|
|
|
+Options:
|
|
|
|
|
+
|
|
|
|
|
+- Add a `\xEF\xBB\xBFsep=,\n` directive at the very top of the CSV (Microsoft proprietary extension; understood by Excel).
|
|
|
|
|
+- Make the separator a settable plugin option.
|
|
|
|
|
+- Document the locale workaround.
|
|
|
|
|
+
|
|
|
|
|
+### 4.2 `SUM(order_products.product_qty)` returns DECIMAL — CSV shows `2.0000` for integer quantities — ✅ resolved in 1.4.3
|
|
|
|
|
+
|
|
|
|
|
+Resolution: `Studiou_DB_Manager::format_quantity()` normalises whole numbers to integer string (`"2"`) and trims trailing zeros from decimals (`"2.5"` for `"2.5000"`).
|
|
|
|
|
+
|
|
|
|
|
+`wc_order_product_lookup.product_qty` is `DECIMAL(8,4)` in the WC schema. `SUM()` of `DECIMAL` returns `DECIMAL`. `$wpdb->get_results()` returns strings. The CSV row therefore contains `2.0000` for an order of two items, not `2`.
|
|
|
|
|
+
|
|
|
|
|
+The print shop's downstream tooling may or may not care. Cosmetic but minor friction.
|
|
|
|
|
+
|
|
|
|
|
+**Suggested fix:** in the PHP post-processing step that already resolves `prod_img_url`, also normalise `$row->qty` — if the value is a whole number, drop trailing zeros and the decimal point.
|
|
|
|
|
+
|
|
|
|
|
+### 4.3 `import_delivered_protocol::update_status('completed')` doesn't pass a note — ✅ resolved in 1.4.3
|
|
|
|
|
+
|
|
|
|
|
+Resolution: both import handlers now pass an explanatory note as the second argument to `update_status` — "Marked In Printing by InPrint Protocol import." and "Marked completed by Delivered Protocol import." The order activity feed reflects how the transition was triggered.
|
|
|
|
|
+
|
|
|
|
|
+`class-db-manager.php:252` — no second argument. WC writes a default-styled order note ("Order status changed from … to completed."). For traceability, passing a note like `"Marked completed by Delivered Protocol import."` would distinguish protocol-driven completions from manual ones. The InPrint and bulk paths similarly don't pass notes; an opportunity to add operational context across the board.
|
|
|
|
|
+
|
|
|
|
|
+### 4.4 Import handlers don't log per-row to `UtilsLog::log` — ✅ resolved in 1.4.3
|
|
|
|
|
+
|
|
|
|
|
+Resolution: both import handlers now write `UtilsLog::log` entries for not-found orders, terminal-status skips, and per-row success — symmetric with `bulk_set_print_status`.
|
|
|
|
|
+
|
|
|
|
|
+`bulk_set_print_status` logs per-order success (`Order #N marked as "to-print"`) and per-order skip. The two import handlers log only at the bulk-action-level (via `UtilsLog::message`); they don't write to `error_log` per row. When debugging a failed import, the operator/developer cannot see which orders were touched and which weren't.
|
|
|
|
|
+
|
|
|
|
|
+Asymmetric with the bulk path; consistency would help.
|
|
|
|
|
+
|
|
|
|
|
+### 4.5 `Order_Status_Manager::set_order_processing_status` has no audit trail of why it was a no-op — ✅ resolved by removal in 1.4.3 (see §3.2)
|
|
|
|
|
+
|
|
|
|
|
+When `set_order_processing_status` skips because of the protected-status guard (`completed`, `to-print`, `in-print`), it returns silently. Adding `UtilsLog::log` would help debug "why didn't payment_complete advance my order to processing?" support questions.
|
|
|
|
|
+
|
|
|
|
|
+Related to §3.2 — if the handler is kept as-is, instrument it; if removed, this is moot.
|
|
|
|
|
+
|
|
|
|
|
+### 4.6 `array_filter` without callback removes order ID 0 — ✅ resolved in 1.4.3
|
|
|
|
|
+
|
|
|
|
|
+Resolution: `array_filter` now uses an explicit `function ($id) { return $id > 0; }` callback. Intent is unambiguous in the source.
|
|
|
|
|
+
|
|
|
|
|
+`class-db-manager.php:42` — `array_filter(array_map('intval', (array) $order_ids))`. `array_filter` with no callback drops all falsy values, including integer `0`. WC orders cannot have ID 0 in practice, so this is harmless, but it's slightly misleading — a future contributor adapting this for `WP_Term` IDs or anything where 0 is a legitimate value would inherit the bug.
|
|
|
|
|
+
|
|
|
|
|
+Cosmetic.
|
|
|
|
|
+
|
|
|
|
|
+### 4.7 Trashed products still appear in the export — ✅ documented in README "Known limitations"
|
|
|
|
|
+
|
|
|
|
|
+Resolution path: this is intentional — historical orders should reflect what was sold. The README "Known limitations" section now makes that explicit. No code change.
|
|
|
|
|
+
|
|
|
|
|
+The `JOIN posts products ON products.ID = order_products.product_id` doesn't filter on `post_status`. A product whose post has been moved to trash (or set to draft) still produces a CSV row for any historical order line item. The print shop sees the product name as it was at the time of the trash, not what the customer originally saw.
|
|
|
|
|
+
|
|
|
|
|
+Probably intended — historical orders should reflect what was sold, not the current product state — but worth documenting.
|
|
|
|
|
+
|
|
|
|
|
+### 4.8 `import_inprint_protocol` and `import_delivered_protocol` emit one `Invalid row format in CSV.` error per malformed row — ✅ resolved in 1.4.3
|
|
|
|
|
+
|
|
|
|
|
+Resolution: `Import_Manager::emit_import_summary()` runs `array_unique()` on the error list before formatting the notice. The numeric total still reflects the full error count.
|
|
|
|
|
+
|
|
|
|
|
+`class-db-manager.php:206, 244`. If a CSV has 100 malformed rows, the error list has 100 identical strings. The Import Manager's `emit_import_summary` joins them with `, ` and shows the count — but the joined string can become very long. A simple `array_unique` on the errors list or a per-error-kind counter would tighten the UX.
|
|
|
|
|
+
|
|
|
|
|
+### 4.9 `wp_kses_post()` allows `<a href>` — admin notice content could include arbitrary links — ✅ resolved in 1.4.3
|
|
|
|
|
+
|
|
|
|
|
+Resolution: `UtilsLog::render_notices()` now uses `esc_html()` for the notice body. All current callers pass plain text; future callers wanting markup will need to pre-format with their own escape strategy.
|
|
|
|
|
+
|
|
|
|
|
+`utils-log.php:66`. None of the current call sites pass HTML containing user-controlled links, but the `message()` API doesn't restrict what callers can pass. If a future caller passes a partially-escaped string with a user-controlled URL, `wp_kses_post` would allow `<a href="...">` through. The current set of callers builds notices from sprintf-of-translated-strings plus `esc_html`'d data, so no live risk — but the trust boundary is the API not the call sites.
|
|
|
|
|
+
|
|
|
|
|
+A narrower allowlist via `wp_kses($html, array())` for plain-text-only or `wp_kses($html, array('code' => array(), 'strong' => array(), 'em' => array()))` would be defense-in-depth.
|
|
|
|
|
+
|
|
|
|
|
+### 4.10 `register_post_status` keeps being called even though HPOS doesn't read it — ⏸ left as-is
|
|
|
|
|
+
|
|
|
|
|
+Carried over from `revise-1.4.1.md` §4.8 — harmless under HPOS, kept for admin-chrome compatibility. No change.
|
|
|
|
|
+
|
|
|
|
|
+---
|
|
|
|
|
+
|
|
|
|
|
+## 5. Verification gaps (carried over from earlier reviews)
|
|
|
|
|
+
|
|
|
|
|
+1. **HPOS search filter name** `woocommerce_order_table_search_query_meta_keys` against WC 10.7.0 source — still pending live verification.
|
|
|
|
|
+2. **`woocommerce_process_shop_order_meta`** hook behaviour under WC 10.7.0 block-based order edit screen — pending live verification.
|
|
|
|
|
+3. **Object-cache scenarios for `UtilsLog::message`** — pending live verification under Redis/Memcached configurations.
|
|
|
|
|
+
|
|
|
|
|
+---
|
|
|
|
|
+
|
|
|
|
|
+## 6. Status of prior reviews
|
|
|
|
|
+
|
|
|
|
|
+| Review | Status of in-scope findings | Status of deferred findings |
|
|
|
|
|
+|--------|------------------------------|------------------------------|
|
|
|
|
|
+| `revise-1.3.2.md` | All resolved in 1.4.0 | 3 still deferred. |
|
|
|
|
|
+| `revise-1.4.0.md` | All resolved in 1.4.1 | 1 verification-pending (HPOS filter). |
|
|
|
|
|
+| `revise-1.4.1.md` | All resolved in 1.4.2 | 2 documented-without-code-change. |
|
|
|
|
|
+
|
|
|
|
|
+No regressions detected. The new findings in this review (§2.1, §2.2) are pre-existing defects that survived three prior reviews because the paired code paths were never compared side-by-side.
|
|
|
|
|
+
|
|
|
|
|
+---
|
|
|
|
|
+
|
|
|
|
|
+## 7. Risk-prioritised summary
|
|
|
|
|
+
|
|
|
|
|
+Ordered by user-visible severity. Status as of **1.4.3**:
|
|
|
|
|
+
|
|
|
|
|
+1. **§2.1 Delivered import reactivates cancelled/refunded orders to `completed`.** ✅ resolved.
|
|
|
|
|
+2. **§2.2 Shop managers cannot run protocol imports.** ✅ resolved.
|
|
|
|
|
+3. **§3.3 CSV injection in exports.** ✅ resolved.
|
|
|
|
|
+4. **§3.1 `SHOW TABLES LIKE` wildcard escape.** ✅ resolved.
|
|
|
|
|
+5. **§3.2 `set_order_processing_status` is dead code on modern WC.** ✅ resolved (removed).
|
|
|
|
|
+6. **§3.4 `datetime_local_to_mysql` accepts out-of-range structurally-valid values.** ✅ resolved.
|
|
|
|
|
+7. **§4.1 CSV `,` separator vs Czech Excel locale.** ⏸ documented in README "Known limitations".
|
|
|
|
|
+8. **§4.2 `SUM(qty)` produces `2.0000` not `2`.** ✅ resolved.
|
|
|
|
|
+
|
|
|
|
|
+---
|
|
|
|
|
+
|
|
|
|
|
+## 8. Out of scope
|
|
|
|
|
+
|
|
|
|
|
+- Live behaviour testing against a running WP 6.9.4 / WC 10.7.0 install.
|
|
|
|
|
+- Performance profiling of the export query under realistic order volumes.
|
|
|
|
|
+- Refactoring the manager pattern into typed DI / interfaces (PHP 7.4+).
|
|
|
|
|
+- Multisite, uninstall cleanup, block-based order edit screen (carried-over deferrals).
|
|
|
|
|
+
|
|
|
|
|
+---
|
|
|
|
|
+
|
|
|
|
|
+*End of review.*
|