Refuse webhooks to internal addresses or Diffinery itself, and cap them at 20 (#96)
Creating a webhook now needs https to a public host that isn't Diffinery's own, delivery re-checks the URL (hooks saved earlier are never sent to internal addresses), and a repository can have at most 20 webhooks. A trailing dot (localhost.) no longer slips past the importer's and webhooks' host checks. Claude-Session: https://claude.ai/code/session_016yhgAu6oKfqes812Yd4qjo Co-authored-by: Claude <noreply@anthropic.com>
Canberk Demirci authored · GitHub committed signed1 parent e256545commit bb177786c3d348142d7614ea3b3f7bd9a97cd1adBrowse files
10 files changed+60−9
Files (10)
- apps/diffinery/docs/reference/limits.md+2−1
- apps/diffinery/docs/repositories/webhooks.md+2−2
- apps/diffinery/src/app.ts+1−0
- apps/diffinery/src/imports/options.ts+4−2
- apps/diffinery/src/lib/webhooks.ts+10−0
- apps/diffinery/src/repo/RepoDO.ts+2−2
- apps/diffinery/src/repo/collab.ts+10−2
- apps/diffinery/test/e2e/security.test.mjs+13−0
- apps/diffinery/test/unit/imports.test.ts+6−0
- apps/diffinery/test/unit/webhooks.test.ts+10−0
modifiedapps/diffinery/docs/reference/limits.md+2−1View file
| @@ -300,7 +300,8 @@ | ||
| 300 | 300 | |
| 301 | 301 | | Limit | Value | Applies to | |
| 302 | 302 | | --- | --- | --- | |
| 303 | -| Webhook URL | `https://` only | Other schemes fail with `webhook URL must use https`. | | |
| 303 | +| Webhook URL | `https://` to a public host | Other schemes fail with `webhook URL must use https`. `localhost`, single-label names, `.local` and `.internal` names, and private, loopback and link-local addresses fail with `webhook URL must be a public host`; Diffinery's own host and its subdomains fail with `webhook URL can't point at Diffinery itself`. A webhook saved before these checks that points at such an address is never delivered to (recorded status `0`). | | |
| 304 | +| Webhooks per repository | 20 | The 21st fails with `422` and `a repository can have at most 20 webhooks`, as on GitHub. | | |
| 304 | 305 | | Delivery timeout | 10 seconds | A receiver that takes longer counts as failed (recorded status `0`). | |
| 305 | 306 | | Redirects | Not followed | A `3xx` answer is recorded as the delivery's status. | |
| 306 | 307 | | Failed deliveries | Retried up to 5 times | A network error, a timeout, `408`, `429` or a `5xx` status is retried 1 minute, 5 minutes, 30 minutes, 1 hour and 3 hours after successive failures (6 attempts at most, about 4 hours and 36 minutes in all), with the same `X-GitHub-Delivery` ID. Other statuses, including `3xx` and other `4xx`, are not retried. A retry is dropped if the webhook was deleted. Each attempt updates the last status and time shown on the webhook. See [Retries](/docs/repositories/webhooks#retries). | |
modifiedapps/diffinery/docs/repositories/webhooks.md+2−2View file
| @@ -40,7 +40,7 @@ | ||
| 40 | 40 | |
| 41 | 41 | 1. Open the repository, for example `https://diffinery.com/acme/website`. |
| 42 | 42 | 2. Select **Settings**, then **Webhooks** in the sidebar (`https://diffinery.com/acme/website/settings/webhooks`). |
| 43 | -3. Under **Add a webhook**, enter the **Payload URL**. It must start with `https://`. | |
| 43 | +3. Under **Add a webhook**, enter the **Payload URL**. It must start with `https://` and name a public host: not `localhost`, a private or loopback address, or Diffinery itself. | |
| 44 | 44 | 4. Under **Events**, keep **Everything** checked to receive all events, or uncheck it and check the events you want: `push`, `pull_request`, `pull_request_review`, `issues`, `issue_comment`, `workflow_run`, `workflow_dispatch`. If **Everything** stays checked, the webhook receives every event regardless of the other boxes, including `release`, `status`, `deployment`, `deployment_status` and `repository_dispatch`. If you uncheck every box, the webhook also receives every event. |
| 45 | 45 | 5. Optionally enter a **Secret**. Leave it empty to have Diffinery generate one. |
| 46 | 46 | 6. Select **Add webhook**. |
| @@ -66,7 +66,7 @@ | ||
| 66 | 66 | |
| 67 | 67 | | Field | Required | Description | |
| 68 | 68 | |---|---|---| |
| 69 | -| `config.url` | yes | The payload URL. Must start with `https://`, otherwise the request fails with `422` and `webhook URL must use https`. | | |
| 69 | +| `config.url` | yes | The payload URL. Must start with `https://`, otherwise the request fails with `422` and `webhook URL must use https`. It must name a public host: `localhost`, private and loopback addresses fail with `webhook URL must be a public host`, and Diffinery's own host with `webhook URL can't point at Diffinery itself`. A repository can have at most 20 webhooks. | | |
| 70 | 70 | | `config.secret` | no | The signing secret. When omitted, Diffinery generates a random one. | |
| 71 | 71 | | `events` | no | Event names to subscribe to. Omitted, empty or not a list means `["*"]` (every event). | |
| 72 | 72 |
modifiedapps/diffinery/src/app.ts+1−0View file
| @@ -149,6 +149,7 @@ | ||
| 149 | 149 | } |
| 150 | 150 | |
| 151 | 151 | const webhookDeps = (env: Env): WebhookDeps => ({ |
| 152 | + ownHost: new URL(env.PUBLIC_URL).hostname, | |
| 152 | 153 | hook: async (repoId, hookId) => (await repoStub(env, repoId).webhooks()).find((h) => h.id === hookId && h.active) ?? null, |
| 153 | 154 | record: (repoId, hookId, status) => repoStub(env, repoId).recordDelivery(hookId, status), |
| 154 | 155 | requeue: async (r, delaySeconds) => { |
modifiedapps/diffinery/src/imports/options.ts+4−2View file
| @@ -61,7 +61,7 @@ | ||
| 61 | 61 | * Hosts a server-side fetch may reach: HTTPS on a public name. Loopback and |
| 62 | 62 | * plain HTTP are allowed only in local development (`dev`), for test doubles. |
| 63 | 63 | */ |
| 64 | -export function checkPublicUrl(raw: string, dev: boolean, what = "URL"): URL { | |
| 64 | +export function checkPublicUrl(raw: string, dev: boolean, what = "URL", ownHost?: string): URL { | |
| 65 | 65 | let u: URL; |
| 66 | 66 | try { |
| 67 | 67 | u = new URL(raw); |
| @@ -71,9 +71,11 @@ | ||
| 71 | 71 | if (u.username || u.password) bad(`${what} must not contain credentials`); |
| 72 | 72 | if (dev && (u.protocol === "http:" || u.protocol === "https:")) return u; |
| 73 | 73 | if (u.protocol !== "https:") bad(`${what} must use https`); |
| 74 | - const host = u.hostname.toLowerCase().replace(/^\[|\]$/g, ""); | |
| 74 | + // A fully qualified name's trailing dot (`localhost.`) names the same host. | |
| 75 | + const host = u.hostname.toLowerCase().replace(/^\[|\]$/g, "").replace(/\.+$/, ""); | |
| 75 | 76 | const singleLabel = !host.includes(".") && !host.includes(":"); |
| 76 | 77 | if (singleLabel || host === "localhost" || /\.(localhost|local|internal)$/.test(host) || isPrivateAddress(host)) bad(`${what} must be a public host`); |
| 78 | + if (ownHost && (host === ownHost.toLowerCase() || host.endsWith(`.${ownHost.toLowerCase()}`))) bad(`${what} can't point at Diffinery itself`); | |
| 77 | 79 | return u; |
| 78 | 80 | } |
| 79 | 81 |
modifiedapps/diffinery/src/lib/webhooks.ts+10−0View file
| @@ -1,3 +1,4 @@ | ||
| 1 | +import { checkPublicUrl } from "../imports/options.js"; | |
| 1 | 2 | import { hmacSha256Hex } from "./util.js"; |
| 2 | 3 | |
| 3 | 4 | /** One webhook delivery; failed ones go back on the events queue with a delay. */ |
| @@ -16,6 +17,8 @@ | ||
| 16 | 17 | export interface WebhookDeps { |
| 17 | 18 | /** The hook as it is now (it may have been deleted, disabled or re-keyed), or null. */ |
| 18 | 19 | hook(repoId: string, hookId: string): Promise<{ id: string; url: string; secret: string } | null>; |
| 20 | + /** Diffinery's own host name: deliveries never go there. */ | |
| 21 | + ownHost?: string; | |
| 19 | 22 | record(repoId: string, hookId: string, status: number): Promise<void>; |
| 20 | 23 | requeue(r: WebhookRetry, delaySeconds: number): Promise<void>; |
| 21 | 24 | fetch?: typeof fetch; |
| @@ -31,6 +34,13 @@ | ||
| 31 | 34 | export async function deliverWebhook(deps: WebhookDeps, r: WebhookRetry, known?: { id: string; url: string; secret: string }): Promise<number | null> { |
| 32 | 35 | const h = known ?? (await deps.hook(r.repoId, r.hookId)); |
| 33 | 36 | if (!h) return null; |
| 37 | + // A hook saved before URLs were checked may point at an internal or Diffinery's own address: never deliver there. | |
| 38 | + try { | |
| 39 | + checkPublicUrl(h.url, false, "webhook URL", deps.ownHost); | |
| 40 | + } catch { | |
| 41 | + await deps.record(r.repoId, h.id, 0); | |
| 42 | + return 0; | |
| 43 | + } | |
| 34 | 44 | let status = 0; |
| 35 | 45 | try { |
| 36 | 46 | const res = await (deps.fetch ?? fetch)(h.url, { |
modifiedapps/diffinery/src/repo/RepoDO.ts+2−2View file
| @@ -904,8 +904,8 @@ | ||
| 904 | 904 | webhooks(...a: Parameters<Collab["webhooks"]>) { |
| 905 | 905 | return this.collab.webhooks(...a); |
| 906 | 906 | } |
| 907 | - createWebhook(...a: Parameters<Collab["createWebhook"]>) { | |
| 908 | - return this.collab.createWebhook(...a); | |
| 907 | + createWebhook(url: string, events: string[], secret?: string) { | |
| 908 | + return this.collab.createWebhook(url, events, secret, new URL(this.env.PUBLIC_URL).hostname); | |
| 909 | 909 | } |
| 910 | 910 | deleteWebhook(...a: Parameters<Collab["deleteWebhook"]>) { |
| 911 | 911 | return this.collab.deleteWebhook(...a); |
modifiedapps/diffinery/src/repo/collab.ts+10−2View file
| @@ -1,4 +1,5 @@ | ||
| 1 | 1 | import { ZERO_OID, isAncestor, mergeBase, mergeTrees, readCommit, serializeCommit, walkCommits } from "@diffinery/git-core"; |
| 2 | +import { checkPublicUrl } from "../imports/options.js"; | |
| 2 | 3 | import type { QueueEntry } from "./queue.js"; |
| 3 | 4 | import { HttpError, checkText, newId, now, randomToken } from "../lib/util.js"; |
| 4 | 5 | import type { RepoDO } from "./RepoDO.js"; |
| @@ -161,6 +162,9 @@ | ||
| 161 | 162 | |
| 162 | 163 | const actorOf = (c: Caller) => ({ id: c.id, login: c.login, kind: c.kind, onBehalfOf: c.onBehalfOf }); |
| 163 | 164 | |
| 165 | +/** Webhooks one repository may have, as on GitHub. */ | |
| 166 | +export const MAX_WEBHOOKS = 20; | |
| 167 | + | |
| 164 | 168 | export class Collab { |
| 165 | 169 | constructor(private repo: RepoDO) {} |
| 166 | 170 | |
| @@ -959,8 +963,12 @@ | ||
| 959 | 963 | })); |
| 960 | 964 | } |
| 961 | 965 | |
| 962 | - createWebhook(url: string, events: string[], secret?: string): { id: string; secret: string } { | |
| 963 | - if (!/^https:\/\//.test(url)) throw new HttpError(422, "webhook URL must use https"); | |
| 966 | + createWebhook(url: string, events: string[], secret: string | undefined, ownHost: string): { id: string; secret: string } { | |
| 967 | + // HTTPS to a public host other than Diffinery's own, at most 20 per repository (as on GitHub), so a | |
| 968 | + // repository can't aim deliveries at internal addresses or multiply every event into a flood. | |
| 969 | + checkPublicUrl(url, false, "webhook URL", ownHost); | |
| 970 | + const count = this.sql.exec<{ n: number }>("SELECT count(*) AS n FROM webhooks").one().n; | |
| 971 | + if (count >= MAX_WEBHOOKS) throw new HttpError(422, `a repository can have at most ${MAX_WEBHOOKS} webhooks`); | |
| 964 | 972 | const id = newId(); |
| 965 | 973 | const s = secret ?? randomToken(24); |
| 966 | 974 | this.sql.exec("INSERT INTO webhooks (id, url, secret, events, created_at) VALUES (?, ?, ?, ?, ?)", id, url, s, JSON.stringify(events.length ? events : ["*"]), now()); |
modifiedapps/diffinery/test/e2e/security.test.mjs+13−0View file
| @@ -2021,3 +2021,16 @@ | ||
| 2021 | 2021 | g(["push", "-q", "origin", "HEAD:main"]); |
| 2022 | 2022 | assert.equal((await ok("GET", `${R}/branches/main`)).commit.sha, g(["rev-parse", "HEAD"])); |
| 2023 | 2023 | }); |
| 2024 | + | |
| 2025 | +test("security: webhooks reach only public hosts other than Diffinery, at most 20 per repository", async () => { | |
| 2026 | + const R = `/repos/${OWNER}/hooked`; | |
| 2027 | + await repoWithReadme("hooked"); | |
| 2028 | + const own = new URL(base).hostname; | |
| 2029 | + for (const url of ["https://localhost./x", "https://127.0.0.1/x", "https://[::1]/x", "https://169.254.169.254/latest", "http://example.com/x", "https://metadata.internal/x", `https://${own}/x`]) { | |
| 2030 | + const refused = await status(422, "POST", `${R}/hooks`, { config: { url } }); | |
| 2031 | + assert.match(refused.message, /webhook URL/, url); | |
| 2032 | + } | |
| 2033 | + for (let i = 0; i < 20; i++) await status(201, "POST", `${R}/hooks`, { config: { url: `https://example.com/hook/${i}` } }); | |
| 2034 | + const full = await status(422, "POST", `${R}/hooks`, { config: { url: "https://example.com/hook/20" } }); | |
| 2035 | + assert.match(full.message, /at most 20 webhooks/); | |
| 2036 | +}); |
modifiedapps/diffinery/test/unit/imports.test.ts+6−0View file
| @@ -103,6 +103,12 @@ | ||
| 103 | 103 | expect(() => checkPublicUrl("http://127.0.0.1:8787", true)).not.toThrow(); |
| 104 | 104 | }); |
| 105 | 105 | |
| 106 | + it("treats a trailing dot as the same host, and can refuse Diffinery's own host", () => { | |
| 107 | + for (const u of ["https://localhost./x", "https://localhost../x", "https://metadata.internal./x", "https://intranet./x"]) expect(() => checkPublicUrl(u, false)).toThrow(/public host/); | |
| 108 | + for (const u of ["https://diffinery.com/x", "https://DIFFINERY.COM./x", "https://api.diffinery.com/x"]) expect(() => checkPublicUrl(u, false, "webhook URL", "diffinery.com")).toThrow(/Diffinery itself/); | |
| 109 | + expect(() => checkPublicUrl("https://notdiffinery.com/x", false, "webhook URL", "diffinery.com")).not.toThrow(); | |
| 110 | + }); | |
| 111 | + | |
| 106 | 112 | it("recognises private address ranges", () => { |
| 107 | 113 | for (const h of ["127.0.0.1", "10.1.2.3", "192.168.0.1", "172.16.0.1", "169.254.0.1", "100.64.0.1", "::1", "fd00::1"]) expect(isPrivateAddress(h)).toBe(true); |
| 108 | 114 | for (const h of ["8.8.8.8", "140.82.112.3", "2606:2800::1"]) expect(isPrivateAddress(h)).toBe(false); |
modifiedapps/diffinery/test/unit/webhooks.test.ts+10−0View file
| @@ -59,4 +59,14 @@ | ||
| 59 | 59 | expect(await deliverWebhook(h.deps, { ...msg, attempt: 1 })).toBeNull(); |
| 60 | 60 | expect(h.sent).toEqual([]); |
| 61 | 61 | }); |
| 62 | + | |
| 63 | + it("never delivers to an internal address or Diffinery itself, even for a hook saved before URLs were checked", async () => { | |
| 64 | + for (const url of ["https://127.0.0.1/x", "https://localhost./x", "http://hooks.example/x", "https://diffinery.com/api/v3/x"]) { | |
| 65 | + const h = harness([200], { ...hook, url }); | |
| 66 | + expect(await deliverWebhook({ ...h.deps, ownHost: "diffinery.com" }, msg)).toBe(0); | |
| 67 | + expect(h.sent).toEqual([]); | |
| 68 | + expect(h.recorded).toEqual([0]); | |
| 69 | + expect(h.requeued).toEqual([]); | |
| 70 | + } | |
| 71 | + }); | |
| 62 | 72 | }); |