Limit password-session cookies to local-owner loopback clients and document device/session-token requirements for remote and multi-user v5 GA access.
9.8 KiB
Veritas Kanban v5.0 Security Review Notes
Review date: 2026-06-03
Scope: #350, covering multi-user auth, remote pairing, WebSocket scoping, desktop bridge/native surfaces, migration/import, attachments, preview processes, diagnostics, and release gates.
Review Outcome
The review fixed five high or critical implementation issues in the #350 implementation PR. One additional high issue remains a GA blocker and is called out below.
| Finding | Severity | Outcome |
|---|---|---|
Desktop will-navigate trusted renderer checks used a string prefix, so http://127.0.0.1:3000@attacker.example could load attacker-controlled content in the Electron window. |
Critical | Fixed. Desktop navigation now compares parsed URL origins and covers the userinfo bypass in desktop/src/main/__tests__/navigation.test.ts. |
Desktop setWindowOpenHandler and will-navigate opened external URLs directly instead of using the typed bridge validator. |
High | Fixed. External navigation now reuses validateOpenExternalRequest, blocking unsafe protocols and credentialed URLs before calling Electron shell.openExternal. |
| WebSocket chat broadcasts were channel-scoped but not chat-session-scoped. A subscribed or legacy authenticated client could receive live chat deltas for other sessions in the same workspace. | High | Fixed. Chat delivery now requires an explicit matching session subscription, with broadcast regression coverage. |
| Revoked API tokens and device sessions stopped new authentication but did not close already-open WebSocket connections. | High | Fixed. WebSocket auth now retains token/session identifiers and identity revocation routes close matching live sockets. |
| Preview start/stop routes used read permissions while starting local child processes for repo preview servers. | High | Fixed. Preview status/output remain readable with task:read, but start/stop now require admin:manage; preview process output is redacted before retention. |
GA Blocker Resolution
Browser password sessions still use the compatibility local-owner model. The JWT proves only that a password login happened; it does not carry a persisted user subject, session id, workspace membership, disabled-user state, or membership downgrade check. authMethod: "session" maps to the local admin authority.
The v5 GA release resolves this by taking the smaller safe boundary: password
sessions are explicitly local-owner only. The server accepts a password-session
cookie only on loopback requests with loopback Host, Origin, Referer, and
forwarded-host metadata. Remote/server, PWA, CLI/MCP, and multi-user workflows
must use trusted device sessions or scoped API tokens instead.
Persisted per-user browser sessions remain the future path if password login is ever promoted to a remote or multi-user auth method.
Accepted Hardening Risks For This Review
These were not treated as #350 blockers after the fixes above, but they should be triaged before #353 final release sign-off:
| Area | Risk | Rationale |
|---|---|---|
| API token and pairing lifetimes | API tokens can be non-expiring and pairing-created device sessions accept future expirations without a server-side maximum. | Admin-created credentials are already revocable and scoped, but GA should set maximum recommended lifetimes for remote deployments. |
WebSocket api_key query fallback |
Query-string credentials are easier to leak via logs/proxies than cookies or headers. | Kept only for WebSocket clients that cannot set upgrade headers. HTTP does not accept query API keys. |
| Production origins | If CORS_ORIGINS is unset, defaults still include loopback development origins. |
Remote docs require exact trusted origins. GA packaging/deployment checks should warn or fail on ambiguous production origin config. |
| Desktop command dispatch | The renderer can invoke the typed dispatchCommand enum, including native restart/update/quit/copy-diagnostics commands. |
The critical remote-content path is fixed by origin enforcement. A future hardening pass should split menu-only native commands from renderer commands or add main-issued intent tokens. |
| Desktop connection validation | The desktop bridge can fetch renderer-supplied http/https hosts to validate remote config. |
This is a limited network oracle by design. Keep scheme and credential validation, and consider private-address confirmation before public remote GA. |
| SQLite portability paths | Admin restore/import APIs accept filesystem paths and can copy/replace targets. | Admin-only, but final GA should add canonical base-directory and symlink policy tests before broad backup/restore support is advertised. |
| SVG attachments | SVG is accepted as an image type while image previews render image/* URLs. |
Downloads use attachment disposition and MIME validation exists. GA should either isolate/sanitize SVG previews or remove SVG from inline-previewable types. |
| Desktop child logs and debug bundles | Desktop process supervisor writes raw child stdout/stderr. | Server logs are redacted and diagnostics bundle creation is stubbed/redacted today. Any debug-bundle implementation must redact process logs before export. |
| Future filesystem bridge methods | Current validators require absolute local paths but do not enforce workspace/bookmark boundaries. | The exposed file-picker/export handlers are currently stubbed. Implement boundary checks before enabling real filesystem writes. |
| macOS entitlements | Electron entitlements include unsigned executable memory and disabled library validation. | Common Electron tradeoff, but release packaging should trim entitlements if the final build allows it. |
Controls Verified
- REST v1 route groups use explicit permission guards, with a route-map parity gate in
scripts/check-permission-coverage.mjs. - WebSocket connections validate origin, authenticate before accepting events, filter event delivery by workspace and permission, close backpressured clients, and now require matching chat-session subscriptions for chat streams.
- Device pairing uses random codes, server-side hashes, signed payloads, TTL, attempt locking, one-use redemption, revocation, downgraded-scope clamping, and device-session auth for REST/WebSocket.
- Desktop renderer runs with
nodeIntegration: false,contextIsolation: true, andsandbox: true. - Desktop bridge methods are declared in a typed registry. Dangerous methods require validators, event cleanup is covered, and unsupported client modes cannot call desktop-only bridge methods.
- Desktop safeStorage has no plaintext fallback for runtime secrets.
- Attachments sanitize filenames/task ids, validate resolved paths stay under attachment roots, validate MIME type using content where possible, and serve downloads with attachment content disposition.
- Diagnostics/support snapshots and preview output have redaction coverage for tokens, API keys, bearer values, and private local paths.
Regression Evidence
Focused tests added or exercised by the review:
pnpm --filter @veritas-kanban/desktop test -- navigation.test.ts bridge-contracts.test.tspnpm --filter @veritas-kanban/server test -- broadcast-service.test.ts v1-permission-guards.test.ts preview-service.test.ts
Full release gates should still run before merging the #350 PR:
node scripts/check-permission-coverage.mjspnpm typecheckpnpm lintpnpm build