Skip to content

fix(security): sprint-2 hardening — M-1, M-2, HD-1, HD-2, I-2#27

Open
stackbilt-admin wants to merge 1 commit intomainfrom
sprint-2-hardening
Open

fix(security): sprint-2 hardening — M-1, M-2, HD-1, HD-2, I-2#27
stackbilt-admin wants to merge 1 commit intomainfrom
sprint-2-hardening

Conversation

@stackbilt-admin
Copy link
Copy Markdown
Member

Summary

Follow-up to the 2026-04-10 audit critical chain (fixed in 256ba06). Closes five remaining findings in a single batch on a dedicated branch.

Audit report: Stackbilt-dev/codebeastdocs/audits/mcp-gateway-2026-04-10.md

Findings closed

ID Severity Change
M-1 MEDIUM (exploitable) Social OAuth state now bound to the initiating browser via an HttpOnly cookie (oauth_binding). handleSocialOAuthCallback rejects callbacks whose cookie doesn't match the KV-stored binding — kills the session-fixation path where any party holding the stateKey could complete the callback.
M-2 MEDIUM (exploitable) New userError(prefix, err, traceId) helper routes every tool-error return through sanitizeError + truncate (200 chars) + trace-id. sanitizeError extended with SERVICE_BINDING_NAMES regex so AUTH_SERVICE, ENGINE, DEPLOYER, etc. are stripped from any user-visible error text. Replaces direct err.message interpolation in scaffold_publish, scaffold_deploy, visual_*, scaffold_import, and all proxyToolCall error paths. Full errors still land in worker logs tied to the trace id.
HD-1 MEDIUM (hardening) KV sliding-window rate limit on /login (5 per email per 5 min + 20 per IP per 5 min) and /signup (3 per IP per 10 min). Runs before the auth RPC so brute-force attempts can't burn quota on the auth service or probe timing. Returns 429 + Retry-After.
HD-2 MEDIUM (hardening) Double-submit CSRF cookie on all OAuth form POSTs. New loginResponse() / signupResponse() wrappers mint a fresh token per page render, inject it as a hidden csrf_token field into all forms (email login, signup, oauth/github, oauth/google), and stamp a matching HttpOnly cookie. Every POST handler calls verifyCsrf() before touching anything else. SameSite=Lax on the cookie provides belt-and-suspenders protection on top of the double-submit check.
I-2 INFO Expanded the comment block on handlePostAuthorize's redirect-URI validation so a future maintainer can't mistake the deny-branch lookupClient() for dead code and delete it. Explains why approve doesn't need an explicit lookup (completeAuthorization does it internally).

New helpers (all exported + tested)

  • getCookie(request, name) — parses Cookie header, URL-decodes
  • buildCookie(name, value, maxAgeSeconds) — HttpOnly/Secure/SameSite=Lax/Path=/
  • clearCookie(name) — Max-Age=0 form
  • mintCsrf() — fresh random token
  • verifyCsrf(request, formData) — double-submit check
  • checkRateLimit(kv, key, limit, windowSeconds) — returns {allowed, retryAfter?}
  • getClientIp(request) — CF-Connecting-IP preferred, falls back to X-Forwarded-For

Test plan

  • npx tsc --noEmit — clean
  • npm test138 / 138 passing (was 120; +18 new unit tests)
  • New test coverage:
    • Cookie helpers (multi-cookie parsing, URL decoding, missing header)
    • buildCookie attribute contract
    • verifyCsrf (match, form-missing, cookie-missing, mismatch, empty-string)
    • checkRateLimit (under-limit, over-limit, independent keys, denied path doesn't re-increment)
    • getClientIp (CF-Connecting-IP, X-Forwarded-For fallback, unknown)
  • CI pipeline (.github/workflows/ci.yml) — unchanged, will gate this PR

Migration notes

  • Social OAuth state format changed from raw oauthParams string to JSON {oauthParams, browserBinding}. handleSocialOAuthCallback accepts the legacy format during the first ~5 minutes post-deploy with a console warning, so in-flight flows drain cleanly. After the TTL expires, all state writes use the new format.
  • CSRF tokens are mandatory on form POSTs. Browsers mid-flow when this deploys will hit a "Your session has expired" message and be re-prompted with a fresh CSRF cookie — a single redirect's worth of friction, no lost auth.
  • No breaking changes to the MCP tool surface. scaffold_publish still requires github_token (unchanged since H-2).

Out of scope (next sprint)

  • HD-3 gateway.ts decomposition (1321 LOC → 5 files)
  • HD-5 other decompositions (oauth-handler.ts, scaffold-materializer.ts, tool-registry.ts)
  • L-1 through L-4 cleanup pass
  • Pre-existing devDep CVEs (vite, miniflare, undici) flagged by npm audit --audit-level=high — CI step is continue-on-error: true so this is advisory; fix via npm audit fix in the cleanup pass

Audit log entry to follow

Will update docs/audits/mcp-gateway-2026-04-10.md in Stackbilt-dev/codebeast with a "2026-04-10 — Sprint 2 hardening closed" block once this PR merges and deploys.

Follow-up to 2026-04-10 audit critical chain. Closes five findings:

- M-1: social OAuth state bound to browser via HttpOnly cookie
       (kills session-fixation path)
- M-2: userError() helper routes every tool-error through
       sanitizeError + truncate + trace-id; sanitizeError now
       strips service binding names (ENGINE, AUTH_SERVICE, etc.)
- HD-1: rate limit /login (5/email/5min + 20/IP/5min) and
        /signup (3/IP/10min) via KV sliding window
- HD-2: double-submit CSRF cookie on all OAuth form POSTs
        (login, signup, oauth/github, oauth/google)
- I-2:  documented redirect URI validation asymmetry in
        handlePostAuthorize so the deny-branch lookupClient
        doesn't look like dead code to future maintainers

Tests: +18 (cookie, CSRF, rate limit, getClientIp). 120 → 138 passing.

Refs: Stackbilt-dev/codebeast docs/audits/mcp-gateway-2026-04-10.md
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