Issue edits respect triage, numeric payload ids, org secret deletes, an Actions settings page (#43)
Authors without triage can no longer set labels or assignees by editing. Webhook payload user, organization and repository ids are numbers. Deleting an organization secret that doesn't exist is a 404 and isn't audited. Repo settings gain an Actions tab for workflow permissions and the create-and-approve toggle. The security test's audit log read retries while the dev server holds the database lock. Claude-Session: https://claude.ai/code/session_01QMFviVKD61QaDPxdrh7C3f
Canberk Demirci authored · GitHub committed signed1 parent 79c6c38commit 59f524715e852df99578213ea0b950265b382008Browse files
9 files changed+122−11
Files (9)
- apps/diffinery/src/docs/api-meta.ts+1−1
- apps/diffinery/src/http/api.ts+3−1
- apps/diffinery/src/repo/actions.ts+1−1
- apps/diffinery/src/repo/collab.ts+2−0
- apps/diffinery/src/repo/event-payloads.ts+7−5
- apps/diffinery/src/web/repo-settings.tsx+63−1
- apps/diffinery/test/e2e/e2e.test.mjs+15−0
- apps/diffinery/test/e2e/security.test.mjs+23−1
- apps/diffinery/test/unit/actions-runtime.test.ts+7−1
modifiedapps/diffinery/src/docs/api-meta.ts+1−1View file
| @@ -1061,7 +1061,7 @@ | ||
| 1061 | 1061 | status: 204, |
| 1062 | 1062 | body: { value: ["string", "Secret value.", true] }, |
| 1063 | 1063 | }, |
| 1064 | - "DELETE /orgs/{org}/actions/secrets/{name}": { summary: "Delete an organization secret", access: "org-admin", status: 204 }, | |
| 1064 | + "DELETE /orgs/{org}/actions/secrets/{name}": { summary: "Delete an organization secret", description: "404 when the organization has no secret with this name. Audited.", access: "org-admin", status: 204 }, | |
| 1065 | 1065 | |
| 1066 | 1066 | // ------------------------------------------------------------ webhooks |
| 1067 | 1067 | "GET /repos/{owner}/{repo}/hooks": { summary: "List webhooks", description: "Secrets are never returned.", access: "admin" }, |
modifiedapps/diffinery/src/http/api.ts+3−1View file
| @@ -827,7 +827,9 @@ | ||
| 827 | 827 | }); |
| 828 | 828 | api.delete("/orgs/:org/actions/secrets/:name", async (c) => { |
| 829 | 829 | const { a, org } = await orgAdmin(c); |
| 830 | - await c.env.DB.prepare("DELETE FROM org_secrets WHERE org_id = ? AND name = ?").bind(org.id, c.req.param("name").toUpperCase()).run(); | |
| 830 | + const r = await c.env.DB.prepare("DELETE FROM org_secrets WHERE org_id = ? AND name = ?").bind(org.id, c.req.param("name").toUpperCase()).run(); | |
| 831 | + // Like GitHub: deleting a secret that doesn't exist is a 404, and nothing is audited. | |
| 832 | + if (!r.meta?.changes) throw new HttpError(404, "secret not found"); | |
| 831 | 833 | c.executionCtx.waitUntil(c.get("dir").audit({ actorId: a.id, actorLogin: a.login, action: "org.secret.delete", target: org.login, detail: { name: c.req.param("name").toUpperCase(), via: "api" } })); |
| 832 | 834 | return c.body(null, 204); |
| 833 | 835 | }); |
modifiedapps/diffinery/src/repo/actions.ts+1−1View file
| @@ -401,7 +401,7 @@ | ||
| 401 | 401 | head_ref: pr?.head?.ref ?? "", |
| 402 | 402 | base_ref: pr?.base?.ref ?? "", |
| 403 | 403 | repository: `${m.owner}/${m.name}`, |
| 404 | - repository_id: m.repoId, | |
| 404 | + repository_id: String(numericId(m.repoId)), | |
| 405 | 405 | repository_owner: m.owner, |
| 406 | 406 | repositoryUrl: `${this.env.PUBLIC_URL}/${m.owner}/${m.name}.git`, |
| 407 | 407 | actor: actor.login, |
modifiedapps/diffinery/src/repo/collab.ts+2−0View file
| @@ -369,6 +369,8 @@ | ||
| 369 | 369 | const r = this.row(number); |
| 370 | 370 | const isAuthor = r.author_id === caller.id; |
| 371 | 371 | if (!isAuthor && !atLeast(caller.role, "triage")) throw new HttpError(403, "permission denied"); |
| 372 | + // As when creating: authors without triage access can't set labels or assignees, which are silently dropped. | |
| 373 | + if (!atLeast(caller.role, "triage")) patch = { ...patch, labels: undefined, assignees: undefined }; | |
| 372 | 374 | if (r.kind === "pull" && patch.state === "open" && this.pullRow(number).merged) throw new HttpError(422, "a merged pull request cannot be reopened"); |
| 373 | 375 | const t = now(); |
| 374 | 376 | const state = patch.state ?? r.state; |
modifiedapps/diffinery/src/repo/event-payloads.ts+7−5View file
| @@ -1,5 +1,6 @@ | ||
| 1 | 1 | import type { Comment, Issue } from "./collab.js"; |
| 2 | 2 | import type { RepoMeta } from "./types.js"; |
| 3 | +import { nodeId, numericId } from "../http/gh.js"; | |
| 3 | 4 | |
| 4 | 5 | /** |
| 5 | 6 | * Webhook-style event payloads (what a job reads from GITHUB_EVENT_PATH and |
| @@ -20,8 +21,9 @@ | ||
| 20 | 21 | const bot = who.kind === "agent" || who.kind === "system" || who.login.endsWith("[bot]"); |
| 21 | 22 | return { |
| 22 | 23 | login: who.login, |
| 23 | - id: who.id ?? who.login, | |
| 24 | - node_id: who.id ?? who.login, | |
| 24 | + // Numeric like GitHub's and the REST API's (ghUser), from the account id when it's known. | |
| 25 | + id: numericId(who.id ?? who.login), | |
| 26 | + node_id: nodeId(bot ? "Bot" : "User", who.id ?? who.login), | |
| 25 | 27 | type: bot ? "Bot" : "User", |
| 26 | 28 | site_admin: false, |
| 27 | 29 | avatar_url: `${s.publicUrl}/${encodeURIComponent(who.login)}.png`, |
| @@ -33,7 +35,7 @@ | ||
| 33 | 35 | export function organizationPayload(s: PayloadSite): Record<string, unknown> | undefined { |
| 34 | 36 | const m = s.meta; |
| 35 | 37 | if (m.ownerKind !== "org") return undefined; |
| 36 | - return { login: m.owner, id: m.ownerId, node_id: m.ownerId, url: `${api(s)}/orgs/${m.owner}`, repos_url: `${api(s)}/orgs/${m.owner}/repos`, avatar_url: `${s.publicUrl}/${m.owner}.png`, description: null }; | |
| 38 | + return { login: m.owner, id: numericId(m.ownerId), node_id: nodeId("Organization", m.ownerId), url: `${api(s)}/orgs/${m.owner}`, repos_url: `${api(s)}/orgs/${m.owner}/repos`, avatar_url: `${s.publicUrl}/${m.owner}.png`, description: null }; | |
| 37 | 39 | } |
| 38 | 40 | |
| 39 | 41 | export function repositoryPayload(s: PayloadSite): Record<string, unknown> { |
| @@ -43,8 +45,8 @@ | ||
| 43 | 45 | const html = `${s.publicUrl}/${full}`; |
| 44 | 46 | const host = new URL(s.publicUrl).host; |
| 45 | 47 | return { |
| 46 | - id: m.repoId, | |
| 47 | - node_id: m.repoId, | |
| 48 | + id: numericId(m.repoId), | |
| 49 | + node_id: nodeId("Repository", m.repoId), | |
| 48 | 50 | name: m.name, |
| 49 | 51 | full_name: full, |
| 50 | 52 | private: s.visibility !== "public", |
modifiedapps/diffinery/src/web/repo-settings.tsx+63−1View file
| @@ -11,7 +11,7 @@ | ||
| 11 | 11 | import { Actor, CopyButton, Form, PendingInvites, ROLES, Time, cap, act, ago, flash, post, refNames, render, repoUrl, statusOf, type WebApp, prm } from "./ui.js"; |
| 12 | 12 | |
| 13 | 13 | type Loaded = Awaited<ReturnType<typeof loadRepo>>; |
| 14 | -type Tab = "general" | "access" | "branches" | "secrets" | "webhooks"; | |
| 14 | +type Tab = "general" | "access" | "branches" | "actions" | "secrets" | "webhooks"; | |
| 15 | 15 | |
| 16 | 16 | const WEBHOOK_EVENTS = ["push", "pull_request", "pull_request_review", "issues", "issue_comment", "workflow_run", "workflow_dispatch"]; |
| 17 | 17 | |
| @@ -34,6 +34,7 @@ | ||
| 34 | 34 | ["general", "General", base], |
| 35 | 35 | ["access", "Collaborators", `${base}/access`], |
| 36 | 36 | ["branches", "Branch protection", `${base}/branches`], |
| 37 | + ["actions", "Actions", `${base}/actions`], | |
| 37 | 38 | ["secrets", "Secrets and variables", `${base}/secrets`], |
| 38 | 39 | ["webhooks", "Webhooks", `${base}/webhooks`], |
| 39 | 40 | ]; |
| @@ -396,6 +397,53 @@ | ||
| 396 | 397 | ); |
| 397 | 398 | } |
| 398 | 399 | |
| 400 | +/** GitHub's Settings → Actions → General → Workflow permissions: what a job's GITHUB_TOKEN gets by default. */ | |
| 401 | +async function actionsPage(c: AppContext, l: Loaded) { | |
| 402 | + const base = repoUrl(l.repo); | |
| 403 | + const settings = await l.stub.workflowSettings(); | |
| 404 | + return page( | |
| 405 | + c, | |
| 406 | + l, | |
| 407 | + "actions", | |
| 408 | + <> | |
| 409 | + <h1>Actions</h1> | |
| 410 | + <Form action={`${base}/settings/actions`} t={csrfToken(c)} class="box"> | |
| 411 | + <div class="head"> | |
| 412 | + <h2>Workflow permissions</h2> | |
| 413 | + </div> | |
| 414 | + <div class="body"> | |
| 415 | + <p class="muted small m0"> | |
| 416 | + The default permissions of the <code>GITHUB_TOKEN</code> each job gets. A workflow or job can ask for less, or more, with a <code>permissions:</code> block. | |
| 417 | + </p> | |
| 418 | + <div class="field"> | |
| 419 | + <KRadio | |
| 420 | + name="default_permissions" | |
| 421 | + value={settings.defaultPermissions} | |
| 422 | + items={[ | |
| 423 | + { value: "read", label: "Read repository contents and packages permissions", description: "Jobs can read the repository and its packages, and nothing else unless the workflow asks." }, | |
| 424 | + { value: "write", label: "Read and write permissions", description: "Jobs can read and write in every scope unless the workflow asks for less." }, | |
| 425 | + ]} | |
| 426 | + /> | |
| 427 | + </div> | |
| 428 | + <div class="field"> | |
| 429 | + <KCheckbox | |
| 430 | + name="can_approve" | |
| 431 | + checked={settings.canApprovePullRequests} | |
| 432 | + label={ | |
| 433 | + <> | |
| 434 | + Allow GitHub Actions to create and approve pull requests | |
| 435 | + <span class="muted small"> — approvals from workflows never count toward required reviews.</span> | |
| 436 | + </> | |
| 437 | + } | |
| 438 | + /> | |
| 439 | + </div> | |
| 440 | + <button class="btn primary">Save</button> | |
| 441 | + </div> | |
| 442 | + </Form> | |
| 443 | + </>, | |
| 444 | + ); | |
| 445 | +} | |
| 446 | + | |
| 399 | 447 | async function secretsPage(c: AppContext, l: Loaded) { |
| 400 | 448 | const base = repoUrl(l.repo); |
| 401 | 449 | const t = csrfToken(c); |
| @@ -574,9 +622,23 @@ | ||
| 574 | 622 | web.get("/:owner/:repo/settings", async (c) => generalPage(c, await load(c))); |
| 575 | 623 | web.get("/:owner/:repo/settings/access", async (c) => accessPage(c, await load(c))); |
| 576 | 624 | web.get("/:owner/:repo/settings/branches", async (c) => branchesPage(c, await load(c))); |
| 625 | + web.get("/:owner/:repo/settings/actions", async (c) => actionsPage(c, await load(c))); | |
| 577 | 626 | web.get("/:owner/:repo/settings/secrets", async (c) => secretsPage(c, await load(c))); |
| 578 | 627 | web.get("/:owner/:repo/settings/webhooks", async (c) => webhooksPage(c, await load(c))); |
| 579 | 628 | |
| 629 | + post(web, "/:owner/:repo/settings/actions", async (c, f) => { | |
| 630 | + const l = await load(c); | |
| 631 | + return act( | |
| 632 | + c, | |
| 633 | + `${repoUrl(l.repo)}/settings/actions`, | |
| 634 | + async () => { | |
| 635 | + const defaultPermissions = f.s("default_permissions") === "write" ? "write" : "read"; | |
| 636 | + await l.stub.setWorkflowSettings({ defaultPermissions, canApprovePullRequests: f.s("can_approve") === "1" }, l.caller); | |
| 637 | + }, | |
| 638 | + "Actions settings saved.", | |
| 639 | + ); | |
| 640 | + }); | |
| 641 | + | |
| 580 | 642 | post(web, "/:owner/:repo/settings/general", async (c, f) => { |
| 581 | 643 | const l = await load(c); |
| 582 | 644 | const dir = c.get("dir"); |
modifiedapps/diffinery/test/e2e/e2e.test.mjs+15−0View file
| @@ -884,6 +884,21 @@ | ||
| 884 | 884 | const bot = (await ok("POST", "/diffinery/agents", { login: "policy-bot" })).token.token; |
| 885 | 885 | assert.equal((await api("PUT", settingsPath, { can_approve_pull_request_reviews: true }, bot)).status, 403, "agents cannot change what workflows may do"); |
| 886 | 886 | assert.equal((await api("PUT", settingsPath, { default_workflow_permissions: "admin" })).status, 422); |
| 887 | + // The same settings on the repository's Settings → Actions page. | |
| 888 | + const actionsPage = `/${owner}/policy/settings/actions`; | |
| 889 | + const form = async (fields) => { | |
| 890 | + const res = await fetch(`${base}${actionsPage}`); | |
| 891 | + const html = await res.text(); | |
| 892 | + assert.match(html, /Allow GitHub Actions to create and approve pull requests/); | |
| 893 | + const csrf = /name="_csrf" value="([^"]+)"/.exec(html)?.[1]; | |
| 894 | + const cookie = /dfy_csrf=([^;]+)/.exec(res.headers.get("set-cookie") ?? "")?.[1]; | |
| 895 | + const r = await fetch(`${base}${actionsPage}`, { method: "POST", body: new URLSearchParams({ _csrf: csrf, ...fields }), redirect: "manual", headers: { cookie: `dfy_csrf=${cookie}`, "sec-fetch-site": "same-origin" } }); | |
| 896 | + assert.equal(r.status, 303); | |
| 897 | + }; | |
| 898 | + await form({ default_permissions: "write", can_approve: "1" }); | |
| 899 | + assert.deepEqual(await ok("GET", settingsPath), { default_workflow_permissions: "write", can_approve_pull_request_reviews: true }); | |
| 900 | + await form({ default_permissions: "read" }); | |
| 901 | + assert.deepEqual(await ok("GET", settingsPath), { default_workflow_permissions: "read", can_approve_pull_request_reviews: false }); | |
| 887 | 902 | |
| 888 | 903 | const src = join(work, "policy"); |
| 889 | 904 | mkdirSync(join(src, ".github/workflows"), { recursive: true }); |
modifiedapps/diffinery/test/e2e/security.test.mjs+23−1View file
| @@ -502,8 +502,19 @@ | ||
| 502 | 502 | assert.notEqual(bypassed.sha, mainSha); |
| 503 | 503 | }); |
| 504 | 504 | |
| 505 | -/** Audit entries recorded so far, read straight from the local D1 database. */ | |
| 505 | +/** Audit entries recorded so far, read straight from the local D1 database. The running dev server can hold its lock, so a busy database is retried. */ | |
| 506 | 506 | function auditLog(action) { |
| 507 | + for (let attempt = 1; ; attempt++) { | |
| 508 | + try { | |
| 509 | + return readAuditLog(action); | |
| 510 | + } catch (e) { | |
| 511 | + if (attempt >= 10 || !/SQLITE_BUSY|database is locked/.test(String(e.stderr ?? e.message))) throw e; | |
| 512 | + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 300 * attempt); | |
| 513 | + } | |
| 514 | + } | |
| 515 | +} | |
| 516 | + | |
| 517 | +function readAuditLog(action) { | |
| 507 | 518 | const out = execFileSync("pnpm", ["exec", "wrangler", "d1", "execute", "diffinery-dev", "--local", "-e", "dev", "--persist-to", join(work, "state"), "--json", "--command", `SELECT actor_login, action, target, detail FROM audit_log WHERE action = '${action}'`], { cwd: APP, stdio: "pipe", env: { ...process.env, CI: "1" } }); |
| 508 | 519 | return JSON.parse(out.toString())[0].results; |
| 509 | 520 | } |
| @@ -531,6 +542,10 @@ | ||
| 531 | 542 | assert.deepEqual([fromReader.labels.length, fromReader.assignees.length], [0, 0]); |
| 532 | 543 | const fromAdmin = await ok("POST", `${R}/issues`, { title: "from the owner", labels: ["bug"] }); |
| 533 | 544 | assert.equal(fromAdmin.labels[0].name, "bug"); |
| 545 | + // Editing their own issue doesn't let them in either. | |
| 546 | + const edited = await ok("PATCH", `${R}/issues/${fromReader.number}`, { title: "edited by its author", labels: ["bug"], assignees: [OWNER] }, T.rita); | |
| 547 | + assert.equal(edited.title, "edited by its author"); | |
| 548 | + assert.deepEqual([edited.labels.length, edited.assignees.length], [0, 0]); | |
| 534 | 549 | |
| 535 | 550 | // ------------------------------------------------------------ settings changed through the API are on the record |
| 536 | 551 | await status(204, "PUT", `${R}/collaborators/rita`, { permission: "push" }); |
| @@ -652,4 +667,11 @@ | ||
| 652 | 667 | // Outsiders don't. |
| 653 | 668 | assert.equal((await mcpTool(T.sam, "get_organization", { org: "guild" })).data.default_repository_permission, undefined); |
| 654 | 669 | assert.equal((await ok("GET", "/orgs/guild", undefined, T.sam)).default_repository_permission, undefined); |
| 670 | + | |
| 671 | + // Deleting an organization secret that doesn't exist is a 404 and leaves no audit entry. | |
| 672 | + await status(404, "DELETE", "/orgs/guild/actions/secrets/NEVER_SET"); | |
| 673 | + await status(204, "PUT", "/orgs/guild/actions/secrets/DEPLOY_KEY", { value: "k" }); | |
| 674 | + await status(204, "DELETE", "/orgs/guild/actions/secrets/DEPLOY_KEY"); | |
| 675 | + await until("the secret's audit entry", async () => auditLog("org.secret.delete").some((e) => JSON.parse(e.detail).name === "DEPLOY_KEY")); | |
| 676 | + assert.ok(!auditLog("org.secret.delete").some((e) => JSON.parse(e.detail).name === "NEVER_SET")); | |
| 655 | 677 | }); |
modifiedapps/diffinery/test/unit/actions-runtime.test.ts+7−1View file
| @@ -10,6 +10,7 @@ | ||
| 10 | 10 | import { b64, fromB64url } from "../../src/lib/util.js"; |
| 11 | 11 | import { completePayload, issuePayload, releasePayload } from "../../src/repo/event-payloads.js"; |
| 12 | 12 | import type { RepoMeta } from "../../src/repo/types.js"; |
| 13 | +import { numericId } from "../../src/http/gh.js"; | |
| 13 | 14 | |
| 14 | 15 | const MASTER = b64(new Uint8Array(32).fill(7)); |
| 15 | 16 | const OTHER = b64(new Uint8Array(32).fill(9)); |
| @@ -336,7 +337,7 @@ | ||
| 336 | 337 | const repo = p.repository as Record<string, unknown>; |
| 337 | 338 | expect(repo).toMatchObject({ name: "app", full_name: "acme/app", default_branch: "main", clone_url: "https://d.example/acme/app.git", url: "https://d.example/api/v3/repos/acme/app" }); |
| 338 | 339 | expect(repo.owner).toMatchObject({ login: "acme", type: "Organization" }); |
| 339 | - expect(p.sender).toMatchObject({ login: "ada", id: "u1", type: "User" }); | |
| 340 | + expect(p.sender).toMatchObject({ login: "ada", id: numericId("u1"), type: "User" }); | |
| 340 | 341 | expect(p.organization).toMatchObject({ login: "acme" }); |
| 341 | 342 | expect(p.ref).toBe("refs/heads/main"); |
| 342 | 343 | }); |
| @@ -347,6 +348,11 @@ | ||
| 347 | 348 | expect(pr.head).toMatchObject({ ref: "feat", sha: "abc", label: "acme:feat" }); |
| 348 | 349 | expect(pr.head.repo.full_name).toBe("acme/app"); |
| 349 | 350 | expect(pr.base.repo.full_name).toBe("acme/app"); |
| 351 | + // Numeric ids, as on GitHub and in the REST API. | |
| 352 | + expect(pr.head.repo.id).toBe(numericId("r1")); | |
| 353 | + expect(pr.base.repo.id).toBe(numericId("r1")); | |
| 354 | + expect(typeof pr.user.id).toBe("number"); | |
| 355 | + expect((p.organization as Record<string, unknown>).id).toBe(numericId("o1")); | |
| 350 | 356 | expect(pr).toMatchObject({ number: 3, title: "T", state: "open", html_url: "https://d.example/acme/app/pull/3" }); |
| 351 | 357 | expect(pr.user.login).toBe("bob"); |
| 352 | 358 | expect((p.sender as Record<string, unknown>).type).toBe("Bot"); |