fix(auth): remove hardcoded fallback session cookie signing secret - #1463
Open
mukktinaadh wants to merge 1 commit into
Open
mukktinaadh wants to merge 1 commit into
mukktinaadh wants to merge 1 commit into
Conversation
`AUTH_COOKIE_SECRET` fell back to the committed literal `"your_cookie_signing_secret"` whenever the environment variable was unset. That value signs the session cookie JWT in `_encode_session_cookie` and verifies it in `_decode_session_cookie`, so a deployment running without the variable accepts session cookies forged with a key that is public in the source tree. Verified against this commit: before the change, a cookie signed with the committed literal decoded to a valid `Session`; afterwards the same cookie raises `AuthException`. The literal now appears only in the regression test, to prove it can no longer verify anything. - Read `AUTH_COOKIE_SECRET` with no default. - When unset, generate an ephemeral per-process secret via `secrets.token_hex(32)` and warn. Local development keeps working without a committed signing key; sessions signed with the ephemeral value are dropped on restart and are not shared across replicas, which the warning states explicitly. - Drop the now-dead "unsafe AUTH_COOKIE_SECRET" check. Fixes AgentOps-AI#1446
This branch has not been deployed
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.
Fixes #1446.
Problem
AUTH_COOKIE_SECRETfell back to a committed literal:That value signs the session cookie JWT (
_encode_session_cookie) and verifies it(
_decode_session_cookie). Any deployment running without the variable set thereforeaccepted session cookies signed with a key that is public in the source tree.
Verification
Against the parent commit, a cookie forged with the committed literal and naming a
session that exists decoded to that valid
Session:After the change, the same cookie raises
AuthException, and a cookie signed with theconfigured secret still round-trips.
Changes
AUTH_COOKIE_SECRETis read from the environment with no default.secrets.token_hex(32)and a warning is logged. Local development keeps working with no committed signing
key; cookies signed with the ephemeral value are dropped on restart and are not valid
across replicas, which the warning states explicitly.
This is fail-safe rather than fail-closed: a missing variable in production now means
sessions do not survive a restart, instead of a silently known signing key. If you would
rather the process refuse to start when the variable is missing, that is a small change
on top and I am happy to switch it.
Tests
New
app/api/tests/auth/test_session_cookie.py, 6 tests:Nonesession_idruff checkandruff format --checkpass on both files.One caveat on execution:
tests/_conftest/clickhouse.pydefines a session-scopedautousefixture that builds the ClickHouse image via Docker, and Docker is unavailablein my environment. I verified the behaviour each test asserts directly against the module
(see Verification above) and confirmed all six are collected, but I have not executed the
file under
pytestlocally. CI should be able to, sinceis_github_actions()skips theDocker build.