WFDASH-WEBAPP/docs/audit/security-perf-prod.md

# Security, Performance & Production-Readiness Audit

**Scope:** WFDASH Next.js app + PocketBase backend.
**Date:** 2026-08-28
**Author:** tom (audit only — no code changes shipped)

This document catalogues the issues that stood out while reading the
codebase end-to-end. It is intentionally a "report", not a fix-it-up
PR. Each finding has a severity (critical / major / minor), a
`file:line` reference where it lives, and a recommended fix.

Conventions used in the file refs:

- `src/...` — relative to repo root.
- Line numbers reference the current state of the file at audit time.

---

## 0. Headline summary

| Severity | Count | What it is |
| -------- | ----- | ---------- |
| Critical | 4     | Hardcoded production-looking secrets, missing security headers, very weak `AGENT_API_KEY` default, plaintext `ai_settings.api_key` storage |
| Major    | 11    | Many `/api/v1/*` routes have no auth, no rate limiting, no request-size limits, unauthenticated file proxy, public CORS `*` on MCP, `localStorage` PB token, no observability, dependency risks, console noise leaking PII, no CSP, no backup plan for `ai_settings` |
| Minor    | 9     | N+1 patterns, untyped env reads, missing `no-store` on sensitive endpoints, no CI, file type / extension trust, race conditions on shared rows, no HTTP `secure` cookie enforcement in dev, etc. |

The good news: there is no `dangerouslySetInnerHTML` of user-controlled
content without a custom sanitizer (`src/components/quote-email-card.tsx:195-237`),
no `eval`/`new Function` anywhere, the file-proxy has a hard whitelist
of collections, the TV PIN handler uses a constant-time-ish check with
a 250 ms flattening delay (`src/app/api/tv/login/route.ts:34-46`), and
the auth helper re-validates the PB token against PB on every call
instead of trusting the JWT.

The bad news is mostly the auth surface, secret handling, and the
"we trust the dev identity cookie" model. Those are blocking for
production.

---

## 1. Critical findings

### 1.1 Hardcoded production-looking credentials in repo root

**File:** `.env:5` (PB_ADMIN_PASSWORD=<redacted — see .env, ~10-digit numeric>)
**File:** `.env:7` (AGENT_API_KEY=<redacted — see .env, 6-digit numeric>)

`/Volumes/EXT/DEVELOPMENT-FORGEJO/WFDASH-WEBAPP/.env` is checked into
the working tree on disk and contains what look like real values
for the production deployment (literal values redacted here to avoid
permanently leaking them into git history when this doc is committed —
see the actual `.env` file for the current values):

```
PB_ADMIN_PASSWORD=<redacted — 10-digit numeric string>
AGENT_API_KEY=<redacted — 6-digit numeric string>
```

`.gitignore` correctly excludes `.env*` (`.gitignore:28`), so the file
is **not** in git history — but a `.env.local` and `.env.local.bak`
sitting next to it (`ls -la` output) suggest the same values have been
copied into per-developer files for a while. Whoever runs `git status`
today won't see them, but a copy-paste into a screenshot, a
`tar`/backup, a CI artifact, or a sibling repo will leak them. The
length (6 digits for `AGENT_API_KEY`, 10 numeric for
`PB_ADMIN_PASSWORD`) makes both trivially brute-forceable against a
public deployment.

**Fix:**

1. Rotate both immediately. Treat them as compromised.
2. Move all secrets to a real secret store (Doppler / Vault / Coolify
   env / a 1Password CLI hook). Do **not** rely on `.env.local` for
   production.
3. Add a `pre-commit` + CI grep that fails on the literal patterns
   `PB_ADMIN_PASSWORD=`, `AGENT_API_KEY=`, `ANTHROPIC_API_KEY=`,
   `sk-`, `Bearer\s+\w+` in any committed file.
4. Make `AGENT_API_KEY` at least 32 bytes of CSPRNG output
   (`openssl rand -hex 32`).

---

### 1.2 `AGENT_API_KEY` is a single bearer shared with the browser bundle

**Files:**
- `src/lib/server/agent-auth.ts:4-19` (the check)
- `src/app/api/mcp/route.ts` (the route)
- `next.config.ts:16` (the rest)

`assertAgentAuth` accepts `Authorization: Bearer <AGENT_API_KEY>` and
that single token is used by:

- The MCP `/api/mcp` route, which calls the REST `/api/v1/*` routes
  internally.
- The agent CLIs that drive the same routes from outside.

The MCP route is exposed with `Access-Control-Allow-Origin: *`
(`src/app/api/mcp/route.ts:78-87` — `response.headers.set("Access-Control-Allow-Origin", origin ?? "*")`).
That means **any origin on the public internet** can call
`/api/mcp` with that bearer token, and the operator's n8n workflow
broker the same key. Combined with finding 1.1 (`AGENT_API_KEY` is a
6-digit numeric string — see redacted value),
this is the single most damaging exposure in the system.

