diff --git a/CLAUDE.md b/CLAUDE.md index 6d8a3cc0..c8f6b991 100644 --- a/CLAUDE.md +++ b/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 | diff --git a/devdocs/modernization/SECURITY-REVIEW.md b/devdocs/modernization/SECURITY-REVIEW.md new file mode 100644 index 00000000..5ad06f80 --- /dev/null +++ b/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 + `): **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 | diff --git a/devdocs/modernization/security/routemap.py b/devdocs/modernization/security/routemap.py new file mode 100644 index 00000000..7ad03f66 --- /dev/null +++ b/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 +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')