Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis PR centralizes email and SMS delivery behind new adapter interfaces exposed via DataAdapters, updates call sites to use Changes
Sequence Diagram(s)sequenceDiagram
participant Client as User/Client
participant AuthService as Auth Service
participant DataAdapter as Data Adapter (ctx.env.data)
participant EmailAdapter as EmailServiceAdapter
participant SmsAdapter as SmsServiceAdapter
Client->>AuthService: Trigger action requiring delivery (email/SMS)
AuthService->>DataAdapter: resolve emailService / smsService
DataAdapter-->>AuthService: EmailServiceAdapter / SmsServiceAdapter instance
alt Email path
AuthService->>EmailAdapter: call .send(EmailServiceSendParams)
EmailAdapter->>EmailAdapter: perform delivery logic
EmailAdapter-->>AuthService: Promise<void> resolved
AuthService->>Client: continue flow
else SMS path
AuthService->>SmsAdapter: call .send(SmsServiceSendParams)
SmsAdapter->>SmsAdapter: perform delivery logic
SmsAdapter-->>AuthService: Promise<void> resolved
AuthService->>Client: continue flow
end
opt service missing
AuthService->>AuthService: throw "<Service> service not configured"
AuthService->>Client: error response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
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/routes/universal-login/u2-routes.tsx (1)
994-1018:⚠️ Potential issue | 🟠 MajorPlumb
extraScriptthrough every HTML render path.
result.extraScriptonly reaches the GET default<WidgetPage>branch. The Liquid-template path and the POST re-render path still drop the screen result’s page script, so passkey enrollment won't auto-start for tenants with custom templates and any server-rendered retry/error page comes back without the WebAuthn bootstrap code.Also applies to: 1023-1037, 1302-1343
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/routes/universal-login/u2-routes.tsx` around lines 994 - 1018, The screen result's extraScript (result.extraScript) is not being passed into the Liquid-template render and POST re-render paths; ensure the page script is plumbed through by passing extraScript into the HTML generation calls: update the calls to generateWidgetContent (and generateFooterContent if it injects scripts) to accept an extraScript parameter and include that script in the returned widget/footer HTML, and then pass that same extraScript into applyLiquidTemplate so the final renderedHtml contains the script before returning ctx.html; make the same change in the POST re-render code paths that rebuild widgetContent/footerContent and in any places that render customTemplate.body so the WebAuthn/bootstrap script always flows through.
🧹 Nitpick comments (2)
packages/authhero/src/routes/universal-login/screens/types.ts (1)
90-91: DocumentextraScriptas trusted-only content.Since this field enables page-level script injection, add an explicit contract note that it must be server-generated trusted code (never user-derived strings) to reduce accidental misuse later.
📝 Suggested contract hardening
- /** Optional inline script to inject at page level (e.g. WebAuthn ceremony) */ + /** Optional inline script to inject at page level (e.g. WebAuthn ceremony). + * Must be trusted server-generated JS; never interpolate unescaped user input. + */ extraScript?: string;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/routes/universal-login/screens/types.ts` around lines 90 - 91, The JSDoc for the extraScript property is insufficiently restrictive—document it explicitly as trusted-only server-generated script content to prevent misuse: update the comment for the extraScript field (symbol: extraScript in the types definition) to state it must be generated by the server, never derived from user input, and only used for known-safe scripts (e.g., WebAuthn ceremonies); include a short security note indicating that callers must sanitize/validate sources and that the field is intended for admin/trusted code injection only.packages/authhero/test/middlewares/tenant.spec.ts (1)
22-22: Add a dedicated test for query-based tenant resolution.Line 22 adds the query mock, but there’s still no explicit assertion for the new
tenant_idquery path. A focused test here will protect this new behavior from regressions.✅ Suggested test addition
+ it("should set tenant_id from tenant_id query when no header/domain tenant is resolved", async () => { + mockHeaderFn.mockReturnValue(null); + mockCtx.req.query = vi.fn((key?: string) => + key === "tenant_id" ? "tenant_from_query" : undefined, + ); + + await tenantMiddleware(mockCtx, mockNext); + + expect(mockSet).toHaveBeenCalledWith("tenant_id", "tenant_from_query"); + expect(mockNext).toHaveBeenCalled(); + });Based on learnings: Test all changes across multiple tenants to ensure proper data isolation and tenant-specific functionality.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/test/middlewares/tenant.spec.ts` at line 22, Add a focused unit test in tenant.spec.ts that exercises the query-based tenant resolution path: replace the existing query mock (query: vi.fn().mockReturnValue(undefined)) with a mock that returns an object containing tenant_id (e.g. query: vi.fn().mockReturnValue({ tenant_id: 'tenant-123' })), invoke the middleware/utility under test (the tenant resolution function used in the spec), and assert the resolved tenant equals 'tenant-123'; also add a second case with a different tenant_id to verify multi-tenant isolation. Ensure you reference and update the existing query: vi.fn() mock and the test that calls the tenant middleware/resolver so the new assertions cover the query-based flow.
🤖 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/adapter-interfaces/src/adapters/index.ts`:
- Around line 85-86: The DataAdapters type currently marks emailService and
smsService as optional which lets storage-only adapter factories (e.g.
createAdapters in packages/aws and packages/kysely) satisfy init() while missing
delivery adapters; update the adapter types so the runtime bootstrap accepts a
stricter RuntimeAdapters shape (or make emailService: EmailServiceAdapter and
smsService: SmsServiceAdapter required on the runtime contract) and change the
init() signature to require RuntimeAdapters instead of the storage-only
DataAdapters; then update the adapter factory returns (createAdapters in
packages/aws and packages/kysely) or add a separate storage-only type so those
factories cannot be passed directly to init() without wiring real
EmailServiceAdapter/SmsServiceAdapter implementations.
In `@packages/authhero/locales/cs.json`:
- Around line 361-362: The new Czech strings use "Přístupový klíč" which is
inconsistent with the rest of the locale that uses the English term "passkey";
update the keys so the terminology is consistent—either change the values for
passkeyLabel and passkeyDescription to use the same term as the existing
passkeys.* strings (e.g., keep "passkey") or translate all other passkeys.*
entries to "Přístupový klíč"/the chosen Czech phrase; locate the keys
passkeyLabel and passkeyDescription in the locale and apply the same term across
all passkeys.* entries.
In `@packages/authhero/locales/fi.json`:
- Around line 361-362: The Finnish locale introduces an inconsistent term:
change the value of "passkeyLabel" (and optionally "passkeyDescription") to use
the same term as the existing passkeys.* entries (currently
"Tunnistautumisavain") so the selector and enrollment screens match; locate the
keys "passkeyLabel" and "passkeyDescription" in
packages/authhero/locales/fi.json and replace "Pääsyavain" (and any differing
phrasing) with "Tunnistautumisavain" (or update the other passkeys.* keys to
"Pääsyavain" if you prefer that term) to ensure consistent terminology across
passkeyLabel, passkeyDescription, and the passkeys.* strings.
In `@packages/authhero/locales/it.json`:
- Around line 361-362: The Italian translation is inconsistent: the new MFA key
uses "passkeyLabel" = "Chiave di accesso" while other keys in the passkey flow
still use the English term "passkey"; either make this key use the English term
to match the rest (set "passkeyLabel" back to "passkey") or translate all
related keys in the passkey flow for consistency (update every "passkeys.*" key
to use "Chiave di accesso" / corresponding Italian wording, and ensure
"passkeyDescription" and any other "passkeys.*" strings are updated
accordingly).
In `@packages/authhero/locales/nb.json`:
- Around line 361-362: The locale string for passkey is inconsistent: change the
value of the JSON key "passkeyLabel" from "Tilgangsnøkkel" to the same term used
by the other passkeys strings (e.g., "Passnøkkel") so it matches the existing
"passkeys.*" entries; update only the "passkeyLabel" value (and optionally scan
for other stray occurrences of "Tilgangsnøkkel") to ensure consistent use of
"passnøkkel" across the file.
In `@packages/authhero/src/middlewares/tenant.ts`:
- Around line 67-74: The middleware currently trusts ctx.req.query("tenant_id")
and sets ctx.var.tenant_id directly (tenantIdQuery), so restrict and validate
that value before setting it: only allow query-derived tenant_id on the specific
enrollment endpoint(s) (check the request path via ctx.req.path or ctx.req.url),
validate the tenantIdQuery format (e.g., UUID or expected tenant id pattern),
and verify it matches an enrollment ticket or other server-side artifact
(perform a lookup/verifyTicket(ticketId) or query the DB) tied to the request
before calling ctx.set("tenant_id", tenantIdQuery) and proceeding to await
next(); otherwise reject the query-supplied tenant_id and do not mutate
ctx.var.tenant_id.
In `@packages/authhero/src/routes/management-api/guardian.ts`:
- Line 564: The ticketUrl is built via string interpolation which can break if
tenantId or other values contain reserved characters; instead construct it using
the URL API: create a URL with new URL('u2/guardian/enroll', issuer) (or new
URL(path, issuer)), then use url.searchParams.append('ticket', ticketId) and
url.searchParams.append('tenant_id', tenantId) before reading url.toString();
update the code that assigns ticketUrl (the variable defined from issuer,
ticketId, tenantId in guardian.ts) to use this approach so all query parameters
are properly encoded.
In `@packages/authhero/src/routes/universal-login/screens/passkey-enrollment.ts`:
- Around line 117-163: The try/catch around navigator.credentials.create in
extraScript only logs errors to the console, leaving the user stuck; update the
catch block to surface a user-facing error by finding the same form (the form
lookup used for cf and af), then either (a) set a hidden/visible error field
(e.g., querySelector('[name="webauthn-error"]') or create/insert an element with
id 'webauthn-error') and put a friendly message (use e.message or a generic
"Registration failed") into it, and set the action-field (af) or a status field
so the server/page knows an error occurred, or (b) if you prefer to auto-submit
to show a server-side error, set a hidden error input value and submit the form;
modify the catch in extraScript to perform one of these actions so users see a
retry/clear error instead of only a console.error.
- Around line 553-572: Replace the hardcoded English success copy in the
successScreen.description with a localized string from the existing localization
helper "m" (you already call m.title()); e.g. call an appropriate localization
method on m (such as m.passkeyEnrollmentSuccess() or a similarly named key you
maintain) and assign that to successScreen.description so the
guardian_enrollment success message is localized for non-English flows.
- Around line 96-105: The skip link's inline JS currently queries document for
the form and action field which fails when the form lives inside
authhero-widget's shadow root; update the href generation for the link with id
"skip" so the script first attempts document.querySelector('form') and, if not
found, locates document.querySelector('authhero-widget')?.shadowRoot and queries
that shadowRoot for the form and action field (or uses form.querySelector to
find the action field), then sets the action-field value to 'skip' and submits
the form—mirror the same fallback logic used elsewhere in this file to ensure
the skip action works inside the widget shadow root.
- Around line 96-105: The client-only removal of the "skip" link is
insufficient—add a server-side guard so POSTs with action=skip are honored only
for continuation flows; in the POST handler that processes this screen (where
createFrontChannelAuthResponse is called), check the request/session flag
(isContinuation or equivalent on the session/extra) before executing the skip
branch and return an error or re-render if false, and likewise ensure any other
handlers referenced around the other affected blocks (the POST handling code
sections that call createFrontChannelAuthResponse) have the same isContinuation
validation to prevent bypassing MFA.
In `@packages/authhero/src/routes/universal-login/u2-widget-page.tsx`:
- Around line 58-59: The injected extraScript currently embeds extra.optionsJSON
into a template unquoted after only escaping "<" (safeOptions), which can allow
JS syntax breakouts; modify the construction of extraScript in
u2-widget-page.tsx to serialize the options safely by using JSON.stringify on
the parsed object (or by injecting a quoted JSON string) before embedding, e.g.,
replace the safeOptions logic with a fully JSON-encoded value and assign it to
opts (reference symbols: extra.optionsJSON, safeOptions, extraScript, and the
dangerouslySetInnerHTML usage; related generator: generateRegistrationOptions in
passkey-enrollment.ts) so the assignment cannot be broken by quotes or braces in
the payload.
---
Outside diff comments:
In `@packages/authhero/src/routes/universal-login/u2-routes.tsx`:
- Around line 994-1018: The screen result's extraScript (result.extraScript) is
not being passed into the Liquid-template render and POST re-render paths;
ensure the page script is plumbed through by passing extraScript into the HTML
generation calls: update the calls to generateWidgetContent (and
generateFooterContent if it injects scripts) to accept an extraScript parameter
and include that script in the returned widget/footer HTML, and then pass that
same extraScript into applyLiquidTemplate so the final renderedHtml contains the
script before returning ctx.html; make the same change in the POST re-render
code paths that rebuild widgetContent/footerContent and in any places that
render customTemplate.body so the WebAuthn/bootstrap script always flows
through.
---
Nitpick comments:
In `@packages/authhero/src/routes/universal-login/screens/types.ts`:
- Around line 90-91: The JSDoc for the extraScript property is insufficiently
restrictive—document it explicitly as trusted-only server-generated script
content to prevent misuse: update the comment for the extraScript field (symbol:
extraScript in the types definition) to state it must be generated by the
server, never derived from user input, and only used for known-safe scripts
(e.g., WebAuthn ceremonies); include a short security note indicating that
callers must sanitize/validate sources and that the field is intended for
admin/trusted code injection only.
In `@packages/authhero/test/middlewares/tenant.spec.ts`:
- Line 22: Add a focused unit test in tenant.spec.ts that exercises the
query-based tenant resolution path: replace the existing query mock (query:
vi.fn().mockReturnValue(undefined)) with a mock that returns an object
containing tenant_id (e.g. query: vi.fn().mockReturnValue({ tenant_id:
'tenant-123' })), invoke the middleware/utility under test (the tenant
resolution function used in the spec), and assert the resolved tenant equals
'tenant-123'; also add a second case with a different tenant_id to verify
multi-tenant isolation. Ensure you reference and update the existing query:
vi.fn() mock and the test that calls the tenant middleware/resolver so the new
assertions cover the query-based flow.
🪄 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: 3764f31a-9b89-42fd-b0b7-4b942075093a
📒 Files selected for processing (33)
.changeset/modern-lies-juggle.mdapps/docs/customization/adapter-interfaces/index.mdapps/docs/customization/configuration.mdpackages/adapter-interfaces/src/adapters/EmailService.tspackages/adapter-interfaces/src/adapters/SmsService.tspackages/adapter-interfaces/src/adapters/index.tspackages/authhero/README.mdpackages/authhero/locales/cs.jsonpackages/authhero/locales/da.jsonpackages/authhero/locales/en.jsonpackages/authhero/locales/fi.jsonpackages/authhero/locales/it.jsonpackages/authhero/locales/nb.jsonpackages/authhero/locales/pl.jsonpackages/authhero/locales/sv.jsonpackages/authhero/src/authentication-flows/mfa.tspackages/authhero/src/emails/index.tspackages/authhero/src/middlewares/tenant.tspackages/authhero/src/routes/management-api/guardian.tspackages/authhero/src/routes/universal-login/error-handler.tsxpackages/authhero/src/routes/universal-login/screens/mfa-login-options.tspackages/authhero/src/routes/universal-login/screens/passkey-enrollment.tspackages/authhero/src/routes/universal-login/screens/types.tspackages/authhero/src/routes/universal-login/u2-routes.tsxpackages/authhero/src/routes/universal-login/u2-widget-page.tsxpackages/authhero/src/types/Bindings.tspackages/authhero/src/types/EmailService.tspackages/authhero/src/types/SMSService.tspackages/authhero/test/helpers/mock-email-service.tspackages/authhero/test/helpers/mock-sms-service.tspackages/authhero/test/helpers/test-server.tspackages/authhero/test/middlewares/tenant.spec.tspackages/kysely/src/loginSessions/create.ts
💤 Files with no reviewable changes (3)
- packages/authhero/src/types/SMSService.ts
- packages/authhero/src/types/Bindings.ts
- packages/authhero/src/types/EmailService.ts
packages/authhero/src/routes/universal-login/screens/passkey-enrollment.ts
Show resolved
Hide resolved
packages/authhero/src/routes/universal-login/screens/passkey-enrollment.ts
Show resolved
Hide resolved
packages/authhero/src/routes/universal-login/screens/passkey-enrollment.ts
Show resolved
Hide resolved
Contributor
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/test/i18n/locales.spec.ts (1)
67-83:⚠️ Potential issue | 🟠 Major
"passkey"in the global loanword allowlist hides real untranslated labelsLine 71 makes
"Passkey"auto-exempt from untranslated detection. That suppresses real gaps likepackages/authhero/locales/cs.jsonandpackages/authhero/locales/it.jsonwherepasskeyLabelis still identical to English, while other passkey strings are translated. Please avoid allowlistingpasskeyglobally.Suggested minimal fix
const loanwords = new Set([ "email", "status", "password", - "passkey", "ok", "invitation", ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/test/i18n/locales.spec.ts` around lines 67 - 83, The current loanwords allowlist in the test (the loanwords Set used by canBeIdentical) includes "passkey", which masks untranslated real labels; remove "passkey" from the loanwords Set so canBeIdentical will no longer auto-exempt that token and the untranslated detection will catch identical passkey labels (adjust any test expectations if they assumed the exemption). Ensure the change is made where the loanwords Set and the canBeIdentical function are defined.
🧹 Nitpick comments (2)
packages/authhero/src/routes/universal-login/screens/passkey-enrollment.ts (1)
384-399: Consider consolidatingstate_dataparsing in POST handler.The
loginSession.state_datais parsed multiple times (lines 385, 489, 572) within the POST handler. While functionally correct, consolidating this into a single parse near the top of the handler would improve clarity and avoid redundant JSON parsing.💡 Example consolidation
// At top of post handler after fetching loginSession const loginSession = await ctx.env.data.loginSessions.get(...); + const stateData = loginSession?.state_data + ? JSON.parse(loginSession.state_data) + : {}; if (!loginSession || !loginSession.user_id) { ... } // Allow passkey enrollment from continuation flow or MFA enrollment flow - const postStateData = loginSession.state_data - ? JSON.parse(loginSession.state_data) - : {}; - const isGuardianEnrollment = postStateData.guardian_enrollment === true; + const isGuardianEnrollment = stateData.guardian_enrollment === true;Also applies to: 489-491, 572-574
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/routes/universal-login/screens/passkey-enrollment.ts` around lines 384 - 399, The POST handler parses loginSession.state_data multiple times; consolidate by parsing it once into a single variable (e.g., postStateData) at the top of the handler and reuse that variable everywhere instead of repeating JSON.parse(loginSession.state_data). Update all occurrences that re-parse state_data (the checks around postStateData.guardian_enrollment, the later references at lines you flagged, and any other JSON.parse(loginSession.state_data) in this handler) to use the single parsed variable, and guard the initial parse with a safe fallback (try/catch or conditional) so postStateData defaults to {} on missing/invalid JSON; keep existing logic that uses hasValidContinuationScope, passkeyEnrollmentScreen, and LoginSessionState.AWAITING_MFA unchanged.packages/authhero/src/routes/universal-login/u2-routes.tsx (1)
1928-1946: Consider documenting supported guardian enrollment factors.The routing logic handles OTP, SMS, and WebAuthn factors but omits
push_notification,recovery_code, andduofactors that exist in the tenant schema. If this is intentional (e.g., guardian enrollment only supports a subset of MFA methods), a brief comment would clarify this design decision for future maintainers.💡 Suggested documentation
const state = encodeURIComponent(loginSession.id); + // Guardian enrollment currently supports OTP, SMS, and WebAuthn factors. + // Email, push_notification, recovery_code, and duo are handled elsewhere. const hasOtp = factors?.otp === true; const hasSms = factors?.sms === true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authhero/src/routes/universal-login/u2-routes.tsx` around lines 1928 - 1946, The routing block that computes hasOtp, hasSms, hasWebauthn and factorCount and then redirects (the conditional that returns ctx.redirect(...) for login-options, totp-enrollment, phone-enrollment, passkey/enrollment) only accounts for otp, sms and webauthn factors; either expand the logic to include the other tenant factors (email, push_notification, recovery_code, duo) with appropriate redirects/handlers, or explicitly add a concise inline comment above the hasOtp/hasSms/hasWebauthn and factorCount logic stating that guardian enrollment intentionally supports only otp/sms/webauthn and why, so future maintainers understand the design decision. Ensure you reference the same symbols (hasOtp, hasSms, hasWebauthn, factorCount, and the ctx.redirect destinations) when updating the code or comment.
🤖 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/universal-login/screens/passkey-enrollment.ts`:
- Around line 181-195: Update getRpId and getExpectedOrigin to prefer the
x-forwarded-host header before falling back to host (and then "localhost") so
WebAuthn origin/rpId reflect the client-facing domain behind proxies;
specifically modify getRpId(ctx) and getExpectedOrigin(ctx) to read
ctx.req.header("x-forwarded-host") || ctx.req.header("host") || "localhost",
keep the existing port-stripping in getRpId and the localhost->http protocol
logic in getExpectedOrigin, and ensure the functions continue to return the rpId
(host without port) and the full origin string respectively.
---
Outside diff comments:
In `@packages/authhero/test/i18n/locales.spec.ts`:
- Around line 67-83: The current loanwords allowlist in the test (the loanwords
Set used by canBeIdentical) includes "passkey", which masks untranslated real
labels; remove "passkey" from the loanwords Set so canBeIdentical will no longer
auto-exempt that token and the untranslated detection will catch identical
passkey labels (adjust any test expectations if they assumed the exemption).
Ensure the change is made where the loanwords Set and the canBeIdentical
function are defined.
---
Nitpick comments:
In `@packages/authhero/src/routes/universal-login/screens/passkey-enrollment.ts`:
- Around line 384-399: The POST handler parses loginSession.state_data multiple
times; consolidate by parsing it once into a single variable (e.g.,
postStateData) at the top of the handler and reuse that variable everywhere
instead of repeating JSON.parse(loginSession.state_data). Update all occurrences
that re-parse state_data (the checks around postStateData.guardian_enrollment,
the later references at lines you flagged, and any other
JSON.parse(loginSession.state_data) in this handler) to use the single parsed
variable, and guard the initial parse with a safe fallback (try/catch or
conditional) so postStateData defaults to {} on missing/invalid JSON; keep
existing logic that uses hasValidContinuationScope, passkeyEnrollmentScreen, and
LoginSessionState.AWAITING_MFA unchanged.
In `@packages/authhero/src/routes/universal-login/u2-routes.tsx`:
- Around line 1928-1946: The routing block that computes hasOtp, hasSms,
hasWebauthn and factorCount and then redirects (the conditional that returns
ctx.redirect(...) for login-options, totp-enrollment, phone-enrollment,
passkey/enrollment) only accounts for otp, sms and webauthn factors; either
expand the logic to include the other tenant factors (email, push_notification,
recovery_code, duo) with appropriate redirects/handlers, or explicitly add a
concise inline comment above the hasOtp/hasSms/hasWebauthn and factorCount logic
stating that guardian enrollment intentionally supports only otp/sms/webauthn
and why, so future maintainers understand the design decision. Ensure you
reference the same symbols (hasOtp, hasSms, hasWebauthn, factorCount, and the
ctx.redirect destinations) when updating the code or comment.
🪄 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: cbb443c3-3a0f-4afb-bbbc-310aa6f650ae
📒 Files selected for processing (12)
packages/authhero/locales/cs.jsonpackages/authhero/locales/da.jsonpackages/authhero/locales/en.jsonpackages/authhero/locales/fi.jsonpackages/authhero/locales/it.jsonpackages/authhero/locales/nb.jsonpackages/authhero/locales/pl.jsonpackages/authhero/locales/sv.jsonpackages/authhero/src/routes/management-api/guardian.tspackages/authhero/src/routes/universal-login/screens/passkey-enrollment.tspackages/authhero/src/routes/universal-login/u2-routes.tsxpackages/authhero/test/i18n/locales.spec.ts
✅ Files skipped from review due to trivial changes (2)
- packages/authhero/locales/en.json
- packages/authhero/locales/da.json
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/authhero/locales/nb.json
- packages/authhero/src/routes/management-api/guardian.ts
- packages/authhero/locales/fi.json
- packages/authhero/locales/cs.json
- packages/authhero/locales/sv.json
- packages/authhero/locales/it.json
- packages/authhero/locales/pl.json
Comment on lines
181
to
195
| function getRpId(ctx: any): string { | ||
| const host = ctx.var.custom_domain || ctx.req.header("host") || "localhost"; | ||
| const host = ctx.req.header("host") || "localhost"; | ||
| // Strip port if present | ||
| return host.split(":")[0]; | ||
| } | ||
|
|
||
| /** | ||
| * Get the origin URL for WebAuthn verification | ||
| * Get the origin URL for WebAuthn verification. | ||
| * Must use the actual request host to match the browser's origin. | ||
| */ | ||
| function getExpectedOrigin(ctx: any): string { | ||
| const host = ctx.var.custom_domain || ctx.req.header("host") || "localhost"; | ||
| const host = ctx.req.header("host") || "localhost"; | ||
| const protocol = host.includes("localhost") ? "http" : "https"; | ||
| return `${protocol}://${host}`; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if X-Forwarded-Host is used elsewhere in the codebase for similar patterns
rg -n "X-Forwarded-Host" --type=ts
# Check for any proxy-aware host resolution utilities
rg -n "getHost|get.*[Hh]ost" --type=ts -C2 | head -50Repository: markusahlstrand/authhero
Length of output: 3400
🏁 Script executed:
# Get context around the X-Forwarded-Host usage in tenant.ts
cat -n packages/authhero/src/middlewares/tenant.ts | sed -n '25,40p'Repository: markusahlstrand/authhero
Length of output: 721
🏁 Script executed:
# Check if there are other host resolution functions in the codebase
rg -n "req\.header.*host" --type=ts -i -C3Repository: markusahlstrand/authhero
Length of output: 8351
🏁 Script executed:
# Look at the full context of the functions in question
cat -n packages/authhero/src/routes/universal-login/screens/passkey-enrollment.ts | sed -n '175,200p'Repository: markusahlstrand/authhero
Length of output: 1107
Use proxy-aware host resolution for WebAuthn.
These functions should check x-forwarded-host before falling back to the host header to match how the codebase handles proxied requests elsewhere (e.g., tenant.ts middleware). Behind a reverse proxy, the host header reflects the proxy's internal address, not the client-facing domain that the browser uses for origin verification.
Consider:
function getRpId(ctx: any): string {
const host = ctx.req.header("x-forwarded-host") || ctx.req.header("host") || "localhost";
return host.split(":")[0];
}
function getExpectedOrigin(ctx: any): string {
const host = ctx.req.header("x-forwarded-host") || ctx.req.header("host") || "localhost";
const protocol = host.includes("localhost") ? "http" : "https";
return `${protocol}://${host}`;
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/authhero/src/routes/universal-login/screens/passkey-enrollment.ts`
around lines 181 - 195, Update getRpId and getExpectedOrigin to prefer the
x-forwarded-host header before falling back to host (and then "localhost") so
WebAuthn origin/rpId reflect the client-facing domain behind proxies;
specifically modify getRpId(ctx) and getExpectedOrigin(ctx) to read
ctx.req.header("x-forwarded-host") || ctx.req.header("host") || "localhost",
keep the existing port-stripping in getRpId and the localhost->http protocol
logic in getExpectedOrigin, and ensure the functions continue to return the rpId
(host without port) and the full origin string respectively.
Owner
Author
There was a problem hiding this comment.
Already fixed. ctx.var.host is set in the tenant middleware at tenant.ts:18-20 to xForwardedHost || hostHeader || ..., which already prefers x-forwarded-host before host. The getRpId and getExpectedOrigin functions use ctx.var.host, so they already get the correct proxy-aware host. No change needed.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/authhero/src/middlewares/tenant.ts | sed -n '1,35p'
Length of output: 1207
@markusahlstrand, confirmed — tenant.ts lines 16–21 clearly set ctx.var.host as:
xForwardedHost || hostHeader || new URL(getIssuer(ctx.env)).hostYou're right that getRpId and getExpectedOrigin already get the correct proxy-aware host via ctx.var.host. My concern was invalid. Apologies for the noise!
✏️ Learnings added
Learnt from: markusahlstrand
Repo: markusahlstrand/authhero PR: 675
File: packages/authhero/src/routes/universal-login/screens/passkey-enrollment.ts:181-195
Timestamp: 2026-04-01T08:10:49.309Z
Learning: In `packages/authhero/src/middlewares/tenant.ts` (lines 16-21), `ctx.var.host` is set early in the tenant middleware as `x-forwarded-host || host header || issuer host`, making it proxy-aware. Functions that resolve the browser-facing host (e.g., `getRpId`, `getExpectedOrigin` in `passkey-enrollment.ts`) should use `ctx.var.host` directly — do not flag these as missing x-forwarded-host handling.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: markusahlstrand
Repo: markusahlstrand/authhero PR: 646
File: packages/authhero/src/routes/universal-login/screens/mfa-phone-challenge.ts:176-178
Timestamp: 2026-03-23T06:59:11.333Z
Learning: In `packages/authhero/src/routes/universal-login/screens/`, ensure that the universal-login post handler return type uses the discriminated union shape: `{ screen: ScreenResult } | { redirect: string; cookies?: string[] } | { error: string; screen: ScreenResult } | { response: Response }`, where `ScreenResult` is `{ screen: UiScreen; branding?: ScreenBranding }`. Therefore, returning `{ screen: await someScreen(context) }` is correct (it intentionally wraps a `ScreenResult` under the outer `screen` key). Do not flag this as incorrect nesting.
Learnt from: CR
Repo: markusahlstrand/authhero PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-25T13:20:21.406Z
Learning: Applies to src/{index,App,TenantsApp,*Tenant*,*Context*}.{ts,tsx} : When extracting tenant ID from URL path, ensure it is properly parsed and passed to all tenant-scoped operations
Learnt from: markusahlstrand
Repo: markusahlstrand/authhero PR: 6
File: packages/authhero/src/authentication-flows/authorization-code.ts:47-52
Timestamp: 2024-12-08T17:06:51.637Z
Learning: In the `authorizationCodeGrant` function within `packages/authhero/src/authentication-flows/authorization-code.ts`, revoked keys are removed in the data-adapter before being used. However, adding an additional check for `key.revoked` and verifying the `key.algorithm` can enhance safety.
Learnt from: markusahlstrand
Repo: markusahlstrand/authhero PR: 13
File: packages/authhero/src/routes/auth-api/dbconnections.ts:185-191
Timestamp: 2024-12-16T08:03:19.741Z
Learning: In the `change_password` route in `packages/authhero/src/routes/auth-api/dbconnections.ts`, the IP address is already included in the session data via `getClientInfo`.
Learnt from: CR
Repo: markusahlstrand/authhero PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-25T13:20:21.406Z
Learning: Applies to src/dataProvider.ts,src/**/*{api,service,provider}*.{ts,tsx} : Always include tenant ID in API request headers for data scoping to specified tenant
Learnt from: markusahlstrand
Repo: markusahlstrand/authhero PR: 173
File: apps/react-admin/src/components/users/edit.tsx:552-566
Timestamp: 2025-08-14T07:49:13.288Z
Learning: The user markusahlstrand prefers to validate API endpoint changes in separate PRs before implementing suggested fixes, especially for user permission management endpoints.
This was referenced Apr 3, 2026
Merged
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
1 participant
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.
Summary by CodeRabbit
New Features
Behavior / UX
Documentation
Localization