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

Security review pass 8: email and notifications

Browse Source 
From code: F3 and F4 confirmed; F17 confirmed and re-rated medium (names and
links reach any address, no reports). New: SMTP certificate never checked (F30),
SMTP password returned to read-only roles (F31), SMTP credentials in the DEBUG
defaults of raven and sockeye source (F32; password to be rotated). Review only,
nothing sent.

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

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

@@ -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** |