amadan
fix: three findings from an independent review pass
Codex reviewed the branch (AGENTS.md §Review — a model that did not write the code). It returned REQUEST CHANGES with three findings. All three were traced before anything was changed; two were real as stated, one was right about the line and wrong about the reason. **The rename route parsed an unbounded body.** Reported as a revocation race — check mayWrite, block in FormValue while the client dribbles the body, write after the grant is gone. That ordering is the app's uniform pattern (openTab before decode in handlers_format, handlers_size, handlers_grid), so singling out rename would have made it the odd one out for a window every write route has. What IS unique to rename is what sits in that gap: it was the only write route reading a form with nothing bounding it, so FormValue buffered the entire request to find one field — for a title truncated to maxTitleRunes immediately after. handlers_format's comment describes this exact trap on this exact shape of post and settled on 8 KiB, so that is the number here too. The wider check-then-write question is a real one about every route and belongs in the discussion, not smuggled into one handler. TestRenameCapsTheBodyItWillParse asserts the sheet is unchanged rather than the status code, because MaxBytesReader makes the parse fail, FormValue answers "", and a blank title already means "keep the name". Mutation-tested: with the cap removed a 64 KiB body renames the sheet and the test fails, which is the whole point of writing it. **The drop overlay's recovery controls were unreachable by keyboard.** Both carried tabindex="-1" "to keep them out of the tab order while the overlay is up" — exactly backwards. The overlay is deliberately LEFT up after a refused drop so those two controls can be used to recover, and a negative tabindex sent Tab straight past them into the list behind. The other two states never needed it: `hidden` on the form removes the subtree at rest, and [data-drop-page-recover] is display:none under [data-drop-over] mid-drag. Redundant where it was harmless, harmful where it was not. drop.mjs now presses Tab rather than reading a style, because this looked perfect in a screenshot — the same lesson the menu bar taught when it worked from the keyboard and not from a mouse. Mutation-tested: putting one tabindex back fails both new checks. **Three browser drives still filled the deleted #title field.** clipboard, menus and tableformat would have timed out on sign-in — they were missed when chrome.mjs and drive.mjs were fixed, because nothing runs them in CI. All three now click the top bar's Create and rename, which is the only way a sheet is named. Verified by running all five drives against this binary; every one passes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HYALupDmJYgztTGao9WFJV
7 files changed,
+108
−7
internal/sheets/handlers_rename.go+9 −0internal/sheets/handlers_rename_test.go+34 −0internal/sheets/templates/sheets.html+13 −4tools/drive/clipboard.mjs+5 −1tools/drive/drop.mjs+35 −0tools/drive/menus.mjs+7 −1tools/drive/tableformat.mjs+5 −1
diff --git a/internal/sheets/handlers_rename.go b/internal/sheets/handlers_rename.go| index 38f19dc..f91f78f 100644 |
| --- a/internal/sheets/handlers_rename.go |
| +++ b/internal/sheets/handlers_rename.go |
| @@ -51,6 +51,15 @@ func (a *App) handleSheetRename(w http.ResponseWriter, r *http.Request) { |
| 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) |
| + |
| // 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. |
diff --git a/internal/sheets/handlers_rename_test.go b/internal/sheets/handlers_rename_test.go| index 90a3e6f..aa3a605 100644 |
| --- a/internal/sheets/handlers_rename_test.go |
| +++ b/internal/sheets/handlers_rename_test.go |
| @@ -111,6 +111,40 @@ func TestRenameTruncatesToTheSameCapAsCreate(t *testing.T) { |
| } |
| } |
| +// The body is capped before it is parsed. |
| +// |
| +// An independent review pass found rename reading its form with nothing |
| +// bounding the request — the only write route in the app doing so. |
| +// FormValue buffers the WHOLE body to pull one field out of it, so |
| +// 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. |
| +func TestRenameCapsTheBodyItWillParse(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Budget") |
| + |
| + // 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()) |
| + } |
| + got, err := app.sheetSummary(s.ID, signedInMember(t, app)) |
| + if err != nil { |
| + t.Fatalf("sheetSummary: %v", err) |
| + } |
| + if got.Title != "Budget" { |
| + t.Errorf("an oversized body renamed the sheet to %q; the cap is not there", 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/internal/sheets/templates/sheets.html b/internal/sheets/templates/sheets.html| index 8269e9e..4844b61 100644 |
| --- a/internal/sheets/templates/sheets.html |
| +++ b/internal/sheets/templates/sheets.html |
| @@ -138,12 +138,21 @@ first worksheet is imported.</p> |
| a file picker to somebody mid-drag is a second way to do the |
| thing they are already doing. |
| - tabindex="-1" keeps them out of the tab order while the overlay |
| - is up; `hidden` on the form handles it the rest of the time. */}} |
| + They carry no tabindex, and its absence is a fix rather than an |
| + omission. Both used to carry tabindex="-1", to "keep them out of |
| + the tab order while the overlay is up" — which is exactly backwards. |
| + The overlay stays up after a REFUSED drop precisely so these two |
| + controls can be used, and a negative tabindex meant a keyboard user |
| + tabbed straight past the recovery form into the list behind it. |
| + The other two states never needed the help: `hidden` on the form |
| + takes the whole subtree out of the tab order at rest, and |
| + [data-drop-page-recover] is display:none under [data-drop-over], |
| + which does the same mid-drag. So it was redundant where it was |
| + harmless and harmful where it was not. */}} |
| <div data-drop-page-recover> |
| -<input rst-input name="file" type="file" accept=".xlsx,.xlsm" tabindex="-1" |
| +<input rst-input name="file" type="file" accept=".xlsx,.xlsm" |
| aria-label="Spreadsheet to import"> |
| -<button rst-btn="primary" type="submit" tabindex="-1">Import</button> |
| +<button rst-btn="primary" type="submit">Import</button> |
| </div> |
| </div> |
| </div> |
diff --git a/tools/drive/clipboard.mjs b/tools/drive/clipboard.mjs| index 61d8f16..97d58ec 100644 |
| --- a/tools/drive/clipboard.mjs |
| +++ b/tools/drive/clipboard.mjs |
| @@ -33,7 +33,11 @@ await page.goto(`${base}/signin`); await page.fill('input[type=email]','paul@exa |
| await Promise.all([page.waitForLoadState('networkidle'),page.click('button[type=submit]')]); |
| const mm=[...fs.readFileSync(process.env.SHEETS_LOG,'utf8').matchAll(/http:\/\/127\.0\.0\.1:8799\/auth\/verify\?[^\s\\"]+/g)].pop(); |
| await page.goto(mm[0]); await page.waitForURL(/\/sheets/); |
| -await page.fill('#title','Clip2'); await Promise.all([page.waitForURL(/\/sheets\/\d+$/),page.click('form[action="/sheets"] button[type=submit]')]); |
| +// Create from the top bar, then name it by renaming: the list's #title |
| +// field is gone, so a sheet is born "Untitled sheet" and named after. |
| +await Promise.all([page.waitForURL(/\/sheets\/\d+$/),page.click('[rst-shell-tail] form[action="/sheets"] button[type=submit]')]); |
| +await page.fill('[data-doc-title-input]','Clip2'); |
| +await Promise.all([page.waitForURL(/\/sheets\/\d+$/),page.press('[data-doc-title-input]','Enter')]); |
| await page.waitForFunction(()=>window.SheetsGrid&&window.SheetsGrid.worksheet()); |
| const w=(r,c,v)=>page.evaluate(([r,c,v])=>window.SheetsGrid.write(r,c,v),[r,c,v]); |
| const sel=(x1,y1,x2,y2)=>page.evaluate(([a,b,c,d])=>window.SheetsGrid.worksheet().updateSelectionFromCoords(a,b,c,d),[x1,y1,x2,y2]); |
diff --git a/tools/drive/drop.mjs b/tools/drive/drop.mjs| index 542035e..c96deba 100644 |
| --- a/tools/drive/drop.mjs |
| +++ b/tools/drive/drop.mjs |
| @@ -176,6 +176,41 @@ check('two files are refused in words', refused.hint === 'Drop one file at a tim |
| check('...with the overlay left up to say so', refused.hidden === false, refused); |
| check('...and the chooser back, to recover with', refused.recover !== 'none', refused.recover); |
| check('...having submitted nothing', refused.path === '/sheets', refused.path); |
| + |
| +/* The recovery form must be reachable by KEYBOARD, not merely visible. |
| + * |
| + * An independent review found both controls carrying tabindex="-1", |
| + * which put them out of the tab order at the one moment they exist to |
| + * be used: the overlay stays up after a refusal so they can be used to |
| + * recover, and Tab went straight past them into the list behind. It |
| + * looked completely correct in a screenshot, which is why this check |
| + * presses Tab instead of reading a style — the same lesson the menu bar |
| + * taught when it worked from the keyboard and not from a mouse. |
| + * |
| + * Tabbing from the panel must land ON the panel's own controls. */ |
| +const reachable = await page.evaluate(() => { |
| + const rec = document.querySelector('[data-drop-page-recover]'); |
| + const controls = [...rec.querySelectorAll('input,button')]; |
| + return { |
| + negative: controls.filter((c) => c.tabIndex < 0).length, |
| + count: controls.length, |
| + }; |
| +}); |
| +check('the recovery controls are in the tab order', reachable.negative === 0 |
| + && reachable.count === 2, JSON.stringify(reachable)); |
| + |
| +/* And prove it by actually tabbing: focus the panel, press Tab, and the |
| + * focus must be inside the panel rather than on the list underneath. */ |
| +await page.evaluate(() => document.querySelector('[data-drop-page-recover] input').focus()); |
| +const focusedInPanel = await page.evaluate(() => |
| + !!document.activeElement.closest('[data-drop-page-recover]')); |
| +check('...and focus really lands in the panel', focusedInPanel, 'focus escaped the overlay'); |
| +await page.keyboard.press('Tab'); |
| +const nextInPanel = await page.evaluate(() => |
| + !!document.activeElement.closest('[data-drop-page-recover]')); |
| +check('...and Tab moves to Import, not past the overlay', nextInPanel, |
| + 'Tab left the recovery form'); |
| + |
| await page.screenshot({ path: OUT + '/list-refused.png' }); |
| await page.keyboard.press('Escape'); |
diff --git a/tools/drive/menus.mjs b/tools/drive/menus.mjs| index 01c11ab..d508b85 100644 |
| --- a/tools/drive/menus.mjs |
| +++ b/tools/drive/menus.mjs |
| @@ -48,7 +48,13 @@ await page.goto(`${base}/signin`); await page.fill('input[type=email]','paul@exa |
| await Promise.all([page.waitForLoadState('networkidle'),page.click('button[type=submit]')]); |
| const mm=[...fs.readFileSync(LOG,'utf8').matchAll(/http:\/\/127\.0\.0\.1:8799\/auth\/verify\?[^\s\\"]+/g)].pop(); |
| await page.goto(mm[0]); await page.waitForURL(/\/sheets/); |
| -if(!(await page.$('[data-sheet-list]'))){await page.fill('#title','Audit');await Promise.all([page.waitForURL(/\/sheets\/\d+$/),page.click('form[action="/sheets"] button[type=submit]')]);} |
| +// No sheets yet: Create from the top bar, then name it by renaming. |
| +// The list's #title field is gone — a sheet is born "Untitled sheet". |
| +if(!(await page.$('[data-sheet-list]'))){ |
| + await Promise.all([page.waitForURL(/\/sheets\/\d+$/),page.click('[rst-shell-tail] form[action="/sheets"] button[type=submit]')]); |
| + await page.fill('[data-doc-title-input]','Audit'); |
| + await Promise.all([page.waitForURL(/\/sheets\/\d+$/),page.press('[data-doc-title-input]','Enter')]); |
| +} |
| else {await page.click('[data-sheet-list] [rst-row-main] a'); await page.waitForURL(/\/sheets\/\d+$/);} |
| const url=page.url(); |
| await page.waitForFunction(()=>window.SheetsGrid&&window.SheetsGrid.worksheet()); |
diff --git a/tools/drive/tableformat.mjs b/tools/drive/tableformat.mjs| index 31eb994..040da0f 100644 |
| --- a/tools/drive/tableformat.mjs |
| +++ b/tools/drive/tableformat.mjs |
| @@ -26,7 +26,11 @@ await page.goto(`${base}/signin`); await page.fill('input[type=email]','paul@exa |
| await Promise.all([page.waitForLoadState('networkidle'),page.click('button[type=submit]')]); |
| const mm=[...fs.readFileSync(process.env.SHEETS_LOG,'utf8').matchAll(/http:\/\/127\.0\.0\.1:8799\/auth\/verify\?[^\s\\"]+/g)].pop(); |
| await page.goto(mm[0]); await page.waitForURL(/\/sheets/); |
| -await page.fill('#title','Table format'); await Promise.all([page.waitForURL(/\/sheets\/\d+$/),page.click('form[action="/sheets"] button[type=submit]')]); |
| +// Create from the top bar, then name it by renaming: the list's #title |
| +// field is gone, so a sheet is born "Untitled sheet" and named after. |
| +await Promise.all([page.waitForURL(/\/sheets\/\d+$/),page.click('[rst-shell-tail] form[action="/sheets"] button[type=submit]')]); |
| +await page.fill('[data-doc-title-input]','Table format'); |
| +await Promise.all([page.waitForURL(/\/sheets\/\d+$/),page.press('[data-doc-title-input]','Enter')]); |
| const url=page.url(); |
| await page.waitForFunction(()=>window.SheetsGrid&&window.SheetsGrid.worksheet()); |
| for(const [r,c,v] of [[0,0,'Region'],[0,1,'Revenue'],[1,0,'North'],[1,1,'1200'],[2,0,'South'],[2,1,'900']]) |