trevato/forge

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

More
Forks1g fMapg aRuns16g rEventsFlows

One trunk again: bring Forgejo's 4 grokbot commits into main, and land outside agents only through the forge's pulls #2615

Merged masoncrew opened this in answer to trevatoowner

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

ConversationFiles
Merged

mason/forge:mason/2614-one-trunk-again-bring-forgejo-s-4-grokbo → main 2 commits fork 1 thread · 1 unresolved

  • Merged. mason/2614-one-trunk-again-bring-forgejo-s-4-grokbo was merged into main as 52f8958.

Review

Sign in to review this change.
4 files changed +169 −2
docs/THREATS.md +4
@@ -85,6 +85,10 @@ person approved.
8585 Test: `test/crew-sandbox-real.test.ts`
8686 - An agent cannot push to the default branch or merge; a person merges.
8787 Test: `test/protection.test.ts`
88 - An outside agent that drives a person's account cannot push past the pulls
89 either: a protected default takes a push from its admins only, so every
90 other writer lands through a pull, its gate and its review.
91 Test: `test/protected-default-push.test.ts`
8892 - Merge gates (checks, review) are enforced in the merge path, not by convention.
8993 Test: `test/security-merge.test.ts`
9094 - Spend, time and turns per run are limited.
src/http/server.ts +16 −1
@@ -51,7 +51,13 @@ import {
5151 REQUEST_MAX,
5252 settleBody,
5353 } from "./limits.ts";
54 import { canRead, canWrite, getRepo, getRepoById } from "../core/repos.ts";
54 import {
55 canAdmin,
56 canRead,
57 canWrite,
58 getRepo,
59 getRepoById,
60 } from "../core/repos.ts";
5561 import { trackPullHead } from "../core/pulls.ts";
5662 import { createRepo, repositoryLimitMessage, resolveOwner } from "../actions/repos.ts";
5763 import { sessionOf } from "../core/sessions.ts";
@@ -1766,6 +1772,15 @@ async function handleHook(app: App, request: Request): Promise<Response> {
17661772 },
17671773 );
17681774 }
1775 // A protected default takes commits from its admins only, as the web
1776 // editor's (`actions/contents.ts`): every other writer lands through
1777 // a pull. A merge moves the ref itself and never comes here.
1778 if (!canAdmin(app.db, repo, actor)) {
sentinelcrew

Correct place for the check: it only runs when protection is on and the ref is the default branch, and after the agent and suspension refusals. canAdmin(undefined) is false, but both receive-pack doors already require an authenticated writer, so no actorless push reaches here.
1779 return new Response(
1780 `${repo.defaultBranch} is protected: only its admins push it; open a pull from a branch`,
1781 { status: 403 },
1782 );
1783 }
17691784 if (!ZERO_OID.test(update.oldOid)) {
17701785 const dir = repoDir(app.reposRoot, repo.ownerName, repo.name);
17711786 // Quarantine env forwarded from the hook makes the new objects visible.
src/web/pages/settings.ts +2 −1
@@ -256,7 +256,8 @@ export async function settingsPage(ctx: Ctx): Promise<Response> {
256256 ${repo.protectDefault ? html`checked` : null}
257257 />
258258 <span
259 >Protect the default branch — no force-push, no deletion. Enforced
259 >Protect the default branch — no force-push, no deletion, and only
260 its admins push it; everyone else lands through a pull. Enforced
260261 by the pre-receive hook, so it holds for every client.</span
261262 >
262263 </label>
test/protected-default-push.test.ts added +147
@@ -0,0 +1,147 @@
1 /**
2 * A protected default branch takes a `git push` from its admins only, the
3 * rule the web editor already keeps (`actions/contents.ts`): every other
4 * writer — a person's account an outside agent drives included — lands
5 * through a pull, which the forge merges by moving the ref itself (#2614).
6 */
7
8 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
9 import { mkdtemp, rm } from "node:fs/promises";
10 import { tmpdir } from "node:os";
11 import { join } from "node:path";
12 import { createApp, type App } from "../src/app.ts";
13 import { mergePull, openPull } from "../src/actions/pulls.ts";
14 import { addTeamMember, createOrg, createTeam } from "../src/core/orgs.ts";
15 import { createRepo, updateRepoSettings, type Repo } from "../src/core/repos.ts";
16 import { createToken } from "../src/core/tokens.ts";
17 import { createUser, type User } from "../src/core/users.ts";
18 import { resolveOid } from "../src/git/merge.ts";
19 import { repoDir } from "../src/git/repos.ts";
20 import { commitFiles } from "../src/git/write.ts";
21 import { serve, type Servers } from "../src/http/server.ts";
22 import { gitClientEnv } from "./helpers/git-client.ts";
23 import { quietLogs } from "./helpers/globals.ts";
24
25 quietLogs();
26
27 const clientEnv = gitClientEnv();
28
29 async function sh(args: string[], cwd: string) {
30 const proc = Bun.spawn(args, { cwd, env: clientEnv, stdout: "pipe", stderr: "pipe" });
31 const [out, err, code] = await Promise.all([
32 new Response(proc.stdout).text(),
33 new Response(proc.stderr).text(),
34 proc.exited,
35 ]);
36 return { code, out: out.trim(), err: err.trim() };
37 }
38
39 const ok = <T>(r: { ok: true; value: T } | { ok: false; error: unknown }): T => {
40 if (!r.ok) throw new Error(JSON.stringify(r.error));
41 return r.value;
42 };
43
44 describe("a protected default branch takes pushes from its admins only", () => {
45 let dataDir: string;
46 let work: string;
47 let app: App;
48 let servers: Servers;
49 let repo: Repo;
50 let founder: User;
51 let writer: User;
52 let writerClone = "";
53 let founderClone = "";
54
55 const mainTip = () =>
56 resolveOid(repoDir(app.reposRoot, repo.ownerName, repo.name), "refs/heads/main");
57 const commit = async (clone: string, file: string) => {
58 await Bun.write(join(clone, file), `${file}\n`);
59 expect((await sh(["git", "add", file], clone)).code).toBe(0);
60 expect((await sh(["git", "commit", "-qm", file], clone)).code).toBe(0);
61 };
62 const protect = (on: boolean) => {
63 repo = updateRepoSettings(app.db, repo, { protectDefault: on });
64 };
65
66 beforeAll(async () => {
67 dataDir = await mkdtemp(join(tmpdir(), "forge-protected-push-"));
68 work = await mkdtemp(join(tmpdir(), "forge-protected-push-work-"));
69 app = createApp({ dataDir, port: 0 });
70 servers = await serve(app);
71 const seed = (username: string, adminIfFirst: boolean) =>
72 createUser(app.db, {
73 origin: "seed",
74 trusted: true,
75 username,
76 email: `${username}@forge.local`,
77 password: "a-good-passphrase",
78 adminIfFirst,
79 });
80 // The site admin is someone else: the founder is admin by the org alone.
81 ok(await seed("siteadmin", true));
82 founder = ok(await seed("carol", false));
83 writer = ok(await seed("dave", false));
84 const org = ok(createOrg(app.db, "acme", founder.id));
85 const team = ok(createTeam(app.db, org.id, "writers", "write"));
86 addTeamMember(app.db, team, writer.id);
87 repo = ok(
88 await createRepo(app.db, app.reposRoot, { ownerId: org.id, name: "app", visibility: "public" }),
89 );
90 ok(
91 await commitFiles(repoDir(app.reposRoot, repo.ownerName, repo.name), {
92 branch: "main",
93 message: "seed",
94 files: [{ path: "README.md", content: "hi\n" }],
95 author: { name: "Founder", email: "[email protected]" },
96 }),
97 );
98 const remote = (user: User) =>
99 `${servers.url.replace("://", `://${user.username}:${createToken(app.db, user.id, "push", "rw").token}@`)}/acme/app.git`;
100 writerClone = join(work, "writer");
101 founderClone = join(work, "founder");
102 expect((await sh(["git", "clone", "-q", remote(writer), writerClone], work)).code).toBe(0);
103 expect((await sh(["git", "clone", "-q", remote(founder), founderClone], work)).code).toBe(0);
104 }, 30_000);
105
106 afterAll(async () => {
107 servers?.stop();
108 app?.close();
109 await rm(dataDir, { recursive: true, force: true });
110 await rm(work, { recursive: true, force: true });
111 });
112
113 test("unprotected, a writer's fast-forward to main still lands", async () => {
114 protect(false);
115 await commit(writerClone, "open.txt");
116 expect((await sh(["git", "push", "-q", "origin", "HEAD:main"], writerClone)).code).toBe(0);
117 });
118
119 test("protected, a writer's push to main is refused with a line pointing at pulls", async () => {
120 protect(true);
121 const before = await mainTip();
122 await commit(writerClone, "direct.txt");
123 const pushed = await sh(["git", "push", "-q", "origin", "HEAD:main"], writerClone);
124 expect(pushed.code).not.toBe(0);
125 expect(pushed.err).toContain("main is protected: only its admins push it; open a pull from a branch");
126 expect(await mainTip()).toBe(before);
127 });
128
129 test("protected, the writer's branch lands through a pull an admin merges", async () => {
130 const branch = await sh(["git", "push", "-q", "origin", "HEAD:refs/heads/change"], writerClone);
131 expect(branch.code).toBe(0);
132 const pull = ok(
133 await openPull(app, writer, repo, { title: "A change", baseBranch: "main", headBranch: "change" }),
134 );
135 ok(await mergePull(app, founder, repo, pull, "merge"));
136 const landed = await sh(["git", "fetch", "-q", "origin"], writerClone);
137 expect(landed.code).toBe(0);
138 const contains = await sh(["git", "merge-base", "--is-ancestor", "HEAD", "origin/main"], writerClone);
139 expect(contains.code).toBe(0);
140 });
141
142 test("protected, its admin's fast-forward to main lands", async () => {
143 expect((await sh(["git", "pull", "-q", "--ff-only", "origin", "main"], founderClone)).code).toBe(0);
144 await commit(founderClone, "admin.txt");
145 expect((await sh(["git", "push", "-q", "origin", "HEAD:main"], founderClone)).code).toBe(0);
146 });
147 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs