Skip to content

fix(web): restore and extend dashboard origin/session hardening - #55

Open
shoham-utila wants to merge 1 commit into
DaveDushi:masterfrom
shoham-utila:fix/web-dashboard-security
Open

shoham-utila wants to merge 1 commit into
DaveDushi:masterfrom
shoham-utila:fix/web-dashboard-security

Conversation

@shoham-utila

@shoham-utila shoham-utila commented Jul 31, 2026 •

Copy link
Copy Markdown

Two groups of changes, both in the dashboard request path. Found while doing a security review of the project.

1. Four v0.3.8 hardening measures were lost in a merge

The CHANGELOG under v0.3.8 → Security claims these. None of them are in master:

Claimed in v0.3.8 In v0.4.8
Constant-time token comparison absent — plain ===
Single-use URL token absent — replayable indefinitely
KOLSHEK_DEV dev-mode gating absent
Secure cookie flag absent

Traced to merge 712ebf7: first parent 488a420 carried the hardening, second parent a31b007 (the dashboard-ai-chat-agent branch, forked before v0.3.8) did not, and the resolution took the version without it. Nothing tested any of it, so nothing caught it.

This PR restores all four.

2. Findings from the review

  • Gate the :5173 Vite origins behind KOLSHEK_DEV. Cookies are not port-scoped (RFC 6265 §8.5) and SameSite compares scheme + registrable domain ignoring port, so :5173 is same-site with the dashboard. Trusting it unconditionally let any local process that binds that port issue credentialed, readable, state-changing requests — with no session token. This is the same gate as the lost v0.3.8 one.
  • .dev-session is dev-only now, written 0600 instead of Bun.write's default 0644, and unlinked on exit. It holds a live session token.
  • Persist the validated page layout (result.data.layout) instead of the raw request body on PUT /api/v2/pages/:id. Zod returns a stripped copy rather than mutating its input, and z.object() drops unknown keys silently instead of rejecting, so the raw body carried arbitrary unvalidated fields past validation. Those fields reach an inline <style> sink in WidgetGrid.
  • Align WidgetGrid with gridSchema (columns.sm/md/lg, not colsSm/colsMd/colsLg) and clamp interpolated values to bounded integers at the sink. The naming mismatch had silently disabled responsive columns.
  • Omit Access-Control-Allow-Origin for unknown origins rather than falling back to allowedOrigins[0].
  • Reject non-loopback Host headers (DNS rebinding), before auth or routing.
  • Reject reads carrying a present-but-unrecognised Origin. CORS already stops the caller reading the response; no reason to run the handler for it.
  • Narrow sanitizeError to absolute paths. The previous pattern matched any a/b substring, mangling messages like date must be YYYY/MM/DD.
  • Extend isSafeRegex to quantified alternations (a|a)+ and quantified quantified-bodies (?:a{10}){10}.

Verification

Beyond the 34 new tests, the behaviour was checked against a running server:

                                    default    KOLSHEK_DEV=1
GET  /api  Origin :5173             403        200
POST /api  Origin :5173             403        reaches handler
POST /api  Origin own-port          reaches handler
GET  /api  Host: evil.example       403        403
.dev-session                        absent     0600, removed on exit

The real dev workflow was also round-tripped (KOLSHEK_DEV=1 … dashboard + bun run dev:web, requests through the actual Vite proxy) to confirm the gate doesn't break it: GET through the proxy returns 200.

bun run typecheck clean, bun test 302 pass / 0 fail.

Two things worth your call

  1. Secure on the session cookie. Restored from v0.3.8. Verified working in Chrome on both localhost and 127.0.0.1, but unverified on Safari, which is historically the holdout for Secure cookies over plain http://localhost — and open uses the default browser. If Safari rejects it, every request after the redirect 401s and the dashboard is unusable there. Its security value is near zero given the loopback bind plus the new Host check, so it's easy to drop if you'd rather not risk it.

  2. Bundling. The v0.3.8 restoration and the review findings are in one commit because they touch the same function — this fix is one of the lost measures. Happy to split if you'd prefer them separate.

