Use custom domains for openid-configuration#673
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis pull request implements custom domain support for OpenID configuration endpoints and refines login session state handling. The Changes
Possibly Related PRs
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/authhero/src/authentication-flows/common.ts (1)
643-672:⚠️ Potential issue | 🟠 MajorDon’t recover every
EXPIREDsession as if it were stillPENDING.The state machine still treats
expiredas final. Coercing every expired record back toPENDINGhere means you lose whether the timeout happened inPENDING,AWAITING_MFA,AWAITING_HOOK, orAWAITING_CONTINUATION, then write backAUTHENTICATEDanyway. That can resume a timed-out flow past the step it was waiting on. If expired callbacks need recovery, model that explicitly and persist enough pre-expiry state to recover only the safe cases.Also applies to: 1153-1157
🧹 Nitpick comments (1)
packages/authhero/src/routes/auth-api/callback.ts (1)
203-215: Extract the 400-error parsing into one helper.The GET and POST handlers now duplicate the same
JSON.parse(err.message)fallback block. A small helper would keep theJSONHTTPException→returnError()mapping from drifting the next time this behavior changes.Also applies to: 318-329
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/routes/auth-api/callback.ts` around lines 203 - 215, Extract the duplicated JSON error-parsing block into a small helper (e.g., parseJSONHttpError(err): { errorCode, errorMessage }) that tries JSON.parse(err.message) and falls back to err.message, returning the same errorCode/errorMessage logic currently used; replace the duplicated try/catch + returnError(...) sequences in both the GET and POST handlers of the callback route with a call to this helper and then call returnError(ctx, state, errorCode, errorMessage) so behavior remains identical but centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/authhero/src/routes/auth-api/well-known.ts`:
- Around line 72-82: The discovery response uses ctx.var.custom_domain (via
getIssuer/getAuthUrl/openIDConfigurationSchema) but is still served as a shared
public cache; update the caching to vary by the forwarded host so a cached
document for one custom domain isn't reused for another — specifically ensure
the response cache key includes the X-Forwarded-Host header (or set the Vary:
x-forwarded-host header) when returning the OpenID config generated by the code
paths that call getIssuer and getAuthUrl using ctx.var.custom_domain (the
well-known route handling the openIDConfigurationSchema result); apply the same
change for the other discovery block around where
jwks_uri/registration_endpoint/revocation_endpoint are built (lines ~146-152).
---
Nitpick comments:
In `@packages/authhero/src/routes/auth-api/callback.ts`:
- Around line 203-215: Extract the duplicated JSON error-parsing block into a
small helper (e.g., parseJSONHttpError(err): { errorCode, errorMessage }) that
tries JSON.parse(err.message) and falls back to err.message, returning the same
errorCode/errorMessage logic currently used; replace the duplicated try/catch +
returnError(...) sequences in both the GET and POST handlers of the callback
route with a call to this helper and then call returnError(ctx, state,
errorCode, errorMessage) so behavior remains identical but centralized.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: da6cb658-09f8-40cc-ba4d-92c905877d51
📒 Files selected for processing (7)
.changeset/thirty-papers-lick.mdpackages/authhero/src/authentication-flows/common.tspackages/authhero/src/routes/auth-api/callback.tspackages/authhero/src/routes/auth-api/well-known.tspackages/authhero/src/variables.tspackages/authhero/test/routes/auth-api/callback.spec.tspackages/authhero/test/routes/auth-api/well-known.spec.ts
| const customDomain = ctx.var.custom_domain; | ||
| const result = openIDConfigurationSchema.parse({ | ||
| issuer: getIssuer(ctx.env), | ||
| authorization_endpoint: `${getAuthUrl(ctx.env)}authorize`, | ||
| token_endpoint: `${getAuthUrl(ctx.env)}oauth/token`, | ||
| device_authorization_endpoint: `${getAuthUrl(ctx.env)}oauth/device/code`, | ||
| userinfo_endpoint: `${getAuthUrl(ctx.env)}userinfo`, | ||
| mfa_challenge_endpoint: `${getAuthUrl(ctx.env)}mfa/challenge`, | ||
| jwks_uri: `${getAuthUrl(ctx.env)}.well-known/jwks.json`, | ||
| registration_endpoint: `${getAuthUrl(ctx.env)}oidc/register`, | ||
| revocation_endpoint: `${getAuthUrl(ctx.env)}oauth/revoke`, | ||
| issuer: getIssuer(ctx.env, customDomain), | ||
| authorization_endpoint: `${getAuthUrl(ctx.env, customDomain)}authorize`, | ||
| token_endpoint: `${getAuthUrl(ctx.env, customDomain)}oauth/token`, | ||
| device_authorization_endpoint: `${getAuthUrl(ctx.env, customDomain)}oauth/device/code`, | ||
| userinfo_endpoint: `${getAuthUrl(ctx.env, customDomain)}userinfo`, | ||
| mfa_challenge_endpoint: `${getAuthUrl(ctx.env, customDomain)}mfa/challenge`, | ||
| jwks_uri: `${getAuthUrl(ctx.env, customDomain)}.well-known/jwks.json`, | ||
| registration_endpoint: `${getAuthUrl(ctx.env, customDomain)}oidc/register`, | ||
| revocation_endpoint: `${getAuthUrl(ctx.env, customDomain)}oauth/revoke`, |
There was a problem hiding this comment.
Advertise x-forwarded-host in the cache key.
This discovery document now depends on ctx.var.custom_domain, but it is still marked public cacheable without varying on the forwarded host. In the proxied custom-domain path, a shared cache can reuse one domain’s issuer and endpoints for another.
Suggested change
return ctx.json(result, {
headers: {
"access-control-allow-origin": "*",
"access-control-allow-method": "GET",
+ vary: "x-forwarded-host",
"cache-control": `public, max-age=${JWKS_CACHE_TIMEOUT_IN_SECONDS}, stale-while-revalidate=${JWKS_CACHE_TIMEOUT_IN_SECONDS * 2
}, stale-if-error=86400`,
},
});Also applies to: 146-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/authhero/src/routes/auth-api/well-known.ts` around lines 72 - 82,
The discovery response uses ctx.var.custom_domain (via
getIssuer/getAuthUrl/openIDConfigurationSchema) but is still served as a shared
public cache; update the caching to vary by the forwarded host so a cached
document for one custom domain isn't reused for another — specifically ensure
the response cache key includes the X-Forwarded-Host header (or set the Vary:
x-forwarded-host header) when returning the OpenID config generated by the code
paths that call getIssuer and getAuthUrl using ctx.var.custom_domain (the
well-known route handling the openIDConfigurationSchema result); apply the same
change for the other discovery block around where
jwks_uri/registration_endpoint/revocation_endpoint are built (lines ~146-152).
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests