feat: implemented token revocation - #760
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 1089273 The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
Implements token revocation by persisting token IDs (jti) in storage, adding revoked_at markers, and enforcing revocation/expiry checks during token introspection. This closes the gap where decryption alone previously implied validity, and makes project/preview secrets explicitly revocable.
Changes:
- Add
revoked_atto token records across SQLite/Postgres/Spanner and introduce revocation updates (by token ID and by session ID), plus supporting indexes/migrations. - Enforce revocation at verification time by switching API auth from “decrypt only” to
IntrospectToken+ storage lookup for revocable token types. - Make project and preview secrets revocable by issuing them as persisted token records (with stored
jti), and add statement + integration tests pinning the contract.
Reviewed changes
Copilot reviewed 29 out of 32 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/storage/v2/stmttest/token_test.go | Adds cross-dialect revocation contract tests (record survives, idempotent revoke, expiry inactive). |
| internal/storage/v2/dialect/sqlite/token.go | Adds revoke statements, scans revoked_at, and supports nullable user_id for project credentials. |
| internal/storage/v2/dialect/sqlite/session.go | Rotating session tokens now revokes the previous token instead of deleting it. |
| internal/storage/v2/dialect/sqlite/migration/sql/000002_token_revocation.sql | Rebuilds SQLite tokens table to add revoked_at and allow project/preview token types; adds session index. |
| internal/storage/v2/dialect/spanner/token.go | Adds revoke statements and includes revoked_at in reads/schema bindings. |
| internal/storage/v2/dialect/spanner/session.go | Rotating session tokens now revokes the previous token instead of deleting it. |
| internal/storage/v2/dialect/spanner/migration/sql/000018_token_revocation.sql | Adds revoked_at, widens type constraints, and adds a (null-filtered) session index. |
| internal/storage/v2/dialect/postgres/token.go | Adds revoke statements and includes revoked_at in reads/schema bindings. |
| internal/storage/v2/dialect/postgres/session.go | Rotating session tokens now revokes the previous token instead of deleting it. |
| internal/storage/v2/dialect/postgres/session_test.go | Updates rotation test to assert revoked predecessor token record instead of missing token. |
| internal/storage/v2/dialect/postgres/migration/sql/000015_token_revocation.sql | Adds new token type enum values, adds revoked_at, updates constraints, and adds session index. |
| internal/service/token.go | Introduces RevokeToken and IntrospectToken enforcing storage-backed revocation/expiry for revocable tokens. |
| internal/service/statement.go | Extends TokenStatements with revocation methods. |
| internal/service/session.go | Deletes session and revokes all its tokens in a single transaction. |
| internal/service/mocks/token.mock.go | Regenerates token service mock for new interface methods. |
| internal/service/mocks/statement.mock.go | Regenerates statement mocks to include new token revocation methods. |
| internal/domain/tokentype_enumer.go | Updates generated enum string tables for new token types. |
| internal/domain/token.go | Adds RevokedAt, Active(now), IsRevocable(), and ErrTokenRevoked; extends validation and fields. |
| internal/domain/token_type.go | Adds project token types and marks them persistable. |
| internal/domain/project.go | Switches from “encrypt secret directly” to producing token payloads for project/preview credentials. |
| internal/api/security.go | Uses IntrospectToken for auth, enabling revocation enforcement for requests. |
| internal/api/security_test.go | Updates tests to expect IntrospectToken calls. |
| internal/api/project.go | Issues project/preview secrets via token service (persist + encrypt), making them revocable. |
| internal/api/integration_test/token_revocation_test.go | Adds integration coverage proving revoked session tokens and revoked project secrets are rejected. |
| internal/api/integration_test/session_me_test.go | Issues project secret via token service (persisted token), aligning with revocable credentials. |
| internal/api/integration_test/helpers/token.go | Wires token service with statement pool in integration harness. |
| internal/api/integration_test/helpers/client.go | Updates client helpers to issue secrets via token service rather than project methods. |
| internal/api/error_handler_test.go | Updates tests to expect IntrospectToken calls. |
| cmd/server/server.go | Wires token service with statement pool; passes token service into runtime resolver. |
| cmd/server/console_runtime.go | Uses token service to mint preview publishable key as a revocable token. |
| cmd/server/console_runtime_test.go | Updates runtime resolver tests to mock token service instead of key service/crypter. |
| .changeset/token-revocation-jti.md | Adds server changeset describing revocation behavior and compatibility notes. |
Files not reviewed (3)
- internal/domain/tokentype_enumer.go: Generated file
- internal/service/mocks/statement.mock.go: Generated file
- internal/service/mocks/token.mock.go: Generated file
| } | ||
| publishableKey, err := project.PreviewSecret(tokenCrypter) | ||
| tkn := project.PreviewToken() | ||
| publishableKey, err := tokens.GenerateJWE(ctx, tkn) |
There was a problem hiding this comment.
This line now writes a database row on every HTTP request, which I don't think is intended.
Before this PR, building the publishable key was just encryption: project.PreviewSecret(crypter) serialized a token and encrypted it, touching no storage. tokenService.GenerateJWE inserts a row into tokens first, because that insert is what mints the jti that makes the secret revocable.
The problem is where this particular call sits. This resolver runs once per request to GET /console/runtime.json (see the comment on line 46: the document is deliberately recomputed per request so a newly created default project shows up without a restart). That route is registered directly on the http.ServeMux at server.go line 460, above the catch-all mount that carries the API middleware, so it is reachable without any credentials.
So every console page load, and every anonymous request to that URL, adds a project_preview row. Nothing removes them: project.PreviewToken() sets no ExpiresAt (project.go line 79), so even the expires_at sweeper the changeset describes as follow-up work would never collect them.
There is a second effect worth calling out: because each request mints a fresh jti, every previously issued publishable key stays independently valid. Revoking a leaked one only retires the single key from that one request, not the project's publishable key.
Suggested fix: mint the publishable key once per project and reuse it (resolve it lazily and cache it, or store it on the project), keeping the per-request refresh for ConsoleProjectID only. That keeps the key revocable, which is the point of the change, without an insert per request.
One note on test coverage: console_runtime_test.go mocks GenerateJWE, so no existing test would notice the extra writes.
|
|
||
| -- +goose Up | ||
| -- +goose StatementBegin | ||
| CREATE TABLE tokens_new ( |
There was a problem hiding this comment.
Pre-release migrations are modified in place in this repo rather than appended to, so these three new files probably belong folded into the migrations that created tokens: 000009_tokens.sql for postgres/spanner and 000001_init.sql for sqlite.
Precedent: #740 (04e77a71) folded claim_challenges into sqlite's squashed init, stating "pre-release migrations are modified in place"; #637 (7ea32f82) edited 000007_user_agents_and_sessions.sql, #649 (4b984afb) edited 000012_crypto_keys.sql, and #646 (fd31b20c) edited 000002_teams.sql.
That matters here because all three files only widen the existing CHECK constraint and add one index. Folding removes this 135-line table rebuild entirely, along with the ALTER TYPE ... ADD VALUE / NO TRANSACTION sequencing in 000015_token_revocation.sql#L6-L17 and both Down sections.
There was a problem hiding this comment.
Ok what I would do is:
- Extract the SQL migration from
init.sqlto a separate file - Modify that extracted code to match the new structure of the tokens table
| return s.v2Pool.Transaction(ctx, func(ctx context.Context, tx Statementer[AllStatements]) error { | ||
| err := tx.Statements().DeleteSessionByID(ctx, input.ProjectID, input.SessionID) | ||
| if err != nil { | ||
| if errors.Is(err, domain.ErrSessionNotFound()) { |
There was a problem hiding this comment.
This early return nil fires before DeleteTokensBySessionID on line 133, so a session whose row is already gone keeps its token records, and with revocation now modeled as record deletion, those tokens stay valid. Falling through to the token delete preserves the idempotency added in #768 (fa907c22), since deleting by session id is already a no-op on zero rows.
Separately, the sentinel check on line 135 looks like copy-paste: DeleteTokensBySessionID never returns ErrSessionNotFound, and if it ever did, a genuine failure to revoke would be swallowed as success.
| these credentials have no expiry of their own. | ||
|
|
||
| Expired records are never honoured — verification checks `expires_at` too. There | ||
| is no background sweeper yet, so records that expired without being revoked |
There was a problem hiding this comment.
#511 lists "add a revocation store keyed by jti/signature with expiry-based cleanup" as a task, so deferring the sweeper leaves that acceptance item open. The rest of #511 is met: jti is the revocation key, nothing hashed or full-token is stored, and IntrospectToken rejects a revoked id.
Worth filing the follow-up and linking it here so #511 can be closed against something concrete. Note that an expires_at-based sweeper alone would not bound the table given the per-request preview tokens flagged in cmd/server/console_runtime.go. Those carry no expires_at at all.
| mock := gomock.NewController(t) | ||
| tokenService := mocks.NewMockTokenService(mock) | ||
| tokenService.EXPECT().VerifyToken(gomock.Any(), gomock.Any()).Return(nil, errors.New("bad token")) | ||
| tokenService.EXPECT().IntrospectToken(gomock.Any(), gomock.Any()).Return(nil, errors.New("bad token")) |
There was a problem hiding this comment.
Might be worth switching gomock.Any() on the second argument to http.Cookie.Value to make a stronger verification
| mock := gomock.NewController(t) | ||
| tokenService := mocks.NewMockTokenService(mock) | ||
| tokenService.EXPECT().VerifyToken(gomock.Any(), gomock.Any()).Return(token, nil) | ||
| tokenService.EXPECT().IntrospectToken(gomock.Any(), gomock.Any()).Return(token, nil) |
There was a problem hiding this comment.
Same comment as before for all the calls to IntrospectToken: can change second argument gomock.Any() to the actual expected value
Summary
Validation
go test ./...go test -tags postgres_integration ./...go test -tags spanner_integration -parallel 1 ./...Release notes / changeset
Notes