fix(auth): make user authority revocation atomic - #800
Conversation
|
Warning Review limit reachedNext included review available in 23 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 94 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (20)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2054632d21
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| AND (NOT $3 OR authorizer.role = 'admin') | ||
| AND subject.enabled | ||
| AND subject.auth_revision = $5 | ||
| FOR UPDATE OF authorizer, subject |
There was a problem hiding this comment.
Lock the source session while minting delegated credentials
If Logout or RevokeSession updates the source session after this query reads it but before the creation transaction commits, that revocation can commit because source is not one of the locked rows; the transaction can then still create an API key, device-login session, or impersonation session from authority that was already revoked. Those derived credentials remain independently valid, so include the source session in the FOR UPDATE lock, with consistent lock ordering, to serialize revocation and credential creation.
Useful? React with 👍 / 👎.
| if claims.TokenType == auth.TokenTypeAPIKey || claims.SessionID == "" || claims.ImpersonatorUserID != nil { | ||
| writeError(w, http.StatusForbidden, "forbidden", "API key management requires an ordinary native session") |
There was a problem hiding this comment.
Update the API-key contract for ordinary sessions
When an impersonated access JWT, or another JWT without a native session_id, calls an API-key management endpoint, this new condition returns 403; however, docs/api-keys-api.md lines 45–48 still state that a JWT access token is sufficient and identify only API-key authentication as rejected. Update that API document with the ordinary, non-impersonated session requirement and its failure response so third-party clients do not implement against an inaccurate contract.
AGENTS.md reference: AGENTS.md:L127-L135
Useful? React with 👍 / 👎.
Problem
Related issue: N/A — audit-validated security fix
Sensitive user changes updated
usersbefore separately revoking native and Jellyfin sessions. A database failure or concurrent credential grant could therefore leave a role downgrade, disable, or password reset committed while an older access token, refresh token, impersonation session, device-login grant, or Jellyfin credential remained usable. Jellyfin's per-process cache also let another replica continue serving a cached session after local invalidation.Approach
Add a monotonic
users.auth_revisionauthority epoch and snapshot it into native sessions. Native middleware, refresh, impersonation, and Jellyfin session validation now compare the snapshot with current subject and impersonator state.Sensitive admin mutations run the user update, native owner/impersonator revocation, and Jellyfin persistence cleanup in one database transaction. Credential creation that can cross the middleware boundary—API keys, impersonation sessions, and device-login polling—revalidates the non-impersonated authorizing session while locking the same user rows, so it is ordered with an authority reset.
Jellyfin persistence now binds each compat session to its native session. Cache hits validate shared database state before use, and guarded upserts cannot make a revoked session usable again on another replica.
The migration expires existing Jellyfin sessions once. Their encrypted native tokens do not expose enough information for SQL to backfill the native session IDs safely.
Validation
Repository gate on the complete diff:
make embed-stub— passedgo build ./...— passedgofmt -l .— passed with no outputgo vet ./...— passedgolangci-lint run --new-from-merge-base="origin/main" ./...with the CI-pinned v2.12.2 binary — passed, 0 issuesmake test-goon the exact Linux candidate source — passedpnpm install --frozen-lockfile— passedpnpm run lint— passed with existing warnings onlypnpm run format:check— passedpnpm run build— passed with existing font/chunk-size warnings onlymake test-web— passed: 294 files, 2,184 testsmake migrate-validate— passedmake verify-settings-bindings-all— passedmake verify-playback-fixtures— passedmake verify-local-paths— passedSecurity regressions on isolated PostgreSQL, including
-race, passed for role downgrade, disable, password reset, mutation/revocation rollback injection, stale session insertion, device-login grants, API-key creation after middleware authorization, impersonation creation, and two independent Jellyfin replica pools.The exact candidate was built and deployed in the isolated
audit-session-revokesandbox.doctorpassed container, database, API, and frontend checks. A synthetic-user HTTP probe verified:The sandbox and its temporary probe were removed after validation.
On macOS,
make test-gostill fails the two inheritedinternal/jellycompatprocess-lock identity tests because the implementation cannot read a Linux-style process start token there. Those files are unchanged. Both tests and the complete suite pass on the exact Linux candidate used above.Risks
AI Disclosure
Checklist