diff --git a/devdocs/modernization/SECURITY-REVIEW.md b/devdocs/modernization/SECURITY-REVIEW.md index 03e6db51..f255bfc8 100644 --- a/devdocs/modernization/SECURITY-REVIEW.md +++ b/devdocs/modernization/SECURITY-REVIEW.md @@ -15,8 +15,8 @@ keep it that way while findings are open. | 2 | Unauthenticated routes, login, tokens, password reset, 2FA | Done 2026-10-23 | | 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) | Done 2026-10-23 | -| 5 | File handling: attachments, uploads, exports, logs, backups, restore, translations, logo | Next | -| 6 | Raw SQL: data lists, saved filters, search, KPI, schedule, pick lists | | +| 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 | Next | | 7 | Staff roles: the authenticated routes without a role check in the action (list below) | | | 8 | Email and notifications (`direct-smtp`, templates, delivery) | | | 9 | Client (XSS, token storage, `npm audit` triage), CORS, headers, Swagger UI | | @@ -198,6 +198,8 @@ Status: *candidate* (seen in code, not yet confirmed), *confirmed*, *fixed in ca | 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 | | 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 | | 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 | @@ -207,6 +209,7 @@ Status: *candidate* (seen in code, not yet confirmed), *confirmed*, *fixed in ca | 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 | +| 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 | | 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 @@ -467,7 +470,7 @@ name replaces the folder entirely; the result is passed to `PhysicalFile`. Any l `config.json` (database password, JWT secret). Linux is likely not affected (`/` stays encoded as `%2F` in route values); IIS request filtering may block some forms. Not tested live by choice. The same unchecked pattern is in `log-file/{logname}`, `log-file/download/{logname}` -and the backup download and delete routes (ops roles; pass 5). Fix: accept only a bare file +and the backup download route (ops roles; pass 5 confirmed: `LogFilesController` `GetLog` and `DownloadLog` need `LogFile` read, i.e. OpsAdmin or OpsAdminRestricted; `BackupController.DownloadAsync` needs `Backup` change, i.e. OpsAdmin; backup *delete* is safe, `FileUtil.EraseBackupFile` already strips directories with `Path.GetFileName`, the pattern the other routes should share). Fix: accept only a bare file name (`Path.GetFileName(name) == name`, no `..`), and check the resolved full path is inside the folder, in one helper used by every such route. @@ -481,6 +484,48 @@ browser bug reached through a template is not contained; Linux runs as `www-data as the logged-in user. Fix: owner check on job routes; consider enabling the sandbox where the platform allows. +### F23 Logo upload: any content type, served inline (pass 5, from code) + +`LogoController.UploadAsync` stores the bytes and the content type the browser sent +(`file.ContentType`), with no check of either; `DownloadLogo` (anonymous, `GET logo/{size}`) +returns them inline with that type from the AyaNova origin. A file uploaded as HTML (or SVG +with script) therefore runs as a page of the AyaNova site when someone opens its URL, and the +client keeps its session in `localStorage` (`vuex-persistedstate`, key `AyaNova`), which such a +page can read. Upload and delete check `HasReadFullRole(Global)`: BizAdmin, and +BizAdminRestricted, whose rights on Global are meant to be read-only. Fix: require +`HasModifyRole(Global)`; accept only PNG, JPEG, GIF, WebP (check the bytes, not the header; +no SVG); serve with `X-Content-Type-Options: nosniff` and a restrictive +`Content-Security-Policy` (pass 9 looks at headers generally). + +### F24 Translation import without a role check (pass 5, from code) + +`TranslationController.UploadAsync` (`POST translation/upload`) and `TranslationBiz.ImportAsync` +check no role, the same shape as F19. Any login, customer contacts included, can add +translations (up to 15 MiB each; imported as non-stock under a unique name). They affect +only users who select them, unless an administrator makes one the default; whether +translated text is ever rendered as HTML in the client is a pass 9 question. Fix: require +`HasCreateRole(Translation)`, like the duplicate route. + +### F25 Attachment and export gaps (pass 5, from code) + +- `AttachmentController.UploadAsync` streams the whole upload to disk (limit 10 GiB) before + it checks rights on the target record, then deletes it on 403: any login can make the + server write large files repeatedly. Check rights before reading the body, or lower the + limit for users without rights. +- The form field `OverrideUserId` is accepted from anyone and recorded as "attached by": + limit it to the import path that needs it, or to SuperUser. +- `export/download/{fileName}` accepts any user's download key; the name is + `{type}-{yyyyMMddHHmmssfff}.json` and files are deleted after about 15 minutes, so guessing + another user's export is impractical, but the route should still check that the caller + made it. +- Customer contacts can attach files to any customer's service request (part of F14; fixed + by the same ownership check). + +OK in this pass: attachments are served as downloads (`Content-Disposition: attachment`), stored +under content-hash names in the attachments folder; batch move, batch delete and maintenance +check rights; data import (`POST import`) checks create rights per type; there is no restore +route (restore is a manual `pg_restore`, see the manual). + ### F2 Windows folder permissions (from the 8.3.0 installer test) `lan.iss` and `standalone.iss` set `Permissions: users-modify` on `{app}` (the program @@ -555,3 +600,4 @@ roles that should not see the schedule (pass 7). | 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) | | 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 |