Security review pass 5: file handling

From code: logo upload accepts any content type and is served inline from the
app's origin (F23); translation import has no role check (F24); attachment and
export gaps (F25). F21 extended to the log and backup download routes; backup
delete found safe. Review only, no code changes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

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 |