Preserve mobile deep-link destination through sign-in (#793)#856
Preserve mobile deep-link destination through sign-in (#793)#856dennisonbertram wants to merge 6 commits into
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…793) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…#793) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66bf1c5970
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| size="lg" | ||
| callbackUrl="/get-started?next=/sessions" | ||
| /> | ||
| <SignInButton size="lg" callbackUrl={signInCallbackUrl} /> |
There was a problem hiding this comment.
Preserve next for all landing sign-in CTAs
When /?next=/m/chat/... is open and the user clicks the sticky nav CTA after scrolling (or the lower bento CTA), those buttons still render without a callbackUrl (LandingNav/LandingBento). SignInButton then falls back to window.location.pathname + window.location.search and sends OAuth back to /?next=...; app/page.tsx redirects signed-in root requests to /sessions, dropping the mobile target. Please pass the resolved callback URL through those landing CTAs too, otherwise the protected mobile deep-link path is preserved only for the hero button.
Useful? React with 👍 / 👎.
Why
Implements #793. Protected path: "A signed-out user who opens a mobile deep link, signs in, and is returned to that same mobile destination" —
/m/*routes guarded byMobileLayout.Before this fix,
apps/web/app/(mobile)/m/layout.tsxredirected every unauthenticated/m/*visit to a bareredirect("/"), dropping the requested path entirely. A signed-out visitor who opened a shared mobile deep link (e.g./m/chat/abc123) landed on the desktopSignedOutHero, and even after signing in was dropped on/sessions— a different app shell than the one they clicked through to open. This is the most common mobile entry point (a shared link), so it made the mobile experience feel broken on first contact.Follow-up from epic #779.
What changed
apps/web/proxy.ts(Next.js 16's request proxy, formerlymiddleware.ts) now stamps the incoming pathname+search onto anx-invoke-pathrequest header for every/m/:path*request, extending its existing matcher (previously scoped only to/shared/*content-negotiation). Server Components have no other way to read the current request path —usePathname()is Client-Component-only.apps/web/app/(mobile)/m/layout.tsx: the unauthenticated redirect now readsx-invoke-pathviaheaders()and redirects to/?next=<encoded original path>instead of a bare/. Falls back to bare/if the header is somehow absent (defense-in-depth; should not happen given the proxy matcher).apps/web/components/auth/signed-out-hero.tsx: reads thenextsearch param (client-side, same pattern already used for theerrorparam) and resolves it through the existingsanitizeInternalRedirectguard (apps/web/lib/redirect-safety.ts) before passing it as the heroSignInButton'scallbackUrl. No new open-redirect guard was written — this reuses the guard already used by/get-startedandrequire-onboarded.ts. Falls back to the unchanged default/get-started?next=/sessionswhennextis absent or fails the safety check (absolute URL, protocol-relative//, etc).New/updated tests:
apps/web/app/(mobile)/m/layout.test.ts(new) — proves the redirect target carries the encoded original path (and its query string), falls back to bare/if the path header is missing, and does not redirect at all when signed in.apps/web/proxy.test.ts(extended) — proves/m,/m/new,/m/chat/some-id(with and without query string) getx-invoke-pathstamped, and unrelated routes (/sessions) do not.apps/web/components/auth/signed-out-hero.test.tsx(extended) — proves the hero CTA'scallbackUrlprop equals a validnextparam, falls back to the default when absent, and falls back to the default (not the raw value) for an absolute-URL or protocol-relativenext.Out of scope
redirect("/")shape onapps/web/app/repos/layout.tsxandapps/web/app/sessions/layout.tsx— mobile-only per this ticket's scope. A future contributor should not assume this PR covers those; file a separate follow-up for parity there if wanted.nexthandling (api/github/app/install,api/github/post-link) — unrelated, already works.How it was verified
Red state (observed failing before implementation):
Green state + full gate, all exit 0:
Browser smoke (
PORT=3043 bun run web,agent-browser --session w793):curl http://localhost:3043/m/chat/abc123(signed out) →307to/?next=%2Fm%2Fchat%2Fabc123(also verified for/mand/m/new)./?next=%2Fm%2Fchat%2Fabc123, clicked the hero "Sign in with Vercel" CTA, captured the actualPOST /api/auth/sign-in/socialrequest body:{"provider":"vercel","callbackURL":"/m/chat/abc123",...}— the live network call, not just the unit test, carries the mobile destination through./with nonextparam, clicked sign-in:callbackURLwas unchanged at"/get-started?next=/sessions"(default flow not regressed)./?next=https%3A%2F%2Fevil.example.com%2Fphish, clicked sign-in:callbackURLfell back to"/get-started?next=/sessions", proving the open-redirect guard rejects the unsafe value rather than forwarding it.agent-browser errorsoutput during the walk.Evidence quality
Deterministic Bun test coverage (layout, proxy, and landing-page CTA) plus a local dev-server browser smoke with real network-request inspection (see screenshot in the epic evidence bundle — headless
ghcannot upload images here). No preview/prod deploy involved; this is dev-only proof for a pure redirect-plumbing change.Risk and rollback
No schema changes, no new env vars, no migration. Pure redirect-target and CTA-prop plumbing.
proxy.ts's matcher extension is additive (/m,/m/:path*added alongside the existing/shared/:path*); the pre-existing/shared/*markdown-negotiation behavior is covered by its original passing tests, unchanged. Rollback is a straight revert of this PR's commits — no data migration or follow-up cleanup needed.Reviewer guide
apps/web/app/(mobile)/m/layout.tsx(the core fix) and its test, thenapps/web/proxy.ts(the mechanism that gets the path into a Server Component) and its test.apps/web/components/auth/signed-out-hero.tsx's diff is small (one new helper function + one prop wire-up); the interesting part is that it reusessanitizeInternalRedirectrather than inventing new sanitization.apps/web/components/auth/signed-out-hero.test.tsxdiff includes an unrelated-lookingresetHeroSignInButtonCapture()helper — that's working around a real (reproduced, minimal-repro'd) TypeScript control-flow-narrowing quirk withbun-types'expect().toBe()generic default, not a behavior change; see the code comment at its definition.sanitizeInternalRedirectreuse and the absolute/protocol-relative-URL test cases extra attention — that's the actual security-relevant surface of this change.Follow-up from epic #779.