fix: treat undecryptable user rows as not-found#208
Merged
Conversation
When pool.query returns a row whose tokens can't be decrypted (e.g. a legacy plaintext row left over from before encryption-at-rest, or any row encrypted under a since-rotated key), toUser() throws inside findUserBySpotifyId. That throw bubbles through passport's deserializeUser, which runs on every authenticated request — so the user 500s on every endpoint they hit, including /api/auth/user/logout. The cookie holds them in a state they can't escape from in-app. Catch the decrypt error and return null instead. Passport sees no user, treats the session as anonymous, and the next interaction redirects to OAuth, where findOrCreateUser's ON CONFLICT … DO UPDATE rewrites the row with freshly-encrypted tokens. We log a warning so the recovery is visible in droplet logs.
Mirror the logger.warn spy pattern from tokenCrypto.test.ts so the recovery is visibly logged. Without this assertion the test only covered the return value, leaving the warn() call (which is how the condition would surface in droplet logs) uncovered.
cdtinney
force-pushed
the
cdtinney/skip-bad-tokens
branch
from
0f4c168 to
52f0476
Compare
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
Follow-up to #207. When
pool.queryreturns ausersrow whose tokens can't be decrypted,toUser()throws insidefindUserBySpotifyId. That throw bubbles through passport'sdeserializeUser, which runs on every authenticated request — so the user 500s on every endpoint, including/api/auth/user/logout. The cookie holds them in a state they can't escape from in-app, and the only recovery is clearing cookies in DevTools or wiping thesessiontable on the droplet (#207's script).This catches the decrypt error in
findUserBySpotifyIdand returnsnullinstead. Passport sees no user, treats the session as anonymous, and the next interaction redirects to OAuth, wherefindOrCreateUser'sON CONFLICT … DO UPDATErewrites the row with freshly-encrypted tokens. A warning is logged so the recovery is visible in droplet logs.How a row gets into this state:
TOKEN_ENCRYPTION_KEY—SUPABASE.mdalready documents that rotating the key forces re-auth; this PR makes the re-auth happen automatically instead of via 500s.The fix only touches the read path.
findOrCreateUserandupdateUserAccessTokenBySpotifyIdwrite freshly-encrypted tokens before callingtoUseron theRETURNINGrow, so they can't hit this path in normal flow and don't need the same treatment.Testing
userQueries.test.tscovers three cases: no row → null, valid row → user, decrypt-throws → null.pnpm vitest runserver suite: 53 tests pass (was 50 before this PR; the 3 pre-existing test-file load failures are unrelated to this change — they fail onmaintoo, due topassport-spotifyrequiring credentials at strategy construction).pnpm typecheckclean.pnpm lintclean for new files.