Public: anyone can read this; the crew can change it.
GET /api/v1/repos/:owner/:name/hooks and …/hooks/:id/deliveries carry an ETag and answer a repeat with 304 until a hook or a delivery changes #9173
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/9169-get-api-v1-repos-owner-name-hooks-and-ho → main 3 commits fork 3 threads · 3 unresolved
2 files
changed +178
−4
src/http/api/hooks.ts +20 −4
| @@ -10,11 +10,25 @@ import { createWebhook, deleteWebhook } from "../../actions/repos.ts"; | ||
| 10 | 10 | import { canAdmin } from "../../core/repos.ts"; |
| 11 | 11 | import { notePhase, takeSessionTime, timePhase } from "../../core/route_timing.ts"; |
| 12 | 12 | import { getWebhook, listWebhooks, pingWebhook, recentDeliveries } from "../../core/webhooks.ts"; |
| 13 | import { loadRepo, pageOf, pageParams } from "../api.ts"; | |
| 14 | import type { Handler } from "../context.ts"; | |
| 13 | import { loadRepo, pageParams, slicePage } from "../api.ts"; | |
| 14 | import type { Ctx, Handler } from "../context.ts"; | |
| 15 | import { holds, weakEtag } from "../etag.ts"; | |
| 15 | 16 | import { body, json, notFound, requireScope, respondAccess, str } from "../respond.ts"; |
| 16 | 17 | import type { Router } from "../router.ts"; |
| 17 | 18 | |
| 19 | /** | |
| 20 | * An admin's answer, tagged by its own bytes: `private, no-cache` and varied | |
| 21 | * on the credential, so a repeat holding the tag is a bodiless 304. Called | |
| 22 | * only after the admin check, so a stranger never reaches a 304. | |
| 23 | */ | |
| 24 | function tagged(ctx: Ctx, data: unknown): Response { | |
| 25 | const text = JSON.stringify(data); | |
| 26 | const etag = weakEtag(text); | |
| 27 | const headers = { etag, "cache-control": "private, no-cache", vary: "authorization, cookie" }; | |
| 28 | if (holds(ctx.request, etag)) return new Response(null, { status: 304, headers }); | |
| 29 | return new Response(text, { headers: { "content-type": "application/json", ...headers } }); | |
| 30 | } | |
| 31 | ||
| 18 | 32 | export function hookRoutes(router: Router<Handler>): void { |
| 19 | 33 | router.add("GET", "/api/v1/repos/:owner/:name/hooks", (ctx) => { |
| 20 | 34 | notePhase(ctx.request, "session", takeSessionTime(ctx.request)); |
| @@ -25,7 +39,9 @@ export function hookRoutes(router: Router<Handler>): void { | ||
| 25 | 39 | const page = pageParams(ctx); |
| 26 | 40 | if (page instanceof Response) return page; |
| 27 | 41 | const rows = timePhase(ctx.request, "hooks", () => listWebhooks(ctx.app.db, repo.id)); |
| 28 | return timePhase(ctx.request, "render", () => pageOf(rows, page)); | |
| 42 | const sliced = slicePage(rows, page); | |
| 43 | if (sliced instanceof Response) return sliced; | |
| 44 | return timePhase(ctx.request, "render", () => tagged(ctx, sliced)); | |
| 29 | 45 | }); |
| 30 | 46 | |
| 31 | 47 | router.add("GET", "/api/v1/repos/:owner/:name/hooks/:id/deliveries", (ctx) => { |
| @@ -35,7 +51,7 @@ export function hookRoutes(router: Router<Handler>): void { | ||
| 35 | 51 | const repo = loadRepo(ctx); |
| 36 | 52 | if (!repo || !canAdmin(ctx.app.db, repo, ctx.auth.user)) return notFound(); |
| 37 | 53 | if (!getWebhook(ctx.app.db, repo.id, ctx.params.id!)) return notFound(); |
| 38 | return json({ items: recentDeliveries(ctx.app.db, ctx.params.id!, 20) }); | |
| 54 | return tagged(ctx, { items: recentDeliveries(ctx.app.db, ctx.params.id!, 20) }); | |
| 39 | 55 | }); |
| 40 | 56 | |
| 41 | 57 | router.add("POST", "/api/v1/repos/:owner/:name/hooks/:id/ping", (ctx) => { |
test/hooks-etag.test.ts added +158
| @@ -0,0 +1,158 @@ | ||
| 1 | /** GET …/hooks and …/hooks/:id/deliveries carry an ETag and answer a repeat with 304 (#9169). */ | |
| 2 | import { afterAll, beforeAll, expect, test } from "bun:test"; | |
| 3 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 4 | import { tmpdir } from "node:os"; | |
| 5 | import { join } from "node:path"; | |
| 6 | import { createApp, type App } from "../src/app.ts"; | |
| 7 | import { createRepo } from "../src/actions/repos.ts"; | |
| 8 | import { createToken } from "../src/core/tokens.ts"; | |
| 9 | import { createUser } from "../src/core/users.ts"; | |
| 10 | import { createWebhook } from "../src/core/webhooks.ts"; | |
| 11 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 12 | import { quietLogs } from "./helpers/globals.ts"; | |
| 13 | ||
| 14 | quietLogs(); | |
| 15 | ||
| 16 | let dataDir: string; | |
| 17 | let app: App; | |
| 18 | let servers: Servers; | |
| 19 | let adminTok = ""; | |
| 20 | let readTok = ""; | |
| 21 | let otherTok = ""; | |
| 22 | let hookA = ""; | |
| 23 | let hookB = ""; | |
| 24 | const cells = process.env["FORGE_CELLS"]; | |
| 25 | const HOOKS = "/api/v1/repos/alice/a/hooks"; | |
| 26 | const get = (path: string, tok: string, headers: Record<string, string> = {}) => | |
| 27 | fetch(`${servers.url}${path}`, { headers: { authorization: `Bearer ${tok}`, ...headers } }); | |
| 28 | const ping = (id: string) => | |
| 29 | fetch(`${servers.url}${HOOKS}/${id}/ping`, { method: "POST", headers: { authorization: `Bearer ${adminTok}` } }); | |
| 30 | const tagOf = async (path: string) => { | |
| 31 | const res = await get(path, adminTok); | |
| 32 | expect(res.status).toBe(200); | |
| 33 | await res.text(); | |
| 34 | return res.headers.get("etag")!; | |
| 35 | }; | |
| 36 | ||
| 37 | beforeAll(async () => { | |
| 38 | process.env["FORGE_CELLS"] = "off"; | |
| 39 | dataDir = await mkdtemp(join(tmpdir(), "forge-hooks-etag-")); | |
| 40 | app = createApp({ dataDir, port: 0 }); | |
| 41 | servers = await serve(app); | |
| 42 | const mk = async (username: string, first: boolean) => { | |
| 43 | const made = await createUser(app.db, { | |
| 44 | origin: "seed", | |
| 45 | trusted: true, | |
| 46 | username, | |
| 47 | email: `${username}@forge.local`, | |
| 48 | password: "a-good-passphrase", | |
| 49 | adminIfFirst: first, | |
| 50 | }); | |
| 51 | if (!made.ok) throw new Error(String(made.error)); | |
| 52 | return made.value; | |
| 53 | }; | |
| 54 | const alice = await mk("alice", true); | |
| 55 | const bob = await mk("bob", false); | |
| 56 | const ra = await createRepo(app, alice, { name: "a", visibility: "public" }); | |
| 57 | if (!ra.ok) throw new Error("repo"); | |
| 58 | const a = createWebhook(app.db, { repoId: ra.value.id, url: "https://a.example/h" }); | |
| 59 | const b = createWebhook(app.db, { repoId: ra.value.id, url: "https://b.example/h" }); | |
| 60 | if (!a.ok || !b.ok) throw new Error("hook"); | |
| 61 | hookA = a.value.webhook.id; | |
| 62 | hookB = b.value.webhook.id; | |
| 63 | adminTok = createToken(app.db, alice.id, "t", "admin").token; | |
| 64 | readTok = createToken(app.db, alice.id, "r", "ro").token; | |
| 65 | otherTok = createToken(app.db, bob.id, "o", "admin").token; | |
| 66 | }); | |
| 67 | afterAll(async () => { | |
| 68 | await servers.stop(); | |
| 69 | app.close(); | |
| 70 | if (cells === undefined) delete process.env["FORGE_CELLS"]; | |
| 71 | else process.env["FORGE_CELLS"] = cells; | |
| 72 | await rm(dataDir, { recursive: true, force: true }); | |
| 73 | }); | |
| 74 | ||
| 75 | const hooksUrl = () => HOOKS; | |
| 76 | const deliveriesUrl = () => `${HOOKS}/${hookA}/deliveries`; | |
| 77 | ||
| 78 | function sharedTests(label: string, url: () => string, strictNotFound: boolean) { | |
| 79 | test(`${label}: a weak tag, private no-cache, vary`, async () => { | |
| 80 | const res = await get(url(), adminTok); | |
| 81 | expect(res.status).toBe(200); | |
| 82 | expect(res.headers.get("etag")).toMatch(/^W\/"/); | |
| 83 | expect(res.headers.get("cache-control")).toBe("private, no-cache"); | |
| 84 | expect(res.headers.get("vary")).toContain("authorization, cookie"); | |
| 85 | await res.text(); | |
| 86 | }); | |
| 87 | ||
| 88 | test(`${label}: a repeat is a bodiless 304 with the floor`, async () => { | |
outdated
|
||
| 89 | const tag = await tagOf(url()); | |
| 90 | const again = await get(url(), adminTok, { "if-none-match": tag }); | |
| 91 | expect(again.status).toBe(304); | |
| 92 | expect(again.headers.get("etag")).toBe(tag); | |
| 93 | expect(again.headers.get("cache-control")).toBe("private, no-cache"); | |
| 94 | expect(again.headers.get("vary")).toContain("authorization, cookie"); | |
| 95 | expect(again.headers.get("x-content-type-options")).toBe("nosniff"); | |
| 96 | expect(again.headers.get("referrer-policy")).not.toBeNull(); | |
| 97 | expect(await again.text()).toBe(""); | |
| 98 | }); | |
| 99 | ||
| 100 | test(`${label}: non-admins and strangers get no tag, even with *`, async () => { | |
| 101 | for (const tok of [readTok, otherTok]) { | |
| 102 | const res = await get(url(), tok, { "if-none-match": "*" }); | |
| 103 | expect(res.headers.get("etag")).toBeNull(); | |
| 104 | expect(res.status).not.toBe(304); | |
| 105 | expect(strictNotFound ? [404] : [403, 404]).toContain(res.status); | |
| 106 | await res.text(); | |
| 107 | } | |
| 108 | }); | |
| 109 | } | |
| 110 | ||
| 111 | sharedTests("hooks", hooksUrl, false); | |
| 112 | sharedTests("deliveries", deliveriesUrl, true); | |
| 113 | ||
| 114 | test("a missing hook is 404 without a tag, even with *", async () => { | |
| 115 | const res = await get(`${HOOKS}/nope/deliveries`, adminTok, { "if-none-match": "*" }); | |
| 116 | expect(res.status).toBe(404); | |
| 117 | expect(res.headers.get("etag")).toBeNull(); | |
| 118 | await res.text(); | |
| 119 | }); | |
| 120 | ||
| 121 | test("adding and removing a hook moves the hooks tag", async () => { | |
| 122 | let tag = await tagOf(HOOKS); | |
| 123 | const made = await fetch(`${servers.url}${HOOKS}`, { | |
| 124 | method: "POST", | |
| 125 | headers: { authorization: `Bearer ${adminTok}`, "content-type": "application/json" }, | |
| 126 | body: JSON.stringify({ url: "https://c.example/h" }), | |
| 127 | }); | |
| 128 | expect(made.status).toBe(201); | |
| 129 | const id = ((await made.json()) as { id: string }).id; | |
| 130 | const res = await get(HOOKS, adminTok, { "if-none-match": tag }); | |
| 131 | expect(res.status).toBe(200); | |
| 132 | expect(res.headers.get("etag")).not.toBe(tag); | |
| 133 | tag = res.headers.get("etag")!; | |
| 134 | await res.text(); | |
| 135 | const del = await fetch(`${servers.url}${HOOKS}/${id}`, { method: "DELETE", headers: { authorization: `Bearer ${adminTok}` } }); | |
| 136 | expect(del.status).toBe(200); | |
| 137 | const after = await get(HOOKS, adminTok, { "if-none-match": tag }); | |
| 138 | expect(after.status).toBe(200); | |
| 139 | expect(after.headers.get("etag")).not.toBe(tag); | |
| 140 | await after.text(); | |
outdated
Race: this assumes the ping has recorded its delivery before the 202 returns. If delivery is recorded asynchronously the assertion depends on timing. Poll until the tag moves, with a bounded wait. |
||
| 141 | }); | |
| 142 | ||
| 143 | test("a ping moves that hook's deliveries tag and not the other's", async () => { | |
| 144 | const a = await tagOf(`${HOOKS}/${hookA}/deliveries`); | |
| 145 | const b = await tagOf(`${HOOKS}/${hookB}/deliveries`); | |
| 146 | expect((await ping(hookA)).status).toBe(202); | |
| 147 | // Poll, bounded, so the test never depends on when the row lands. | |
| 148 | let moved = await get(`${HOOKS}/${hookA}/deliveries`, adminTok, { "if-none-match": a }); | |
| 149 | for (let i = 0; i < 50 && moved.status === 304; i++) { | |
| 150 | await Bun.sleep(20); | |
| 151 | moved = await get(`${HOOKS}/${hookA}/deliveries`, adminTok, { "if-none-match": a }); | |
| 152 | } | |
| 153 | expect(moved.status).toBe(200); | |
| 154 | expect(moved.headers.get("etag")).not.toBe(a); | |
| 155 | await moved.text(); | |
| 156 | const still = await get(`${HOOKS}/${hookB}/deliveries`, adminTok, { "if-none-match": b }); | |
| 157 | expect(still.status).toBe(304); | |
| 158 | }); |
new above
offline
paged.clone().text()clones and reads a Response just to get text thatpageOfhad already built. It also drops any non-200 handling by passing through only on status. Prefer building the page body once (slicePage) and tagging that text directly, as the deliveries route does.