**Fix:**

- Restrict `Access-Control-Allow-Origin` to an explicit list (e.g. the
  Hermes domain and the agent's own origin). Reject any other origin
  with 403.
- Issue **per-integration** keys (n8n key, Hermes key, agent CLI key)
  and track them separately. Revoke the shared one.
- Move MCP and `/api/v1/*` agent routes behind a different auth path
  (mTLS, signed JWT with short TTL, or at minimum an HMAC request
  signature that includes timestamp + nonce).

---

### 1.3 Plaintext `api_key` in `ai_settings` PB row, with no at-rest encryption

**File:** `src/lib/server/ai-settings-service.ts:36-39` (the comment
that explicitly says "plaintext by design")
**File:** `src/lib/server/ai-settings-service.ts:132-133, 235-236`
(where the key is read/written)

`ai_settings.api_key` is stored as a plain string field on the PB
`ai_settings` row. The operator explicitly chose visibility over
encryption-at-rest in August 2026 (per the file's own comment), but:

- The PB admin JWT and the in-app UI both read the row through
  admin PB (`getAdminPocketBase`), which means the key is in PB's
  on-disk SQLite. Anyone with shell access to the PB container
  (`pocketbase/data/data.db`) gets every operator's LLM API key.
- The `/api/v1/ai/settings` route (`src/app/api/v1/ai/settings/route.ts:43-60`)
  does NOT include the secret in the response — but it does include
  `api_key_set: boolean`, so any bearer holder can probe the row to
  confirm whether a key is configured.
- The write path accepts the key in a JSON body over HTTPS only if
  the operator has Coolify terminating TLS. In dev (HTTP) it goes
  plaintext.

**Fix:**

- Encrypt `api_key` at rest using a key derived from
  `process.env.AI_SETTINGS_ENCRYPTION_KEY` (32-byte CSPRNG, rotated
  quarterly). Decrypt on read inside the service.
- Move the key to a sidecar (1Password CLI, Vault Agent, Coolify
  secret). The PB row should hold a *reference* (vault path /
  secret id), not the secret itself.
- Audit PB's `data.db` to confirm no prior keys were committed to
  backup snapshots.

---

### 1.4 Zero security headers and no CSP

**File:** `next.config.ts` (no `headers()` block at all)

`next.config.ts:1-46` configures `output: "standalone"`,
`serverExternalPackages`, `allowedDevOrigins` (which includes
`"*"`!), and redirects. It does **not** set any
`Content-Security-Policy`, `X-Frame-Options`,
`Strict-Transport-Security`, `Referrer-Policy`, or
`X-Content-Type-Options`. The only "origin policy" in the file is
`allowedDevOrigins: [..., "*"]`, which is the most permissive
possible value and is shipped in the production build because there
is no env-gated branch.

**Fix:**

- Add a `headers()` block in `next.config.ts`:

  ```ts
  async headers() {
    return [{
      source: "/:path*",
      headers: [
        { key: "Content-Security-Policy", value: "default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; img-src 'self' data: https:; connect-src 'self' <POCKETBASE_URL> <HERMES_URL> <N8N_URL>; frame-ancestors 'none'; base-uri 'self'; form-action 'self';" },
        { key: "Strict-Transport-Security", value: "max-age=63072000; includeSubDomains; preload" },
        { key: "X-Content-Type-Options", value: "nosniff" },
        { key: "X-Frame-Options", value: "DENY" },
        { key: "Referrer-Policy", value: "strict-origin-when-cross-origin" },
        { key: "Permissions-Policy", value: "geolocation=(), microphone=(), camera=()" },
      ],
    }];
  }
  ```

  Tweak `script-src` to match your actual scripts (you have
  `next/script` and `dangerouslySetInnerHTML` in two components, so
  `'unsafe-inline'` is currently needed; replace with nonces when
  you can).
- Move `allowedDevOrigins` out of the production config. Gate it
  on `process.env.NODE_ENV === "development"` and remove the `*`.
- Turn on HSTS at the Coolify/proxy level too (the header is
  already standard for Coolify + Traefik).

---

## 2. Major findings

### 2.1 Wide attack surface: 102 of 180 API routes have no auth

**Files:** `src/app/api/v1/orders/[id]/attachments/upload/route.ts`,
`src/app/api/v1/agent/scan/route.ts`, `src/app/api/v1/files/[collection]/[recordId]/[filename]/route.ts`,
plus every route in the following directories (sample listing):

- `src/app/api/v1/stock-purchases/**/attachments/**`
- `src/app/api/v1/purchase-orders/**/attachments/**`
- `src/app/api/v1/quotes/**/attachments/**`
- `src/app/api/v1/quotes/[id]/pdf/**` (PDF render endpoints)
- `src/app/api/v1/repairs/**` (entire resource — all 6 routes
  unauthenticated)
- `src/app/api/v1/buyer-tasks/**` (PO generation, mark-ready)
- `src/app/api/v1/collection-confirm/**` (confirm-stock, invoice-upload)
- `src/app/api/v1/sprint/items/**/claim` and `attachments/upload`
- `src/app/api/v1/invoicing/**`
- `src/app/api/counter/orders/[id]/attachments/route.ts`
- `src/app/api/invoicing/delivery-note/route.ts`
- `src/app/api/invoicing/customers/route.ts`
- `src/app/api/invoicing/delivery-note-pdf/route.ts`
- `src/app/api/tv/login/route.ts` (PIN-gated but no rate limiting,
  see 2.4)

A spot count via `find src/app/api -name "route.ts" -exec grep -L
"assertBrowserSession\|assertAgentAuth" {} \; | wc -l` shows
**102 of 180 routes have no `assertAgentAuth` or `assertBrowserSession`
call**. They rely on the route being called from the in-app UI, which
in turn relies on PocketBase's collection rules being tight.

Concrete consequences:

- `POST /api/v1/orders/[id]/attachments/upload` — anyone with the URL
  can append arbitrary files to any order's `order_attachements` field
  on PB. PB will accept up to 10 files per order (PB schema is the
  only gate).
- `GET /api/v1/agent/scan` — the entire PO triage report (which
  orders are stuck, which suppliers owe responses) is world-readable
  to anyone who can reach the app.
- `GET /api/v1/files/[collection]/[recordId]/[filename]` — gated by
  the `ALLOWED_COLLECTIONS` whitelist, but the whitelist is a
  reasonable superset of every collection that holds
  operator-uploaded documents. Anyone can read any operator's PDFs
  by guessing or scraping the URL shape
  (`/api/v1/files/orders/<id>/<pdf>`).
- `POST /api/v1/repairs/[id]/files/[field]` — no auth, no
  rate-limit, no size check beyond what PB enforces. The field is
  validated against `ALLOWED_FIELDS` but nothing else.

**Fix:**

- Apply a single auth boundary, ideally in middleware. The
  current `src/middleware.ts:8-20` is a no-op pass-through. The
  comment correctly explains why (the cookie-validating code uses
  `node:crypto`, which Edge runtime can't bundle), but the
  alternative isn't "do nothing" — it's "validate auth in each
  route handler" and that work hasn't been done for these 102
  routes.
- Pragmatic stopgap: add a global API middleware (Node runtime)
  that:
  1. Skips only `/api/v1/health`, `/api/tv/login`,
     `/api/counter/orders/[id]/attachments` (PWA kiosk).
  2. For everything else under `/api/v1/*` and `/api/mcp`,
     require a valid `AGENT_API_KEY` *or* a `assertBrowserSession`
     pass.
  3. For `/api/invoicing/*`, `/api/counter/*`, `/api/tv/*` —
     per-route auth as appropriate.
- Long-term: switch to a per-route `_auth` wrapper that the
  middleware can compose.

---

### 2.2 PocketBase auth token kept in `localStorage`

**File:** `src/lib/pocketbase.ts:18-46`

The PB SDK's `authStore` is persisted to `localStorage` under
`pocketbase_auth` with the raw JWT and the user record. Any XSS
on the app would let an attacker read this and impersonate the
operator with full PB privileges (the token is the user's PB JWT,
so it can do whatever the user can do per PB's collection rules).

The app has at least two `dangerouslySetInnerHTML` callsites that
render either operator-controlled data (the email body) or
theme-bootstrap script. The email body sanitizer
(`src/components/quote-email-card.tsx:195-237`) is a
hand-rolled regex sanitizer — it blocks `javascript:`, strips
event handlers, and limits tags. But the comment at line 177
explicitly says "This is NOT a general-purpose sanitizer (use
DOMPurify for that)." That is a future footgun if anyone adds a
new tags-allowing bypass (e.g. `<svg>`-with-`<use>` is currently
allowed because `<svg>` is not in the allow-list, but a small
regression in the regex could let a `<style>` through if the
stripper changes).

**Fix:**

- Move PB tokens out of `localStorage` into an `httpOnly;
  SameSite=Lax; Secure` cookie set by a server endpoint. The
  browser can keep using PB's SDK by hitting that cookie. The
  current `assertBrowserSession` already does server-side
  validation — the gap is the storage location.
- Replace the hand-rolled email sanitizer with DOMPurify
  (`isomorphic-dompurify` for SSR). Add it to the dep list and
  drop ~70 lines of custom regex code.
- Add a strict CSP (see 1.4) so even if XSS happens, it can't
  call out.

---

### 2.3 No rate limiting anywhere

**Files:** every `/api/**/route.ts`, especially:
- `src/app/api/tv/login/route.ts` (6-digit PIN brute-force window)
- `src/app/api/v1/quotes/[id]/from-email/route.ts` (PB write loop)
- `src/app/api/mcp/route.ts` (admin PB on every tool)
- `src/app/api/v1/orders/[id]/attachments/upload/route.ts`
  (multipart upload)

The TV PIN is 6 digits, so 1 000 000 combinations. The handler has
a 250 ms flattening delay (`src/app/api/tv/login/route.ts:35-38`),
which means ~4000 guesses/min from a single IP. No
`X-Forwarded-For` trust boundary is enforced, so an attacker
spoofing source IPs can multiply that. No per-IP lockout.

`assertAgentAuth` does not rate-limit at all. A leaked
`AGENT_API_KEY` (see 1.1) means a free pass to admin PB.

**Fix:**

- Add a simple token-bucket rate limiter in `src/middleware.ts`
  (Edge runtime) keyed on the request IP. ~30 req/min per IP
  globally, ~5/min on `/api/tv/login`, ~10/min on agent routes.
- For the TV PIN, lock out an IP for 60 seconds after 5 wrong
  attempts in a 5-minute window.
- For the MCP and `/api/v1/*` agent routes, rate-limit per
  `Authorization` token (so a leaked key is at least throttled).
- Consider adopting `@upstash/ratelimit` or `next-safe-action`'s
  rate-limit helper to avoid hand-rolling token buckets.

---

### 2.4 File upload routes have no per-route size cap and trust PB's defaults

**Files:**
- `src/app/api/v1/orders/[id]/attachments/upload/route.ts:39-50`
  (the comment says "default 5 MB but operators usually set 25 MB")
- `src/app/api/v1/collection-confirm/invoice-upload/route.ts`
- `src/app/api/counter/orders/[id]/attachments/route.ts:14-22`
  (only enforces a photo cap, but does it via env)
- `src/app/api/v1/repairs/[id]/files/[field]/route.ts`

The order-attachment route says "PB enforces 25 MB" but never
re-validates against the runtime value of PB's schema. An
attacker can POST a 25 MB binary, PB will store it, the proxy
will serve it back, and the operator pays the storage + bandwidth
cost.

More importantly, **MIME type is not validated** at the route.
`file.type` is browser-supplied and trivially spoofable in a
direct `curl`. PB stores whatever the client claims. A PDF-claimed
HTML file uploaded to `order_attachements` is then served back
through `/api/v1/files/orders/<id>/<file>` with `Content-Type:
text/html` (PB echoes the stored MIME) and rendered in the
operator's preview pane. The preview pane uses `<iframe>` /
`<img>` / `<object>` depending on type, so a successful upload
turns into an XSS on the operator's next visit to the order
detail page.

**Fix:**

- In each upload route, after `req.formData()`:
  1. Enforce a per-route cap (e.g. 10 MB for orders, 8 MB for
     counter photos) using `file.size` BEFORE handing to PB.
  2. Validate `file.type` against an allow-list per field
     (`order_attachements`: `image/*`, `application/pdf`; counter:
     `image/*` only; repairs.photos: `image/*` only;
     repairs.quote/PO/DN: `application/pdf` only).
  3. Validate the **filename** is not a path traversal attempt
     (no `..`, no leading `/`, no null bytes) and has an
     appropriate extension.
- Set `Content-Disposition: attachment; filename="..."` in the
  proxy (`src/app/api/v1/files/[collection]/[recordId]/[filename]/route.ts:152-154`)
  for HTML and any other "executable" MIME, **even if the
  operator originally uploaded it as PDF**. The proxy's
  current `isViewable` heuristic trusts PB's stored content-type,
  which the attacker controls.

---

### 2.5 `next.config.ts` ships `allowedDevOrigins: ["*"]`

**File:** `next.config.ts:18-20`

`allowedDevOrigins` is a dev-server feature that lets the HMR
client connect from a non-`localhost` origin. Setting it to `*`
in a `next.config.ts` that's the same file used for `next build`
and `next start` means the production server also allows HMR
from any origin. While the HMR socket is dev-only, having `*` in
the config is misleading and a sign that the dev/prod split is
missing.

**Fix:**

```ts
const isDev = process.env.NODE_ENV !== "production";
const nextConfig: NextConfig = {
  output: "standalone",
  serverExternalPackages: ["pdf-parse", "pdfjs-dist", "@napi-rs/canvas"],
  ...(isDev ? { allowedDevOrigins: ["127.0.0.1", "localhost", "192.168.1.42", "192.168.1.122", "192.168.1.128"] } : {}),
  ...
};
```

---

### 2.6 `next.config.ts` redirects include the local PIN

**File:** `next.config.ts:30-44`

The `/counter → /new-order` and `/tv → /pin` redirects are fine.
But the underscore-to-hyphen redirects for `/api/v1/order_items`
etc. are a soft commitment: agents (and humans copy-pasting from
PB collection names) reach the right URL, but it means the
server will accept *both* spellings forever. The redirect is
`permanent: true` so it 308s in perpetuity. That's a long-term
naming inconsistency baked into the public API.

**Fix:**

- Keep the redirects, but add an HTTP `Deprecation` or `Sunset`
  header on the underscore paths so callers can migrate.
- Or, change PB collection names to use hyphens (PB allows it)
  and drop the redirect.

---

### 2.7 CORS on `/api/mcp` is `*` (any origin can use the agent key)

**File:** `src/app/api/mcp/route.ts:78-87`

```ts
function withCors(response: NextResponse, req: NextRequest): NextResponse {
  const origin = req.headers.get("origin");
  response.headers.set("Access-Control-Allow-Origin", origin ?? "*");
  ...
}
```

If the request has no `Origin` header (e.g. `curl`, a server-side
agent), the response sets `*`. With an `Origin`, the response
echoes the origin back. That is functionally `Access-Control-Allow-Origin: *`
**and** credentialed CORS at once. The comment on line 68-77 says
"Production should narrow the origin allow-list" — it has not been
narrowed.

**Fix:**

- Replace with an allow-list:

  ```ts
  const ALLOWED_ORIGINS = new Set([
    process.env.NEXT_PUBLIC_SITE_URL,
    "https://hermes.example.com",
  ]);
  const origin = req.headers.get("origin");
  if (origin && ALLOWED_ORIGINS.has(origin)) {
    response.headers.set("Access-Control-Allow-Origin", origin);
    response.headers.set("Vary", "Origin");
  }
  // No CORS headers = browser will block; CLI/server callers ignore
  ```

---

### 2.8 No structured logging / observability

**Files:** 132 `console.*` callsites across `src/`, no logger
abstraction, no Sentry / OpenTelemetry / pino / datadog.

`grep -rn "console\." src/ --include="*.ts" --include="*.tsx" | wc -l` → 132.
The counts include:

- 13 in `src/app/(app)/orders/[id]/process/page.tsx` (operator PII
  in the body of the log lines)
- 10 in `src/app/api/v1/quotes/[id]/pdf/route.ts`
- 7 in `src/app/api/v1/quotes/[id]/submit/route.ts`

The messages include `console.log` of error bodies that contain
the customer's email subject, the supplier name, and the parsed
email body (`src/app/(app)/orders/[id]/process/page.tsx:1396-1405`).
That is PII leaking into the server's stdout.

`process.env` is also used directly in many places without a typed
loader (`grep -rn "process.env" src/lib/server/` returns 30+
results), so a typo in an env var name (`ORDER_ITEMS_COLLECTIONS`
vs `ORDER_ITEMS_COLLECTION`) silently falls back to a default and
nobody notices.

**Fix:**

- Add `pino` with a redact list (`req.headers.authorization`,
  `pb.authStore.token`, `*.email`, `*.password`). Wire it into
  the route-handler error catch in `src/app/api/v1/*`.
- For the Next.js server side, set up `pino-pretty` in dev and
  ship JSON to stdout in prod (Coolify picks it up).
- Add Sentry (or self-hosted GlitchTip) with the same redact
  config. Wrap the `assertBrowserSession` and `assertAgentAuth`
  calls so the path / actor / status code are recorded.
- Centralise env access in `src/lib/server/env.ts` using
  `zod` or `t3-env` so typos fail at boot.
- Replace `console.log("[send-po] ...", pdfErr)` and similar
  with `logger.error({ err, ...context }, "send-po failed")`.

---

### 2.9 Lucide-react pinned to a very old `1.16.0` fork

**File:** `package.json:73` — `"lucide-react": "^1.16.0"`
**File:** `package-lock.json:8499-8506` — confirmed version

The current published `lucide-react` package is at version
`0.4xx+` (the `0.x` line is the actively-maintained one by the
Lucide maintainers). Version `1.16.0` is from a *separate* fork
(the original Lucide pre-rebrand, ISC-licensed, last published
2018-ish). Peer-dep is `"react": "^16.5.1 || ^17.0.0 || ^18.0.0 ||
^19.0.0"`, so it does work with React 19, but it ships the old
"Lucide" icon set (outlined style) and is no longer receiving
security or icon updates.

This is a maintenance risk, not a known CVE. If a future
advisory lands against the `1.x` line, the project has no
upgrade path short of re-mapping every icon import.

**Fix:**

- Audit the icon list used (`grep -rn "from \"lucide-react\"" src/`).
- Migrate to the `0.x` line (`lucide-react@^0.460.0`) — every
  icon name has been preserved, but you'll get active maintenance
  + a much larger icon set.
- If migrating is too expensive today, pin a known-good lockfile
  revision and add a Renovate / Dependabot rule to track upstream.

---

### 2.10 PDF rendering libs: `pdfjs-dist` 5.x and `pdf-parse` 2.4.5

**File:** `package.json:78-79`
**File:** `next.config.ts:10-13` (the `serverExternalPackages`
escape hatch)

`pdfjs-dist` 5.x has had multiple CVEs in 2024-2025 (font
parser, JBIG2 decoder, arbitrary code execution on crafted
PDFs). The escape hatch `serverExternalPackages: ["pdf-parse",
"pdfjs-dist", "@napi-rs/canvas"]` is a workaround for webpack's
RSC runtime, not a security control — and the comment
(`next.config.ts:8-13`) admits the underlying lib has ESM/CJS
quirks that suggest an older or untested version may be in use.

`pdf-parse@2.4.5` is *not* the same package as the
`pdf-parse@1.1.1` that npm originally had; it's a separate
maintainer with a recent rewrite, and there's no public
security audit of 2.x yet.

**Fix:**

- Pin both to the latest patch of a known-good line. Add
  `npm audit` to CI.
- Prefer `@react-pdf/renderer` (already a dep) for any
  *generated* PDFs. Only use `pdfjs-dist` for parsing
  untrusted PDFs, and:
  1. Strip active content (JS, embedded files) before parse.
  2. Run parsing in a worker (already done via Next.js's
     node runtime) so a memory-corruption bug stays in
     the worker.
  3. Cap input size (see 2.4) so an attacker can't
     OOM the worker.

---

### 2.11 `dev-identity` cookie is the de-facto auth for many flows

**File:** `src/lib/dev-identity.ts:46-67`

`getServerIdentity` reads identity in this order:

1. `X-User-Email` request header (any caller can set this)
2. `wfdash_dev_email` cookie (the UI switcher)
3. `DEV_USER_EMAIL` env var
4. `DEFAULT_DEV_EMAIL = "david.els1@outlook.com"`

The header is explicitly listed as a per-request override — but
**any HTTP client can set `X-User-Email: <anything>` and be
attributed as that operator**. The function is used by routes
that need an actor name (audit notes, claim owners), so a
malicious caller can spoof audit-trail entries by setting the
header.

The comment block at the top of the file (`dev-identity.ts:1-20`)
calls out a real migration plan: "when real PB auth lands,
replace `getServerIdentity` with `assertBrowserSession`." Until
then, the system is in an explicit "no auth" mode, but the routes
that depend on identity don't all gate on it.

**Fix:**

- Drop the `X-User-Email` header override. Make the cookie the
  only override. Validate the cookie against a known set of
  dev emails (or against `pb.collection("users").authRefresh`).
- Audit every call site:

  ```bash
  grep -rn "getServerIdentity\|DEV_IDENTITY" src/app/api
  ```

  and ensure each is `assertBrowserSession`-gated before
  trusting the identity.

- Set a clear `DEADLINE` (e.g. 30 days) for removing the
  `DEFAULT_DEV_EMAIL` fallback so this can't quietly ship
  to prod.

---

## 3. Minor findings

### 3.1 CORS preflight sets `Access-Control-Max-Age: 86400`

**File:** `src/app/api/mcp/route.ts:85`

CORS preflight is cached for 24h. If you tighten the CORS
allow-list (see 2.7), browsers will keep allowing disallowed
origins for 24h. Lower to 600s (10 min) during the transition.

### 3.2 No request size cap on JSON endpoints

**File:** all `req.json()` callers in `src/app/api/v1/*/route.ts`

The default Next.js `req.json()` is bounded by the framework
defaults, but no route explicitly sets a max. A 100 MB JSON
body on `POST /api/v1/orders/create` would parse, allocate, and
only then be rejected as "items must be a non-empty array".

**Fix:** Add an explicit content-length check or use `req.json({
limit: "1mb" })` where supported.

### 3.3 `assertBrowserSession` doesn't bind to PB user id

**File:** `src/lib/server/browser-session.ts:30-44`

The function refreshes the JWT but never extracts the user id.
Downstream code that needs to "is this user the one who claimed
this row?" can't tell. Add a return value:

```ts
export async function assertBrowserSession(req: NextRequest): Promise<{ id: string; record: PBRecord }> { ... }
```

and propagate the `id` into `getServerIdentity` so the two
paths converge.

### 3.4 PB admin auth token cached in module-level memory indefinitely

**File:** `src/lib/server/pocketbase-admin.ts:7-15, 78-89`

`cachedAdminAuth` lives in the Node module scope. In a serverless
deploy (Vercel, AWS Lambda) this is fine (per-invocation). In
the standalone Next.js server (Coolify), it lives for the
process lifetime. If an operator's PB password is rotated, every
running instance keeps using the old token until process restart
(5-minute drift, per the `ADMIN_TOKEN_REFRESH_BUFFER_MS = 60_000`
buffer). The `clearAdminPocketBaseAuthCache` export exists but
no caller uses it.

**Fix:** Call `clearAdminPocketBaseAuthCache` from a
`process.on("SIGHUP")` (or a `/api/v1/admin/reload-auth`
debug-only endpoint) so a PB password rotation can take effect
without a process restart.

### 3.5 Many `getFullList` calls — N+1 risk

**File:** `src/lib/server/orders-service.ts:433, 473, 482, 621, 657, 696, 873`

The service uses `pb.collection(...).getFullList(...)` in many
places. PB paginates this internally (default 500/batch), so
"full list" really means "iterate until done". For collections
in the tens of thousands (`order_items` is approaching that),
this is slow under load.

**Fix:** Replace with paginated reads or filter-then-page
where the caller only needs a slice. Add a regression test
that asserts total latency for a 5 000-order workspace.

### 3.6 No CI, no test gating, no pre-commit hooks

**Files:** `package.json:73-90` has no `lint:fix`, no
`format:check`, no `test:ci` script. No `.github/workflows/`,
no `.gitlab-ci.yml`. The Dockerfile builds but doesn't run
tests.

**Fix:**

- Add a GitHub Actions workflow that runs `npm run lint`,
  `npm run test:run`, and `npm audit --audit-level=high`
  on every PR.
- Add Husky + lint-staged to enforce formatting on commit.

### 3.7 `console.error` messages embed PII (operator names, customer emails)

**File:** `src/app/(app)/orders/[id]/process/page.tsx:1276, 1285, 1396, 1405`

Examples (paraphrased — see actual file for exact strings):

- `console.error("[send-po] PDF fetch threw:", pdfErr)` — `pdfErr`
  includes the full response body, which includes the order
  number + customer email.
- `console.error("[send-po] PDF generation failed:", errBody)` —
  ditto.
- `console.log(...)` in the same file dumps the customer
  subject line to stdout.

**Fix:** Adopt a `logger.error({ err, orderId, userId, route }, ...)`
shape (see 2.8) and never log raw request/response bodies.

### 3.8 TV session cookie is not `secure` in dev

**File:** `src/lib/server/tv-session.ts:80-83`
**File:** `src/app/api/tv/login/route.ts:64-66`

The cookie sets `secure: req.nextUrl.protocol === "https:"`. In
dev (`http://localhost`), the cookie goes out as `Secure: false`
which is correct for localhost. But there is no environment
check that prevents `secure: false` from shipping to a
production Coolify deploy that terminates TLS at a proxy and
forwards plain HTTP internally — if `req.nextUrl.protocol` ever
reads `http` in that scenario (it should, since Next sees the
incoming protocol from `X-Forwarded-Proto` only when explicitly
configured), the cookie will be sent over plain HTTP and an
attacker on the LAN can steal it.

**Fix:** Force `secure: process.env.NODE_ENV === "production"`
in the cookie options; trust the env, not the request protocol.

### 3.9 `MCP_ERR_NETWORK` (-32010) and `MCP_ERR_UPSTREAM` (-32011) can leak internal URLs

**File:** `src/lib/server/mcp-upstream-fetch.ts:55-75`

The error envelope includes `data.url` — the full upstream URL
the MCP route tried to call. In a container deploy that URL is
`https://0.0.0.0:3000` (per the `Dockerfile` `HOSTNAME="0.0.0.0"`
and the `upstreamOrigin` fix in the MCP route). The comment
acknowledges the issue but the fix is partial. An agent that
reads this error and uses the URL to call back will hit
`0.0.0.0` from outside the container and fail.

**Fix:** Strip the URL down to origin + path in production.
For local-dev, keep the full URL. Use an env flag.

### 3.10 No CORS on `/api/v1/files/...` proxy

**File:** `src/app/api/v1/files/[collection]/[recordId]/[filename]/route.ts`

The file proxy is meant to be loaded by `<iframe src=...>` and
`<a target=_blank href=...>`. Both are same-origin in the
operator flow, so no CORS is needed today. But if a Hermes
frontend tries to embed a PDF preview in an `<iframe>`, the
browser will block it because there's no `Access-Control-Allow-Origin`.
Either add CORS to the proxy for the Hermes origin, or document
that the proxy is same-origin-only.

### 3.11 `tsconfig` paths used in tests require real `process.env` setup

**File:** `vitest.config.ts` (not opened but referenced in
README)
**File:** `src/lib/server/agent-auth.test.ts:6-16` (must set
`AGENT_API_KEY` before import)

Many tests assume `process.env.PB_ADMIN_EMAIL` / `PB_ADMIN_PASSWORD`
are set. If they're missing at import time, tests fail with
confusing errors. Add a `test-setup.ts` (referenced in
`package.json`) that asserts the env or stubs `getAdminPocketBase`.

### 3.12 `output: "standalone"` includes dev artifacts in the image

**File:** `Dockerfile:1-31`

The Dockerfile uses `node:20-alpine` (3-year-old EOL track —
Node 20 is in maintenance until April 2026, EOL October 2026).
The `builder` stage copies `.` into the image, including the
`docs/`, `scripts/`, `e2e/`, `screenshots`, `.audit-screenshots/`,
and PB exports. The `runner` stage only copies `public/`,
`.next/standalone/`, `.next/static/`, so the extras don't ship —
but the build cache is bigger than it needs to be.

**Fix:**

- Add `.dockerignore` excluding everything except `src/`,
  `public/`, `package.json`, `package-lock.json`, `next.config.ts`,
  `postcss.config.mjs`, `tsconfig.json`, and `next-env.d.ts`.
- Bump base image to `node:22-alpine` (current LTS as of
  2026-08). Pin to a specific minor (e.g. `node:22.11-alpine`)
  for reproducibility.

---

## 4. Production readiness checklist (roll-up)

| # | Item | Status | Owner |
|---|------|--------|-------|
| 1 | Rotate `AGENT_API_KEY` to ≥32 bytes CSPRNG | blocking | ops |
| 2 | Rotate `PB_ADMIN_PASSWORD` | blocking | ops |
| 3 | Move secrets out of `.env` files into a real store | blocking | ops |
| 4 | Add CSP, HSTS, X-CTO, X-Frame-Options, Referrer-Policy headers | blocking | dev |
| 5 | Add a global API auth middleware; remove `allowedDevOrigins: "*"` from prod config | blocking | dev |
| 6 | Restrict CORS on `/api/mcp` to an allow-list | blocking | dev |
| 7 | Move PB auth token out of `localStorage` into `httpOnly` cookie | blocker for prod | dev |
| 8 | Replace hand-rolled email HTML sanitizer with DOMPurify | major | dev |
| 9 | Add per-route upload size cap + MIME validation | major | dev |
| 10 | Add rate limiting (TV PIN, agent routes, global) | major | dev |
| 11 | Encrypt `ai_settings.api_key` at rest | major | dev |
| 12 | Add structured logging + Sentry/GlitchTip; redact PII | major | dev |
| 13 | Add CI (lint + test + audit) | major | dev |
| 14 | Replace `getFullList` patterns with paginated reads where feasible | perf | dev |
| 15 | Drop `X-User-Email` header override in `getServerIdentity` | security | dev |
| 16 | Force `Secure` cookie in prod for TV session | minor | dev |
| 17 | Migrate `lucide-react` to the `0.x` maintained line | maintenance | dev |
| 18 | Bump base Docker image to Node 22 LTS | maintenance | ops |
| 19 | Centralise `process.env` access in a typed env module | quality | dev |
| 20 | Add CORS to file proxy for Hermes embed | depends on Hermes scope | dev |

---

## 5. What I deliberately did **not** flag

- **`escapeFilterValue` in `src/app/(app)/invoicing/page.tsx:103`** —
  The filter strings are built from server-resolved PB ids and the
  comment makes the escaping contract explicit. This is safe.
- **The hand-rolled email sanitizer in `src/components/quote-email-card.tsx:195-237`** —
  Has gaps (no DOMPurify) but is well-tested in scope. The
  recommendation is to swap it for DOMPurify, not to remove the
  current check.
- **`getServerIdentity` cookie path** — The cookie path is `/`,
  `SameSite=Lax`, and 1 year max-age. For the dev-identity
  temporary stand-in this is acceptable; the real fix is to
  replace it (see 2.11).
- **`assertAgentAuth` timing** — String comparison of fixed-length
  tokens is not constant-time, but the token is large enough that
  timing attacks are impractical. If you rotate to a 32-byte
  CSPRNG token, this becomes irrelevant.
- **PDF and image generation libs (`jspdf`, `pdf-lib`,
  `@react-pdf/renderer`)** — These are widely used and have no
  known critical CVEs at the pinned versions. Re-evaluate at
  next major bump.
- **`xlsx` and `mammoth`** — Used for *operator-supplied* file
  parsing (price lists, supplier docs). They have a long history
  of CVEs around embedded XXE / formula injection. They are
  scoped to trusted-operator input today, but if you ever expose
  the upload UI to a customer, the same input boundary needs the
  same treatment as 2.4.

---

*End of audit. No code was changed during this task — all
recommendations are documented here for the team to triage.*