Notes

  • The page-layout fix is preventive and can't be reproduced end-to-end today: custom pages can't be created at all, because widgetSchema puts z.lazy() members inside z.discriminatedUnion(), which zod v3 doesn't support (the as any casts hide it from tsc), so every page write returns 400 with a TypeError. Confirmed on both zod 3.25.76 and 3.24.1 — a latent defect, not dependency drift. Filing separately; not fixed here to keep this PR to security changes.
  • .dev-session uses writeFileSync rather than Bun.write because Bun.write silently ignores { mode } for string sources. Only the BunFile-copy path honours it (fix(io): respect mode option when copying files with Bun.write() oven-sh/bun#25906); string sources are still pending in Bun.write: apply mode for string, buffer, Blob and stream sources on POSIX oven-sh/bun#32885. This matches what src/security/keychain.ts already does for credentials.key.

Two groups of changes, both in the dashboard request path.

## Regressions restored

v0.3.8 shipped four dashboard hardening measures (see CHANGELOG "v0.3.8 →
Security"). All four were lost at merge commit 712ebf7 ("Merge remote-tracking
branch 'origin/master'"): its first parent 488a420 carried the hardening, its
second parent a31b007 — the dashboard-ai-chat-agent branch, forked before
v0.3.8 — did not, and the resolution took the version without it. They have
been absent since, while the CHANGELOG continued to claim them. Nothing tested
them, so nothing caught it.

- Constant-time token comparison (safeEqual/timingSafeEqual), replacing `===`
  on the session cookie and the /auth/token endpoint
- Single-use URL token — consumed on first visit, so replay from browser
  history or terminal scrollback is rejected
- KOLSHEK_DEV gating for the Vite dev integration
- Secure attribute on the session cookie

## Audit findings

- Gate the :5173 Vite origins behind KOLSHEK_DEV. Cookies are not port-scoped
  (RFC 6265 §8.5) and SameSite compares scheme + registrable domain ignoring
  port, so :5173 is *same-site* with the dashboard. Trusting it unconditionally
  let any local process that binds that port issue credentialed, readable,
  state-changing requests with no session token.
- Write .dev-session only in dev mode, at 0600 rather than Bun.write's default
  0644, and unlink it on exit. It holds a live session token.
- Persist the validated page layout (`result.data.layout`) instead of the raw
  request body on PUT /api/v2/pages/:id. Zod returns a stripped copy rather
  than mutating its input, and z.object() drops unknown keys silently instead
  of rejecting, so the raw body carried arbitrary unvalidated fields past
  validation. Those fields reach an inline <style> sink in WidgetGrid.
- Align WidgetGrid with gridSchema (columns.sm/md/lg, not colsSm/colsMd/colsLg)
  and clamp every interpolated value to a bounded integer at the sink. The
  naming mismatch had silently disabled responsive columns.
- Omit Access-Control-Allow-Origin entirely for unknown origins rather than
  falling back to allowedOrigins[0].
- Reject non-loopback Host headers (DNS rebinding), before auth or routing.
- Reject reads carrying a present-but-unrecognised Origin. CORS already stops
  the caller reading the response; no reason to run the handler for it.
- Narrow sanitizeError to absolute paths. The previous pattern matched any
  "a/b" substring, mangling messages like "date must be YYYY/MM/DD".
- Extend isSafeRegex to quantified alternations ((a|a)+) and quantified
  quantified-bodies ((?:a{10}){10}).

## Tests

34 new tests across a unit file and an auth integration file — the coverage
whose absence let the v0.3.8 hardening regress unnoticed.

Note: the page-layout fix is preventive, and cannot be reproduced end-to-end
today. Custom pages cannot be created at all: widgetSchema puts z.lazy()
members inside z.discriminatedUnion(), which zod v3 does not support (`as any`
casts hid it from tsc), so every page write returns 400. Reported separately;
not fixed here to keep this PR to security changes.

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

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant