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:
📝 WalkthroughWalkthroughAdds OpenID Connect (OIDC) Single Sign-On: docs and .env entries, runtime flags, frontend sign-in/sign-up SSO buttons and handlers, auth-client plugin registration, backend global and per-organization SSO config, trusted-origin adjustments, DB migration/schema for Changes
Sequence Diagram(s)sequenceDiagram
participant User as Browser User
participant FE as Frontend (Auth Page)
participant AC as Auth Client
participant BE as Backend (App Server)
participant IdP as Identity Provider
User->>FE: Click "Sign in with SSO"
FE->>AC: authClient.signIn.oauth2({ providerId: "oidc", callbackURL })
AC->>BE: Initiate OIDC auth flow (redirect / authorize)
BE->>IdP: Discovery + Authorization Request (issuer validation, PKCE)
IdP-->>User: Redirect to IdP login
User->>IdP: Authenticate
IdP-->>FE: Redirect back with code (callback URL)
FE->>BE: Callback request with code
BE->>IdP: Exchange code for tokens, validate issuer
IdP-->>BE: ID token + profile
BE->>BE: mapProfileToUser() → create/update user & session
BE-->>FE: Redirect to dashboard with session cookie
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
🚅 Deployed to the reqcore-pr-136 environment in applirank
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
nuxt.config.ts (1)
216-223: Minor inconsistency in OIDC enablement check vs. env schema.The
oidcEnabledcheck uses rawprocess.envtruthiness, whileserver/utils/env.tsusesemptyToUndefinedpreprocessing that converts whitespace-only strings (e.g.," ") toundefined.If someone sets
OIDC_CLIENT_ID=" "(whitespace only), this config would evaluateoidcEnabled: true(whitespace string is truthy), but the server-side env validation would reject it, causing the SSO button to appear but fail at runtime.This is an unlikely edge case, but for consistency you could trim the values:
♻️ Optional: Align with server-side preprocessing
/** Whether OIDC SSO is enabled (all three OIDC env vars are set) */ oidcEnabled: !!( - process.env.OIDC_CLIENT_ID && - process.env.OIDC_CLIENT_SECRET && - process.env.OIDC_DISCOVERY_URL + process.env.OIDC_CLIENT_ID?.trim() && + process.env.OIDC_CLIENT_SECRET?.trim() && + process.env.OIDC_DISCOVERY_URL?.trim() ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nuxt.config.ts` around lines 216 - 223, The oidcEnabled check in nuxt.config.ts uses raw process.env truthiness which treats whitespace-only values as truthy; update the oidcEnabled computation to mirror the server-side preprocessing (e.g., use the same emptyToUndefined logic or trim+validate the three vars) so that OIDC_CLIENT_ID, OIDC_CLIENT_SECRET, and OIDC_DISCOVERY_URL that are whitespace-only evaluate as undefined/false; reference the oidcEnabled property and the process.env keys and align with server/utils/env.ts behavior (or call the same helper) to ensure consistency with server-side validation and prevent the SSO button showing for invalid env values.server/utils/env.ts (1)
117-118: Enforce OIDC discovery URL format in schema validation.The comment correctly states that
OIDC_DISCOVERY_URLmust point to a.well-known/openid-configurationendpoint, which aligns with better-auth'sgenericOAuthdocumentation. However, the schema validation only checks generic URL format withz.string().url(), allowing misconfigured values likehttps://example.comorhttp://insecure.urlto pass validation but fail at runtime.Add validation to enforce:
- HTTPS protocol (required for OIDC)
- The
.well-known/openid-configurationendpoint path (or at least reject URLs missing it)This prevents misconfiguration from reaching runtime where better-auth would reject it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/utils/env.ts` around lines 117 - 118, The OIDC_DISCOVERY_URL schema currently allows any valid URL but must enforce HTTPS and the .well-known/openid-configuration path; update the OIDC_DISCOVERY_URL definition (the symbol named OIDC_DISCOVERY_URL that uses emptyToUndefined.pipe(z.string().url()).optional()) to add a zod refinement that ensures the URL's protocol is "https:" and that the pathname ends with or contains ".well-known/openid-configuration" (or rejects values missing that segment), preserving optional() and emptyToUndefined behavior so invalid configs fail validation early.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/pages/auth/sign-in.vue`:
- Around line 78-93: handleSsoSignIn currently hardcodes callbackURL to
localePath("/dashboard") and therefore drops any invitation context; change it
to read the invitation token from route.query.invitation (same source used by
handleSignIn) and, when present, set callbackURL to the invitation-accepting
route (e.g., localePath("/accept-invitation") or the same path used by
handleSignIn) including the invitation query param so SSO redirects back to the
invitation flow after auth; otherwise fall back to localePath("/dashboard").
Update the call in authClient.signIn.oauth2 to use this conditional callbackURL
and keep existing error/isLoading handling in handleSsoSignIn.
In `@app/pages/auth/sign-up.vue`:
- Around line 92-107: handleSsoSignUp doesn't include the pendingInvitation
context when calling authClient.signIn.oauth2, so SSO users lose their
invitation; update handleSsoSignUp to build the callbackURL using the existing
pendingInvitation (same param used in the email flow)—e.g. if pendingInvitation
is present set callbackURL to
localePath(`/auth/accept-invitation/${pendingInvitation}`) otherwise fallback to
localePath("/dashboard")—and pass that callbackURL into authClient.signIn.oauth2
to preserve invitation context.
In `@server/utils/auth.ts`:
- Around line 126-138: The mapProfileToUser function currently assigns
profile.email directly which can be undefined; update mapProfileToUser to
validate and normalize the email (e.g., check profile.email is a non-empty
string and optionally fallback to profile.preferred_username or null) and ensure
downstream code expects a nullable email. Specifically modify the async
mapProfileToUser(profile) implementation to perform a defensive check on
profile.email, sanitize/trim it, and return a stable value (empty string or
null) when absent so callers of mapProfileToUser (user creation/login code)
won't receive undefined.
In `@server/utils/env.ts`:
- Around line 8-11: The emptyToUndefined helper currently uses z.preprocess with
a fixed inner schema, causing `.optional()` placed outside to not short-circuit
when preprocess returns undefined; refactor emptyToUndefined into a function
that accepts a zod schema (e.g., const emptyToUndefined = (schema) =>
z.preprocess((val) => (typeof val === "string" && val.trim() === "" ? undefined
: val), schema)), then update every schema usage (examples: BETTER_AUTH_SECRET,
RAILWAY_GIT_PR_NUMBER and other entries using emptyToUndefined or
z.preprocess(...).optional()) so that optionality is applied to the inner schema
argument (e.g., pass z.string().optional() or z.string().min(...)) rather than
calling .optional() on the preprocess wrapper.
---
Nitpick comments:
In `@nuxt.config.ts`:
- Around line 216-223: The oidcEnabled check in nuxt.config.ts uses raw
process.env truthiness which treats whitespace-only values as truthy; update the
oidcEnabled computation to mirror the server-side preprocessing (e.g., use the
same emptyToUndefined logic or trim+validate the three vars) so that
OIDC_CLIENT_ID, OIDC_CLIENT_SECRET, and OIDC_DISCOVERY_URL that are
whitespace-only evaluate as undefined/false; reference the oidcEnabled property
and the process.env keys and align with server/utils/env.ts behavior (or call
the same helper) to ensure consistency with server-side validation and prevent
the SSO button showing for invalid env values.
In `@server/utils/env.ts`:
- Around line 117-118: The OIDC_DISCOVERY_URL schema currently allows any valid
URL but must enforce HTTPS and the .well-known/openid-configuration path; update
the OIDC_DISCOVERY_URL definition (the symbol named OIDC_DISCOVERY_URL that uses
emptyToUndefined.pipe(z.string().url()).optional()) to add a zod refinement that
ensures the URL's protocol is "https:" and that the pathname ends with or
contains ".well-known/openid-configuration" (or rejects values missing that
segment), preserving optional() and emptyToUndefined behavior so invalid configs
fail validation early.
🪄 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: 8288327e-2b25-4670-8e45-a07dec707ff7
📒 Files selected for processing (9)
.env.exampleSELF-HOSTING.mdapp/pages/auth/sign-in.vueapp/pages/auth/sign-up.vueapp/utils/auth-client.tsnuxt.config.tsserver/utils/auth.tsserver/utils/env.tstests/unit/oidc-sso.test.ts
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
app/pages/auth/sign-in.vue (1)
95-110:⚠️ Potential issue | 🟠 MajorSSO sign-in doesn't preserve pending invitation context.
Both
handleSelfHostedSsoandhandleEnterpriseSsohardcodecallbackURLto/dashboard, losing any pending invitation fromroute.query.invitation. Users arriving via invitation links would be redirected to the dashboard instead of accepting their invitation after SSO.🔧 Proposed fix to preserve invitation context
async function handleSelfHostedSso() { isLoading.value = true; error.value = ""; + const pendingInvitation = route.query.invitation as string | undefined; + const callbackURL = pendingInvitation + ? localePath(`/auth/accept-invitation/${pendingInvitation}`) + : localePath("/dashboard"); try { await authClient.signIn.oauth2({ providerId: "oidc", - callbackURL: localePath("/dashboard"), + callbackURL, });Apply the same pattern to
handleEnterpriseSso:async function handleEnterpriseSso() { // ... validation ... ssoRedirecting.value = true; error.value = ""; + const pendingInvitation = route.query.invitation as string | undefined; + const callbackURL = pendingInvitation + ? localePath(`/auth/accept-invitation/${pendingInvitation}`) + : localePath("/dashboard"); try { const result = await authClient.signIn.sso({ email: email.value, - callbackURL: localePath("/dashboard"), + callbackURL, errorCallbackURL: localePath("/auth/sign-in"), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/auth/sign-in.vue` around lines 95 - 110, handleSelfHostedSso (and likewise handleEnterpriseSso) hardcode callbackURL to localePath("/dashboard") which drops any pending invitation in route.query.invitation; update both functions to build the callbackURL from localePath("/dashboard") plus the invitation query when present (e.g., append ?invitation=<encoded-value> or add it to existing query string), reading route.query.invitation and only appending when defined/nonnull, and ensure the value is URL-encoded so the authClient.signIn.oauth2 call preserves invitation context through the SSO flow.server/utils/auth.ts (1)
162-174:⚠️ Potential issue | 🟡 MinorMissing email validation in profile mapping.
The
mapProfileToUserfunction usesprofile.emailwithout verifying it exists. Some OIDC providers may not return an email if unverified or if the scope wasn't granted, potentially creating users with undefined email addresses.🛡️ Proposed defensive check
async mapProfileToUser(profile) { + if (!profile.email) { + throw new Error( + "Email is required but was not provided by the identity provider. Ensure the 'email' scope is granted and the user has a verified email.", + ); + } return { name: profile.name || [profile.given_name, profile.family_name] .filter(Boolean) .join(" ") || profile.preferred_username || profile.email, email: profile.email, image: profile.picture, }; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/utils/auth.ts` around lines 162 - 174, The mapProfileToUser mapping currently uses profile.email without validation; update the async function mapProfileToUser to first verify profile.email exists (and optionally profile.email_verified if present) and handle the missing/unverified case instead of returning undefined email—for example, return null (or throw a clear Error) when email is absent/unverified so the caller can stop user creation, and otherwise return the existing object (keep the name/image fallbacks intact); reference profile.email and profile.email_verified in the validation logic inside mapProfileToUser.
🧹 Nitpick comments (1)
app/pages/dashboard/settings/sso.vue (1)
133-146: Consider guardingwindowaccess and improving error handling.While
getCallbackUrlis only called from click handlers (client-side), adding a guard improves robustness. The empty catch block incopyCallbackUrlsilently swallows errors without user feedback.♻️ Suggested improvements
function getCallbackUrl(providerId: string) { + if (!import.meta.client) return '' const base = window.location.origin return `${base}/api/auth/sso/callback/${providerId}` } async function copyCallbackUrl(providerId: string) { try { await navigator.clipboard.writeText(getCallbackUrl(providerId)) copiedProviderId.value = providerId setTimeout(() => { copiedProviderId.value = null }, 2000) } catch { - // Fallback for non-HTTPS environments + // Clipboard API unavailable (non-HTTPS or denied permission) + formError.value = 'Could not copy to clipboard. Please copy the URL manually.' } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/dashboard/settings/sso.vue` around lines 133 - 146, getCallbackUrl currently accesses window unguarded and copyCallbackUrl swallows errors; update getCallbackUrl to check for typeof window !== 'undefined' (or use optional chaining on window.location.origin) and fall back to a relative path (e.g., `/api/auth/sso/callback/${providerId}`) when origin is unavailable, and update copyCallbackUrl to catch the error into a variable, log it (console.error/processLogger), and provide user feedback (set copiedProviderId appropriately or surface an error message) and optionally attempt a DOM-based clipboard fallback if navigator.clipboard fails; reference getCallbackUrl and copyCallbackUrl and the reactive copiedProviderId.value when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/pages/dashboard/settings/sso.vue`:
- Line 420: The template references window directly which causes a
TypeScript/Vue error; add a computed property named appOrigin in the <script
setup> that returns window.location.origin when running on client (use
import.meta.client) and returns the fallback 'https://app.reqcore.com' on
server, then replace the inline typeof window check in the template (the
Redirect URI template expression) with appOrigin so the template no longer
references window directly.
In `@server/api/sso/providers.get.ts`:
- Around line 9-19: The code reimplements org authorization but doesn't check
for a missing active organization; ensure you validate
session.session.activeOrganizationId after calling requireAuth and before
calling (auth.api as any).hasPermission: if activeOrganizationId is
null/undefined/empty, throw createError({ statusCode: 403, statusMessage:
'Forbidden: no active organization' }) (matching the behavior in
requirePermission), otherwise pass the org id into the permission check so
org-scoped queries use a valid id; update the block around requireAuth/session
and the hasPermission call in providers.get.ts to perform this explicit
active-org check.
In `@server/api/sso/providers.post.ts`:
- Around line 20-21: Check for a missing active organization before calling
registerSSOProvider: after obtaining session via requireAuth(event) and reading
session.session.activeOrganizationId into orgId, validate that orgId is present
(not null/undefined) and if missing return or throw an appropriate HTTP error
(e.g., 400/401) with a clear message instead of calling registerSSOProvider;
update the flow that calls registerSSOProvider to only proceed when orgId is
truthy so registration always has proper organization context.
In `@server/api/sso/providers/`[id].delete.ts:
- Around line 14-24: The code currently reads
session.session.activeOrganizationId and runs a manual (auth.api as
any).hasPermission check which misses the explicit "No active organization"
failure; replace this manual logic with the shared requirePermission helper so
the active organization presence and permission check are enforced consistently:
call requirePermission(event, { permissions: { organization: ['update'] } }) (or
the helper's actual signature), use the returned activeOrganizationId instead of
session.session.activeOrganizationId, and remove the existing
auth.api.hasPermission block and its custom 403 throw to rely on
requirePermission's error handling.
- Around line 28-42: Replace the two-step select-then-delete TOCTOU pattern
around ssoProvider with a single atomic delete that returns the deleted row;
call db.delete(ssoProvider).where(and(eq(ssoProvider.id, id),
eq(ssoProvider.organizationId, orgId))).returning(...) and check the returned
rows — if none were returned, throw createError({ statusCode: 404,
statusMessage: 'SSO provider not found' }); otherwise treat it as success;
remove the initial select and ensure you reference ssoProvider and orgId in the
single delete call.
In `@server/database/schema/sso.ts`:
- Around line 19-21: Change the SSO schema field organizationId to be mandatory
by making the column definition organizationId:
text('organization_id').notNull().references(() => organization.id, { onDelete:
'cascade' }) in server/database/schema/sso.ts, then update the migration
0019_noisy_strong_guy.sql to apply the NOT NULL constraint (e.g. ALTER TABLE ...
ALTER COLUMN organization_id SET NOT NULL) and include any necessary data
migration steps (backfill null organization_id rows to a valid org or
delete/abort if appropriate) so the DB constraint and existing data stay
consistent with the new not-null requirement.
---
Duplicate comments:
In `@app/pages/auth/sign-in.vue`:
- Around line 95-110: handleSelfHostedSso (and likewise handleEnterpriseSso)
hardcode callbackURL to localePath("/dashboard") which drops any pending
invitation in route.query.invitation; update both functions to build the
callbackURL from localePath("/dashboard") plus the invitation query when present
(e.g., append ?invitation=<encoded-value> or add it to existing query string),
reading route.query.invitation and only appending when defined/nonnull, and
ensure the value is URL-encoded so the authClient.signIn.oauth2 call preserves
invitation context through the SSO flow.
In `@server/utils/auth.ts`:
- Around line 162-174: The mapProfileToUser mapping currently uses profile.email
without validation; update the async function mapProfileToUser to first verify
profile.email exists (and optionally profile.email_verified if present) and
handle the missing/unverified case instead of returning undefined email—for
example, return null (or throw a clear Error) when email is absent/unverified so
the caller can stop user creation, and otherwise return the existing object
(keep the name/image fallbacks intact); reference profile.email and
profile.email_verified in the validation logic inside mapProfileToUser.
---
Nitpick comments:
In `@app/pages/dashboard/settings/sso.vue`:
- Around line 133-146: getCallbackUrl currently accesses window unguarded and
copyCallbackUrl swallows errors; update getCallbackUrl to check for typeof
window !== 'undefined' (or use optional chaining on window.location.origin) and
fall back to a relative path (e.g., `/api/auth/sso/callback/${providerId}`) when
origin is unavailable, and update copyCallbackUrl to catch the error into a
variable, log it (console.error/processLogger), and provide user feedback (set
copiedProviderId appropriately or surface an error message) and optionally
attempt a DOM-based clipboard fallback if navigator.clipboard fails; reference
getCallbackUrl and copyCallbackUrl and the reactive copiedProviderId.value when
making these changes.
🪄 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: 28c2fd3d-13b4-43ca-8c38-6ef59b7dacb5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
app/components/SettingsMobileNav.vueapp/components/SettingsSidebar.vueapp/pages/auth/sign-in.vueapp/pages/dashboard/settings/sso.vueapp/utils/auth-client.tspackage.jsonserver/api/sso/providers.get.tsserver/api/sso/providers.post.tsserver/api/sso/providers/[id].delete.tsserver/database/migrations/0019_noisy_strong_guy.sqlserver/database/migrations/meta/0019_snapshot.jsonserver/database/migrations/meta/_journal.jsonserver/database/schema/index.tsserver/database/schema/sso.tsserver/utils/auth.ts
✅ Files skipped from review due to trivial changes (1)
- server/database/schema/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/utils/auth-client.ts
Comment on lines
+19
to
+21
| userId: text('user_id').notNull().references(() => user.id, { onDelete: 'cascade' }), | ||
| providerId: text('provider_id').notNull(), | ||
| organizationId: text('organization_id').references(() => organization.id, { onDelete: 'cascade' }), |
There was a problem hiding this comment.
organizationId should be mandatory for org-scoped SSO providers.
Every new provider endpoint in this PR scopes records by active organization. Leaving Line 21 nullable means you can persist SSO providers that the GET/DELETE API can never see again, which is a bad fit for a tenant-owned auth resource.
Proposed fix
- organizationId: text('organization_id').references(() => organization.id, { onDelete: 'cascade' }),
+ organizationId: text('organization_id').notNull().references(() => organization.id, { onDelete: 'cascade' }),Please regenerate or update server/database/migrations/0019_noisy_strong_guy.sql to keep the DB constraint aligned.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| userId: text('user_id').notNull().references(() => user.id, { onDelete: 'cascade' }), | |
| providerId: text('provider_id').notNull(), | |
| organizationId: text('organization_id').references(() => organization.id, { onDelete: 'cascade' }), | |
| userId: text('user_id').notNull().references(() => user.id, { onDelete: 'cascade' }), | |
| providerId: text('provider_id').notNull(), | |
| organizationId: text('organization_id').notNull().references(() => organization.id, { onDelete: 'cascade' }), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/database/schema/sso.ts` around lines 19 - 21, Change the SSO schema
field organizationId to be mandatory by making the column definition
organizationId: text('organization_id').notNull().references(() =>
organization.id, { onDelete: 'cascade' }) in server/database/schema/sso.ts, then
update the migration 0019_noisy_strong_guy.sql to apply the NOT NULL constraint
(e.g. ALTER TABLE ... ALTER COLUMN organization_id SET NOT NULL) and include any
necessary data migration steps (backfill null organization_id rows to a valid
org or delete/abort if appropriate) so the DB constraint and existing data stay
consistent with the new not-null requirement.
- Introduced unit tests for SSO integration readiness, validating environment variables and OIDC configurations. - Implemented tests for SSO provider API validation, ensuring input sanitization and domain validation. - Added security-focused tests for SSO, covering domain uniqueness, provider ID uniqueness, and response shape to prevent secret leakage. - Developed tests for trusted origins resolver logic, verifying URL parsing and filtering for SSO callback flows. - Enhanced input sanitization edge cases to prevent injection attacks and ensure robust validation.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
server/utils/env.ts (1)
114-123:⚠️ Potential issue | 🟠 MajorThe
emptyToUndefined.pipe(...).optional()pattern may reject empty strings instead of treating them as unset.When
emptyToUndefinedpreprocesses""toundefined, the piped inner schema (z.string().min(1)) receivesundefinedand fails validation—.optional()on the outer layer doesn't help because it only skips validation when the original input isundefined.For Railway deployments where env vars may be set to
"", this could cause unexpected validation failures.Recommended refactor
Move
.optional()inside the pipe so the inner schema acceptsundefined:-OIDC_CLIENT_ID: emptyToUndefined.pipe(z.string().min(1)).optional(), -OIDC_CLIENT_SECRET: emptyToUndefined.pipe(z.string().min(1)).optional(), -OIDC_DISCOVERY_URL: emptyToUndefined.pipe( - z.string().url().refine( - (url) => url.startsWith('https://') || url.startsWith('http://'), - 'OIDC_DISCOVERY_URL must use HTTPS (or HTTP for local development)', - ), -).optional(), +OIDC_CLIENT_ID: emptyToUndefined.pipe(z.string().min(1).optional()), +OIDC_CLIENT_SECRET: emptyToUndefined.pipe(z.string().min(1).optional()), +OIDC_DISCOVERY_URL: emptyToUndefined.pipe( + z.string().url().refine( + (url) => url.startsWith('https://') || url.startsWith('http://'), + 'OIDC_DISCOVERY_URL must use HTTPS (or HTTP for local development)', + ).optional(), +),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/utils/env.ts` around lines 114 - 123, The env schema uses emptyToUndefined.pipe(z.string().min(1)).optional(), which still fails when emptyToUndefined turns "" into undefined because the inner schema rejects undefined; update the three entries (OIDC_CLIENT_ID, OIDC_CLIENT_SECRET, OIDC_DISCOVERY_URL) to move .optional() inside the pipe so the inner schema accepts undefined (e.g., emptyToUndefined.pipe(z.string().min(1).optional()) for the first two and emptyToUndefined.pipe(z.string().url().refine(...).optional()) for OIDC_DISCOVERY_URL), ensuring empty strings are treated as unset while preserving the existing validations.app/pages/dashboard/settings/sso.vue (1)
425-425:⚠️ Potential issue | 🔴 CriticalFix TypeScript error:
windowis not available in Vue template context.The pipeline is failing because Vue templates cannot access
windowdirectly. Thetypeof window !== 'undefined'guard doesn't help since the template compiler still seeswindowas an undefined property.Proposed fix using a computed property
Add this computed property in the
<script setup>section:const appOrigin = computed(() => { if (import.meta.client) { return window.location.origin } return 'https://app.reqcore.com' })Then update line 425:
-<li>Set the <strong>Redirect URI</strong> to: <code class="bg-surface-100 dark:bg-surface-800 px-1 py-0.5 rounded text-xs">{{ `${typeof window !== 'undefined' ? window.location.origin : 'https://app.reqcore.com'}/api/auth/sso/callback/{provider-id}` }}</code></li> +<li>Set the <strong>Redirect URI</strong> to: <code class="bg-surface-100 dark:bg-surface-800 px-1 py-0.5 rounded text-xs">{{ `${appOrigin}/api/auth/sso/callback/{provider-id}` }}</code></li>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/dashboard/settings/sso.vue` at line 425, The template is referencing window directly which the Vue compiler treats as an undefined template property; add a computed property named appOrigin in the <script setup> that returns window.location.origin when running client-side (use import.meta.client) and returns the fallback 'https://app.reqcore.com' for SSR, then replace the inline window expression in the template with the appOrigin computed value (use `${appOrigin}/api/auth/sso/callback/{provider-id}`) so the template no longer references window directly.app/pages/auth/sign-in.vue (1)
95-103:⚠️ Potential issue | 🟠 MajorSSO flows still drop invitation context on redirect.
Line 101 and Line 129 always send users to
/dashboard, unlikehandleSignInwhich preservesroute.query.invitation. This breaks invite acceptance after SSO. Also preserve invitation onerrorCallbackURLso failed SSO returns to the same invite-aware page.🔧 Suggested fix
async function handleSelfHostedSso() { isLoading.value = true; error.value = ""; + const pendingInvitation = route.query.invitation as string | undefined; + const callbackURL = pendingInvitation + ? localePath(`/auth/accept-invitation/${pendingInvitation}`) + : localePath("/dashboard"); try { await authClient.signIn.oauth2({ providerId: "oidc", - callbackURL: localePath("/dashboard"), + callbackURL, }); } catch (e: unknown) {async function handleEnterpriseSso() { @@ + const pendingInvitation = route.query.invitation as string | undefined; + const callbackURL = pendingInvitation + ? localePath(`/auth/accept-invitation/${pendingInvitation}`) + : localePath("/dashboard"); + const errorCallbackURL = pendingInvitation + ? localePath(`/auth/sign-in?invitation=${encodeURIComponent(pendingInvitation)}`) + : localePath("/auth/sign-in"); + try { const result = await authClient.signIn.sso({ email: email.value, - callbackURL: localePath("/dashboard"), - errorCallbackURL: localePath("/auth/sign-in"), + callbackURL, + errorCallbackURL, });Also applies to: 116-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/auth/sign-in.vue` around lines 95 - 103, The SSO flow in handleSelfHostedSso (and the related SSO handler around lines 116-131) always redirects to localePath("/dashboard") and thus drops route.query.invitation; update the authClient.signIn.oauth2 call(s) to preserve invitation by building callbackURL and errorCallbackURL that include the invitation query when present (e.g., if route.query.invitation exists append ?invitation=... or merge into existing query), mirroring the behavior in handleSignIn so successful and failed SSO returns keep the invite context.
🧹 Nitpick comments (1)
tests/unit/sso-integration-readiness.test.ts (1)
268-278: Avoid mirroringresolveBetterAuthUrllogic inside tests.Re-implementing production logic in-test risks lockstep bugs and false positives. Prefer importing the real function (or extracting/exporting it) and testing that directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/sso-integration-readiness.test.ts` around lines 268 - 278, The test defines a duplicate implementation of resolveBetterAuthUrl which risks diverging from production; instead import the real resolveBetterAuthUrl from its source (or extract and export it from the production module into a shared util) and use that in tests; update tests/unit/sso-integration-readiness.test.ts to remove the inline resolveBetterAuthUrl function and replace calls with the imported resolveBetterAuthUrl so the test exercises actual production logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/pages/auth/sign-in.vue`:
- Around line 55-60: Wrap the call to authClient.signIn.email({ email:
email.value, password: password.value }) in a try/catch/finally block so
isLoading.value is always reset; in the try run the sign-in, in catch set a
user-facing error state (e.g., assign to your existing error variable or create
one) with the caught error message, and in finally set isLoading.value = false
to guarantee recovery even on network/runtime exceptions.
In `@tests/unit/sso-integration-readiness.test.ts`:
- Around line 199-228: Replace the ad-hoc rolePermissions map and the local
hasPermission helper in the test with assertions that import and use the
authoritative permission definitions used by the application (e.g., the shared
permission matrix or policy object) so tests reflect real authorization;
specifically, remove the local rolePermissions and hasPermission and import the
shared permission source (the module that exports the app's role-to-permissions
mapping or policy) and call its lookup/check method (or use the same utility the
app uses to evaluate resource/action permissions) to assert that 'owner' and
'admin' have organization:update and 'member' and unknown roles do not.
- Around line 236-261: Replace the tautological literals in
tests/unit/sso-integration-readiness.test.ts with assertions against the real
OIDC config produced by your auth utilities: import the OIDC config
builder/export from server/utils/auth.ts (e.g., the function that returns
global/per-organization oidc config) and call it to get the runtime object, then
assert its pkce, requireIssuerValidation and scopes include 'openid' (and verify
the object shape passed to registerSSOProvider for per-org config) instead of
testing hardcoded local variables.
---
Duplicate comments:
In `@app/pages/auth/sign-in.vue`:
- Around line 95-103: The SSO flow in handleSelfHostedSso (and the related SSO
handler around lines 116-131) always redirects to localePath("/dashboard") and
thus drops route.query.invitation; update the authClient.signIn.oauth2 call(s)
to preserve invitation by building callbackURL and errorCallbackURL that include
the invitation query when present (e.g., if route.query.invitation exists append
?invitation=... or merge into existing query), mirroring the behavior in
handleSignIn so successful and failed SSO returns keep the invite context.
In `@app/pages/dashboard/settings/sso.vue`:
- Line 425: The template is referencing window directly which the Vue compiler
treats as an undefined template property; add a computed property named
appOrigin in the <script setup> that returns window.location.origin when running
client-side (use import.meta.client) and returns the fallback
'https://app.reqcore.com' for SSR, then replace the inline window expression in
the template with the appOrigin computed value (use
`${appOrigin}/api/auth/sso/callback/{provider-id}`) so the template no longer
references window directly.
In `@server/utils/env.ts`:
- Around line 114-123: The env schema uses
emptyToUndefined.pipe(z.string().min(1)).optional(), which still fails when
emptyToUndefined turns "" into undefined because the inner schema rejects
undefined; update the three entries (OIDC_CLIENT_ID, OIDC_CLIENT_SECRET,
OIDC_DISCOVERY_URL) to move .optional() inside the pipe so the inner schema
accepts undefined (e.g., emptyToUndefined.pipe(z.string().min(1).optional()) for
the first two and emptyToUndefined.pipe(z.string().url().refine(...).optional())
for OIDC_DISCOVERY_URL), ensuring empty strings are treated as unset while
preserving the existing validations.
---
Nitpick comments:
In `@tests/unit/sso-integration-readiness.test.ts`:
- Around line 268-278: The test defines a duplicate implementation of
resolveBetterAuthUrl which risks diverging from production; instead import the
real resolveBetterAuthUrl from its source (or extract and export it from the
production module into a shared util) and use that in tests; update
tests/unit/sso-integration-readiness.test.ts to remove the inline
resolveBetterAuthUrl function and replace calls with the imported
resolveBetterAuthUrl so the test exercises actual production logic.
🪄 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: d8aee1da-8af1-42ee-91a6-0f50c10233ca
📒 Files selected for processing (14)
app/components/SettingsSidebar.vueapp/pages/auth/sign-in.vueapp/pages/auth/sign-up.vueapp/pages/dashboard/settings/sso.vueserver/api/sso/providers.get.tsserver/api/sso/providers.post.tsserver/api/sso/providers/[id].delete.tsserver/utils/env.tstests/unit/sso-auth-config.test.tstests/unit/sso-env-extended.test.tstests/unit/sso-integration-readiness.test.tstests/unit/sso-providers-api.test.tstests/unit/sso-security.test.tstests/unit/sso-trusted-origins.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- app/components/SettingsSidebar.vue
- server/api/sso/providers.post.ts
- server/api/sso/providers/[id].delete.ts
- app/pages/auth/sign-up.vue
Comment on lines
+199
to
+228
| // Simulate Better Auth's default org permission structure | ||
| // owner and admin have organization:update, member does not | ||
| const rolePermissions: Record<string, { organization: string[] }> = { | ||
| owner: { organization: ['create', 'update', 'delete'] }, | ||
| admin: { organization: ['create', 'update'] }, | ||
| member: { organization: [] }, | ||
| } | ||
|
|
||
| function hasPermission(role: string, resource: string, action: string): boolean { | ||
| const perms = rolePermissions[role] | ||
| if (!perms) return false | ||
| const actions = (perms as Record<string, string[]>)[resource] | ||
| return actions?.includes(action) ?? false | ||
| } | ||
|
|
||
| it('owner can manage SSO providers', () => { | ||
| expect(hasPermission('owner', 'organization', 'update')).toBe(true) | ||
| }) | ||
|
|
||
| it('admin can manage SSO providers', () => { | ||
| expect(hasPermission('admin', 'organization', 'update')).toBe(true) | ||
| }) | ||
|
|
||
| it('member CANNOT manage SSO providers', () => { | ||
| expect(hasPermission('member', 'organization', 'update')).toBe(false) | ||
| }) | ||
|
|
||
| it('unknown role CANNOT manage SSO providers', () => { | ||
| expect(hasPermission('unknown-role', 'organization', 'update')).toBe(false) | ||
| }) |
There was a problem hiding this comment.
Permission-boundary tests are disconnected from real role definitions.
This local rolePermissions map can diverge from actual roles/policies, so these tests may pass while authorization is broken. Import and assert against the shared permission source used by auth configuration instead of a handcrafted matrix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/sso-integration-readiness.test.ts` around lines 199 - 228, Replace
the ad-hoc rolePermissions map and the local hasPermission helper in the test
with assertions that import and use the authoritative permission definitions
used by the application (e.g., the shared permission matrix or policy object) so
tests reflect real authorization; specifically, remove the local rolePermissions
and hasPermission and import the shared permission source (the module that
exports the app's role-to-permissions mapping or policy) and call its
lookup/check method (or use the same utility the app uses to evaluate
resource/action permissions) to assert that 'owner' and 'admin' have
organization:update and 'member' and unknown roles do not.
Comment on lines
+236
to
+261
| it('global OIDC config has PKCE enabled', () => { | ||
| const globalOidcConfig = { | ||
| scopes: ['openid', 'email', 'profile'], | ||
| pkce: true, | ||
| requireIssuerValidation: true, | ||
| } | ||
| expect(globalOidcConfig.pkce).toBe(true) | ||
| expect(globalOidcConfig.requireIssuerValidation).toBe(true) | ||
| }) | ||
|
|
||
| it('enterprise SSO config has PKCE in oidcConfig', () => { | ||
| // This is what gets passed to Better Auth's registerSSOProvider | ||
| const perOrgConfig = { | ||
| clientId: 'test-client', | ||
| clientSecret: 'test-secret', | ||
| scopes: ['openid', 'email', 'profile'], | ||
| pkce: true, | ||
| } | ||
| expect(perOrgConfig.pkce).toBe(true) | ||
| expect(perOrgConfig.scopes).toContain('openid') | ||
| }) | ||
|
|
||
| it('scopes always include openid (required for OIDC)', () => { | ||
| const scopes = ['openid', 'email', 'profile'] | ||
| expect(scopes.includes('openid')).toBe(true) | ||
| }) |
There was a problem hiding this comment.
PKCE/issuer tests are tautological and don’t validate runtime auth config.
These assertions check hardcoded literals created inside the test, not the actual OIDC config in server/utils/auth.ts. The suite currently can’t catch regressions in real plugin options.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/sso-integration-readiness.test.ts` around lines 236 - 261, Replace
the tautological literals in tests/unit/sso-integration-readiness.test.ts with
assertions against the real OIDC config produced by your auth utilities: import
the OIDC config builder/export from server/utils/auth.ts (e.g., the function
that returns global/per-organization oidc config) and call it to get the runtime
object, then assert its pkce, requireIssuerValidation and scopes include
'openid' (and verify the object shape passed to registerSSOProvider for per-org
config) instead of testing hardcoded local variables.
…il requirement in profile mapping
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/pages/auth/sign-up.vue (1)
53-61:⚠️ Potential issue | 🟠 MajorHandle thrown sign-up failures before checking
result.error.
authClient.signUp.email(...)can still throw on transport/runtime failures, and this path currently leavesisLoadingstuck attruewith no visible error. Please wrap the await the same wayhandleSignIn()does on the sign-in page.🛠️ Suggested hardening
isLoading.value = true; track("signup_submitted"); - const result = await authClient.signUp.email({ - email: email.value, - password: password.value, - name: name.value, - }); + let result: Awaited<ReturnType<typeof authClient.signUp.email>>; + try { + result = await authClient.signUp.email({ + email: email.value, + password: password.value, + name: name.value, + }); + } catch (e: unknown) { + error.value = + e instanceof Error ? e.message : "Sign-up failed. Please try again."; + track("signup_failed", { error_type: "exception" }); + isLoading.value = false; + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/auth/sign-up.vue` around lines 53 - 61, The sign-up flow can throw from authClient.signUp.email(...) and currently leaves isLoading.value true with no error handling; wrap the await call in a try/catch similar to handleSignIn(), set isLoading.value = false in both success and error paths (use finally or explicit sets), handle thrown errors by assigning a user-visible error (e.g., signUpError or the same error state used by handleSignIn) and log/track the failure, and still check result.error after the awaited call; update the block around authClient.signUp.email to mirror the error/cleanup pattern used in handleSignIn().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/pages/auth/sign-in.vue`:
- Around line 15-19: Replace the two separate reactive flags isLoading and
ssoRedirecting with a single shared busy flag (e.g., busy) and update all
references (initial declarations and usages inside the email sign-in handler,
SSO redirect handler, and button disabled bindings) so that starting either auth
flow sets busy = true and both buttons check busy to disable; ensure the flag is
cleared on both success and any error paths in the functions that currently set
isLoading or ssoRedirecting (look for handlers that reference isLoading and
ssoRedirecting and the template bindings that disable buttons).
In `@server/utils/auth.ts`:
- Around line 51-63: The current code in auth.ts loads all ssoProvider rows
(db.select from schema.ssoProvider) and unions every issuer origin into the
global trustedOrigins (idpOrigins/staticOrigins), which both broadens the CSRF
allowlist across tenants and causes a full-table read; instead, change the
lookup to resolve only the issuer relevant to the current SSO flow: accept the
incoming issuer/tenant identifier from the request context, query
schema.ssoProvider with a WHERE filter (e.g., where schema.ssoProvider.issuer =
:issuer OR where tenant_id = :tenantId) using a parameterized db.select, parse
that single provider's issuer URL into an origin, and merge that origin with
staticOrigins (preserving the Set de-dup step) so you no longer read or trust
all rows globally.
---
Duplicate comments:
In `@app/pages/auth/sign-up.vue`:
- Around line 53-61: The sign-up flow can throw from
authClient.signUp.email(...) and currently leaves isLoading.value true with no
error handling; wrap the await call in a try/catch similar to handleSignIn(),
set isLoading.value = false in both success and error paths (use finally or
explicit sets), handle thrown errors by assigning a user-visible error (e.g.,
signUpError or the same error state used by handleSignIn) and log/track the
failure, and still check result.error after the awaited call; update the block
around authClient.signUp.email to mirror the error/cleanup pattern used in
handleSignIn().
🪄 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: 46003dc5-b8e5-4936-8ca8-0e2a81d5c8af
📒 Files selected for processing (4)
app/pages/auth/sign-in.vueapp/pages/auth/sign-up.vueserver/api/sso/providers/[id].delete.tsserver/utils/auth.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- server/api/sso/providers/[id].delete.ts
Comment on lines
+15
to
+19
| const email = ref(""); | ||
| const password = ref(""); | ||
| const error = ref(""); | ||
| const isLoading = ref(false); | ||
| const ssoRedirecting = ref(false); |
There was a problem hiding this comment.
Use one busy state for both auth paths.
isLoading and ssoRedirecting currently disable different buttons, so users can kick off email sign-in and enterprise SSO at the same time. Disable both actions off a shared busy flag to avoid racing two auth flows.
🔧 Suggested simplification
const error = ref("");
const isLoading = ref(false);
const ssoRedirecting = ref(false);
+const authBusy = computed(() => isLoading.value || ssoRedirecting.value);- :disabled="isLoading"
+ :disabled="authBusy"- :disabled="ssoRedirecting"
+ :disabled="authBusy"Also applies to: 240-246, 264-267
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/pages/auth/sign-in.vue` around lines 15 - 19, Replace the two separate
reactive flags isLoading and ssoRedirecting with a single shared busy flag
(e.g., busy) and update all references (initial declarations and usages inside
the email sign-in handler, SSO redirect handler, and button disabled bindings)
so that starting either auth flow sets busy = true and both buttons check busy
to disable; ensure the flag is cleared on both success and any error paths in
the functions that currently set isLoading or ssoRedirecting (look for handlers
that reference isLoading and ssoRedirecting and the template bindings that
disable buttons).
Comment on lines
+51
to
+63
| // Dynamically load registered SSO provider issuers | ||
| try { | ||
| const providers = await db | ||
| .select({ issuer: schema.ssoProvider.issuer }) | ||
| .from(schema.ssoProvider); | ||
|
|
||
| const idpOrigins = providers | ||
| .map((p) => { | ||
| try { return new URL(p.issuer).origin; } catch { return null; } | ||
| }) | ||
| .filter((o): o is string => o !== null); | ||
|
|
||
| return Array.from(new Set([...staticOrigins, ...idpOrigins])); |
There was a problem hiding this comment.
Don't trust every registered issuer globally.
This query unions all sso_provider.issuer origins into trustedOrigins for any SSO request. That lets one tenant's provider registration expand the CSRF allowlist for every other tenant, and it also turns each SSO auth hit into a full-table read. Resolve the issuer for the current SSO flow only instead of loading every row.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/utils/auth.ts` around lines 51 - 63, The current code in auth.ts loads
all ssoProvider rows (db.select from schema.ssoProvider) and unions every issuer
origin into the global trustedOrigins (idpOrigins/staticOrigins), which both
broadens the CSRF allowlist across tenants and causes a full-table read;
instead, change the lookup to resolve only the issuer relevant to the current
SSO flow: accept the incoming issuer/tenant identifier from the request context,
query schema.ssoProvider with a WHERE filter (e.g., where
schema.ssoProvider.issuer = :issuer OR where tenant_id = :tenantId) using a
parameterized db.select, parse that single provider's issuer URL into an origin,
and merge that origin with staticOrigins (preserving the Set de-dup step) so you
no longer read or trust all rows globally.
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
Type of change
Validation
DCO
Signed-off-by) viagit commit -sSummary by CodeRabbit