admin/raven
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Security review pass 7: staff roles

Browse Source 
F27 (high, confirmed on the local server): restricted roles get whole work-order
items through report data, rendering and export. From code: users with change
rights on Customer can give contact logins any role (F28); server-info gives
the DB ID and licence to every login (F29). Remaining routes without a role
check found OK. Review only, no code changes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
John Cardinal
2026-10-23 20:23:42 -07:00
parent 8642d6e627
commit f90dfe98c7
1 changed files with 60 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files

62
devdocs/modernization/SECURITY-REVIEW.md
View File

@@ -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") |