# Shopify Plugin — SQL Injection Hardening: Change Summary & Smoke-Test Methodology

**Date:** 2026-06-05
**Scope:** `plugins/shopify/plugin.php` and shared include `invspecsearchsection.inc`
**Goal:** Close every request-derived SQL interpolation surface in the Shopify plugin without changing observable behaviour.

---

## Part 1 — Summary of Changes

### A. ORDER BY injection sinks (3) — column/direction whitelisting

Each register built `ORDER BY $_REQUEST[sortby] $_REQUEST[sortdir]` from raw input. Fixed using the
established `validateSortby($allowed,$default)` / `safeSortdir()` helpers from `functions.php`
(**Group B pattern**: validate the raw value *before* the function's existing alias/IF transforms,
then leave those transforms untouched).

| # | Function | ~Line | Whitelist anchor |
|---|----------|------|------------------|
| 1 | `fetchInventoryRegistry` (inventory register) | 3489 | `code, desc, sku, cost, price, soh` → default `code` |
| 2 | `fetchProjectRegistry` (jobs register) | 5936 | `id, statustext, j.company, reference, suburb, s.firstname, c.name, price, dateadded, nextaction, uninvoicedwip` → default `id` |
| 3 | `fetchAccountsRegistry` (clients register) | 8944 | `code, company, trading_names, abn, address4, state, telephone, email, createdate, id` → default `id` |

Whitelists mirror each register's client-side `data-sort-field` values.

### B. WHERE-clause parameterization of the 3 search registers

These functions built dynamic `WHERE` fragments by interpolating request data. The two that used
`getFieldArray()` were **converted to `DB::SELECT(...)`** because `getFieldArray`/`selectQuery` in
`functions.php` have **no bind-parameter support**. All converted calls use
`PDO::FETCH_ASSOC` (5th arg) because downstream code reads `$result[$i]['col']`.

**Technique:** build SQL fragments with positional `?` placeholders while accumulating values into a
`$params` array *in exact left-to-right query order*; `array_merge()` fragments in placeholder order.

- **`fetchInventoryRegistry` (~3382):**
  - desc multi-word search → ``(`desc` like ?)`` fragments, values pushed to `$sqlParams`.
  - category filter → `cat IN (?,?,…)` via `array_fill`, values = `$subcats`.
  - main query passed `$invParams = array_merge([code, code], $sqlParams, [desc], $catParams, [sku, sku, status])`.
  - `$page` cast to `(int)`.
  - `$invspecsearch` / `$shopifysearch` left interpolated — DB-derived / sanitized (see §D).

- **`fetchAccountsRegistry` (~8863):** removed manual `\'` escaping; rebuilt `$clientSQL`
  (LIKE/REGEXP branches) with `?` + `$clientParams`; converted
  `getFieldArray("*","contacts",$where)` → `DB::SELECT("SELECT * from contacts WHERE …", $clientParams + 9-keyword-block + 6-filter-block, null, 0, PDO::FETCH_ASSOC)`. `$page` cast `(int)`.

- **`fetchProjectRegistry` (~5743):** removed `\'` escaping; `$contactSQL`, custommeta sub-query
  (`(meta_id,meta_val) IN (?,?…)` + `HAVING COUNT(*) = ?`), `$pms` (`pmId IN (?,…)`),
  `$retentionSQL` (`rdate >= ? / <= ? / BETWEEN ? AND ?`), and `branch`/`cat like ?` all parameterized.
  Converted `getFieldArray(…,"jobs,contacts,staff,jobCats",…)` → `DB::SELECT(…, $jobParams, null, 0, PDO::FETCH_ASSOC)`.
  Param order: **contact → type → pm → branch → retention**. `$request_search_string`,
  `$new_flag`, `$statusSql`, `$jobSql`, `$shopifysearch` left interpolated (sanitized / constants / DB-derived).

### C. 5 additional `$search_string` / `$jobinvid` LIKE sinks parameterized

| Function | ~Line | Change |
|----------|------|--------|
| `getEvoProducts` (`call=getEvoProducts`, bulkproducts modal) | 1986 | 4× `'%$search_string%'` → `?`; params `[cat, str×4]` |
| `getEvoCats` (`call=getEvoCats`, bulkcategory modal) | 2718 | 2× → `?` |
| `getB2BCompanies` (`call=getB2BCompanies`) | 3813 | 5× → `?` |
| `fetchCategoryRegistry` (`call=getCats`) | 9058 | 2× → `?`; `$shopifysearch` is constant literals |
| `getDispatchedJobItems` (called by order/dispatch sync, 8339/8514) | 8245 | `jobid = ?`, `jobInventory.id LIKE ?` bound |

### D. Deliberately left interpolated (provably safe — documented for reviewers)

- `$request_search_string` — sanitized `preg_replace('/[^A-Za-z0-9]/',' ',…)` → alphanumeric+space only.
- `$shopifysearch`, `$new_flag`, `$statusSql`, `$jobSql` — built from constant literals / DB-derived ids, not request input.
- `$valueSql` (~8922/8928) — guarded by `is_numeric()`; **not used** in the contacts WHERE (dead in that path). Untouched.

### E. Shared include `invspecsearchsection.inc` — hardened in place

This include builds SQL fragments that **consumers interpolate** (core `inventory.inc` also uses it), so
it cannot bind. Hardened at the source instead:

- `$invsid` → `(int)` cast.
- `$invsval` → `addslashes()` (quote-escaped; was already comma-split).
- `$invscond` → whitelisted to literal `AND` / `OR` (default `AND`).
- `$invsname` left as-is — not a SQL sink.

