fix(auth): correct callback URL resolution across SSR and hydration - #6217
Conversation
`callbackUrl` was seeded into `useState('/workspace')` and overwritten from
a `useEffect` that read `searchParams`, so the first painted frame always
carried the default. On any deep link with `?callbackUrl=`, the "Sign in
with email" and "Sign up" links briefly pointed at `/workspace` instead of
the requested destination, and a click landing in that window navigated to
the wrong place.
- derive `callbackUrl` from `searchParams` during render; the validation
gate is unchanged, so an off-origin or malformed value still falls back
to `/workspace`
- keep the warning for a rejected value in an effect, now keyed on the
param itself rather than the `searchParams` object, so it fires once per
actual change instead of once per identity change
- add first-frame tests via `renderToString`, which runs no effects and so
pins exactly the window the old code got wrong
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview SSO form:
Adds Reviewed by Cursor Bugbot for commit 24f0940. Configure here. |
Greptile SummaryThe PR fixes callback URL resolution so SSR and hydration use consistent validation and SSO links carry the intended destination on their first render.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the prior SSR/client-origin divergence is addressed by resolving server validation against the configured application origin while browser validation continues to use the browser origin.
|
| Filename | Overview |
|---|---|
| apps/sim/ee/sso/components/sso-form.tsx | Derives and validates the callback parameter during render, removing the incorrect first-frame default while retaining invalid-value logging. |
| apps/sim/lib/core/security/input-validation.ts | Uses the configured application origin for server-side callback validation and safely falls back to the sentinel when configuration cannot be resolved. |
| apps/sim/lib/core/utils/urls.ts | Removes the unsafe browser-origin fallback and now throws consistently when the application URL is absent. |
| packages/testing/src/mocks/urls.mock.ts | Aligns the shared URL mock with the production helper’s stricter missing-configuration behavior. |
Sequence Diagram
sequenceDiagram
participant Server as SSR
participant Config as App URL configuration
participant Browser as Hydration
participant Form as Auth form
Server->>Config: Read NEXT_PUBLIC_APP_URL
Config-->>Server: Configured application origin
Server->>Form: Validate callback and render links
Browser->>Form: Validate against window.location.origin
Form-->>Browser: Preserve matching callback destination
Reviews (4): Last reviewed commit: "fix(env): drop the getBaseUrl browser-or..." | Re-trigger Greptile
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit fb97b27. Configure here.
`validateCallbackUrl` compared against a sentinel base (`https://callback-url-validator.invalid`) when `window` was undefined, so the server rejected every absolute URL — including the same-origin ones the function documents as valid. A component deriving a callback URL during render therefore produced one destination in the SSR markup and a different one after hydration. The exposure was not new to the SSO form: `login-form.tsx` and `signup-form.tsx` already derive their callback URL during render on `force-dynamic` pages, so both carried the same divergence. - resolve against the deployment's own origin server-side, so the server reaches the same verdict the browser will after hydration - fall back to the sentinel when the app URL is unset or unparseable, which keeps the server fail-closed: absolute URLs are rejected, as before - cover the absolute same-origin case and the unset-app-URL fallback in the existing suite; all 15 open-redirect rejection cases are unchanged
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 73587c7. Configure here.
The fallback was added in #6214 as a safety net while the real cause — the hosted env script losing its `beforeInteractive` strategy — was fixed in the same PR. With the injection ordering restored, `window.__ENV` is populated before hydration, so the fallback is unreachable in any correctly configured deployment. Guessing the origin was also unsafe in the one case it could still fire. An opaque origin — a sandboxed iframe, and `/chat/*` is deliberately embeddable — serializes to the string `'null'`, which is truthy, so `getBaseUrl()` would have returned `'null'` and every call site would have silently built `null/api/...`. A throw surfaces the misconfiguration instead of encoding it into request URLs. - restore the unconditional throw when NEXT_PUBLIC_APP_URL is unset or blank - mirror it back in the shared testing mock - flip the two fallback tests to assert the throw, keeping whitespace-only coverage Server-side behavior is unchanged: there was never a `window` to fall back to, so callers that already guard `getBaseUrl()` (`getBaseDomain`, `validateCallbackUrl`) keep their existing fail-closed paths.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 24f0940. Configure here.
Summary
Three follow-ups to #6214, all on the same URL-resolution surface.
1. Derive the SSO callback URL during render (
fb97b2761)callbackUrlwas seeded intouseState('/workspace')and then overwritten from auseEffect, so the first painted frame always carried the default. On any deep link with?callbackUrl=, the "Sign in with email" and "Sign up" links briefly pointed at/workspace— a click landing in that window navigated to the wrong place. Now derived during render, matchinglogin-form.tsx:104andsignup-form.tsx:118, which already did this;/ssowas the lone outlier. The rejected-value warning moves to an effect keyed on the param itself rather than thesearchParamsobject, so it fires once per actual change.2. Resolve callback URLs against the app origin server-side (
73587c7ea)Both reviewers flagged an SSR/client divergence.
validateCallbackUrlresolved against a sentinel base (https://callback-url-validator.invalid) whenwindowwas undefined, so the server rejected every absolute URL — including the same-origin ones its own docstring documents as valid. Server and client disagreed, and a callback URL derived during render produced one destination in the SSR markup and another after hydration.That exposure was not new to this PR —
login-form.tsxandsignup-form.tsxalready derive during render onforce-dynamicpages, so both carried it. Fixed at the root rather than worked around:This makes the server match the documented contract rather than loosening it. The base is operator-controlled env, never attacker-controlled, and it stays fail-closed — an unset or unparseable app URL falls back to the sentinel and rejects every absolute URL exactly as before.
3. Drop the
getBaseUrlbrowser-origin fallback (24f09409a)The
window.location.originfallback was added in #6214 as a safety net while the real cause — the hosted env script losingbeforeInteractive— was fixed in the same PR. With injection ordering restored,window.__ENVis populated before hydration, so the fallback is unreachable in any correctly configured deployment.It was also unsafe in the one case it could still fire: an opaque origin — a sandboxed iframe, and
/chat/*is deliberately embeddable — serializes to the string'null', which is truthy.getBaseUrl()would have returned'null'and every call site would have silently builtnull/api/.... A throw surfaces the misconfiguration instead of encoding it into request URLs.Server-side behavior is unchanged throughout: there was never a
windowto fall back to, sogetBaseDomain()andgetCallbackValidationOrigin()keep their existing fail-closed paths.Type of Change
Testing
sso-form.test.tsxrenders viarenderToString, which runs no effects and so captures exactly the first-frame window the old code got wrong: a validcallbackUrlpresent on the first frame, no param falling back to/workspace, and an off-origin value (https://evil.example.com/steal) rejected and absent from the markup. Confirmed the first-frame test fails against the pre-fix implementation.input-validation.test.tsgains the absolute same-origin acceptance and the unset-app-URL fail-closed fallback. All 15 existing open-redirect rejection cases (//evil.com, userinfo smuggling, subdomain confusion, wrong port, wrong protocol,javascript:,data:, tab/newline-stripped variants) pass unchanged. Confirmed the new acceptance test fails against the sentinel-only version.urls.test.tsflips the two fallback tests to assert the throw, keeping whitespace-only coverage.503 tests pass across the directly affected suites. Full
apps/simsuite: 18195 passing — the single failure isexecutor/handlers/pi/cloud-review-tools.test.ts, which shells out torgvia a pythonsubprocess; ripgrep is a shell function rather than a binary on my machine, so it is an environment artifact unrelated to these files.tscclean,bun run lint:checkclean. Structural gates all pass:check:boundaries,check:client-boundary,check:realtime-prune,check:tool-registry-boundary,check:react-query,check:utils,check:api-validation.Checklist