Skip to content

Commit ca6af00

Browse files
themightychristhemightychris
authored
Merge pull request #176 from CodeForPhilly/develop
Release: v0.2.2
2 parents 07bc8d0 + 628705a commit ca6af00

9 files changed

Lines changed: 140 additions & 62 deletions

File tree

‎apps/api/src/routes/saml.ts‎

Lines changed: 11 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@ import type { Person, PrivateProfile } from '@cfp/shared/schemas';
3232
const CHAT_CHANNEL_REGEX = /^[a-z0-9][a-z0-9_-]{0,40}$/;
3333
const RESUME_COOKIE = 'cfp_saml_resume';
3434
const RESUME_COOKIE_TTL_SECONDS = 10 * 60;
35+
/** Site-relative — it is also the `?return=` handed to /login (see safeReturnPath). */
36+
const RESUME_PATH = '/api/saml/slack/sso/resume';
3537

3638
/**
3739
* SAML ID values must start with an XML NCName character — `_` followed by
@@ -55,26 +57,6 @@ function safeReturnPath(input: string | undefined | null): string {
5557
return input;
5658
}
5759

58-
function selfUrl(request: FastifyRequest): string {
59-
const protocol = request.headers['x-forwarded-proto']
60-
? String(request.headers['x-forwarded-proto']).split(',')[0]?.trim() ?? 'http'
61-
: request.protocol;
62-
const host = request.headers['x-forwarded-host']
63-
? String(request.headers['x-forwarded-host']).split(',')[0]?.trim() ?? request.hostname
64-
: request.hostname;
65-
return `${protocol}://${host}${request.url}`;
66-
}
67-
68-
function originBase(request: FastifyRequest): string {
69-
const protocol = request.headers['x-forwarded-proto']
70-
? String(request.headers['x-forwarded-proto']).split(',')[0]?.trim() ?? 'http'
71-
: request.protocol;
72-
const host = request.headers['x-forwarded-host']
73-
? String(request.headers['x-forwarded-host']).split(',')[0]?.trim() ?? request.hostname
74-
: request.hostname;
75-
return `${protocol}://${host}`;
76-
}
77-
7860
function htmlEscape(value: string): string {
7961
return value
8062
.replace(/&/g, '&')
@@ -361,8 +343,10 @@ async function handleSpInitiatedSso(
361343
path: '/api/saml',
362344
maxAge: RESUME_COOKIE_TTL_SECONDS,
363345
});
364-
const resumeReturn = `${originBase(request)}/api/saml/slack/sso/resume`;
365-
return reply.redirect(`/login?return=${encodeURIComponent(resumeReturn)}`);
346+
// Site-relative on purpose: `/login` and the OAuth start both run the
347+
// return through the same leading-slash guard as `safeReturnPath`, so an
348+
// absolute URL here is silently dropped and the user lands on `/`.
349+
return reply.redirect(`/login?return=${encodeURIComponent(RESUME_PATH)}`);
366350
}
367351

368352
// Signed in — build the assertion immediately.
@@ -462,10 +446,11 @@ export async function samlRoutes(fastify: FastifyInstance): Promise<void> {
462446
async (request, reply) => {
463447
const cfg = fastify.config;
464448

465-
// Anonymous → bounce through /login preserving the current URL.
449+
// Anonymous → bounce through /login preserving the current path + query
450+
// (`channel` / `redir` ride along). Site-relative: see the SP-initiated
451+
// handler for why an absolute URL would be dropped.
466452
if (!request.session.personId) {
467-
const here = selfUrl(request);
468-
return reply.redirect(`/login?return=${encodeURIComponent(here)}`);
453+
return reply.redirect(`/login?return=${encodeURIComponent(safeReturnPath(request.url))}`);
469454
}
470455

471456
const query = request.query as { channel?: string; redir?: string };
@@ -589,7 +574,7 @@ export async function samlRoutes(fastify: FastifyInstance): Promise<void> {
589574
// -------------------------------------------------------------------------
590575

591576
fastify.get(
592-
'/api/saml/slack/sso/resume',
577+
RESUME_PATH,
593578
{
594579
schema: {
595580
tags: ['saml'],

‎apps/api/tests/saml.test.ts‎

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,16 @@ function slackAuthnRequestXml(id: string, acsHost: string = SLACK_TEAM_HOST): st
7373
<samlp:AuthnRequest xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" ID="${id}" Version="2.0" IssueInstant="2026-05-01T00:00:00Z" AssertionConsumerServiceURL="https://${acsHost}/sso/saml" ProtocolBinding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST"><saml:Issuer>https://slack.com</saml:Issuer></samlp:AuthnRequest>`;
7474
}
7575

76+
/**
77+
* The decoded `?return=` a /login redirect carries. Must be site-relative:
78+
* both /login and the OAuth start drop anything not beginning with one `/`.
79+
*/
80+
function loginReturnOf(location: string | string[] | undefined): string {
81+
const url = new URL(String(location), 'http://placeholder.test');
82+
expect(url.pathname).toBe('/login');
83+
return url.searchParams.get('return') ?? '';
84+
}
85+
7686
/**
7787
* HTTP-Redirect binding encoding (saml-bindings §3.4.4.1): raw DEFLATE →
7888
* base64 → URL-encode. Returns the ready-to-append query string.
@@ -232,10 +242,21 @@ describe('SAML IdP — Slack', () => {
232242
expect(formats).toContain('urn:oasis:names:tc:SAML:2.0:nameid-format:persistent');
233243
});
234244

235-
it('GET /api/saml/slack/launch (anonymous) redirects to /login', async () => {
245+
it('GET /api/saml/slack/launch (anonymous) redirects to /login with a site-relative return', async () => {
236246
const res = await app.inject({ method: 'GET', url: '/api/saml/slack/launch' });
237247
expect(res.statusCode).toBe(302);
238-
expect(res.headers.location).toMatch(/^\/login\?return=/);
248+
// An absolute URL here is silently dropped by /login's leading-slash guard
249+
// and the user lands on `/` instead of coming back to finish SSO.
250+
expect(loginReturnOf(res.headers.location)).toBe('/api/saml/slack/launch');
251+
});
252+
253+
it('GET /api/saml/slack/launch (anonymous) keeps channel/redir in the return', async () => {
254+
const res = await app.inject({
255+
method: 'GET',
256+
url: '/api/saml/slack/launch?channel=general',
257+
});
258+
expect(res.statusCode).toBe(302);
259+
expect(loginReturnOf(res.headers.location)).toBe('/api/saml/slack/launch?channel=general');
239260
});
240261

241262
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', () => {
400421
url: `/api/saml/slack/sso?${redirectBindingQuery(slackAuthnRequestXml('id-redirect-1'), 'opaque-redirect-state')}`,
401422
});
402423
expect(res.statusCode).toBe(302);
403-
expect(res.headers.location).toMatch(/^\/login\?return=/);
424+
expect(loginReturnOf(res.headers.location)).toBe('/api/saml/slack/sso/resume');
404425
expect(resumeCookieValue(res)).not.toBe('');
405426
});
406427

@@ -513,7 +534,7 @@ describe('SAML IdP — Slack', () => {
513534
headers: { 'content-type': 'application/x-www-form-urlencoded' },
514535
});
515536
expect(res.statusCode).toBe(302);
516-
expect(res.headers.location).toMatch(/^\/login\?return=/);
537+
expect(loginReturnOf(res.headers.location)).toBe('/api/saml/slack/sso/resume');
517538
// Resume cookie set
518539
const cookies = res.headers['set-cookie'];
519540
const cookieStr = Array.isArray(cookies) ? cookies.join('\n') : String(cookies ?? '');

‎apps/web/src/lib/return-path.ts‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
import type { NavigateFunction } from 'react-router';
2+
3+
/**
4+
* Where to land after sign-in / account claim. Mirrors the API's
5+
* `safeReturnPath`: site-relative only (one leading slash), so a crafted
6+
* `?return=` can't bounce the browser off-site. Anything else → `/`.
7+
*/
8+
export function safeReturn(input: string | null): string {
9+
if (!input) return '/';
10+
if (!input.startsWith('/') || input.startsWith('//')) return '/';
11+
return input;
12+
}
13+
14+
/**
15+
* `/api/**` is served by Fastify, not the SPA — the SAML resume endpoint hands
16+
* the browser on to Slack from there. Client-side routing to one of those
17+
* paths never reaches the server and renders the SPA's `*` NotFound instead,
18+
* so they need a real navigation. Everything else stays in-app.
19+
*/
20+
export function goToReturn(navigate: NavigateFunction, path: string): void {
21+
if (path.startsWith('/api/')) {
22+
window.location.assign(path);
23+
return;
24+
}
25+
void navigate(path, { replace: true });
26+
}

