From 161463da0d91221d0668cada5e637eb8792d3040 Mon Sep 17 00:00:00 2001 From: Chris Alfano Date: Sat, 30 May 2026 08:56:29 -0400 Subject: [PATCH 1/3] chore(plans): open chat-redirect (in-progress) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /chat is spec'd in specs/screens/chat.md as a 302 to the Code for Philly Slack workspace, with optional ?channel=foo deep-linking. The route doesn't exist today — every "Open Slack" link across the SPA falls through to the SPA catch-all. Plan covers route, channel validation (reusing Project.chatChannel regex for open-redirect safety), tests. Closes #79. Co-Authored-By: Claude Opus 4.7 (1M context) --- plans/chat-redirect.md | 80 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) create mode 100644 plans/chat-redirect.md diff --git a/plans/chat-redirect.md b/plans/chat-redirect.md new file mode 100644 index 0000000..88c31eb --- /dev/null +++ b/plans/chat-redirect.md @@ -0,0 +1,80 @@ +--- +status: in-progress +depends: [] +specs: + - specs/screens/chat.md +issues: [79] +--- + +# Plan: /chat Slack redirect + +## Scope + +[`specs/screens/chat.md`](../specs/screens/chat.md) declares `/chat` as a server-side 302 to the Code for Philly Slack workspace, optionally deep-linking to a channel via `?channel=foo`. Nothing serves it today — every "Open Slack" link across the SPA falls through to the SPA's catch-all (200 HTML). + +The redirect rules from the spec: + +| Request | Redirect target | HTTP | +|---|---|---| +| `/chat` | `https:///` | 302 | +| `/chat?channel=foo` | `https:///channels/foo` | 302 | +| `/chat?channel=` (empty) | Same as `/chat` | 302 | +| `/chat?channel=` | Same as `/chat`, with a log warn | 302 | + +302 (temporary) rather than 301 so we can change the destination later without browser-cached redirects sticking. + +Closes [#79](https://github.com/CodeForPhilly/codeforphilly-ng/issues/79). + +## Implements + +- [screens/chat.md](../specs/screens/chat.md) — full spec. + +## Approach + +### 1. Route, not hook + +A plain Fastify `fastify.get('/chat', ...)` route. The two existing redirect plugins (`slug-redirect`, `legacy-redirect`) use `onRequest` hooks because they pattern-match across many URL shapes; `/chat` is exact and Fastify routing matches before the SPA notFoundHandler runs, so a route is the right shape. + +### 2. Channel validation + +The channel regex matches `Project.chatChannel` in `packages/shared/src/schemas/project.ts`: `/^[a-z0-9][a-z0-9_-]{0,40}$/`. Hoist that to a small shared constant or import the schema's pattern; either way the route gates on the same regex so a channel that came from a project record always works. + +Invalid channels — including empty — fall back to the workspace root + a `warn` log entry with the offending value (URL-component-encoded to keep the log line safe). Per spec, no 4xx; just degrade. + +### 3. Open-redirect protection + +The host is hardcoded from env (`fastify.config.SLACK_TEAM_HOST`, already exists, defaults to `codeforphilly.slack.com`). The channel value only feeds the path segment, never the host. The regex restricts it to `[a-z0-9_-]` so there's no `..`, no `/`, no protocol smuggling. + +### 4. Tests + +`apps/api/tests/chat-redirect.test.ts`: + +- `GET /chat` → 302, `Location: https://codeforphilly.slack.com/` +- `GET /chat?channel=general` → 302, `Location: …/channels/general` +- `GET /chat?channel=` → 302, root (no `/channels/`) +- `GET /chat?channel=foo/bar` → 302, root (regex rejects `/`) +- `GET /chat?channel=Capital` → 302, root (regex rejects uppercase) +- `GET /chat?channel=` with all `_` and `-` chars allowed (`philly_civic-tech`) → channel deep-link +- `Cache-Control` header — `no-cache` so changing the destination is immediate (302 is already temp, but the header is belt + suspenders) +- The route doesn't appear on `/api/*` paths — only on `/chat`. Verify `/api/chat` 404s as JSON. + +## Validation + +- [ ] Route registered, 302s match the spec table. +- [ ] Channel regex matches `Project.chatChannel`. +- [ ] Invalid channels fall back to root + emit a warn log. +- [ ] Existing 310 API tests still pass. +- [ ] `npm run type-check && npm run lint` clean. + +## Risks / unknowns + +- **Slack URL shape stability.** `/channels/` is the current Slack deep-link path; if Slack changes it, our redirect breaks. Acceptable risk — the 302 means we can flip the destination in one commit. +- **No-cache header vs CDN.** 302s without explicit `Cache-Control` may be cached briefly by intermediaries. Adding `Cache-Control: no-cache` is cheap and matches the spec's "we can change the destination later" intent. + +## Notes + +_(filled at done time)_ + +## Follow-ups + +_(filled at done time)_ From 39c3ef65d9b0388adccad4d08b3ca004e3479e7a Mon Sep 17 00:00:00 2001 From: Chris Alfano Date: Sat, 30 May 2026 08:56:43 -0400 Subject: [PATCH 2/3] feat(api): /chat Slack workspace redirect (closes #79) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET /chat 302s to https:/// and GET /chat?channel= 302s to /channels/. Empty or malformed channels fall back to the workspace root with a warn log (per spec: degrade quietly rather than 4xx). Cache-Control: no-cache so the destination can flip without browser caches sticking; 302 (not 301) for the same reason. Channel validation reuses the Project.chatChannel regex (/^[a-z0-9][a-z0-9_-]{0,40}$/) — same shape the data already enforces, and tight enough that the channel can't smuggle a slash, protocol, or relative-path traversal into the redirect URL. The hardcoded host comes from fastify.config.SLACK_TEAM_HOST, never the request. Per specs/screens/chat.md. Co-Authored-By: Claude Opus 4.7 (1M context) --- apps/api/src/app.ts | 2 + apps/api/src/routes/chat.ts | 70 ++++++++++++++++++++++ apps/api/tests/chat-redirect.test.ts | 90 ++++++++++++++++++++++++++++ 3 files changed, 162 insertions(+) create mode 100644 apps/api/src/routes/chat.ts create mode 100644 apps/api/tests/chat-redirect.test.ts diff --git a/apps/api/src/app.ts b/apps/api/src/app.ts index 7a338b8..874c0fb 100644 --- a/apps/api/src/app.ts +++ b/apps/api/src/app.ts @@ -57,6 +57,7 @@ import { helpWantedRoutes } from './routes/projects-help-wanted.js'; import { projectMembershipRoutes } from './routes/projects-members.js'; import { previewRoutes } from './routes/preview.js'; import { attachmentRoutes } from './routes/attachments.js'; +import { chatRoutes } from './routes/chat.js'; import { samlRoutes } from './routes/saml.js'; import { internalRoutes } from './routes/internal.js'; @@ -194,6 +195,7 @@ export async function buildApp(opts: BuildAppOptions = {}): Promise/ + * GET /chat?channel= → 302 to https:///channels/ + * GET /chat?channel= → fall back to workspace root + * GET /chat?channel= → fall back to root + warn log + * + * 302 (temporary) so the destination can flip later without browser caches + * sticking. Channel format matches Project.chatChannel + * (`/^[a-z0-9][a-z0-9_-]{0,40}$/`) — same regex protects against + * open-redirect via URL injection. + * + * Per specs/screens/chat.md. + */ +import type { FastifyInstance } from 'fastify'; + +const CHANNEL_REGEX = /^[a-z0-9][a-z0-9_-]{0,40}$/; + +export async function chatRoutes(fastify: FastifyInstance): Promise { + fastify.get( + '/chat', + { + schema: { + tags: ['chat'], + summary: 'Redirect to the Code for Philly Slack workspace', + querystring: { + type: 'object', + properties: { channel: { type: 'string' } }, + additionalProperties: false, + }, + }, + }, + async (request, reply) => { + const slackHost = fastify.config.SLACK_TEAM_HOST; + const root = `https://${slackHost}/`; + + const raw = (request.query as { channel?: string }).channel; + // Empty string is spec'd to behave like no channel — fall back to root. + const channel = raw && raw.length > 0 ? raw : null; + + if (channel === null) { + return reply + .code(302) + .header('Location', root) + .header('Cache-Control', 'no-cache') + .send(); + } + + if (!CHANNEL_REGEX.test(channel)) { + fastify.log.warn( + // The encoded value keeps log-injection-style payloads benign. + { channel: encodeURIComponent(channel) }, + 'chat redirect: invalid channel format; falling back to root', + ); + return reply + .code(302) + .header('Location', root) + .header('Cache-Control', 'no-cache') + .send(); + } + + return reply + .code(302) + .header('Location', `https://${slackHost}/channels/${channel}`) + .header('Cache-Control', 'no-cache') + .send(); + }, + ); +} diff --git a/apps/api/tests/chat-redirect.test.ts b/apps/api/tests/chat-redirect.test.ts new file mode 100644 index 0000000..9701d62 --- /dev/null +++ b/apps/api/tests/chat-redirect.test.ts @@ -0,0 +1,90 @@ +/** + * Tests for GET /chat — Slack-workspace redirect per specs/screens/chat.md. + */ +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import type { FastifyInstance } from 'fastify'; +import { buildApp } from '../src/app.js'; +import { createFullDataRepo, createPrivateStorageDir } from './helpers/test-full-repo.js'; + +let dataRepo: { path: string; cleanup: () => Promise }; +let privateStore: { path: string; cleanup: () => Promise }; +let app: FastifyInstance; + +beforeAll(async () => { + dataRepo = await createFullDataRepo(); + privateStore = await createPrivateStorageDir(); + app = await buildApp({ + serverOptions: { logger: false }, + overrideEnv: { + CFP_DATA_REPO_PATH: dataRepo.path, + STORAGE_BACKEND: 'filesystem', + CFP_PRIVATE_STORAGE_PATH: privateStore.path, + CFP_JWT_SIGNING_KEY: 'test-jwt-signing-key-at-least-32-chars!!', + NODE_ENV: 'test', + }, + }); +}, 60_000); + +afterAll(async () => { + await app.close(); + await dataRepo.cleanup(); + await privateStore.cleanup(); +}); + +describe('GET /chat', () => { + it('redirects to the Slack workspace root when no channel is given', async () => { + const res = await app.inject({ method: 'GET', url: '/chat' }); + expect(res.statusCode).toBe(302); + expect(res.headers.location).toBe('https://codeforphilly.slack.com/'); + expect(res.headers['cache-control']).toContain('no-cache'); + }); + + it('deep-links to a valid channel', async () => { + const res = await app.inject({ method: 'GET', url: '/chat?channel=general' }); + expect(res.statusCode).toBe(302); + expect(res.headers.location).toBe('https://codeforphilly.slack.com/channels/general'); + }); + + it('accepts hyphens and underscores in the channel name', async () => { + const res = await app.inject({ method: 'GET', url: '/chat?channel=philly_civic-tech' }); + expect(res.statusCode).toBe(302); + expect(res.headers.location).toBe('https://codeforphilly.slack.com/channels/philly_civic-tech'); + }); + + it('falls back to root for an empty channel', async () => { + const res = await app.inject({ method: 'GET', url: '/chat?channel=' }); + expect(res.statusCode).toBe(302); + expect(res.headers.location).toBe('https://codeforphilly.slack.com/'); + }); + + it('falls back to root for uppercase characters (invalid format)', async () => { + const res = await app.inject({ method: 'GET', url: '/chat?channel=General' }); + expect(res.statusCode).toBe(302); + expect(res.headers.location).toBe('https://codeforphilly.slack.com/'); + }); + + it('falls back to root for slashes (path-injection attempt)', async () => { + const res = await app.inject({ method: 'GET', url: '/chat?channel=foo%2Fbar' }); + expect(res.statusCode).toBe(302); + expect(res.headers.location).toBe('https://codeforphilly.slack.com/'); + }); + + it('falls back to root for an over-long channel name', async () => { + // 42 chars total (> 41 max per the regex) + const channel = 'a'.repeat(42); + const res = await app.inject({ method: 'GET', url: `/chat?channel=${channel}` }); + expect(res.statusCode).toBe(302); + expect(res.headers.location).toBe('https://codeforphilly.slack.com/'); + }); + + it('falls back to root for leading hyphen (invalid first char)', async () => { + const res = await app.inject({ method: 'GET', url: '/chat?channel=-leading-hyphen' }); + expect(res.statusCode).toBe(302); + expect(res.headers.location).toBe('https://codeforphilly.slack.com/'); + }); + + it('does not register on /api/chat (only /chat)', async () => { + const res = await app.inject({ method: 'GET', url: '/api/chat' }); + expect(res.statusCode).toBe(404); + }); +}); From 104ccc1d6ca2107e58e59d9c59d64f0dd160cc1d Mon Sep 17 00:00:00 2001 From: Chris Alfano Date: Sat, 30 May 2026 09:15:00 -0400 Subject: [PATCH 3/3] chore(plans): close out chat-redirect (PR #100) All validation checkboxes ticked. Notes covers the channel-regex hoist trade-off (duplicate literal rather than Zod-internals plumbing), the Fastify querystring typing pattern, and the /api/chat 404 assertion. Follow-ups: typeahead is None (no real ask); per-channel analytics deferred to next observability pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- plans/chat-redirect.md | 50 +++++++++++++++++++++++++++++++++++------- 1 file changed, 42 insertions(+), 8 deletions(-) diff --git a/plans/chat-redirect.md b/plans/chat-redirect.md index 88c31eb..508eb04 100644 --- a/plans/chat-redirect.md +++ b/plans/chat-redirect.md @@ -1,9 +1,10 @@ --- -status: in-progress +status: done depends: [] specs: - specs/screens/chat.md issues: [79] +pr: 100 --- # Plan: /chat Slack redirect @@ -60,11 +61,11 @@ The host is hardcoded from env (`fastify.config.SLACK_TEAM_HOST`, already exists ## Validation -- [ ] Route registered, 302s match the spec table. -- [ ] Channel regex matches `Project.chatChannel`. -- [ ] Invalid channels fall back to root + emit a warn log. -- [ ] Existing 310 API tests still pass. -- [ ] `npm run type-check && npm run lint` clean. +- [x] Route registered, 302s match the spec table. +- [x] Channel regex matches `Project.chatChannel`. +- [x] Invalid channels fall back to root + emit a warn log. +- [x] All tests pass — 319 API + 30 web + 69 shared. +- [x] `npm run type-check && npm run lint` clean. ## Risks / unknowns @@ -73,8 +74,41 @@ The host is hardcoded from env (`fastify.config.SLACK_TEAM_HOST`, already exists ## Notes -_(filled at done time)_ +Three commits: plan-open, feat (route + tests), closeout. The route +itself is ~50 lines including the JSDoc — the bulk of the work was +nailing the channel regex semantics so it matches what `Project.chatChannel` +already enforces, and writing the test sweep that exercises the +fall-back paths (empty, uppercase, slashes, leading hyphen, over-long). + +Surprises: + +- **Channel-regex hoist not needed.** I considered importing the + `Project.chatChannel` Zod regex from `packages/shared/src/schemas/project.ts` + so the two stay in lockstep, but Zod doesn't expose its underlying + `RegExp` cleanly without `.shape` plumbing. A duplicated literal with + a comment pointing at the canonical source is plainly the right + trade-off here — the regex is one line, will rarely change, and any + drift would be caught the next time someone adds a `Project.chatChannel` + case to the route tests. +- **`request.query` typing.** Fastify's JSON-Schema querystring + validator gives the handler a properly-typed `request.query` only + when the JSON Schema's TypeScript-Provider is wired up. We don't + have that across the codebase yet — every other route does + `(request.query as { ... })`. Followed the established pattern + rather than introducing TypeBox here. +- **`/api/chat` 404s as expected.** The route is registered without + the `/api` prefix (Fastify's prefix only applies to plugin-scoped + routes, and this route is registered at the app root). A test asserts + that `/api/chat` returns 404 so future-me doesn't wonder whether it + needed `/api/chat` for the spec. ## Follow-ups -_(filled at done time)_ +- **Slack channel directory.** Eventually the spec'd Volunteer screen + may want a typeahead of valid channels. Out of scope here; would be + a separate enhancement on top of an actual Slack-integration story. + *None* — not worth tracking until there's a real ask. +- **Per-channel analytics.** Knowing which `?channel=` deep-links get + used would inform what to feature on the SPA. Tiny — could wrap the + log line with a metric tag. *Deferred to plan* — bundle with the + next observability pass if/when we wire metrics.