feat(core): add OAuth2 consent flow support#584
Conversation
🦋 Changeset detectedLatest commit: 9fb70db The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@Jorgagu is attempting to deploy a commit to the ory Team on Vercel. A member of the Team first needs to authorize it. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #584 +/- ##
===========================================
+ Coverage 42.43% 60.64% +18.21%
===========================================
Files 136 186 +50
Lines 2008 3781 +1773
Branches 288 623 +335
===========================================
+ Hits 852 2293 +1441
- Misses 1149 1364 +215
- Partials 7 124 +117
🚀 New features to boost your workflow:
|
Jorgagu
changed the title
|
@vinckr @jonas-jonas @aeneasr Happy New Year ! 🎉 Could you please review this one ? |
|
hi @Jorgagu, thank you very much for this contribution, and happy new year! We'll take a look at this in the coming weeks. We do have some code for this already; it just wasn't ready to be published, so we might need to do some merging with that. And just a heads-up, we're quite busy ramping up after the holidays again, so it might take a couple days longer for us to get to this. |
70b3e84 to
73020fc
Compare
|
hi thanks for this contrib @Jorgagu ! this really helps our team to understand more context on the Ory FE stack here |
jonas-jonas
left a comment
jonas-jonas
left a comment
There was a problem hiding this comment.
Thank you for this contribution! Overall it looks solid. I do have one note about the missing CSRF protection. Happy to discuss this in more detail, if needed!
| consentChallenge={consentRequest} | ||
| session={session} | ||
| config={config} | ||
| csrfToken="" |
There was a problem hiding this comment.
The endpoints used in this component are susceptible to CSRF attacks. This is also the reason the consent screen is not yet part of the elements examples.
Since the <Consent /> component does not yet support server actions, the CSRF protection must be done through middleware. This is kind of awkward to implement, and there aren't many libraries out there that support this.
Ideally, we'd move the Consent screen to a server actions-based form submission because they're CSRF protected by default, but that would make support for the pages router impossible.
https://www.npmjs.com/package/@edge-csrf/nextjs?activeTab=readme is the best library for this (even though it is unmaintained, and vulnerable to subdomain attacks). Could you integrate that into this change?
There was a problem hiding this comment.
Hey @jonas-jonas, thanks for the review!
A few things worth noting about the current state:
- The consent form submits via
fetch()withContent-Type: application/json(line 161 ofuseOryFormSubmit.ts), so cross-origin CSRF is blocked by CORS preflight. - I've added server-side session identity validation on both routers so the server checks
session.identity.idmatches
consentRequest.subjectbefore processing. - The
csrfToken=""is not validated anywhere server-side though, which is a gap if the submission mechanism ever changes.
Regarding @edge-csrf/nextjs, it uses the Naive Double-Submit Cookie Pattern (vulnerable to subdomain attacks) and the maintainer is looking to hand off the project (last stable release: October 2024).
Here are the options I see:
| Approach | Trade-offs |
|---|---|
@edge-csrf/nextjs |
Subdomain vulnerability, uncertain maintenance |
| Signed Double Submit Cookie (custom) | No dependency, works with both routers, no subdomain vulnerability (OWASP recommended) |
| Custom request header | Simplest with fetch(), breaks if we ever switch to HTML form POST |
| Server Actions | CSRF-protected by default, but kills Pages Router support |
I'd lean toward the Signed Double Submit Cookie. What's your preference?
There was a problem hiding this comment.
Thanks for the investigation.
Signed double submit seems like the best solution. However, we generally try not to reinvent the wheel (unless necessary!), and CSRF seems like a well-solved issue, so ideally we use a library to solve this. "Don't roll your own security" applies here.
However, when I last looked into this, there were few libraries that actually solved this for Next.js specifically. Do you know of any?
There was a problem hiding this comment.
I looked into Next.js CSRF libraries that implement the Signed Double Submit Cookie pattern. Here's what I found:
| Package | Pattern | Version | Latest Release | Monthly Downloads | App Router | Pages Router | Status |
|---|---|---|---|---|---|---|---|
@edge-csrf/nextjs |
Naive Double Submit | v2.5.3 | Nov 2024 | 117,278 | Yes | Yes | Maintainer stepping down, subdomain vulnerability |
next-csrf |
Synchronizer Token (stateful) | v0.2.1 | Apr 2022 | 23,608 | No | Yes | Abandoned |
@csrf-armor/nextjs |
Signed Double Submit (HMAC) | v1.4.1 | Feb 2026 | 694 | Yes | Yes | Active |
@simple-csrf/next |
Signed Double Submit | v0.1.1 | Apr 2025 | 176 | Yes | No | Requires React 19 |
@csrf-armor/nextjs is the only library that checks all the boxes: Signed Double Submit (HMAC), both routers, Edge
Runtime compatible, zero dependencies, TypeScript, and CodeQL analysis. The adoption is low, but the code is auditable
and the core package (@csrf-armor/core) is framework-agnostic, which
could allow building a Nuxt adapter on top of it for the Vue/Nuxt side of ory-elements in the future.
What do you think about going with @csrf-armor/nextjs?
There was a problem hiding this comment.
Thanks for the compilation. I think that's right; let's use @csrf-armor/nextjs here. Would you mind adjusting the examples to use the package?
There was a problem hiding this comment.
@jonas-jonas its done, could you please try it and give me review on it ?
0afc868 to
a4ba0fd
Compare
|
hey @Jorgagu, thanks a lot for your work on this, it really helps!
Do you think it make sense to include i18n for the consent flow in this PR or should this be part of a new issue/PR? Thanks in advance for your answer. |
|
Good catch @alexanderNthl, you're right the consent flow doesn't have proper i18n yet. I looked into it and the Kratos already has numeric ID ranges per flow (
The existing string keys ( @jonas-jonas do you think I should include this in the current PR or handle it separately? |
jonas-jonas
left a comment
jonas-jonas
left a comment
There was a problem hiding this comment.
One more comment; otherwise, this looks good! Could you resolve the merge conflicts once more?
| const configWithLogo = getConfigWithOAuth2Logo( | ||
| config, | ||
| flow.oauth2_login_request?.client?.logo_uri, | ||
| ) |
There was a problem hiding this comment.
I don't think this is correct. The login screen is for the main application (think Google, Facebook, etc.), whereas with this change, it would render as the client logo (e.g., My ACME app). This would be very confusing to end users ("Am I logging in with my ACME app account or my google account?").
|
@Jorgagu The message IDs issue has to be addressed; it's on our list internally, but it's out of scope of this PR, as the component already existed before. |
Add getConsentFlow, acceptConsentRequest and rejectConsentRequest for the app router, and getServerSideConsentFlow plus useSession for the pages router. The consent request is always fetched server side since reading it requires the Hydra admin API and the project API token. Accepting and rejecting validate that the session identity matches the consent request subject to prevent consent hijacking with a stolen consent_challenge. URL rewriting is consent-aware in rewriteUrls.
The consent card lists the requested scopes as grant_scope checkboxes and shows the OAuth2 client name. The client logo override only applies to the consent screen: per review feedback, login and registration keep the application's own branding.
Add consent pages and API routes to the app router, custom components, and pages router examples. CSRF protection uses the @csrf-armor/nextjs middleware plus an explicit double-submit comparison in the consent routes, since the library falls back to the csrf cookie when extracting the submitted token. Middleware path matching is aligned with the @ory/nextjs proxy match list, and the consent pages only render when the session identity matches the consent request subject.
a4ba0fd to
7ec7ef5
Compare
📝 WalkthroughWalkthroughAdds OAuth2 consent APIs for Next.js App and Pages Routers, consent-specific Elements rendering, CSRF-protected consent examples, client branding support, and OAuth2-aware URL rewriting. ChangesOAuth2 consent flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant ConsentPage
participant NextjsAPI
participant OAuth2Service
Browser->>ConsentPage: request consent challenge
ConsentPage->>NextjsAPI: load consent request and session
NextjsAPI->>OAuth2Service: fetch consent request
OAuth2Service-->>NextjsAPI: consent request
ConsentPage-->>Browser: render consent form
Browser->>NextjsAPI: submit CSRF-protected decision
NextjsAPI->>OAuth2Service: accept or reject consent
OAuth2Service-->>Browser: redirect_to
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ 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 |
|
@jonas-jonas the branch is rebased on the current main and the merge conflicts are resolved. Changes since your last review:
The PR description is updated accordingly. |
![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 (8)
packages/nextjs/src/pages/client.ts (1)
27-29: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRemove redundant and unsafe
window.location.originaccess.
guessPotentiallyProxiedOrySdkUrlalready safely defaults towindow.location.originwhen running on the client. Accessingwindow.location.originunconditionally here bypasses that safety and will throw aReferenceErrorif this factory is ever accidentally invoked during Server-Side Rendering (SSR).♻️ Proposed refactor
- basePath: guessPotentiallyProxiedOrySdkUrl({ - knownProxiedUrl: window.location.origin, - }), + basePath: guessPotentiallyProxiedOrySdkUrl(),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nextjs/src/pages/client.ts` around lines 27 - 29, Remove the explicit knownProxiedUrl value from the guessPotentiallyProxiedOrySdkUrl call in the client factory, allowing the helper to apply its safe client-side default without unconditionally accessing window during SSR.packages/nextjs/src/pages/consent.ts (1)
76-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider logging the error before swallowing it.
Returning
nullon failure handles invalid challenges gracefully, but silently swallowing all errors (such as 500s or misconfigured API keys) can make debugging difficult. Consider logging the error to help developers identify configuration or network issues.♻️ Proposed refactor
return serverSideOAuth2Client() .getOAuth2ConsentRequest({ consentChallenge }) - .catch(() => null) + .catch((error) => { + console.error("Failed to fetch OAuth2 consent request:", error) + return null + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nextjs/src/pages/consent.ts` around lines 76 - 79, Update the OAuth2 consent request flow in serverSideOAuth2Client().getOAuth2ConsentRequest to log the caught error before returning null. Preserve the existing graceful null fallback while ensuring failures, including configuration and network errors, include useful error details in the established logging mechanism.packages/nextjs/src/pages/session.ts (1)
47-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify state updates with
finally.You can use a
.finally()block to handle setting the loading state, reducing duplication across the success and error paths.♻️ Proposed refactor
.then((session) => { setSession(session) - setLoading(false) - return }) .catch((err: unknown) => { setError(err instanceof Error ? err : new Error(String(err))) - setLoading(false) + }) + .finally(() => { + setLoading(false) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nextjs/src/pages/session.ts` around lines 47 - 55, Refactor the promise chain in the session-loading flow to move setLoading(false) from both the success and error handlers into a shared finally block. Keep setSession in the success handler and setError in the catch handler, preserving their existing behavior.packages/nextjs/src/app/consent.test.ts (1)
82-161: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd tests for the identity-match/mismatch branch.
No test in either
describeblock passesidentityId, so the "Forbidden: Session identity does not match..." guard (the security-critical part of both functions) is never exercised. The mocks here also don't stubgetOAuth2ConsentRequest, so this branch would currently throw if invoked.Add cases for: (1) matching
identityIdproceeds to accept/reject, (2) mismatchedidentityIdthrows.Also applies to: 163-208
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nextjs/src/app/consent.test.ts` around lines 82 - 161, Extend both consent test describe blocks covering acceptConsentRequest and rejectConsentRequest with identityId cases: stub getOAuth2ConsentRequest with a request identity for each test, verify matching identityId proceeds to the underlying accept/reject call, and verify mismatched identityId throws the forbidden error without proceeding. Keep assertions focused on the security guard and ensure the mocks support the lookup path.packages/elements-react/src/theme/default/utils/oauth2-config.ts (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a relative import for internal types.
Importing from the package's own public entry point (
@ory/elements-react) from within its internal source code can cause module resolution or circular dependency issues during the build step. Please use a relative path instead.♻️ Proposed fix
-import { OryClientConfiguration } from "`@ory/elements-react`" +import { OryClientConfiguration } from "../../../util/clientConfiguration"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/elements-react/src/theme/default/utils/oauth2-config.ts` at line 4, Update the OryClientConfiguration import in oauth2-config.ts to use the appropriate relative internal source path instead of the package public entry point, preserving the existing type usage.packages/nextjs/src/utils/rewrite.ts (1)
82-85: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winHandle top-level arrays safely to prevent type conversion issues.
If
objis an Array (e.g., a top-level JSON array response), allowing it to fall through toObject.fromEntries(Object.entries(obj)...)will incorrectly convert it into an Object with stringified numeric keys (e.g.,{"0": item1, "1": item2}).Consider adding early handling for arrays to prevent accidental data corruption.
♻️ Proposed refactor
// Handle null/undefined input to prevent runtime errors if (!obj) { return obj } + if (Array.isArray(obj)) { + return obj + .map((item) => + typeof item === "object" && item !== null + ? rewriteJsonResponse(item, proxyUrl) + : typeof item === "string" && proxyUrl + ? item.replaceAll(orySdkUrl(), proxyUrl) + : item, + ) + .filter((item) => item !== undefined) as unknown as T + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nextjs/src/utils/rewrite.ts` around lines 82 - 85, Update the null/undefined guard in the object-rewrite logic to detect top-level arrays and return them unchanged before the Object.fromEntries(Object.entries(...)) conversion. Preserve the existing handling for non-array objects and nullish inputs.examples/nextjs-app-router/middleware.ts (1)
34-51: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd
export const config = { matcher: [...] }to scope middleware execution.Neither middleware file defines a
matcher, so per Next.js defaults this middleware (CSRF processing + Ory path routing) runs on every request, including static assets, images, and favicon — unlike the@csrf-armor/nextjsdocumented setup, which explicitly excludes_next/static,_next/image, andfavicon.ico.
examples/nextjs-app-router/middleware.ts#L34-L51: add amatcherexport excluding static asset paths.examples/nextjs-app-router-custom-components/middleware.ts#L34-L51: apply the same addition.♻️ Proposed addition
const response = NextResponse.next() const result = await csrfProtect(request, response) if (!result.success) { return NextResponse.json( { error: "CSRF validation failed" }, { status: 403 }, ) } return response } + +export const config = { + matcher: ["/((?!_next/static|_next/image|favicon.ico).*)"], +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/nextjs-app-router/middleware.ts` around lines 34 - 51, Add an exported Next.js middleware config with a matcher that excludes _next/static, _next/image, and favicon.ico from execution. Apply the same configuration alongside middleware in examples/nextjs-app-router/middleware.ts and examples/nextjs-app-router-custom-components/middleware.ts; leave the middleware request handling unchanged.examples/nextjs-app-router/app/api/consent/route.ts (1)
82-114: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate
actionexplicitly instead of defaulting unknown values to reject.Any
body.actionother than exactly"accept"(including missing/malformed values) silently falls through torejectConsentRequest. This fails safe (denies rather than grants), but it hides malformed-request bugs as spurious consent denials instead of a clear400.♻️ Proposed validation
+ if (action !== "accept" && action !== "reject") { + return NextResponse.json( + { error: "invalid_request", error_description: "Invalid or missing action" }, + { status: 400 }, + ) + } + if (!consentChallenge) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/nextjs-app-router/app/api/consent/route.ts` around lines 82 - 114, Validate body.action explicitly before the consent operation: accept only “accept” or “reject”, and return a 400 invalid_request response for missing or any other value. Update the branching around acceptConsentRequest and rejectConsentRequest so rejectConsentRequest is called only when action is exactly “reject”.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/nextjs-app-router/middleware.ts`:
- Around line 42-50: Return the protected response from csrfProtect instead of
the original response. Update the success path in
examples/nextjs-app-router/middleware.ts (lines 42-50) and
examples/nextjs-app-router-custom-components/middleware.ts (lines 42-50) to
return result.response, while preserving the existing CSRF failure response.
In `@examples/nextjs-pages-router/pages/auth/consent.tsx`:
- Around line 12-51: Update getServerSideProps to read and decode the CSRF token
from the request cookie, include it in ConsentPageProps, and pass it through the
returned props. Update ConsentPage to use the prop instead of
getCsrfTokenFromCookie, then remove that client-side helper.
In `@packages/nextjs/src/app/consent.ts`:
- Around line 112-156: Make identity validation mandatory in
acceptConsentRequest by requiring identityId in its options and always
retrieving the consent request to verify its subject matches identityId before
accepting it. Apply the same requirement and validation to rejectConsentRequest,
preferably by extracting and reusing an assertConsentSubject helper so both
consent flows enforce identical checks.
In `@packages/nextjs/src/utils/rewrite.ts`:
- Around line 43-63: Update the URL regex in the rewrite logic around the regex
construction to require a valid domain boundary after baseUrlNormalized,
preventing matches inside longer hostnames. In the callback’s OAuth2 and UI
mapping checks, replace raw path.startsWith checks with boundary-aware matching
so only the exact path or a path beginning with the configured segment followed
by a separator is rewritten.
---
Nitpick comments:
In `@examples/nextjs-app-router/app/api/consent/route.ts`:
- Around line 82-114: Validate body.action explicitly before the consent
operation: accept only “accept” or “reject”, and return a 400 invalid_request
response for missing or any other value. Update the branching around
acceptConsentRequest and rejectConsentRequest so rejectConsentRequest is called
only when action is exactly “reject”.
In `@examples/nextjs-app-router/middleware.ts`:
- Around line 34-51: Add an exported Next.js middleware config with a matcher
that excludes _next/static, _next/image, and favicon.ico from execution. Apply
the same configuration alongside middleware in
examples/nextjs-app-router/middleware.ts and
examples/nextjs-app-router-custom-components/middleware.ts; leave the middleware
request handling unchanged.
In `@packages/elements-react/src/theme/default/utils/oauth2-config.ts`:
- Line 4: Update the OryClientConfiguration import in oauth2-config.ts to use
the appropriate relative internal source path instead of the package public
entry point, preserving the existing type usage.
In `@packages/nextjs/src/app/consent.test.ts`:
- Around line 82-161: Extend both consent test describe blocks covering
acceptConsentRequest and rejectConsentRequest with identityId cases: stub
getOAuth2ConsentRequest with a request identity for each test, verify matching
identityId proceeds to the underlying accept/reject call, and verify mismatched
identityId throws the forbidden error without proceeding. Keep assertions
focused on the security guard and ensure the mocks support the lookup path.
In `@packages/nextjs/src/pages/client.ts`:
- Around line 27-29: Remove the explicit knownProxiedUrl value from the
guessPotentiallyProxiedOrySdkUrl call in the client factory, allowing the helper
to apply its safe client-side default without unconditionally accessing window
during SSR.
In `@packages/nextjs/src/pages/consent.ts`:
- Around line 76-79: Update the OAuth2 consent request flow in
serverSideOAuth2Client().getOAuth2ConsentRequest to log the caught error before
returning null. Preserve the existing graceful null fallback while ensuring
failures, including configuration and network errors, include useful error
details in the established logging mechanism.
In `@packages/nextjs/src/pages/session.ts`:
- Around line 47-55: Refactor the promise chain in the session-loading flow to
move setLoading(false) from both the success and error handlers into a shared
finally block. Keep setSession in the success handler and setError in the catch
handler, preserving their existing behavior.
In `@packages/nextjs/src/utils/rewrite.ts`:
- Around line 82-85: Update the null/undefined guard in the object-rewrite logic
to detect top-level arrays and return them unchanged before the
Object.fromEntries(Object.entries(...)) conversion. Preserve the existing
handling for non-array objects and nullish inputs.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 225bf684-e938-4b95-b2fa-4640b5eced41
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (42)
.changeset/oauth2-consent-flow.md.gitignoreexamples/nextjs-app-router-custom-components/.envexamples/nextjs-app-router-custom-components/app/api/consent/route.tsexamples/nextjs-app-router-custom-components/app/auth/consent/consent-form.tsxexamples/nextjs-app-router-custom-components/app/auth/consent/page.tsxexamples/nextjs-app-router-custom-components/components/consent-utils.tsexamples/nextjs-app-router-custom-components/components/custom-consent-scope-checkbox.tsxexamples/nextjs-app-router-custom-components/components/custom-footer.tsxexamples/nextjs-app-router-custom-components/components/index.tsxexamples/nextjs-app-router-custom-components/middleware.tsexamples/nextjs-app-router-custom-components/package.jsonexamples/nextjs-app-router/.envexamples/nextjs-app-router/app/api/consent/route.tsexamples/nextjs-app-router/app/auth/consent/consent-form.tsxexamples/nextjs-app-router/app/auth/consent/page.tsxexamples/nextjs-app-router/middleware.tsexamples/nextjs-app-router/package.jsonexamples/nextjs-pages-router/.envexamples/nextjs-pages-router/middleware.tsexamples/nextjs-pages-router/package.jsonexamples/nextjs-pages-router/pages/api/consent.tsexamples/nextjs-pages-router/pages/auth/consent.tsxpackages/elements-react/src/components/card/card-consent.test.tspackages/elements-react/src/components/card/card-consent.tsxpackages/elements-react/src/components/form/nodes/input.tsxpackages/elements-react/src/components/form/nodes/node-button.tsxpackages/elements-react/src/theme/default/flows/consent.tsxpackages/elements-react/src/theme/default/utils/oauth2-config.tspackages/elements-react/src/theme/default/utils/oauth2.tspackages/nextjs/api-report/nextjs-client.api.jsonpackages/nextjs/api-report/nextjs.api.mdpackages/nextjs/src/app/client.tspackages/nextjs/src/app/consent.test.tspackages/nextjs/src/app/consent.tspackages/nextjs/src/app/index.tspackages/nextjs/src/pages/client.tspackages/nextjs/src/pages/consent.tspackages/nextjs/src/pages/index.tspackages/nextjs/src/pages/session.tspackages/nextjs/src/utils/rewrite.test.tspackages/nextjs/src/utils/rewrite.ts
💤 Files with no reviewable changes (1)
- packages/elements-react/src/components/form/nodes/node-button.tsx
Make identityId mandatory in acceptConsentRequest and rejectConsentRequest so the subject check cannot be skipped by omission, via a shared assertConsentSubject helper. Harden rewriteUrls against prefix collisions: a lookahead enforces a domain boundary so hostnames merely sharing the base URL prefix are left untouched, and UI path overrides only match whole path segments so "/login" no longer captures "/login-methods".
Return result.response from csrfProtect instead of the pre-protection response. The current adapter mutates the passed response in place, but the processed response is the documented contract and stays correct if that implementation detail changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/nextjs/src/utils/rewrite.ts`:
- Around line 56-60: In the uiPathMappings loop, replace the
String.prototype.replace call with string concatenation using path.slice so the
configUrl value is never interpreted for replacement patterns. Preserve the
existing matched uiPath prefix and append the unchanged suffix after uiPath.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fcd8702f-078b-4c16-a113-6bb5e54a802e
📒 Files selected for processing (9)
examples/nextjs-app-router-custom-components/middleware.tsexamples/nextjs-app-router/middleware.tsexamples/nextjs-pages-router/middleware.tspackages/nextjs/api-report/nextjs-client.api.jsonpackages/nextjs/api-report/nextjs.api.mdpackages/nextjs/src/app/consent.test.tspackages/nextjs/src/app/consent.tspackages/nextjs/src/utils/rewrite.test.tspackages/nextjs/src/utils/rewrite.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- examples/nextjs-pages-router/middleware.ts
- packages/nextjs/src/utils/rewrite.test.ts
- packages/nextjs/api-report/nextjs.api.md
- examples/nextjs-app-router/middleware.ts
- examples/nextjs-app-router-custom-components/middleware.ts
- packages/nextjs/src/app/consent.test.ts
| for (const [uiPath, configUrl] of Object.entries(uiPathMappings)) { | ||
| if (path && configUrl && pathStartsWithSegment(path, uiPath)) { | ||
| return path.replace(uiPath, new URL(configUrl, selfUrl).toString()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid pattern interpolation in String.prototype.replace.
When using String.prototype.replace(searchValue, replaceValue), the replaceValue string is parsed for special replacement patterns like $&, $', or $n. Since configUrl is populated from the project configuration, if it contains any of these sequences (for example, within a ?redirect=$& query parameter), they will be unintentionally evaluated and replaced with the matched string.
Use string concatenation with path.slice instead, which is safer and avoids pattern interpolation entirely.
🛠️ Proposed fix
for (const [uiPath, configUrl] of Object.entries(uiPathMappings)) {
if (path && configUrl && pathStartsWithSegment(path, uiPath)) {
- return path.replace(uiPath, new URL(configUrl, selfUrl).toString())
+ return new URL(configUrl, selfUrl).toString() + path.slice(uiPath.length)
}
}📝 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.
| for (const [uiPath, configUrl] of Object.entries(uiPathMappings)) { | |
| if (path && configUrl && pathStartsWithSegment(path, uiPath)) { | |
| return path.replace(uiPath, new URL(configUrl, selfUrl).toString()) | |
| } | |
| } | |
| for (const [uiPath, configUrl] of Object.entries(uiPathMappings)) { | |
| if (path && configUrl && pathStartsWithSegment(path, uiPath)) { | |
| return new URL(configUrl, selfUrl).toString() + path.slice(uiPath.length) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/nextjs/src/utils/rewrite.ts` around lines 56 - 60, In the
uiPathMappings loop, replace the String.prototype.replace call with string
concatenation using path.slice so the configUrl value is never interpreted for
replacement patterns. Preserve the existing matched uiPath prefix and append the
unchanged suffix after uiPath.
5 participants
Add complete OAuth2 consent flow support to
@ory/nextjsand@ory/elements-reactpackages, enabling applications to handle OAuth2 authorization consent screens with Ory Hydra.Related Issue or Design Document
Fixes #327
Features
Consent Flow Utilities (
@ory/nextjs)getConsentFlow,acceptConsentRequest,rejectConsentRequestgetServerSideConsentFlow(forgetServerSideProps) anduseSessionconsent_challengeOAuth2 Client Info Display (
@ory/elements-react)grant_scopecheckboxes and shows the OAuth2 client nameExample Implementations
@csrf-armor/nextjsmiddleware, plus an explicit double-submit comparison in the consent API routes (the library falls back to the csrf cookie when extracting the submitted token, so the cookie pair alone is not sufficient)Improvements
rewriteUrlsto single-pass regex replacementrewriteJsonResponseTests
Checklist
If this pull request addresses a security vulnerability,
I confirm that I got approval (please contact security@ory.sh) from the maintainers to push the changes.
Further comments
This implementation follows the pattern established in kratos-selfservice-ui-node for handling OAuth2 flows. The OAuth2 client logo is displayed by overriding the project configuration's
logo_light_urlwhen an OAuth2 client logo is available, keeping the existingDefaultCardLogocomponent unchanged; the override is applied by the consent flow component only.Summary by CodeRabbit