fix(security): helmet, request size limit, login rate limiting#44
fix(security): helmet, request size limit, login rate limiting#44
Conversation
Specifies the complete test plan for building a safety net before refactoring index.ts — route tests, WS integration, component tests, hook tests, and targeted e2e tests across 9 PRs. Made-with: Cursor
- supertest + @types/supertest for route-layer testing - @testing-library/react, jest-dom, user-event for component/hook tests - jsdom for frontend test environment - vitest.workspace.ts splits server (node) and frontend (jsdom) envs Made-with: Cursor
Moves all Express route registrations into server/app.ts which exports the app instance. index.ts retains HTTP server, WebSocket, and startup. This enables supertest-based route testing without starting the server. Zero behavior change — same routes, same middleware ordering. Made-with: Cursor
27 tests covering auth, permissions, sessions, config, and file routes using supertest against the exported Express app. Tests verify auth middleware ordering, path validation, and error responses. Made-with: Cursor
- 25 component tests: PermissionBanner (7), ThinkingBlock (4), ErrorBoundary (3), ToolPill (3), ToolGroup (3) - 9 hook tests: useChatSession (4), useChatConnection (3), usePermission (2) - jsdom installed at root for vitest environment support - Removed vitest.workspace.ts in favor of per-file @vitest-environment - Total: 265 tests across 31 files (up from 236/23) Made-with: Cursor
- npm audit --audit-level=high (continue-on-error for now) - mcp-server build check added to CI pipeline - Test count guard raised from 200 to 250 (actual: 265) - Summary step updated to include mcp-server build Made-with: Cursor
Lock file generated with Node 24/npm 11 needs matching CI runtime. Made-with: Cursor
jsdom's optional native dependencies (@emnapi/*) vary by platform. Lock file generated on macOS ARM doesn't include Linux variants, causing npm ci to fail on Ubuntu runners. Made-with: Cursor
Made-with: Cursor
- helmet with CSP (self + websocket connect-src) - express.json limit set to 10MB (prevents large payload abuse) - Login endpoint rate-limited to 5 req/min per IP - Zero behavior change for normal usage Made-with: Cursor
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Blocking issue:
The new Helmet CSP in server/app.ts is likely to break existing frontend behavior that relies on data: image URLs.
Current policy sets defaultSrc 'self' and does not define img-src, so browsers fall back to default-src for images. Mitzo currently renders image previews and CSS icons from data URLs, so this can regress attachment preview and some UI assets.
Suggested fix:
- Add an explicit
imgSrcdirective that includes at least'self'anddata:. - Then run a quick manual verification for chat image preview and basic UI rendering on mobile Safari.
Non-blocking follow-up:
- Add route-level tests for login rate limiting (
429) and a test/assertion for CSP headers so this doesn’t regress silently.
|
Additional issue (non-blocking, but should be tracked): Security behavior added in this PR is not explicitly covered by regression tests yet. Requested additions:
Reason: these controls are easy to regress during middleware refactors; route-level assertions keep us honest. |
- CSP imgSrc directive now allows 'self' and data: URIs (fixes broken image previews, paste attachments, and CSS data-URI icons) - Route tests assert CSP img-src and X-Content-Type-Options headers - Login rate-limit test verifies 429 after threshold exceeded Made-with: Cursor
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Review: LGTM (with fixes applied)
Fixes pushed in f876507:
Blocking issue resolved
- CSP
imgSrcdirective added —["'self'", "data:"]now in the helmet config. Verified against codebase:resizeImage()returns data URIs for paste/attach previews,MessageBubblerenders them as<img src>, andglobal.cssuses a data URI SVG for select dropdowns. All would have broken without this.
Tests added
- CSP header assertion — verifies response includes
img-src 'self' data:andx-content-type-options: nosniff - Rate-limit test — fires 10 rapid login attempts, asserts at least one returns 429 with the expected error message
Security implementation review
Helmet — Good defaults. CSP is appropriately scoped: script-src 'self' (no inline scripts), style-src allows 'unsafe-inline' (necessary for React's style usage), connect-src includes WS/WSS for the chat WebSocket. No issues.
Request size limit — 10MB is reasonable. Image attachments are resized client-side (MAX_DIMENSION=1600) before sending, so even 4 max-size images won't approach this.
Rate limiting — 5 req/min on login is tight but appropriate for a single-user app. standardHeaders: 'draft-7' is correct. The limiter is applied after authMiddleware but authMiddleware correctly exempts /auth/login (line 55 of auth.ts checks req.path === '/auth/login'), so unauthenticated login requests reach the rate limiter.
CI gap
This PR targets test/infra-setup, not main. CI only triggers on PRs to main, so these changes won't be CI-validated until the base branch merges. After #43 lands, retarget this to main to get a CI run.
Total tests: 268 (up from 265), all passing locally.
Resolve conflicts in ci.yml (keep npm install comment), app.ts (keep helmet/rate-limit additions), and routes.test.ts (keep security test blocks). Made-with: Cursor
dimakis
force-pushed
the
fix/security-quick-wins
branch
from
3afd62c to
a3b20c6
Compare
1 participant
Summary
Three quick security hardening changes — zero behavior change for normal usage.
Test plan
Made with Cursor