diff --git a/devdocs/modernization/SECURITY-REVIEW.md b/devdocs/modernization/SECURITY-REVIEW.md index 699d662d..de9e3dd1 100644 --- a/devdocs/modernization/SECURITY-REVIEW.md +++ b/devdocs/modernization/SECURITY-REVIEW.md @@ -17,8 +17,8 @@ keep it that way while findings are open. | 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 | 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) | | +| 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) | Next | | 9 | Client (XSS, token storage, `npm audit` triage), CORS, headers, Swagger UI | | | 10 | Deployment: installers, Linux setup script, nginx, systemd, config and data permissions | | | 11 | Sockeye public routes (licence fetch, trial requests) | | @@ -198,6 +198,8 @@ Status: *candidate* (seen in code, not yet confirmed), *confirmed*, *fixed in ca | F16 | High | **confirmed** | `schedule/svc` and `schedule/user` have no role check: anyone logged in sees the whole service schedule | | F17 | High | candidate | Notification subscriptions accept any `UserId`: a contact could subscribe on a staff user's behalf with delivery to its own email | | 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 | | F3 | High | candidate | 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 | @@ -211,6 +213,7 @@ Status: *candidate* (seen in code, not yet confirmed), *confirmed*, *fixed in ca | F11 | Low | confirmed | API root page shows licensee, version, licence terms (and DB ID when unlicensed) to anyone | | 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 | | 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 @@ -566,6 +569,60 @@ Moving free-text values to `NpgsqlParameter`s (pick lists, data-list text filter tags) would remove the whole class of bug; at least F18 and F26 should use parameters or an allowlist rather than escaping. +### F27 Restricted roles bypass work-order item filtering (pass 7, confirmed) + +`WorkOrderBiz.WorkOrderGetAsync` removes, for TechRestricted, SubContractor and +SubContractorRestricted users, every item they are not scheduled on, other users' labour, +travel and scheduling, and (by role) parts, units, expenses, loans and outside services. +`GetReportData` (also used by `GetExportData`) calls `WorkOrderGetPartialAsync`, which applies +that filtering only when the requested type is `WorkOrder`; for the child types (item, +labour, expense, part, ...) it loads the parent header and the whole item, with every +descendant when `IncludeWoItemDescendants` is set by the caller, and filters nothing. These +roles have change (so read) rights on the child types they work with, so `report/data`, +`report/render-job` and `export/render` accept them. **Proof**: on work order 1, seeded +`SubContractorRestricted` got 0 items from `workorder/1` (correct, not scheduled), and from +`report/data` with a labour id and descendants on got the item with another user's labour, +the scheduling and 2 parts (a role that should see no parts at all). Fix: apply the same +restricted-user filtering in `WorkOrderGetPartialAsync` (one shared method used by both +paths), and refuse child-type report and export requests for items the user is not scheduled +on. Check `QuoteBiz` and `PMBiz` for the same split (they also reference restricted users). + +### F28 Contact logins with staff roles (pass 7, candidate, code reading) + +`UserBiz` lets anyone with modify (create) rights on `Customer` create and edit customer contact +users (`IsOutsideCustomerContactTypeUser`), and `BizRoles` gives `Customer` change rights to +Service, Sales and Accounting. Changing a user between inside and outside types needs rights +on both User and Customer (`UserBiz.cs:803`), but nothing limits the **roles** a contact user may +hold: `ValidateAsync` checks only that the roles value is valid. Authorisation is by role +everywhere, so a contact with BizAdmin or OpsAdmin roles is an administrator. A Sales, Service +or Accounting user could therefore create a contact login with administrator roles and a +password of their choosing. Not tested live. The F13 to F17 allowlist would limit what a +customer-type login can call, but the roles must still be constrained. Fix: in +`UserBiz.ValidateAsync`, allow only `Customer` and `CustomerRestricted` roles on Customer and +HeadOffice users (and only inside roles on inside users); reject anything else on create and +update. + +### F29 `server-info` to every login (pass 7) + +`GET server-info` has no role check and returns the version, schema, server time zone, the +server DB ID and the licence as JSON, to any login including customer contacts. The about +page needs some of it; contacts need little or none. Whether the DB ID matters to Sockeye +(licence fetch, trial requests) is checked in pass 11. Fix: return the DB ID and licence only +to staff, or only to ops and admin roles. + +### Pass 7: routes without a role check that were found OK + +`DELETE notify/{id}` (owner checked), `notify/new-count`, `notify/app-notifications`, +`notify-subscription/list` (the caller's own), data-list column views (validation refuses +another user's id), saved filters (own or public), `user-option/{id}` (own unless admin), +`auth/change-password`, `auth/totp*` (own), `POST kpi` (each KPI checks its own roles; but see +F26), `POST search` (results filtered by read rights per type), `GET user/amsu` (returns only +whether the caller is SuperUser), enum lists, form field references, translations (read), +tag lists, role lists, build mode, server state, `global-biz-setting/client` (display +settings only). Already covered elsewhere: `job-operations/*` (F22), `schedule/*` (F16), +`workorder/customer/{id}` (F13), `report/upload` (F19), `translation/upload` (F24), +`notify/direct-smtp` and `direct-message` (F3, F4). + ### F2 Windows folder permissions (from the 8.3.0 installer test) `lan.iss` and `standalone.iss` set `Permissions: users-modify` on `{app}` (the program @@ -642,3 +699,4 @@ roles that should not see the schedule (pass 7). | 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") | +| 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") |