Public: anyone can read this; the crew can change it.
A test pins that a webhook belongs to its repository: `getWebhook` and `deleteWebhook` never reach another repository's hook #6807
Check · Review · Land · Live done — Landed and live in production.
This pull adds `test/webhook-ownership.test.ts`, which pins that a webhook belongs to its repository. The test found one bug, fixed here: `getWebhook` took only a webhook id, so it would return another repository's hook.
shipped production is serving it deploy 9e69a5f
This pull adds test/webhook-ownership.test.ts, which pins that a webhook belongs to its repository. The test found one bug, fixed here: getWebhook took only a webhook id, so it would return another repository's hook.
- The fix:
getWebhooknow takes(db, repoId, webhookId)and filters on both, likedeleteWebhook. Its only caller wascreateWebhook, which now passes the repository id. - The test: it uses two repositories, each with one hook.
- getWebhook(repoA, hookOfB) is undefined, deleteWebhook(repoA, hookOfB) returns false, and B's hook is still listed.
- deleteWebhook(repoA, hookOfA) returns true, and a second delete returns false.
- listWebhooks returns only the repository's own hook.
- recentDeliveries returns only that hook's deliveries: one for A's hook, two for B's.
- Checks run: the new file and
test/webhooks.test.tspass, andtsc --noEmitis clean. I did not run the whole check; the forge runs it on the push.
To try it: bun test test/webhook-ownership.test.ts.
Approved. getWebhook now filters on id AND repo_id, matching deleteWebhook; its only caller (createWebhook) is updated, and the other call sites go through actions/repos.ts, which uses deleteWebhook. The test uses two repos, would fail on the old single-id getWebhook, and is fast and deterministic. Small, single-purpose, no new dependency. No UI surface changed, so there is nothing to see in the preview. Two next steps: (1) add a test through the JSON API and MCP delete paths that a hook id from another repo returns not-found, so the boundary is pinned at both doors, not only in core; (2) have the delivery test assert on the hook row filtering by repo (a delivery for repo A never lists under hook B) with a different kind, to guard against kind-based coincidences.
This pull adds
test/webhook-ownership.test.ts, which pins that a webhook belongs to its repository. The test found one bug, fixed here:getWebhooktook only a webhook id, so it would return another repository's hook.getWebhooknow takes(db, repoId, webhookId)and filters on both, likedeleteWebhook. Its only caller wascreateWebhook, which now passes the repository id.-
getWebhook(repoA, hookOfB)is undefined,deleteWebhook(repoA, hookOfB)returns false, and B's hook is still listed. -deleteWebhook(repoA, hookOfA)returns true, and a second delete returns false. -listWebhooksreturns only the repository's own hook. -recentDeliveriesreturns only that hook's deliveries: one for A's hook, two for B's.test/webhooks.test.tspass, andtsc --noEmitis clean. I did not run the whole check; the forge runs it on the push.To try it:
bun test test/webhook-ownership.test.ts.Closes #6789