Sweep expired authorization sessions so abandoned PKCE verifiers do not persist - #1569
Open
GeiserX wants to merge 4 commits into
Open
Sweep expired authorization sessions so abandoned PKCE verifiers do not persist#1569GeiserX wants to merge 4 commits into
GeiserX wants to merge 4 commits into
Conversation
An in-flight flow parks its PKCE verifier in oauth_session in plaintext, which is fine while the flow can still spend it. The happy path and cancel delete the row, and an expired redemption drops it lazily — but a completion that FAILED did not, and nothing sweeps the table. A flow that died there kept its verifier indefinitely, and for an abandoned flow the lazy path never runs. restartRequired is the authorization the code already computes for this: false means the caller may redeem the same state again, so deleting then would turn a retryable hiccup into a forced restart. Only the unredeemable case is cleaned up, best-effort, so a failed cleanup cannot replace the real error.
The helper's any-typed parameters widened the Effect error and context channels to unknown, so the suite passed while typecheck failed. Inlining the flow lets the real types flow through.
An abandoned flow is never completed, so the lazy expiry check in complete never runs for it, and nothing else sweeps the table — its PKCE verifier sat there in plaintext indefinitely. Closing that was the larger half of the earlier session cleanup and had been left open for needing host lifecycle work. It does not: sweeping on start costs one delete on a path that is already writing, needs no scheduler in any host, and bounds the table by how often authorization is STARTED rather than by how often it is abandoned. The delete is owner-scoped by the table's own policy, so a caller only ever sweeps rows it can already see, and it is best-effort so tidying up cannot stop someone connecting an account.
This was referenced Aug 12, 2026
Author
|
Context for this one: #1585 explains why this PR and twelve others exist — they came out of a single pass over credential handling, asking for each credential where it ends up, how long it stays, and who can read it once it's there. This PR stands alone and doesn't depend on any of the others. |
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.
TL;DR
An OAuth authorization session stores its PKCE verifier so the callback can redeem the code. Expired sessions were only discarded lazily, on completion — but an abandoned flow is never completed, so that check never ran for it and nothing else swept the table.
The verifier sat in the database in plaintext, indefinitely, for every flow a user started and walked away from.
What happens today
completechecks whether the session has expired and discards it if so. That is the only cleanup in the table. It runs exactly once per flow, and only for flows that come back.Users abandon OAuth flows constantly — they close the popup, deny consent, get distracted at the provider's login page. None of those sessions is ever completed, so none is ever discarded. The table grows without bound and each row holds a live secret.
What this changes
Starting a new authorization sweeps sessions that have already expired.
Doing it on
startrather than on a timer is deliberate:It is owner-scoped by the table's own delete policy, so a caller only ever sweeps rows it can already see, and best-effort — failing to tidy up must never stop someone connecting an account.
Separately: a session whose completion cannot be retried is now dropped rather than left behind. Previously a flow that failed in a way the user would have to restart still left its verifier in place.
What this does not claim
The sweep bounds how long a verifier persists; it does not stop one being stored. A verifier is only useful together with its matching authorization code, which is single-use and short-lived, so the residual value of a swept-late verifier is low. The point is that "low value" and "kept forever" is still a worse position than "low value" and "kept until it expires".
Tests
Two tests: the sweep fires on
startand removes only expired rows, and a non-retryable completion deletes its session. Both mutation-checked.Scope
Independent of #1564 — it touches only
oauth-service.ts.