Skip to content

Commit dc247d0

Browse files
themightychristhemightychris
authored
Merge pull request #186 from CodeForPhilly/develop
Release: v0.3.0
2 parents b23759e + 6bd3707 commit dc247d0

4 files changed

Lines changed: 190 additions & 91 deletions

File tree

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

Lines changed: 66 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,16 @@
11
/**
2-
* Chat redirect.
2+
* Chat launch.
33
*
4-
* GET /chat → 302 to https://<SLACK_TEAM_HOST>/
5-
* GET /chat?channel=<name> → 302 to https://<SLACK_TEAM_HOST>/channels/<name>
6-
* GET /chat?channel= → fall back to workspace root
7-
* GET /chat?channel=<invalid> → fall back to root + warn log
4+
* GET /chat → 302 to Slack SSO start, redir=/messages/general/
5+
* GET /chat/<channel> → 302 to Slack SSO start, redir=/messages/<channel>/
6+
* GET /chat?channel=<name> → same as the path form
7+
* GET /chat?channel= or invalid → fall back to general (+ warn log for invalid)
8+
*
9+
* The target is Slack's SP-initiated SSO start URL, not the workspace: Slack
10+
* sends an AuthnRequest to our IdP (/api/saml/slack/sso), the member signs in
11+
* here if needed, and Slack honours `redir` to open the channel. This mirrors
12+
* laddr's Emergence\Slack\Connector::handleLaunchRequest, which the
13+
* codeforphilly.org/chat/<channel> links in the wild were built for.
814
*
915
* 302 (temporary) so the destination can flip later without browser caches
1016
* sticking. Channel format matches Project.chatChannel
@@ -13,58 +19,71 @@
1319
*
1420
* Per specs/screens/chat.md.
1521
*/
16-
import type { FastifyInstance } from 'fastify';
22+
import type { FastifyInstance, FastifyReply } from 'fastify';
1723

1824
const CHANNEL_REGEX = /^[a-z0-9][a-z0-9_-]{0,40}$/;
25+
const DEFAULT_CHANNEL = 'general';
26+
27+
export function slackSsoStartUrl(slackHost: string, channel: string): string {
28+
return `https://${slackHost}/sso/saml/start?redir=${encodeURIComponent(`/messages/${channel}/`)}`;
29+
}
1930

