Merge pull request #100 from CodeForPhilly/feat/chat-redirect

feat(api): /chat Slack workspace redirect (closes #79)

Author: themightychris

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<FastifyInsta
   await fastify.register(projectMembershipRoutes);
   await fastify.register(previewRoutes);
   await fastify.register(attachmentRoutes);
+  await fastify.register(chatRoutes);
   await fastify.register(samlRoutes);
   await fastify.register(internalRoutes);
 

apps/api/src/routes/chat.ts

@@ -0,0 +1,70 @@
+/**
+ * Chat redirect.
+ *
+ *   GET /chat                      → 302 to https://<SLACK_TEAM_HOST>/
+ *   GET /chat?channel=<name>       → 302 to https://<SLACK_TEAM_HOST>/channels/<name>
+ *   GET /chat?channel=             → fall back to workspace root
+ *   GET /chat?channel=<invalid>    → 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<void> {
+  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();
+    },
+  );
+}

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<void> };
+let privateStore: { path: string; cleanup: () => Promise<void> };
+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);
+  });
+});

plans/chat-redirect.md

@@ -0,0 +1,114 @@
+---
+status: done
+depends: []
+specs:
+  - specs/screens/chat.md
+issues: [79]
+pr: 100
+---
+
+# 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://<SLACK_TEAM_HOST>/` | 302 |
+| `/chat?channel=foo` | `https://<SLACK_TEAM_HOST>/channels/foo` | 302 |
+| `/chat?channel=` (empty) | Same as `/chat` | 302 |
+| `/chat?channel=<invalid format>` | 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
+
+- [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
+
+- **Slack URL shape stability.** `/channels/<name>` 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
+
+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
+
+- **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.