From 75c0c6677829b5aed968ba0fb3a653d940cb2211 Mon Sep 17 00:00:00 2001 From: Chris Alfano Date: Thu, 17 Sep 2026 17:39:21 -0400 Subject: [PATCH 1/5] docs(specs): make the SAML sign-in return a site-relative path The spec said "encoded current URL" for /launch and a stale `?resume=1` shape for /sso. Every consumer of `?return=` accepts only a path with one leading slash (the laddr/Emergence convention), so pin the contract to that and note the login page must fully navigate when the return is an API route rather than an SPA route. Co-Authored-By: Claude Fable 5.1 --- specs/api/saml.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/specs/api/saml.md b/specs/api/saml.md index 05d53e1..7b0114c 100644 --- a/specs/api/saml.md +++ b/specs/api/saml.md @@ -114,7 +114,7 @@ Rules: ### Behavior -1. Require a signed-in session. If not signed in → redirect to `/login?return=`. +1. Require a signed-in session. If not signed in → redirect to `/login?return=`. The return is a path, never an absolute URL: `/login` and the OAuth start both drop anything that does not begin with a single `/`. 2. Validate the member is permitted (default: any `user` accountLevel; configurable via the same `userIsPermitted` hook the legacy code provided). 3. Build a SAML Response containing the NameID + attributes for the current Person. 4. Sign the Response with the IdP private key. @@ -159,7 +159,7 @@ Query string (saml-bindings §3.4.4.1, `DEFLATE` encoding): ### Behavior 1. Decode + parse the AuthnRequest (inflate first for the Redirect binding). Validate signature if Slack signs requests (configurable; usually no for Slack). -2. Require a signed-in session. If not → store the AuthnRequest in a short-lived signed cookie, redirect to `/login?return=/api/saml/slack/sso?resume=1`. After login the user comes back here and the AuthnRequest replays from the cookie. +2. Require a signed-in session. If not → store the AuthnRequest in a short-lived signed cookie, redirect to `/login?return=/api/saml/slack/sso/resume` (site-relative, as for `/launch`). After login the user comes back to `/sso/resume` and the AuthnRequest replays from the cookie. Because that return is an API route rather than an SPA route, the login page performs a full navigation to it instead of client-side routing. 3. Resolve the AuthnRequest's `AssertionConsumerServiceURL` against Slack's documented ACS endpoint(s) — only Slack's ACS is accepted. 4. Build + sign a SAML Response as in `/launch`. 5. POST back to Slack's ACS via the auto-submitting form, including `RelayState`. The Response always goes back over HTTP-POST regardless of which binding carried the request — Slack's ACS only accepts POST. From 5d2a792b7da1090fd763abe178d7cb575f09cdb2 Mon Sep 17 00:00:00 2001 From: Chris Alfano Date: Thu, 17 Sep 2026 17:39:21 -0400 Subject: [PATCH 2/5] fix(api): send site-relative returns from the SAML entry points Both anonymous SAML paths handed /login an absolute URL. The login page and the OAuth start drop anything that doesn't begin with a single `/`, so Slack's first live "Test configuration" signed the user in and then landed them on `/` instead of resuming the AuthnRequest. Use the resume path itself for SP-initiated SSO and safeReturnPath of the request URL for /launch so channel/redir survive. The absolute-URL helpers had no other callers. Tests previously matched only the `/login?return=` prefix and passed with the bad value; they now decode the return and assert the exact path. Co-Authored-By: Claude Fable 5.1 --- apps/api/src/routes/saml.ts | 37 +++++++++++-------------------------- apps/api/tests/saml.test.ts | 29 +++++++++++++++++++++++++---- 2 files changed, 36 insertions(+), 30 deletions(-) diff --git a/apps/api/src/routes/saml.ts b/apps/api/src/routes/saml.ts index 8331cb1..0f6ebf2 100644 --- a/apps/api/src/routes/saml.ts +++ b/apps/api/src/routes/saml.ts @@ -32,6 +32,8 @@ import type { Person, PrivateProfile } from '@cfp/shared/schemas'; const CHAT_CHANNEL_REGEX = /^[a-z0-9][a-z0-9_-]{0,40}$/; const RESUME_COOKIE = 'cfp_saml_resume'; const RESUME_COOKIE_TTL_SECONDS = 10 * 60; +/** Site-relative — it is also the `?return=` handed to /login (see safeReturnPath). */ +const RESUME_PATH = '/api/saml/slack/sso/resume'; /** * SAML ID values must start with an XML NCName character — `_` followed by @@ -55,26 +57,6 @@ function safeReturnPath(input: string | undefined | null): string { return input; } -function selfUrl(request: FastifyRequest): string { - const protocol = request.headers['x-forwarded-proto'] - ? String(request.headers['x-forwarded-proto']).split(',')[0]?.trim() ?? 'http' - : request.protocol; - const host = request.headers['x-forwarded-host'] - ? String(request.headers['x-forwarded-host']).split(',')[0]?.trim() ?? request.hostname - : request.hostname; - return `${protocol}://${host}${request.url}`; -} - -function originBase(request: FastifyRequest): string { - const protocol = request.headers['x-forwarded-proto'] - ? String(request.headers['x-forwarded-proto']).split(',')[0]?.trim() ?? 'http' - : request.protocol; - const host = request.headers['x-forwarded-host'] - ? String(request.headers['x-forwarded-host']).split(',')[0]?.trim() ?? request.hostname - : request.hostname; - return `${protocol}://${host}`; -} - function htmlEscape(value: string): string { return value .replace(/&/g, '&') @@ -361,8 +343,10 @@ async function handleSpInitiatedSso( path: '/api/saml', maxAge: RESUME_COOKIE_TTL_SECONDS, }); - const resumeReturn = `${originBase(request)}/api/saml/slack/sso/resume`; - return reply.redirect(`/login?return=${encodeURIComponent(resumeReturn)}`); + // Site-relative on purpose: `/login` and the OAuth start both run the + // return through the same leading-slash guard as `safeReturnPath`, so an + // absolute URL here is silently dropped and the user lands on `/`. + return reply.redirect(`/login?return=${encodeURIComponent(RESUME_PATH)}`); } // Signed in — build the assertion immediately. @@ -462,10 +446,11 @@ export async function samlRoutes(fastify: FastifyInstance): Promise { async (request, reply) => { const cfg = fastify.config; - // Anonymous → bounce through /login preserving the current URL. + // Anonymous → bounce through /login preserving the current path + query + // (`channel` / `redir` ride along). Site-relative: see the SP-initiated + // handler for why an absolute URL would be dropped. if (!request.session.personId) { - const here = selfUrl(request); - return reply.redirect(`/login?return=${encodeURIComponent(here)}`); + return reply.redirect(`/login?return=${encodeURIComponent(safeReturnPath(request.url))}`); } const query = request.query as { channel?: string; redir?: string }; @@ -589,7 +574,7 @@ export async function samlRoutes(fastify: FastifyInstance): Promise { // ------------------------------------------------------------------------- fastify.get( - '/api/saml/slack/sso/resume', + RESUME_PATH, { schema: { tags: ['saml'], diff --git a/apps/api/tests/saml.test.ts b/apps/api/tests/saml.test.ts index 1fdc524..bdfc014 100644 --- a/apps/api/tests/saml.test.ts +++ b/apps/api/tests/saml.test.ts @@ -73,6 +73,16 @@ function slackAuthnRequestXml(id: string, acsHost: string = SLACK_TEAM_HOST): st https://slack.com`; } +/** + * The decoded `?return=` a /login redirect carries. Must be site-relative: + * both /login and the OAuth start drop anything not beginning with one `/`. + */ +function loginReturnOf(location: string | string[] | undefined): string { + const url = new URL(String(location), 'http://placeholder.test'); + expect(url.pathname).toBe('/login'); + return url.searchParams.get('return') ?? ''; +} + /** * HTTP-Redirect binding encoding (saml-bindings §3.4.4.1): raw DEFLATE → * base64 → URL-encode. Returns the ready-to-append query string. @@ -232,10 +242,21 @@ describe('SAML IdP — Slack', () => { expect(formats).toContain('urn:oasis:names:tc:SAML:2.0:nameid-format:persistent'); }); - it('GET /api/saml/slack/launch (anonymous) redirects to /login', async () => { + it('GET /api/saml/slack/launch (anonymous) redirects to /login with a site-relative return', async () => { const res = await app.inject({ method: 'GET', url: '/api/saml/slack/launch' }); expect(res.statusCode).toBe(302); - expect(res.headers.location).toMatch(/^\/login\?return=/); + // An absolute URL here is silently dropped by /login's leading-slash guard + // and the user lands on `/` instead of coming back to finish SSO. + expect(loginReturnOf(res.headers.location)).toBe('/api/saml/slack/launch'); + }); + + it('GET /api/saml/slack/launch (anonymous) keeps channel/redir in the return', async () => { + const res = await app.inject({ + method: 'GET', + url: '/api/saml/slack/launch?channel=general', + }); + expect(res.statusCode).toBe(302); + expect(loginReturnOf(res.headers.location)).toBe('/api/saml/slack/launch?channel=general'); }); it('GET /api/saml/slack/launch (signed-in) returns auto-submit form with signed SAML response', async () => { @@ -400,7 +421,7 @@ describe('SAML IdP — Slack', () => { url: `/api/saml/slack/sso?${redirectBindingQuery(slackAuthnRequestXml('id-redirect-1'), 'opaque-redirect-state')}`, }); expect(res.statusCode).toBe(302); - expect(res.headers.location).toMatch(/^\/login\?return=/); + expect(loginReturnOf(res.headers.location)).toBe('/api/saml/slack/sso/resume'); expect(resumeCookieValue(res)).not.toBe(''); }); @@ -513,7 +534,7 @@ describe('SAML IdP — Slack', () => { headers: { 'content-type': 'application/x-www-form-urlencoded' }, }); expect(res.statusCode).toBe(302); - expect(res.headers.location).toMatch(/^\/login\?return=/); + expect(loginReturnOf(res.headers.location)).toBe('/api/saml/slack/sso/resume'); // Resume cookie set const cookies = res.headers['set-cookie']; const cookieStr = Array.isArray(cookies) ? cookies.join('\n') : String(cookies ?? ''); From 5ea5a04426c7770bd2d2bb0c5213995e0459bfe6 Mon Sep 17 00:00:00 2001 From: Chris Alfano Date: Thu, 17 Sep 2026 17:39:21 -0400 Subject: [PATCH 3/5] fix(web): hard-navigate sign-in returns that target API routes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The SAML resume endpoint lives under /api, which Fastify serves — the SPA's router has no such route and rendered NotFound when the login and account-claim pages navigate()d there after sign-in. Only GitHub OAuth worked, because the API redirects server-side. One shared helper replaces three copy-pasted safeReturn functions and the login page's weaker inline guard (which lacked the `//` check), and uses window.location.assign for /api/ targets. Co-Authored-By: Claude Fable 5.1 --- apps/web/src/lib/return-path.ts | 26 +++++++++++++++++++ apps/web/src/pages/AccountClaim.tsx | 11 +++----- apps/web/src/pages/AccountClaimByPassword.tsx | 9 ++----- .../pages/AccountClaimRequestStaffReview.tsx | 9 ++----- apps/web/src/pages/LoginPlaceholder.tsx | 14 +++++----- 5 files changed, 39 insertions(+), 30 deletions(-) create mode 100644 apps/web/src/lib/return-path.ts diff --git a/apps/web/src/lib/return-path.ts b/apps/web/src/lib/return-path.ts new file mode 100644 index 0000000..83ea99e --- /dev/null +++ b/apps/web/src/lib/return-path.ts @@ -0,0 +1,26 @@ +import type { NavigateFunction } from 'react-router'; + +/** + * Where to land after sign-in / account claim. Mirrors the API's + * `safeReturnPath`: site-relative only (one leading slash), so a crafted + * `?return=` can't bounce the browser off-site. Anything else → `/`. + */ +export function safeReturn(input: string | null): string { + if (!input) return '/'; + if (!input.startsWith('/') || input.startsWith('//')) return '/'; + return input; +} + +/** + * `/api/**` is served by Fastify, not the SPA — the SAML resume endpoint hands + * the browser on to Slack from there. Client-side routing to one of those + * paths never reaches the server and renders the SPA's `*` NotFound instead, + * so they need a real navigation. Everything else stays in-app. + */ +export function goToReturn(navigate: NavigateFunction, path: string): void { + if (path.startsWith('/api/')) { + window.location.assign(path); + return; + } + void navigate(path, { replace: true }); +} diff --git a/apps/web/src/pages/AccountClaim.tsx b/apps/web/src/pages/AccountClaim.tsx index a939b04..217130d 100644 --- a/apps/web/src/pages/AccountClaim.tsx +++ b/apps/web/src/pages/AccountClaim.tsx @@ -11,6 +11,7 @@ import { import { Button } from '@/components/ui/button'; import { Separator } from '@/components/ui/separator'; import { useAuth } from '@/hooks/useAuth'; +import { goToReturn, safeReturn } from '@/lib/return-path'; import { api, ApiError, @@ -19,12 +20,6 @@ import { } from '@/lib/api'; import { formatAbsoluteDate, formatRelativeTime } from '@/lib/time'; -function safeReturn(input: string | null): string { - if (!input) return '/'; - if (!input.startsWith('/') || input.startsWith('//')) return '/'; - return input; -} - export function AccountClaim() { const [searchParams] = useSearchParams(); const navigate = useNavigate(); @@ -82,7 +77,7 @@ export function AccountClaim() { await api.accountClaim.confirm(candidate.personId); toast.success(`Welcome back, ${candidate.fullName}`); await reload(); - void navigate(returnPath, { replace: true }); + goToReturn(navigate, returnPath); } catch (err) { const msg = err instanceof ApiError ? err.message : 'Failed to confirm'; toast.error(msg); @@ -96,7 +91,7 @@ export function AccountClaim() { await api.accountClaim.decline(); toast.success("Got it — we'll set up a fresh profile"); await reload(); - void navigate(returnPath, { replace: true }); + goToReturn(navigate, returnPath); } catch (err) { toast.error(err instanceof ApiError ? err.message : 'Failed to start fresh'); setDeclining(false); diff --git a/apps/web/src/pages/AccountClaimByPassword.tsx b/apps/web/src/pages/AccountClaimByPassword.tsx index 36dd04c..f48f319 100644 --- a/apps/web/src/pages/AccountClaimByPassword.tsx +++ b/apps/web/src/pages/AccountClaimByPassword.tsx @@ -13,12 +13,7 @@ import { Input } from '@/components/ui/input'; import { Label } from '@/components/ui/label'; import { api, ApiError } from '@/lib/api'; import { useAuth } from '@/hooks/useAuth'; - -function safeReturn(input: string | null): string { - if (!input) return '/'; - if (!input.startsWith('/') || input.startsWith('//')) return '/'; - return input; -} +import { goToReturn, safeReturn } from '@/lib/return-path'; export function AccountClaimByPassword() { const [searchParams] = useSearchParams(); @@ -39,7 +34,7 @@ export function AccountClaimByPassword() { await api.accountClaim.byPassword(slug.trim(), password); toast.success('Welcome back'); await reload(); - void navigate(returnPath, { replace: true }); + goToReturn(navigate, returnPath); } catch (err) { if (err instanceof ApiError) { if (err.status === 401 && err.code === 'claim_token_invalid') { diff --git a/apps/web/src/pages/AccountClaimRequestStaffReview.tsx b/apps/web/src/pages/AccountClaimRequestStaffReview.tsx index 23321d4..04c4195 100644 --- a/apps/web/src/pages/AccountClaimRequestStaffReview.tsx +++ b/apps/web/src/pages/AccountClaimRequestStaffReview.tsx @@ -14,12 +14,7 @@ import { Label } from '@/components/ui/label'; import { Textarea } from '@/components/ui/textarea'; import { api, ApiError } from '@/lib/api'; import { useAuth } from '@/hooks/useAuth'; - -function safeReturn(input: string | null): string { - if (!input) return '/'; - if (!input.startsWith('/') || input.startsWith('//')) return '/'; - return input; -} +import { goToReturn, safeReturn } from '@/lib/return-path'; export function AccountClaimRequestStaffReview() { const [searchParams] = useSearchParams(); @@ -55,7 +50,7 @@ export function AccountClaimRequestStaffReview() { try { await api.accountClaim.decline(); await reload(); - void navigate(returnPath, { replace: true }); + goToReturn(navigate, returnPath); } catch (err) { toast.error(err instanceof ApiError ? err.message : 'Failed to continue'); setContinuing(false); diff --git a/apps/web/src/pages/LoginPlaceholder.tsx b/apps/web/src/pages/LoginPlaceholder.tsx index 621c16d..7ae1b67 100644 --- a/apps/web/src/pages/LoginPlaceholder.tsx +++ b/apps/web/src/pages/LoginPlaceholder.tsx @@ -13,6 +13,7 @@ import { Label } from '@/components/ui/label'; import { GitHubIcon } from '@/components/icons/GitHubIcon'; import { useAuth } from '@/hooks/useAuth'; import { api, ApiError } from '@/lib/api'; +import { goToReturn, safeReturn } from '@/lib/return-path'; type ErrorCode = | 'access_denied' @@ -93,15 +94,14 @@ export function LoginPlaceholder() { const navigate = useNavigate(); const [searchParams] = useSearchParams(); - const returnPath = searchParams.get('return'); + const rawReturn = searchParams.get('return'); + const returnPath = safeReturn(rawReturn); const errorCode = searchParams.get('error') as ErrorCode | null; // Redirect already-authenticated users useEffect(() => { if (!loading && person) { - const target = - returnPath && returnPath.startsWith('/') ? returnPath : '/'; - void navigate(target, { replace: true }); + goToReturn(navigate, returnPath); } }, [loading, person, navigate, returnPath]); @@ -117,7 +117,7 @@ export function LoginPlaceholder() { ); } - const startUrl = returnPath + const startUrl = rawReturn ? `/api/auth/github/start?return=${encodeURIComponent(returnPath)}` : '/api/auth/github/start'; @@ -126,9 +126,7 @@ export function LoginPlaceholder() { // picks up the new session before we navigate. Without this, the // navbar stays in its anonymous state until the next hard refresh. await reload(); - const target = - returnPath && returnPath.startsWith('/') ? returnPath : '/'; - void navigate(target, { replace: true }); + goToReturn(navigate, returnPath); }; return ( From 57188a40cfea93f322718318edb0ebc31de11f2f Mon Sep 17 00:00:00 2001 From: Chris Alfano Date: Thu, 17 Sep 2026 17:39:21 -0400 Subject: [PATCH 4/5] chore(plans): add saml-login-return-path Co-Authored-By: Claude Fable 5.1 --- plans/saml-login-return-path.md | 63 +++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 plans/saml-login-return-path.md diff --git a/plans/saml-login-return-path.md b/plans/saml-login-return-path.md new file mode 100644 index 0000000..421c27e --- /dev/null +++ b/plans/saml-login-return-path.md @@ -0,0 +1,63 @@ +--- +status: in-progress +depends: [saml-authn-statement] +specs: + - specs/api/saml.md +issues: [] +pr: null +--- + +# Plan: SAML sign-in return path + +## Scope + +Slack's "Test configuration" against production got as far as our `/login`, +then dropped the user on `/` instead of finishing the SSO round-trip. Two +defects, found by tracing the first real end-to-end attempt: + +1. Both anonymous SAML entry points hand `/login?return=` an **absolute** URL + (`https:///api/saml/slack/sso/resume`, and `selfUrl()` for + `/launch`). Every consumer of `?return=` — the login page, the OAuth start, + the account-claim pages — accepts only a site-relative path (one leading + `/`, per the laddr/Emergence convention) and falls back to `/` otherwise. +2. Even with a correct path, the login and account-claim pages hand the + return to React Router. `/api/**` is served by Fastify, not the SPA, so a + client-side `navigate()` there renders the `*` NotFound page and never + reaches the resume endpoint. Only the GitHub OAuth path escaped, because + the API redirects server-side. + +In: spec the return contract, fix both entry points, one shared web helper +that hard-navigates for API routes, tests that pin the exact return. Out: +a general "remember where you were" for arbitrary protected pages (already +handled per-page via `?return=`); any change to the assertion itself. + +## Implements + +- [api/saml.md](../specs/api/saml.md) — `## GET /api/saml/slack/launch` step 1 + and `## GET | POST /api/saml/slack/sso` step 2: the return is a + site-relative path (never absolute), and the login page performs a full + navigation when it is an API route. + +## Approach + +- `apps/api/src/routes/saml.ts`: `RESUME_PATH` constant used by both the park + redirect and the route registration; `/launch` returns + `safeReturnPath(request.url)` so `channel`/`redir` ride along. Drop the two + absolute-URL helpers that no longer have callers. +- `apps/web/src/lib/return-path.ts`: `safeReturn` (was copy-pasted into three + account-claim pages; the login page had a weaker inline guard without the + `//` check) and `goToReturn`, which uses `window.location.assign` for + `/api/` targets and `navigate()` otherwise. All four pages use it. +- Tests: the existing anonymous-redirect assertions only matched + `^/login?return=` and passed with the absolute URL. Decode the `return` and + assert the exact path, plus a `/launch?channel=` case. + +## Validation + +- `npm run type-check && npm run lint && npm test` clean. +- Slack "Test configuration" against the deployed build completes the + round-trip after a fresh sign-in (the live check this plan exists for). + +## Follow-ups + +None. From 7071e52e0b942ce6a7f57a0c39aab3405bb5c358 Mon Sep 17 00:00:00 2001 From: Chris Alfano Date: Thu, 17 Sep 2026 17:39:59 -0400 Subject: [PATCH 5/5] chore(plans): mark saml-login-return-path done (PR #175) Co-Authored-By: Claude Fable 5.1 --- plans/saml-login-return-path.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plans/saml-login-return-path.md b/plans/saml-login-return-path.md index 421c27e..ca0e470 100644 --- a/plans/saml-login-return-path.md +++ b/plans/saml-login-return-path.md @@ -1,10 +1,10 @@ --- -status: in-progress +status: done depends: [saml-authn-statement] specs: - specs/api/saml.md issues: [] -pr: null +pr: 175 --- # Plan: SAML sign-in return path