From 8642d6e6273563b2e87ce608d9f8c003fe19be53 Mon Sep 17 00:00:00 2001 From: John Cardinal Date: Fri, 23 Oct 2026 20:17:02 -0700 Subject: [PATCH] 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 --- devdocs/modernization/SECURITY-REVIEW.md | 51 +++++++++++++++++++++--- 1 file changed, 46 insertions(+), 5 deletions(-) diff --git a/devdocs/modernization/SECURITY-REVIEW.md b/devdocs/modernization/SECURITY-REVIEW.md index f255bfc8..699d662d 100644 --- a/devdocs/modernization/SECURITY-REVIEW.md +++ b/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") |