Public: anyone can read this; the crew can change it.
POST /contents commits to a protected default branch: commitToBranch skips repo.protectDefault and size caps #5143
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/5132-post-contents-commits-to-a-protected-def → main 3 commits fork 1 thread · 1 unresolved
5 files
changed +77
−6
src/actions/repos.ts +31 −1
| @@ -33,9 +33,11 @@ import { | ||
| 33 | 33 | type CreateRepoError, |
| 34 | 34 | type DecisionsBy, |
| 35 | 35 | type ForkRepoError, |
| 36 | canAdmin, | |
| 36 | 37 | type Repo, |
| 37 | 38 | type RepoSettings, |
| 38 | 39 | } from "../core/repos.ts"; |
| 40 | import { MAX_COMMIT_BYTES, MAX_FILE_BYTES } from "./contents.ts"; | |
| 39 | 41 | import { commitIdentity } from "../core/commit_identity.ts"; |
| 40 | 42 | import type { User } from "../core/users.ts"; |
| 41 | 43 | import { |
| @@ -629,6 +631,13 @@ export function deleteWebhook( | ||
| 629 | 631 | return ok(); |
| 630 | 632 | } |
| 631 | 633 | |
| 634 | export type CommitToBranchError = | |
| 635 | | AccessError | |
| 636 | | CommitFilesError | |
| 637 | | { kind: "protected_branch"; branch: string } | |
| 638 | | { kind: "file_too_large"; path: string; bytes: number } | |
| 639 | | { kind: "commit_too_large"; bytes: number }; | |
| 640 | ||
| 632 | 641 | export interface CommitToBranchInput { |
| 633 | 642 | /** @default the repo's default branch */ |
| 634 | 643 | branch?: string; |
| @@ -649,10 +658,31 @@ export async function commitToBranch( | ||
| 649 | 658 | actor: User | undefined, |
| 650 | 659 | repo: Repo, |
| 651 | 660 | input: CommitToBranchInput, |
| 652 | ): Promise<Result<CommitResult, AccessError | CommitFilesError>> { | |
| 661 | ): Promise<Result<CommitResult, CommitToBranchError>> { | |
| 653 | 662 | const writer = requireWriter(app, actor, repo); |
| 654 | 663 | if (!writer.ok) return writer; |
| 655 | 664 | const branch = input.branch || repo.defaultBranch; |
| 665 | // The same mechanical rules `editFiles` applies: a protected default takes | |
| 666 | // commits from repo admins only, and no file or commit is unbounded. | |
| 667 | if ( | |
| 668 | branch === repo.defaultBranch && | |
| 669 | repo.protectDefault && | |
| 670 | !canAdmin(app.db, repo, writer.value) | |
| 671 | ) | |
| 672 | return err({ kind: "protected_branch", branch }); | |
| 673 | let total = 0; | |
| 674 | for (const file of input.files) { | |
| 675 | if (file.content === null) continue; | |
| 676 | const bytes = | |
| 677 | typeof file.content === "string" | |
| 678 | ? Buffer.byteLength(file.content, "utf8") | |
| 679 | : file.content.length; | |
| 680 | if (bytes > MAX_FILE_BYTES) | |
| 681 | return err({ kind: "file_too_large", path: file.path, bytes }); | |
| 682 | total += bytes; | |
| 683 | } | |
| 684 | if (total > MAX_COMMIT_BYTES) | |
| 685 | return err({ kind: "commit_too_large", bytes: total }); | |
| 656 | 686 | const startPoint = input.base || repo.defaultBranch; |
| 657 | 687 | const committed = await commitFiles( |
| 658 | 688 | repoDir(app.reposRoot, repo.ownerName, repo.name), |
src/contract/openapi.ts +3
| @@ -887,6 +887,9 @@ export const ROUTES: readonly Route[] = [ | ||
| 887 | 887 | invalid_path: "a file path is not a safe relative path", |
| 888 | 888 | path_conflict: "a path is a directory in the tree, or an ancestor of it is a file", |
| 889 | 889 | no_files: "files holds nothing to write", |
| 890 | protected_branch: "the default branch is protected and the caller is not a repo admin", | |
| 891 | file_too_large: "a file is over the per-file limit", | |
| 892 | commit_too_large: "the files together are over the per-commit limit", | |
| 890 | 893 | branch_moved: "the branch moved while committing (409)", |
| 891 | 894 | land_refused: "the commit could not be landed on the branch", |
| 892 | 895 | git: "git failed while committing", |
src/http/api.ts +1 −2
| @@ -3060,8 +3060,7 @@ export function apiRoutes(router: Router<Handler>): void { | ||
| 3060 | 3060 | if (!committed.ok) { |
| 3061 | 3061 | if (typeof committed.error === "string") |
| 3062 | 3062 | return respondAccess(committed.error, 422); |
| 3063 | const kind = committed.error.kind; | |
| 3064 | return json({ error: kind }, kind === "branch_moved" ? 409 : 422); | |
| 3063 | return editFailure(committed.error); | |
| 3065 | 3064 | } |
| 3066 | 3065 | return json(committed.value, 201); |
| 3067 | 3066 | }); |
test/edits.test.ts +39
| @@ -277,6 +277,45 @@ describe("edits: files, branches and a pull in one call", () => { | ||
| 277 | 277 | expect(tree.entries.map((e) => e.name).sort()).toEqual(["a.txt"]); |
| 278 | 278 | }); |
| 279 | 279 | |
| 280 | test("POST /contents honours protection and the size caps", async () => { | |
| 281 | const send = (token: string, files: unknown[]) => | |
| 282 | api( | |
| 283 | "/api/v1/repos/acme/widget/contents", | |
| 284 | { | |
| 285 | method: "POST", | |
| 286 | body: JSON.stringify({ branch: "main", message: "x", files }), | |
| 287 | }, | |
| 288 | token, | |
| 289 | ); | |
| 290 | const refused = await send(bobToken, [{ path: "c.txt", content: "no\n" }]); | |
| 291 | expect(refused.status).toBe(403); | |
| 292 | expect(await read<{ error: string }>(refused)).toMatchObject({ | |
| 293 | error: "protected_branch", | |
| 294 | }); | |
| 295 | const big = await send(admin, [ | |
| 296 | { path: "big.txt", content: "x".repeat(2 * 1024 * 1024 + 1) }, | |
| 297 | ]); | |
| 298 | expect(big.status).toBe(413); | |
| 299 | expect(await read<{ error: string }>(big)).toMatchObject({ | |
| 300 | error: "file_too_large", | |
| 301 | path: "big.txt", | |
| 302 | }); | |
| 303 | const many = await send( | |
| 304 | admin, | |
| 305 | Array.from({ length: 6 }, (_, i) => ({ | |
| 306 | path: `m${i}.txt`, | |
| 307 | content: "x".repeat(1_900_000), | |
| 308 | })), | |
| 309 | ); | |
| 310 | expect(many.status).toBe(413); | |
| 311 | expect(await read<{ error: string }>(many)).toMatchObject({ | |
| 312 | error: "commit_too_large", | |
| 313 | }); | |
| 314 | expect((await send(admin, [{ path: "ok.txt", content: "ok\n" }])).status).toBe( | |
| 315 | 201, | |
| 316 | ); | |
| 317 | }); | |
| 318 | ||
| 280 | 319 | test("a protected default refuses a writer and accepts an admin", async () => { |
| 281 | 320 | const refused = await edit( |
| 282 | 321 | "acme/widget", |
test/openapi.test.ts +3 −3
| @@ -228,11 +228,11 @@ test("a repository's code writes are described: branches and contents", () => { | ||
| 228 | 228 | expect(doc.paths[`${R}/branches/{branch}`].delete.responses["204"]).toBeDefined(); |
| 229 | 229 | }); |
| 230 | 230 | |
| 231 | test("the contents row names the refusals of its own handler, not /edits'", () => { | |
| 231 | test("the contents row names the refusals of its own handler, protection and size caps included", () => { | |
| 232 | 232 | const doc = openapi("https://f.test", "1") as any; |
| 233 | 233 | const R = "/api/v1/repos/{owner}/{name}"; |
| 234 | 234 | const text = JSON.stringify(doc.paths[`${R}/contents`].post.responses); |
| 235 | 235 | expect(text).toContain("path_conflict"); |
| 236 | expect(text).not.toContain("protected_branch"); | |
| 237 | expect(text).not.toContain("file_too_large"); | |
| 236 | expect(text).toContain("protected_branch"); | |
| 237 | expect(text).toContain("file_too_large"); | |
| 238 | 238 | }); |
new above
offline
Nit: this repeats
sizeOffromsrc/actions/contents.ts(editFiles usessizeOf(file.content)). ExportsizeOf, or a smallcheckSizes(files)that both callers share, so the two size rules can't drift apart. Not blocking.