Security review pass 6: raw SQL

From code: KPI interval criterion placed in SQL unchecked, reachable by staff
down to TechRestricted (F26, critical). F18 (pick-list search text) completed
by code. Data-list filters, sort, ClientCriteria, saved filters, search, other
KPI criteria and remaining raw queries traced and found safe. Review only.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

devdocs/modernization/SECURITY-REVIEW.md

@@ -16,8 +16,8 @@ keep it that way while findings are open.
 | 3 | Customer contact users: what they can reach and read | Done 2026-10-23 |
 | 4 | Report rendering (templates, the server-side browser, report data and download routes) | Done 2026-10-23 |
 | 5 | File handling: attachments, uploads, exports, logs, backups, restore, translations, logo | Done 2026-10-23 |
-| 6 | Raw SQL: data lists, saved filters, search, KPI, schedule, pick lists | Next |
-| 7 | Staff roles: the authenticated routes without a role check in the action (list below) | |
+| 6 | Raw SQL: data lists, saved filters, search, KPI, schedule, pick lists | Done 2026-10-23 |
+| 7 | Staff roles: the authenticated routes without a role check in the action (list below) | Next |
 | 8 | Email and notifications (`direct-smtp`, templates, delivery) | |
 | 9 | Client (XSS, token storage, `npm audit` triage), CORS, headers, Swagger UI | |
 | 10 | Deployment: installers, Linux setup script, nginx, systemd, config and data permissions | |
@@ -190,7 +190,8 @@ Status: *candidate* (seen in code, not yet confirmed), *confirmed*, *fixed in ca
 | F1 | Critical (installs that kept it) | **confirmed** | Placeholder JWT secret in the LAN/IIS and Linux server config templates |
 | F19 | Critical | **confirmed** (import) | Any logged-in user, customer contacts included, can import report templates, whose JavaScript runs in the server's browser |
 | F21 | Critical | candidate | Download file names from the URL are not checked: `report/download` and `export/download` may serve files outside the temp folder on Windows |
-| F18 | Critical | candidate | Pick-list search text is put into SQL without escaping; any logged-in user can reach it |
+| F18 | Critical | confirmed (code) | Pick-list search text is put into SQL without escaping; any logged-in user can reach it |
+| F26 | Critical | confirmed (code) | KPI `interval` criterion is put into SQL unchecked; reachable by staff users down to TechRestricted |
 | F13 | High | **confirmed** | Customer contacts can open any customer's work order (customer view, customer report, attachments) by id |
 | F14 | High | **confirmed** | Customer contacts can read, and by code change, delete and create, any customer's service requests |
 | F15 | High | **confirmed** | Customer contacts can list every customer (and vendors, users, parts...) with any pick-list columns, phones and emails included |
@@ -412,7 +413,7 @@ its own email address, and receive staff notifications, possibly with reports at
 tested live because it sends real email. Fix: force `UserId` to the caller unless the caller
 has user-admin rights. Pass 8 to confirm how delivery checks rights.
 
-### F18 Pick-list search text in SQL (pass 3, candidate, code reading only)
+### F18 Pick-list search text in SQL (pass 3; code reading completed in pass 6)
 
 `PickListSqlBuilder.Build` puts the search text and the tag search text into `like` fragments
 by string interpolation, and no escaping was found on the way from
@@ -420,7 +421,7 @@ by string interpolation, and no escaping was found on the way from
 every logged-in user, customer contacts included. Data-list text filters escape quotes since
 case 4676; pick lists were not covered. Deliberately not tested live. Fix: pass the search
 texts as SQL parameters (preferred) or escape them as the data-list builder does; review the
-rest of the pick-list builder in pass 6.
+rest of the pick-list builder in pass 6. **Pass 6:** the search text (`autoCompleteQuery`) and the tag search text (`tagSpecificQuery`) are the only free-text inputs of the pick-list builder, and neither is escaped on any path; template fields map to column definitions, ids are `long`, variants are parsed as numbers or matched to fixed strings. Status: confirmed by code.
 
 
 ### F19 Report template import open to every login (pass 4, confirmed)
@@ -526,6 +527,45 @@ under content-hash names in the attachments folder; batch move, batch delete and
 check rights; data import (`POST import`) checks create rights per type; there is no restore
 route (restore is a manual `pg_restore`, see the manual).
 