‎apps/web/src/pages/AccountClaim.tsx‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
import { Button } from '@/components/ui/button';
1212
import { Separator } from '@/components/ui/separator';
1313
import { useAuth } from '@/hooks/useAuth';
14+
import { goToReturn, safeReturn } from '@/lib/return-path';
1415
import {
1516
api,
1617
ApiError,
@@ -19,12 +20,6 @@ import {
1920
} from '@/lib/api';
2021
import { formatAbsoluteDate, formatRelativeTime } from '@/lib/time';
2122

22-
function safeReturn(input: string | null): string {
23-
if (!input) return '/';
24-
if (!input.startsWith('/') || input.startsWith('//')) return '/';
25-
return input;
26-
}
27-
2823
export function AccountClaim() {
2924
const [searchParams] = useSearchParams();
3025
const navigate = useNavigate();
@@ -82,7 +77,7 @@ export function AccountClaim() {
8277
await api.accountClaim.confirm(candidate.personId);
8378
toast.success(`Welcome back, ${candidate.fullName}`);
8479
await reload();
85-
void navigate(returnPath, { replace: true });
80+
goToReturn(navigate, returnPath);
8681
} catch (err) {
8782
const msg = err instanceof ApiError ? err.message : 'Failed to confirm';
8883
toast.error(msg);
@@ -96,7 +91,7 @@ export function AccountClaim() {
9691
await api.accountClaim.decline();
9792
toast.success("Got it — we'll set up a fresh profile");
9893
await reload();
99-
void navigate(returnPath, { replace: true });
94+
goToReturn(navigate, returnPath);
10095
} catch (err) {
10196
toast.error(err instanceof ApiError ? err.message : 'Failed to start fresh');
10297
setDeclining(false);

‎apps/web/src/pages/AccountClaimByPassword.tsx‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,7 @@ import { Input } from '@/components/ui/input';
1313
import { Label } from '@/components/ui/label';
1414
import { api, ApiError } from '@/lib/api';
1515
import { useAuth } from '@/hooks/useAuth';
16-
17-
function safeReturn(input: string | null): string {
18-
if (!input) return '/';
19-
if (!input.startsWith('/') || input.startsWith('//')) return '/';
20-
return input;
21-
}
16+
import { goToReturn, safeReturn } from '@/lib/return-path';
2217

2318
export function AccountClaimByPassword() {
2419
const [searchParams] = useSearchParams();
@@ -39,7 +34,7 @@ export function AccountClaimByPassword() {
3934
await api.accountClaim.byPassword(slug.trim(), password);
4035
toast.success('Welcome back');
4136
await reload();
42-
void navigate(returnPath, { replace: true });
37+
goToReturn(navigate, returnPath);
4338
} catch (err) {
4439
if (err instanceof ApiError) {
4540
if (err.status === 401 && err.code === 'claim_token_invalid') {

‎apps/web/src/pages/AccountClaimRequestStaffReview.tsx‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,7 @@ import { Label } from '@/components/ui/label';
1414
import { Textarea } from '@/components/ui/textarea';
1515
import { api, ApiError } from '@/lib/api';
1616
import { useAuth } from '@/hooks/useAuth';
17-
18-
function safeReturn(input: string | null): string {
19-
if (!input) return '/';
20-
if (!input.startsWith('/') || input.startsWith('//')) return '/';
21-
return input;
22-
}
17+
import { goToReturn, safeReturn } from '@/lib/return-path';
2318

2419
export function AccountClaimRequestStaffReview() {
2520
const [searchParams] = useSearchParams();
@@ -55,7 +50,7 @@ export function AccountClaimRequestStaffReview() {
5550
try {
5651
await api.accountClaim.decline();
5752
await reload();
58-
void navigate(returnPath, { replace: true });
53+
goToReturn(navigate, returnPath);
5954
} catch (err) {
6055
toast.error(err instanceof ApiError ? err.message : 'Failed to continue');
6156
setContinuing(false);

‎apps/web/src/pages/LoginPlaceholder.tsx‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import { Label } from '@/components/ui/label';
1313
import { GitHubIcon } from '@/components/icons/GitHubIcon';
1414
import { useAuth } from '@/hooks/useAuth';
1515
import { api, ApiError } from '@/lib/api';
16+
import { goToReturn, safeReturn } from '@/lib/return-path';
1617

1718
type ErrorCode =
1819
| 'access_denied'
@@ -93,15 +94,14 @@ export function LoginPlaceholder() {
9394
const navigate = useNavigate();
9495
const [searchParams] = useSearchParams();
9596

96-
const returnPath = searchParams.get('return');
97+
const rawReturn = searchParams.get('return');
98+
const returnPath = safeReturn(rawReturn);
9799
const errorCode = searchParams.get('error') as ErrorCode | null;
98100

99101
// Redirect already-authenticated users
100102
useEffect(() => {
101103
if (!loading && person) {
102-
const target =
103-
returnPath && returnPath.startsWith('/') ? returnPath : '/';
104-
void navigate(target, { replace: true });
104+
goToReturn(navigate, returnPath);
105105
}
106106
}, [loading, person, navigate, returnPath]);
107107

@@ -117,7 +117,7 @@ export function LoginPlaceholder() {
117117
);
118118
}
119119

120-
const startUrl = returnPath
120+
const startUrl = rawReturn
121121
? `/api/auth/github/start?return=${encodeURIComponent(returnPath)}`
122122
: '/api/auth/github/start';
123123

@@ -126,9 +126,7 @@ export function LoginPlaceholder() {
126126
// picks up the new session before we navigate. Without this, the
127127
// navbar stays in its anonymous state until the next hard refresh.
128128
await reload();
129-
const target =
130-
returnPath && returnPath.startsWith('/') ? returnPath : '/';
131-
void navigate(target, { replace: true });
129+
goToReturn(navigate, returnPath);
132130
};
133131

134132
return (

‎plans/saml-login-return-path.md‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
---
2+
status: done
3+
depends: [saml-authn-statement]
4+
specs:
5+
- specs/api/saml.md
6+
issues: []
7+
pr: 175
8+
---
9+
10+
# Plan: SAML sign-in return path
11+
12+
## Scope
13+
14+
Slack's "Test configuration" against production got as far as our `/login`,
15+
then dropped the user on `/` instead of finishing the SSO round-trip. Two
16+
defects, found by tracing the first real end-to-end attempt:
17+
18+
1. Both anonymous SAML entry points hand `/login?return=` an **absolute** URL
19+
(`https://<host>/api/saml/slack/sso/resume`, and `selfUrl()` for
20+
`/launch`). Every consumer of `?return=` — the login page, the OAuth start,
21+
the account-claim pages — accepts only a site-relative path (one leading
22+
`/`, per the laddr/Emergence convention) and falls back to `/` otherwise.
23+
2. Even with a correct path, the login and account-claim pages hand the
24+
return to React Router. `/api/**` is served by Fastify, not the SPA, so a
25+
client-side `navigate()` there renders the `*` NotFound page and never
26+
reaches the resume endpoint. Only the GitHub OAuth path escaped, because
27+
the API redirects server-side.
28+
29+
In: spec the return contract, fix both entry points, one shared web helper
30+
that hard-navigates for API routes, tests that pin the exact return. Out:
31+
a general "remember where you were" for arbitrary protected pages (already
32+
handled per-page via `?return=`); any change to the assertion itself.
33+
34+
## Implements
35+
36+
- [api/saml.md](../specs/api/saml.md) — `## GET /api/saml/slack/launch` step 1
37+
and `## GET | POST /api/saml/slack/sso` step 2: the return is a
38+
site-relative path (never absolute), and the login page performs a full
39+
navigation when it is an API route.
40+
41+
## Approach
42+
43+
- `apps/api/src/routes/saml.ts`: `RESUME_PATH` constant used by both the park
44+
redirect and the route registration; `/launch` returns
45+
`safeReturnPath(request.url)` so `channel`/`redir` ride along. Drop the two
46+
absolute-URL helpers that no longer have callers.
47+
- `apps/web/src/lib/return-path.ts`: `safeReturn` (was copy-pasted into three
48+
account-claim pages; the login page had a weaker inline guard without the
49+
`//` check) and `goToReturn`, which uses `window.location.assign` for
50+
`/api/` targets and `navigate()` otherwise. All four pages use it.
51+
- Tests: the existing anonymous-redirect assertions only matched
52+
`^/login?return=` and passed with the absolute URL. Decode the `return` and
53+
assert the exact path, plus a `/launch?channel=` case.
54+
55+
## Validation
56+
57+
- `npm run type-check && npm run lint && npm test` clean.
58+
- Slack "Test configuration" against the deployed build completes the
59+
round-trip after a fresh sign-in (the live check this plan exists for).
60+
61+
## Follow-ups
62+
63+
None.

‎specs/api/saml.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ Rules:
114114

115115
### Behavior
116116

117-
1. Require a signed-in session. If not signed in → redirect to `/login?return=<encoded current URL>`.
117+
1. Require a signed-in session. If not signed in → redirect to `/login?return=<encoded site-relative path + query of this request>`. The return is a path, never an absolute URL: `/login` and the OAuth start both drop anything that does not begin with a single `/`.
118118
2. Validate the member is permitted (default: any `user` accountLevel; configurable via the same `userIsPermitted` hook the legacy code provided).
119119
3. Build a SAML Response containing the NameID + attributes for the current Person.
120120
4. Sign the Response with the IdP private key.
@@ -159,7 +159,7 @@ Query string (saml-bindings §3.4.4.1, `DEFLATE` encoding):
159159
### Behavior
160160

161161
1. Decode + parse the AuthnRequest (inflate first for the Redirect binding). Validate signature if Slack signs requests (configurable; usually no for Slack).
162-
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.
162+
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.
163163
3. Resolve the AuthnRequest's `AssertionConsumerServiceURL` against Slack's documented ACS endpoint(s) — only Slack's ACS is accepted.
164164
4. Build + sign a SAML Response as in `/launch`.
165165
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.

0 commit comments

Comments
 (0)