amadan

oficina / home Public

← Discussions

Review: Home's own pages on the shared chrome (R2 row 8) open

On branch feat/r2-shared-chrome

claude operated by paul@keymail.dev session 21a32889 · claude-code/2.1.280 · MacBookPro.localdomain

Review request: Home's own signed-in pages on the shared chrome. This is estilo spec shared-chrome §12.1 row 8 (Home), plus Home findings C1, C2 and C4 from polish audit r3.

Stacked on chore/rastrillo-path (#80). I branched from it, not from main, because both branches touch the layout, tokens.css and every render call site. Stacking meant one set of conflicts instead of two. Merge #80 first, then merge origin/main into this branch before its squash, following R1-BRIEF's stacked-branch note. git diff chore/rastrillo-path..feat/r2-shared-chrome is this branch's own change.

Commits

  • 535d41c shared_chrome_test.go, red on purpose (polish-audit/r2-home-chrome-red.txt). It has seven tests: the bar on every page, the front page, Appearance, error pages split by Accept, the destination kept (C1), the hop's sky (C2) and the preloads (C4). One assertion was relaxed in b8894b7. A non-browser's GET to a POST-only path keeps its old 405 (an existing test pins it), so plain text may be 404 or 405.
  • b8894b7 the change.

What changes

  • The bar. estilo's 52px estilo-bar, branded Oficina, is on every page except the public front page. Stylesheets load in the spec's order (base, menu, chrome).
  • Directory (oficina.works), signed in. One estilo account menu: - The identity row says "Signed in", with the person glyph. The directory holds a person ref and never an address (the one rule), so it cannot show who you are. - Passkeys and Recovery codes link to id.*'s /passkey/enrol and /passkey/codes. Until now nothing linked to them. - Appearance: Light, Dark or System. It POSTs to /appearance, which sits inside Protect and outside any session requirement. The cookie is per host. - Help → Open source. - The one Sign out. - The lone Help dropdown and the hub's Sign out button are gone.
  • Directory, signed out. The corner says "Sign in" (/auth/start?return_to=…). The signed-out page leaves the corner empty.
  • id.oficina.works. Every page is estilo's door (estilo-door-open/close, the card on the sky) under a brand-only bar. The brand links to the directory. Nothing account-shaped renders on the credential origin.
  • Team home. "Your apps" sits on the sky in a full-width header band under the bar. The sky appears only on the door, this header and the front page's hero. The team chooser, Open source and the error pages are plain pages.
  • Errors. estilo's error page on both origins for Accept: text/html, with the account menu when signed in, and no-store. Other clients get plain text. Unknown hosts keep the empty 404. A template that fails to render, and the two passkey http.Error sites, now give the 500 page with a logged reference.
  • Titles. - Pages are titled <Page> · Oficina. - The directory and front page are "Oficina", which was "Oficina — open your new office suite" (spec §10). - The Docs continuation gains " · Oficina". - No user-visible "Home" was left: door-copy had already removed the catalog's. The footer "Source → Home" link is left for Paul.
  • C1. "Use a different address" and "Start again" now go to /signin?return_to=<the remembered return>. A bare /signin cleared the return cookie, so the next link landed you on the signed-out front page.
  • C2. The hand-off hop keeps the bar and the sky's inline placeholder (a data: URI, so no request). Only its card waits 1.5s. It still has exactly one link, no skip link and no busy.js, as the continuation tests require.
  • C4. Onest and the Oficina mark are preloaded on every page, and the six app marks on the front page.
  • Not loaded: chrome.js (Escape and outside-click close). AGENTS.md's script rule prefers none, and every menu works without it. The cost, as estilo states it: an open menu closes from its own trigger. AGENTS.md now records this.

Evidence

  • make test-vet test-fmt test-build test-go: green. make test-browser: green.
  • Shared chrome check (polish-audit/r2-home-check.config.mjs; output in r2-home-check.output.txt): 691 ok, 0 FAIL. It covers 10 signed-out and 4 signed-in URLs across both origins at 1440, 840, 800, 390 and 320, plus the menu stacking, list-row colour, 404-with-bar and CLS hops. On the base branch it fails has [estilo-bar] everywhere.
  • r2-home-extra.mjs: 53 ok, 0 FAIL. It checks what the shared check can't reach (output in r2-home-extra.output.txt): - id.'s account pages: brand-only bar, 52px, the right title, no Sign out and no browser-blue link. - The account menu reaches them. - The hop at 300ms shows the bar and the sky, with the card hidden. - The front page has no app bar. - C1 end to end:* Use a different address, then the second link, lands signed in on the directory. - Nothing overflows at 390 or 320. - On the base it fails (r2-home-extra-red-on-path.txt).
  • CLS (r2-home-cls-chrome.output.txt): 0 on every hop. Front page cold load is 0.0028 → 0 at 1440 and 0.0034 → 0 at 390.
  • Throttled cold load (120ms, 200KB/s; r2-home-flash-.output.txt): - On id./signin the first frame now has Onest and the mark. On main it painted the fallback face. - The front page's first frame now has the mark (main at 1440 had none), but is still the fallback face, because estilo's font-display: swap paints before the font arrives. - The preload delays id.*'s first paint at that throttle (about 1.45s → 1.8s).
  • Screenshots: polish-audit/shots/r2-home-chrome-after[-touch]-.png. The before set is r2-home-path-after-.png, this branch's base. Both cover 19 pages at 1440, 390 and 320, plus the account menu open, both 404s signed in and out, sign-in, sent, error and after sign-out. There is also the hop at 300ms (r2-home-chrome-after-hop-1440-at300ms.png). No page overflows.

For Paul

  • The directory can't say who you are (r3 item 8). Showing the address needs the assertion to carry it, which the one rule forbids at rest. As built, the menu says "Signed in".
  • Appearance is per host (spec Q6). A choice made on oficina.works doesn't follow you to id.oficina.works' account pages.
  • The team home heading is now "Your apps", with the lede "Every app here opens with the account you’re signed in with." That's my wording.
  • Copy the audit flagged but this branch leaves alone: "Confirm it is you", the recovery page's engineering copy, "0 unused recovery codes", "Press the button…", "Select Sign out to finish…".

Review. AGENTS.md requires a whole-branch review by the highest-capability model before merge. Not reviewed yet.

🤖 Generated with Claude Code

claude operated by paul@keymail.dev session 21a32889 · claude-code/2.1.280 · MacBookPro.localdomain

CI passed at b8894b7 (amadan ci status exit 0): vet (with -tags browser), fmt, build, go-test and race. Not merged; waiting for review, after #80.

claude operated by paul@keymail.dev session 21a32889 · claude-code/2.1.280 · MacBookPro.localdomain

Whole-branch review — feat/r2-shared-chrome @ b8894b7

Reviewer: Claude Fable 5.1 (claude-fable-5-1), the highest-capability model available to this session, as AGENTS.md requires. Read as one diff against origin/chore/rastrillo-path (b2f937b), on which it is stacked. Review only; nothing edited, committed, pushed or merged. Reviewed with the sign-in hub's blast radius in mind: every item in the security list below was probed on a production-shaped local stack, not only read.

VERDICT: APPROVE-WITH-NITS

Security checks (all hold)

  • return_to / destination (r3 C1). againHref (signin.go:200) builds the link from readReturn, which admits exactly one cookie and passes it through credentialReturn's closed set (/passkey/enrol, /passkey/codes, /authorize/resume); the link's target is validated again by rememberReturn on the next GET. Probed on the stack: return_to=https://evil.example, //evil.example and %2Fpasskey%2Fcodes%2F..%2F..%2Fx all clear the cookie; a forged __Host-oficina-return=https://evil.example renders a bare /signin. Not an open redirect by construction — it is a path from an allowlist, not a path from the request.
  • Appearance. /appearance is inside csrf.Protect (directory.go:132) and outside any session group, as spec §8.3 says. Handler is estilo's chrome.HandleAppearance, which redirects via rastrillo's sessions.SafeReturn(r, "/") — the spec's mandated validator, not a re-implementation. Probed: return_to=//evil.example/, /%09/evil.example, /%5c/evil.example and https://evil.example/ all 303 to /; a cross-origin or Origin-less POST is 403; an unknown mode is 400. The hidden return_to the menu emits is r.URL.EscapedPath() only — never the query (chrome.go:231).
  • Error pages. 404/405 render fixed copy; nothing from the request is in the body (a ?x=<script> 404 shows only the fixed title). The 500 logs ref and r.URL.Path only — no query, so no token or address can reach the log — and shows the reference. Cache-Control: no-store on both; the CSP and X-Frame-Options from rastrillo's securityHeaders are present on them. Unknown hosts still get the empty 421/404 from serveKnownHost. Non-browsers (no Accept: text/html) keep plain text, including a plain 405 for a non-GET.
  • *Account menu links to id..** Built from a.origins.Credential (config), not from the request or Host header; onCredential compares against the configured host after serveKnownHost has already refused anything else. Rendered exactly once each, and >Sign out< appears exactly once on the signed-in directory.
  • Sign-out chain unchanged. signout.go and exchange.go change only in render/failAssertion signatures; HOME_CONSUMERS, the 307 to id./signout and the same-origin "Finish signing out" page are as before. Probed: directory POST /signout → 307 to https://id.home.test/signout with the directory cookie cleared; id. GET /signout → the branded page. On the chain stack (home-run/chain, Home b8894b7 + Calendar origin/main), Chromium and WebKit both pass steps 1 (app Sign out → id. row revoked, next sign-in asks for the address), 2b (strict-origin page admitted) and 4 (foreign POST to id./signout refused with the branded page, session kept). The script's remaining failures are not Home's: Calendar origin/main's door-copy renamed its sign-in field and buttons and its /auth/finish now stops the script (steps 1b/3), and step 5's confirm click returns before busy.js's 650 ms hold submits. I reproduced step 5 in isolation on this build: the click POSTs after the hold, lands on home.test/signed-out, and the id.* cookie is gone. The chain e2e in polish-audit needs a selector/timing refresh, not this branch.
  • Hop and interstitial. With script: assertion.js still finds form[data-assertion-handoff] and submits; the card is hidden 1.5 s. Without script: <noscript> shows the card at once with the visible Continue button; the hop still has exactly one link, no skip link, no busy.js (the existing continuation tests pin this). The sky on the hop is the inline data: placeholder only — no grain/filaments fetch.
  • Credential origin renders nothing account-authored. id.* pages carry a brand-only bar; the 500's "Try again" href is the request's own URI, HTML-escaped by the template.

Gates

  • GOTOOLCHAIN=go1.26.5 make test: green except internal/provisioner under -race (Linux-only; CI covered it). make test-browser: green (with RASTRILLO_CHROME, see #80 nit 3).
  • Shared chrome check (r2-chrome-check.mjs + r2-home-check.config.mjs): 691 ok, 0 FAIL. r2-home-extra.mjs: 53 ok, 0 FAIL. Preloads verified real: /static/estilo/fonts/v1/onest-latin.woff2 is served (33 KB) and is the one file fonts.css names, so the preload is used, not wasted; the brand icon preload carries the same ?v= as the <img>.
  • Mutations, each red: againHref always returning /signin → TestStartingAgainKeepsTheDestination; unwiring r.NotFound(a.notFound) on the directory → TestErrorPagesSplitByAccept (both directory cases); wantsHTML forced false → TestErrorPagesSplitByAccept; hopSky keeping every layer → TestHopKeepsTheSkyPlaceholder.

Findings (none blocking)

  1. LOW — internal/home/chrome.go:291–293. On a 500 from a GET, data.Home is replaced by "Try again" pointing at the failing URL, so the person loses the "Go to Oficina" way out on the one page where the app has just failed. Keep the Home link and add "Try again" beside it if chrome.ErrorPage can carry two links; otherwise keep Home.
  2. LOW — internal/home/chrome.go:233. The Appearance Return is the current path for every directory page, including the 404 page and POST responses (e.g. the chooser re-rendering after POST /apps/docs/choose). Choosing an appearance from the chooser's POST response then 303s to a POST-only path and lands on the new 404. Suggest Return: "/" unless r.Method == GET, and / on error pages.
  3. LOW — internal/home/shared_chrome_test.go:1013 (TestAppearanceSetsTheTheme). It asserts the happy path only; the hostile return_to fallback is estilo's test to keep, but since this is the sign-in hub a two-line Home-side assertion (return_to=//evil.example → Location: /) would pin the spec's requirement where it matters most.
  4. NIT — internal/home/chrome.go:268. errorCopy has entries for 404 and 500 only; errorPage with any other status renders an empty title and body. Only those two are routed today; a comment or a fallback to 500's copy keeps the next caller honest.
  5. NIT — internal/home/templates/assertion.html:4–7. The comment still says "Spec shared-chrome §9.3" twice after the rewrite; harmless.
  6. INFO — polish-audit/home-run/chain/e2e.mjs (not this branch): stale against Calendar origin/main's door copy (#address → input[name=address], "Continue to Home"/"Connect accounts" → "Connect", summary:has-text("Account") → [estilo-account] summary) and against busy.js's hold on the id.* confirm page (wait for the POST /signout navigation, not networkidle). The directory's Sign out is now inside the closed <details> (step 6 needs to open the summary first). I ran a scratch copy with those edits; the Home-relevant steps pass in both engines.

On the questions for Paul

  • "Signed in" only. Right, and keep it. The directory holds a person ref and no address by the one rule; the only way to show who is to carry the address on the assertion into the directory session, which puts a readable address at rest on the directory side — the thing the two-origin split exists to prevent. The person glyph plus Passkeys/Recovery codes is enough of a "this is you" affordance. If it ever needs more, that is a spec change, not a chrome one.
  • Appearance per host. Accept. The cookie is host-only by estilo's design (spec Q6); a parent-domain cookie would be the first thing shared between oficina.works and id.oficina.works, and id.'s pages are seconds long. If it grates, the cheapest fix is for id. to honour prefers-color-scheme only, which is what it does now.
  • "Your apps". Fine. The lede "Every app here opens with the account you're signed in with" is a little knotty; "Every app here opens with your Oficina account." says the same in fewer words. Paul's call; not blocking.
  • Preload under heavy throttle. Keep both preloads. At 200 KB/s id.* first paint moves 1.45 s → 1.8 s, but that first frame now has Onest and the mark instead of a fallback face that swaps 300 ms later; at ordinary bandwidth the cost is invisible and CLS went to 0 on every hop. The font is a single 33 KB file that every page needs anyway. If it is ever revisited, drop the font preload on hop pages first, not on the door.
  • Out-of-scope copy ("Confirm it is you", the recovery page's engineering copy, "0 unused recovery codes", "Press the button…", "Select Sign out to finish…"). Agree it stays out of this branch; it is a copy pass on id.*'s door pages, small, and best done as its own branch after this merges so the diff is readable.

Merge order

Merge #80 first, then merge origin/main into this branch before its squash (the discussion already says so). AGENTS.md's new "shared chrome" paragraph is accurate against what I measured, including the chrome.js cost statement.