security(oidc): stop shipping a hardcoded cookie key and reflecting all CORS origins#8070
Open
JohnMcLear wants to merge 2 commits into
Open
security(oidc): stop shipping a hardcoded cookie key and reflecting all CORS origins#8070JohnMcLear wants to merge 2 commits into
JohnMcLear wants to merge 2 commits into
Conversation
…ing all CORS origins The embedded OIDC provider signed its interaction/session/grant cookies with the committed literal key `['oidc']`, so anyone with the public source could forge valid `.sig` cookies (defeating the provider's cookie-integrity boundary), and `clientBasedCORS` returned `true` for every origin, reflecting arbitrary `Origin` values into `Access-Control-Allow-Origin` on the token/userinfo endpoints. Adds OidcProviderSecurity.ts with two unit-tested pure helpers: - resolveOidcCookieKeys(): prefers an operator-supplied `settings.sso.cookieKeys` (ordered array for rotation), otherwise derives a secret key from the persisted session secret via a domain-separated SHA-256 — stable across restarts and multi-pod, never committed to source; falls back to an ephemeral random key. - isOriginAllowedForOidcClient(): allows a CORS origin only when it matches the origin of one of the client's registered redirect URIs. Verified end-to-end against a real oidc-provider@9.9.1 instance (cookies.keys accepted, Client.redirectUris read correctly, attacker origin blocked). Reported privately by meifukun. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
PR Summary by QodoHarden embedded OIDC provider cookie signing and CORS origin validation
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
967
to
977
| "sso": { | ||
| "issuer": "${SSO_ISSUER:http://localhost:9001}", | ||
| /* | ||
| * Optional signing keys for the embedded OIDC provider's cookies. When | ||
| * omitted, Etherpad derives a secret key from the persisted session | ||
| * secret (SESSIONKEY.txt), which is stable across restarts and shared | ||
| * across horizontally-scaled pods. Set an explicit ordered array to | ||
| * rotate: the first key signs, the rest are still accepted for verify. | ||
| * "cookieKeys": ["${OIDC_COOKIE_KEY:}"], | ||
| */ | ||
| "clients": [ |
There was a problem hiding this comment.
2. Template comment misattached 🐞 Bug ⚙ Maintainability
settings.json.template documents sso.cookieKeys but does not include an actual cookieKeys property, so the comment block becomes the leading comment for the next real key (sso.clients) in the Admin UI’s template-comment extraction. This can mis-document sso.clients and make cookieKeys harder to discover/configure correctly.
Agent Prompt
### Issue description
The `cookieKeys` documentation block is inserted above `sso.clients`, but there is no actual `"cookieKeys": ...` key in the template. The Admin UI’s comment extractor associates leading comments with the next property node, so this block will be shown for `sso.clients`.
### Issue Context
Admin settings comments are extracted from `settings.json.template` using `jsonc-parser` node offsets and adjacent comment lookup.
### Fix Focus Areas
- settings.json.template[967-977]
- admin/src/components/settings/templateComments.ts[17-37]
- admin/src/components/settings/comments.ts[33-70]
### Implementation direction
- Add a real `"cookieKeys"` property in `settings.json.template` (for example: `"cookieKeys": ["${OIDC_COOKIE_KEY:}"],`) and place the comment immediately above it.
- Keep `resolveOidcCookieKeys()`’s existing filtering so an empty env var value safely falls through to the derived/persisted behavior.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Pull request overview
This PR hardens Etherpad’s embedded OIDC provider by removing a hardcoded cookie-signing key and by restricting OIDC CORS responses to only origins that match a client’s registered redirect URI origins.
Changes:
- Introduces
resolveOidcCookieKeys()to derive OIDC cookie-signing keys from operator-provided settings or from the persisted session secret, avoiding committed key material. - Introduces
isOriginAllowedForOidcClient()to replace unconditional CORS origin reflection with exact origin matching against registered redirect URIs. - Adds backend unit tests to cover both helpers and updates settings types/template docs to expose the new
sso.cookieKeysoption.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/node/security/OAuth2Provider.ts | Removes hardcoded cookie key and replaces permissive CORS callback with helper-based allow-listing. |
| src/node/security/OidcProviderSecurity.ts | Adds dependency-light, unit-testable helpers for cookie key resolution and CORS origin checks. |
| src/tests/backend/specs/OidcProviderSecurity.ts | Adds unit tests covering cookie key derivation behavior and exact-origin CORS allow logic. |
| src/node/utils/Settings.ts | Extends SettingsType with optional sso.cookieKeys for operator-supplied key rotation. |
| settings.json.template | Documents optional sso.cookieKeys configuration in the default template. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+31
to
+35
| * any pod — without ever committing key material to source. | ||
| * 3. As a last resort (no session key available), an ephemeral per-process | ||
| * random key. The integrity boundary holds, but interactions won't survive | ||
| * a restart or span multiple pods. | ||
| */ |
…ault rotation config Addresses Qodo review on #8070. With Etherpad's default cookie settings (keyRotationInterval + sessionLifetime set), key rotation is enabled and `settings.sessionKey` stays null unless SESSIONKEY.txt is provisioned, so the previous fallback handed the OIDC provider a per-process random key — breaking in-flight OIDC cookies on restart and across horizontally-scaled pods on a default install. resolveOidcCookieKeys() now takes an optional rotatedSecrets array (priority: operator cookieKeys > rotated secrets > session-key derivation > random) and returns it by reference so a live rotation propagates to keygrip. OAuth2Provider.expressCreateServer() creates a dedicated `oidcCookieSecrets` SecretRotator — the same DB-backed mechanism the Express session cookies use — when the operator hasn't pinned settings.sso.cookieKeys and rotation is enabled. Adds unit tests for the rotatedSecrets priority/by-reference behavior and an integration test proving the default config yields stable DB-backed secrets rather than a random key. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
|
Thanks @qodo-free-for-open-source-projects — good catch. Confirmed: with default |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Comment on lines
304
to
311
| sso: { | ||
| issuer: string, | ||
| clients?: {client_id: string}[] | ||
| // Optional operator-supplied signing keys for the embedded OIDC provider's | ||
| // cookies. When unset, a secret key is derived from the session secret. | ||
| // Provide an ordered array `[newKey, ...oldKeys]` to rotate. | ||
| cookieKeys?: string[] | ||
| }, |
Comment on lines
+969
to
977
| /* | ||
| * Optional signing keys for the embedded OIDC provider's cookies. When | ||
| * omitted, Etherpad derives a secret key from the persisted session | ||
| * secret (SESSIONKEY.txt), which is stable across restarts and shared | ||
| * across horizontally-scaled pods. Set an explicit ordered array to | ||
| * rotate: the first key signs, the rest are still accepted for verify. | ||
| * "cookieKeys": ["${OIDC_COOKIE_KEY:}"], | ||
| */ | ||
| "clients": [ |
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
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.
Fixes two issues in the embedded OIDC provider (
src/node/security/OAuth2Provider.ts), reported privately by meifukun (credit requested asmeifukun, https://github.com/meifukun). Verified against 3.3.2 @9a6109b; both lines are unchanged on currentdevelop.1. Hardcoded OIDC cookie signing key (
['oidc'])The provider signed its interaction/session/grant cookies (
_interaction,_interaction_resume,_grant,_session) with the committed literal keyoidc, so anyone with the public source could compute valid.sigcookie signatures and defeat the provider's cookie-integrity boundary.Fix:
resolveOidcCookieKeys()prefers an operator-suppliedsettings.sso.cookieKeys(ordered array for rotation), otherwise derives a secret key from the persisted session secret (SESSIONKEY.txt) via a domain-separated SHA-256 — stable across restarts and horizontally-scaled pods, never committed to source. Falls back to an ephemeral random key when no session secret exists.2.
clientBasedCORSreturnedtruefor every originThe token/userinfo endpoints reflected any
OriginintoAccess-Control-Allow-Origin, letting unregistered origins read responses that use non-cookie credentials (Authorization headers, POST-body client secrets).Fix:
isOriginAllowedForOidcClient()allows a CORS origin only when it matches the origin of one of the client's registered redirect URIs.Tests / verification
OidcProviderSecurity.ts— 14 unit tests over both pure helpers (stable derivation, no hardcoded key, exact-origin matching, look-alike/scheme-mismatch rejection).oidc-provider@9.9.1instance: cookies.keys accepted,Client.redirectUrisread correctly, attacker origin blocked.tsc --noEmitclean.A private GHSA draft tracks both issues.
🤖 Generated with Claude Code