Uh oh!
There was an error while loading. Please reload this page.
Bug: startNewFlow uses absolute path "/self-service/...", breaks integrations mounted under a sub-path - #597
Conversation
|
@hersentino is attempting to deploy a commit to the ory Team on Vercel. A member of the Team first needs to authorize it. |
|
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nextjs/src/app/utils.ts (1)
65-69:⚠️ Potential issue | 🟠 MajorRelative path breaks when baseUrl has no trailing slash—baseUrl is normalized to strip trailing slashes in production
Line 65 assumes baseUrl has a trailing slash, but
orySdkUrl()andgetProjectApiKey()explicitly remove them. Withnew URL("self-service/...", "https://example.com/app"), theappsegment is dropped instead of appending the flow path.The codebase already has
joinUrlPaths()utility (inpackages/nextjs/src/utils/utils.ts) with tests covering this exact scenario. Either use that utility or implement the path-aware fix suggested below.Suggested fix
export function startNewFlow( params: QueryParams, flowType: FlowType, baseUrl: string, ) { // Take advantage of the fact, that Ory handles the flow creation for us and redirects the user to the default // return to automatically if they're logged in already. - return redirect(- new URL(- "self-service/" +- flowType.toString() +- "/browser?" +- urlQueryToSearchParams(params).toString(),- baseUrl,- ).toString(),- RedirectType.replace,- )+ const flowUrl = new URL(baseUrl)+ const basePath = flowUrl.pathname.endsWith("/")+ ? flowUrl.pathname+ : `${flowUrl.pathname}/`+ const query = urlQueryToSearchParams(params).toString()++ flowUrl.pathname = `${basePath}self-service/${flowType.toString()}/browser`+ flowUrl.search = `?${query}`++ return redirect(flowUrl.toString(), RedirectType.replace) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nextjs/src/app/utils.ts` around lines 65 - 69, The URL construction currently concatenates "self-service/" + flowType + "/browser?" + urlQueryToSearchParams(params).toString() with baseUrl which fails when baseUrl has no trailing slash; update the code that builds the return URL in packages/nextjs/src/app/utils.ts (the function using flowType, urlQueryToSearchParams and baseUrl) to use the existing joinUrlPaths(...) utility from packages/nextjs/src/utils/utils.ts (or implement equivalent path-aware joining) to combine baseUrl and the relative path before appending the query string so that base path segments are preserved even when baseUrl has no trailing slash.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/nextjs/src/app/utils.ts`:
- Around line 65-69: The URL construction currently concatenates "self-service/"
+ flowType + "/browser?" + urlQueryToSearchParams(params).toString() with
baseUrl which fails when baseUrl has no trailing slash; update the code that
builds the return URL in packages/nextjs/src/app/utils.ts (the function using
flowType, urlQueryToSearchParams and baseUrl) to use the existing
joinUrlPaths(...) utility from packages/nextjs/src/utils/utils.ts (or implement
equivalent path-aware joining) to combine baseUrl and the relative path before
appending the query string so that base path segments are preserved even when
baseUrl has no trailing slash.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 82bdff4f-7602-4bbb-8a0d-0e24028979b3
📒 Files selected for processing (1)
packages/nextjs/src/app/utils.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #597 +/- ##
===========================================
+ Coverage 42.43% 59.79% +17.36%
===========================================
Files 136 182 +46 Lines 2008 3557 +1549 Branches 288 563 +275 ===========================================
+ Hits 852 2127 +1275 - Misses 1149 1306 +157 - Partials 7 124 +117
🚀 New features to boost your workflow:
|
Preflight checklist
Ory Network Project
Self-hosted Ory Kratos (proxied through
@ory/nextjsmiddleware).Describe the bug
startNewFlow()in@ory/nextjs/appbuilds the redirect URL with a leading slash:Because the first argument starts with
/, the URL constructor treats it as anabsolute path and discards any path prefix carried by
baseUrl.baseUrlhere comes fromguessPotentiallyProxiedOrySdkUrl({ knownProxiedUrl: await getPublicUrl() }),which returns the public origin of the Next.js app (the URL the browser sees).
For any deployment where the Next.js app is mounted under a sub-path —
e.g. behind an ingress / reverse proxy that routes
https://example.com/app/*to the app, or when using Next.js
basePath— the redirect drops the prefixand points to
https://example.com/self-service/...instead ofhttps://example.com/app/self-service/.... The middleware never sees therequest, the Kratos proxy never runs, and the user lands on a 404 (or worse,
an unrelated route on the parent domain).
This affects every flow that goes through
getFlowFactory→startNewFlow:login, registration, recovery, verification, settings.
The fix is to drop the leading
/so the path is resolved relative tobaseUrl, which is the documented contract ofnew URL(input, base):When
baseUrlhas no sub-path (the common case), the resulting URL isidentical, so this change is backwards-compatible for vanilla deployments
and fixes sub-path deployments.
Reproducing the bug
@ory/nextjsbehind a reverse proxy that mountsit under a sub-path, e.g.
https://example.com/app/. Make sure the proxyforwards
Host/X-Forwarded-*headers sogetPublicUrl()returnshttps://example.com/app/.https://example.com/app/loginwithout an existing flow ID.Location: https://example.com/self-service/login/browser?...instead of
https://example.com/app/self-service/login/browser?....@ory/nextjsmiddleware (matched on/app/self-service/*) never runs,so the request never reaches Kratos and the browser hits a 404 / unrelated route.
The same problem occurs with Next.js
basePath: "/app"configured innext.config.ts.Relevant log output
Relevant configuration
Version
@ory/nextjs@1.0.0-rc.1On which operating system are you observing this issue?
Linux (containerised, behind ingress)
In which environment are you deploying?
Kubernetes (Helm), with an ingress mounting the app under a sub-path.
Additional Context
Source location:
src/app/flow.ts→startNewFlow.Spec reference for the URL constructor behavior:
https://url.spec.whatwg.org/#concept-url-parser — an input starting with
/is parsed as a path-absolute URL and replaces
base.pathname.I'm opening a PR with the one-character fix.
Summary by CodeRabbit