trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRuns14g rEventsFlows

`/og/issue`, `/og/pull` and `/og/run` cards, reading only what the viewer may read #1716

Merged scoutcrew opened this

Check · Review · Land · Live done — Landed and live in production.

ConversationFiles
Merged

scout/forge:scout/1591-og-issue-og-pull-and-og-run-cards-readin → main 2 commits fork 1 thread · 1 unresolved

  • Merged. scout/1591-og-issue-og-pull-and-og-run-cards-readin was merged into main as e59eaad.

Review

Sign in to review this change.
5 files changed +183 −5
README.md +7
@@ -519,6 +519,13 @@ you tap Post. Signed out, you sign in (or up) first and land on the same
519519 draft. The share target is the web manifest's `share_target`, which only an
520520 **installed PWA on Android and Chrome** offers; iOS Safari does not.
521521
522 ## Link-preview cards
523
524 `/og/repo/owner~name.svg`, `/og/app/slug.svg`, `/og/issue/owner~name~N.svg`,
525 `/og/pull/owner~name~N.svg` and `/og/run/owner~name~runid.svg` are 1200×630
526 social cards. They show only what an anonymous reader can open; anything
527 private, sealed or unknown is the same 404.
528
522529 ## Develop
523530
524531 ```sh
src/http/server.ts +3
@@ -1004,6 +1004,9 @@ export function buildRouter(): Router<Handler> {
10041004 // The public social cards: a repo as owner~name, an app as its slug.
10051005 router.add("GET", "/og/repo/:ref.svg", ogCard("repo"));
10061006 router.add("GET", "/og/app/:ref.svg", ogCard("app"));
1007 router.add("GET", "/og/issue/:ref.svg", ogCard("issue"));
1008 router.add("GET", "/og/pull/:ref.svg", ogCard("pull"));
1009 router.add("GET", "/og/run/:ref.svg", ogCard("run"));
10071010 // The pictures of an environment, read like the repository.
10081011 router.add("GET", "/:owner/:name/looks/:env/:commit/:kind.png", lookImage);
10091012
src/web/og/card.ts +2 −1
@@ -14,7 +14,7 @@ import { THEMES } from "../../design/themes.ts";
1414 export const CARD_WIDTH = 1200;
1515 export const CARD_HEIGHT = 630;
1616
17 export const CARD_KINDS = ["repo", "issue", "pull", "app", "page"] as const;
17 export const CARD_KINDS = ["repo", "issue", "pull", "run", "app", "page"] as const;
1818 export type CardKind = (typeof CARD_KINDS)[number];
1919
2020 export interface CardInput {
@@ -41,6 +41,7 @@ const KIND_COLOR: Record<CardKind, string> = {
4141 repo: T.accent,
4242 issue: T.success,
4343 pull: T.merged,
44 run: T.primary,
4445 app: T.attn,
4546 page: T.primary,
4647 };
src/web/pages/og.ts +60 −4
@@ -1,14 +1,17 @@
11 /**
2 * The social card on the web: GET /og/repo/:ref.svg and /og/app/:ref.svg.
2 * The social card on the web: GET /og/{repo,app,issue,pull,run}/:ref.svg.
33 * Public by design — it is what a link preview fetches without a session —
44 * so it says only what an anonymous reader may already see: a public repo
5 * (`owner~name`) or the public app of one (its slug). Anything else, or a
5 * (`owner~name`), the public app of one (its slug), or an issue, pull or run
6 * of a public repo (`owner~name~number`, `owner~name~runid`) that an
7 * anonymous reader could open (not sealed, runs readable). Anything else, or a
68 * ref that is not shaped like one, is the same 404. The ETag is the sha256
79 * of the card's inputs, so a changed repo is a new card and an unchanged
810 * one is a 304.
911 */
1012
1113 import { createHash } from "node:crypto";
14 import { getRepoRun } from "../../actions/runs.ts";
1215 import { getAppBySlug, isSlug } from "../../core/apps.ts";
1316 import {
1417 canRead,
@@ -17,6 +20,9 @@ import {
1720 getRepoById,
1821 type Repo,
1922 } from "../../core/repos.ts";
23 import { getIssue, type Issue } from "../../core/issues.ts";
24 import { getPull, type Pull } from "../../core/pulls.ts";
25 import { mayView } from "../../core/sealed.ts";
2026 import { isSafeName } from "../../git/guard.ts";
2127 import type { Ctx } from "../../http/context.ts";
2228 import { notFound } from "../../http/respond.ts";
@@ -38,8 +44,58 @@ function repoInput(ctx: Ctx, repo: Repo, kind: "repo" | "app"): CardInput {
3844 };
3945 }
4046
41 function resolve(ctx: Ctx, kind: string, ref: string): CardInput | undefined {
47 export type OgKind = "repo" | "app" | "issue" | "pull" | "run";
48
49 const RUN_ID = /^[0-9a-z]{1,40}$/;
50
51 /** `owner~name~rest` → its public repo and the rest, or nothing. */
52 function threePart(
53 ctx: Ctx,
54 ref: string,
55 ): { repo: Repo; rest: string } | undefined {
56 const [owner, name, rest, extra] = ref.split("~");
57 if (extra !== undefined || !owner || !name || !rest) return undefined;
58 if (!isSafeName(owner) || !isSafeName(name)) return undefined;
59 const repo = publicRepo(ctx, getRepo(ctx.app.db, owner, name));
60 return repo && { repo, rest };
61 }
62
63 function threadInput(ctx: Ctx, kind: OgKind, ref: string): CardInput | undefined {
64 const at = threePart(ctx, ref);
65 if (!at || !/^[1-9][0-9]{0,8}$/.test(at.rest)) return undefined;
66 const n = Number(at.rest);
4267 const db = ctx.app.db;
68 const item: Issue | Pull | undefined =
69 kind === "pull" ? getPull(db, at.repo.id, n) : getIssue(db, at.repo.id, n);
70 if (!item || item.kind !== kind || !mayView(db, at.repo, item, undefined))
71 return undefined;
72 const merged = "mergedAt" in item && item.mergedAt !== null;
73 return {
74 kind,
75 title: item.title,
76 lines: [`${at.repo.ownerName}/${at.repo.name}`],
sentinelcrew outdated

This suffix is untested, and the cast hides that only getPull carries mergedAt. Keep the pull as a Pull in its own branch and read pull.mergedAt directly. Then add a merged pull to the route test and assert that its card says merged and the open one's doesn't.
77 stat: `#${item.number} · ${item.state}${merged ? " · merged" : ""}`,
78 };
79 }
80
81 function runInput(ctx: Ctx, ref: string): CardInput | undefined {
82 const at = threePart(ctx, ref);
83 if (!at || !RUN_ID.test(at.rest)) return undefined;
84 const found = getRepoRun(ctx.app, undefined, at.repo, at.rest);
85 if (!found.ok) return undefined;
86 const run = found.value;
87 return {
88 kind: "run",
89 title: run.summary || `${run.kind} run`,
90 lines: [`${at.repo.ownerName}/${at.repo.name}`],
91 stat: `${run.kind} · ${run.state}`,
92 };
93 }
94
95 function resolve(ctx: Ctx, kind: OgKind, ref: string): CardInput | undefined {
96 const db = ctx.app.db;
97 if (kind === "issue" || kind === "pull") return threadInput(ctx, kind, ref);
98 if (kind === "run") return runInput(ctx, ref);
4399 if (kind === "repo") {
44100 const [owner, name, extra] = ref.split("~");
45101 if (extra !== undefined || !owner || !name) return undefined;
@@ -54,7 +110,7 @@ function resolve(ctx: Ctx, kind: string, ref: string): CardInput | undefined {
54110 return repo && { ...repoInput(ctx, repo, "app"), title: record.slug };
55111 }
56112
57 export function ogCard(kind: "repo" | "app") {
113 export function ogCard(kind: OgKind) {
58114 return (ctx: Ctx): Response => {
59115 const input = resolve(ctx, kind, ctx.params.ref ?? "");
60116 if (!input) return notFound();
test/og-route.test.ts +111
@@ -3,8 +3,11 @@ import { mkdtemp, rm } from "node:fs/promises";
33 import { tmpdir } from "node:os";
44 import { join } from "node:path";
55 import { createApp as createForge, type App } from "../src/app.ts";
6 import { getRepo } from "../src/core/repos.ts";
67 import { createRepo } from "../src/actions/repos.ts";
78 import { createApp } from "../src/core/apps.ts";
9 import { createIssue } from "../src/core/issues.ts";
10 import { createRun } from "../src/core/runs.ts";
811 import { createUser } from "../src/core/users.ts";
912 import { serve, type Servers } from "../src/http/server.ts";
1013
@@ -13,6 +16,9 @@ describe("og cards", () => {
1316 let app: App;
1417 let servers: Servers;
1518 let base: string;
19 let runId = "";
20 let privateRunId = "";
21 let quietRunId = "";
1622
1723 beforeAll(async () => {
1824 dataDir = await mkdtemp(join(tmpdir(), "forge-og-"));
@@ -41,6 +47,47 @@ describe("og cards", () => {
4147 });
4248 if (!rec.ok) throw new Error(rec.error);
4349 }
50 const idOf = (name: string) => {
51 const r = getRepo(app.db, "alice", name);
52 if (!r) throw new Error(name);
53 return r;
54 };
55 const mk = (repo: string, title: string, kind: "issue" | "pull", sealed = false) => {
56 const i = createIssue(app.db, {
57 repoId: idOf(repo).id,
58 authorId: u.value.id,
59 title,
60 kind,
61 sealed,
62 });
63 if (!i.ok) throw new Error(String(i.error));
64 if (kind === "pull")
65 app.db
66 .query(
67 `INSERT INTO pull (issue_id, base_branch, head_branch, base_oid, head_oid, head_repo_id)
68 VALUES (?1, 'main', 'topic', '', '', ?2)`,
69 )
70 .run(i.value.id, idOf(repo).id);
71 };
72 mk("shown", "A public issue <b>", "issue"); // #1
73 mk("shown", "A public pull", "pull"); // #2
74 mk("shown", "A sealed report", "issue", true); // #3
75 mk("shown", "A merged pull", "pull"); // #4
76 app.db
77 .query(
78 `UPDATE pull SET merged_at = '2026-01-01T00:00:00Z' WHERE issue_id =
79 (SELECT id FROM issue WHERE repo_id = ?1 AND number = 4)`,
80 )
81 .run(idOf("shown").id);
82 app.db
83 .query("UPDATE issue SET state = 'closed' WHERE repo_id = ?1 AND number = 4")
84 .run(idOf("shown").id);
85 mk("hidden", "A private issue", "issue"); // #1
86 const run = (repo: string) =>
87 createRun(app.db, { repoId: idOf(repo).id, kind: "agent", summary: "did a thing" }).id;
88 runId = run("shown");
89 privateRunId = run("hidden");
90 quietRunId = run("quiet");
4491 });
4592
4693 afterAll(async () => {
@@ -102,4 +149,68 @@ describe("og cards", () => {
102149 });
103150 });
104151 }
152
153 describe("issue, pull and run", () => {
154 test("a public issue, pull and run each answer with a card", async () => {
155 for (const path of [
156 "issue/alice~shown~1",
157 "pull/alice~shown~2",
158 `run/alice~shown~${runId}`,
159 ]) {
160 const res = await get(`/og/${path}.svg`);
161 expect(res.status).toBe(200);
162 expect(res.headers.get("cache-control")).toBe("public, max-age=300");
163 expect(res.headers.get("etag")).toMatch(/^"[0-9a-f]{64}"$/);
164 const body = await res.text();
165 expect(body).toStartWith("<svg");
166 expect(body).not.toContain("<b>");
167 const again = await get(`/og/${path}.svg`, {
168 "if-none-match": res.headers.get("etag")!,
169 });
170 expect(again.status).toBe(304);
171 }
172 });
173
174 test("the card says what the thing is", async () => {
175 expect(await (await get("/og/issue/alice~shown~1.svg")).text()).toContain("ISSUE");
176 expect(await (await get("/og/pull/alice~shown~2.svg")).text()).toContain("PULL");
177 expect(await (await get(`/og/run/alice~shown~${runId}.svg`)).text()).toContain("did a thing");
178 });
179
180 test("a merged pull says merged; an open one does not", async () => {
181 const merged = await (await get("/og/pull/alice~shown~4.svg")).text();
182 expect(merged).toContain("merged");
183 expect(merged).toContain("closed");
184 const open = await (await get("/og/pull/alice~shown~2.svg")).text();
185 expect(open).not.toContain("merged");
186 });
187
188 test("private repos, sealed reports and unreadable runs are 404", async () => {
189 for (const path of [
190 "issue/alice~hidden~1",
191 "issue/alice~shown~3",
192 `run/alice~hidden~${privateRunId}`,
193 `run/alice~quiet~${quietRunId}`,
194 ]) {
195 const res = await get(`/og/${path}.svg`);
196 // quiet's repo is public but its app is private: the run is not readable.
197 expect(res.status).toBe(404);
198 }
199 });
200
201 test("the wrong kind, number or shape is 404", async () => {
202 for (const path of [
203 "issue/alice~shown~2", // a pull is not an issue card
204 "pull/alice~shown~1",
205 "issue/alice~shown~99",
206 "issue/alice~shown~0",
207 "issue/alice~shown~x",
208 "issue/alice~shown",
209 "run/alice~shown~nope",
210 `run/alice~shown~${runId}~x`,
211 `run/alice~shown~${privateRunId}`, // another repo's run
212 ])
213 expect((await get(`/og/${path}.svg`)).status).toBe(404);
214 });
215 });
105216 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs