From c1bcd3d13422d579ceb1ac5a984599160d45be1f Mon Sep 17 00:00:00 2001 From: guzmonne Date: Sun, 16 Aug 2026 16:10:10 -0300 Subject: [PATCH 01/10] feat(web): show story owner on the backlog row storyOwner() picks the first assignee (GitHub's order, not sorted) and reports how many are left over, returning null when a story has no assignees so no call site can render an "unassigned" placeholder by accident. issue-row.tsx renders "@login" plus "+N" between the label chips and the relative-time stamp, and nothing when the story is unassigned. --- apps/web/components/issues/issue-row.tsx | 9 +++++++- apps/web/lib/pipeline.ts | 10 +++++++++ apps/web/test/pipeline-story.test.ts | 26 +++++++++++++++++++++++- 3 files changed, 43 insertions(+), 2 deletions(-) diff --git a/apps/web/components/issues/issue-row.tsx b/apps/web/components/issues/issue-row.tsx index c96b2375..4c7029b9 100644 --- a/apps/web/components/issues/issue-row.tsx +++ b/apps/web/components/issues/issue-row.tsx @@ -6,7 +6,7 @@ import { useRouter } from "next/navigation"; import { useState } from "react"; import { CiStatusLink } from "@/components/ci-status"; import type { PipelineStory } from "@/lib/pipeline"; -import { storyHref } from "@/lib/pipeline"; +import { storyHref, storyOwner } from "@/lib/pipeline"; function fmtAgo(iso: string | null) { if (!iso) return "—"; @@ -61,6 +61,7 @@ export function IssueRow({ } const current = story.currentRun; + const owner = storyOwner(story.assignees); const openPull = story.prs.find((pull) => pull.state === "open") ?? null; const failedAgent = current?.mode.includes("architect") ? "architect" @@ -194,6 +195,12 @@ export function IssueRow({ {label} ))} + {owner ? ( + + @{owner.login} + {owner.extra > 0 ? ` +${owner.extra}` : ""} + + ) : null} {fmtAgo(story.ghUpdatedAt)} {action()} diff --git a/apps/web/lib/pipeline.ts b/apps/web/lib/pipeline.ts index 679199c5..8d1f897a 100644 --- a/apps/web/lib/pipeline.ts +++ b/apps/web/lib/pipeline.ts @@ -104,6 +104,16 @@ export function storyHref( return `/projects/${projectId}/stories/${story.number}?${storyQuery(story)}`; } +export type StoryOwner = { login: string; extra: number }; + +/** The story's lead assignee, GitHub-ordered, with a count of the rest. */ +export function storyOwner(assignees: string[]): StoryOwner | null { + const logins = assignees.map((login) => login.trim()).filter(Boolean); + const [login] = logins; + if (!login) return null; + return { login, extra: logins.length - 1 }; +} + export function pipelineStories(pipeline: Pipeline): PipelineStory[] { return pipeline.stages.flatMap((stage) => stage.stories); } diff --git a/apps/web/test/pipeline-story.test.ts b/apps/web/test/pipeline-story.test.ts index 836e08c5..de60a4ba 100644 --- a/apps/web/test/pipeline-story.test.ts +++ b/apps/web/test/pipeline-story.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it } from "vitest"; import { ciStatusLabel } from "@/components/ci-status"; import type { PipelineStageKey, PipelineStory, Proposal, StoryDetail } from "@/lib/api"; -import { reviewablePullRequests, storyHref } from "@/lib/pipeline"; +import { reviewablePullRequests, storyHref, storyOwner } from "@/lib/pipeline"; import { deriveStoryTimeline, proposalsForStory } from "@/lib/story"; describe("story presentation contract", () => { @@ -277,6 +277,30 @@ describe("story presentation contract", () => { expect(proposalsForStory([linked, unrelated], detail, false)).toEqual([linked]); }); + it("names no owner for an unassigned story", () => { + expect(storyOwner([])).toBeNull(); + }); + + it("names the sole assignee with nothing left over", () => { + expect(storyOwner(["a"])).toEqual({ login: "a", extra: 0 }); + }); + + it("counts the remaining assignees past the first", () => { + expect(storyOwner(["a", "b", "c"])).toEqual({ login: "a", extra: 2 }); + }); + + it("keeps GitHub's assignee order rather than sorting it", () => { + expect(storyOwner(["zoe", "adam"])).toEqual({ login: "zoe", extra: 1 }); + }); + + it("drops empty and blank assignees before naming an owner", () => { + expect(storyOwner(["", " ", "a"])).toEqual({ login: "a", extra: 0 }); + }); + + it("trims whitespace around an assignee's login", () => { + expect(storyOwner([" a "])).toEqual({ login: "a", extra: 0 }); + }); + it("does not count draft pull requests as waiting for human review", () => { const story = storyDetail(); story.prs = [ From ed444e6b46b9fd4d172c70266579b75b0ff0fc17 Mon Sep 17 00:00:00 2001 From: guzmonne Date: Sun, 16 Aug 2026 16:13:56 -0300 Subject: [PATCH 02/10] feat(web): show story owner in the story header Render the story's lead assignee beside the label chips in the story header, using the same @login (+N) grammar already used on the Backlog row. The header now reads story.assignees from StoryDetail, which previously arrived from the API and was dropped on the floor. --- .../(app)/projects/[projectId]/stories/[number]/page.tsx | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/apps/web/app/(app)/projects/[projectId]/stories/[number]/page.tsx b/apps/web/app/(app)/projects/[projectId]/stories/[number]/page.tsx index 43c432c9..aa386d80 100644 --- a/apps/web/app/(app)/projects/[projectId]/stories/[number]/page.tsx +++ b/apps/web/app/(app)/projects/[projectId]/stories/[number]/page.tsx @@ -9,7 +9,7 @@ import { PullRequestLinks } from "@/components/story/pull-request-links"; import { StoryTimeline } from "@/components/story/timeline"; import { StoryTriggerButtons } from "@/components/story/trigger-buttons"; import { api } from "@/lib/api"; -import { pipelineStories } from "@/lib/pipeline"; +import { pipelineStories, storyOwner } from "@/lib/pipeline"; import { detachablePullRequests, linkableIssues, @@ -102,6 +102,7 @@ export default async function StoryPage({ stageLabels, }); const stage = story.stage; + const owner = storyOwner(story.assignees); const prLinks = new Map(); for (const pr of story.prs) prLinks.set(pr.number, pr.url); @@ -165,6 +166,12 @@ export default async function StoryPage({ {label} ))} + {owner ? ( + + @{owner.login} + {owner.extra > 0 ? ` +${owner.extra}` : ""} + + ) : null} Date: Sun, 16 Aug 2026 16:17:25 -0300 Subject: [PATCH 03/10] feat(web): add a mine filter chip to the stories board Adds ownedBy() and boardHref() as pure helpers in lib/pipeline.ts, and uses boardHref for all four board filter chips (all, stage, status clear, mine) instead of hand-built URL strings. The mine chip narrows each stage's stories to the signed-in viewer's GitHub login, composes with the existing stage/status filters, and only renders when the viewer has a GitHub login to match against. Stage chip counts and the active-open-stories subtitle now read from the mine-scoped stories so they never go stale relative to what's shown. --- .../projects/[projectId]/stories/page.tsx | 45 ++++++++++++++----- apps/web/lib/pipeline.ts | 23 ++++++++++ apps/web/test/pipeline-story.test.ts | 40 ++++++++++++++++- 3 files changed, 97 insertions(+), 11 deletions(-) diff --git a/apps/web/app/(app)/projects/[projectId]/stories/page.tsx b/apps/web/app/(app)/projects/[projectId]/stories/page.tsx index 839a5b69..cf12e611 100644 --- a/apps/web/app/(app)/projects/[projectId]/stories/page.tsx +++ b/apps/web/app/(app)/projects/[projectId]/stories/page.tsx @@ -8,7 +8,7 @@ import { StageSection } from "@/components/project/stage-section"; import { LiveRefresh } from "@/components/shell/live-refresh"; import { api } from "@/lib/api"; import type { PipelineStageKey, PipelineStageKind, PipelineStageState } from "@/lib/pipeline"; -import { pipelineStageStateLabel, pipelineStories } from "@/lib/pipeline"; +import { boardHref, ownedBy, pipelineStageStateLabel, pipelineStories } from "@/lib/pipeline"; export const metadata = { title: "stories" }; @@ -35,9 +35,9 @@ export default async function ProjectStoriesPage({ searchParams, }: { params: Promise<{ projectId: string }>; - searchParams: Promise<{ stage?: string; status?: string }>; + searchParams: Promise<{ stage?: string; status?: string; mine?: string }>; }) { - const [{ projectId }, { stage, status }] = await Promise.all([params, searchParams]); + const [{ projectId }, { stage, status, mine }] = await Promise.all([params, searchParams]); const [pipelineResult, me] = await Promise.all([api.pipeline(projectId), api.me()]); if (!pipelineResult.ok && pipelineResult.offline) return ; @@ -45,18 +45,30 @@ export default async function ProjectStoriesPage({ const permissions = me.ok ? me.data.permissions : []; const canTrigger = hasPermission(permissions, "runs:trigger"); const canSync = hasPermission(permissions, "repos:write"); + const viewerLogin = me.ok ? me.data.principal.githubLogin : undefined; + const mineOn = mine === "1"; const stages = pipelineResult.ok ? pipelineResult.data.stages : []; const stageKeys = new Set(stages.map((candidate) => candidate.key)); const activeStage = stage && stageKeys.has(stage as PipelineStageKey) ? (stage as PipelineStageKey) : null; + // Validated against every story, not just the mine-scoped set, so a status filter + // never silently drops out of the URL when "mine" empties the board. const items = pipelineResult.ok ? pipelineStories(pipelineResult.data) : []; const stageStates = new Set(items.map((story) => story.stageState)); const activeStatus = activeStage && status && stageStates.has(status as PipelineStageState) ? (status as PipelineStageState) : null; - const counts = [...stages].reverse(); - const activeOpenStoryCount = items.filter((story) => story.state === "open").length; + const scoped = mineOn + ? stages.map((s) => ({ + ...s, + stories: s.stories.filter((story) => ownedBy(story.assignees, viewerLogin)), + })) + : stages; + const counts = [...scoped].reverse(); + const activeOpenStoryCount = scoped + .flatMap((s) => s.stories) + .filter((story) => story.state === "open").length; const stageFiltered = activeStage ? counts.filter((candidate) => candidate.key === activeStage) @@ -95,7 +107,7 @@ export default async function ProjectStoriesPage({
( 0 ? FILTER_COUNT_TONE[s.kind] : "text-(--dim)", + s.stories.length > 0 ? FILTER_COUNT_TONE[s.kind] : "text-(--dim)", )} > - {s.count} + {s.stories.length} ))} @@ -134,7 +146,7 @@ export default async function ProjectStoriesPage({ {activeStatusLabel} @@ -143,6 +155,19 @@ export default async function ProjectStoriesPage({ ) : null} + {viewerLogin ? ( + + mine + + ) : null}
{!pipelineResult.ok ? ( diff --git a/apps/web/lib/pipeline.ts b/apps/web/lib/pipeline.ts index 8d1f897a..8ade27f6 100644 --- a/apps/web/lib/pipeline.ts +++ b/apps/web/lib/pipeline.ts @@ -104,6 +104,29 @@ export function storyHref( return `/projects/${projectId}/stories/${story.number}?${storyQuery(story)}`; } +export type BoardFilter = { + stage?: PipelineStageKey | null; + status?: PipelineStageState | null; + mine?: boolean; +}; + +/** The stories board URL for a given combination of filter chips. */ +export function boardHref(projectId: string, filter: BoardFilter = {}) { + const params = new URLSearchParams(); + if (filter.stage) params.set("stage", filter.stage); + if (filter.status) params.set("status", filter.status); + if (filter.mine) params.set("mine", "1"); + const query = params.toString(); + return `/projects/${projectId}/stories${query ? `?${query}` : ""}`; +} + +/** Whether a story's assignees include the signed-in viewer, by GitHub login. */ +export function ownedBy(assignees: string[], login: string | undefined): boolean { + if (!login) return false; + const target = login.toLowerCase(); + return assignees.some((assignee) => assignee.toLowerCase() === target); +} + export type StoryOwner = { login: string; extra: number }; /** The story's lead assignee, GitHub-ordered, with a count of the rest. */ diff --git a/apps/web/test/pipeline-story.test.ts b/apps/web/test/pipeline-story.test.ts index de60a4ba..43b4ddf8 100644 --- a/apps/web/test/pipeline-story.test.ts +++ b/apps/web/test/pipeline-story.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it } from "vitest"; import { ciStatusLabel } from "@/components/ci-status"; import type { PipelineStageKey, PipelineStory, Proposal, StoryDetail } from "@/lib/api"; -import { reviewablePullRequests, storyHref, storyOwner } from "@/lib/pipeline"; +import { boardHref, ownedBy, reviewablePullRequests, storyHref, storyOwner } from "@/lib/pipeline"; import { deriveStoryTimeline, proposalsForStory } from "@/lib/story"; describe("story presentation contract", () => { @@ -311,6 +311,44 @@ describe("story presentation contract", () => { expect(reviewablePullRequests([story]).map(({ pull }) => pull.number)).toEqual([22]); }); + + it("never counts a story as owned when the viewer has no GitHub login", () => { + expect(ownedBy(["alice"], undefined)).toBe(false); + expect(ownedBy([], undefined)).toBe(false); + }); + + it("matches an assignee to the viewer's login regardless of case", () => { + expect(ownedBy(["Alice"], "alice")).toBe(true); + }); + + it("finds no owner in an empty assignee list", () => { + expect(ownedBy([], "alice")).toBe(false); + }); + + it("does not match an assignee who isn't the viewer", () => { + expect(ownedBy(["bob"], "alice")).toBe(false); + }); + + it("builds a mine-only board link with no other filters", () => { + expect(boardHref("project-1", { mine: true })).toBe("/projects/project-1/stories?mine=1"); + }); + + it("combines the stage and mine filters in one board link", () => { + expect(boardHref("project-1", { stage: "backlog", mine: true })).toBe( + "/projects/project-1/stories?stage=backlog&mine=1", + ); + }); + + it("omits the mine key entirely when mine is off", () => { + expect(boardHref("project-1", { mine: false })).toBe("/projects/project-1/stories"); + }); + + it("keeps mine on when the all chip clears the stage", () => { + expect(boardHref("project-1", { stage: "backlog", status: "ready_to_plan", mine: true })).toBe( + "/projects/project-1/stories?stage=backlog&status=ready_to_plan&mine=1", + ); + expect(boardHref("project-1", { mine: true })).toBe("/projects/project-1/stories?mine=1"); + }); }); function pipelinePull( From 7012ddc28fdf09c4bb4fa8360d5340e385adc07d Mon Sep 17 00:00:00 2001 From: guzmonne Date: Sun, 16 Aug 2026 16:20:15 -0300 Subject: [PATCH 04/10] fix(web): stop the mine filter from trapping a login-less viewer mineOn previously read straight from the mine=1 query param, so a viewer with no GitHub login (a key principal, or any user whose principal.githubLogin is unset) who arrived at ?mine=1 via a shared link, bookmark, or browser history landed on a board with every story filtered out by ownedBy(), no mine chip to undo it (it only renders when a login exists), and no other chip to recover with, since all four preserve mine. Lift the derivation into mineFilterOn(mine, login) in lib/pipeline.ts, which is false whenever the viewer has no login to match against regardless of the raw query param. The board now renders normally for such a viewer even with ?mine=1 in the URL, and every chip link emits a clean, mine-free href. --- .../projects/[projectId]/stories/page.tsx | 10 ++++++-- apps/web/lib/pipeline.ts | 11 ++++++++ apps/web/test/pipeline-story.test.ts | 25 ++++++++++++++++++- 3 files changed, 43 insertions(+), 3 deletions(-) diff --git a/apps/web/app/(app)/projects/[projectId]/stories/page.tsx b/apps/web/app/(app)/projects/[projectId]/stories/page.tsx index cf12e611..bf6c8d3b 100644 --- a/apps/web/app/(app)/projects/[projectId]/stories/page.tsx +++ b/apps/web/app/(app)/projects/[projectId]/stories/page.tsx @@ -8,7 +8,13 @@ import { StageSection } from "@/components/project/stage-section"; import { LiveRefresh } from "@/components/shell/live-refresh"; import { api } from "@/lib/api"; import type { PipelineStageKey, PipelineStageKind, PipelineStageState } from "@/lib/pipeline"; -import { boardHref, ownedBy, pipelineStageStateLabel, pipelineStories } from "@/lib/pipeline"; +import { + boardHref, + mineFilterOn, + ownedBy, + pipelineStageStateLabel, + pipelineStories, +} from "@/lib/pipeline"; export const metadata = { title: "stories" }; @@ -46,7 +52,7 @@ export default async function ProjectStoriesPage({ const canTrigger = hasPermission(permissions, "runs:trigger"); const canSync = hasPermission(permissions, "repos:write"); const viewerLogin = me.ok ? me.data.principal.githubLogin : undefined; - const mineOn = mine === "1"; + const mineOn = mineFilterOn(mine, viewerLogin); const stages = pipelineResult.ok ? pipelineResult.data.stages : []; const stageKeys = new Set(stages.map((candidate) => candidate.key)); const activeStage = diff --git a/apps/web/lib/pipeline.ts b/apps/web/lib/pipeline.ts index 8ade27f6..c6d44990 100644 --- a/apps/web/lib/pipeline.ts +++ b/apps/web/lib/pipeline.ts @@ -127,6 +127,17 @@ export function ownedBy(assignees: string[], login: string | undefined): boolean return assignees.some((assignee) => assignee.toLowerCase() === target); } +/** + * Whether the mine filter should actually apply. It is inert — never on — + * for a viewer with no GitHub login to match against, even if `?mine=1` + * is already sitting in the URL (a shared link, a bookmark, browser + * history), so such a viewer is never trapped on a board with every + * story filtered out and no chip left to undo it. + */ +export function mineFilterOn(mine: string | undefined, login: string | undefined): boolean { + return mine === "1" && Boolean(login); +} + export type StoryOwner = { login: string; extra: number }; /** The story's lead assignee, GitHub-ordered, with a count of the rest. */ diff --git a/apps/web/test/pipeline-story.test.ts b/apps/web/test/pipeline-story.test.ts index 43b4ddf8..01c49716 100644 --- a/apps/web/test/pipeline-story.test.ts +++ b/apps/web/test/pipeline-story.test.ts @@ -1,7 +1,14 @@ import { describe, expect, it } from "vitest"; import { ciStatusLabel } from "@/components/ci-status"; import type { PipelineStageKey, PipelineStory, Proposal, StoryDetail } from "@/lib/api"; -import { boardHref, ownedBy, reviewablePullRequests, storyHref, storyOwner } from "@/lib/pipeline"; +import { + boardHref, + mineFilterOn, + ownedBy, + reviewablePullRequests, + storyHref, + storyOwner, +} from "@/lib/pipeline"; import { deriveStoryTimeline, proposalsForStory } from "@/lib/story"; describe("story presentation contract", () => { @@ -349,6 +356,22 @@ describe("story presentation contract", () => { ); expect(boardHref("project-1", { mine: true })).toBe("/projects/project-1/stories?mine=1"); }); + + it("turns the mine filter on only when the viewer has a GitHub login to match against", () => { + expect(mineFilterOn("1", "alice")).toBe(true); + expect(mineFilterOn("1", undefined)).toBe(false); + expect(mineFilterOn(undefined, "alice")).toBe(false); + expect(mineFilterOn(undefined, undefined)).toBe(false); + }); + + it("recovers a login-less viewer who arrives with ?mine=1 already in the URL", () => { + // A shared link, bookmark, or browser history can carry `mine=1` for a + // viewer with no GitHub login. The derived flag must stay off so the + // board renders normally and the all chip offers a clean way out. + const mineOn = mineFilterOn("1", undefined); + expect(mineOn).toBe(false); + expect(boardHref("project-1", { mine: mineOn })).toBe("/projects/project-1/stories"); + }); }); function pipelinePull( From 6c21a71336b1a0f0eebcca45aae336f63d78d683 Mon Sep 17 00:00:00 2001 From: guzmonne Date: Tue, 25 Aug 2026 15:52:45 -0300 Subject: [PATCH 05/10] fix(web): surface a failed /v1/me instead of silently dropping the mine filter A failed identity request used to be collapsed into 'viewer has no GitHub login', so ?mine=1 showed every story while removing the chip that could undo it. mineFilterState now returns off/on/blocked so the board can say the identity check failed rather than pretend the filter found nothing. Also adds the empty state the reviewer asked for: when the mine filter is active and nothing is assigned to the viewer, the board says so and links back to the unfiltered board instead of rendering every stage empty. --- .../projects/[projectId]/stories/page.tsx | 130 +++++++++++------- apps/web/lib/pipeline.ts | 34 ++++- apps/web/test/pipeline-story.test.ts | 41 ++++-- 3 files changed, 140 insertions(+), 65 deletions(-) diff --git a/apps/web/app/(app)/projects/[projectId]/stories/page.tsx b/apps/web/app/(app)/projects/[projectId]/stories/page.tsx index bf6c8d3b..22233ab9 100644 --- a/apps/web/app/(app)/projects/[projectId]/stories/page.tsx +++ b/apps/web/app/(app)/projects/[projectId]/stories/page.tsx @@ -10,7 +10,8 @@ import { api } from "@/lib/api"; import type { PipelineStageKey, PipelineStageKind, PipelineStageState } from "@/lib/pipeline"; import { boardHref, - mineFilterOn, + type MeOutcome, + mineFilterState, ownedBy, pipelineStageStateLabel, pipelineStories, @@ -44,15 +45,21 @@ export default async function ProjectStoriesPage({ searchParams: Promise<{ stage?: string; status?: string; mine?: string }>; }) { const [{ projectId }, { stage, status, mine }] = await Promise.all([params, searchParams]); - const [pipelineResult, me] = await Promise.all([api.pipeline(projectId), api.me()]); + const [pipelineResult, meResult] = await Promise.all([api.pipeline(projectId), api.me()]); if (!pipelineResult.ok && pipelineResult.offline) return ; - const permissions = me.ok ? me.data.permissions : []; + // The identity request is allowed to fail independently of the pipeline: a + // partial failure must not silently downgrade `?mine=1` into "show + // everything", so the outcome — not just the login — feeds the filter. + const me: MeOutcome = meResult.ok + ? { ok: true, githubLogin: meResult.data.principal.githubLogin } + : { ok: false, message: meResult.message }; + const permissions = me.ok && meResult.ok ? meResult.data.permissions : []; const canTrigger = hasPermission(permissions, "runs:trigger"); const canSync = hasPermission(permissions, "repos:write"); - const viewerLogin = me.ok ? me.data.principal.githubLogin : undefined; - const mineOn = mineFilterOn(mine, viewerLogin); + const viewerLogin = me.ok ? me.githubLogin : undefined; + const mineState = mineFilterState(mine, me); const stages = pipelineResult.ok ? pipelineResult.data.stages : []; const stageKeys = new Set(stages.map((candidate) => candidate.key)); const activeStage = @@ -65,13 +72,16 @@ export default async function ProjectStoriesPage({ activeStage && status && stageStates.has(status as PipelineStageState) ? (status as PipelineStageState) : null; - const scoped = mineOn - ? stages.map((s) => ({ - ...s, - stories: s.stories.filter((story) => ownedBy(story.assignees, viewerLogin)), - })) - : stages; + const scoped = + mineState.kind === "on" + ? stages.map((s) => ({ + ...s, + stories: s.stories.filter((story) => ownedBy(story.assignees, mineState.login)), + })) + : stages; + const mineOn = mineState.kind === "on"; const counts = [...scoped].reverse(); + const scopedTotal = scoped.reduce((total, s) => total + s.stories.length, 0); const activeOpenStoryCount = scoped .flatMap((s) => s.stories) .filter((story) => story.state === "open").length; @@ -94,6 +104,47 @@ export default async function ProjectStoriesPage({ ) : null; + const boardBody = () => ( +
+ {visibleStages.map((s) => { + const stageItems = s.stories; + return ( + story.runState === "live").length} + failedCount={ + stageItems.filter( + (story) => story.runState === "failed" || story.ciState === "failure", + ).length + } + defaultOpen={activeStage !== null || s.key !== "shipped"} + > + {stageItems.length === 0 ? ( +

+ Nothing here right now. +

+ ) : ( +
+ {stageItems.map((story) => ( + + ))} +
+ )} + + ); + })} +
+ ); + return (
@@ -184,50 +235,31 @@ export default async function ProjectStoriesPage({ : `Couldn't load stories — ${pipelineResult.message}` } /> + ) : mineState.kind === "blocked" ? ( + + ) : mineState.kind === "on" && items.length > 0 && scopedTotal === 0 ? ( +
+

+ Nothing is assigned to{" "} + @{mineState.login} right now. Stories + you're assigned to in GitHub will appear here after the next sync. +

+ + show all stories + +
) : items.length === 0 ? (

No active stories right now. Closed and merged stories leave Shipped after seven days; sync refreshes the GitHub mirror.

) : ( -
- {visibleStages.map((s) => { - const stageItems = s.stories; - return ( - story.runState === "live").length} - failedCount={ - stageItems.filter( - (story) => story.runState === "failed" || story.ciState === "failure", - ).length - } - defaultOpen={activeStage !== null || s.key !== "shipped"} - > - {stageItems.length === 0 ? ( -

- Nothing here right now. -

- ) : ( -
- {stageItems.map((story) => ( - - ))} -
- )} - - ); - })} -
+ boardBody() )}
); diff --git a/apps/web/lib/pipeline.ts b/apps/web/lib/pipeline.ts index c6d44990..02fa48bf 100644 --- a/apps/web/lib/pipeline.ts +++ b/apps/web/lib/pipeline.ts @@ -127,15 +127,35 @@ export function ownedBy(assignees: string[], login: string | undefined): boolean return assignees.some((assignee) => assignee.toLowerCase() === target); } +/** What the control plane said about the signed-in viewer. */ +export type MeOutcome = + | { ok: true; githubLogin: string | undefined } + | { ok: false; message: string }; + /** - * Whether the mine filter should actually apply. It is inert — never on — - * for a viewer with no GitHub login to match against, even if `?mine=1` - * is already sitting in the URL (a shared link, a bookmark, browser - * history), so such a viewer is never trapped on a board with every - * story filtered out and no chip left to undo it. + * Why the mine filter is or isn't applied: + * + * - `off` — not requested, or requested by a viewer with no GitHub login to + * match against (a shared link, a bookmark, browser history). Such a viewer + * is never trapped on a board with every story filtered out and no chip + * left to undo it. + * - `on` — requested and matchable; `login` is the identity to match. + * - `blocked` — requested, but the `/v1/me` request itself failed. This is + * kept distinct from `off` on purpose: silently showing the unfiltered + * board would read as "the filter found nothing", and dropping the + * parameter from every chip URL would erase the reader's intent. The board + * must say the identity check failed instead. */ -export function mineFilterOn(mine: string | undefined, login: string | undefined): boolean { - return mine === "1" && Boolean(login); +export type MineFilterState = + | { kind: "off" } + | { kind: "on"; login: string } + | { kind: "blocked"; reason: string }; + +export function mineFilterState(requested: string | undefined, me: MeOutcome): MineFilterState { + if (requested !== "1") return { kind: "off" }; + if (!me.ok) return { kind: "blocked", reason: me.message }; + if (!me.githubLogin) return { kind: "off" }; + return { kind: "on", login: me.githubLogin }; } export type StoryOwner = { login: string; extra: number }; diff --git a/apps/web/test/pipeline-story.test.ts b/apps/web/test/pipeline-story.test.ts index 01c49716..904911ba 100644 --- a/apps/web/test/pipeline-story.test.ts +++ b/apps/web/test/pipeline-story.test.ts @@ -3,7 +3,7 @@ import { ciStatusLabel } from "@/components/ci-status"; import type { PipelineStageKey, PipelineStory, Proposal, StoryDetail } from "@/lib/api"; import { boardHref, - mineFilterOn, + mineFilterState, ownedBy, reviewablePullRequests, storyHref, @@ -358,19 +358,42 @@ describe("story presentation contract", () => { }); it("turns the mine filter on only when the viewer has a GitHub login to match against", () => { - expect(mineFilterOn("1", "alice")).toBe(true); - expect(mineFilterOn("1", undefined)).toBe(false); - expect(mineFilterOn(undefined, "alice")).toBe(false); - expect(mineFilterOn(undefined, undefined)).toBe(false); + expect(mineFilterState("1", { ok: true, githubLogin: "alice" })).toEqual({ + kind: "on", + login: "alice", + }); + expect(mineFilterState("1", { ok: true, githubLogin: undefined })).toEqual({ kind: "off" }); + expect(mineFilterState(undefined, { ok: true, githubLogin: "alice" })).toEqual({ kind: "off" }); + expect(mineFilterState(undefined, { ok: true, githubLogin: undefined })).toEqual({ + kind: "off", + }); }); it("recovers a login-less viewer who arrives with ?mine=1 already in the URL", () => { // A shared link, bookmark, or browser history can carry `mine=1` for a - // viewer with no GitHub login. The derived flag must stay off so the + // viewer with no GitHub identity. The derived state must stay off so the // board renders normally and the all chip offers a clean way out. - const mineOn = mineFilterOn("1", undefined); - expect(mineOn).toBe(false); - expect(boardHref("project-1", { mine: mineOn })).toBe("/projects/project-1/stories"); + const state = mineFilterState("1", { ok: true, githubLogin: undefined }); + expect(state).toEqual({ kind: "off" }); + expect(boardHref("project-1", { mine: state.kind === "on" })).toBe( + "/projects/project-1/stories", + ); + }); + + it("reports a failed /v1/me as blocked rather than as a filter that found nothing", () => { + // Regression: a partial failure (pipeline loads, identity request fails) + // used to be collapsed into "no login", which silently showed every story + // under `?mine=1` while removing the chip that could undo it. The board + // must surface the failure instead. + const state = mineFilterState("1", { ok: false, message: "identity lookup timed out" }); + expect(state).toEqual({ kind: "blocked", reason: "identity lookup timed out" }); + expect(boardHref("project-1", { mine: state.kind === "on" })).toBe( + "/projects/project-1/stories", + ); + }); + + it("keeps an unrequested mine filter off even when the identity request failed", () => { + expect(mineFilterState(undefined, { ok: false, message: "down" })).toEqual({ kind: "off" }); }); }); From 7de325009570d1f78182422ae1d43a2496a9c060 Mon Sep 17 00:00:00 2001 From: guzmonne Date: Mon, 17 Aug 2026 10:44:24 -0300 Subject: [PATCH 06/10] feat(web): show assignee avatars on the story row The board named the assignee in text but did not show them. Add a shared Avatar primitive and use it on the story row, immediately left of @login. The image is painted as a CSS background rather than as an . An that fails to load makes every browser draw its own broken-image glyph over the letter beneath it, and alt="" does not suppress it; a background that fails to load paints nothing. So a deployment whose browsers cannot reach github.com falls back to the initial letter on its own, with nothing to configure. The avatar URL and the fallback letter are derived in apps/web, not in packages/ui, which keeps the primitive free of any knowledge of GitHub and puts the rules where there is a test runner to pin them. Refs theam/facility#174 --- apps/web/components/issues/issue-row.tsx | 17 +++++--- apps/web/lib/pipeline.ts | 20 +++++++++ apps/web/test/pipeline-story.test.ts | 39 +++++++++++++++++ packages/ui/src/avatar.tsx | 55 ++++++++++++++++++++++++ packages/ui/src/index.ts | 1 + 5 files changed, 127 insertions(+), 5 deletions(-) create mode 100644 packages/ui/src/avatar.tsx diff --git a/apps/web/components/issues/issue-row.tsx b/apps/web/components/issues/issue-row.tsx index 4c7029b9..5bc3ecb6 100644 --- a/apps/web/components/issues/issue-row.tsx +++ b/apps/web/components/issues/issue-row.tsx @@ -1,12 +1,12 @@ "use client"; -import { Button, ButtonLink, StatusDot, toneFor } from "@facility/ui"; +import { Avatar, Button, ButtonLink, StatusDot, toneFor } from "@facility/ui"; import Link from "next/link"; import { useRouter } from "next/navigation"; import { useState } from "react"; import { CiStatusLink } from "@/components/ci-status"; import type { PipelineStory } from "@/lib/pipeline"; -import { storyHref, storyOwner } from "@/lib/pipeline"; +import { avatarInitial, avatarUrlFor, storyHref, storyOwner } from "@/lib/pipeline"; function fmtAgo(iso: string | null) { if (!iso) return "—"; @@ -196,9 +196,16 @@ export function IssueRow({ ))} {owner ? ( - - @{owner.login} - {owner.extra > 0 ? ` +${owner.extra}` : ""} + + + + @{owner.login} + {owner.extra > 0 ? ` +${owner.extra}` : ""} + ) : null} {fmtAgo(story.ghUpdatedAt)} diff --git a/apps/web/lib/pipeline.ts b/apps/web/lib/pipeline.ts index 02fa48bf..86916f6e 100644 --- a/apps/web/lib/pipeline.ts +++ b/apps/web/lib/pipeline.ts @@ -168,6 +168,26 @@ export function storyOwner(assignees: string[]): StoryOwner | null { return { login, extra: logins.length - 1 }; } +/** + * GitHub serves an avatar for any login at this path, so no avatar URL has to + * travel on the wire. It 302s to `avatars.githubusercontent.com`. + * + * `?size=40` rather than the 14–20 CSS px we draw at, so 2× displays stay sharp. + */ +export function avatarUrlFor(login: string): string | null { + const trimmed = login.trim(); + if (!trimmed) return null; + return `https://github.com/${encodeURIComponent(trimmed)}.png?size=40`; +} + +/** The letter an avatar falls back to when there is no image to draw. */ +export function avatarInitial(value: string | null | undefined): string { + const trimmed = (value ?? "").trim(); + // Spread, not `[0]`, so an astral first character survives intact. + const [first] = [...trimmed]; + return first ? first.toUpperCase() : "?"; +} + export function pipelineStories(pipeline: Pipeline): PipelineStory[] { return pipeline.stages.flatMap((stage) => stage.stories); } diff --git a/apps/web/test/pipeline-story.test.ts b/apps/web/test/pipeline-story.test.ts index 904911ba..eb974502 100644 --- a/apps/web/test/pipeline-story.test.ts +++ b/apps/web/test/pipeline-story.test.ts @@ -2,6 +2,8 @@ import { describe, expect, it } from "vitest"; import { ciStatusLabel } from "@/components/ci-status"; import type { PipelineStageKey, PipelineStory, Proposal, StoryDetail } from "@/lib/api"; import { + avatarInitial, + avatarUrlFor, boardHref, mineFilterState, ownedBy, @@ -308,6 +310,43 @@ describe("story presentation contract", () => { expect(storyOwner([" a "])).toEqual({ login: "a", extra: 0 }); }); + it("builds a GitHub avatar URL from a login, at twice the drawn size", () => { + expect(avatarUrlFor("octocat")).toBe("https://github.com/octocat.png?size=40"); + }); + + it("trims a login before building its avatar URL", () => { + expect(avatarUrlFor(" octocat ")).toBe("https://github.com/octocat.png?size=40"); + }); + + it("escapes a login rather than letting it shape the avatar URL", () => { + expect(avatarUrlFor("a/b?c")).toBe("https://github.com/a%2Fb%3Fc.png?size=40"); + }); + + it("has no avatar URL to offer for a blank login", () => { + expect(avatarUrlFor("")).toBeNull(); + expect(avatarUrlFor(" ")).toBeNull(); + }); + + it("falls back to the first letter of a login, uppercased", () => { + expect(avatarInitial("octocat")).toBe("O"); + expect(avatarInitial("Octocat")).toBe("O"); + }); + + it("falls back to the first letter of an email when there is no login", () => { + expect(avatarInitial("ada@example.test")).toBe("A"); + }); + + it("keeps an astral first character whole in the fallback", () => { + expect(avatarInitial("😀nn")).toBe("😀"); + }); + + it("shows a question mark rather than an empty box when there is nothing to draw", () => { + expect(avatarInitial("")).toBe("?"); + expect(avatarInitial(" ")).toBe("?"); + expect(avatarInitial(null)).toBe("?"); + expect(avatarInitial(undefined)).toBe("?"); + }); + it("does not count draft pull requests as waiting for human review", () => { const story = storyDetail(); story.prs = [ diff --git a/packages/ui/src/avatar.tsx b/packages/ui/src/avatar.tsx new file mode 100644 index 00000000..a7d1bd10 --- /dev/null +++ b/packages/ui/src/avatar.tsx @@ -0,0 +1,55 @@ +import { cx } from "./cx"; + +/** `"` and `\` would end the CSS string early; whitespace would end the url() token. */ +function cssUrl(src: string): string { + return `url("${src.replace(/["\\\s]/g, encodeURIComponent)}")`; +} + +/** + * Square avatar with an initial-letter fallback underneath the image. + * + * The image is painted as a CSS background rather than as an `` on + * purpose. An `` that fails to load makes every browser draw its own + * broken-image glyph over the letter — `alt=""` does not suppress it — while a + * background that fails to load paints nothing at all. So a deployment whose + * browsers cannot reach the image host degrades to the letter on its own, with + * nothing to configure. + * + * Decorative: the login it stands for is always written out beside it. + */ +export function Avatar({ + src, + initial, + size, + className, +}: { + src?: string; + initial: string; + size: number; + className?: string; +}) { + return ( + + {initial} + {src ? ( + + ) : null} + + ); +} diff --git a/packages/ui/src/index.ts b/packages/ui/src/index.ts index ad740f9d..3bce0af7 100644 --- a/packages/ui/src/index.ts +++ b/packages/ui/src/index.ts @@ -1,3 +1,4 @@ +export { Avatar } from "./avatar"; export { Button, ButtonLink } from "./button"; export { cx } from "./cx"; export { Field, Select, TextArea, TextInput } from "./field"; From 91fa980af01a217ff7e6ac1de37585c8d38c385d Mon Sep 17 00:00:00 2001 From: guzmonne Date: Mon, 17 Aug 2026 10:44:48 -0300 Subject: [PATCH 07/10] feat(web): show the assignee avatar in the story header The same primitive as the story row, at 16px to sit with the header's larger type. The header is a server component and the row is a client component, so this is also what proves one primitive serves both. Refs theam/facility#174 --- .../[projectId]/stories/[number]/page.tsx | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/apps/web/app/(app)/projects/[projectId]/stories/[number]/page.tsx b/apps/web/app/(app)/projects/[projectId]/stories/[number]/page.tsx index aa386d80..ee87c7cf 100644 --- a/apps/web/app/(app)/projects/[projectId]/stories/[number]/page.tsx +++ b/apps/web/app/(app)/projects/[projectId]/stories/[number]/page.tsx @@ -1,4 +1,4 @@ -import { Eyebrow, PillTag, StatusDot } from "@facility/ui"; +import { Avatar, Eyebrow, PillTag, StatusDot } from "@facility/ui"; import Link from "next/link"; import { notFound } from "next/navigation"; import { CiStatusLink } from "@/components/ci-status"; @@ -9,7 +9,7 @@ import { PullRequestLinks } from "@/components/story/pull-request-links"; import { StoryTimeline } from "@/components/story/timeline"; import { StoryTriggerButtons } from "@/components/story/trigger-buttons"; import { api } from "@/lib/api"; -import { pipelineStories, storyOwner } from "@/lib/pipeline"; +import { avatarInitial, avatarUrlFor, pipelineStories, storyOwner } from "@/lib/pipeline"; import { detachablePullRequests, linkableIssues, @@ -167,9 +167,16 @@ export default async function StoryPage({ ))} {owner ? ( - - @{owner.login} - {owner.extra > 0 ? ` +${owner.extra}` : ""} + + + + @{owner.login} + {owner.extra > 0 ? ` +${owner.extra}` : ""} + ) : null} Date: Mon, 17 Aug 2026 10:45:33 -0300 Subject: [PATCH 08/10] feat(web): give the topbar an avatar fallback The topbar rendered an image when the principal had an avatar URL and nothing at all when it did not, so a user whose GitHub identity has no avatar saw an empty space. It now uses the same primitive as the board and falls back to the initial letter of the login, or of the email when there is no login. Two visible consequences, both deliberate: - The avatar is square rather than round. PillTag is documented as the only pill-shaped element in the design system, and the board avatars are square, so one shape now serves all three sites. - The image is no longer an , so it can no longer carry referrerPolicy="no-referrer". Referrer policy belongs to the fetch initiator and CSS cannot set one; measured across Chromium, Firefox and WebKit, a pseudo-element, a child element, an inline style, an external stylesheet carrying referrerpolicy, and a custom-property indirection all send the origin. The avatar host therefore now learns the deployment's origin. The only alternatives are document-wide and would change every other request the app makes. Refs theam/facility#174 --- apps/web/components/shell/topbar.tsx | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/apps/web/components/shell/topbar.tsx b/apps/web/components/shell/topbar.tsx index 1d9e2fb8..a539bbae 100644 --- a/apps/web/components/shell/topbar.tsx +++ b/apps/web/components/shell/topbar.tsx @@ -1,8 +1,8 @@ -import { PillTag } from "@facility/ui"; -import Image from "next/image"; +import { Avatar, PillTag } from "@facility/ui"; import { SignOutButton } from "@/components/shell/sign-out"; import { ProjectSwitcher } from "@/components/shell/switcher"; import type { Me, Project } from "@/lib/api"; +import { avatarInitial } from "@/lib/pipeline"; export function Topbar({ me, @@ -26,17 +26,11 @@ export function Topbar({ className="flex items-center gap-2 font-mono text-[11px] text-(--dim)" title={me.principal.email} > - {me.principal.avatarUrl ? ( - - ) : null} + {me.principal.githubLogin ? `@${me.principal.githubLogin}` : me.principal.email} From e36412decbfc6533f9038dc956b32208341b8ef0 Mon Sep 17 00:00:00 2001 From: guzmonne Date: Tue, 25 Aug 2026 15:49:46 -0300 Subject: [PATCH 09/10] fix(web): serve avatars from the deployment's own origin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Browsers no longer fetch avatars from GitHub. Images are proxied through new /api/avatars/u/{login} and /api/avatars/id/{id} routes, whose server- side fetch carries fresh headers — no cookies, forwarding chain, or referrer — so the deployment origin never reaches GitHub with avatar lookups, restoring the guarantee the first cut lost when it dropped referrerPolicy="no-referrer". An operator can set NEXT_PUBLIC_FACILITY_AVATARS=off to draw initial letters only; unknown values fail closed to off. There is deliberately no browser-direct mode left: it required third-party egress that air-gapped deployments must not make. The route serves only two exact path shapes pinned to two GitHub hosts, and maps every upstream failure to 404, which leaves the CSS background unset and the initial letter showing. Stored principal avatar URLs are rewritten onto the proxy only when they match a known GitHub shape; anything else falls back to the login-derived source so the browser is never pointed at an unreviewed host. --- .env.example | 9 + apps/docs/docs/self-host/production.md | 5 + .../[projectId]/stories/[number]/page.tsx | 5 +- apps/web/app/api/avatars/[...target]/route.ts | 54 ++++++ apps/web/components/issues/issue-row.tsx | 5 +- apps/web/components/shell/topbar.tsx | 3 +- apps/web/lib/avatar-policy.ts | 84 +++++++++ apps/web/lib/avatar-proxy.ts | 58 ++++++ apps/web/lib/pipeline.ts | 12 +- apps/web/test/avatar-component.test.tsx | 40 +++++ apps/web/test/avatar-policy.test.ts | 91 ++++++++++ apps/web/test/avatar-proxy.test.ts | 169 ++++++++++++++++++ apps/web/test/pipeline-story.test.ts | 18 -- 13 files changed, 519 insertions(+), 34 deletions(-) create mode 100644 apps/web/app/api/avatars/[...target]/route.ts create mode 100644 apps/web/lib/avatar-policy.ts create mode 100644 apps/web/lib/avatar-proxy.ts create mode 100644 apps/web/test/avatar-component.test.tsx create mode 100644 apps/web/test/avatar-policy.test.ts create mode 100644 apps/web/test/avatar-proxy.test.ts diff --git a/.env.example b/.env.example index 20481563..8032086f 100644 --- a/.env.example +++ b/.env.example @@ -1,6 +1,15 @@ # Facility platform — development environment # Copy to .env and adjust. NEVER commit .env. +# --- Web (apps/web) --- +# Avatar delivery for assignees and the signed-in topbar. "proxy" (default) +# serves every avatar from the web app's own /api/avatars routes: browsers +# never contact GitHub, and no referrer or origin leaks to it. "off" draws +# initial letters only — zero remote requests. There is no browser-direct +# mode; deployments that must not reach GitHub at all can additionally +# firewall the server's egress, and avatars degrade to letters. +NEXT_PUBLIC_FACILITY_AVATARS=proxy + # --- Core --- DATABASE_URL=postgres://facility:facility@localhost:5461/facility # 32 bytes, base64. Generate: openssl rand -base64 32 diff --git a/apps/docs/docs/self-host/production.md b/apps/docs/docs/self-host/production.md index 4de7433c..61f5cbb9 100644 --- a/apps/docs/docs/self-host/production.md +++ b/apps/docs/docs/self-host/production.md @@ -160,6 +160,11 @@ order, and end-to-end verification. - Gateway reachable from sandboxes and CI only (it holds no read endpoints, but it is the money path). - Sandboxes on an isolated network segment; egress per profile. +- Avatars never load from third-party hosts in the browser: they are proxied + through this deployment's `/api/avatars` routes, and the server-side fetch + carries no cookies or referrer. Set `NEXT_PUBLIC_FACILITY_AVATARS=off` to + draw initial letters only; a deployment whose server egress to GitHub is + firewalled degrades to letters on its own. - Backups: Postgres PITR + object-store lifecycle; audit retention per your compliance window. - Keep `node packages/cli/bin/facility.mjs doctor --url https:// --key diff --git a/apps/web/app/(app)/projects/[projectId]/stories/[number]/page.tsx b/apps/web/app/(app)/projects/[projectId]/stories/[number]/page.tsx index ee87c7cf..00b88c75 100644 --- a/apps/web/app/(app)/projects/[projectId]/stories/[number]/page.tsx +++ b/apps/web/app/(app)/projects/[projectId]/stories/[number]/page.tsx @@ -9,7 +9,8 @@ import { PullRequestLinks } from "@/components/story/pull-request-links"; import { StoryTimeline } from "@/components/story/timeline"; import { StoryTriggerButtons } from "@/components/story/trigger-buttons"; import { api } from "@/lib/api"; -import { avatarInitial, avatarUrlFor, pipelineStories, storyOwner } from "@/lib/pipeline"; +import { avatarSrcFor } from "@/lib/avatar-policy"; +import { avatarInitial, pipelineStories, storyOwner } from "@/lib/pipeline"; import { detachablePullRequests, linkableIssues, @@ -170,7 +171,7 @@ export default async function StoryPage({ diff --git a/apps/web/app/api/avatars/[...target]/route.ts b/apps/web/app/api/avatars/[...target]/route.ts new file mode 100644 index 00000000..21726bff --- /dev/null +++ b/apps/web/app/api/avatars/[...target]/route.ts @@ -0,0 +1,54 @@ +import { avatarMode } from "@/lib/avatar-policy"; +import { + avatarUpstreamHeaders, + avatarUpstreamUrl, + isForwardableAvatarResponse, + parseAvatarTarget, +} from "@/lib/avatar-proxy"; + +export const dynamic = "force-dynamic"; + +/** + * Same-origin avatar images: `/api/avatars/u/{login}` and + * `/api/avatars/id/{id}`. The browser never contacts GitHub; this route + * fetches server-side with fresh, referrer-free headers and forwards only + * successful image bytes. Any other path, an invalid target, a disabled + * avatar mode, or an upstream failure maps to 404 — which leaves the + * caller's CSS background unset and its initial letter showing. + */ +export async function GET( + _request: Request, + { params }: { params: Promise<{ target: string[] }> }, +) { + if (avatarMode(process.env.NEXT_PUBLIC_FACILITY_AVATARS) === "off") { + return new Response(null, { status: 404 }); + } + + const { target: segments } = await params; + const target = parseAvatarTarget(segments ?? []); + if (!target) return new Response(null, { status: 404 }); + + try { + const upstream = await fetch(avatarUpstreamUrl(target), { + headers: avatarUpstreamHeaders(), + redirect: "follow", + signal: AbortSignal.timeout(5_000), + cache: "no-store", + }); + if (!isForwardableAvatarResponse(upstream)) return new Response(null, { status: 404 }); + + return new Response(upstream.body, { + status: 200, + headers: { + "content-type": upstream.headers.get("content-type") ?? "image/png", + // Avatars change rarely; let the browser and any shared cache keep + // one for a day, and revalidate against this route afterwards. + "cache-control": "private, max-age=86400", + "content-security-policy": "default-src 'none'; sandbox", + "x-content-type-options": "nosniff", + }, + }); + } catch { + return new Response(null, { status: 404 }); + } +} diff --git a/apps/web/components/issues/issue-row.tsx b/apps/web/components/issues/issue-row.tsx index 5bc3ecb6..2d3fc43e 100644 --- a/apps/web/components/issues/issue-row.tsx +++ b/apps/web/components/issues/issue-row.tsx @@ -5,8 +5,9 @@ import Link from "next/link"; import { useRouter } from "next/navigation"; import { useState } from "react"; import { CiStatusLink } from "@/components/ci-status"; +import { avatarSrcFor } from "@/lib/avatar-policy"; import type { PipelineStory } from "@/lib/pipeline"; -import { avatarInitial, avatarUrlFor, storyHref, storyOwner } from "@/lib/pipeline"; +import { avatarInitial, storyHref, storyOwner } from "@/lib/pipeline"; function fmtAgo(iso: string | null) { if (!iso) return "—"; @@ -199,7 +200,7 @@ export function IssueRow({ diff --git a/apps/web/components/shell/topbar.tsx b/apps/web/components/shell/topbar.tsx index a539bbae..bc5f1aeb 100644 --- a/apps/web/components/shell/topbar.tsx +++ b/apps/web/components/shell/topbar.tsx @@ -2,6 +2,7 @@ import { Avatar, PillTag } from "@facility/ui"; import { SignOutButton } from "@/components/shell/sign-out"; import { ProjectSwitcher } from "@/components/shell/switcher"; import type { Me, Project } from "@/lib/api"; +import { principalAvatarSrc } from "@/lib/avatar-policy"; import { avatarInitial } from "@/lib/pipeline"; export function Topbar({ @@ -28,7 +29,7 @@ export function Topbar({ > {me.principal.githubLogin ? `@${me.principal.githubLogin}` : me.principal.email} diff --git a/apps/web/lib/avatar-policy.ts b/apps/web/lib/avatar-policy.ts new file mode 100644 index 00000000..68668913 --- /dev/null +++ b/apps/web/lib/avatar-policy.ts @@ -0,0 +1,84 @@ +/** + * Avatar delivery policy, shared by every call site and pinned by tests. + * + * Browsers never contact an avatar host. Images travel through this + * deployment's own `/api/avatars/…` routes (mode "proxy"), or do not exist + * and the initial letter underneath shows instead (mode "off"). The + * browser-direct mode the first cut of this feature shipped is gone: it + * leaked the deployment origin to GitHub through the referrer and required + * third-party egress that air-gapped deployments must not make. + */ + +export type AvatarMode = "proxy" | "off"; + +export const AVATAR_MODE_ENV = "NEXT_PUBLIC_FACILITY_AVATARS"; + +/** Read the operator's avatar mode. Unknown values fail closed to "off". */ +export function avatarMode(envValue: string | undefined | null): AvatarMode { + const value = (envValue ?? "").trim().toLowerCase(); + // Default: same-origin proxying. Nothing leaves the deployment's origin + // from the browser. + if (!value || value === "proxy") return "proxy"; + return "off"; +} + +function currentMode(): AvatarMode { + // NEXT_PUBLIC_* is inlined at build time, so client components can read it. + return avatarMode(process.env[AVATAR_MODE_ENV]); +} + +/** GitHub logins: alphanumerics and inner hyphens, at most 39 characters. */ +const GITHUB_LOGIN = /^[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,37}[a-zA-Z0-9])?$/; + +/** + * The image source for an assignee's avatar, served by this deployment, or + * null when there is nothing to draw (blank login, or avatars disabled). + * Null keeps the CSS background from ever being set, so the initial letter + * underneath shows through untouched. + */ +export function avatarSrcFor(login: string, mode?: AvatarMode): string | null { + const trimmed = login.trim(); + if (!trimmed) return null; + if ((mode ?? currentMode()) === "off") return null; + return `/api/avatars/u/${encodeURIComponent(trimmed)}`; +} + +/** + * The image source for a principal's stored avatar URL, rewritten onto this + * deployment's proxy routes. Only known GitHub avatar shapes are rewritten + * (`github.com/{login}.png`, `avatars.githubusercontent.com/u/{id}`); any + * other host falls back to the login-based source so the browser is never + * pointed at an unreviewed origin. Null when there is nothing to draw. + */ +export function principalAvatarSrc( + avatarUrl: string | null | undefined, + login: string | null | undefined, + mode?: AvatarMode, +): string | null { + const effective = mode ?? currentMode(); + if (effective === "off") return null; + + if (avatarUrl) { + try { + const url = new URL(avatarUrl); + if ( + url.protocol === "https:" && + url.host === "avatars.githubusercontent.com" && + /^\/u\/\d{1,12}$/.test(url.pathname) + ) { + return `/api/avatars/id/${url.pathname.slice("/u/".length)}`; + } + if ( + url.protocol === "https:" && + url.host === "github.com" && + GITHUB_LOGIN.test(url.pathname.slice(1).replace(/\.png$/, "")) && + /^\/[^/]+\.png$/.test(url.pathname) + ) { + return `/api/avatars/u/${url.pathname.slice(1).replace(/\.png$/, "")}`; + } + } catch { + // Not a URL at all — fall through to the login-derived source. + } + } + return login ? avatarSrcFor(login, effective) : null; +} diff --git a/apps/web/lib/avatar-proxy.ts b/apps/web/lib/avatar-proxy.ts new file mode 100644 index 00000000..d528efb1 --- /dev/null +++ b/apps/web/lib/avatar-proxy.ts @@ -0,0 +1,58 @@ +/** + * Server-side policy for `/api/avatars/...` routes. + * + * The browser only ever talks to this deployment. These functions decide, + * on the server, which upstream hosts may be fetched and with what headers: + * targets are pinned to two GitHub avatar hosts by exact-shape match, so no + * request URL can ever point anywhere else, and the outbound request + * carries nothing about the deployment or the viewer — no cookies, no + * forwarding chain, no referrer. A failed upstream fetch maps to a plain + * 404, which leaves the CSS background unset and the initial letter + * underneath untouched on every client. + */ + +/** GitHub logins: alphanumerics and inner hyphens, at most 39 characters. */ +const GITHUB_LOGIN = /^[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,37}[a-zA-Z0-9])?$/; + +const GITHUB_USER_ID = /^\d{1,12}$/; + +export type AvatarTarget = { kind: "login"; login: string } | { kind: "id"; id: string }; + +/** + * Parse an `/api/avatars/u/{login}` or `/api/avatars/id/{id}` tail into a + * validated upstream target, or null when the path is not one of the two + * exact shapes this route serves. + */ +export function parseAvatarTarget(segments: string[]): AvatarTarget | null { + if (segments.length !== 2) return null; + const [kind, value] = segments; + if (!kind || !value) return null; + if (kind === "u" && GITHUB_LOGIN.test(value)) return { kind: "login", login: value }; + if (kind === "id" && GITHUB_USER_ID.test(value)) return { kind: "id", id: value }; + return null; +} + +/** The upstream URL for a validated target. Nothing else is ever fetched. */ +export function avatarUpstreamUrl(target: AvatarTarget): string { + return target.kind === "login" + ? `https://github.com/${target.login}.png?size=40` + : `https://avatars.githubusercontent.com/u/${target.id}?v=4&size=40`; +} + +/** + * Outbound headers: deliberately fresh. No authorization, cookies, or + * forwarding chain from the inbound request survive, and no referrer or + * origin travels to GitHub — the request is made by the deployment server, + * not the viewer's browser. + */ +export function avatarUpstreamHeaders(): Headers { + const headers = new Headers({ accept: "image/*" }); + headers.delete("referer"); + headers.delete("origin"); + return headers; +} + +/** Only successful image responses are forwarded; everything else fails closed. */ +export function isForwardableAvatarResponse(response: Response): boolean { + return response.ok && (response.headers.get("content-type") ?? "").startsWith("image/"); +} diff --git a/apps/web/lib/pipeline.ts b/apps/web/lib/pipeline.ts index 86916f6e..03284e98 100644 --- a/apps/web/lib/pipeline.ts +++ b/apps/web/lib/pipeline.ts @@ -169,18 +169,8 @@ export function storyOwner(assignees: string[]): StoryOwner | null { } /** - * GitHub serves an avatar for any login at this path, so no avatar URL has to - * travel on the wire. It 302s to `avatars.githubusercontent.com`. - * - * `?size=40` rather than the 14–20 CSS px we draw at, so 2× displays stay sharp. + * The letter an avatar falls back to when there is no image to draw. */ -export function avatarUrlFor(login: string): string | null { - const trimmed = login.trim(); - if (!trimmed) return null; - return `https://github.com/${encodeURIComponent(trimmed)}.png?size=40`; -} - -/** The letter an avatar falls back to when there is no image to draw. */ export function avatarInitial(value: string | null | undefined): string { const trimmed = (value ?? "").trim(); // Spread, not `[0]`, so an astral first character survives intact. diff --git a/apps/web/test/avatar-component.test.tsx b/apps/web/test/avatar-component.test.tsx new file mode 100644 index 00000000..e1f68157 --- /dev/null +++ b/apps/web/test/avatar-component.test.tsx @@ -0,0 +1,40 @@ +import { Avatar } from "@facility/ui"; +import { renderToStaticMarkup } from "react-dom/server"; +import { describe, expect, it } from "vitest"; +import { avatarSrcFor } from "../lib/avatar-policy"; + +/** + * Component boundary: what the browser is actually handed for each avatar + * mode. The Avatar primitive paints the image as a CSS background over an + * initial letter, so "no image" and "image that failed to load" must be + * indistinguishable in markup — only a set background-image differs. + */ +function markup(login: string, mode?: Parameters[1]): string { + const src = avatarSrcFor(login, mode) ?? undefined; + return renderToStaticMarkup( + , + ); +} + +describe("Avatar at the component boundary", () => { + it("enabled (proxy): renders same-origin background over the letter", () => { + const html = markup("octocat", "proxy"); + expect(html).toContain("background-image:url("/api/avatars/u/octocat")"); + expect(html).toContain(">O<"); + // Nothing points outside this deployment. + expect(html).not.toContain("github"); + }); + + it("disabled (off): renders the letter with no image request at all", () => { + const html = markup("octocat", "off"); + expect(html).not.toContain("background-image"); + expect(html).toContain(">O<"); + }); + + it("failed load: markup identical to disabled — the letter survives", () => { + // A CSS background that fails paints nothing; there is no broken-image + // glyph and no error state, so failed-load markup equals no-src markup. + const html = markup("guzmonne", "off"); + expect(html).toBe(renderToStaticMarkup()); + }); +}); diff --git a/apps/web/test/avatar-policy.test.ts b/apps/web/test/avatar-policy.test.ts new file mode 100644 index 00000000..f8b2672f --- /dev/null +++ b/apps/web/test/avatar-policy.test.ts @@ -0,0 +1,91 @@ +import { describe, expect, it } from "vitest"; +import { + AVATAR_MODE_ENV, + avatarMode, + avatarSrcFor, + principalAvatarSrc, +} from "../lib/avatar-policy"; + +describe("avatar delivery policy", () => { + it("defaults to same-origin proxying and treats unknown values as off", () => { + expect(avatarMode(undefined)).toBe("proxy"); + expect(avatarMode("")).toBe("proxy"); + expect(avatarMode("proxy")).toBe("proxy"); + expect(avatarMode(" PROXY ")).toBe("proxy"); + expect(avatarMode("off")).toBe("off"); + // Fail closed: anything unrecognized disables remote images entirely. + for (const hostile of ["direct", "bogus", "https://evil.example"]) { + expect(avatarMode(hostile)).toBe("off"); + } + }); + + it(`reads ${AVATAR_MODE_ENV} at the boundary between proxy and off`, () => { + // The env var is inlined at build time; these pin the accepted values. + expect(avatarMode("proxy")).toBe("proxy"); + expect(avatarMode("off")).toBe("off"); + }); + + describe("assignee avatars", () => { + it("serves every login from this deployment's own origin", () => { + const src = avatarSrcFor("octocat"); + expect(src).toBe("/api/avatars/u/octocat"); + expect(new URL(src ?? "", "https://app.example").host).toBe("app.example"); + }); + + it("encodes the login so it cannot shape or escape the proxy path", () => { + expect(avatarSrcFor("../admin")).toBe("/api/avatars/u/..%2Fadmin"); + expect(avatarSrcFor("a/b?c")).toBe("/api/avatars/u/a%2Fb%3Fc"); + }); + + it("offers nothing to draw for a blank login", () => { + expect(avatarSrcFor("")).toBeNull(); + expect(avatarSrcFor(" ")).toBeNull(); + }); + + it("draws no image at all when avatars are disabled", () => { + expect(avatarSrcFor("octocat", "off")).toBeNull(); + }); + }); + + describe("principal avatars", () => { + it("rewrites known GitHub avatar URLs onto the same-origin proxy", () => { + expect( + principalAvatarSrc("https://avatars.githubusercontent.com/u/583231?v=4", "octocat"), + ).toBe("/api/avatars/id/583231"); + expect(principalAvatarSrc("https://github.com/octocat.png", "someone-else")).toBe( + "/api/avatars/u/octocat", + ); + }); + + it("never points the browser at an unreviewed host from a stored URL", () => { + const hostile = [ + "http://avatars.githubusercontent.com/u/583231?v=4", + "https://evil.example/u/583231", + "https://avatars.githubusercontent.com.evil.example/u/583231", + "https://github.com/octocat/avatar", + "not a url", + ]; + for (const url of hostile) { + const src = principalAvatarSrc(url, "octocat"); + expect(src).toBe("/api/avatars/u/octocat"); + } + }); + + it("falls back to the login when the stored URL is absent", () => { + expect(principalAvatarSrc(null, "octocat")).toBe("/api/avatars/u/octocat"); + expect(principalAvatarSrc(undefined, " octocat ")).toBe("/api/avatars/u/octocat"); + }); + + it("draws nothing when there is neither a usable URL nor a login", () => { + expect(principalAvatarSrc(null, null)).toBeNull(); + expect(principalAvatarSrc(null, "")).toBeNull(); + }); + + it("draws nothing at all when avatars are disabled, whatever is stored", () => { + expect( + principalAvatarSrc("https://avatars.githubusercontent.com/u/583231?v=4", "octocat", "off"), + ).toBeNull(); + expect(principalAvatarSrc(null, "octocat", "off")).toBeNull(); + }); + }); +}); diff --git a/apps/web/test/avatar-proxy.test.ts b/apps/web/test/avatar-proxy.test.ts new file mode 100644 index 00000000..430186ac --- /dev/null +++ b/apps/web/test/avatar-proxy.test.ts @@ -0,0 +1,169 @@ +import { createServer, type RequestListener } from "node:http"; +import { afterEach, describe, expect, it } from "vitest"; +import { GET } from "../app/api/avatars/[...target]/route"; + +// A local fake of the GitHub avatar surface: deterministic bytes, no +// network access. The suite never needs live credentials or egress. +const PNG_BYTES = new Uint8Array([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]); + +type CapturedRequest = { url: string; headers: Headers }; +let upstreamRequests: CapturedRequest[] = []; + +function startUpstream(listener: RequestListener): Promise { + return new Promise((resolve) => { + const server = createServer(listener); + serversToClose.push(server); + server.listen(0, "127.0.0.1", () => { + const address = server.address(); + const port = typeof address === "object" && address ? address.port : 0; + resolve(`http://127.0.0.1:${port}`); + }); + }); +} + +async function serveAvatar( + respond: (request: CapturedRequest) => { status: number; body?: Uint8Array; type?: string }, +): Promise { + const origin = await startUpstream((request, response) => { + request.resume(); // Drain so 'end' fires; we only need headers. + request.on("end", () => { + const parsed = new URL(request.url ?? "/", origin); + const upstreamUrl = + parsed.pathname + .replace(/^\/gh/, "https://github.com") + .replace(/^\/avatars\/u\//, "https://avatars.githubusercontent.com/u/") + + (parsed.search || ""); + const captured = { + // The route's fixed upstream hosts are rewritten onto this local + // fake; strip the rewrite so assertions read the real target. + url: upstreamUrl, + headers: new Headers(request.headers as Record), + }; + upstreamRequests.push(captured); + const outcome = respond(captured); + response.writeHead(outcome.status, { "content-type": outcome.type ?? "text/plain" }); + response.end(outcome.body ?? null); + }); + }); + // Point the module's fixed hosts at the local fake for this test only. + const originalFetch = globalThis.fetch; + globalThis.fetch = ((input: RequestInfo | URL, init?: RequestInit) => { + const rewritten = String(input instanceof URL ? input : input) + .replace("https://github.com", `${origin}/gh`) + .replace("https://avatars.githubusercontent.com", `${origin}/avatars`); + return originalFetch(new Request(rewritten, init)); + }) as typeof fetch; + cleanup.push(() => { + globalThis.fetch = originalFetch; + }); +} + +const cleanup: (() => Promise | void)[] = []; +const serversToClose: ReturnType[] = []; + +afterEach(async () => { + upstreamRequests = []; + // Unwind in reverse so nested fetch overrides restore correctly. + for (const undo of cleanup.splice(0).reverse()) await undo(); + await Promise.all( + serversToClose.splice(0).map( + (server) => + new Promise((resolve) => { + // Undici keeps idle keep-alive sockets open; drop them so close resolves. + server.closeIdleConnections(); + server.close(() => resolve()); + }), + ), + ); +}); + +function routeGet(path: string, env: Record = {}): Promise { + // The route reads the mode from process.env; swap it per call. + const previous = process.env.NEXT_PUBLIC_FACILITY_AVATARS; + for (const [key, value] of Object.entries(env)) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + const segments = path.split("/").filter(Boolean); + return GET(new Request(`https://app.example/api/avatars/${path}`), { + params: Promise.resolve({ target: segments }), + }).finally(() => { + if (previous === undefined) delete process.env.NEXT_PUBLIC_FACILITY_AVATARS; + else process.env.NEXT_PUBLIC_FACILITY_AVATARS = previous; + }); +} + +describe("the /api/avatars proxy route", () => { + it("forwards a valid login target as image bytes from this origin", async () => { + await serveAvatar(() => ({ status: 200, body: PNG_BYTES, type: "image/png" })); + const response = await routeGet("u/octocat"); + expect(response.status).toBe(200); + expect(response.headers.get("content-type")).toBe("image/png"); + const body = new Uint8Array(await response.arrayBuffer()); + expect([...body]).toEqual([...PNG_BYTES]); + expect(upstreamRequests).toHaveLength(1); + const captured: CapturedRequest = upstreamRequests[0] ?? { url: "", headers: new Headers() }; + expect(captured.url).toBe("https://github.com/octocat.png?size=40"); + // Fresh outbound request: nothing about the deployment or viewer leaks. + expect(captured.headers?.get("cookie")).toBeNull(); + expect(captured.headers?.get("authorization")).toBeNull(); + expect(captured.headers?.get("referer")).toBeNull(); + }); + + it("serves numeric-ID targets from the avatars host", async () => { + await serveAvatar(() => ({ status: 200, body: PNG_BYTES, type: "image/png" })); + const response = await routeGet("id/583231"); + expect(response.status).toBe(200); + expect(upstreamRequests[0]?.url).toContain("https://avatars.githubusercontent.com/u/583231"); + }); + + it("rejects any path that is not one of the two exact shapes", async () => { + for (const hostile of [ + "u/../etc/passwd", + "u/octocat/extra", + "id/not-a-number", + "u/-leading-hyphen", + "other/octocat", + "", + ]) { + const response = await routeGet(hostile); + expect(response.status).toBe(404); + } + expect(upstreamRequests).toHaveLength(0); + }); + + it("fails closed to 404 when the upstream answer is not an image", async () => { + await serveAvatar(() => ({ status: 200, type: "text/html", body: new Uint8Array([60]) })); + expect((await routeGet("u/octocat")).status).toBe(404); + + await serveAvatar(() => ({ status: 404 })); + expect((await routeGet("u/octocat")).status).toBe(404); + }); + + it("fails closed to 404 when the upstream is unreachable", async () => { + // No fake started: fetch fails outright. + const originalFetch = globalThis.fetch; + globalThis.fetch = (() => Promise.reject(new Error("ECONNREFUSED"))) as typeof fetch; + cleanup.push(() => { + globalThis.fetch = originalFetch; + }); + const response = await routeGet("u/octocat"); + expect(response.status).toBe(404); + expect(response.headers.get("cache-control")).not.toBe("max-age=86400"); + }); + + it("serves nothing but 404 when the avatar mode is off", async () => { + await serveAvatar(() => ({ status: 200, body: PNG_BYTES, type: "image/png" })); + const response = await routeGet("u/octocat", { NEXT_PUBLIC_FACILITY_AVATARS: "off" }); + expect(response.status).toBe(404); + expect(upstreamRequests).toHaveLength(0); + }); + + it("marks successful responses as cacheable but private", async () => { + await serveAvatar(() => ({ status: 200, body: PNG_BYTES, type: "image/png" })); + const response = await routeGet("u/octocat"); + expect(response.headers.get("cache-control")).toBe("private, max-age=86400"); + expect(response.headers.get("content-security-policy")).toContain("default-src 'none'"); + expect(response.headers.get("x-content-type-options")).toBe("nosniff"); + }); +}); diff --git a/apps/web/test/pipeline-story.test.ts b/apps/web/test/pipeline-story.test.ts index eb974502..abe13955 100644 --- a/apps/web/test/pipeline-story.test.ts +++ b/apps/web/test/pipeline-story.test.ts @@ -3,7 +3,6 @@ import { ciStatusLabel } from "@/components/ci-status"; import type { PipelineStageKey, PipelineStory, Proposal, StoryDetail } from "@/lib/api"; import { avatarInitial, - avatarUrlFor, boardHref, mineFilterState, ownedBy, @@ -310,23 +309,6 @@ describe("story presentation contract", () => { expect(storyOwner([" a "])).toEqual({ login: "a", extra: 0 }); }); - it("builds a GitHub avatar URL from a login, at twice the drawn size", () => { - expect(avatarUrlFor("octocat")).toBe("https://github.com/octocat.png?size=40"); - }); - - it("trims a login before building its avatar URL", () => { - expect(avatarUrlFor(" octocat ")).toBe("https://github.com/octocat.png?size=40"); - }); - - it("escapes a login rather than letting it shape the avatar URL", () => { - expect(avatarUrlFor("a/b?c")).toBe("https://github.com/a%2Fb%3Fc.png?size=40"); - }); - - it("has no avatar URL to offer for a blank login", () => { - expect(avatarUrlFor("")).toBeNull(); - expect(avatarUrlFor(" ")).toBeNull(); - }); - it("falls back to the first letter of a login, uppercased", () => { expect(avatarInitial("octocat")).toBe("O"); expect(avatarInitial("Octocat")).toBe("O"); From 3504a033bf94825fa9511aca30b7670dd588b470 Mon Sep 17 00:00:00 2001 From: guzmonne Date: Tue, 1 Sep 2026 08:53:38 -0300 Subject: [PATCH 10/10] fix(web): bound avatar proxy egress to signed-in viewers and vetted hops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found two ways the /api/avatars routes let a caller drive this deployment's outbound traffic. Both are fixed here rather than in two commits because they share one route body and one test harness. The routes were public, and every request produced a fresh upstream fetch, so anyone who could reach the deployment could cycle valid-looking logins or IDs and make it call GitHub without limit. A request now reaches an upstream only when three things hold: it carries a session the control plane recognises, no cached answer already exists, and the viewer's fetch allowance is not spent. Cookie presence alone is not enough — the session is sealed, so only /v1/me can judge it — and a control plane that cannot answer denies rather than admits. Verdicts are cached per token, rejections included, so a replayed token cannot turn avatar loads into control-plane load either. Answers are cached server side: 512 entries, a day for image bytes, five minutes for a miss so an avatar-less login is not refetched on every paint, LRU eviction and a per-entry byte ceiling. Cache hits spend no allowance, so the per-viewer ceiling binds only on targets nobody has asked for. Both bounds are per web process, which the hardening checklist now says. redirect: "follow" let the upstream redirect anywhere: a fake upstream pointing at a second server had its bytes returned as an avatar. The fetch now follows hops by hand — each Location resolved against the URL that produced it, vetted against the two permitted GitHub hosts over https before it becomes a request, and the chain cut after three hops rather than run to whatever length the upstream chooses. Regression tests were checked by mutation rather than assumed: restoring follow-any-host fails the redirect cases, including the assertion that the second server receives nothing, and removing the session gate fails four cases. apps/web is at 133 tests, up from 88. Refs theam/facility#175 --- .env.example | 3 + apps/docs/docs/self-host/production.md | 6 + apps/web/app/api/avatars/[...target]/route.ts | 138 ++++++++-- apps/web/lib/avatar-egress.ts | 61 +++++ apps/web/lib/avatar-gate.ts | 171 ++++++++++++ apps/web/lib/avatar-proxy.ts | 60 +++- apps/web/test/avatar-egress.test.ts | 149 ++++++++++ apps/web/test/avatar-proxy.test.ts | 257 +++++++++++++++++- 8 files changed, 804 insertions(+), 41 deletions(-) create mode 100644 apps/web/lib/avatar-egress.ts create mode 100644 apps/web/lib/avatar-gate.ts create mode 100644 apps/web/test/avatar-egress.test.ts diff --git a/.env.example b/.env.example index 8032086f..afb7f25b 100644 --- a/.env.example +++ b/.env.example @@ -8,6 +8,9 @@ # initial letters only — zero remote requests. There is no browser-direct # mode; deployments that must not reach GitHub at all can additionally # firewall the server's egress, and avatars degrade to letters. +# In "proxy" mode the routes serve signed-in viewers only, cache what they +# fetch, and cap how much any one viewer can fetch, so the deployment's +# outbound avatar traffic does not grow with inbound request volume. NEXT_PUBLIC_FACILITY_AVATARS=proxy # --- Core --- diff --git a/apps/docs/docs/self-host/production.md b/apps/docs/docs/self-host/production.md index 61f5cbb9..5c389f32 100644 --- a/apps/docs/docs/self-host/production.md +++ b/apps/docs/docs/self-host/production.md @@ -165,6 +165,12 @@ order, and end-to-end verification. carries no cookies or referrer. Set `NEXT_PUBLIC_FACILITY_AVATARS=off` to draw initial letters only; a deployment whose server egress to GitHub is firewalled degrades to letters on its own. +- The avatar routes need a session the control plane recognises, so nobody who + can merely reach the deployment can make it call GitHub. Answers are cached + in the web process, and each signed-in viewer has a ceiling on how many + avatars they can make it fetch, so the traffic those routes send to GitHub + is bounded by the people who have accounts rather than by request volume. + Both bounds are per web process; run more than one and each holds its own. - Backups: Postgres PITR + object-store lifecycle; audit retention per your compliance window. - Keep `node packages/cli/bin/facility.mjs doctor --url https:// --key diff --git a/apps/web/app/api/avatars/[...target]/route.ts b/apps/web/app/api/avatars/[...target]/route.ts index 21726bff..1794bc29 100644 --- a/apps/web/app/api/avatars/[...target]/route.ts +++ b/apps/web/app/api/avatars/[...target]/route.ts @@ -1,8 +1,20 @@ import { avatarMode } from "@/lib/avatar-policy"; import { + allowUpstreamFetch, + avatarViewerId, + type CachedAvatar, + cachedAvatar, + MAX_AVATAR_BYTES, + rememberAvatar, +} from "@/lib/avatar-gate"; +import { + type AvatarTarget, avatarUpstreamHeaders, avatarUpstreamUrl, + isAllowedAvatarUpstream, isForwardableAvatarResponse, + MAX_AVATAR_REDIRECT_HOPS, + nextAvatarRedirect, parseAvatarTarget, } from "@/lib/avatar-proxy"; @@ -12,43 +24,117 @@ export const dynamic = "force-dynamic"; * Same-origin avatar images: `/api/avatars/u/{login}` and * `/api/avatars/id/{id}`. The browser never contacts GitHub; this route * fetches server-side with fresh, referrer-free headers and forwards only - * successful image bytes. Any other path, an invalid target, a disabled - * avatar mode, or an upstream failure maps to 404 — which leaves the - * caller's CSS background unset and its initial letter showing. + * successful image bytes. + * + * Reaching GitHub is a privilege, not a side effect of being reachable. A + * request only causes an upstream fetch when it carries a session the control + * plane recognises, when no cached answer already exists, and when the + * viewer's fetch allowance is not spent — so the number of requests that can + * arrive at this route no longer bounds the egress it produces. + * + * An unauthenticated caller gets 401. Any other path, an invalid target, a + * disabled avatar mode, a spent allowance, or an upstream failure maps to + * 404. Every one of those leaves the caller's CSS background unset and its + * initial letter showing, so no failure is visible as a broken image. */ export async function GET( - _request: Request, + request: Request, { params }: { params: Promise<{ target: string[] }> }, ) { if (avatarMode(process.env.NEXT_PUBLIC_FACILITY_AVATARS) === "off") { - return new Response(null, { status: 404 }); + return empty(404); } const { target: segments } = await params; const target = parseAvatarTarget(segments ?? []); - if (!target) return new Response(null, { status: 404 }); + if (!target) return empty(404); + + const viewer = await avatarViewerId(request); + if (!viewer) return empty(401); + + const key = avatarCacheKey(target); + const cached = cachedAvatar(key); + if (cached) return deliver(cached); + + if (!allowUpstreamFetch(viewer)) return empty(429); + + const fetched = await fetchAvatar(target); + rememberAvatar(key, fetched); + return deliver(fetched); +} + +function avatarCacheKey(target: AvatarTarget): string { + return target.kind === "login" ? `u/${target.login}` : `id/${target.id}`; +} +/** + * Fetch one avatar, following redirects by hand. `redirect: "manual"` is the + * point: the fetch layer would follow a hop to any host the upstream names, + * so each Location is resolved and vetted against the permitted hosts here + * before it becomes a request, and the chain is cut off after a fixed number + * of hops rather than run to whatever length the upstream chooses. + */ +async function fetchAvatar(target: AvatarTarget): Promise { + let url = avatarUpstreamUrl(target); try { - const upstream = await fetch(avatarUpstreamUrl(target), { - headers: avatarUpstreamHeaders(), - redirect: "follow", - signal: AbortSignal.timeout(5_000), - cache: "no-store", - }); - if (!isForwardableAvatarResponse(upstream)) return new Response(null, { status: 404 }); - - return new Response(upstream.body, { - status: 200, - headers: { - "content-type": upstream.headers.get("content-type") ?? "image/png", - // Avatars change rarely; let the browser and any shared cache keep - // one for a day, and revalidate against this route afterwards. - "cache-control": "private, max-age=86400", - "content-security-policy": "default-src 'none'; sandbox", - "x-content-type-options": "nosniff", - }, - }); + for (let hop = 0; hop <= MAX_AVATAR_REDIRECT_HOPS; hop += 1) { + if (!isAllowedAvatarUpstream(url)) return { kind: "missing" }; + const response = await fetch(url, { + headers: avatarUpstreamHeaders(), + redirect: "manual", + signal: AbortSignal.timeout(5_000), + cache: "no-store", + }); + + const redirect = nextAvatarRedirect(response.status, response.headers.get("location"), url); + if (redirect) { + await response.body?.cancel(); + if (redirect.kind === "deny") return { kind: "missing" }; + url = redirect.url; + continue; + } + + if (!isForwardableAvatarResponse(response)) { + await response.body?.cancel(); + return { kind: "missing" }; + } + const declared = Number(response.headers.get("content-length") ?? "0"); + if (declared > MAX_AVATAR_BYTES) { + await response.body?.cancel(); + return { kind: "missing" }; + } + const bytes = new Uint8Array(await response.arrayBuffer()); + if (bytes.byteLength > MAX_AVATAR_BYTES) return { kind: "missing" }; + return { + kind: "image", + bytes, + contentType: response.headers.get("content-type") ?? "image/png", + }; + } + // Hops exhausted: the upstream is redirecting in a loop, not serving. + return { kind: "missing" }; } catch { - return new Response(null, { status: 404 }); + return { kind: "missing" }; } } + +function deliver(avatar: CachedAvatar) { + if (avatar.kind === "missing") return empty(404); + return new Response(avatar.bytes, { + status: 200, + headers: { + "content-type": avatar.contentType, + // Avatars change rarely; let the browser keep one for a day, and + // revalidate against this route afterwards. Private: the bytes are + // served only to an authenticated viewer, so no shared cache may hold + // them on behalf of everyone else. + "cache-control": "private, max-age=86400", + "content-security-policy": "default-src 'none'; sandbox", + "x-content-type-options": "nosniff", + }, + }); +} + +function empty(status: number) { + return new Response(null, { status, headers: { "cache-control": "no-store" } }); +} diff --git a/apps/web/lib/avatar-egress.ts b/apps/web/lib/avatar-egress.ts new file mode 100644 index 00000000..6baec1a6 --- /dev/null +++ b/apps/web/lib/avatar-egress.ts @@ -0,0 +1,61 @@ +/** + * Pure egress policy for `/api/avatars/…`. + * + * The route fetches a third party, so two things must be bounded: who may + * cause a fetch, and how many fetches they may cause. Both decisions live + * here as plain functions over plain values, with the clock passed in. The + * mutable stores that apply them are in `avatar-gate.ts`. + */ + +/** The session cookie the control plane mints. Absent means unauthenticated. */ +export const SESSION_COOKIE = "facility_session"; + +/** The viewer's session token from a raw Cookie header, or null when absent. */ +export function sessionTokenFrom(cookieHeader: string | null | undefined): string | null { + for (const pair of (cookieHeader ?? "").split(";")) { + const separator = pair.indexOf("="); + if (separator < 0) continue; + if (pair.slice(0, separator).trim() !== SESSION_COOKIE) continue; + const value = pair.slice(separator + 1).trim(); + return value || null; + } + return null; +} + +export type BucketLimit = { burst: number; perMinute: number }; +export type BucketState = { tokens: number; updatedAt: number }; + +/** A viewer's allowance of upstream fetches: a full bucket, as of `now`. */ +export function newBucket(limit: BucketLimit, now: number): BucketState { + return { tokens: limit.burst, updatedAt: now }; +} + +/** + * Spend one upstream fetch from a viewer's bucket, refilling for the time + * since it was last touched. `allowed: false` leaves the bucket empty and the + * caller must not reach the upstream. + */ +export function spendToken( + state: BucketState, + limit: BucketLimit, + now: number, +): { state: BucketState; allowed: boolean } { + const elapsed = Math.max(0, now - state.updatedAt); + const refilled = Math.min(limit.burst, state.tokens + (elapsed * limit.perMinute) / 60_000); + if (refilled < 1) return { state: { tokens: refilled, updatedAt: now }, allowed: false }; + return { state: { tokens: refilled - 1, updatedAt: now }, allowed: true }; +} + +/** + * How many entries a store of `size` must drop to stay within `max`. Callers + * take that many from the front of an insertion-ordered map, which is the + * least recently used end when reads re-insert. + */ +export function overCapacity(size: number, max: number): number { + return Math.max(0, size - max); +} + +/** Whether a stored entry is still within its lifetime at `now`. */ +export function isFresh(entry: { expiresAt: number }, now: number): boolean { + return entry.expiresAt > now; +} diff --git a/apps/web/lib/avatar-gate.ts b/apps/web/lib/avatar-gate.ts new file mode 100644 index 00000000..3e599d98 --- /dev/null +++ b/apps/web/lib/avatar-gate.ts @@ -0,0 +1,171 @@ +import { + type BucketLimit, + type BucketState, + isFresh, + newBucket, + overCapacity, + SESSION_COOKIE, + sessionTokenFrom, + spendToken, +} from "@/lib/avatar-egress"; + +/** + * The mutable side of avatar egress: the caches and allowances that decide + * whether a request reaches GitHub at all. Every rule these stores apply is a + * pure function from `avatar-egress.ts`; this module only holds state, reads + * the clock, and asks the control plane who the viewer is. + * + * Each store is bounded in entries and in lifetime, so a caller cycling + * logins or IDs cannot grow this process's memory, and a viewer's fetches + * against the upstream are capped whether or not the cache answers. + */ + +/** Session lookups are cheap to repeat but not free; hold each answer briefly. */ +const SESSION_TTL_MS = 60_000; +const SESSION_CACHE_MAX = 1_024; + +/** Avatars change rarely, so a hit here is the common case after warm-up. */ +const IMAGE_TTL_MS = 86_400_000; +/** A login with no avatar is remembered too, or it is refetched every paint. */ +const MISS_TTL_MS = 300_000; +const IMAGE_CACHE_MAX = 512; + +/** An avatar is a few kilobytes. Anything this large is not one. */ +export const MAX_AVATAR_BYTES = 256 * 1024; + +/** + * Per viewer: enough to paint a full board of distinct assignees at once, + * then a steady trickle. Cache hits cost nothing, so the ceiling only binds + * on someone asking for logins nobody has asked for before. + */ +const UPSTREAM_LIMIT: BucketLimit = { burst: 60, perMinute: 30 }; +const BUCKET_TTL_MS = 600_000; +const BUCKET_CACHE_MAX = 4_096; + +type Entry = { value: V; expiresAt: number }; + +type BoundedStore = { + get(key: string, now: number): V | undefined; + set(key: string, value: V, ttlMs: number, now: number): void; + clear(): void; +}; + +/** + * A map bounded in both directions: entries expire, and the oldest are + * dropped once `max` is exceeded. A read re-inserts, so insertion order is + * least-recently-used order and eviction takes from the front. + */ +function boundedStore(max: number): BoundedStore { + const entries = new Map>(); + return { + get(key, now) { + const entry = entries.get(key); + if (!entry) return undefined; + if (!isFresh(entry, now)) { + entries.delete(key); + return undefined; + } + entries.delete(key); + entries.set(key, entry); + return entry.value; + }, + set(key, value, ttlMs, now) { + entries.delete(key); + entries.set(key, { value, expiresAt: now + ttlMs }); + for (const stale of [...entries].filter(([, e]) => !isFresh(e, now)).map(([k]) => k)) { + entries.delete(stale); + } + for (const oldest of [...entries.keys()].slice(0, overCapacity(entries.size, max))) { + entries.delete(oldest); + } + }, + clear() { + entries.clear(); + }, + }; +} + +/** The principal a session belongs to, or "" for a token the control plane rejected. */ +const sessions = boundedStore(SESSION_CACHE_MAX); +const images = boundedStore(IMAGE_CACHE_MAX); +const buckets = boundedStore(BUCKET_CACHE_MAX); + +export type CachedAvatar = + | { kind: "image"; bytes: Uint8Array; contentType: string } + | { kind: "missing" }; + +function controlPlaneUrl() { + // Read at request time so a promoted standalone image sees the deployment's + // runtime environment, matching lib/api.ts. + return process.env.FACILITY_API_URL ?? "http://localhost:4400"; +} + +/** + * The principal behind this request's session cookie, or null when there is + * no cookie or the control plane does not recognise it. Null must stop the + * request: an unauthenticated caller may not make this deployment fetch + * anything from GitHub. + * + * The answer is cached per token, rejections included, so a caller replaying + * one token cannot turn avatar loads into control-plane load either. + */ +export async function avatarViewerId(request: Request): Promise { + const token = sessionTokenFrom(request.headers.get("cookie")); + if (!token) return null; + + const now = Date.now(); + const cached = sessions.get(token, now); + if (cached !== undefined) return cached || null; + + let viewer = ""; + try { + const response = await fetch(`${controlPlaneUrl()}/v1/me`, { + headers: { cookie: `${SESSION_COOKIE}=${token}`, accept: "application/json" }, + cache: "no-store", + redirect: "error", + signal: AbortSignal.timeout(5_000), + }); + if (response.ok) { + const body = (await response.json()) as { principal?: { id?: unknown } }; + if (typeof body?.principal?.id === "string") viewer = body.principal.id; + } else if (response.status >= 500) { + // A control plane that is down has not said this session is invalid. + // Deny the request without remembering the answer. + return null; + } + } catch { + return null; + } + + sessions.set(token, viewer, SESSION_TTL_MS, now); + return viewer || null; +} + +/** + * Whether this viewer may cause one more upstream fetch. Callers ask only + * after the cache has missed, so a warm avatar never spends an allowance. + */ +export function allowUpstreamFetch(viewerId: string): boolean { + const now = Date.now(); + const current = buckets.get(viewerId, now) ?? newBucket(UPSTREAM_LIMIT, now); + const { state, allowed } = spendToken(current, UPSTREAM_LIMIT, now); + buckets.set(viewerId, state, BUCKET_TTL_MS, now); + return allowed; +} + +/** A previously fetched avatar, or a remembered absence, for this target. */ +export function cachedAvatar(key: string): CachedAvatar | undefined { + return images.get(key, Date.now()); +} + +/** Remember a fetched avatar, or the fact that the upstream has none. */ +export function rememberAvatar(key: string, value: CachedAvatar): void { + images.set(key, value, value.kind === "image" ? IMAGE_TTL_MS : MISS_TTL_MS, Date.now()); +} + +/** Drop every store. Tests call this so one case cannot answer the next. */ +export function resetAvatarGate(): void { + sessions.clear(); + images.clear(); + buckets.clear(); +} diff --git a/apps/web/lib/avatar-proxy.ts b/apps/web/lib/avatar-proxy.ts index d528efb1..52905f19 100644 --- a/apps/web/lib/avatar-proxy.ts +++ b/apps/web/lib/avatar-proxy.ts @@ -6,9 +6,13 @@ * targets are pinned to two GitHub avatar hosts by exact-shape match, so no * request URL can ever point anywhere else, and the outbound request * carries nothing about the deployment or the viewer — no cookies, no - * forwarding chain, no referrer. A failed upstream fetch maps to a plain - * 404, which leaves the CSS background unset and the initial letter + * forwarding chain, no referrer. The same pinning applies to every redirect + * hop, because a host that may serve an avatar may still answer with a + * Location pointing somewhere it may not. A failed upstream fetch maps to a + * plain 404, which leaves the CSS background unset and the initial letter * underneath untouched on every client. + * + * Who may cause a fetch, and how often, is decided in `avatar-egress.ts`. */ /** GitHub logins: alphanumerics and inner hyphens, at most 39 characters. */ @@ -56,3 +60,55 @@ export function avatarUpstreamHeaders(): Headers { export function isForwardableAvatarResponse(response: Response): boolean { return response.ok && (response.headers.get("content-type") ?? "").startsWith("image/"); } + +/** + * The only hosts an avatar fetch may reach — at the first request and at + * every redirect hop alike. `github.com/{login}.png` answers with a 302 to + * `avatars.githubusercontent.com`, so hops must be followed, but an upstream + * that answers with a redirect anywhere else is not serving an avatar. + */ +const AVATAR_UPSTREAM_HOSTS = new Set(["github.com", "avatars.githubusercontent.com"]); + +/** + * Redirect hops followed before the fetch fails closed. GitHub's own chain is + * one hop; the rest is slack, not an invitation. + */ +export const MAX_AVATAR_REDIRECT_HOPS = 3; + +/** Whether a URL is one this deployment may fetch an avatar from. */ +export function isAllowedAvatarUpstream(url: string): boolean { + let parsed: URL; + try { + parsed = new URL(url); + } catch { + return false; + } + return parsed.protocol === "https:" && AVATAR_UPSTREAM_HOSTS.has(parsed.host); +} + +const REDIRECT_STATUSES = new Set([301, 302, 303, 307, 308]); + +export type AvatarRedirect = { kind: "follow"; url: string } | { kind: "deny" }; + +/** + * Where a redirect points, resolved against the URL that produced it, or null + * when the response is not a redirect and its body is the answer. A hop off + * the two permitted hosts, a missing Location, and an unparsable Location all + * come back as `deny`: the route never fetches a URL it has not vetted, which + * is what following redirects in the fetch layer gave away. + */ +export function nextAvatarRedirect( + status: number, + location: string | null, + from: string, +): AvatarRedirect | null { + if (!REDIRECT_STATUSES.has(status)) return null; + if (!location) return { kind: "deny" }; + let resolved: string; + try { + resolved = new URL(location, from).toString(); + } catch { + return { kind: "deny" }; + } + return isAllowedAvatarUpstream(resolved) ? { kind: "follow", url: resolved } : { kind: "deny" }; +} diff --git a/apps/web/test/avatar-egress.test.ts b/apps/web/test/avatar-egress.test.ts new file mode 100644 index 00000000..dd8eee0e --- /dev/null +++ b/apps/web/test/avatar-egress.test.ts @@ -0,0 +1,149 @@ +import { describe, expect, it } from "vitest"; +import { + type BucketLimit, + isFresh, + newBucket, + overCapacity, + sessionTokenFrom, + spendToken, +} from "../lib/avatar-egress"; +import { isAllowedAvatarUpstream, nextAvatarRedirect } from "../lib/avatar-proxy"; + +describe("reading the viewer's session token", () => { + it("finds the session cookie among others", () => { + expect(sessionTokenFrom("theme=dark; facility_session=abc123; last_project=p_1")).toBe( + "abc123", + ); + expect(sessionTokenFrom("facility_session=abc123")).toBe("abc123"); + }); + + it("reports no token rather than an empty one", () => { + expect(sessionTokenFrom(null)).toBeNull(); + expect(sessionTokenFrom("")).toBeNull(); + expect(sessionTokenFrom("theme=dark")).toBeNull(); + expect(sessionTokenFrom("facility_session=")).toBeNull(); + expect(sessionTokenFrom("facility_session")).toBeNull(); + }); + + it("does not mistake a cookie whose name merely ends in the session name", () => { + expect(sessionTokenFrom("not_facility_session=abc123")).toBeNull(); + expect(sessionTokenFrom("xfacility_session=abc; facility_session=real")).toBe("real"); + }); +}); + +describe("the per-viewer upstream allowance", () => { + const limit: BucketLimit = { burst: 3, perMinute: 60 }; + + it("spends one token per fetch and then denies", () => { + let state = newBucket(limit, 0); + for (const expected of [true, true, true, false]) { + const result = spendToken(state, limit, 0); + expect(result.allowed).toBe(expected); + state = result.state; + } + }); + + it("refills over time, up to the burst ceiling", () => { + let state = newBucket(limit, 0); + for (let i = 0; i < 3; i += 1) state = spendToken(state, limit, 0).state; + expect(spendToken(state, limit, 0).allowed).toBe(false); + + // One token per second at 60/minute. + expect(spendToken(state, limit, 1_000).allowed).toBe(true); + + // An idle hour cannot bank more than the burst. + const rested = spendToken(state, limit, 3_600_000); + expect(rested.allowed).toBe(true); + expect(rested.state.tokens).toBe(limit.burst - 1); + }); + + it("treats a clock that goes backwards as no elapsed time", () => { + const state = { tokens: 0, updatedAt: 10_000 }; + expect(spendToken(state, limit, 0).allowed).toBe(false); + }); +}); + +describe("bounding a store", () => { + it("reports how many entries must go", () => { + expect(overCapacity(3, 10)).toBe(0); + expect(overCapacity(10, 10)).toBe(0); + expect(overCapacity(13, 10)).toBe(3); + }); + + it("treats an entry as stale at its expiry instant", () => { + expect(isFresh({ expiresAt: 100 }, 99)).toBe(true); + expect(isFresh({ expiresAt: 100 }, 100)).toBe(false); + expect(isFresh({ expiresAt: 100 }, 101)).toBe(false); + }); +}); + +describe("vetting an upstream URL", () => { + it("permits only the two GitHub avatar hosts over https", () => { + expect(isAllowedAvatarUpstream("https://github.com/octocat.png?size=40")).toBe(true); + expect(isAllowedAvatarUpstream("https://avatars.githubusercontent.com/u/1?v=4")).toBe(true); + }); + + it("refuses another host, another scheme, and a lookalike host", () => { + for (const hostile of [ + "http://github.com/octocat.png", + "https://evil.example/octocat.png", + "https://github.com.evil.example/octocat.png", + "https://evilgithub.com/octocat.png", + "https://user:pass@evil.example/x.png", + "file:///etc/passwd", + "not a url", + "https://127.0.0.1/x.png", + "https://169.254.169.254/latest/meta-data", + ]) { + expect(isAllowedAvatarUpstream(hostile), hostile).toBe(false); + } + }); +}); + +describe("deciding a redirect hop", () => { + const from = "https://github.com/octocat.png?size=40"; + + it("is not a redirect when the status is not one", () => { + expect(nextAvatarRedirect(200, null, from)).toBeNull(); + expect(nextAvatarRedirect(404, null, from)).toBeNull(); + // A Location on a non-redirect is not a hop either. + expect(nextAvatarRedirect(200, "https://evil.example/x.png", from)).toBeNull(); + }); + + it("follows a hop that stays on a permitted host", () => { + expect( + nextAvatarRedirect(302, "https://avatars.githubusercontent.com/u/583231?v=4", from), + ).toEqual({ kind: "follow", url: "https://avatars.githubusercontent.com/u/583231?v=4" }); + }); + + it("resolves a relative Location against the URL that produced it", () => { + expect(nextAvatarRedirect(301, "/other.png", from)).toEqual({ + kind: "follow", + url: "https://github.com/other.png", + }); + }); + + it("denies a hop off the permitted hosts, however it is written", () => { + for (const [status, location] of [ + [302, "https://evil.example/x.png"], + [301, "//evil.example/x.png"], + [303, "http://github.com/octocat.png"], + [307, "https://github.com.evil.example/x.png"], + [308, "file:///etc/passwd"], + [302, null], + [302, ""], + ] as const) { + expect(nextAvatarRedirect(status, location, from), `${status} ${location}`).toEqual({ + kind: "deny", + }); + } + }); + + it("denies every redirect status equally", () => { + for (const status of [301, 302, 303, 307, 308]) { + expect(nextAvatarRedirect(status, "https://evil.example/x.png", from)).toEqual({ + kind: "deny", + }); + } + }); +}); diff --git a/apps/web/test/avatar-proxy.test.ts b/apps/web/test/avatar-proxy.test.ts index 430186ac..97c49745 100644 --- a/apps/web/test/avatar-proxy.test.ts +++ b/apps/web/test/avatar-proxy.test.ts @@ -1,11 +1,16 @@ import { createServer, type RequestListener } from "node:http"; import { afterEach, describe, expect, it } from "vitest"; import { GET } from "../app/api/avatars/[...target]/route"; +import { resetAvatarGate } from "../lib/avatar-gate"; -// A local fake of the GitHub avatar surface: deterministic bytes, no -// network access. The suite never needs live credentials or egress. +// A local fake of the GitHub avatar surface and of this deployment's own +// control plane: deterministic bytes, no network access. The suite never +// needs live credentials or egress. const PNG_BYTES = new Uint8Array([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]); +/** The one session token the fake control plane recognises. */ +const VALID_SESSION = "sealed-session-for-user-1"; + type CapturedRequest = { url: string; headers: Headers }; let upstreamRequests: CapturedRequest[] = []; @@ -21,8 +26,15 @@ function startUpstream(listener: RequestListener): Promise { }); } +type UpstreamAnswer = { + status: number; + body?: Uint8Array; + type?: string; + headers?: Record; +}; + async function serveAvatar( - respond: (request: CapturedRequest) => { status: number; body?: Uint8Array; type?: string }, + respond: (request: CapturedRequest) => UpstreamAnswer, ): Promise { const origin = await startUpstream((request, response) => { request.resume(); // Drain so 'end' fires; we only need headers. @@ -41,7 +53,10 @@ async function serveAvatar( }; upstreamRequests.push(captured); const outcome = respond(captured); - response.writeHead(outcome.status, { "content-type": outcome.type ?? "text/plain" }); + response.writeHead(outcome.status, { + "content-type": outcome.type ?? "text/plain", + ...(outcome.headers ?? {}), + }); response.end(outcome.body ?? null); }); }); @@ -58,11 +73,51 @@ async function serveAvatar( }); } +/** + * A stand-in for this deployment's control plane. It answers /v1/me for the + * one session token above and 401s everything else, which is what decides + * whether the route is allowed to reach an upstream at all. + */ +async function serveControlPlane( + options: { valid?: string[]; status?: number } = {}, +): Promise<{ requests: number }> { + const valid = new Set(options.valid ?? [VALID_SESSION]); + const counter = { requests: 0 }; + const origin = await startUpstream((request, response) => { + request.resume(); + request.on("end", () => { + counter.requests += 1; + if (options.status && options.status >= 400) { + response.writeHead(options.status, { "content-type": "application/json" }); + response.end(JSON.stringify({ error: "unavailable" })); + return; + } + const cookie = String(request.headers.cookie ?? ""); + const token = cookie.replace(/^.*facility_session=/, "").split(";")[0]; + if (request.url !== "/v1/me" || !valid.has(token ?? "")) { + response.writeHead(401, { "content-type": "application/json" }); + response.end(JSON.stringify({ error: "unauthorized" })); + return; + } + response.writeHead(200, { "content-type": "application/json" }); + response.end(JSON.stringify({ principal: { id: "user_1" }, org: null, permissions: [] })); + }); + }); + const previous = process.env.FACILITY_API_URL; + process.env.FACILITY_API_URL = origin; + cleanup.push(() => { + if (previous === undefined) delete process.env.FACILITY_API_URL; + else process.env.FACILITY_API_URL = previous; + }); + return counter; +} + const cleanup: (() => Promise | void)[] = []; const serversToClose: ReturnType[] = []; afterEach(async () => { upstreamRequests = []; + resetAvatarGate(); // Unwind in reverse so nested fetch overrides restore correctly. for (const undo of cleanup.splice(0).reverse()) await undo(); await Promise.all( @@ -77,17 +132,24 @@ afterEach(async () => { ); }); -function routeGet(path: string, env: Record = {}): Promise { +function routeGet( + path: string, + options: { env?: Record; session?: string | null } = {}, +): Promise { // The route reads the mode from process.env; swap it per call. const previous = process.env.NEXT_PUBLIC_FACILITY_AVATARS; - for (const [key, value] of Object.entries(env)) { + for (const [key, value] of Object.entries(options.env ?? {})) { if (value === undefined) delete process.env[key]; else process.env[key] = value; } + const session = options.session === undefined ? VALID_SESSION : options.session; const segments = path.split("/").filter(Boolean); - return GET(new Request(`https://app.example/api/avatars/${path}`), { - params: Promise.resolve({ target: segments }), - }).finally(() => { + return GET( + new Request(`https://app.example/api/avatars/${path}`, { + headers: session ? { cookie: `theme=dark; facility_session=${session}` } : {}, + }), + { params: Promise.resolve({ target: segments }) }, + ).finally(() => { if (previous === undefined) delete process.env.NEXT_PUBLIC_FACILITY_AVATARS; else process.env.NEXT_PUBLIC_FACILITY_AVATARS = previous; }); @@ -95,6 +157,7 @@ function routeGet(path: string, env: Record = {}): P describe("the /api/avatars proxy route", () => { it("forwards a valid login target as image bytes from this origin", async () => { + await serveControlPlane(); await serveAvatar(() => ({ status: 200, body: PNG_BYTES, type: "image/png" })); const response = await routeGet("u/octocat"); expect(response.status).toBe(200); @@ -111,6 +174,7 @@ describe("the /api/avatars proxy route", () => { }); it("serves numeric-ID targets from the avatars host", async () => { + await serveControlPlane(); await serveAvatar(() => ({ status: 200, body: PNG_BYTES, type: "image/png" })); const response = await routeGet("id/583231"); expect(response.status).toBe(200); @@ -118,6 +182,8 @@ describe("the /api/avatars proxy route", () => { }); it("rejects any path that is not one of the two exact shapes", async () => { + await serveControlPlane(); + await serveAvatar(() => ({ status: 200, body: PNG_BYTES, type: "image/png" })); for (const hostile of [ "u/../etc/passwd", "u/octocat/extra", @@ -133,17 +199,24 @@ describe("the /api/avatars proxy route", () => { }); it("fails closed to 404 when the upstream answer is not an image", async () => { + await serveControlPlane(); await serveAvatar(() => ({ status: 200, type: "text/html", body: new Uint8Array([60]) })); expect((await routeGet("u/octocat")).status).toBe(404); await serveAvatar(() => ({ status: 404 })); - expect((await routeGet("u/octocat")).status).toBe(404); + expect((await routeGet("u/someone-else")).status).toBe(404); }); it("fails closed to 404 when the upstream is unreachable", async () => { - // No fake started: fetch fails outright. + await serveControlPlane(); + const controlPlaneUrl = process.env.FACILITY_API_URL ?? ""; + // Every call but the control-plane check fails outright. const originalFetch = globalThis.fetch; - globalThis.fetch = (() => Promise.reject(new Error("ECONNREFUSED"))) as typeof fetch; + globalThis.fetch = ((input: RequestInfo | URL, init?: RequestInit) => { + const url = String(input instanceof URL ? input : input); + if (url.startsWith(controlPlaneUrl)) return originalFetch(input as RequestInfo, init); + return Promise.reject(new Error("ECONNREFUSED")); + }) as typeof fetch; cleanup.push(() => { globalThis.fetch = originalFetch; }); @@ -153,13 +226,15 @@ describe("the /api/avatars proxy route", () => { }); it("serves nothing but 404 when the avatar mode is off", async () => { + await serveControlPlane(); await serveAvatar(() => ({ status: 200, body: PNG_BYTES, type: "image/png" })); - const response = await routeGet("u/octocat", { NEXT_PUBLIC_FACILITY_AVATARS: "off" }); + const response = await routeGet("u/octocat", { env: { NEXT_PUBLIC_FACILITY_AVATARS: "off" } }); expect(response.status).toBe(404); expect(upstreamRequests).toHaveLength(0); }); it("marks successful responses as cacheable but private", async () => { + await serveControlPlane(); await serveAvatar(() => ({ status: 200, body: PNG_BYTES, type: "image/png" })); const response = await routeGet("u/octocat"); expect(response.headers.get("cache-control")).toBe("private, max-age=86400"); @@ -167,3 +242,159 @@ describe("the /api/avatars proxy route", () => { expect(response.headers.get("x-content-type-options")).toBe("nosniff"); }); }); + +describe("who may make the route fetch from GitHub", () => { + it("refuses a request with no session and never reaches the upstream", async () => { + await serveControlPlane(); + await serveAvatar(() => ({ status: 200, body: PNG_BYTES, type: "image/png" })); + const response = await routeGet("u/octocat", { session: null }); + expect(response.status).toBe(401); + expect(upstreamRequests).toHaveLength(0); + }); + + it("refuses a session the control plane does not recognise", async () => { + await serveControlPlane(); + await serveAvatar(() => ({ status: 200, body: PNG_BYTES, type: "image/png" })); + const response = await routeGet("u/octocat", { session: "forged-or-expired" }); + expect(response.status).toBe(401); + expect(upstreamRequests).toHaveLength(0); + }); + + it("refuses every target while unauthenticated, however many are tried", async () => { + await serveControlPlane(); + await serveAvatar(() => ({ status: 200, body: PNG_BYTES, type: "image/png" })); + for (let i = 0; i < 25; i += 1) { + const response = await routeGet(`u/probe${i}`, { session: null }); + expect(response.status).toBe(401); + } + expect(upstreamRequests).toHaveLength(0); + }); + + it("does not re-ask the control plane about a token it has already judged", async () => { + const controlPlane = await serveControlPlane(); + await serveAvatar(() => ({ status: 200, body: PNG_BYTES, type: "image/png" })); + for (const login of ["octocat", "hubot", "mona"]) await routeGet(`u/${login}`); + expect(controlPlane.requests).toBe(1); + expect(upstreamRequests).toHaveLength(3); + }); + + it("denies the request when the control plane cannot answer", async () => { + await serveControlPlane({ status: 503 }); + await serveAvatar(() => ({ status: 200, body: PNG_BYTES, type: "image/png" })); + const response = await routeGet("u/octocat"); + expect(response.status).toBe(401); + expect(upstreamRequests).toHaveLength(0); + }); +}); + +describe("bounding how much GitHub traffic a viewer can cause", () => { + it("serves a repeated target from the server-side cache, fetching once", async () => { + await serveControlPlane(); + await serveAvatar(() => ({ status: 200, body: PNG_BYTES, type: "image/png" })); + for (let i = 0; i < 10; i += 1) { + const response = await routeGet("u/octocat"); + expect(response.status).toBe(200); + expect([...new Uint8Array(await response.arrayBuffer())]).toEqual([...PNG_BYTES]); + } + expect(upstreamRequests).toHaveLength(1); + }); + + it("remembers an upstream miss instead of refetching it on every paint", async () => { + await serveControlPlane(); + await serveAvatar(() => ({ status: 404 })); + for (let i = 0; i < 10; i += 1) { + expect((await routeGet("u/ghost")).status).toBe(404); + } + expect(upstreamRequests).toHaveLength(1); + }); + + it("caps a viewer cycling fresh logins, and stops fetching once capped", async () => { + await serveControlPlane(); + await serveAvatar(() => ({ status: 200, body: PNG_BYTES, type: "image/png" })); + const statuses: number[] = []; + for (let i = 0; i < 90; i += 1) { + statuses.push((await routeGet(`u/probe${i}`)).status); + } + expect(statuses.filter((status) => status === 429).length).toBeGreaterThan(0); + // The allowance, not the request count, decides how much egress happens. + expect(upstreamRequests.length).toBeLessThan(90); + expect(upstreamRequests.length).toBe(statuses.filter((status) => status === 200).length); + }); +}); + +describe("following an upstream redirect", () => { + it("follows a hop that stays on a permitted GitHub host", async () => { + await serveControlPlane(); + await serveAvatar((request) => + request.url.startsWith("https://github.com/") + ? { + status: 302, + headers: { location: "https://avatars.githubusercontent.com/u/583231?v=4" }, + } + : { status: 200, body: PNG_BYTES, type: "image/png" }, + ); + const response = await routeGet("u/octocat"); + expect(response.status).toBe(200); + expect([...new Uint8Array(await response.arrayBuffer())]).toEqual([...PNG_BYTES]); + expect(upstreamRequests.map((r) => r.url)).toEqual([ + "https://github.com/octocat.png?size=40", + "https://avatars.githubusercontent.com/u/583231?v=4", + ]); + }); + + it("refuses a hop to a host outside GitHub and never requests it", async () => { + await serveControlPlane(); + let elsewhereRequests = 0; + const elsewhere = await startUpstream((request, response) => { + request.resume(); + elsewhereRequests += 1; + response.writeHead(200, { "content-type": "image/png" }); + response.end(Buffer.from([0xde, 0xad, 0xbe, 0xef])); + }); + await serveAvatar(() => ({ status: 302, headers: { location: `${elsewhere}/evil.png` } })); + + const response = await routeGet("u/octocat"); + expect(response.status).toBe(404); + expect(elsewhereRequests).toBe(0); + expect(upstreamRequests).toHaveLength(1); + }); + + it("refuses an off-host hop however the Location is written", async () => { + for (const location of [ + "https://evil.example/x.png", + "//evil.example/x.png", + "http://github.com/octocat.png", + "https://github.com.evil.example/x.png", + ]) { + await serveControlPlane(); + await serveAvatar(() => ({ status: 302, headers: { location } })); + const response = await routeGet("u/octocat"); + expect(response.status, location).toBe(404); + expect(upstreamRequests, location).toHaveLength(1); + + upstreamRequests = []; + resetAvatarGate(); + for (const undo of cleanup.splice(0).reverse()) await undo(); + } + }); + + it("cuts off an upstream that redirects without end", async () => { + await serveControlPlane(); + await serveAvatar(() => ({ + status: 302, + headers: { location: "https://avatars.githubusercontent.com/u/1?v=4" }, + })); + const response = await routeGet("u/octocat"); + expect(response.status).toBe(404); + // One initial request plus a fixed number of hops — never the upstream's choice. + expect(upstreamRequests.length).toBeLessThanOrEqual(4); + expect(upstreamRequests.length).toBeGreaterThan(1); + }); + + it("treats a redirect with no Location as a failure", async () => { + await serveControlPlane(); + await serveAvatar(() => ({ status: 302 })); + expect((await routeGet("u/octocat")).status).toBe(404); + expect(upstreamRequests).toHaveLength(1); + }); +});