Skip to content

fix: OIDC discovery trusted origins for IdP registration#3714

Merged
joeyorlando merged 2 commits intomainfrom
codex/fix-oidc-discovery-trusted-origin
Apr 11, 2026
Merged

fix: OIDC discovery trusted origins for IdP registration#3714
joeyorlando merged 2 commits intomainfrom
codex/fix-oidc-discovery-trusted-origin

Conversation

@joeyorlando
Copy link
Copy Markdown
Contributor

@joeyorlando joeyorlando commented Apr 10, 2026

What changed

This narrows a recent Better Auth trusted-origin workaround to include the admin identity-provider create flow in addition to the existing /sso/register path and internal auth.api registration calls.

Why it changed

There was a report of Untrusted OIDC discovery URL while reinstalling a Generic OIDC provider in an environment even though the Okta/Archestraconfiguration matched a working setup. The root cause is that Better Auth can evaluate OIDC discovery trust against the outer /api/identity-providers request during provider creation, while our existing workaround only widened trust for /sso/register and request-less internal calls.

Impact

Generic OIDC provider creation and reinstall flows now allow Better Auth's discovery validation to proceed for the admin IdP registration request without widening trusted origins for normal auth traffic.

@joeyorlando joeyorlando changed the title [codex] Fix OIDC discovery trusted origins for IdP registration fix: OIDC discovery trusted origins for IdP registration Apr 10, 2026
@joeyorlando joeyorlando marked this pull request as ready for review April 10, 2026 23:18
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 10, 2026

Claude finished @joeyorlando's task in 2m 22s —— View job


PR Review: OIDC discovery trusted origins for IdP registration

  • Read repo context (CLAUDE.md, routes, model layer)
  • Read better-auth.ts and the trusted-origins helper
  • Read new test case in better-auth.test.ts
  • Verify /api/identity-providers is the correct Fastify route
  • Confirm update/delete flows don't need the same workaround
  • Post review feedback

Summary

The fix is correct, minimal, and well-scoped. It addresses a real edge case where Better Auth's trustedOrigins callback is invoked with the outer HTTP request (the admin-facing POST /api/identity-providers) rather than with an internal request-less call, so the existing /sso/register + !request checks alone weren't enough to let OIDC discovery pass during provider reinstall.

The rename from shouldTrustAllOriginsForSsoRegistration to shouldTrustAllOriginsForIdentityProviderRegistration correctly reflects the new broader scope, and the updated JSDoc captures the nuance.

Correctness

  • IDENTITY_PROVIDERS_API_PREFIX is defined as "/api/identity-providers" in platform/backend/src/constants.ts:4, matching the hard-coded path in shouldTrustAllOriginsForIdentityProviderRegistration at platform/backend/src/auth/better-auth.ts:459.
  • The POST handler at platform/backend/src/routes/identity-provider.ee.ts:113 is the only mutating route that actually re-enters Better Auth (via IdentityProviderModel.createauth.api.registerSSOProvider at platform/backend/src/models/identity-provider.ee.ts:598). update/delete go straight to Drizzle, so no workaround is needed there. Good call not widening the PUT path.
  • Fastify is constructed without ignoreTrailingSlash (platform/backend/src/server.ts:250), so a client can't hit /api/identity-providers/ and still reach the route — strict equality (pathname === "/api/identity-providers") is safe and matches Fastify routing behavior.
  • The new URL(request.url) guard with try/catch is preserved, so malformed URLs still fall back to "not trusted" rather than throwing.

Tests

The new test at platform/backend/src/auth/better-auth.test.ts:348 directly exercises the new branch and matches the shape of the existing /sso/register test. The three-test coverage (internal call, /sso/register, /api/identity-providers, plus the negative case) is solid.

One small thought: you might consider adding a negative assertion for a near-miss like POST /api/identity-providers/abc123 (the PUT path) to lock in that updates don't accidentally get wildcard trust if someone later refactors the check to startsWith or includes. Not blocking — the existing "keeps regular auth requests" test already covers the general non-match case.

Nits (non-blocking)

  • platform/backend/src/auth/better-auth.ts:408-410 — the comment phrasing "In practice, the same flow can also inherit the outer /api/identity-providers request" is accurate but a bit subtle. A one-line mention that Better Auth reuses the ambient Fastify request context when the IdP model forwards headers into auth.api.registerSSOProvider would help a future reader understand why the outer URL bleeds through.
  • Consider hoisting the literal "/api/identity-providers" to reference IDENTITY_PROVIDERS_API_PREFIX from @/constants to keep the two in sync. Minor DRY-ness win; the path is unlikely to change, but it would prevent drift.

Security considerations

The wildcard expansion (http://*, https://*, http://*:*, https://*:*) is still gated on an explicit, admin-only route, and the change does not broaden what data is exposed — it only relaxes origin validation during OIDC discovery for provider creation. Since POST /api/identity-providers is authenticated and authorization-guarded upstream (admin permission), this looks like an acceptable trade-off and is consistent with the existing /sso/register handling.

Verdict

LGTM. Ship it once CI is green. The nits above are optional cleanup.
· Branch

@joeyorlando joeyorlando merged commit adb5f5e into main Apr 11, 2026
39 checks passed
@joeyorlando joeyorlando deleted the codex/fix-oidc-discovery-trusted-origin branch April 11, 2026 00:15
joeyorlando added a commit that referenced this pull request Apr 11, 2026
🤖 I have created a release *beep* *boop*
---


##
[1.2.10](platform-v1.2.9...platform-v1.2.10)
(2026-04-11)


### Features

* make posthog analytics configurable
([#3707](#3707))
([7ae9101](7ae9101))


### Bug Fixes

* `/llm/costs` table scrolling
([#3722](#3722))
([6a42ba8](6a42ba8))
* apply MCP OAuth lifetime for gateway slugs
([#3711](#3711))
([362aaec](362aaec))
* Bedrock tool name encoding
([#3706](#3706))
([0e2c2d1](0e2c2d1))
* costs timeframes and surface limit reset settings
([#3709](#3709))
([6e4154b](6e4154b))
* jira oauth discovery overrides
([#3721](#3721))
([2c4cf8f](2c4cf8f))
* OIDC discovery trusted origins for IdP registration
([#3714](#3714))
([adb5f5e](adb5f5e))
* preserve shared chat agents on fork
([#3715](#3715))
([252edfc](252edfc))
* reranker model dropdown labels
([#3704](#3704))
([ebd1c8a](ebd1c8a))
* session logs loading state
([#3712](#3712))
([ffba126](ffba126))


### Miscellaneous Chores

* **ci:** add ID-JAG MCP e2e test
([#3702](#3702))
([1a5078a](1a5078a))
* **deps:** bump next from 16.1.7 to 16.2.3 in /platform/frontend
([#3708](#3708))
([d47967c](d47967c))
* use neutral token prefixes with legacy support
([#3719](#3719))
([db5929c](db5929c))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: archestra-ci[bot] <222894074+archestra-ci[bot]@users.noreply.github.com>
Co-authored-by: Joey Orlando <joey@archestra.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant