Narrow the settings-cog test assertions and record the icon-registry contract
CI / full-gate (push) Successful in 2m37s
Mirror / github-mirror (push) Successful in 6s

This commit was merged in pull request #48.
This commit is contained in:
2026-08-05 00:20:34 +02:00
parent 8f7ab55267
commit 46548ae758
5 changed files with 11 additions and 6 deletions
+8
View File
@@ -168,6 +168,14 @@ them. Revisit only if the stated reason stops holding.
workspace dir, so ci.sh's web-image build races another run's container creation on the workspace dir, so ci.sh's web-image build races another run's container creation on the
`<project>-web` tag. Accepted for a single-maintainer cadence; serialize with a workflow `<project>-web` tag. Accepted for a single-maintainer cadence; serialize with a workflow
`concurrency` group if it ever bites. `concurrency` group if it ever bites.
- **`ICON_NAMES` (`src/ui/icons.ts`) is a host-owned registry, not a frozen plugin contract.** It is
deliberately not re-exported from `#plugin-api`, and README → Nav & permission gates already tells an
author that using a new icon means registering it there. So the palette may narrow when the last
reference to an id goes — `i-gear` left with the settings menu 2026-08-05 — and a plugin needing one
gets it re-registered in the same change. Accepted cost: an unknown sprite id renders a blank icon
instead of failing loud; the `every icon <use> resolves to a defined <symbol>` e2e test catches it for
anything reaching the nav. Removing an id is a core edit, so weigh it per icon rather than sweeping the
registry — a few ids are registered ahead of a caller (see `todo.md`).
## Docker only — no host tooling ## Docker only — no host tooling
+1 -1
View File
@@ -101,7 +101,7 @@ test("the public landing at / is ungated and links to sign in + register", async
await expect(page.locator(".landing")).toBeVisible(); await expect(page.locator(".landing")).toBeVisible();
// the same app shell every page renders — the menu shows even signed out (permission-filtered). // the same app shell every page renders — the menu shows even signed out (permission-filtered).
await expect(page.locator(".sidebar")).toBeVisible(); await expect(page.locator(".sidebar")).toBeVisible();
await expect(page.locator('.side-footer use[href="#i-gear"]')).toHaveCount(0); // no settings cog to offer a signed-out visitor await expect(page.locator('use[href="#i-gear"]')).toHaveCount(0); // no settings cog to offer a signed-out visitor
await expect(page.getByRole("link", { name: "Log in" })).toHaveAttribute("href", "/login"); await expect(page.getByRole("link", { name: "Log in" })).toHaveAttribute("href", "/login");
await expect(page.getByRole("link", { name: "Create account" })).toHaveAttribute("href", "/registration"); await expect(page.getByRole("link", { name: "Create account" })).toHaveAttribute("href", "/registration");
await shot(page, "live-05-public-landing"); await shot(page, "live-05-public-landing");
-1
View File
@@ -282,7 +282,6 @@ span.nav-self { cursor: default; } /* static / non-clickable */
outline: 2px solid var(--focus); outline-offset: 1px; outline: 2px solid var(--focus); outline-offset: 1px;
} }
/* profile row */
.footer-actions { display: flex; align-items: center; gap: 4px; } .footer-actions { display: flex; align-items: center; gap: 4px; }
.profile { .profile {
display: flex; align-items: center; gap: 9px; flex: 1 1 auto; display: flex; align-items: center; gap: 9px; flex: 1 1 auto;
+1 -4
View File
@@ -40,9 +40,6 @@ test("app shell renders sidebar, topbar and the content slot", async () => {
assert.match(html, /<form class="menu-item-form" method="post" action="\/logout">/); assert.match(html, /<form class="menu-item-form" method="post" action="\/logout">/);
assert.match(html, /<input type="hidden" name="_csrf" value="tok\.sig" \/>/); assert.match(html, /<input type="hidden" name="_csrf" value="tok\.sig" \/>/);
// The footer carries the profile and the language picker only — no settings/preferences menu.
assert.doesNotMatch(html, /i-gear|Preferences/);
// Branding, document title, and the inlined icon sprite (so <use> resolves). // Branding, document title, and the inlined icon sprite (so <use> resolves).
assert.match(html, /Acme Console/); assert.match(html, /Acme Console/);
assert.match(html, /<title>People<\/title>/); assert.match(html, /<title>People<\/title>/);
@@ -54,7 +51,7 @@ test("app shell offers Sign in (not Sign out) to an anonymous visitor — so a p
const html = await render({ title: "Overview", brand: { name: "Acme" }, nav: "", body: "x" }); // no user, no signInHref → default const html = await render({ title: "Overview", brand: { name: "Acme" }, nav: "", body: "x" }); // no user, no signInHref → default
assert.match(html, /href="\/login"[^>]*>[\s\S]*?Sign in/); // a path to sign in (default target) assert.match(html, /href="\/login"[^>]*>[\s\S]*?Sign in/); // a path to sign in (default target)
assert.doesNotMatch(html, /action="\/logout"/); // a guest has no session to end assert.doesNotMatch(html, /action="\/logout"/); // a guest has no session to end
assert.doesNotMatch(html, /i-gear|Preferences/); // nor any settings menu to open assert.doesNotMatch(html, /<use href="#i-gear"/); // nor a settings cog to open
// When chrome supplies signInHref (the current page as return_to), the link carries it. // When chrome supplies signInHref (the current page as return_to), the link carries it.
const withReturn = await render({ title: "Overview", brand: { name: "Acme" }, nav: "", body: "x", signInHref: "/login?return_to=%2Fscheduling" }); const withReturn = await render({ title: "Overview", brand: { name: "Acme" }, nav: "", body: "x", signInHref: "/login?return_to=%2Fscheduling" });
+1
View File
@@ -9,6 +9,7 @@
- [ ] Guard against the double-clicked submit, without client-side JavaScript. The README's non-technical persona double-clicks a button that doesn't respond instantly, so a second identical POST is an expected event, not misuse — today it creates two users, mints two recovery codes, or registers two OAuth2 clients. Constraints: HTML/CSS only (no client JS — priority: zero-JS spine), and it must not break an action that is *legitimately* repeatable (an increase-by-one button is not a duplicate, it is two increments). Sketch to evaluate: a CSS-only affordance so the second click has nothing to hit (`:active`/`:focus` state, or the submit visually and semantically settling), paired with the host recognising a duplicate on the server — same session, same route, same payload, within a short window — and then logging it and dropping the second rather than replaying it. Open questions: what identifies "the same submission" (a one-time token minted into each rendered form is stronger than hashing the payload, and the CSRF plumbing already mints per-request tokens), how long the window is, where the record lives given the app is stateless (in-memory like the revoke denylist, or push it to the upstream the plugin already writes to), and how a plugin declares a route as repeatable — an opt-out on the route, or opt-in per form. Raised 2026-08-04 with the personas. - [ ] Guard against the double-clicked submit, without client-side JavaScript. The README's non-technical persona double-clicks a button that doesn't respond instantly, so a second identical POST is an expected event, not misuse — today it creates two users, mints two recovery codes, or registers two OAuth2 clients. Constraints: HTML/CSS only (no client JS — priority: zero-JS spine), and it must not break an action that is *legitimately* repeatable (an increase-by-one button is not a duplicate, it is two increments). Sketch to evaluate: a CSS-only affordance so the second click has nothing to hit (`:active`/`:focus` state, or the submit visually and semantically settling), paired with the host recognising a duplicate on the server — same session, same route, same payload, within a short window — and then logging it and dropping the second rather than replaying it. Open questions: what identifies "the same submission" (a one-time token minted into each rendered form is stronger than hashing the payload, and the CSRF plumbing already mints per-request tokens), how long the window is, where the record lives given the app is stateless (in-memory like the revoke denylist, or push it to the upstream the plugin already writes to), and how a plugin declares a route as repeatable — an opt-out on the route, or opt-in per form. Raised 2026-08-04 with the personas.
- [ ] Decide the caching contract for rendered pages. Responses now carry `Vary: Accept-Language` (they content-negotiate), but nothing sets `Cache-Control` — so a shared cache in front of the app has no instruction, and a signed-in page is not marked `private`. Pre-existing, surfaced by the i18n review 2026-08-03: either set the headers deliberately (public pages cacheable, gated pages `private, no-store`) or record in AGENTS.md that the reverse proxy owns this. - [ ] Decide the caching contract for rendered pages. Responses now carry `Vary: Accept-Language` (they content-negotiate), but nothing sets `Cache-Control` — so a shared cache in front of the app has no instruction, and a signed-in page is not marked `private`. Pre-existing, surfaced by the i18n review 2026-08-03: either set the headers deliberately (public pages cacheable, gated pages `private, no-store`) or record in AGENTS.md that the reverse proxy owns this.
- [ ] Decide whether the single generic Keto `Resource` namespace should become per-domain namespaces (`Shift`, `Document`, …), as Ory's own examples model it. One global `Resource` bucket is the project's own "no catch-all names" rule (`utils`, `helpers`, `misc`) applied to namespaces. Raised 2026-08-03; a design question, not a naming one. - [ ] Decide whether the single generic Keto `Resource` namespace should become per-domain namespaces (`Shift`, `Document`, …), as Ory's own examples model it. One global `Resource` bucket is the project's own "no catch-all names" rule (`utils`, `helpers`, `misc`) applied to namespaces. Raised 2026-08-03; a design question, not a naming one.
- [ ] Decide what `ICON_NAMES` (`src/ui/icons.ts`) actually is. Its comment says "the icons the UI actually references", but `i-chart`, `i-copy`, `i-download` and `i-sliders` have no caller anywhere — so either they go the way `i-gear` just did, or the comment should say the palette is curated and may carry an id ahead of its first use. Not cosmetic: the sprite is inlined into every page, and the rule decides whether a future removal is routine cleanup or a plugin-facing regression (see AGENTS.md → the `ICON_NAMES` deviation). Pre-existing, surfaced by the review 2026-08-05.
- [ ] Decide (once) whether the CSRF token staying unbound to `sub`/session is accepted. `src/auth/csrf.ts` signs `<nonce>.<HMAC(secret, nonce)>` with no session binding, so any validly-signed token passes for any user — an attacker who can write cookies on the origin (a sibling subdomain, or a plaintext hop with `SECURE_COOKIES=false`) can fix a token they know. Standard for unbound signed double-submit and plausibly fine behind `SameSite=Lax` + HSTS. Accepted ⇒ record it in AGENTS.md → "Deliberate architectural deviations" and in README → Security model under "Not guaranteed"; not accepted ⇒ bind the nonce to `sub` (small change). Raised by review 2026-08-02; left undecided because it is a maintainer call, and an undocumented exception reads as a bug to the next reviewer. - [ ] Decide (once) whether the CSRF token staying unbound to `sub`/session is accepted. `src/auth/csrf.ts` signs `<nonce>.<HMAC(secret, nonce)>` with no session binding, so any validly-signed token passes for any user — an attacker who can write cookies on the origin (a sibling subdomain, or a plaintext hop with `SECURE_COOKIES=false`) can fix a token they know. Standard for unbound signed double-submit and plausibly fine behind `SameSite=Lax` + HSTS. Accepted ⇒ record it in AGENTS.md → "Deliberate architectural deviations" and in README → Security model under "Not guaranteed"; not accepted ⇒ bind the nonce to `sub` (small change). Raised by review 2026-08-02; left undecided because it is a maintainer call, and an undocumented exception reads as a bug to the next reviewer.
### Architectural review findings (2026-07-02) ### Architectural review findings (2026-07-02)