unkin/jellyfin-plugin-sso
4
0
Fork 0
forked from mirrors/jellyfin-plugin-sso
Code Pull Requests Activity

Back the SSO OAuth state with the shared cache #3

Merged
benvin merged 4 commits from benvin/valkey-oauth-state into main 2026-09-19 23:38:48 +10:00
Conversation 3 Commits 4 Files Changed 17
+1371 -61
unkin-agent commented 2026-09-19 22:47:52 +10:00
Member
Copy Link
Copy Source

The OAuth state lived in a per-process dictionary, so a login whose provider callback landed on a different replica than its /sso/OID/start failed with "Invalid or expired state".

  • add ISsoStateStore, backed by the server's IConnectionMultiplexer with a bounded in-memory fallback
  • key flows on jellyfin:sso:oid:state:<state> under a 300s TTL, redeemed once on auth
  • discard a rejected state instead of extending its TTL
  • turn an unreachable store into a logged error page, not an unhandled 500
  • drop the PKCE code verifier from /sso/OID/States
  • add tests/SSO-Auth.Tests

Part of unkin/jellyfin-ha-src#27

The OAuth state lived in a per-process dictionary, so a login whose provider callback landed on a different replica than its `/sso/OID/start` failed with "Invalid or expired state". - add `ISsoStateStore`, backed by the server's `IConnectionMultiplexer` with a bounded in-memory fallback - key flows on `jellyfin:sso:oid:state:<state>` under a 300s TTL, redeemed once on auth - discard a rejected state instead of extending its TTL - turn an unreachable store into a logged error page, not an unhandled 500 - drop the PKCE code verifier from `/sso/OID/States` - add `tests/SSO-Auth.Tests` Part of unkin/jellyfin-ha-src#27
unkin-agent commented 2026-09-19 22:57:37 +10:00
Author
Member
Copy Link
Copy Source
  • SSO-Auth/State/InMemorySsoStateStore.cs:16 — entries expire only when that exact key is next Get/Consume/List'd and Invalidate() was deleted, so abandoned flows from the unauthenticated /sso/OID/start/{p} accumulate forever in the fallback store → add a sweep (or a cap) so abandoned states are reclaimed without a later lookup.
  • SSO-Auth/SSO-Auth.csproj:37 — ExcludeAssets="runtime" binds to whatever StackExchange.Redis the server's ALC holds, but this compiles against 2.13.17: StringGetDeleteAsync needs >=2.6.66 and the keepTtl StringSetAsync overload >=2.5, so an older server assembly is a runtime MissingMethodException → pin to the version jellyfin-ha-src actually ships.
  • SSO-Auth/State/RedisSsoStateStore.cs:105 — Valid/Admin/Username plus the PKCE verifier now sit in a passwordless, TLS-less, shared valkey while /sso/OID/Auth/{provider} is unauthenticated, so anything that can read the key can mint a session including an admin one → require AUTH+TLS on that valkey, or state the accepted risk in the PR body.
  • SSO-Auth/Api/SSOController.cs:127,343,433,507,535,1133 — no store call is guarded, so a valkey outage or timeout surfaces as an unhandled exception on every SSO leg instead of the plugin's ReturnError → catch RedisException/TimeoutException and fail closed with a logged message.
  • SSO-Auth/Api/SSOController.cs:343 — the write-back runs before the Valid check, so a state that just failed the role check is rewritten with a fresh 300s TTL → move it into the Valid branch and drop the state on the 401 path.
  • nit: SSO-Auth/SsoAuthServiceRegistrator.cs:30 — the in-memory fallback silently re-creates the multi-replica bug this PR fixes; log it at LogWarning.
  • nit: .woodpecker/ci.yaml:16 — "No test project exists yet; this is a no-op until one lands" is stale now that tests/SSO-Auth.Tests exists.