+### F26 KPI `interval` in SQL (pass 6, from code)
+
+Six KPIs (`WorkOrderCreatedCount`, `WorkOrderStatusCount`, `WorkOrderStatusPct`,
+`WorkOrderCompletedOnTimePct`, `WorkOrderItemLaborQuantitySummary`,
+`WorkOrderItemLaborQuantityEveryoneSummary`) read `interval` from the request
+(`options.Criteria["interval"]`, a string) and put it into the query as
+`DATE_TRUNC('{interval}', ...)` with no check. `POST kpi` has no role check of its own;
+`KPIFetcher` checks the KPI's `AllowedRoles`, which for the labour summary are Tech and
+TechRestricted, so the least privileged staff roles reach it (customer roles do not). A value
+that leaves the quotes changes the query, which can read or change anything in the database
+(password hashes, roles, licence). Fix: accept only the values the client sends (`day`, `week`,
+`month`, `quarter`, `year`; check `raven-client` for the exact list) and reject anything else,
+in one helper used by all six. The other KPI criteria are safe: `timeSpan` goes through the
+data-list date builder (fixed tokens, otherwise `DateTime.Parse`), tags through the tag
+helper (escaped), `wostatus` and `userid` are integers.
+
+### Raw SQL: what was checked and found safe (pass 6)
+
+- **Data-list filters** (`DataListSqlFilterCriteriaBuilder`): column keys are looked up in the
+  list's field definitions (unknown keys throw); text, phone, email and URL values have quotes
+  doubled; numbers, roles and ids are parsed (case 4676); bools become `true`/`false`; dates
+  and relative-date tokens are parsed; durations are rebuilt from parsed integers; tags are
+  normalised and quote-doubled.
+- **Data-list sort and columns**: keys looked up in the field definitions; direction becomes
+  `ASC`/`DESC`.
+- **`ClientCriteria`**: every list that reads it (51) turns it into a typed filter option, so it
+  goes through the filter builder above; saved filters too.
+- **Search**: the word breaker keeps only letters, digits and `%`, so terms cannot contain a
+  quote. Safe by construction, but fragile: parameters would not depend on the tokenizer.
+- **Customer notification "who" query, schedule, attachments list, name fetcher, unit and
+  customer queries, accounting list, server metrics, job and sweeper queries**: typed values
+  or fixed SQL; schedule uses EF.
+- `DbUtil` and `AySchema` build SQL from configuration (database name), not from requests.
+
+Hardening, not a finding: hand-built SQL with string escaping is spread over the builders.
+Moving free-text values to `NpgsqlParameter`s (pick lists, data-list text filters, search,
+tags) would remove the whole class of bug; at least F18 and F26 should use parameters or an
+allowlist rather than escaping.
+
 ### F2 Windows folder permissions (from the 8.3.0 installer test)
 
 `lan.iss` and `standalone.iss` set `Permissions: users-modify` on `{app}` (the program
@@ -601,3 +641,4 @@ roles that should not see the schedule (pass 7).
 | 2026-10-23 | 3 Customer contacts | Mapped the `Customer` role in `BizRoles.cs` and every customer-reachable route. Tested on the local dev server as seeded contact `Customer` (customer 1), read-only: F13, F14 (read), F15, F16 confirmed. F17 and F18 from code (F17 not tested because it sends email; F18 not tested by choice). OK: data lists (server sets the customer filter), KPIs (no customer roles), saved filters and notification subscription reads (scoped to the user), memos (no customer access) |
 | 2026-10-23 | 4 Report rendering | Read `ReportController`, `ReportBiz` (render path, data, browser launch), `ay-report.js` helpers, `CoreJobCustomerNotify` and `AuthController.GenRpt`. Tested on the local dev server: **F19 confirmed** (contact imported a stock template copy, deleted afterwards). F20, F21 and F22 from code; rendering of a crafted template and the download path were deliberately not tested. OK: `report/data` and `render-job` check read rights on the record type; template API calls use the renderer's own token (except notification reports, F20); report output files have random names. Pass 5 picks up F21's other routes |
 | 2026-10-23 | 5 File handling | Read `ApiUploadProcessor`, `AttachmentController` (upload, list, download, move, delete, maintenance), `LogoController`, `ExportController`, `TranslationController` upload, `LogFilesController`, `BackupController`, `ImportController`, and the path helpers in `FileUtil`. No live tests (the role gaps have the same shape as F19, already confirmed). New: F23, F24, F25; F21 extended to the log and backup download routes (ops roles), backup delete found safe |
+| 2026-10-23 | 6 Raw SQL | Code reading only. Listed every hand-built query (`CommandText`, `*SqlRaw`, interpolated SQL) and traced each request value into it: data-list filter, sort and select builders, all 51 `ClientCriteria` users, saved filters, search tokenizer, every KPI's criteria, pick-list builder and variants, customer notification query, controllers and biz classes with raw SQL. New: **F26** (KPI `interval`). F18 completed by code. Everything else safe (see "Raw SQL: what was checked") |