### Verification performed
- `php -l` clean after every edit on both files.
- Placeholder↔parameter count and order re-verified by reading each rewritten query.
- Final grep sweep: no raw `$_GET`/`$_POST` in SQL; no residual `$_REQUEST` in query strings except the documented-safe §D cases.

---

## Part 2 — Smoke-Test Methodology

### Principles
The fixes are **behaviour-preserving**. For every surface, assert four things:
1. **Functional parity** — a normal search returns the same rows as before.
2. **Literal special chars** — `'`, `%`, `_`, `\` are treated as *data*, not syntax (no SQL error, no over-broad match).
3. **Injection neutralized** — classic probes return the literal-match result set (usually empty) and never error or leak.
4. **Regression** — sort, pagination, and adjacent filters still work.

### Test environment
- Use the dev LAMP stack against a non-production tenant DB (see workspace mapping).
- Be logged in with a Shopify-plugin-enabled company so `addHooks()` registers the replace-hooks.
- Watch the MariaDB **general/slow log** (or temporarily enable PDO error display) so a malformed
  query surfaces as an error rather than a silent empty result.
- Tools: browser UI for each register/modal, plus `curl` for direct `call=` probes (faster for injection cases).

### Standard probe payloads (reuse across every field)
| Class | Payload | Pass criterion |
|-------|---------|----------------|
| Quote | `O'Brien` | row matched literally; no SQL error |
| Wildcards | `50%`, `a_b` | treated as literal chars (no over-match) |
| Backslash | `a\b` | literal; no error |
| Boolean SQLi | `' OR '1'='1` | **empty/zero rows** (literal match), no error |
| UNION | `x' UNION SELECT … --` | empty, no error, no extra columns |
| ORDER BY SQLi | `sortby=(CASE WHEN 1=1 THEN id ELSE code END)` | falls back to whitelist default; stable order |
| ORDER BY enum | `sortby=nonexistentcol` | falls back to default column, no error |
| Sortdir | `sortdir=; DROP` | coerced to `asc`/`desc` |

### Per-surface test matrix

**1. Inventory register** — `inveditsave.php`, `call=fetch_inventory` (POST) → `fetchInventoryRegistry`
- Search by code, desc (multi-word, e.g. `steel bracket`), sku.
- Apply a category filter (drives `cat IN (…)`) and the inv-spec search (`invspecsearchsection.inc`).
- Run all probes in `desc`/`code`/`sku`. Multi-word desc must AND the words (parity check).
- Sort each column asc/desc; ORDER BY probes; paginate beyond page 1 (`$page` int-cast).

**2. Jobs register** — `jobeditsave.php`, `call=filter` → `fetchProjectRegistry`
- Search contact (company/lastname/code/trading_names multi-word), filter by status, type,
  one and multiple project managers (`pmId IN`), branch, custom-meta criteria, retention date
  (from-only, to-only, between).
- Probes in contact field; verify PM multi-select still filters correctly; verify retention date ranges.
- Sort each column (incl. `statustext`, `uninvoicedwip`) asc/desc; ORDER BY probes; paginate.

**3. Clients register** — `clienteditsave.php`, `call=getClients` (POST) → `fetchAccountsRegistry`
- Keyword search (LIKE + word-boundary REGEXP branches), company keyword, suburb, state, type,
  staff, stop-credit, status filters.
- `O'Brien`-style names must match literally (was previously `\'`-escaped — verify still correct).
- Probes in keyword & company; sort each column; ORDER BY probes; paginate.

**4. Category register** — `invcats.php`, `call=getCats` (POST) → `fetchCategoryRegistry`
- Search categories by string; probes; verify result parity.

**5. Shopify configurator modals (AJAX `call=`):**
- `getEvoProducts` — bulkproducts modal product filter (also exercises category arg).
- `getEvoCats` — bulkcategory modal.
- `getB2BCompanies` — B2B company picker.
- For each: type a normal search → expect matching rows; run quote/wildcard/`' OR '1'='1` probes → literal/empty, no error.

**6. Dispatch / order sync** — `getDispatchedJobItems` (internal, 8339/8514)
- Not a user search box; exercised by running a Shopify **order fulfilment / dispatch status sync**
  for a job that has dispatched items. Confirm the sync still resolves the correct job-inventory rows
  (the `jobid = ?` / `jobInventory.id LIKE ?` binding). Compare item set against pre-change behaviour.

**7. Core inventory page (shared include regression)** — `?page=inventory`
- Because `invspecsearchsection.inc` is shared, run the inventory page's **spec search**:
  add several spec criteria with AND and OR connectors, including a value containing a quote.
- Verify: AND/OR logic unchanged, quoted values match literally, spec-id filtering still works,
  no SQL error. (Confirms the in-place hardening didn't alter core behaviour.)

### Sign-off checklist
- [ ] All 7 surfaces: normal search parity confirmed.
- [ ] All text fields: quote/wildcard/backslash treated as literals, no errors.
- [ ] All fields: `' OR '1'='1` and UNION probes return literal/empty, no error, no leak.
- [ ] All 3 registers: ORDER BY column-enum + expression probes fall back to default; sortdir coerced.
- [ ] Pagination works past page 1 on inventory/jobs/clients.
- [ ] Job register: PM multi-select, retention date ranges, custom-meta all functional.
- [ ] Core `?page=inventory` spec search (AND/OR + quoted value) unchanged.
- [ ] MariaDB log shows **zero** SQL syntax/prepared-statement errors across the run.