diff --git a/devdocs/modernization/SECURITY-REVIEW.md b/devdocs/modernization/SECURITY-REVIEW.md index de9e3dd1..dfd696bf 100644 --- a/devdocs/modernization/SECURITY-REVIEW.md +++ b/devdocs/modernization/SECURITY-REVIEW.md @@ -18,8 +18,8 @@ keep it that way while findings are open. | 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) | 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 | | +| 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 | | | 11 | Sockeye public routes (licence fetch, trial requests) | | @@ -196,19 +196,22 @@ Status: *candidate* (seen in code, not yet confirmed), *confirmed*, *fixed in ca | 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 | +| F17 | Medium | confirmed (code) | Notification subscriptions accept any `UserId`, and object events reach subscribers without a rights check: record names to any email address | | 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 | +| 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 | | 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 | | F7 | Low | confirmed | Password change or reset does not end existing sessions, and makes the download key non-expiring | | F8 | Low | confirmed | Latest-token check skipped for any URL ending in `/auth` or `notify/hello` | -| F4 | Low | candidate | Any logged-in user can send an in-app notification to any user | +| F4 | Low | confirmed (code) | Any logged-in user can send an in-app notification to any user | | F10 | Low | confirmed | Anyone can record acceptance of the licence agreement (`license/lc`) | | 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 | @@ -414,7 +417,7 @@ has its own `DeliveryAddress` and can attach a report (`LinkReportId`). So, by c 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. +has user-admin rights. Pass 8 to confirm how delivery checks rights. **Pass 8 (code):** confirmed, and it goes further: `NotifyEventHelper` matches object events (created, modified, deleted, age) to subscriptions by record type only, without checking that the subscriber may read that type, and `CoreJobNotify` sends to the subscription's `DeliveryAddress`. A user notification email carries the record type, its name and a link (no data, no report: report attachments exist only on customer notification subscriptions, which staff create). So a contact, or a staff user without rights on a type, can get a running feed of record names (new customers, work orders, ...) at any address. Re-rated Medium. Fix as above, plus a read-rights check on the subscriber when an object event is matched. ### F18 Pick-list search text in SQL (pass 3; code reading completed in pass 6) @@ -623,6 +626,39 @@ settings only). Already covered elsewhere: `job-operations/*` (F22), `schedule/* `workorder/customer/{id}` (F13), `report/upload` (F19), `translation/upload` (F24), `notify/direct-smtp` and `direct-message` (F3, F4). +### F30 SMTP certificate not checked (pass 8, from code) + +`Mailer.SendEmailAsync` sets `client.ServerCertificateValidationCallback = (s, c, h, e) => true`, +so any certificate is accepted. Anyone who can intercept the connection (a hostile network, +DNS) can pose as the mail server and collect the SMTP account and password and every message. +Also, with security set to none, MailKit's `ConnectAsync(host, port, false)` uses STARTTLS only +if offered, which an attacker can suppress. Fix: remove the callback (validate normally); if a +customer needs a self-signed mail server, make that an explicit setting. + +### F31 SMTP password returned to read-only roles (pass 8, from code) + +`GET global-ops-notification-setting` returns the whole settings object, `SmtpPassword` included, +to every role with read rights on `OpsNotificationSettings`: OpsAdminRestricted, BizAdmin and +BizAdminRestricted, while only OpsAdmin may change it. Those users can then use the company's +mail account directly. Fix: never send the password to the client (return a "set" flag; keep the +stored value when the PUT leaves it empty). + +### F32 SMTP credentials in source (pass 8) + +`models/GlobalOpsNotificationSettings.cs`, in the `#if (DEBUG)` constructor, sets a FastMail SMTP +server, account and password; the same file is in `sockeye/server/models`. Release builds use +placeholders, so shipped software is not affected, but the credentials are in both repositories +and their history (contrary to the "secrets never go in a repo" rule), and debug builds send real +email through that account (which explains report 81's real bounces in testing). Fix: rotate that +FastMail app password now (John); remove the values from both files and read debug SMTP settings +from an environment variable or user secrets. History need not be rewritten once the password is +rotated. + +OK in pass 8: SMTP settings change and the test-send route are OpsAdmin only; batch direct SMTP +(`job-operations/batch-direct-smtp`) checks modify rights on the record type and sends only to the +records' own addresses; notification emails are plain text built from record names (no HTML +injection); the customer notification "who" query escapes tags. + ### 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,13 +678,13 @@ Any logged-in user, customer contacts included, can send an email with any recip address. On a hosted or internet-facing server one customer-contact login is enough to send phishing or spam under the company's name. To confirm in pass 8: which callers the client actually has, and the narrowest check that keeps them working (staff roles only; or -recipient restricted to the object's own address). +recipient restricted to the object's own address). **Pass 8 (code):** confirmed. When `ToAddress` is given it is used as is (only when it is empty is the address taken from the Customer, HeadOffice, Vendor or User record); subject, text and HTML body come from the request; one recipient per call (`MailboxAddress.Parse`), no rate limit. The event log records each send. Fix: drop `ToAddress` from the route and always take the address from the record (the Customer form's use), check read rights on that record, and refuse customer-type users; API users who email rendered reports keep working through a record address. ### F4 `notify/direct-message` open to every logged-in user Any logged-in user can add a general notification, with the sender's name and any text, for any user id. Low on its own (in-app only); relevant with F3 for social engineering of staff -by a customer contact. Pass 8. +by a customer contact. Pass 8: confirmed from code (`NotifyController.SendNotifyDirectMessage`, no role check, any `ToUserId`). Fix: staff only, or only to users the caller may see. ## Customer contact route allowlist (draft, for the F13 to F17 fix) @@ -700,3 +736,4 @@ roles that should not see the schedule (pass 7). | 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") | +| 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** |