Security review pass 1: scope, threat model, attack-surface map, first candidate findings

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

CLAUDE.md

@@ -93,6 +93,7 @@ dotnet publish server\AyaNova\AyaNova.csproj -c Release -r linux-x64 --no-self-c
 |---|---|
 | `devdocs/modernization/NET10-PORT.md` | The current port: plan, checklist, open questions, log |
 | `devdocs/modernization/ROADMAP.md` | Modernization steps and their status |
+| `devdocs/modernization/SECURITY-REVIEW.md` | Whole-codebase security review in passes: scope, threat model, route map, findings, pass log. Review only, fixes become cases |
 | `devdocs/modernization/PROJECT-OVERVIEW.md` | Products, deployment targets, business context |
 | `devdocs/modernization/CODEBASE-REFERENCE.md` | Stack, structure, configuration, API conventions |
 | `devdocs/modernization/TESTING.md` | Test suites and their state |

devdocs/modernization/SECURITY-REVIEW.md

@@ -0,0 +1,238 @@
+# AyaNova v8 security review
+
+Whole-codebase security review requested by John (2026-10-23). Done in passes, one area per
+session; each pass ends with its findings written here and committed. **Review only: no code
+changes.** Each fix John approves becomes its own case.
+
+Findings in this file describe weaknesses in shipped software. The repository is private;
+keep it that way while findings are open.
+
+## Status
+
+| Pass | Area | Status |
+|---|---|---|
+| 1 | Threat model and attack-surface map | Done 2026-10-23 |
+| 2 | Unauthenticated routes, login, tokens, password reset, 2FA | Next |
+| 3 | Customer contact users: what they can reach and read | |
+| 4 | Report rendering (templates, the server-side browser, report data and download routes) | |
+| 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) | |
+| 8 | Email and notifications (`direct-smtp`, templates, delivery) | |
+| 9 | Client (XSS, token storage, `npm audit` triage), CORS, headers, Swagger UI | |
+| 10 | Deployment: installers, Linux setup script, nginx, systemd, config and data permissions | |
+| 11 | Sockeye public routes (licence fetch, trial requests) | |
+
+The order is by risk to internet-facing servers. Findings from any pass go in **Findings**.
+
+## Scope
+
+- **In:** the AyaNova server (`raven/server/AyaNova`) and client (`raven-client`); the Windows
+  installers and launcher; Linux packages and the hosted subscription server setup script
+  (`sockeye-client/src/views/biz-subscription-server.vue`); the configuration templates.
+- **Sockeye:** only its public routes (licence fetch and trial requests). Its admin UI is used
+  by John alone (revisit if agents use it later).
+- **Out:** QBI.
+
+## Threat model
+
+**Deployments**
+
+| Deployment | Reachable by | Notes |
+|---|---|---|
+| Hosted subscription droplet (Ubuntu, nginx + Let's Encrypt, `www-data`) | The internet | Most exposed. One company per droplet |
+| Self-hosted Windows IIS | Often the internet | The manual recommends IIS when there is internet access |
+| Self-hosted Linux server | LAN or internet | Customer's own reverse proxy, if any |
+| Windows LAN / Linux desktop | Local network | Runs as the logged-in user (Linux desktop: as root with `sudo ./AyaNova`) |
+| Windows standalone | The PC itself | Mostly evaluation (John) |
+
+**Who might attack**
+
+1. **Anyone on the internet** (no login): 14 anonymous routes, the static client, Swagger UI,
+   health check.
+2. **A customer contact** (customer-user login, roles `Customer`/`CustomerRestricted`):
+   should see only its own customer's data. The highest-value question in this review: can
+   one customer's login read or change another customer's data, or staff data?
+3. **A staff user with limited roles**: should not exceed them (e.g. a restricted tech
+   reaching accounting, ops or user admin).
+4. **Someone holding a leaked token**: a JWT or a download token (`?t=` in URLs, so it can
+   land in browser history and proxy logs).
+5. **A local user** on a Windows or Linux machine running AyaNova.
+6. **A report template author**: report templates are JavaScript run in a browser on the
+   server, with access to report data and the API.
+
+**What is at stake**: customer business data (customers, work orders, invoices,
+attachments), user logins and passwords, the company's email reputation (SMTP), the licence,
+the server itself (code execution, the database, backups), and availability.
+
+## Attack-surface map
+
+### Authentication and authorisation design
+
+- **Login**: `POST auth` returns a JWT (HS256, `jose-jwt`), signed with `AYANOVA_JWT_SECRET`
+  (random per boot if not set). Validation (`Startup.cs:255-279`): signature, issuer
+  `ayanova.com`, expiry, zero clock skew; audience not checked.
+- **One session per user**: the user's latest token is stored (`CurrentAuthToken`); the
+  middleware (`Startup.cs:366-451`) rejects any other valid token with E2004, except on
+  `/auth` and `notify/hello`, and except local report-render requests: host `127.0.0.1`,
+  `Request.IsLocal()` and an `rpl` claim. Behind the hosted nginx the client address is
+  forwarded (`X-Forwarded-For`, `UseForwardedHeaders`), so remote requests are not local.
+  The exception skips only the latest-token check, not signature or expiry.
+- **Roles** are read from the database on every request (middleware), not from the token.
+  **There is no route-level role attribute and no global "login required" policy**: each
+  controller has `[Authorize]` (login only) and each action calls `Authorized.Has…Role(...)`
+  itself. A missing call means any logged-in user, customer contacts included.
+- **Download routes** (`?t=`) take a per-user random download key set at login (`DlKey`,
+  with expiry, `AuthController.cs:272-276`) instead of a JWT. Some check only that the key
+  belongs to some active user (export, report, translation); others resolve the user and
+  check rights (attachments, backups, logs). To verify in passes 4 and 5.
+- **CORS**: any origin, any method, any header (`Startup.cs:167-174`). Tokens travel in the
+  `Authorization` header, not cookies, which limits the impact; checked in pass 9.
+- **Swagger UI** is public at `/api-docs` with the full API description (pass 9).
+
+### Routes
+
+Generated with `devdocs/modernization/security/routemap.py` (`python -I routemap.py
+<Controllers folder> <out.tsv>`): **460 actions** in 71 controllers. Regenerate it in later
+passes rather than trusting these counts if the code has changed.
+
+| Access | Actions |
+|---|---|
+| Anonymous (`[AllowAnonymous]`) | 14 |
+| Download key only (no attribute; check `?t=`) | 2 (`attachment/download/{id}`, `backup/download/{fileName}`) |
+| Login required (`[Authorize]`) | 444 |
+
+Controllers without a class-level `[Authorize]` (`Attachment`, `Backup`, `ServiceBank`) were
+checked: every action has its own `[Authorize]` or is a download-key route; `ServiceBank` is
+entirely commented out.
+
+**Anonymous routes** (pass 2 unless noted)
+
+| Route | Action |
+|---|---|
+| `GET /` (API root) | Index |
+| `POST auth` | PostCreds (login) |
+| `POST auth/tfa-authenticate` | TfaAuthenticate |
+| `POST auth/reset-password` | ResetPassword |
+| `GET export/download/{fileName}` | DownloadAsync (download key; pass 5) |
+| `GET health` | Get |
+| `POST license/lc` | lc |
+| `GET log-file/download/{logname}` | DownloadLog (download key; pass 5) |
+| `GET logo/{size}` | DownloadLogo |
+| `GET notify/hello` | GetPreLoginPing |
+| `GET report/download/{fileName}` | DownloadAsync (download key; pass 4) |
+| `GET report/export/{id}` | DownloadTemplate (download key; pass 4) |
+| `POST translation/subset/{id}` | SubSet |
+| `GET translation/download/{id}` | DownloadTranslation (download key; pass 5) |
+
+**Logged-in routes with no role or user-type check in the action itself** (53, heuristic:
+the check may sit in the business layer, or the route may be harmless, such as lists,
+own settings or own notifications). Each is resolved in pass 7 unless an earlier pass covers
+it: `GET server-info`, `GET role-rights`, `GET build-mode`, `POST auth/change-password`,
+`GET auth/totp`, `POST auth/totp-validate`, `GET authorization-roles/list`,
+`GET/POST/DELETE data-list-column-view…`, `GET data-list/listkeys`, `GET data-list/listfields`,
+`GET/PUT/POST/DELETE data-list-filter…`, `GET enum-list/list/{enumkey}`,
+`GET enum-list/listkeys`, `GET form-field-reference/{key}`, `GET global-biz-setting/client`,
+`GET job-operations/status|progress|logs/{gid}`, `POST job-operations/request-cancel`,
+`POST kpi`, `POST license/permanently-erase-all-data[-keep-tax-codes]`,
+`GET notify/new-count`, `GET notify/app-notifications`, `DELETE notify/{id}`,
+`POST notify/direct-message`, `POST notify/direct-smtp`, `GET notify-subscription/list`,
+`GET pick-list/template/list`, `GET pick-list/template/listfields/{ayaType}`,
+`POST report/data`, `POST report/upload`, `POST schedule/svc`, `POST schedule/user`,
+`POST search`, `GET server-state`, `GET tag-list/list`, `GET tag-list/cloudlist`,
+`GET translation/{id}`, `GET translation/list`, `GET translation/translationkeycoverage`,
+`POST translation/subset`, `POST translation/upload`, `POST trial/seed`, `GET user/amsu`,
+`GET workorder/customer/{id}`.
+
+Already resolved from that list in pass 1:
+- `license/permanently-erase-all-data[-keep-tax-codes]`: SuperUser only (user id 1) plus the
+  typed confirmation `i understand` (`LicenseController.cs:248-275`). OK.
+- `trial/seed`: only with a trial licence and an empty database (only SuperUser exists then)
+  (`TrialController.cs`). OK.
+- `notify/direct-smtp` and `notify/direct-message`: see F3 and F4.
+
+**Highest-interest leads for later passes**: `POST search` and `POST kpi` (do they filter by
+role and customer?), `POST report/data` and the report routes (pass 4), `GET
+workorder/customer/{id}` (customer users reaching other customers' work orders?), `POST
+schedule/svc|user`, `GET user/amsu`, `POST report/upload` and `POST translation/upload` (who
+may upload; what the content can do), `job-operations/*` (another user's job logs?).
+
+### Other ways in
+
+- **Report templates**: user-authored JavaScript and Handlebars run in a headless Chrome on
+  the server (`biz/ReportBiz.cs`), with report data, a server-issued token for API calls from
+  the template (`rpl` claim), and helpers such as `ayPutToAPI` (pass 4).
+- **Raw SQL**: data-list filters and sorts (`DataList/*`; case 4676 fixed the numeric and role
+  values), saved filters, search, KPI, schedules (pass 6).
+- **Files**: attachments (upload, download, move, maintenance), exports, report files and
+  templates, translation upload and download, logo, log files, backups and their deletion
+  (passes 4 and 5).
+- **Background jobs**: backup (`pg_dump`, fixed in this release to run without a shell),
+  notifications, licence fetch (outbound to the licence servers), report rendering, seeding.
+- **Email**: SMTP settings, notification templates, `direct-smtp` (pass 8).
+- **Configuration and secrets**: `config.json` (database password, JWT secret), logs (do
+  they contain tokens or passwords?), the boot log's config line (password redacted).
+- **Deployment**: Windows installers (folder permissions), the launcher, the hosted setup
+  script, nginx headers, the systemd unit (no hardening options), Linux desktop running as
+  root (pass 10).
+
+## Findings
+
+Severity: **Critical** (remote, no login, or full takeover) · **High** (any login, or one
+customer reaching another's data; or serious abuse of the company's resources) · **Medium**
+(needs a specific role, local access or an unusual setup) · **Low** (hardening, information).
+Status: *candidate* (seen in code, not yet confirmed), *confirmed*, *fixed in case NNNN*,
+*accepted*.
+
+| ID | Severity | Status | Title |
+|---|---|---|---|
+| F1 | High | candidate | Placeholder JWT secret in the LAN/IIS and Linux server config templates |
+| F2 | Medium | candidate | Windows installers give all local users modify rights on the program and data folders |
+| F3 | High | candidate | Any logged-in user, customer contacts included, can send arbitrary email through the company's SMTP server |
+| F4 | Low | candidate | Any logged-in user can send an in-app notification to any user |
+
+### F1 Placeholder JWT secret (from the 8.3.0 installer test)
+
+`dist/assets/lan-install-config.json` and `dist/assets/linux-server/config.json` ship
+`AYANOVA_JWT_SECRET` set to a placeholder that is also printed in the manual. The manual
+tells users to change it (Linux: "optional but recommended"). An installation that keeps it
+signs logins with a publicly known secret: anyone who knows it can mint a token for any user
+id, including SuperUser, as far as the signature goes. What else such a forged token needs
+(the per-user latest-token check, `CurrentAuthToken`) is to be confirmed in pass 2, as it may
+limit this considerably. Hosted droplets are not affected (the setup script generates a
+random secret since `64d8f62`; droplets created earlier: to check). Fix options: generate a
+random secret at install time; refuse to boot with the placeholder; or treat the placeholder
+as unset (random per boot).
+
+### F2 Windows folder permissions (from the 8.3.0 installer test)
+
+`lan.iss` and `standalone.iss` set `Permissions: users-modify` on `{app}` (the program
+folder) and on `{commonappdata}\ayanova` (data: database for standalone, backups,
+attachments, logs). Any local Windows user can therefore replace program files that run as
+the user who starts AyaNova or as the IIS application pool, and can read the database files
+and backups. Matters on shared or terminal-server machines. The program-folder permission
+originally let the report browser download beside the app; since 8.3.0 it downloads under
+the data folder, so `{app}` may no longer need it. To confirm in pass 10.
+
+### F3 `notify/direct-smtp` open to every logged-in user
+
+`NotifyController.SendNotifySmtpDirectMessage`: no role or user-type check; the code comment
+says this is intentional (Customer form, and API users emailing rendered reports, case 4310).
+Any logged-in user, customer contacts included, can send an email with any recipient
+(`ToAddress`), subject and HTML body through the company's configured SMTP server, from its
+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).
+
+### 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.
+
+## Pass log
+
+| Date | Pass | Result |
+|---|---|---|
+| 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 |

devdocs/modernization/security/routemap.py

@@ -0,0 +1,51 @@
+# Map every controller action: HTTP verb + route, anonymous or not, and which checks its body makes.
+# Usage: python -I routemap.py <Controllers folder> <out.tsv>
+import os, re, sys
+
+src, out = sys.argv[1], sys.argv[2]
+verb_re = re.compile(r'\[Http(Get|Post|Put|Delete|Patch)(?:\("([^"]*)"\))?\]')
+method_re = re.compile(r'public\s+(?:async\s+)?[\w<>\[\], ]+\s+(\w+)\s*\(')
+checks = {
+    'role': re.compile(r'Authorized\.Has\w+Role|HasAnyRole|HasReadFullRole|HasModifyRole|HasCreateRole|HasDeleteRole|HasSelectRole'),
+    'usertype': re.compile(r'UserTypeFromContext|UserType\.Customer|UserType\.HeadOffice|IsCustomerUser|CustomerUser'),
+    'biz': re.compile(r'Biz\.GetBiz|new \w+Biz\(|\w+Biz\.'),
+    'dltoken': re.compile(r'ValidateDownloadToken'),
+}
+
+rows = []
+for fn in sorted(os.listdir(src)):
+    if not fn.endswith('.cs'):
+        continue
+    text = open(os.path.join(src, fn), encoding='utf-8-sig').read()
+    # drop // comments so commented-out actions are ignored
+    text = '\n'.join(l for l in text.splitlines() if not l.lstrip().startswith('//'))
+    m = re.search(r'\[Route\("([^"]*)"\)\]', text)
+    base = m.group(1).replace('api/v{version:apiVersion}/', '') if m else ''
+    cls_auth = bool(re.search(r'\[Authorize\][^{]*class\s+\w+Controller', text, re.S))
+    for vm in verb_re.finditer(text):
+        after = text[vm.end():]
+        mm = method_re.search(after)
+        if not mm:
+            continue
+        attrs = text[max(0, vm.start() - 300):vm.end()] + after[:mm.start()]
+        anon = '[AllowAnonymous]' in attrs
+        # body: from the method's opening brace to the matching closing brace
+        start = vm.end() + mm.end()
+        brace = text.find('{', start)
+        depth, i = 0, brace
+        while i < len(text):
+            if text[i] == '{': depth += 1
+            elif text[i] == '}':
+                depth -= 1
+                if depth == 0: break
+            i += 1
+        body = text[brace:i]
+        found = [k for k, r in checks.items() if r.search(body)]
+        rows.append((fn.replace('Controller.cs', ''), vm.group(1).upper(), (base + '/' + (vm.group(2) or '')).rstrip('/'),
+                     mm.group(1), 'ANON' if anon else ('auth' if cls_auth or '[Authorize]' in attrs else 'NO-ATTR'), ','.join(found) or '-'))
+
+with open(out, 'w', encoding='utf-8') as f:
+    f.write('controller\tverb\troute\taction\taccess\tchecks\n')
+    for r in rows:
+        f.write('\t'.join(r) + '\n')
+print(len(rows), 'actions')