crueber/walhub

Watch

Fork 0

Anonymous visitors get a 'log in to continue' interstitial for every write action (OIDC anonymous-read mode); one anonymous write leak in ssh-keys #502

New issue

Closed

opened 2026-09-13 23:23:14 +00:00 by crueber · 3 comments

crueber commented

2026-09-13 23:23:14 +00:00

Owner

Current state (verified against the tree; static reading, no repro)

Server-side: per-route gates exist and mostly refuse anonymous — with two defects

The identity contract (internal/identity/access.go Resolve, 295-347) gives an anonymous principal exactly one grant: read on public visibility (line 343). CheckRead/CheckRole/CheckOrgOwner (internal/identity/gate.go:40/64/79) return 401 for anonymous below the required level. Per-surface write gates verified:

Write surface	Route / seam	Gate	Anonymous today
New repo (PUT)	internal/api/summary.go:265 `repoPut` -> `gate(AuthWrite)`	api/env.go:844	403 "write access required" (see defect B)
Delete repo	summary.go `repoDelete` AuthAdmin	same	403
Settings PUT/DELETE, policy PUT/DELETE	routes.go:79-87, AuthAdmin	same	403
Ops `POST ops/{op}`	routes.go:96, api/ops.go:62	same	403
Owner profile PUT	routes.go:61-63, api/profile.go:233 `canEditProfile` -> gate(AuthWrite)	same	403
Issues create/comment/patch/react/labels/milestones/attachments	internal/issues/service.go:140,199,298,610,679,1207,1250,1300,1388,1424,1475; attachments.go:183 — `requireAuthenticated`	401	refused
PR open/comment/merge/update-branch/head-delete; fork	internal/pulls/service.go:319-326, 853, 990; merge.go:681-690 — `requireAuthenticated` + `requireRole("write")`	401	refused
Star / Unstar	internal/social/social.go:40,127 — `requireAuthenticated`	401	refused
Watch PUT/DELETE	internal/notify/http.go:434-438 — `requireAuth`	401	refused
Webhooks CRUD	internal/notify/http.go:357-360 — `requireAuth` + `requireRole(admin)`	401	refused
New org	internal/identity/http.go:502 — anonymous 401	401	refused
Org/repo invitations, org avatar PUT/DELETE, teams	http_invites.go, http.go:663 — `CheckOrgOwner`/`CheckRole`	401	refused
User avatar POST/DELETE, profile PUT	identity/http.go:448-451, 302	401	refused
Mirror create-from-URL	internal/mirror/http.go:462-470 — `p.Write` else anonymous 401	401	refused
Import	internal/repoimport/service.go:504-506 — `checkCreate` anonymous 401	401	refused
Releases/tags create/delete	internal/releases/service.go:60,446,499,702; tags/service.go:128 — local `requireRole` copies	verify mapping yields 401 (identity.CheckRole does; the local copies must be confirmed in the test grid)	likely refused
SSH keys add/delete	internal/api/routes.go:43-44 registered `Auth: AuthRead`; internal/api/sshkeys.go:71-95 `gate(AuthRead)` then `SSHKeys.Add(ctx, p.Name, ...)` with NO anonymous check	LEAK	see defect A

Defect A (the leak): with anonymous_read = true, gate(AuthRead) admits the anonymous principal (env.go:852-856), so POST /api/v1/ssh-keys executes SSHKeyRegistry.Add for principal name "anonymous" (auth.Anonymous(), internal/server/auth.go:411 — a valid single path segment) and writes a key record as a synthetic principal; a second anonymous caller could then delete it. This is a real anonymous write in oidc+anon-read mode. Fix: AuthWrite level (or an explicit anonymous check) on both ssh-key mutations.

Defect B (inconsistent refusal status): gate() (env.go:857-874) checks p.Anonymous && !anonRead FIRST; with anon-read on, an anonymous caller on an AuthWrite/AuthAdmin route falls through to !p.Write/!p.Admin and gets 403 "write access required" instead of 401 "authentication required". Every feature package answers 401; the core api gate answers 403. No write executes either way, but the UI cannot distinguish "sign in" from "you lack permission" and the acceptance matrix cannot be uniform. Fix: in gate(), check p.Anonymous before the write/admin flag checks and answer 401 (+ the existing WWW-Authenticate: Bearer from writePlain) whenever anonymous — anonymous can never be "insufficient role", it is unauthenticated.

No middleware-level catch-all exists (internal/server/middleware.go — the chain is requestID/canonicalHost/serverHeaders/recoverPanic/cors/refreshSession; apiServe injects the principal, router.go:33-42). Enforcement is a per-package convention (requireAuthenticated copy-pasted ~10x). Defense in depth: add ONE middleware-level assert in the api lane seam (apiServe in internal/server/router.go, where the principal is already resolved and injected) — after Authenticate, if cfg.Server.Auth.Mode == "oidc" and the request is a non-GET/HEAD/OPTIONS (i.e. any state-changing method) AND p.Anonymous, answer 401 immediately without invoking the handler. This makes the catch-all a property of the identity chain, not of each package remembering to gate. The per-route requireAuthenticated calls stay (they also protect none-mode-free semantics and keep error messages precise); the middleware is the guarantee. Cite the Resolve contract comment (access.go:295-309): anonymous resolves to read-at-most — the middleware turns that into "and never writes".