2031
export async function chatRoutes(fastify: FastifyInstance): Promise<void> {
32+
const launch = (reply: FastifyReply, requested: string | null | undefined): FastifyReply => {
33+
const slackHost = fastify.config.SLACK_TEAM_HOST;
34+
// Empty string is spec'd to behave like no channel — fall back to general.
35+
let channel = requested && requested.length > 0 ? requested : DEFAULT_CHANNEL;
36+
if (!CHANNEL_REGEX.test(channel)) {
37+
fastify.log.warn(
38+
// The encoded value keeps log-injection-style payloads benign.
39+
{ channel: encodeURIComponent(channel) },
40+
'chat launch: invalid channel format; falling back to general',
41+
);
42+
channel = DEFAULT_CHANNEL;
43+
}
44+
return reply
45+
.code(302)
46+
.header('Location', slackSsoStartUrl(slackHost, channel))
47+
.header('Cache-Control', 'no-cache')
48+
.send();
49+
};
50+
51+
const querySchema = {
52+
type: 'object',
53+
properties: { channel: { type: 'string' } },
54+
additionalProperties: false,
55+
};
56+
2157
fastify.get(
2258
'/chat',
2359
{
2460
schema: {
2561
tags: ['chat'],
26-
summary: 'Redirect to the Code for Philly Slack workspace',
27-
querystring: {
28-
type: 'object',
29-
properties: { channel: { type: 'string' } },
30-
additionalProperties: false,
31-
},
62+
summary: 'Sign into the Code for Philly Slack workspace via SSO',
63+
querystring: querySchema,
3264
},
3365
},
34-
async (request, reply) => {
35-
const slackHost = fastify.config.SLACK_TEAM_HOST;
36-
const root = `https://${slackHost}/`;
37-
38-
const raw = (request.query as { channel?: string }).channel;
39-
// Empty string is spec'd to behave like no channel — fall back to root.
40-
const channel = raw && raw.length > 0 ? raw : null;
41-
42-
if (channel === null) {
43-
return reply
44-
.code(302)
45-
.header('Location', root)
46-
.header('Cache-Control', 'no-cache')
47-
.send();
48-
}
49-
50-
if (!CHANNEL_REGEX.test(channel)) {
51-
fastify.log.warn(
52-
// The encoded value keeps log-injection-style payloads benign.
53-
{ channel: encodeURIComponent(channel) },
54-
'chat redirect: invalid channel format; falling back to root',
55-
);
56-
return reply
57-
.code(302)
58-
.header('Location', root)
59-
.header('Cache-Control', 'no-cache')
60-
.send();
61-
}
62-
63-
return reply
64-
.code(302)
65-
.header('Location', `https://${slackHost}/channels/${channel}`)
66-
.header('Cache-Control', 'no-cache')
67-
.send();
68-
},
66+
async (request, reply) => launch(reply, (request.query as { channel?: string }).channel),
6967
);
68+
69+
// The app does not ignore trailing slashes globally, and links in the wild
70+
// come in both shapes, so register both.
71+
for (const path of ['/chat/:channel', '/chat/:channel/']) {
72+
fastify.get(
73+
path,
74+
{
75+
schema: {
76+
tags: ['chat'],
77+
summary: 'Sign into the Code for Philly Slack workspace via SSO and open a channel',
78+
params: {
79+
type: 'object',
80+
properties: { channel: { type: 'string' } },
81+
required: ['channel'],
82+
},
83+
querystring: querySchema,
84+
},
85+
},
86+
async (request, reply) => launch(reply, (request.params as { channel: string }).channel),
87+
);
88+
}
7089
}

‎apps/api/tests/chat-redirect.test.ts‎

Lines changed: 57 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,21 @@
11
/**
2-
* Tests for GET /chat — Slack-workspace redirect per specs/screens/chat.md.
2+
* Tests for GET /chat and /chat/<channel> — Slack SSO launch per
3+
* specs/screens/chat.md. Every response is a 302 to Slack's SP-initiated
4+
* SSO start URL carrying `redir=/messages/<channel>/`.
35
*/
46
import { afterAll, beforeAll, describe, expect, it } from 'vitest';
57
import type { FastifyInstance } from 'fastify';
68
import { buildApp } from '../src/app.js';
9+
import { slackSsoStartUrl } from '../src/routes/chat.js';
710
import { createFullDataRepo, createPrivateStorageDir } from './helpers/test-full-repo.js';
811

912
let dataRepo: { path: string; cleanup: () => Promise<void> };
1013
let privateStore: { path: string; cleanup: () => Promise<void> };
1114
let app: FastifyInstance;
1215

