feat: improve OpenId Configuration fetching to prevent DoS#2748
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPinned Connect dependencies to 1.5.0. createOIDCProvider now checks for an existing OIDC provider and returns ERR_ALREADY_EXISTS if present; request AbortSignal is propagated into provider creation. Keycloak gained an Axios HTTP client with optional webhook proxy and explicit OpenID discovery/error handling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2748 +/- ##
==========================================
+ Coverage 40.72% 47.40% +6.68%
==========================================
Files 985 1062 +77
Lines 123243 143703 +20460
Branches 5639 9767 +4128
==========================================
+ Hits 50187 68125 +17938
- Misses 71381 73831 +2450
- Partials 1675 1747 +72
🚀 New features to boost your workflow:
|
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
controlplane/package.json (1)
48-50: Consider aligning@connectrpcversions across the monorepo.The
@connectrpc/*packages incontrolplaneare pinned to1.5.0, but other workspace packages use different versions:
cli/package.json:1.4.0(exact)studio/package.json:^1.4.0package.json(root):^1.4.0forprotoc-gen-connect-esThis creates the potential for pnpm to resolve and install multiple versions, leading to runtime inconsistencies. While the upgrade to
1.5.0addresses abort signal handling bugs, the version includes breaking changes to error-code mappings and gRPC/gRPC-Web transport behavior—making consistency across the monorepo particularly important.The devDependency
@connectrpc/protoc-gen-connect-esat line 108 also remains at^1.4.0, which may warrant alignment with the other upgraded packages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/package.json` around lines 48 - 50, The `@connectrpc` packages are inconsistent across the monorepo; align all `@connectrpc/`* dependencies to the same version (choose either 1.5.0 or ^1.5.0 consistently) by updating the controlplane entries ("@connectrpc/connect", "@connectrpc/connect-fastify", "@connectrpc/connect-node"), the cli and studio package.json dependencies, and the devDependency "@connectrpc/protoc-gen-connect-es" in the root package.json to that same version string; after updating, run pnpm install to refresh the lockfile and run tests to validate behavior given the 1.5.0 breaking changes to error-code mappings and transport behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controlplane/src/core/services/Keycloak.ts`:
- Around line 569-579: In Keycloak.ts update the error handling in the Axios
catch block (where e.code is inspected) to fix the typo and avoid non-null
assertion: correct the message text to "the discovery" and replace e.code! with
a safe fallback (e.g., use e.code ?? e.message ?? 'Unknown error') so that when
AxiosError.code is undefined the message is meaningful; adjust the
switch/default logic accordingly in the function handling the discovery endpoint
error.
- Around line 14-21: ParsedOpenIdConfiguration currently marks all OIDC
discovery fields optional but createOIDCProvider uses them directly; add runtime
validation in createOIDCProvider to assert required fields (e.g.,
token_endpoint, authorization_endpoint, jwks_uri, issuer, userinfo_endpoint) are
present on the parsed discovery object and throw or return a clear error if any
are missing, or alternatively make those fields required in
ParsedOpenIdConfiguration and perform a defensive check in createOIDCProvider
before constructing the Keycloak identity provider config; reference the
ParsedOpenIdConfiguration type and the createOIDCProvider function and ensure
the validation produces a descriptive error/log and aborts provider creation
when required fields are undefined.
---
Nitpick comments:
In `@controlplane/package.json`:
- Around line 48-50: The `@connectrpc` packages are inconsistent across the
monorepo; align all `@connectrpc/`* dependencies to the same version (choose
either 1.5.0 or ^1.5.0 consistently) by updating the controlplane entries
("@connectrpc/connect", "@connectrpc/connect-fastify",
"@connectrpc/connect-node"), the cli and studio package.json dependencies, and
the devDependency "@connectrpc/protoc-gen-connect-es" in the root package.json
to that same version string; after updating, run pnpm install to refresh the
lockfile and run tests to validate behavior given the 1.5.0 breaking changes to
error-code mappings and transport behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db77cbfb-a2af-4837-9ee7-c2319960cdf7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
controlplane/package.jsoncontrolplane/src/core/bufservices/sso/createOIDCProvider.tscontrolplane/src/core/build-server.tscontrolplane/src/core/services/Keycloak.tscontrolplane/src/core/services/OidcProvider.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
controlplane/src/core/services/Keycloak.ts (1)
549-572:⚠️ Potential issue | 🟠 MajorValidate the discovery document shape before returning it.
typeof null === 'object', so a discovery endpoint that returns literalnullwill pass Line 551 and then crash on Line 301 before yourPublicErrorpath runs. This helper also still returns payloads withoutjwks_uri, even thoughuseJwksUrl: 'true'makes that field effectively required downstream.🛡️ Proposed fix
- let data = response.data; - if (typeof data === 'object') { - // No need to parse the data - } else if (typeof data === 'string') { + let data: unknown = response.data; + if (typeof data === 'string') { try { data = JSON.parse(data); } catch { // Failed to parse the response as JSON throw new PublicError( EnumStatusCode.ERR, 'Failed to parse response from the provided discovery endpoint as JSON.', ); } - } else { - // Invalid response format + } + + if (typeof data !== 'object' || data === null || Array.isArray(data)) { throw new PublicError( EnumStatusCode.ERR, - 'Failed to parse response from the provided discovery endpoint as JSON.', + 'The discovery endpoint must return a JSON object.', ); } - // Cast the data to the expected type - return data as ParsedOpenIdConfiguration; + const config = data as ParsedOpenIdConfiguration; + if ( + typeof config.token_endpoint !== 'string' || + typeof config.authorization_endpoint !== 'string' || + typeof config.jwks_uri !== 'string' + ) { + throw new PublicError( + EnumStatusCode.ERR, + 'The provided OpenID configuration is missing required fields.', + ); + } + + return config;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/services/Keycloak.ts` around lines 549 - 572, The discovery response handling in Keycloak.ts currently treats any object (including null) as valid and may return payloads missing required fields; update the block that processes response.data to (1) explicitly reject null values (e.g., if data === null throw PublicError with EnumStatusCode.ERR and the existing JSON parse error message) and (2) when the downstream config uses useJwksUrl === 'true' ensure the parsed object (ParsedOpenIdConfiguration) contains a non-empty jwks_uri property and throw PublicError(EnumStatusCode.ERR, 'Missing jwks_uri in discovery document.') if absent; keep all other parse/error paths and returns unchanged so callers of the function/class (Keycloak.ts) still receive a validated ParsedOpenIdConfiguration.
🧹 Nitpick comments (1)
controlplane/src/core/services/Keycloak.ts (1)
283-299: Add the explicit return type on this touched public method.
createOIDCProvidernow has a new parameter but still relies on inference. Please annotate it asPromise<void>to match the repo TypeScript rule.♻️ Proposed fix
}: { realm?: string; clientId: string; clientSecret: string; name: string; alias: string; discoveryEndpoint: string; abortSignal?: AbortSignal; - }) { + }): Promise<void> {As per coding guidelines, "Use explicit type annotations for function parameters and return types in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/services/Keycloak.ts` around lines 283 - 299, The public method createOIDCProvider currently lacks an explicit return type; annotate its signature to return Promise<void> (e.g., change the method declaration for createOIDCProvider to include : Promise<void>) so it conforms to the repo TypeScript rule; ensure the async implementation still returns nothing (or only resolves) so the Promise<void> annotation is correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controlplane/src/core/services/Keycloak.ts`:
- Around line 55-61: When options.webhookProxyUrl is provided but creating the
proxy agents (HttpProxyAgent / HttpsProxyAgent) fails, don't silently continue;
update the block that currently catches the exception and logs via
this.logger.error to instead include the original error and rethrow (or
otherwise fail initialization) so discovery fetch cannot proceed without the
configured proxy. In practice, inside the webhook proxy setup where
HttpProxyAgent and HttpsProxyAgent are instantiated, replace the catch-only
behavior with a throw of a new Error (or rethrow the caught error) after logging
to ensure the service fails fast rather than falling back to no-proxy.
- Around line 573-592: In the catch block inside Keycloak.ts, move the
isCancel(e) check before the isAxiosError(e) branch so cancellation (Axios
CanceledError) is detected first; update the catch flow in the Keycloak error
handler to: if isCancel(e) -> throw new PublicError(EnumStatusCode.ERR, 'Request
cancelled by user'), else if isAxiosError(e) -> keep the existing e.code switch
and message handling, otherwise keep the fallback branch—this ensures canceled
requests hit the dedicated cancellation handler instead of the generic Axios
error path.
---
Duplicate comments:
In `@controlplane/src/core/services/Keycloak.ts`:
- Around line 549-572: The discovery response handling in Keycloak.ts currently
treats any object (including null) as valid and may return payloads missing
required fields; update the block that processes response.data to (1) explicitly
reject null values (e.g., if data === null throw PublicError with
EnumStatusCode.ERR and the existing JSON parse error message) and (2) when the
downstream config uses useJwksUrl === 'true' ensure the parsed object
(ParsedOpenIdConfiguration) contains a non-empty jwks_uri property and throw
PublicError(EnumStatusCode.ERR, 'Missing jwks_uri in discovery document.') if
absent; keep all other parse/error paths and returns unchanged so callers of the
function/class (Keycloak.ts) still receive a validated
ParsedOpenIdConfiguration.
---
Nitpick comments:
In `@controlplane/src/core/services/Keycloak.ts`:
- Around line 283-299: The public method createOIDCProvider currently lacks an
explicit return type; annotate its signature to return Promise<void> (e.g.,
change the method declaration for createOIDCProvider to include : Promise<void>)
so it conforms to the repo TypeScript rule; ensure the async implementation
still returns nothing (or only resolves) so the Promise<void> annotation is
correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1b051099-3f33-4476-8fb4-10cb7b7ae762
📒 Files selected for processing (1)
controlplane/src/core/services/Keycloak.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
controlplane/src/core/services/Keycloak.ts (2)
579-589:⚠️ Potential issue | 🟡 MinorTypo in error message: "he discovery" should be "the discovery".
🔧 Proposed fix
case 'ECONNABORTED': { message = - 'The discovery endpoint did not respond within the time limit. Please, make sure that he discovery endpoint is correct and reachable.'; + 'The discovery endpoint did not respond within the time limit. Please make sure that the discovery endpoint is correct and reachable.'; break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/services/Keycloak.ts` around lines 579 - 589, Fix the typo in the ECONNABORTED error message inside the switch handling in Keycloak.ts: locate the switch on e.code where the variable message is assigned for case 'ECONNABORTED' and change "he discovery endpoint" to "the discovery endpoint" so the message reads correctly; ensure only the string is corrected and keep the surrounding logic (switch, case 'ECONNABORTED', message assignment) unchanged.
55-61:⚠️ Potential issue | 🟠 MajorDon't fail open when proxy setup breaks.
If
webhookProxyUrlis provided but agent creation throws, this only logs and continues. The discovery fetch then runs without the configured proxy path, which is a risky fallback for a misconfigured egress. If proxy is required, failing silently defeats the purpose.🔧 Proposed fix
if (options.webhookProxyUrl) { try { httpAgent = new HttpProxyAgent(options.webhookProxyUrl, {}); httpsAgent = new HttpsProxyAgent(options.webhookProxyUrl, {}); } catch (e) { this.logger.error(e, 'Failed to create proxy agent'); + throw e; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/services/Keycloak.ts` around lines 55 - 61, The code silently continues when proxy agent creation fails (inside the options.webhookProxyUrl block using HttpProxyAgent/HttpsProxyAgent and this.logger.error), which yields an unsafe “fail open” behavior; change the error path to stop initialization when options.webhookProxyUrl is present but agent creation throws—either rethrow the caught error or set a fatal/initialization failure (so discovery fetch cannot proceed without the proxy), and ensure the error log includes context before throwing to aid debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controlplane/src/core/services/Keycloak.ts`:
- Around line 549-556: The discovery response handling currently treats any
typeof object as valid, but since typeof null === 'object' you must explicitly
check for null before using the parsed discovery data; in the Keycloak discovery
logic (variable names: response and data, and later uses like
data.token_endpoint) update the conditional to reject null (e.g., if (data ===
null) throw a PublicError with a clear message) or convert string JSON then
validate that data is a non-null object before proceeding so attempts to access
data.token_endpoint produce a user-friendly PublicError instead of a TypeError.
---
Duplicate comments:
In `@controlplane/src/core/services/Keycloak.ts`:
- Around line 579-589: Fix the typo in the ECONNABORTED error message inside the
switch handling in Keycloak.ts: locate the switch on e.code where the variable
message is assigned for case 'ECONNABORTED' and change "he discovery endpoint"
to "the discovery endpoint" so the message reads correctly; ensure only the
string is corrected and keep the surrounding logic (switch, case 'ECONNABORTED',
message assignment) unchanged.
- Around line 55-61: The code silently continues when proxy agent creation fails
(inside the options.webhookProxyUrl block using HttpProxyAgent/HttpsProxyAgent
and this.logger.error), which yields an unsafe “fail open” behavior; change the
error path to stop initialization when options.webhookProxyUrl is present but
agent creation throws—either rethrow the caught error or set a
fatal/initialization failure (so discovery fetch cannot proceed without the
proxy), and ensure the error log includes context before throwing to aid
debugging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 654a13ec-18e8-486b-8311-ed1532dc041f
📒 Files selected for processing (1)
controlplane/src/core/services/Keycloak.ts
…a-oidc-endpoint-requests
…a-oidc-endpoint-requests
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
controlplane/src/core/services/Keycloak.ts (2)
55-61:⚠️ Potential issue | 🟠 MajorFail fast when the configured proxy cannot be initialized.
If
webhookProxyUrlis set and agent construction throws, this silently falls back to a direct outbound request. That bypasses the egress path this hardening change is trying to control.Suggested fix
if (options.webhookProxyUrl) { try { httpAgent = new HttpProxyAgent(options.webhookProxyUrl, {}); httpsAgent = new HttpsProxyAgent(options.webhookProxyUrl, {}); } catch (e) { this.logger.error(e, 'Failed to create proxy agent'); + throw e; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/services/Keycloak.ts` around lines 55 - 61, When options.webhookProxyUrl is provided and creating HttpProxyAgent/HttpsProxyAgent fails, don't silently continue — log the error and rethrow to fail-fast so outbound requests cannot bypass the proxy; in the Keycloak class constructor (or the block that references options.webhookProxyUrl, HttpProxyAgent, and HttpsProxyAgent) replace the current catch that only calls this.logger.error(e, 'Failed to create proxy agent') with code that logs the error and then throws (or propagates) a new Error including the original error/details so initialization fails immediately.
549-556:⚠️ Potential issue | 🟡 MinorReject
nulland re-validate parsed JSON before casting.
typeof null === 'object', andJSON.parse('null')also reaches the cast here. The next property access increateOIDCProviderthen throws aTypeErrorinstead of returning aPublicError.Suggested fix
// Make sure that the discovery endpoint returned a valid JSON response let data = response.data; - if (typeof data === 'object') { + if (data !== null && typeof data === 'object') { // No need to parse the data } else if (typeof data === 'string') { try { data = JSON.parse(data); } catch { // Failed to parse the response as JSON throw new PublicError( EnumStatusCode.ERR, 'Failed to parse response from the provided discovery endpoint as JSON.', ); } } else { // Invalid response format throw new PublicError( EnumStatusCode.ERR, 'Failed to parse response from the provided discovery endpoint as JSON.', ); } + if (data === null || typeof data !== 'object') { + throw new PublicError( + EnumStatusCode.ERR, + 'Failed to parse response from the provided discovery endpoint as JSON.', + ); + } + // Cast the data to the expected type return data as ParsedOpenIdConfiguration;Also applies to: 571-572
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/services/Keycloak.ts` around lines 549 - 556, The code assumes response.data is a non-null object and blindly casts it before using it in createOIDCProvider, but typeof null === 'object' and JSON.parse('null') can slip through; update the parsing/validation logic around response.data so that after parsing (when typeof data === 'string') you explicitly check data !== null && typeof data === 'object' before casting/returning it, and when data initially typeof 'object' also verify it's not null; apply the same null-check and re-validation to the similar parsing branch used later (the other discovery parse block referenced near createOIDCProvider) so any null or non-object JSON returns the expected PublicError instead of causing a TypeError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controlplane/src/core/services/Keycloak.ts`:
- Around line 536-540: The discovery fetch using
this.#httpClient.get(discoveryEndpoint, { signal, timeout: 10_000,
validateStatus: ... }) must limit response size to avoid buffering huge bodies;
update the call to include Axios size limits (e.g., maxContentLength and
maxBodyLength set to a sensible constant like DISCOVERY_MAX_BYTES = 1_048_576)
or alternatively switch to responseType: 'stream' and implement a
read-with-byte-count that aborts via signal when the limit is exceeded; ensure
the same options (signal, timeout, validateStatus) are preserved and reference
this.#httpClient.get and discoveryEndpoint when making the change.
- Around line 300-305: The openIdConfiguration validation currently only checks
truthiness so non-string values like objects/arrays can slip through; update the
check after calling this.#fetchOpenIdConfiguration(discoveryEndpoint,
abortSignal) to verify that openIdConfiguration.token_endpoint and
openIdConfiguration.authorization_endpoint are both typeof 'string' and have
non-empty trimmed length, otherwise throw the same PublicError. Locate the
validation in Keycloak.ts (the method using
openIdConfiguration/token_endpoint/authorization_endpoint) and replace the loose
truthy check with strict string-and-length checks to ensure only valid URL
strings are accepted before writing into Keycloak config.
In `@controlplane/test/oidc-provider.test.ts`:
- Around line 49-93: The test verifies the duplicate create returns
ERR_ALREADY_EXISTS but doesn't re-read the stored provider to ensure the
original configuration wasn't mutated; after the second createOIDCProvider()
call (the one expecting EnumStatusCode.ERR_ALREADY_EXISTS), call
getOIDCProvider() again and assert the returned provider still has endpoint
'localhost:8080', name 'okta', mappers length 1 and the same mapper
groupId/ssoGroup (use getOIDCProvider() and the same expectations used earlier)
to prove createOIDCProvider() did not overwrite the existing OIDC provider.
---
Duplicate comments:
In `@controlplane/src/core/services/Keycloak.ts`:
- Around line 55-61: When options.webhookProxyUrl is provided and creating
HttpProxyAgent/HttpsProxyAgent fails, don't silently continue — log the error
and rethrow to fail-fast so outbound requests cannot bypass the proxy; in the
Keycloak class constructor (or the block that references
options.webhookProxyUrl, HttpProxyAgent, and HttpsProxyAgent) replace the
current catch that only calls this.logger.error(e, 'Failed to create proxy
agent') with code that logs the error and then throws (or propagates) a new
Error including the original error/details so initialization fails immediately.
- Around line 549-556: The code assumes response.data is a non-null object and
blindly casts it before using it in createOIDCProvider, but typeof null ===
'object' and JSON.parse('null') can slip through; update the parsing/validation
logic around response.data so that after parsing (when typeof data === 'string')
you explicitly check data !== null && typeof data === 'object' before
casting/returning it, and when data initially typeof 'object' also verify it's
not null; apply the same null-check and re-validation to the similar parsing
branch used later (the other discovery parse block referenced near
createOIDCProvider) so any null or non-object JSON returns the expected
PublicError instead of causing a TypeError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 93dfaea0-a1fc-487c-840b-53953ec7e946
📒 Files selected for processing (3)
controlplane/src/core/bufservices/sso/createOIDCProvider.tscontrolplane/src/core/services/Keycloak.tscontrolplane/test/oidc-provider.test.ts
✅ Files skipped from review due to trivial changes (1)
- controlplane/src/core/bufservices/sso/createOIDCProvider.ts
…a-oidc-endpoint-requests
…a-oidc-endpoint-requests
…a-oidc-endpoint-requests
…a-oidc-endpoint-requests
…a-oidc-endpoint-requests
…a-oidc-endpoint-requests
…a-oidc-endpoint-requests # Conflicts: # controlplane/src/core/services/Keycloak.ts # pnpm-lock.yaml
Summary by CodeRabbit
Chores
Bug Fixes
Tests
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.
The goal of this PR is to reduce the possibility of a DoS by validating the OIDC
discovery endpointdirectly instead of delegating it to Keycloak, this way we can apply custom cancellation and timeout to the request.@connectrpc/*packages have been updated to1.5.0to address a bug which was causing therequest.signalto be cancelled before the handler got invoked.