oficina / suite Public

<task-notification> <task-id>a1e59b2995bc1ef9e</task-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-suite/df6721cd-97ce-45b2-b664-3fea70beada0/tasks/a1e59b2995bc1ef9e.output</output-file> <status>completed</status> <summary>Agent "Fable re-review of fix commit" finished</summary> <note>A task-notification fires each time this agent stops with no live background children of its own. The user can send it another message and resume it, so the same task-id may notify more than once.</note> <result># Re-review of `c8415ce` (docs `sharing` branch) Verified by reading the full diff and current sources, plus three throwaway tests and two mutations run against the tree (all deleted afterwards; `git status --short` is empty). ## Findings, most severe first ### 1. BUG (blocking) — a revoked guest writer's saves report "ok" while the server drops them `internal/docs/auth.go` `requireAdmission` (≈L366–377); `fixtures/editor-entry.js:412–418`; `fixtures/save-queue.mjs:78–81`. `requireAdmission` answers **every** method with `303 → /signin`. rastrillo's own `refuseSession` redirects only GET/HEAD and 403s anything else, for exactly this reason. The editor's `fetch(base, {method:'POST'})` follows redirects by default, so the POST becomes `GET /signin` → 200 HTML → `res.ok` → the save queue reports `'ok'`. Scenario: Alice grants Bob (a guest) writer, Bob is typing, Alice revokes. Every subsequent keystroke batch is 303'd, followed, and reported as saved. No "failed" indicator ever lights; Bob types for an hour into a page that says everything is fine. This is strictly worse than the reader-editor blocker you just fixed — that one at least showed "save failed". Measured with a throwaway test: ``` REVOKED GUEST POST /updates -&gt; status 303, Location "/signin" REVOKED GUEST GET /updates -&gt; status 303 (fetchLog/fetchSince get HTML, decode fails) ``` Fix: in `requireAdmission`, redirect only for GET/HEAD and `http.Error(..., 403)` otherwise (mirror `refuseSession`). Add a test that a revoked guest's `POST /d/{sqid}/updates` is a 4xx, not a 3xx. ### 2. BUG (blocking) — the guest share-guard test is not load-bearing; the guard is unverified `internal/docs/guest_test.go:53–55`; `internal/docs/share.go` sharePost/shareRevoke, `publish.go` shareLink. Mutation: removed the `if !h.member(r)` block from `sharePost` → `TestAGuestCannotCreateOrShare` **still passes** (Bob doesn't own Alice's document, so the owner check 404s him anyway; the test only asserts `!= 303`). The `create` mutation is caught. So of the "five mutations caught", the share one was not caught by this test, and `shareRevoke`/`shareLink` have no guest test at all. The commit's claim is wrong for three of the four guarded routes. Fix (cheap): assert `rec.Code != http.StatusForbidden` → error for share, revoke and link (a 404 then kills the mutation). Better: build a guest who owns a document (two `NewForTestWith` on one `DBPath`: first allowlist `alice,bob`, Bob creates; second allowlist `alice` with Bob holding a grant) and assert 403 on all three. ### 3. BUG — guests are shown "New document" controls that always fail with a plain-text generic error `internal/docs/menu.go:172` (File → New document `&lt;form action="/documents"&gt;`), `views/show.html:1381`, `views/list.html` create form (header, unconditional). Verified: guest show page and list page both contain `action="/documents"`; `POST /documents` as guest → `403 "docs.error.generic"`. This is the "appearance of capability" you asked me to hunt for: a scriptless form in the File menu and the list header that a guest can submit and that lands on an unstyled text page. Gate both on membership (`listPage.CanCreate` / `showPage.CanCreate` from `h.member(r)`). ### 4. RISK — ejection on a transient DB error, and the landing page lies `internal/docs/app.go:393–402`, `grants.go` HasGrant, `auth.go` requireAdmission. A member is unaffected (short-circuit). A guest on a DB hiccup or a &gt;2s stall: `HasGrant` logs an error, returns false, and the guest is silently 303'd to `/signin` with their session cookie intact (`Set-Cookie ""` observed). The sign-in page shows the form; if they submit, `gateBegin` says "link sent" and nothing arrives. The 2s timeout makes spurious ejection *more* likely under load, not less. Not a loop (`/signin` never redirects signed-in users), but the failure is unexplained and indistinguishable from real revocation. Suggest `HasGrant` returning `(bool, error)` so `allows` can 503 on error and eject only on a definite false; and on a definite revocation, clear the session (`sessions.SignOut`) and redirect with a reason param. ### 5. RISK — the one-shot render has no `Cache-Control` `internal/docs/share.go:91 renderShare`. Observed `Cache-Control=""` on the render that prints the URL. The plaintext capability sits in the browser's disk cache/bfcache on a shared machine, and any intermediary may keep it. "Shown once" is only a server-side truth today. Add `no-store` to `renderShare` (at least when `PublicURL != ""`). ### 6. RISK — one-shot publish race shows a dead URL `internal/docs/publish.go:116–121`. Two concurrent "Replace the link" clicks: R1 commits token A, R2 commits token B (deleting A), R2 `put(B)`, R1 `put(A)` → the page shows A, which 404s. Rare, same owner, low impact. Also `renderShare` `take()`s even when `IsPublished` errored, so the URL is silently discarded. Otherwise the one-shot holds: key is `(ownerID, sqid)`, only the owner can reach `renderShare` (owner-scoped `Get`), nothing logs the value, sweep-on-put bounds the map to one entry per (owner, doc), 30s is fine. On the design question: in-memory is defensible for one process; the standard alternative is a short-lived encrypted flash cookie (survives restart, no shared state, no sweep). Rendering the POST response directly is worse (resubmit rotates). Not blocking. ### 7. RISK — a revoked reader's open SSE stream lives up to 5 minutes `internal/docs/stream.go:127–149`, TTL 5m. Admission is checked at open only; the stream carries seq numbers, not bytes, and the catch-up fetch now 303s, so the leak is "the document changed", not content. Minor; note it in the `requireAdmission` comment. ### 8. NIT — readers get the full inert formatting bar and greyed menus `views/show.html:1373–1517`. Your claim is verified: command items ship `disabled`, only `menu.mjs`/`toolbar.mjs` enable them, and both are imported solely by `editor-entry.js` (the jumpto bundle imports nothing). So no write surface. But a reader sees a full Bold/Italic/Style/Font/Size bar, Edit/Insert/Format menus all greyed, and Print/View-mode commands they can't use — the "disabled editor look" the commit message argues against. Consider gating `[docs-bar]` and the command-only menus on `CanWrite`, or adding a read-only notice like `public.html`. ### 9. NIT — self-grant check swallows the lookup error `internal/docs/grants.go:181`: `if ownerAddress, err := s.addressOf(...); err == nil &amp;&amp; ...` — on a DB error the self-grant is allowed. ### 10. NIT — stale comments the branch claims to have fixed `internal/docs/export.go:6` still says "a document the signed-in person already owns"; `handlers.go:626–627` (`getDocumentForUpdates`) still says "loads the document through Store.Get". ### 11. NIT — items marked done with no test `seedOnlyMembers` (finding 9), `created_by` population, `no-store`/`X-Robots-Tag` on `/p/`. `TestRevokingTheLastGrantEndsAdmission` and the create assertion accept any non-200/non-303 (a 500 passes); assert the exact status. ### 12. NIT — migrations edited in place Fine pre-merge (main has only 0001–0004), but dev databases built from earlier branch revisions have `links.token` and no `grants.created_by`; `CREATE TABLE IF NOT EXISTS` won't repair them. Say so in the commit or rename to 0007. ## Answers to your specific questions - **Guest route enumeration (all reachable after `requireAdmission`):** `GET /` ListFor ✓; `POST /documents` 403 ✓ (but see #3); `GET /documents/search` SearchFor ✓; `GET /d/{sqid}` GetFor + CanWrite ✓; `GET /d/{sqid}/share` owner-scoped 404 ✓; `POST share|revoke|link` 403 ✓ (untested, #2); `GET/POST updates` GetFor + writable ✓ (but #1); `export.md/.pdf` GetFor ✓; `stream` GetFor ✓ (#7); `POST /signout` ✓. A guest creates only a `Person` row (after admission; no seed — `seedOnlyMembers` verified by reading, untested). No path mints a document or grant. `EnsurePerson` creates nothing for a non-admitted address since it runs after the middleware. - **Hashing:** unsalted SHA-256 is right for 128-bit random input. Collision on `Publish` → UNIQUE insert error → 500, at 2⁻¹²⁸. `ByToken` answers unknown/revoked/deleted identically (`TestPublicRefusalsAreIndistinguishable` pins it). Grep confirms the plaintext exists only in `Publish`'s return, `shareLink`'s `put`, and the template; nothing logs it. - **Test-helper fixes:** `TestSignIn`'s send-count + recipient check is correct and load-bearing. `reader_noeditor_test.go` is sound (mutating `CanWrite` to true fails it; the writer test guards the inverse). `grants_edges_test.go` is sound. `guest_test.go` is the weak one (#2). ## Would I merge `c8415ce` to main now? **No.** #1 reintroduces the exact failure class (a person told their edits are saved when they are not) that the blocker fix was for, and #2 means the member gate on three routes has never been exercised. Required before merge: 1. `requireAdmission`: 303 only for GET/HEAD, 403 otherwise; add a test that a revoked guest's `POST /d/{sqid}/updates` is a 4xx. 2. Make the guest share/revoke/link assertions load-bearing (assert 403; ideally with a guest who owns a document). 3. Gate the two `/documents` create forms on membership. #5 (no-store on the share page) is a one-liner and should go in the same commit. Everything else can follow.</result> <usage><subagent_tokens>280432</subagent_tokens><tool_uses>58</tool_uses><duration_ms>730450</duration_ms></usage> </task-notification>