amadan
fix(rename): the query could name a sheet the body was refused for
A second review pass on the previous commit found a defect the first
fix introduced, and it is the more interesting of the two bugs.
r.FormValue merges the URL query into the form AND swallows the parse
error. So capping the body without checking that error opened a new
door: POST /sheets/1/rename?title=Wrong with an oversized body, the cap
rejects the body, FormValue falls through to the query, and the sheet
is renamed to whatever the URL said. A rename that can travel in a link
is precisely what a POST route must not accept. ParseForm is now called
explicitly, its error answered with 400, and the title read from
PostForm — the body a person submitted, never the URL.
The same pass declined to accept the deferred revocation race, and on
re-reading it was right to. The window is app-wide, but the thing that
makes it exploitable rather than theoretical is that the step between
the check and the write is the BODY READ, whose duration the client
chooses — an editor can open a rename, hold the body while their grant
is revoked, and complete the write afterwards. So the order here is now
cap, parse, authorise, write: the client-paced delay happens before
anything is checked, and mayWrite to UPDATE is straight-line code. That
costs a forbidden request the price of parsing its own 8 KiB, which is
the cheaper half of the trade. Whether every other write route should
be reordered the same way is a real question for the discussion — it is
noted in the handler rather than answered by it.
Both new guards are mutation-tested, and the first attempt failed that
test in a way worth recording: with ParseForm's error checked, a
well-formed body wins under FormValue too, so swapping PostForm back in
passed and proved nothing. The discriminating case is a body carrying
NO title beside a query that does — FormValue answers "FromTheQuery"
and renames, PostForm answers blank and keeps the name. That case is in
the test now, and it fails on the mutation.
Also fixes drive.mjs, which could not run at all: it destructured
`{ chromium }` from a CJS playwright build, where the exports sit on
.default, and died at chromium.launch. Pre-existing and unrelated to
this branch, but it is one of the five drives that verify this work, so
it could not stay broken. All five now pass against this binary.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HYALupDmJYgztTGao9WFJV
3 files changed,
+131
−23
internal/sheets/handlers_rename.go+48 −13internal/sheets/handlers_rename_test.go+75 −9tools/drive/drive.mjs+8 −1
CI passed — run details
Intent
Attributed to this commit: 152 g CO₂e (17251090 tokens). This is an attribution, not a measurement — usage counted from this commit's own marker forward to the next one on worktree-list-chrome-rebase, never backward from it. A turn that produced more than one commit books all of its tokens to the last one, and a concurrent session on this branch can add to the same figure. Some of it was priced using the shared default rate, because no model-specific factor row exists yet for at least one model involved. The Carbon view has the repo total, which is never derived from this attribution.
diff --git a/internal/sheets/handlers_rename.go b/internal/sheets/handlers_rename.go| index f91f78f..f0ddfb1 100644 |
| --- a/internal/sheets/handlers_rename.go |
| +++ b/internal/sheets/handlers_rename.go |
| @@ -46,24 +46,59 @@ func (a *App) handleSheetRename(w http.ResponseWriter, r *http.Request) { |
| a.notFound(w, r) |
| return |
| } |
| - if !a.mayWrite(s.ID, viewer) { |
| - a.notFound(w, r) |
| + // Bounded, then parsed, and only THEN authorised. The order is the |
| + // point and it is deliberately not the order the rest of the app |
| + // uses. |
| + // |
| + // Every other write route here authorises first and reads the body |
| + // after (openTab before the decode, in handlers_format and |
| + // handlers_size). That leaves the body read — the one step whose |
| + // duration the client chooses — sitting between the permission |
| + // check and the write, so an editor can begin a rename, hold the |
| + // body open while their grant is revoked, then finish it and have |
| + // the write go through. Reading first collapses that: the delay a |
| + // client controls now happens BEFORE anything is checked, and the |
| + // gap between mayWrite and the UPDATE is straight-line code. |
| + // |
| + // It is not free — a request that turns out to be forbidden has |
| + // paid to parse its own body — but the cap on the line below is |
| + // what makes that cheap enough to be the obvious trade. The wider |
| + // question, whether every other write route should be reordered |
| + // the same way, is a real one and belongs in the branch discussion |
| + // rather than in this handler. |
| + // |
| + // 8 KiB matches handlers_format's cap on the same shape of form |
| + // post. Go's own ParseForm ceiling for a urlencoded body is 10 MiB, |
| + // so this is not the difference between bounded and unbounded — it |
| + // is three orders of magnitude off a limit no rename needs, for a |
| + // value truncated to maxTitleRunes moments later. |
| + r.Body = http.MaxBytesReader(w, r.Body, 8<<10) |
| + if err := r.ParseForm(); err != nil { |
| + // Refused rather than carried on with. FormValue would swallow |
| + // this error and fall through to the QUERY, which is how |
| + // ?title=x on an oversized body silently became the new name — |
| + // caught by review, not by the suite. |
| + http.Error(w, "Malformed request.", http.StatusBadRequest) |
| return |
| } |
| - |
| - // The body is bounded BEFORE it is parsed. Without this, rename was |
| - // the one write route in the app reading a form with nothing |
| - // capping it: FormValue buffers the whole request to find a single |
| - // field, so an unbounded body is memory held for a title that gets |
| - // truncated to maxTitleRunes a moment later anyway. handlers_format |
| - // describes the identical trap on the identical shape of form post |
| - // and settled on 8 KiB; there is no reason for a second number. |
| - r.Body = http.MaxBytesReader(w, r.Body, 8<<10) |
| - |
| + // PostForm, never Form: the title comes from the body a person |
| + // submitted, never from the URL. r.FormValue merges the query in, |
| + // so with it a GET-shaped ?title= could rename a sheet — a |
| + // state-changing value arriving somewhere it can be linked to. |
| + // |
| // Read field by field into a local, never bound onto a GORM model |
| // (AGENTS.md / Mechanical rules): a form that could reach the model |
| // could set CreatedBy, or DeletedAt, or an id. |
| - if err := a.renameSheet(s.ID, r.FormValue("title")); err != nil { |
| + title := r.PostForm.Get("title") |
| + |
| + // Last thing before the write, with nothing client-paced in |
| + // between. See the note above on why this moved down here. |
| + if !a.mayWrite(s.ID, viewer) { |
| + a.notFound(w, r) |
| + return |
| + } |
| + |
| + if err := a.renameSheet(s.ID, title); err != nil { |
| if errors.Is(err, errBlankTitle) { |
| // Straight back to the sheet, which still shows the name it |
| // had. Nothing was changed and there is nothing to explain |
diff --git a/internal/sheets/handlers_rename_test.go b/internal/sheets/handlers_rename_test.go| index aa3a605..fa05307 100644 |
| --- a/internal/sheets/handlers_rename_test.go |
| +++ b/internal/sheets/handlers_rename_test.go |
| @@ -119,13 +119,10 @@ func TestRenameTruncatesToTheSameCapAsCreate(t *testing.T) { |
| // without the cap a megabyte of "title=" is a megabyte held in memory |
| // on the way to a value that gets truncated to maxTitleRunes anyway. |
| // |
| -// The assertion is deliberately about the sheet rather than the status |
| -// code: MaxBytesReader makes the oversized parse fail, FormValue then |
| -// answers "", and a blank title is already refused by keeping the name |
| -// the sheet had. So the observable, load-bearing fact is that a body |
| -// far past the cap changes nothing — which is also what fails if |
| -// somebody later removes the MaxBytesReader line, since the rename |
| -// would then succeed. |
| +// An oversized body is REFUSED, not quietly ignored. The parse error is |
| +// checked rather than swallowed, which is the difference between this |
| +// and the first attempt at the fix — see the query test below for what |
| +// swallowing it cost. |
| func TestRenameCapsTheBodyItWillParse(t *testing.T) { |
| app, cookies := signIn(t) |
| s, _ := newSheet(t, app, cookies, "Budget") |
| @@ -133,8 +130,8 @@ func TestRenameCapsTheBodyItWillParse(t *testing.T) { |
| // Well past the 8 KiB cap, as one enormous field value. |
| huge := url.Values{"title": {strings.Repeat("x", 64<<10)}} |
| rec := do(t, app, as(postForm(renamePath(s.ID), huge), cookies)) |
| - if rec.Code != http.StatusSeeOther { |
| - t.Fatalf("oversized rename = %d, want 303: %s", rec.Code, rec.Body.String()) |
| + if rec.Code != http.StatusBadRequest { |
| + t.Fatalf("oversized rename = %d, want 400: %s", rec.Code, rec.Body.String()) |
| } |
| got, err := app.sheetSummary(s.ID, signedInMember(t, app)) |
| if err != nil { |
| @@ -145,6 +142,75 @@ func TestRenameCapsTheBodyItWillParse(t *testing.T) { |
| } |
| } |
| +// The title comes from the body, never from the URL. |
| +// |
| +// This is the defect the second review pass found in the first fix. Go's |
| +// r.FormValue merges the query string into the form, and it swallows a |
| +// parse failure rather than reporting it — so capping the body without |
| +// checking the error created a NEW way in: POST ?title=Wrong with an |
| +// oversized body, the cap rejects the body, FormValue falls through to |
| +// the query, and the sheet is renamed to whatever the URL said. A |
| +// state-changing value that can travel in a link is exactly what a POST |
| +// route must not accept. |
| +// |
| +// Both halves are asserted, because either alone would pass a broken |
| +// handler: the query is ignored when the body is fine, and an oversized |
| +// body is refused outright rather than falling back to it. |
| +func TestRenameTakesTheTitleFromTheBodyNotTheQuery(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Budget") |
| + |
| + // A good body and a hostile query: the body must win. |
| + req := postForm(renamePath(s.ID)+"?title=FromTheQuery", |
| + url.Values{"title": {"FromTheBody"}}) |
| + if rec := do(t, app, as(req, cookies)); rec.Code != http.StatusSeeOther { |
| + t.Fatalf("rename = %d, want 303: %s", rec.Code, rec.Body.String()) |
| + } |
| + got, err := app.sheetSummary(s.ID, signedInMember(t, app)) |
| + if err != nil { |
| + t.Fatalf("sheetSummary: %v", err) |
| + } |
| + if got.Title != "FromTheBody" { |
| + t.Errorf("title is %q, want the body's value — the query reached the write", got.Title) |
| + } |
| + |
| + // A body carrying no title at all, and a query that does. This is |
| + // the case that actually distinguishes PostForm from FormValue, and |
| + // it exists because the first version of this test did not: with |
| + // ParseForm's error checked, a well-formed body wins under either |
| + // spelling, so swapping PostForm back for FormValue passed. Here |
| + // r.Form merges the query in and FormValue would answer |
| + // "FromTheQuery"; PostForm answers "", which is blank, which keeps |
| + // the name the sheet had. |
| + req = postForm(renamePath(s.ID)+"?title=FromTheQuery", |
| + url.Values{"unrelated": {"x"}}) |
| + if rec := do(t, app, as(req, cookies)); rec.Code != http.StatusSeeOther { |
| + t.Fatalf("rename with no body title = %d, want 303", rec.Code) |
| + } |
| + got, err = app.sheetSummary(s.ID, signedInMember(t, app)) |
| + if err != nil { |
| + t.Fatalf("sheetSummary: %v", err) |
| + } |
| + if got.Title != "FromTheBody" { |
| + t.Errorf("title is %q; the query renamed the sheet without a body field", got.Title) |
| + } |
| + |
| + // An oversized body with a query title: refused, and the query must |
| + // not stand in for the body it could not read. |
| + huge := url.Values{"title": {strings.Repeat("x", 64<<10)}} |
| + req = postForm(renamePath(s.ID)+"?title=FromTheQuery", huge) |
| + if rec := do(t, app, as(req, cookies)); rec.Code != http.StatusBadRequest { |
| + t.Fatalf("oversized rename = %d, want 400", rec.Code) |
| + } |
| + got, err = app.sheetSummary(s.ID, signedInMember(t, app)) |
| + if err != nil { |
| + t.Fatalf("sheetSummary: %v", err) |
| + } |
| + if got.Title != "FromTheBody" { |
| + t.Errorf("title is %q; a rejected body let the query rename the sheet", got.Title) |
| + } |
| +} |
| + |
| // A viewer may not rename, and is told 404 rather than 403 — AGENTS.md |
| // requires it, because a 403 confirms the sheet exists to somebody who |
| // was never shown it. |
diff --git a/tools/drive/drive.mjs b/tools/drive/drive.mjs| index 74215fd..01bb025 100644 |
| --- a/tools/drive/drive.mjs |
| +++ b/tools/drive/drive.mjs |
| @@ -38,7 +38,14 @@ |
| import net from 'node:net'; |
| import fs from 'node:fs'; |
| const PLAYWRIGHT = process.env.PLAYWRIGHT || 'playwright'; |
| -const { chromium } = await import(PLAYWRIGHT); |
| +/* A CJS build imported from ESM puts its exports on .default, so a |
| + * plain `const { chromium } = await import(...)` yields undefined and |
| + * dies one line later at chromium.launch — which is how this read until |
| + * the branch that added the checks below tried to run it. chrome.mjs |
| + * already spelled it this way; the two now agree. */ |
| +const pw = await import(PLAYWRIGHT); |
| +const chromium = pw.chromium || (pw.default && pw.default.chromium); |
| +if (!chromium) { console.error('playwright: no chromium export'); process.exit(1); } |
| const SOCK = process.env.SHEETS_SOCK || '/tmp/s.sock'; |
| const LOG = process.env.SHEETS_LOG || '/tmp/s.log'; |