From 52763366c2c690a12696b895535b80ab770cadc3 Mon Sep 17 00:00:00 2001 From: John Cardinal Date: Fri, 23 Oct 2026 20:33:26 -0700 Subject: [PATCH] Security review pass 9: client, headers, CORS, Swagger From code: some record text is rendered as HTML without escaping (F33); shipped DOMPurify and Vuetify are behind on security fixes (F34, needs a decision on front-end packages); headers, CORS, Swagger and keys in URLs (F35). npm audit run read-only and triaged. Review only, no code or dependency changes. Co-Authored-By: Claude Opus 5.5 --- devdocs/modernization/SECURITY-REVIEW.md | 57 ++++++++++++++++++++++-- 1 file changed, 54 insertions(+), 3 deletions(-) diff --git a/devdocs/modernization/SECURITY-REVIEW.md b/devdocs/modernization/SECURITY-REVIEW.md index dfd696bf..99964fef 100644 --- a/devdocs/modernization/SECURITY-REVIEW.md +++ b/devdocs/modernization/SECURITY-REVIEW.md @@ -19,8 +19,8 @@ keep it that way while findings are open. | 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) | Done 2026-10-23 | | 8 | Email and notifications (`direct-smtp`, templates, delivery) | Done 2026-10-23 | -| 9 | Client (XSS, token storage, `npm audit` triage), CORS, headers, Swagger UI | Next | -| 10 | Deployment: installers, Linux setup script, nginx, systemd, config and data permissions | | +| 9 | Client (XSS, token storage, `npm audit` triage), CORS, headers, Swagger UI | Done 2026-10-23 | +| 10 | Deployment: installers, Linux setup script, nginx, systemd, config and data permissions | Next | | 11 | Sockeye public routes (licence fetch, trial requests) | | The order is by risk to internet-facing servers. Findings from any pass go in **Findings**. @@ -200,12 +200,14 @@ Status: *candidate* (seen in code, not yet confirmed), *confirmed*, *fixed in ca | F20 | High | candidate | Report templates receive the renderer's token (SuperUser for customer notification reports) and the report browser can reach any network address | | F27 | High | **confirmed** | Restricted roles (TechRestricted, SubContractor, SubContractorRestricted) get whole work-order items through report data, rendering and export | | F28 | High | candidate | Users with change rights on Customer (Service, Sales, Accounting) can give customer contact logins any role, BizAdmin and OpsAdmin included | +| F33 | High | confirmed (code) | Client renders some record text as HTML without escaping (search results, work-order address dialog, unit warranty info, dialogs) | | F3 | High | confirmed (code) | Any logged-in user, customer contacts included, can send arbitrary email through the company's SMTP server | | F23 | Medium | confirmed (code) | Logo upload accepts any content type and the anonymous logo route serves it inline from the app's origin (stored XSS) | | F24 | Medium | confirmed (code) | Translation import has no role check: any login can add translations | | F30 | Medium | confirmed (code) | SMTP: the mail server's TLS certificate is never checked | | F31 | Medium | confirmed (code) | The SMTP password is returned to read-only roles (OpsAdminRestricted, BizAdmin, BizAdminRestricted) | | F32 | Medium | confirmed (code) | A real-looking SMTP account and password are in the DEBUG defaults in `raven` and `sockeye` source | +| F34 | Medium | confirmed | Shipped client libraries behind on security fixes: DOMPurify 2.3.8 (wiki sanitiser), Vuetify 2.6.6 | | F6 | Medium | confirmed | No lockout or rate limit on login or the 2FA step; no password rules | | F9 | Medium | confirmed | Default SuperUser password, and an anonymous route that says whether it is still set | | F2 | Medium | candidate | Windows installers give all local users modify rights on the program and data folders | @@ -217,6 +219,7 @@ Status: *candidate* (seen in code, not yet confirmed), *confirmed*, *fixed in ca | F12 | Low | confirmed | Password hashing: PBKDF2-SHA512 with 10,000 iterations | | F25 | Low | confirmed (code) | Attachment and export gaps: upload stored before the rights check, "attached by" can be set to anyone, exports not tied to their user | | F29 | Low | confirmed (code) | `server-info` gives every login, customer contacts included, the server DB ID and full licence details | +| F35 | Low | confirmed | No security headers from the server itself (self-hosted), no CSP anywhere, CORS any origin, public Swagger UI, download keys in URLs | | F22 | Low | confirmed | Minor report and job route gaps: template export by any download key, job status, logs and cancel by GUID without an owner check, browser without sandbox | **Release decisions.** John, 2026-10-23: **F13 to F17 are urgent and are fixed in 8.3.0**, as @@ -390,7 +393,7 @@ CSR `Change` includes `Customer`, and `HasReadFullRole` counts `Change` as read. `CustomerServiceRequestBiz` never checks `CustomerId` against the contact (create, get, put, delete). **Proof**: the contact read CSR 1 of customer 16 (200, with its text). Change, delete and create for another customer were not tested (they write data) but follow from the same -code. Search also returns CSRs of all customers (it filters by role only). +code. Search also returns CSRs of all customers (it filters by role only). Pass 9: `unit/customerunit` (`UnitBiz.CustomerCreateAsync`) also takes `CustomerId` from the request, so a contact can add units to any customer; same fix (force the contact's customer). ### F15 Pick lists expose the whole customer list (pass 3, confirmed) @@ -659,6 +662,53 @@ OK in pass 8: SMTP settings change and the test-send route are OpsAdmin only; ba records' own addresses; notification emails are plain text built from record names (no HTML injection); the customer notification "who" query escapes tags. +### F33 Unescaped record text rendered as HTML in the client (pass 9, from code) + +Of the 13 `v-html` uses in `raven-client`, these render text that users enter, without +escaping it first: +- `views/home-search.vue`: the search result excerpt (`item.info`, from `Search.GetInfoAsync`, + which does not encode it), with the search terms highlighted. Searchable text includes + fields customer contacts can write (service requests, units). +- `components/work-order-address.vue`: addresses built by `formatAddress` from customer, head + office and unit address fields. +- `components/work-order-item-units.vue`, `quote-item-units.vue`, `pm-item-units.vue`: the + warranty summary (vendor name, receipt number, work order list). +- `components/gzconfirm.vue`: dialog messages, which are translation strings; with F24 any login + can import a translation. + +The session token is kept in `localStorage` (`vuex-persistedstate`), so script running in the +page could act as the signed-in user. Fix: escape these values (an `escapeHtml` helper, or +build the markup with Vue templates instead of strings); encode the excerpt on the server too; +render translation strings as text. Safe: the wiki (`marked` then DOMPurify, see F34), the +calendar summaries (Vuetify escapes event names), and the remaining sinks (fixed strings). +A Content-Security-Policy (F35) would be a second layer. + +### F34 Client libraries behind on security fixes (pass 9) + +`npm audit` (read-only, lockfile) reports 550 advisories, almost all in build tools (webpack, +Vue CLI, Babel, ESLint) that run only on the build machine and do not ship. Of the packages +the app ships: **DOMPurify 2.3.8**, the wiki sanitiser, has sanitiser-bypass fixes in 2.4.x and +2.5.x; **Vuetify 2.6.6** has XSS fixes in 2.6.10 (the last 2.x is 2.7.x); `marked` 1.2.9, `luxon` +1.28.0 and `vue` 2.6.14 have only slow-regex (browser-side) advisories. Fix: raise DOMPurify to +the latest 2.5.x and Vuetify to the latest 2.7.x, both on the same major version. This is a +front-end dependency change, which CLAUDE.md rules out without John's decision; check that +both still build with Node 12.22.9. + +### F35 Headers, CORS, Swagger, keys in URLs (pass 9) + +- The server sets no security headers. Hosted droplets get HSTS, `nosniff`, `X-Frame-Options` + and `Referrer-Policy` from the nginx template in the setup script; self-hosted IIS, Linux and + Windows installs get none. No deployment sets a Content-Security-Policy. +- CORS allows any origin, method and header. Impact is small because the token travels in the + `Authorization` header (not a cookie), so another site cannot use a visitor's session. +- Swagger UI and the full API description are public at `/api-docs`. +- Download keys (`?t=`) are in URLs, so they reach proxy access logs and browser history; + they last 5 days (F7 makes some last longer). + +Fix: add the headers in the app (so every deployment gets them), with a CSP that allows the +client's own scripts; limit CORS to the server's own origin unless API users need more; +consider Swagger UI for logged-in users only. + ### F2 Windows folder permissions (from the 8.3.0 installer test) `lan.iss` and `standalone.iss` set `Permissions: users-modify` on `{app}` (the program @@ -737,3 +787,4 @@ roles that should not see the schedule (pass 7). | 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") | | 2026-10-23 | 7 Staff roles | Went through the remaining logged-in routes without a role check, the restricted-role filtering in `WorkOrderBiz`, and user and contact administration in `UserBiz`. Tested on the local dev server, read-only: **F27 confirmed** (`SubContractorRestricted` through `report/data`). F28 from code (live test not done). New: F29. The rest of the list found OK (see "Pass 7: routes ... found OK") | | 2026-10-23 | 8 Email and notifications | Code reading only, nothing sent. Read `NotifyController` (direct SMTP and message), `Mailer`, `GlobalOpsNotificationSettings` controller, biz and model, `NotifyEventHelper`, `CoreJobNotify`, `CoreJobCustomerNotify`, `NotifySubscriptionBiz`. F3 and F4 confirmed by code; F17 confirmed and re-rated Medium (names and links, no reports). New: F30, F31, F32. **Action for John: rotate the FastMail app password in F32** | +| 2026-10-23 | 9 Client, headers, CORS, Swagger | Code reading of every `v-html` use in `raven-client` and its data source; `npm audit` read-only against the lockfile (no install, no change), triaged to shipped packages; server and nginx headers, CORS, Swagger. New: F33, F34 (needs John's decision on two front-end packages), F35; F14 extended (`unit/customerunit`) |