fix: address code-review findings on security hardening#327
Merged
Conversation
A high-effort review of this branch confirmed ten issues; the two worst would have taken production down at deploy: - CSRF gate: trust Sec-Fetch-Site when the browser sends it, and compare Origin against the configured public origin (AUTH_URL / forwarded host) instead of req.nextUrl.origin — behind Caddy that is the internal listen origin (localhost:3000), so every same-origin mutation 403'd. Same double-check removed from isSameOriginMutation. - DB TLS: follow libpq sslmode semantics — require/prefer encrypt without authenticating the server, so the droplet's self-signed Postgres keeps connecting; verify-ca/verify-full or DATABASE_CA_CERT turn verification on. - Session revocation: stop grandfathering pre-versioning JWTs. They can never gain a persisted claim, so grandfathering exempted every pre-deploy session from revocation forever; they now re-auth once. - Note size: the request byte cap now covers MAX_NOTE_BODY UTF-16 chars at worst-case JSON encoding (6 bytes/char) — large multibyte notes were unsaveable and existing ones uneditable. - CSP: img-src additionally allows the legacy public-bucket origin so images embedded in pre-migration note bodies keep rendering. - Sign-out: back to ending only this browser's session (revoke-all is a separate explicit call), warns before discarding un-synced offline edits, and legacy IndexedDB pending ops migrate again instead of being deleted. - AI metadata: a configured ANTHROPIC_KEY enables it again; AI_METADATA_ENABLED=false is the explicit opt-out. - Upload quota: uploads edited out of every note are swept when a new upload would otherwise exceed the quota, instead of leaking forever. Verified against next start: mutations with the production request shape (public Origin, internal listen origin) pass the CSRF gate and reach auth; foreign-origin and cross-site requests still 403. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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
A post-merge review of #326 confirmed 10 issues, two of which would take production down on the next successful deploy (deploys are currently fail-closed on the missing
DEPLOY_KNOWN_HOSTSsecret, which is why prod is unaffected):Originagainstreq.nextUrl.origin, which undernext startis the internal listen origin (https://localhost:3000). NowSec-Fetch-Siteis authoritative when present, with anOriginfallback checked againstAUTH_URL/ forwarded host.sslmode=requirewith no CA flipped torejectUnauthorized: true. Now follows libpq semantics: require/prefer encrypt without verifying;verify-ca/verify-fullorDATABASE_CA_CERTverify.MAX_NOTE_BODYUTF-16 chars at worst-case JSON encoding (6 bytes/char), so large multibyte notes stay saveable and editable.img-srcadditionally allows the legacy public-bucket origin (no-op unless S3 env is set) so pre-migration note images keep rendering.AI_METADATA_ENABLED=false) instead of opt-in, so a configuredANTHROPIC_KEYworks again with no new env var.Known follow-up: iOS
AuthClientstill POSTs/api/auth/revokeon sign-out, so native sign-out remains "sign out everywhere".Test plan
npm test— 151 passing (updated tests that encoded the old TLS/AI/legacy-DB behavior; added regressions for the byte cap, opt-out gate, proxy-shaped origin check, and legacy pending-op migration)npm run buildgreennext start: production-shaped mutations (publicOrigin, internal listen origin, with/withoutSec-Fetch-Site) pass to auth; foreign-origin and cross-site requests still 403🤖 Generated with Claude Code