Security: authenticated session cookies, /api/asset SSRF fix, OAuth state#11
Open
gndclouds wants to merge 1 commit into
Open
Security: authenticated session cookies, /api/asset SSRF fix, OAuth state#11gndclouds wants to merge 1 commit into
gndclouds wants to merge 1 commit into
Conversation
…h state
Three confirmed vulnerabilities found in a defensive review:
- Session cookies used AES-256-CBC with no MAC/auth tag, so the encrypted
userId was malleable and forgeable, and the secret fell back to a hardcoded
"dev-secret-change-me" string. Switch to authenticated AES-256-GCM (tamper-
evident) and require a real SESSION_SECRET of >=32 chars (no insecure
fallback). Legacy/unauthenticated cookies are now rejected. (critical)
- GET /api/asset gated its server-side fetch with a substring check
(url.includes("vercel-storage.com")), so http://169.254.169.254/?vercel-storage.com
and http://vercel-storage.com.attacker.com/ passed — SSRF plus leak of the
BLOB_READ_WRITE_TOKEN bearer header to an attacker host. Replace with a parsed
https-only exact-hostname allow-list. (high)
- Are.na OAuth had no state parameter -> login CSRF / session fixation. Bind the
round-trip to a random state value in an httpOnly cookie, validated with a
constant-time compare on callback. (high)
Verified: tsc --noEmit, eslint, full `next build`, and a crypto round-trip /
tamper-rejection check all pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: multica-agent <github@multica.ai>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes three confirmed vulnerabilities found in a defensive security review of the app. Each was verified against the actual code (data flow traced, not theoretical).
1. CRITICAL — Forgeable session cookies (
src/lib/session-crypto.ts)userIdand impersonate any user."dev-secret-change-me"whenSESSION_SECRETwas unset — making every session forgeable.Fix: switched to authenticated AES-256-GCM (tamper-evident via auth tag) and now require a real
SESSION_SECRET≥ 32 chars (no insecure fallback, validated lazily so the build doesn't break). Cookies not in the authenticated format — including the legacyiv:ciphertextform — are rejected, so existing users simply re-log in once.2. HIGH — SSRF + token leak in
GET /api/asset(src/app/api/asset/route.ts)?url=with a substring check (url.includes("vercel-storage.com")). Payloads likehttp://169.254.169.254/?vercel-storage.comorhttp://vercel-storage.com.attacker.com/passed and were fetched server-side — and the request carried theBLOB_READ_WRITE_TOKENbearer header, leaking the blob token to an attacker host. The endpoint is unauthenticated.Fix: parse the URL and enforce an https-only exact-hostname allow-list (
vercel-storage.comor*.vercel-storage.com).3. HIGH — OAuth login CSRF / session fixation (
src/app/api/auth/login+callback)stateparameter, enabling login CSRF (a victim silently logged into the attacker's Are.na account).Fix: generate a random
state, store it in an httpOnly cookie at/api/auth/login, and validate it with a constant-time compare on callback; the cookie is cleared afterward. (Shared cookie-name constant lives insrc/lib/oauth.ts— Next.js route files can't export arbitrary symbols.)Testing
npx tsc --noEmit— passesnpx eslinton changed files — passesnpx next build— compiles, type-checks, and generates all routes successfullyNot included (recommended follow-ups)
These were also confirmed in the review but need their own change (new dependency and/or careful template work):
content_html/embed.htmlrendered through triple-stache{{{ }}}in templates (no HTML sanitization anywhere). Needs a sanitizer (e.g.sanitize-html) with an embed allow-list so media embeds keep working.handlebars@4.7.8(→ 4.7.9) andnext@15.5.14(→ latest 15.x).npm audit --omit=devreports 1 critical / 5 high.newsletter/subscribeandwaitlist; sessionsecureflag only set whenNODE_ENV==="production"(plaintext on non-prod HTTP).<style>custom-CSS</style>breakout in the build path; cron secret compared with non-constant-time!==; Are.na access token stored in plaintext.🤖 Generated with Claude Code