- `SSO-Auth/State/InMemorySsoStateStore.cs:16` — entries expire only when that exact key is next Get/Consume/List'd and `Invalidate()` was deleted, so abandoned flows from the unauthenticated `/sso/OID/start/{p}` accumulate forever in the fallback store → add a sweep (or a cap) so abandoned states are reclaimed without a later lookup. - `SSO-Auth/SSO-Auth.csproj:37` — `ExcludeAssets="runtime"` binds to whatever StackExchange.Redis the server's ALC holds, but this compiles against 2.13.17: `StringGetDeleteAsync` needs >=2.6.66 and the `keepTtl` `StringSetAsync` overload >=2.5, so an older server assembly is a runtime `MissingMethodException` → pin to the version jellyfin-ha-src actually ships. - `SSO-Auth/State/RedisSsoStateStore.cs:105` — `Valid`/`Admin`/`Username` plus the PKCE verifier now sit in a passwordless, TLS-less, shared valkey while `/sso/OID/Auth/{provider}` is unauthenticated, so anything that can read the key can mint a session including an admin one → require AUTH+TLS on that valkey, or state the accepted risk in the PR body. - `SSO-Auth/Api/SSOController.cs:127,343,433,507,535,1133` — no store call is guarded, so a valkey outage or timeout surfaces as an unhandled exception on every SSO leg instead of the plugin's `ReturnError` → catch `RedisException`/`TimeoutException` and fail closed with a logged message. - `SSO-Auth/Api/SSOController.cs:343` — the write-back runs before the `Valid` check, so a state that just failed the role check is rewritten with a fresh 300s TTL → move it into the `Valid` branch and drop the state on the 401 path. - nit: `SSO-Auth/SsoAuthServiceRegistrator.cs:30` — the in-memory fallback silently re-creates the multi-replica bug this PR fixes; log it at `LogWarning`. - nit: `.woodpecker/ci.yaml:16` — "No test project exists yet; this is a no-op until one lands" is stale now that `tests/SSO-Auth.Tests` exists.
unkin-agent commented 2026-09-19 23:17:08 +10:00
Author
Member
Copy Link
Copy Source
  • SSO-Auth/State/InMemorySsoStateStore.cs:123 — _entries.OrderBy(...).Take(excess) buffers the ConcurrentDictionary through ICollection.Count + CopyTo, which is not atomic: a concurrent write makes Evict() throw NullReferenceException (dictionary shrank, trailing default KeyValuePairs) or ArgumentException (it grew). Reproduced with two writers once the store is at Capacity; Evict() runs on every CreateAsync/UpdateAsync, so concurrent logins past the cap fail with the 503 page → take _entries.ToArray() (the dictionary's own atomic snapshot, not the LINQ extension) and order that.
  • nit: SSO-Auth/State/InMemorySsoStateStore.cs:123 — every state carries the same 300s TTL, so "nearest to expiry" is "oldest": 1025 anonymous GET /sso/OID/start/{p} calls evict every live in-flight login, which then fails with "Invalid or expired state". Reject the new write at the cap instead, or record in the class doc that the cap trades a memory DoS for a login DoS.
  • nit: tests/SSO-Auth.Tests/InMemorySsoStateStoreTests.cs:96 — Writing_BoundsTheStoreAtItsCapacity writes serially and never exercises Evict() under concurrency, which is how the first finding got through → add a test that writes from several tasks with the store over Capacity.
- `SSO-Auth/State/InMemorySsoStateStore.cs:123` — `_entries.OrderBy(...).Take(excess)` buffers the `ConcurrentDictionary` through `ICollection.Count` + `CopyTo`, which is not atomic: a concurrent write makes `Evict()` throw `NullReferenceException` (dictionary shrank, trailing default `KeyValuePair`s) or `ArgumentException` (it grew). Reproduced with two writers once the store is at `Capacity`; `Evict()` runs on every `CreateAsync`/`UpdateAsync`, so concurrent logins past the cap fail with the 503 page → take `_entries.ToArray()` (the dictionary's own atomic snapshot, not the LINQ extension) and order that. - nit: `SSO-Auth/State/InMemorySsoStateStore.cs:123` — every state carries the same 300s TTL, so "nearest to expiry" is "oldest": 1025 anonymous `GET /sso/OID/start/{p}` calls evict every live in-flight login, which then fails with "Invalid or expired state". Reject the new write at the cap instead, or record in the class doc that the cap trades a memory DoS for a login DoS. - nit: `tests/SSO-Auth.Tests/InMemorySsoStateStoreTests.cs:96` — `Writing_BoundsTheStoreAtItsCapacity` writes serially and never exercises `Evict()` under concurrency, which is how the first finding got through → add a test that writes from several tasks with the store over `Capacity`.
unkin-agent commented 2026-09-19 23:32:01 +10:00
Author
Member
Copy Link
Copy Source

No findings.

No findings.
benvin changed target branch from benvin/jellyfin-12-abi to main 2026-09-19 23:37:19 +10:00
benvin added 4 commits 2026-09-19 23:37:19 +10:00
store OAuth state in the shared cache
ci/woodpecker/pr/ci Pipeline was successful
Details
a167d00b73 
Keys each in-flight OpenID flow in the server's Valkey/Redis connection so a login
can start on one replica and finish on another. Falls back to an in-memory store
when no multiplexer is registered.
keep solution file line endings
ci/woodpecker/pr/ci Pipeline was successful
Details
5d446c16f2
bound the in-memory state store and guard store failures
ci/woodpecker/pr/ci Pipeline was successful
Details
e71df7aa36 
Drop a rejected state instead of refreshing its TTL, and turn a store
outage into the plugin's error page rather than an unhandled 500.
snapshot the state store atomically before evicting
ci/woodpecker/pr/ci Pipeline was successful
Details
e9bc573e7d 
LINQ buffering of a ConcurrentDictionary is not atomic, so a concurrent write made Evict() throw and fail the login.
benvin merged commit 040cffe967 into main 2026-09-19 23:38:48 +10:00
benvin deleted branch benvin/valkey-oauth-state 2026-09-19 23:38:49 +10:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No Label
Milestone
No items
No Milestone
Assignees
Clear assignees
benvin (BenVincent) unkin-agent (Unkin Agent) unkinben (Ben Vincent)
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: unkin/jellyfin-plugin-sso#3
Delete Branch "benvin/valkey-oauth-state"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?