fix(collegemap): read the session signing secret from the environment - #398
Open
returnsvoidjanet wants to merge 1 commit into
Open
fix(collegemap): read the session signing secret from the environment#398returnsvoidjanet wants to merge 1 commit into
returnsvoidjanet wants to merge 1 commit into
Conversation
The session cookie is base64(userId + ":" + sha256(userId + SESSION_SECRET)), and SESSION_SECRET was a literal in this file. This repo is public, so that string was a published signing key: anyone with a user id could mint a valid session for that user. The user ids are not secret either - the calendar page renders every one of them to anonymous visitors. Read it from $env/dynamic/private instead, with no fallback value. A default would silently recreate exactly this bug on any host that forgot to set it, and an app that refuses to boot is a much better failure than one that boots forgeable. It is narrowed through a function returning string so the type is honest rather than asserted. CI needs it too: the postbuild analyse step imports the server modules, which is why the build job already writes DATABASE_URL. A build-time placeholder is enough there. Deploying this invalidates every existing session - the old cookies were signed with the old secret - so everyone is logged out once.
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.
Closes a live authentication bypass on college.petalcat.dev.
The bug
auth.tssigned session cookies with a hardcoded constant:The cookie is
base64(userId + ":" + sha256(userId + SESSION_SECRET)). This repo is public, so that constant is a published signing key — a user id plus that string is a valid session for that user. And the user ids are not secret either: the anonymous/calendarpage renders every one of them.So anyone could sign in as any of the 14 accounts, with no password, from the public internet. Verified rather than inferred — the constant has no env override anywhere in the app, and the ids really are in the anonymous HTML.
The fix
Read it from
$env/dynamic/private, with no default. A fallback string would silently recreate the same bug on any host that forgot to set it, and an app that refuses to boot is a much better failure than one that boots forgeable. It is narrowed through a function returningstringso the type is honest rather than!-asserted.The CI build job now writes a build-time placeholder, because SvelteKit's postbuild
analysestep imports server modules — the same reason that step already writesDATABASE_URL.Verification
Both directions, not just the happy one:
pnpm --filter collegemap buildwith the var set builds clean.check,lint:impeccable,test,manypkg,typesync:check,lint:knip,lint:knip:prod,vp dedupe --check— all green.Deploying this
The container needs
SESSION_SECRETin its environment or it will not start. That is intended. Generate it on the box withopenssl rand -base64 48; it never belongs in the repo.Everyone gets logged out once, because the old cookies were signed with the old secret. Worth knowing given that nobody remembers their password — account claiming is the separate piece of work.
Not in scope here
The cookie is still
secure: false, passwords are still single-round salted SHA-256, and signup still caps passwords at 4 to 8 characters. Those are real and they are next. This PR is the one that stops the bleeding.