16+
const HOST = 'codeforphilly.slack.com';
17+
const start = (channel: string): string => slackSsoStartUrl(HOST, channel);
18+
1319
beforeAll(async () => {
1420
dataRepo = await createFullDataRepo();
1521
privateStore = await createPrivateStorageDir();
@@ -20,6 +26,7 @@ beforeAll(async () => {
2026
STORAGE_BACKEND: 'filesystem',
2127
CFP_PRIVATE_STORAGE_PATH: privateStore.path,
2228
CFP_JWT_SIGNING_KEY: 'test-jwt-signing-key-at-least-32-chars!!',
29+
SLACK_TEAM_HOST: HOST,
2330
NODE_ENV: 'test',
2431
},
2532
});
@@ -31,60 +38,76 @@ afterAll(async () => {
3138
await privateStore.cleanup();
3239
});
3340

41+
async function launch(url: string): Promise<string> {
42+
const res = await app.inject({ method: 'GET', url });
43+
expect(res.statusCode).toBe(302);
44+
expect(res.headers['cache-control']).toBe('no-cache');
45+
return String(res.headers.location);
46+
}
47+
48+
describe('slackSsoStartUrl', () => {
49+
it('targets Slack SSO start with an encoded /messages/<channel>/ redir', () => {
50+
expect(start('general')).toBe(
51+
'https://codeforphilly.slack.com/sso/saml/start?redir=%2Fmessages%2Fgeneral%2F',
52+
);
53+
});
54+
});
55+
3456
describe('GET /chat', () => {
35-
it('redirects to the Slack workspace root when no channel is given', async () => {
36-
const res = await app.inject({ method: 'GET', url: '/chat' });
37-
expect(res.statusCode).toBe(302);
38-
expect(res.headers.location).toBe('https://codeforphilly.slack.com/');
39-
expect(res.headers['cache-control']).toContain('no-cache');
57+
it('launches into #general when no channel is given', async () => {
58+
expect(await launch('/chat')).toBe(start('general'));
4059
});
4160

42-
it('deep-links to a valid channel', async () => {
43-
const res = await app.inject({ method: 'GET', url: '/chat?channel=general' });
44-
expect(res.statusCode).toBe(302);
45-
expect(res.headers.location).toBe('https://codeforphilly.slack.com/channels/general');
61+
it('deep-links a valid ?channel=', async () => {
62+
expect(await launch('/chat?channel=phlask')).toBe(start('phlask'));
4663
});
4764

4865
it('accepts hyphens and underscores in the channel name', async () => {
49-
const res = await app.inject({ method: 'GET', url: '/chat?channel=philly_civic-tech' });
50-
expect(res.statusCode).toBe(302);
51-
expect(res.headers.location).toBe('https://codeforphilly.slack.com/channels/philly_civic-tech');
66+
expect(await launch('/chat?channel=philly_civic-tech')).toBe(start('philly_civic-tech'));
5267
});
5368

54-
it('falls back to root for an empty channel', async () => {
55-
const res = await app.inject({ method: 'GET', url: '/chat?channel=' });
56-
expect(res.statusCode).toBe(302);
57-
expect(res.headers.location).toBe('https://codeforphilly.slack.com/');
69+
it('falls back to #general for an empty channel', async () => {
70+
expect(await launch('/chat?channel=')).toBe(start('general'));
5871
});
5972

60-
it('falls back to root for uppercase characters (invalid format)', async () => {
61-
const res = await app.inject({ method: 'GET', url: '/chat?channel=General' });
62-
expect(res.statusCode).toBe(302);
63-
expect(res.headers.location).toBe('https://codeforphilly.slack.com/');
73+
it('falls back to #general for uppercase characters (invalid format)', async () => {
74+
expect(await launch('/chat?channel=General')).toBe(start('general'));
6475
});
6576

66-
it('falls back to root for slashes (path-injection attempt)', async () => {
67-
const res = await app.inject({ method: 'GET', url: '/chat?channel=foo%2Fbar' });
68-
expect(res.statusCode).toBe(302);
69-
expect(res.headers.location).toBe('https://codeforphilly.slack.com/');
77+
it('falls back to #general for slashes (path-injection attempt)', async () => {
78+
expect(await launch('/chat?channel=foo%2Fbar')).toBe(start('general'));
7079
});
7180

72-
it('falls back to root for an over-long channel name', async () => {
73-
// 42 chars total (> 41 max per the regex)
81+
it('falls back to #general for an over-long channel name', async () => {
7482
const channel = 'a'.repeat(42);
75-
const res = await app.inject({ method: 'GET', url: `/chat?channel=${channel}` });
76-
expect(res.statusCode).toBe(302);
77-
expect(res.headers.location).toBe('https://codeforphilly.slack.com/');
83+
expect(await launch(`/chat?channel=${channel}`)).toBe(start('general'));
7884
});
7985

80-
it('falls back to root for leading hyphen (invalid first char)', async () => {
81-
const res = await app.inject({ method: 'GET', url: '/chat?channel=-leading-hyphen' });
82-
expect(res.statusCode).toBe(302);
83-
expect(res.headers.location).toBe('https://codeforphilly.slack.com/');
86+
it('falls back to #general for a leading hyphen (invalid first char)', async () => {
87+
expect(await launch('/chat?channel=-leading-hyphen')).toBe(start('general'));
8488
});
8589

8690
it('does not register on /api/chat (only /chat)', async () => {
8791
const res = await app.inject({ method: 'GET', url: '/api/chat' });
8892
expect(res.statusCode).toBe(404);
8993
});
9094
});
95+
96+
describe('GET /chat/<channel>', () => {
97+
it('deep-links the path form — the shape of the links in the wild', async () => {
98+
expect(await launch('/chat/phlask')).toBe(start('phlask'));
99+
});
100+
101+
it('tolerates a trailing slash', async () => {
102+
expect(await launch('/chat/phlask/')).toBe(start('phlask'));
103+
});
104+
105+
it('falls back to #general for an invalid path segment', async () => {
106+
expect(await launch('/chat/Not%20A%20Channel')).toBe(start('general'));
107+
});
108+
109+
it('never lets the channel reach the host', async () => {
110+
const location = await launch('/chat/evil.example');
111+
expect(new URL(location).host).toBe(HOST);
112+
});
113+
});

‎plans/chat-sso-launch.md‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
---
2+
status: done
3+
depends: [samlify-esm-interop]
4+
specs:
5+
- specs/screens/chat.md
6+
issues: []
7+
pr: 185
8+
---
9+
10+
# Plan: `/chat/<channel>` signs into Slack and opens the channel
11+
12+
## Scope
13+
14+
Years of distributed links point at `codeforphilly.org/chat/<channel>`. On
15+
laddr that URL signed the member into Slack via SAML and landed them in the
16+
channel. The rewrite's `/chat` only accepted `?channel=` and redirected to
17+
`https://<team>.slack.com/channels/<name>` — which assumes an existing Slack
18+
session — and `/chat/<channel>` fell through to the SPA's 404.
19+
20+
In: spec the path form and the SSO-start target; implement; tests. Out:
21+
IdP-initiated launch (`/api/saml/slack/launch` stays as-is); any change to
22+
the project "Chat Channel" button, which already uses `?channel=`.
23+
24+
## Implements
25+
26+
- [screens/chat.md](../specs/screens/chat.md) — redirect rules: `/chat`,
27+
`/chat/<channel>`, `?channel=`; target is Slack's SP-initiated SSO start
28+
with `redir=/messages/<channel>/`; default channel `general`.
29+
30+
## Approach
31+
32+
- Match laddr exactly: `Emergence\Slack\Connector::handleLaunchRequest`
33+
redirected to `https://<team>.slack.com/sso/saml/start?redir=/messages/<channel>/`.
34+
Slack then drives the SP-initiated flow against `/api/saml/slack/sso`,
35+
verified live on 2026-09-18.
36+
- `apps/api/src/routes/chat.ts`: one `launch()` helper behind `/chat` and
37+
`/chat/:channel`; invalid or empty channel → `general` (warn log on
38+
invalid). Fastify's default `ignoreTrailingSlash` handling covers
39+
`/chat/foo/`.
40+
- Tests: rewrite `chat-redirect.test.ts` expectations to the SSO-start URL
41+
and add the path form + trailing slash + default channel.
42+
43+
## Validation
44+
45+
- `npm run type-check && npm run lint`; chat suite green.
46+
- Live: `/chat/general` from a signed-out browser → CfP login → Slack opens
47+
in #general.
48+
49+
## Follow-ups
50+
51+
None.

‎specs/screens/chat.md‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,38 +1,44 @@
1-
# Screen: Chat redirect
1+
# Screen: Chat launch
22

33
## Route
44

5-
`/chat` — public. Server-side redirect to the Code for Philly Slack workspace.
5+
`/chat` and `/chat/<channel>` — public. Server-side redirect that signs the member into the Code for Philly Slack workspace and lands them in a channel.
66

7-
Optional query parameter `?channel=<name>` redirects to a specific channel.
7+
`?channel=<name>` is accepted as an alternative to the path form (the project "Chat Channel" button uses it).
88

99
## Behavior
1010

1111
Not a rendered screen — a redirect endpoint handled at the API layer (and aliased on the web layer for nice URLs that work without JS).
1212

13+
The redirect targets Slack's **SP-initiated SSO start** URL, not the workspace directly. Slack then sends a SAML AuthnRequest to our IdP (`GET | POST /api/saml/slack/sso` — see [api/saml.md](../api/saml.md)), which signs the member in on our side if needed and posts the assertion back; Slack honours `redir` and opens the channel. This is the legacy laddr behaviour (`Emergence\Slack\Connector::handleLaunchRequest`), and the `codeforphilly.org/chat/<channel>` links distributed over the years depend on it.
14+
1315
### Redirect rules
1416

1517
| Request | Redirect target | HTTP |
1618
| ------- | --------------- | :--: |
17-
| `/chat` | `https://codeforphilly.slack.com/` | 302 |
18-
| `/chat?channel=foo` | `https://codeforphilly.slack.com/channels/foo` | 302 |
19+
| `/chat` | `https://codeforphilly.slack.com/sso/saml/start?redir=%2Fmessages%2Fgeneral%2F` | 302 |
20+
| `/chat/foo` | `https://codeforphilly.slack.com/sso/saml/start?redir=%2Fmessages%2Ffoo%2F` | 302 |
21+
| `/chat?channel=foo` | Same as `/chat/foo` | 302 |
22+
| `/chat/foo/` (trailing slash) | Same as `/chat/foo` | 302 |
1923
| `/chat?channel=` (empty) | Same as `/chat` | 302 |
20-
| `/chat?channel=<invalid format>` | Same as `/chat`, with a query log warning | 302 |
24+
| `/chat/<invalid format>` or `?channel=<invalid format>` | Same as `/chat`, with a log warning | 302 |
25+
26+
The default channel is `general`, as in laddr.
2127

22-
`channel` is validated against the same regex as `Project.chatChannel` (`^[a-z0-9][a-z0-9_-]{0,40}$`) before interpolation, to prevent open-redirect / URL-injection on the Slack workspace URL.
28+
`channel` is validated against the same regex as `Project.chatChannel` (`^[a-z0-9][a-z0-9_-]{0,40}$`) before interpolation, to prevent open-redirect / URL-injection on the Slack workspace URL. The host is always `SLACK_TEAM_HOST`.
2329

2430
Use **302** (temporary) rather than 301 so we can change the destination later without browser-cached redirects sticking.
2531

2632
### Why this exists
2733

28-
- Marketing materials and old links say "join us at codeforphilly.org/chat" — historical, do-not-break.
34+
- Marketing materials and old links say "join us at codeforphilly.org/chat" and deep-link `codeforphilly.org/chat/<channel>` — historical, do-not-break.
2935
- Project pages use `/chat?channel=<chatChannel>` for the "Chat Channel" button so the link looks like part of the site rather than an external Slack URL.
3036
- If we move off Slack later, every link gets re-pointed by changing this one redirect rather than chasing references through the codebase.
3137

3238
## Open redirect protection
3339

34-
`channel` only feeds the path segment after `/channels/`; the host is hard-coded. No user input touches the host.
40+
`channel` only feeds the `redir` value's path segment after `/messages/`; the host is hard-coded. No user input touches the host.
3541

3642
## Authorization
3743

38-
Public.
44+
Public. The SSO round-trip that follows requires a Code for Philly sign-in, and Slack's own workspace membership rules still apply.

0 commit comments

Comments
 (0)