Public: anyone can read this; the crew can change it.
Holding a file from any page goes through one form handler that takes where to return, and refuses an unknown or unreadable repository in the same words #7071
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/7055-holding-a-file-from-any-page-goes-throug → main 1 commit fork 1 thread · 1 unresolved
2 files
changed +154
−5
src/web/pages/file_holds.ts +21 −5
| @@ -2,12 +2,17 @@ | ||
| 2 | 2 | * POST /:owner/:name/settings/holds — hold new work on a file, or release it. |
| 3 | 3 | * Through `adminForm`: an admin of the repository, same-origin, admin scope; |
| 4 | 4 | * the hold names the signed-in admin (`actions/file_holds.ts`). |
| 5 | * | |
| 6 | * The form's answer is `fileHoldForm`, so any page that offers a hold posts | |
| 7 | * through the same words and only says where to return. | |
| 5 | 8 | */ |
| 6 | 9 | |
| 7 | import { holdFileAs, releaseFileAs } from "../../actions/file_holds.ts"; | |
| 10 | import { holdFileAs, releaseFileAs, type FileHoldError } from "../../actions/file_holds.ts"; | |
| 11 | import type { Repo } from "../../core/repos.ts"; | |
| 8 | 12 | import type { Ctx } from "../../http/context.ts"; |
| 13 | import { err, type Result } from "../../kernel/result.ts"; | |
| 9 | 14 | import { repoBase } from "../chrome.ts"; |
| 10 | import { field } from "../form.ts"; | |
| 15 | import { field, type Form } from "../form.ts"; | |
| 11 | 16 | import { flashTo } from "../layout.ts"; |
| 12 | 17 | import { adminForm } from "./settings.ts"; |
| 13 | 18 | |
| @@ -15,10 +20,21 @@ export async function fileHoldUpdate(ctx: Ctx): Promise<Response> { | ||
| 15 | 20 | const gate = await adminForm(ctx); |
| 16 | 21 | if (gate instanceof Response) return gate; |
| 17 | 22 | const { repo, form } = gate; |
| 18 | const to = `${repoBase(repo)}/settings#file-holds`; | |
| 23 | return fileHoldForm(ctx, form, repo, `${repoBase(repo)}/settings#file-holds`); | |
| 24 | } | |
| 25 | ||
| 26 | /** | |
| 27 | * Hold or release the form's `path` in `repo`, then return to `to` with a | |
| 28 | * flash. A repository that is missing (`undefined`) answers exactly as one | |
| 29 | * the caller cannot read, so the refusal never says which it was. | |
| 30 | */ | |
| 31 | export function fileHoldForm(ctx: Ctx, form: Form, repo: Repo | undefined, to: string): Response { | |
| 19 | 32 | const path = field(form, "path"); |
| 33 | const missing = err<FileHoldError>("not_found"); | |
| 20 | 34 | if (field(form, "verb") === "release") { |
| 21 | const released = releaseFileAs(ctx.app, ctx.auth.user, repo, path); | |
| 35 | const released: Result<boolean, FileHoldError> = repo | |
| 36 | ? releaseFileAs(ctx.app, ctx.auth.user, repo, path) | |
| 37 | : missing; | |
| 22 | 38 | return flashTo( |
| 23 | 39 | ctx, |
| 24 | 40 | to, |
| @@ -27,7 +43,7 @@ export async function fileHoldUpdate(ctx: Ctx): Promise<Response> { | ||
| 27 | 43 | : { tone: "danger", message: "That path could not be released." }, |
| 28 | 44 | ); |
| 29 | 45 | } |
| 30 | const held = holdFileAs(ctx.app, ctx.auth.user, repo, path); | |
| 46 | const held = repo ? holdFileAs(ctx.app, ctx.auth.user, repo, path) : missing; | |
| 31 | 47 | return flashTo( |
| 32 | 48 | ctx, |
| 33 | 49 | to, |
test/file-hold-form.test.ts added +133
| @@ -0,0 +1,133 @@ | ||
| 1 | // Holding a file goes through one form handler (#7055): the settings route | |
| 2 | // holds and releases through it, and a missing repository is refused in the | |
| 3 | // very words an unreadable private one is, so the refusal names neither. | |
| 4 | import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | |
| 5 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 6 | import { tmpdir } from "node:os"; | |
| 7 | import { join } from "node:path"; | |
| 8 | import { createApp, type App } from "../src/app.ts"; | |
| 9 | import { createRepo } from "../src/actions/repos.ts"; | |
| 10 | import type { Repo } from "../src/core/repos.ts"; | |
| 11 | import { createUser, type User } from "../src/core/users.ts"; | |
| 12 | import { heldFiles } from "../src/crew/file_holds.ts"; | |
| 13 | import type { Ctx } from "../src/http/context.ts"; | |
| 14 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 15 | import type { Form } from "../src/web/form.ts"; | |
| 16 | import { fileHoldForm } from "../src/web/pages/file_holds.ts"; | |
| 17 | ||
| 18 | describe("the file hold form", () => { | |
| 19 | let dir: string; | |
| 20 | let app: App; | |
| 21 | let servers: Servers; | |
| 22 | let alice: User; | |
| 23 | let bob: User; | |
| 24 | let open: Repo; | |
| 25 | let secret: Repo; | |
| 26 | ||
| 27 | const person = async (username: string, first = false): Promise<User> => { | |
| 28 | const made = await createUser(app.db, { | |
| 29 | origin: "seed", | |
| 30 | trusted: true, | |
| 31 | username, | |
| 32 | email: `${username}@forge.local`, | |
| 33 | password: "a-good-passphrase", | |
| 34 | adminIfFirst: first, | |
| 35 | }); | |
| 36 | if (!made.ok) throw new Error(made.error); | |
| 37 | return made.value; | |
| 38 | }; | |
| 39 | const cookieOf = async (username: string): Promise<string> => | |
| 40 | ( | |
| 41 | ( | |
| 42 | await fetch(`${servers.url}/signin`, { | |
| 43 | method: "POST", | |
| 44 | body: new URLSearchParams({ username, password: "a-good-passphrase" }), | |
| 45 | redirect: "manual", | |
| 46 | }) | |
| 47 | ).headers.get("set-cookie") ?? "" | |
| 48 | ).split(";")[0]!; | |
| 49 | const post = (cookie: string, repo: string, body: Record<string, string>) => | |
| 50 | fetch(`${servers.url}/${repo}/settings/holds`, { | |
| 51 | method: "POST", | |
| 52 | headers: { cookie, origin: servers.url }, | |
| 53 | body: new URLSearchParams(body), | |
| 54 | redirect: "manual", | |
| 55 | }); | |
| 56 | const formOf = (body: Record<string, string>): Promise<Form> => | |
| 57 | new Request("http://forge.local/", { method: "POST", body: new URLSearchParams(body) }).formData(); | |
| 58 | const ctxAs = (user: User | undefined): Ctx => { | |
| 59 | const url = new URL(`${servers.url}/boards`); | |
| 60 | return { app, request: new Request(url.href), url, auth: { user } } as unknown as Ctx; | |
| 61 | }; | |
| 62 | // A page's nonce is fresh on every request and its alternate link echoes | |
| 63 | // the path asked for; neither says whether the repository exists. | |
| 64 | const answer = async (res: Response, asked = "") => ({ | |
| 65 | status: res.status, | |
| 66 | location: res.headers.get("location"), | |
| 67 | cookie: res.headers.get("set-cookie"), | |
| 68 | body: (await res.text()) | |
| 69 | .replace(/nonce="[^"]*"/g, 'nonce=""') | |
| 70 | .replaceAll(encodeURIComponent(asked), "ASKED"), | |
| 71 | }); | |
| 72 | ||
| 73 | beforeAll(async () => { | |
| 74 | dir = await mkdtemp(join(tmpdir(), "file-hold-form-")); | |
| 75 | app = createApp({ dataDir: dir, port: 0 }); | |
| 76 | alice = await person("alice", true); | |
| 77 | bob = await person("bob"); | |
| 78 | servers = await serve(app); | |
| 79 | const a = await createRepo(app, alice, { name: "open", visibility: "public" }); | |
| 80 | const b = await createRepo(app, alice, { name: "secret", visibility: "private" }); | |
| 81 | if (!a.ok || !b.ok) throw new Error("repos"); | |
| 82 | open = a.value; | |
| 83 | secret = b.value; | |
| 84 | }); | |
| 85 | afterAll(async () => { | |
| 86 | servers.stop(); | |
| 87 | app.close(); | |
| 88 | await rm(dir, { recursive: true, force: true }); | |
| 89 | }); | |
| 90 | ||
| 91 | test("the settings route holds and releases, and returns to the settings card", async () => { | |
| 92 | const cookie = await cookieOf("alice"); | |
| 93 | const held = await post(cookie, "alice/open", { path: "src/a.ts" }); | |
| 94 | expect(held.status).toBe(303); | |
| 95 | expect(held.headers.get("location")).toBe("/alice/open/settings#file-holds"); | |
| 96 | expect(heldFiles(app.db, open.id).map((h) => h.path)).toEqual(["src/a.ts"]); | |
| 97 | const released = await post(cookie, "alice/open", { verb: "release", path: "src/a.ts" }); | |
| 98 | expect(released.status).toBe(303); | |
| 99 | expect(released.headers.get("location")).toBe("/alice/open/settings#file-holds"); | |
| 100 | expect(heldFiles(app.db, open.id)).toEqual([]); | |
| 101 | }); | |
| 102 | ||
| 103 | test("over the settings route, a missing and an unreadable repository answer alike", async () => { | |
| 104 | const cookie = await cookieOf("bob"); | |
| 105 | for (const body of [{ path: "src/a.ts" }, { verb: "release", path: "src/a.ts" }]) { | |
| 106 | const missing = await answer(await post(cookie, "alice/nowhere", body), "/alice/nowhere/"); | |
| 107 | const hidden = await answer(await post(cookie, "alice/secret", body), "/alice/secret/"); | |
| 108 | expect(missing.status).toBe(404); | |
| 109 | expect(hidden).toEqual(missing); | |
| 110 | } | |
| 111 | expect(heldFiles(app.db, secret.id)).toEqual([]); | |
| 112 | }); | |
| 113 | ||
| 114 | test("the shared form refuses a missing and an unreadable repository in the same words", async () => { | |
| 115 | const to = "/boards#holds"; | |
| 116 | for (const user of [bob, undefined]) { | |
| 117 | const missing = await answer(fileHoldForm(ctxAs(user), await formOf({ path: "src/a.ts" }), undefined, to)); | |
| 118 | const hidden = await answer(fileHoldForm(ctxAs(user), await formOf({ path: "src/a.ts" }), secret, to)); | |
| 119 | expect(missing.status).toBe(303); | |
| 120 | expect(missing.location).toBe(to); | |
| 121 | expect(decodeURIComponent(missing.cookie ?? "")).toContain("That file could not be held."); | |
| 122 | expect(hidden).toEqual(missing); | |
| 123 | const goneRelease = await answer( | |
| 124 | fileHoldForm(ctxAs(user), await formOf({ verb: "release", path: "src/a.ts" }), undefined, to), | |
| 125 | ); | |
| 126 | const hiddenRelease = await answer( | |
| 127 | fileHoldForm(ctxAs(user), await formOf({ verb: "release", path: "src/a.ts" }), secret, to), | |
| 128 | ); | |
| 129 | expect(hiddenRelease).toEqual(goneRelease); | |
| 130 | } | |
| 131 | expect(heldFiles(app.db, secret.id)).toEqual([]); | |
| 132 | }); | |
| 133 | }); |
new above
offline
Non-blocking, for the follow-up that wires
/boards(#7004): this function checks neither the origin nor the token scope, and it redirects to whatevertoit is handed. Say so in the doc comment: the caller must have passed a same-origin, scoped form gate such asadminForm, andtomust be a path the server built, never one taken from the request. Then the next caller cannot turn it into a CSRF hole or an open redirect.