UI affordances anonymous users see today

Navbar: correct — create (+) gated on nav().showCreate = signedIn (web/src/lib/identity.js:117-118, #466); Login button rendered with /_auth/login?next=<current> (identity.js:104-106, #371). No change needed.
Repo header pills (web/src/pages/Repo.jsx:416-421, 700-740): StarToggle/WatchToggle render for anonymous — their GETs succeed (social Counts/ViewerState serve anonymous read; Repo.jsx:407 hides only on fetch failure). Clicking star/watch fires the SDK mutation: core.js:_call sees the 401 and triggers popup auth (openAuthPopup -> /api-browser/v1/authenticate, web/sdk/src/core.js:478-489, auth.js:45) instead of any interstitial — the visitor gets a jarring popup that then re-runs the write. Same for the Fork pill label link to /{full}/fork (Fork.jsx defaults owner to "" and renders an unusable form for anonymous).
Issues/Pulls toolbars: New issue (Issues.jsx:252) and New pull request (Pulls.jsx:93) render unconditionally — anonymous sees them.
Issue view composer: canComment = role() !== null (Issue.jsx:52) — but useRole resolves read for anonymous on a public repo (perms.jsx:27-29 feeding identity/http_perms.go:57-68, Resolve grants anonymous read on public), so the comment composer and reaction chips render for anonymous; submit 401s -> popup-auth path again.
New repo / New org pages already show the amber "not signed in as a writer — the server will refuse the create (401/403)" note (New.jsx:184-188, OrgNew.jsx:125-128) but still render full forms.

Prescribed pattern: a shared client helper writeGate (headless, web/src/lib/) that pages consult before rendering or invoking any write affordance: anonymous (me().anonymous === true, or me() null under oidc mode) => render the affordance in a soft-disabled state (same pill/button shape, reduced emphasis, title "Sign in to ...") OR hidden — planner's call per surface, but one pattern everywhere — and on click/submit route to /login-required?next=<action>&action=<label> instead of firing the SDK call. The SDK's 401->popup retry (core.js:478) must NOT fire for these flows: either the pre-flight helper prevents the call entirely (preferred), or a flag opts the write call out of popup auth so the 401 surfaces and routes to the interstitial. Server 401s that reach a page without pre-flight (stale identity, direct API use) hook the same way: a thin wrapper around reportError/the mutation catch paths that checks err.unauthorized + anonymous identity and navigates to the interstitial with the current URL as next.

The interstitial page (new)

Route /login-required?next=<path>&action=<label> — a new SPA page (static route registered before /:owner, like /invitations), full-page card in the existing design language, light+dark, mobile-clean at 390px (the #273 rules).
Copy: names the action (action= param, default "this action"), states an account is required, one primary Log in button and one back/cancel link. No emoji art needed; a mirror icon is fine.
The Log in button invokes the existing OIDC pathway: GET /_auth/login?next=<sanitized next> (internal/server/auth_oidc.go:77 — HMAC state carries next, 600 s window, sanitizeNext confines it to a single leading slash; auth.go #344 disabled-state rendering exists). Login completes and the callback 302s to next — the attempted action URL — so the user lands exactly where they were (repo page, issue thread, fork form with its query string intact). next must be the attempted ACTION URL, not the interstitial's referrer chain.
Back/cancel: history.back() fallback to next's page or /explore.

Proposed design (summary)

Backend: fix the ssh-keys gate (AuthWrite/anonymous check); make gate() answer 401 for anonymous on AuthWrite/AuthAdmin before the flag check; add the oidc anonymous write-assert middleware in apiServe; confirm the releases/tags local requireRole copies map anonymous to 401 (align if not).
Client: headless web/src/lib/writeGate.js (pure, node --test-able like identity.js) + interstitial page + per-affordance wiring (star/watch pills, fork pill label, New issue/pull buttons, composer/reactions, New/OrgNew/Import/Fork forms) + the 401-mutation catch hook opting out of popup auth.
Docs: docs/go/06_server_http.md §8.6 + docs/go/12_web_ui.md (SPA routes + the §1.2 401-popup section gains the write-401 carve-out) + docs/features/01_identity_permissions.md §4.1 gains the "anonymous write refusal + interstitial flow" paragraph.

Acceptance criteria

Every inventoried write route refuses an anonymous principal with a consistent 401 (resolution-matrix test grid: anonymous x {repo PUT/DELETE, settings, policy, ops, profile PUT, ssh-keys, org create, invitations, issues create/comment/patch, labels, milestones, reactions, attachments, pulls open/comment/merge, fork, star, unstar, watch, webhooks, releases, tags, mirror, import}, per the #374 test conventions — see identity/visibility374_test.go for the matrix shape)
The middleware-level assert rejects every state-changing request from an anonymous principal in oidc mode regardless of per-route gates (test: route a synthetic write past a hypothetically-ungated handler and observe the 401)
No anonymous write executes: the ssh-keys leak is closed and covered by a pinned test
Star/Watch/Fork/New-issue/New-pull/composer/reactions affordances follow the one writeGate pattern for anonymous users (soft-disabled or hidden, consistent) and clicking routes to the interstitial
The interstitial /login-required?next=&action= explains the requirement, offers Log in via /_auth/login?next= and returns the user to the attempted action after login (state-carry verified through the OIDC callback)
Back/cancel works; sanitizeNext rules hold (no open redirect via next)
The SDK 401->popup path does not intercept these flows (unit test on the opt-out/pre-flight)
Logged-in users are unaffected everywhere (matrix rows for authenticated reader/writer/admin unchanged — same grid, principal dimension)
Interstitial renders clean at 390px and in light + dark
Docs amended: 06_server_http.md §8.6, 12_web_ui.md (route table + §1.2 carve-out), features/01 §4.1

References: #345 (public browsing), #371 (navbar login/identity, /_auth/login?next=), #374 (visibility modes + matrix test conventions), #370 (usernames), #346 (create owner gate), #347 (push guardrails).

With #345 public browsing (anonymous read) enabled in OIDC mode, a signed-out visitor can browse everything but must be able to execute ZERO writes — fork, star, watch, new repo/org/issue/comment/PR, reactions, and "any other way to write the server". Every attempted write must refuse server-side, and the UI must route the visitor to a new "log in to continue" interstitial that explains the action needs an account and offers OIDC login returning them to the attempted action. ## Current state (verified against the tree; static reading, no repro) ### Server-side: per-route gates exist and mostly refuse anonymous — with two defects The identity contract (internal/identity/access.go `Resolve`, 295-347) gives an anonymous principal exactly one grant: `read` on public visibility (line 343). `CheckRead`/`CheckRole`/`CheckOrgOwner` (internal/identity/gate.go:40/64/79) return 401 for anonymous below the required level. Per-surface write gates verified: | Write surface | Route / seam | Gate | Anonymous today | |---|---|---|---| | New repo (PUT) | internal/api/summary.go:265 `repoPut` -> `gate(AuthWrite)` | api/env.go:844 | **403** "write access required" (see defect B) | | Delete repo | summary.go `repoDelete` AuthAdmin | same | **403** | | Settings PUT/DELETE, policy PUT/DELETE | routes.go:79-87, AuthAdmin | same | **403** | | Ops `POST ops/{op}` | routes.go:96, api/ops.go:62 | same | **403** | | Owner profile PUT | routes.go:61-63, api/profile.go:233 `canEditProfile` -> gate(AuthWrite) | same | **403** | | Issues create/comment/patch/react/labels/milestones/attachments | internal/issues/service.go:140,199,298,610,679,1207,1250,1300,1388,1424,1475; attachments.go:183 — `requireAuthenticated` | 401 | refused | | PR open/comment/merge/update-branch/head-delete; fork | internal/pulls/service.go:319-326, 853, 990; merge.go:681-690 — `requireAuthenticated` + `requireRole("write")` | 401 | refused | | Star / Unstar | internal/social/social.go:40,127 — `requireAuthenticated` | 401 | refused | | Watch PUT/DELETE | internal/notify/http.go:434-438 — `requireAuth` | 401 | refused | | Webhooks CRUD | internal/notify/http.go:357-360 — `requireAuth` + `requireRole(admin)` | 401 | refused | | New org | internal/identity/http.go:502 — anonymous 401 | 401 | refused | | Org/repo invitations, org avatar PUT/DELETE, teams | http_invites.go, http.go:663 — `CheckOrgOwner`/`CheckRole` | 401 | refused | | User avatar POST/DELETE, profile PUT | identity/http.go:448-451, 302 | 401 | refused | | Mirror create-from-URL | internal/mirror/http.go:462-470 — `p.Write` else anonymous 401 | 401 | refused | | Import | internal/repoimport/service.go:504-506 — `checkCreate` anonymous 401 | 401 | refused | | Releases/tags create/delete | internal/releases/service.go:60,446,499,702; tags/service.go:128 — local `requireRole` copies | verify mapping yields 401 (identity.CheckRole does; the local copies must be confirmed in the test grid) | likely refused | | **SSH keys add/delete** | **internal/api/routes.go:43-44 registered `Auth: AuthRead`; internal/api/sshkeys.go:71-95 `gate(AuthRead)` then `SSHKeys.Add(ctx, p.Name, ...)` with NO anonymous check** | **LEAK** | see defect A | **Defect A (the leak):** with `anonymous_read = true`, `gate(AuthRead)` admits the anonymous principal (env.go:852-856), so `POST /api/v1/ssh-keys` executes `SSHKeyRegistry.Add` for principal name `"anonymous"` (auth.Anonymous(), internal/server/auth.go:411 — a valid single path segment) and **writes a key record as a synthetic principal**; a second anonymous caller could then delete it. This is a real anonymous write in oidc+anon-read mode. Fix: `AuthWrite` level (or an explicit anonymous check) on both ssh-key mutations. **Defect B (inconsistent refusal status):** `gate()` (env.go:857-874) checks `p.Anonymous && !anonRead` FIRST; with anon-read on, an anonymous caller on an AuthWrite/AuthAdmin route falls through to `!p.Write`/`!p.Admin` and gets **403 "write access required"** instead of 401 "authentication required". Every feature package answers 401; the core api gate answers 403. No write executes either way, but the UI cannot distinguish "sign in" from "you lack permission" and the acceptance matrix cannot be uniform. Fix: in `gate()`, check `p.Anonymous` before the write/admin flag checks and answer 401 (+ the existing `WWW-Authenticate: Bearer` from writePlain) whenever anonymous — anonymous can never be "insufficient role", it is unauthenticated. **No middleware-level catch-all exists** (internal/server/middleware.go — the chain is requestID/canonicalHost/serverHeaders/recoverPanic/cors/refreshSession; `apiServe` injects the principal, router.go:33-42). Enforcement is a per-package convention (`requireAuthenticated` copy-pasted ~10x). Defense in depth: add ONE middleware-level assert in the api lane seam (`apiServe` in internal/server/router.go, where the principal is already resolved and injected) — after `Authenticate`, if `cfg.Server.Auth.Mode == "oidc"` and the request is a non-GET/HEAD/OPTIONS (i.e. any state-changing method) AND `p.Anonymous`, answer 401 immediately without invoking the handler. This makes the catch-all a property of the identity chain, not of each package remembering to gate. The per-route `requireAuthenticated` calls stay (they also protect `none`-mode-free semantics and keep error messages precise); the middleware is the guarantee. Cite the Resolve contract comment (access.go:295-309): anonymous resolves to read-at-most — the middleware turns that into "and never writes". ### UI affordances anonymous users see today - Navbar: correct — create (+) gated on `nav().showCreate = signedIn` (web/src/lib/identity.js:117-118, #466); Login button rendered with `/_auth/login?next=<current>` (identity.js:104-106, #371). No change needed. - Repo header pills (web/src/pages/Repo.jsx:416-421, 700-740): `StarToggle`/`WatchToggle` render for anonymous — their GETs succeed (social Counts/ViewerState serve anonymous read; Repo.jsx:407 hides only on fetch failure). Clicking star/watch fires the SDK mutation: `core.js:_call` sees the 401 and triggers **popup auth** (`openAuthPopup` -> `/api-browser/v1/authenticate`, web/sdk/src/core.js:478-489, auth.js:45) instead of any interstitial — the visitor gets a jarring popup that then re-runs the write. Same for the Fork pill label link to `/{full}/fork` (Fork.jsx defaults owner to "" and renders an unusable form for anonymous). - Issues/Pulls toolbars: `New issue` (Issues.jsx:252) and `New pull request` (Pulls.jsx:93) render unconditionally — anonymous sees them. - Issue view composer: `canComment = role() !== null` (Issue.jsx:52) — but `useRole` resolves `read` for anonymous on a public repo (perms.jsx:27-29 feeding identity/http_perms.go:57-68, Resolve grants anonymous read on public), so **the comment composer and reaction chips render for anonymous**; submit 401s -> popup-auth path again. - New repo / New org pages already show the amber "not signed in as a writer — the server will refuse the create (401/403)" note (New.jsx:184-188, OrgNew.jsx:125-128) but still render full forms. Prescribed pattern: a shared client helper `writeGate` (headless, `web/src/lib/`) that pages consult before rendering or invoking any write affordance: anonymous (me().anonymous === true, or me() null under oidc mode) => render the affordance in a soft-disabled state (same pill/button shape, reduced emphasis, title "Sign in to ...") OR hidden — planner's call per surface, but one pattern everywhere — and on click/submit route to `/login-required?next=<action>&action=<label>` instead of firing the SDK call. The SDK's 401->popup retry (core.js:478) must NOT fire for these flows: either the pre-flight helper prevents the call entirely (preferred), or a flag opts the write call out of popup auth so the 401 surfaces and routes to the interstitial. Server 401s that reach a page without pre-flight (stale identity, direct API use) hook the same way: a thin wrapper around `reportError`/the mutation catch paths that checks `err.unauthorized` + anonymous identity and navigates to the interstitial with the current URL as `next`. ### The interstitial page (new) - Route `/login-required?next=<path>&action=<label>` — a new SPA page (static route registered before `/:owner`, like `/invitations`), full-page card in the existing design language, light+dark, mobile-clean at 390px (the #273 rules). - Copy: names the action (`action=` param, default "this action"), states an account is required, one primary `Log in` button and one back/cancel link. No emoji art needed; a mirror icon is fine. - The Log in button invokes the existing OIDC pathway: `GET /_auth/login?next=<sanitized next>` (internal/server/auth_oidc.go:77 — HMAC state carries `next`, 600 s window, `sanitizeNext` confines it to a single leading slash; auth.go #344 disabled-state rendering exists). Login completes and the callback 302s to `next` — the attempted action URL — so the user lands exactly where they were (repo page, issue thread, fork form with its query string intact). `next` must be the attempted ACTION URL, not the interstitial's referrer chain. - Back/cancel: `history.back()` fallback to `next`'s page or `/explore`. ## Proposed design (summary) 1. Backend: fix the ssh-keys gate (AuthWrite/anonymous check); make `gate()` answer 401 for anonymous on AuthWrite/AuthAdmin before the flag check; add the oidc anonymous write-assert middleware in `apiServe`; confirm the releases/tags local `requireRole` copies map anonymous to 401 (align if not). 2. Client: headless `web/src/lib/writeGate.js` (pure, node --test-able like identity.js) + interstitial page + per-affordance wiring (star/watch pills, fork pill label, New issue/pull buttons, composer/reactions, New/OrgNew/Import/Fork forms) + the 401-mutation catch hook opting out of popup auth. 3. Docs: docs/go/06_server_http.md §8.6 + docs/go/12_web_ui.md (SPA routes + the §1.2 401-popup section gains the write-401 carve-out) + docs/features/01_identity_permissions.md §4.1 gains the "anonymous write refusal + interstitial flow" paragraph. ## Acceptance criteria - [ ] Every inventoried write route refuses an anonymous principal with a consistent 401 (resolution-matrix test grid: anonymous x {repo PUT/DELETE, settings, policy, ops, profile PUT, ssh-keys, org create, invitations, issues create/comment/patch, labels, milestones, reactions, attachments, pulls open/comment/merge, fork, star, unstar, watch, webhooks, releases, tags, mirror, import}, per the #374 test conventions — see identity/visibility374_test.go for the matrix shape) - [ ] The middleware-level assert rejects every state-changing request from an anonymous principal in oidc mode regardless of per-route gates (test: route a synthetic write past a hypothetically-ungated handler and observe the 401) - [ ] No anonymous write executes: the ssh-keys leak is closed and covered by a pinned test - [ ] Star/Watch/Fork/New-issue/New-pull/composer/reactions affordances follow the one writeGate pattern for anonymous users (soft-disabled or hidden, consistent) and clicking routes to the interstitial - [ ] The interstitial `/login-required?next=&action=` explains the requirement, offers Log in via `/_auth/login?next=` and returns the user to the attempted action after login (state-carry verified through the OIDC callback) - [ ] Back/cancel works; sanitizeNext rules hold (no open redirect via `next`) - [ ] The SDK 401->popup path does not intercept these flows (unit test on the opt-out/pre-flight) - [ ] Logged-in users are unaffected everywhere (matrix rows for authenticated reader/writer/admin unchanged — same grid, principal dimension) - [ ] Interstitial renders clean at 390px and in light + dark - [ ] Docs amended: 06_server_http.md §8.6, 12_web_ui.md (route table + §1.2 carve-out), features/01 §4.1 References: #345 (public browsing), #371 (navbar login/identity, `/_auth/login?next=`), #374 (visibility modes + matrix test conventions), #370 (usernames), #346 (create owner gate), #347 (push guardrails).

crueber added this to the v1 milestone

2026-09-13 23:23:14 +00:00

crueber added the

issues

oidc

labels

2026-09-13 23:23:19 +00:00

crueber referenced this issue from a commit

2026-09-14 00:17:54 +00:00

Fix #502: anonymous write interstitial (401 grid + middleware assert + writeGate UI)

crueber referenced this issue from a pull request that will close it,

2026-09-14 00:17:56 +00:00

Fix #502: anonymous write interstitial #507

crueber commented

2026-09-14 00:18:01 +00:00

Author

Owner

Fixed by PR #507 (#507): anonymous write interstitial + uniform-401 grid + oidc write-assert middleware + ssh-keys leak closure. All 10 acceptance criteria covered (tests + docs); browser proof open per workspace rules.

Fixed by PR #507 (https://git.packden.us/crueber/walhub/pulls/507): anonymous write interstitial + uniform-401 grid + oidc write-assert middleware + ssh-keys leak closure. All 10 acceptance criteria covered (tests + docs); browser proof open per workspace rules.

crueber commented

2026-09-14 00:38:01 +00:00

Author

Owner

Review: PR #507 (fix/issue-502) — adversarial security gate

Verified in scratch worktree /tmp/pr507 @ 1908744 (removed afterward).
Main worktree untouched (still clean on main, read-only throughout except
git fetch). No browser drive (per task instructions — tests + reasoning;
stated explicitly below). No docker, no live-instance contact (note: something
is listening on 127.0.0.1:8080 with /setup → 403 — I probed it read-only
via curl for the smoke-test analysis and touched nothing).

1. ssh-keys leak (defect A) — CLOSED, pinned, self-service preserved

internal/api/sshkeys.go:80-87,124-131 — both mutations keep AuthRead but
refuse p.Anonymous with 401 before touching the registry. Read-only
authenticated principals still get 201 (TestAuthedWriteRowsUnchanged502),
auth-none still gets 201 (principal is auth.None(), never anonymous —
pinned in TestSSHKeysAnonLeakClosed502), and the matrix asserts the
registry stays empty after anonymous POST/DELETE. No remaining AuthRead-write
hole in internal/api: the only other POST-under-AuthRead routes are
policy/validate, policy/dry-run, settings/validate (pure computation,
no writes — and now also 401'd for anonymous by the middleware assert anyway).

2. gate() 401-for-anonymous (defect B) — correct, law-9 clean

internal/api/env.go:862,871 — if p.Anonymous now precedes the flag checks
on both AuthWrite/AuthAdmin. All three new refusal paths emit
WWW-Authenticate: Bearer realm="walgit" (gate via writePlain, ssh-keys
via writePlain, apiServe explicitly) — asserted per-route in the matrix
test. Law 9 improves: the old 403 fall-through never triggered git-credential
erase; 401 does. No legit flow depended on anonymous-403 (anonymous could do
nothing with it); service accounts carry tokens (non-anonymous, pinned
passthrough in TestAPIServeOIDCAuthedWritePasses502). Stale-pin flips
403→401 in gaps3_test.go:118, placeholder_edge_test.go:47,
profile_test.go:185 are exactly this behavior change — legitimate.

3. Middleware assert — lawful placement, no breakage found

internal/server/router.go:50-54 — fires only when ALL hold: anonymous +
Mode == "oidc" + non-GET/HEAD/OPTIONS, after Authenticate (so invalid
credentials still 401 first) and after identityForward. Checked each risk:
OIDC callback/login/logout are GET-only (authFlow, router.go:204-216) —
unaffected. health/setup are separate lanes, not apiServe — unaffected.
none-mode never anonymous — unaffected (comment says so, code confirms:
auth.None() is not anonymous). Preflight OPTIONS excluded; isWriteMethod
fail-closes every other verb. Token mode untouched (pinned passthrough).
Inter-instance/service calls use Bearer tokens → non-anonymous → pass.
"Webhooks with secrets": no such inbound path exists — internal/notify
delivery routes are outbound test-pings (admin-gated, in-seam); CRUD was
already 401/403-gated. All feature routes mount through the RouteProvider
chain (bind_api.go) → s.api.Serve ← apiServe, so the assert genuinely
covers issues/pulls/social/notify/mirror/import/identity/releases/tags —
verified the wiring, not assumed. Two pre-existing out-of-seam notes (NOT
blockers, refused either way, unchanged by this PR): POST /_events/notify
anonymous → 403 via requireWrite (auth.go:336), and setup PUT anonymous →
403 via requireAdmin — both pre-date this issue's inventory.

4. Releases/tags 401 — verified in code, not assumed

internal/releases/releases.go:362-372 and internal/tags/service.go:380-390:
requireRole returns ErrUnauthorized (→401) for anonymous,
ErrForbidden for authenticated-but-insufficient. Untouched, as claimed.

5. Matrix grid — scoped honestly, covered in combination

internal/api/anonwrite502_test.go grids anonymous × the 10 seam-package
writes (repo PUT/DELETE, settings PUT/DELETE, policy PUT/DELETE, ops POST,
profile PUT, ssh-keys POST/DELETE) — all 401 + header pins; authenticated
rows pinned unchanged (read-only→403, admin past-the-gate, read-only
ssh-keys→201, none-mode→201). The wider inventory (issues/pulls/star/watch/
webhooks/releases/tags/mirror/import/…) is covered by the catch-all test
(api_anon_write502_test.go: POST/PUT/PATCH/DELETE past a counting handler
→ 401, handler never runs; GET/HEAD/OPTIONS + authed POST pass through) plus
the pre-existing per-package requireAuthenticated tests. Given (3)'s
wiring verification, that combination closes the criterion.

6. Interstitial — no open redirect, return-to-action intact

web/src/lib/writeGate.js:31-35 sanitizeNextClient mirrors the server
(auth_oidc.go): must start with single /, //host and schemes → /.
LoginRequired.jsx defaults next=/, action="this action"; login href is
built only from discovery-advertised login_url + sanitized next, so no
attacker-controlled host ever enters the redirect. Back (href=next) +
Cancel (history.back() with next-fallback) both present. Static route
registered before /:owner (index.jsx:71-74) with the reservation
documented — same class as /orgs/new. 390px/themes by reasoning only
(max-w-xl px-4 card, shared card/btn/muted + dark: variants, no
browser drive per instructions) — stated, not proven.

7. writeGate wiring — one pattern, logged-in unaffected

Repo star/watch pre-flight + forkHref() gate (Repo.jsx:203,438,608),
New-issue/New-pull hrefs, composer fallbacks + reaction pre-flight
(Issue.jsx/Pull.jsx), submit pre-flight + 401-catch on IssueNew/PullNew/
Fork/New/OrgNew/Import/ReleaseNew. write401Target keys off
isAnonymousViewer(me, discovery) + 401, and /api/v1/me always emits
anonymous: false for authenticated (discovery.go:165-177) — so a logged-in
user with a stale session falls to the existing tray/popup path, never the
interstitial. noPopupAuth default keeps the popup-retry path byte-identical
(core.js:478-499); opt-out threads through every writer's existing opts
spread. Import.jsx keeps a local anonymous() mirroring the same verdict
— duplication smell, same fail-closed direction, not worth a round-trip.

8-9. Coverage, hygiene, docs, deps — all green

internal/api 95.3% / internal/server 98.4% (≥95% gate holds),
-race clean on api/server/identity/releases/tags,
node --test web/test/unit/*.test.js 1121 pass / 1 fail — the 1 failure is
the smoke.test.js live-server subtest, reproduced on pristine main
(pass 2 / fail 1; the :8080 occupant serves /setup → 403), i.e.
pre-existing and environmental. vite build + esbuild (run directly via
node_modules/.bin, pnpm shim is broken inside worktrees — environmental)
green; go build ./..., gofmt clean, go vet clean. No manifest changes
(go.mod/package.json untouched — zero new deps). Docs amended as
specified: 06_server_http.md §8.6 + Decisions, 12_web_ui.md §1.2 carve-out

§2.3 route table + Decisions, features/01 §4.1 + Decisions.

Recommendation: READY TO MERGE

No fixes pushed (nothing structural found; the two nits above are
pre-existing/out-of-scope). Do not merge from this review — merging was
explicitly out of scope.

## Review: PR #507 (fix/issue-502) — adversarial security gate Verified in scratch worktree `/tmp/pr507` @ `1908744` (removed afterward). Main worktree untouched (still clean on `main`, read-only throughout except `git fetch`). No browser drive (per task instructions — tests + reasoning; stated explicitly below). No docker, no live-instance contact (note: something is listening on `127.0.0.1:8080` with `/setup` → 403 — I probed it read-only via curl for the smoke-test analysis and touched nothing). ### 1. ssh-keys leak (defect A) — CLOSED, pinned, self-service preserved `internal/api/sshkeys.go:80-87,124-131` — both mutations keep `AuthRead` but refuse `p.Anonymous` with 401 before touching the registry. Read-only authenticated principals still get 201 (`TestAuthedWriteRowsUnchanged502`), auth-none still gets 201 (principal is `auth.None()`, never anonymous — pinned in `TestSSHKeysAnonLeakClosed502`), and the matrix asserts the registry stays empty after anonymous POST/DELETE. No remaining AuthRead-write hole in `internal/api`: the only other POST-under-AuthRead routes are `policy/validate`, `policy/dry-run`, `settings/validate` (pure computation, no writes — and now also 401'd for anonymous by the middleware assert anyway). ### 2. gate() 401-for-anonymous (defect B) — correct, law-9 clean `internal/api/env.go:862,871` — `if p.Anonymous` now precedes the flag checks on both AuthWrite/AuthAdmin. All three new refusal paths emit `WWW-Authenticate: Bearer realm="walgit"` (gate via `writePlain`, ssh-keys via `writePlain`, apiServe explicitly) — asserted per-route in the matrix test. Law 9 improves: the old 403 fall-through never triggered git-credential erase; 401 does. No legit flow depended on anonymous-403 (anonymous could do nothing with it); service accounts carry tokens (non-anonymous, pinned passthrough in `TestAPIServeOIDCAuthedWritePasses502`). Stale-pin flips 403→401 in `gaps3_test.go:118`, `placeholder_edge_test.go:47`, `profile_test.go:185` are exactly this behavior change — legitimate. ### 3. Middleware assert — lawful placement, no breakage found `internal/server/router.go:50-54` — fires only when ALL hold: anonymous + `Mode == "oidc"` + non-GET/HEAD/OPTIONS, after `Authenticate` (so invalid credentials still 401 first) and after `identityForward`. Checked each risk: OIDC callback/login/logout are GET-only (`authFlow`, router.go:204-216) — unaffected. health/setup are separate lanes, not apiServe — unaffected. none-mode never anonymous — unaffected (comment says so, code confirms: `auth.None()` is not anonymous). Preflight OPTIONS excluded; `isWriteMethod` fail-closes every other verb. Token mode untouched (pinned passthrough). Inter-instance/service calls use Bearer tokens → non-anonymous → pass. "Webhooks with secrets": no such inbound path exists — `internal/notify` delivery routes are outbound test-pings (admin-gated, in-seam); CRUD was already 401/403-gated. All feature routes mount through the `RouteProvider` chain (`bind_api.go`) → `s.api.Serve` ← `apiServe`, so the assert genuinely covers issues/pulls/social/notify/mirror/import/identity/releases/tags — verified the wiring, not assumed. Two pre-existing out-of-seam notes (NOT blockers, refused either way, unchanged by this PR): `POST /_events/notify` anonymous → 403 via `requireWrite` (auth.go:336), and setup PUT anonymous → 403 via `requireAdmin` — both pre-date this issue's inventory. ### 4. Releases/tags 401 — verified in code, not assumed `internal/releases/releases.go:362-372` and `internal/tags/service.go:380-390`: `requireRole` returns `ErrUnauthorized` (→401) for anonymous, `ErrForbidden` for authenticated-but-insufficient. Untouched, as claimed. ### 5. Matrix grid — scoped honestly, covered in combination `internal/api/anonwrite502_test.go` grids anonymous × the 10 seam-package writes (repo PUT/DELETE, settings PUT/DELETE, policy PUT/DELETE, ops POST, profile PUT, ssh-keys POST/DELETE) — all 401 + header pins; authenticated rows pinned unchanged (read-only→403, admin past-the-gate, read-only ssh-keys→201, none-mode→201). The wider inventory (issues/pulls/star/watch/ webhooks/releases/tags/mirror/import/…) is covered by the catch-all test (`api_anon_write502_test.go`: POST/PUT/PATCH/DELETE past a counting handler → 401, handler never runs; GET/HEAD/OPTIONS + authed POST pass through) plus the pre-existing per-package `requireAuthenticated` tests. Given (3)'s wiring verification, that combination closes the criterion. ### 6. Interstitial — no open redirect, return-to-action intact `web/src/lib/writeGate.js:31-35` `sanitizeNextClient` mirrors the server (`auth_oidc.go`): must start with single `/`, `//host` and schemes → `/`. `LoginRequired.jsx` defaults `next=/`, `action="this action"`; login href is built only from discovery-advertised `login_url` + sanitized next, so no attacker-controlled host ever enters the redirect. Back (`href=next`) + Cancel (`history.back()` with next-fallback) both present. Static route registered before `/:owner` (`index.jsx:71-74`) with the reservation documented — same class as `/orgs/new`. 390px/themes by reasoning only (`max-w-xl px-4` card, shared `card`/`btn`/`muted` + `dark:` variants, no browser drive per instructions) — stated, not proven. ### 7. writeGate wiring — one pattern, logged-in unaffected Repo star/watch pre-flight + `forkHref()` gate (Repo.jsx:203,438,608), New-issue/New-pull hrefs, composer fallbacks + reaction pre-flight (Issue.jsx/Pull.jsx), submit pre-flight + 401-catch on IssueNew/PullNew/ Fork/New/OrgNew/Import/ReleaseNew. `write401Target` keys off `isAnonymousViewer(me, discovery)` + 401, and `/api/v1/me` always emits `anonymous: false` for authenticated (discovery.go:165-177) — so a logged-in user with a stale session falls to the existing tray/popup path, never the interstitial. `noPopupAuth` default keeps the popup-retry path byte-identical (`core.js:478-499`); opt-out threads through every writer's existing opts spread. `Import.jsx` keeps a local `anonymous()` mirroring the same verdict — duplication smell, same fail-closed direction, not worth a round-trip. ### 8-9. Coverage, hygiene, docs, deps — all green `internal/api` 95.3% / `internal/server` 98.4% (≥95% gate holds), `-race` clean on api/server/identity/releases/tags, `node --test web/test/unit/*.test.js` 1121 pass / 1 fail — the 1 failure is the `smoke.test.js` live-server subtest, reproduced on pristine `main` (pass 2 / fail 1; the :8080 occupant serves `/setup` → 403), i.e. pre-existing and environmental. `vite build` + `esbuild` (run directly via `node_modules/.bin`, pnpm shim is broken inside worktrees — environmental) green; `go build ./...`, `gofmt` clean, `go vet` clean. No manifest changes (`go.mod`/`package.json` untouched — zero new deps). Docs amended as specified: `06_server_http.md` §8.6 + Decisions, `12_web_ui.md` §1.2 carve-out + §2.3 route table + Decisions, `features/01` §4.1 + Decisions. ### Recommendation: READY TO MERGE No fixes pushed (nothing structural found; the two nits above are pre-existing/out-of-scope). Do not merge from this review — merging was explicitly out of scope.

crueber closed this issue

2026-09-14 00:38:46 +00:00

crueber commented

2026-09-14 00:38:46 +00:00

Author

Owner

Fixed by PR #507 (review clean — all 9 adversarial checks pass, leak closed, middleware lawful, matrix green), merged. Closing.

crueber referenced this issue from a commit

2026-09-14 00:38:48 +00:00

Merge pull request 'Fix #502: anonymous write interstitial' (#507) from fix/issue-502 into main

crueber referenced this issue

2026-09-14 22:42:07 +00:00