diff --git a/devdocs/modernization/SECURITY-REVIEW.md b/devdocs/modernization/SECURITY-REVIEW.md index c36b8757..e768f4bc 100644 --- a/devdocs/modernization/SECURITY-REVIEW.md +++ b/devdocs/modernization/SECURITY-REVIEW.md @@ -13,8 +13,8 @@ keep it that way while findings are open. |---|---|---| | 1 | Threat model and attack-surface map | Done 2026-10-23 | | 2 | Unauthenticated routes, login, tokens, password reset, 2FA | Done 2026-10-23 | -| 3 | Customer contact users: what they can reach and read | Next | -| 4 | Report rendering (templates, the server-side browser, report data and download routes) | | +| 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) | Next | | 5 | File handling: attachments, uploads, exports, logs, backups, restore, translations, logo | | | 6 | Raw SQL: data lists, saved filters, search, KPI, schedule, pick lists | | | 7 | Staff roles: the authenticated routes without a role check in the action (list below) | | @@ -188,6 +188,12 @@ Status: *candidate* (seen in code, not yet confirmed), *confirmed*, *fixed in ca |---|---|---|---| | F5 | **Critical** | **confirmed** | 2FA login step accepts an empty or missing temp token: the password is skipped for users with 2FA on | | F1 | Critical (installs that kept it) | **confirmed** | Placeholder JWT secret in the LAN/IIS and Linux server config templates | +| F18 | Critical | candidate | Pick-list search text is put into SQL without escaping; any logged-in user can reach it | +| 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 | +| 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 | | F3 | High | candidate | Any logged-in user, customer contacts included, can send arbitrary email through the company's SMTP server | | 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 | @@ -315,6 +321,94 @@ PBKDF2-HMAC-SHA512, 10,000 iterations, 32-byte random salt (`Hasher.cs`). Curren (OWASP) is 210,000 iterations for SHA-512. Only matters if the database or a backup leaks. Raising it needs re-hashing at next login (store the iteration count). Roadmap item, not 8.3.0. +### Pass 3 overview: customer contact isolation + +Customer and head-office contacts are users with `UserType` Customer/HeadOffice and the +`Customer` role (seeded contacts: `AuthorizationRoles.Customer`). Isolation to their own +customer is enforced **only in data lists**: `DataListController` allows them two lists +(`CustomerServiceRequestDataList`, `CustomerWorkOrderDataList`) and sets the customer +filter itself. Everywhere else the checks are role sets designed for staff, and +`AuthorizationRoles.All` includes `Customer` and `CustomerRestricted`: `Select` is `All` on +almost every type (Customer, User, Vendor, Part, Unit, WorkOrder…), `ReadFullRecord` is `All` +on Report, TaskGroup, QuoteStatus, and the `Customer` role can change any +CustomerServiceRequest. The customer-specific rights helper +(`UserBiz.CustomerUserEffectiveRightsAsync`) answers "is this feature on for this contact +and these tags", never "does this record belong to the contact's customer". + +All proofs below: local dev server, seeded contact `Customer` (customer id 1), read-only +requests; seeded data is fake. + +**Fix direction** (one case, possibly in steps): deny customer-type users by default, with an +allowlist of the routes the customer UI actually calls (to be listed from the client), as a +filter or in the token middleware, where `AY_USER_TYPE` is already set; then add the +ownership check in each allowed route that takes an id (work order customer view, customer +report, attachments, CSR). This also covers routes not yet reviewed. + +### F13 Any customer's work order by id (pass 3, confirmed) + +`GET workorder/customer/{id}` (`WorkOrderBiz.WorkOrderGetForCustomerAsync`) loads the work +order by id and never compares its `CustomerId` with the contact's. It returns serial, service +date, customer name, customer reference number, contact name, invoice number, and the wiki +when tag rules allow it. **Proof**: the contact of customer 1 read work order 1 of customer 12 +(200, with that customer's name). A staff user calling it gets a 500 (the helper throws for +non-customer users). The same tag-only rights, without ownership, also gate (code reading): + +- the customer work-order report (`ReportBiz`, report id `-100`): takes `SelectedRowIds[0]`, + gets the effective report from that work order's tags, then renders with the role check + skipped (`RequestIsCustomerWorkOrderReport`), so a contact can render the configured + customer report for any work order (pass 4 to confirm); +- work-order attachments (`attachment/list`, `attachment/download/{id}` for customer users): + `ThisWOCanAttachments` from tags only. + +These need the customer work-order features turned on in global settings (common where the +customer portal is used); the customer view route does not, it always answers. + +### F14 Customer service requests across customers (pass 3, confirmed for reading) + +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). + +### F15 Pick lists expose the whole customer list (pass 3, confirmed) + +`POST pick-list/list` checks only `Select`, which the `Customer` role has for customers, head +offices, vendors, users, parts, units, contracts, projects, rates and more. The caller also +sends its own `template`, which chooses the returned columns. **Proof**: the contact got all +70 seeded customers with phone and email. `pick-list/single` works the same way. A +customer's contact can therefore download the company's customer list with contact details. +Fix: no `Select` for customer roles on these types (keep what the customer UI needs, likely +units of its own customer) and ignore `template` from customer-type users. + +### F16 Service schedule open to everyone logged in (pass 3, confirmed) + +`POST schedule/svc` and `POST schedule/user` have no role check. **Proof**: the contact got +1,257 schedule items, each "work order number + customer name" with times and the assigned +technicians. Fix: require read rights on `WorkOrderItemScheduledUser` (or `WorkOrder`) and +refuse customer-type users. (`schedule/personal` is the caller's own schedule.) + +### F17 Notification subscription for someone else (pass 3, candidate) + +`NotifySubscriptionBiz.CreateAsync` and `PutAsync` keep the `UserId` sent by the client, and +validation checks the event type against *that* user's type, not the caller's. A subscription +has its own `DeliveryAddress` and can attach a report (`LinkReportId`). So, by code, a +customer contact could create a subscription with a staff user's id, a staff event type and +its own email address, and receive staff notifications, possibly with reports attached. Not +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) + +`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 +`PickListController.PostList` through `PickListBiz.GetPickListAsync`. The route is open to +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. + + ### F2 Windows folder permissions (from the 8.3.0 installer test) `lan.iss` and `standalone.iss` set `Permissions: users-modify` on `{app}` (the program @@ -348,3 +442,4 @@ by a customer contact. Pass 8. |---|---|---| | 2026-10-23 | 1 Threat model and map | Scope agreed with John. Middleware, JWT validation, role model, download keys, CORS and Swagger read. Route map generated (460 actions; script kept in `security/routemap.py`). No route lacks authentication by accident. 53 logged-in routes have no role check in the action: 3 resolved (erase data, trial seed: OK; direct SMTP: F3), the rest assigned to later passes. Local-request exception not reachable through the hosted nginx. Findings F1 to F4 recorded as candidates | | 2026-10-23 | 2 Login, tokens, reset, 2FA, anonymous routes | Read `AuthController`, the token middleware, `Hasher`, download-key check, password reset, TOTP setup, and every anonymous route (`notify/hello`, `license/lc`, `translation/subset`, `logo`, API root, health). Tested on the local dev server with the seeded `Inventory` user: **F5 critical, confirmed** (2FA step without password); **F1 confirmed** (current token rebuilt from the secret in 6 tries); F8 confirmed. New: F6, F7, F9 to F12. OK: reset codes (32 random characters, 48 h, cleared on use, same reply for unknown codes), login replies do not reveal whether a login exists, roles read from the database on every request, deactivation ends the session, TOTP secret 160 bits. Leads for pass 4: `AuthController.GenRpt` issues SuperUser (id 1) tokens with an `rpl` claim for report rendering; pass 5: the logo is served with the content type given at upload | +| 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) |