Skip to content

refactor(inspector): mount at root, drop /inspector/login + legacy bearer cookie#7

Merged
aaronbrethorst merged 6 commits into
mainfrom
landing-page
May 10, 2026
Merged

refactor(inspector): mount at root, drop /inspector/login + legacy bearer cookie#7
aaronbrethorst merged 6 commits into
mainfrom
landing-page

Conversation

@aaronbrethorst
Copy link
Copy Markdown
Member

@aaronbrethorst aaronbrethorst commented May 10, 2026

Summary

  • Move the inspector from /inspector/... to the server root: / (events dashboard), /me, /tokens, /push, /users, /audit, /events/{source}/{seq}, /logout. Static assets move from /inspector/static/ to /assets/stylesheets/.
  • Remove the deprecated /inspector/login admin-token form and the now-functionally-dead legacy hooks_inspector_token bearer-cookie auth path. Inspector auth is session-only via /login (the email/password page).
  • Fix a latent bug: GET /logout now actually invalidates the server-side session and clears cookies, instead of the previous no-op redirect. This affected anyone clicking "Sign out" in the header — they appeared to be logged out but their session row was still alive.

Also includes doc updates (CLAUDE.md, README, docs/), removal of stale task-number references per CLAUDE.md guidance, and a small dead-code simplification in inspector_users.go where if in.Sessions != nil guards became unreachable after the auth-path removal.

Test plan

  • make lint — 0 issues
  • go test -race ./... — all green
  • Browser smoke (Playwright): signup → sign-in → land on / events dashboard; nav links resolve to new paths; /logout clears the session
  • curl smoke: anonymous / and /me 302 to /login?next=...; /inspector and /inspector/login return 404; /assets/stylesheets/style.css serves CSS
  • New unit test TestInspectorLogoutDestroysSession locks in the session-destruction behavior so a future refactor can't silently regress it

Review notes

Pre-existing concern surfaced during review (not addressed in this PR, flagging for visibility): admin-only mutation routes /tokens/* and /push/* are session-authenticated but not CSRF-protected. With bearer-cookie auth removed, every admin route now relies on the session cookie, which means CSRF is now in scope for these endpoints. Worth a follow-up PR.

Migration notes

After this lands, anyone with an old /inspector-scoped browser cookie will simply be treated as anonymous (path mismatch — browser won't send it) and bounced to /login. No data migration needed.

Summary by CodeRabbit

  • Documentation

    • Updated docs and quickstart to reflect admin UI mounted at root routes and revised authentication guidance; signup flow and troubleshooting updated.
  • Refactor

    • Admin interface moved to root-level routes (/, /me, /users, /audit, /tokens, /push); templates and navigation updated; styles now served from /assets/stylesheets/.
    • Authentication switched to session-cookie only; legacy bearer-cookie path removed.
  • Tests

    • Tests updated to cover new routes and session-based flows; legacy bearer-cookie tests removed.
  • Chores

    • Added .playwright-mcp/ to .gitignore.

Review Change Stack

…arer cookie

The inspector now lives at the server root (/, /me, /tokens, /push, /users,
/audit, /events/{source}/{seq}). Static assets moved from /inspector/static/
to /assets/stylesheets/. The deprecated /inspector/login admin-token form is
removed, and with it the legacy hooks_inspector_token bearer-cookie auth
path (functionally dead post-rename: browser-stored cookies at Path=/inspector
no longer match any served route). Auth is now session-only via /login.

GET /logout now actually invalidates the server-side session and clears
cookies, instead of the previous no-op redirect. This was a real bug for
session-authenticated users who clicked "Sign out" in the header.

Also: stripped task-number references from doc comments per CLAUDE.md
guidance ("references to the current task ... rot as the codebase
evolves"); ignored .playwright-mcp/ smoke-test artifacts.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 10, 2026

Warning

Rate limit exceeded

@aaronbrethorst has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 49 minutes and 53 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 1f5dc476-8d83-493e-a557-c5a2301530a2

📥 Commits

Reviewing files that changed from the base of the PR and between b8bb2bb and d632f34.

📒 Files selected for processing (1)
  • .sonarcloud.properties
📝 Walkthrough

Walkthrough

The inspector admin UI is migrated from the legacy /inspector/* path space to root-mounted paths, and authentication is switched from bearer-token cookies to session-based authentication via auth.Manager. Type contracts, route registration, handlers, templates, tests, and documentation are updated throughout to support this refactor.

Changes

Inspector Refactor

Layer / File(s) Summary
Type Contract & Interfaces
internal/inspector/inspector.go
Inspector struct removes Auth *tokens.Authenticator field and documents Sessions significance; new Cascader interface added for user cascade operations.
Constructor & Server Wiring
internal/inspector/inspector.go, internal/server/server.go, internal/inspector/inspector_test.go
New() constructor drops tokens.Authenticator parameter; embedded templates and static FS initialized; server wires auth.Manager into Inspector.Sessions; test fixture refactored to use session-based auth with makeUser and login helpers.
Route Registration & Mounting
internal/inspector/inspector.go
Register() mounts handlers at root paths (/, /events/..., /tokens, /push, /me, /users, /audit) with optional session middleware; static assets served from /assets/stylesheets/.
Authorization & Auth Handlers
internal/inspector/inspector.go
requireAdmin checks session for admin role, redirects non-admin GETs to /me, returns 403 for non-GET mutations; denyUnauthorized redirects to /login?next=...; logout invalidates session row and clears cookies.
Audit Handler & Tests
internal/inspector/inspector_audit.go, internal/inspector/inspector_audit_test.go
Audit page at /audit (non-admin redirects to /me); test routes updated from /inspector/audit to /audit.
Me (Profile) Handler & Tests
internal/inspector/inspector_me.go, internal/inspector/inspector_me_test.go
Profile page at /me with token/subscription management; form actions at /me/tokens, /me/tokens/{id}/revoke; revoke redirects to /me; tests updated for new paths.
Me/Push Handler & Tests
internal/inspector/inspector_me_push.go, internal/inspector/inspector_me_push_test.go
User-owned subscriptions at /me/push; pause/resume/test/rotate/delete redirect to /me/push; admin banner link to /push; tests updated for new paths.
Users Management Handler & Logic
internal/inspector/inspector_users.go, internal/inspector/inspector_users_test.go
User page at /users; CreatedByUserID and audit ActorUserID always derived from session (non-nil); post-mutation redirects to /users; tests updated for new paths.
Redirect Target Updates
internal/inspector/inspector.go
Event replay redirects to /events/..., token revoke to /tokens, push actions to /push.
Template Path Updates
internal/inspector/templates/*.tmpl.html, internal/webpages/templates/*.tmpl.html
All form action URLs updated from /inspector/... to root paths; stylesheets updated from /inspector/static/style.css to /assets/stylesheets/style.css; nav links to /, /me, /push, /tokens, /users, /audit, /logout.
Test Coverage: Sessions & Features
internal/inspector/inspector_test.go, internal/inspector/inspector_session_test.go, internal/server/server_test.go, feature test files
Session tests updated for root redirects and legacy bearer-token tests removed; feature tests updated for new routes; server integration test updated to request root path; logout session-destruction tests added.
Webpages & Login Flow
internal/webpages/pages.go, internal/webpages/pages_test.go, internal/webpages/templates/*.tmpl.html
LoginPOST default redirect changed from /inspector to /; safeNext allows / and /me (removes /inspector paths); device/signup templates updated to use shared stylesheet path.
API & System Documentation
CLAUDE.md, docs/*.md, README.md, cmd/hooks/main.go
CLAUDE.md documents new root-mount paths and session-cookie auth (no bearer-token path); docs updated to /, /users, /audit, /push paths; quickstart and README show new inspector entry point.
Comments & Minor Changes
Multiple test and utility files
Comment-only updates removing task references; added PAT expiry validation tests in internal/tokens/middleware_test.go; .gitignore updated for .playwright-mcp/.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 73.17% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main changes: refactoring the inspector to mount at root paths and removing /inspector/login and legacy bearer cookie authentication.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch landing-page

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
internal/inspector/templates/push.tmpl.html (1)

8-49: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Admin /push forms are missing CSRF token fields.

None of the POST forms here include a csrf_token hidden input (compare with me.tmpl.html which does include one). Now that bearer-cookie auth is gone and these admin mutations are cookie-session-authenticated only, they are exposed to CSRF. The PR description already calls this out as in-scope follow-up; flagging here so the template change isn't forgotten when CSRF is wired into /push/* mutation handlers.

🛡️ Sketch of the per-form change (paired with handler-side CSRF middleware)
-<form method="post" action="/push/create" class="form-row">
+<form method="post" action="/push/create" class="form-row">
+  <input type="hidden" name="csrf_token" value="{{.CSRFToken}}">
   <label>Source <input type="text" name="source" required></label>
   ...
 </form>
@@
-        <form method="post" action="/push/{{.ID}}/resume"><button>Resume</button></form>
+        <form method="post" action="/push/{{.ID}}/resume"><input type="hidden" name="csrf_token" value="{{$.CSRFToken}}"><button>Resume</button></form>

…and analogous additions for /pause, /test, /rotate, /delete. The render path for this template will also need to plumb CSRFToken into the data map.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/inspector/templates/push.tmpl.html` around lines 8 - 49, The POST
forms in push.tmpl.html (notably the /push/create form and each per-subscription
forms posting to /push/{{.ID}}/pause, /push/{{.ID}}/resume, /push/{{.ID}}/test,
/push/{{.ID}}/rotate, /push/{{.ID}}/delete) lack a hidden csrf_token field and
must include one; add a hidden input named csrf_token with the template value
(e.g. value="{{.CSRFToken}}") to every POST form, and update the handler render
path that supplies this template to include CSRFToken in the data map so the
token is available to the template.
♻️ Duplicate comments (1)
internal/inspector/templates/tokens.tmpl.html (1)

3-37: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Same CSRF-token gap as push.tmpl.html.

The token-create and token-revoke forms post to admin mutation routes (/tokens/create, /tokens/{id}/revoke) without a csrf_token hidden field. After the bearer-cookie auth removal these are cookie-session-only; please add the same hidden CSRF input pattern used in me.tmpl.html when the matching CSRF middleware is added to the /tokens/* handlers.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/inspector/templates/tokens.tmpl.html` around lines 3 - 37, Add the
same hidden CSRF input pattern used in me.tmpl.html to the token creation and
revoke forms: include a hidden input named "csrf_token" with the template value
(e.g. {{.CSRFToken}}) inside the <form method="post" action="/tokens/create"
...> block and inside the per-token revoke form that posts to
"/tokens/{{.ID}}/revoke"; update the tokens.tmpl.html template to render that
hidden field wherever the CSRF middleware will validate requests so both the
create (form action="/tokens/create") and revoke
(action="/tokens/{{.ID}}/revoke") handlers receive the token.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@CLAUDE.md`:
- Around line 59-60: Update the CLAUDE.md paragraph in the internal/tokens docs
to remove the statement that the `admin` scope grants access to the inspector:
explicitly note that the inspector is session-only after the refactor and is
accessed via the new `/login` flow, and clarify that `admin` still does NOT
implicitly grant `/subscribe/<source>` and that
`tokens.AttachVerifier`/`LookupByPlaintext` behavior is unchanged; in short,
delete or replace the sentence claiming inspector access for `admin` and add a
brief line pointing operators to the `/login` session flow instead.

In `@internal/inspector/inspector.go`:
- Around line 137-179: The listed admin POST/critical routes (in.replay,
in.tokensList, in.tokensCreate, in.tokensRevoke, in.pushList, in.pushCreate,
in.pushPause, in.pushResume, in.pushTest, in.pushRotate, in.pushDelete and their
/me and /users equivalents) are not using the CSRF middleware; wrap these
handlers the same way as /me and /users by passing them through the csrf helper
(csrf := func(h http.Handler) http.Handler { return
web.Middleware(web.CSRFConfig{}, h) }) and registering them with
wrapH(csrf(http.HandlerFunc(...))) or wrapH(csrf(...)) so POST mutating routes
(replay, tokens create/revoke, push create/pause/resume/test/rotate/delete) are
protected.

In `@README.md`:
- Line 114: Update the README sentence that currently reads "A new row in the
inspector at `http://localhost:8080/` (sign in with the admin email/password you
set during `hooks init`)." to correctly state that the admin credentials are
created during signup via the bootstrap link generated by `hooks init` (e.g.,
"sign in with the admin email/password you created during signup via the
bootstrap link generated by `hooks init`"); edit the line containing that exact
sentence to replace the incorrect credential source wording.

---

Outside diff comments:
In `@internal/inspector/templates/push.tmpl.html`:
- Around line 8-49: The POST forms in push.tmpl.html (notably the /push/create
form and each per-subscription forms posting to /push/{{.ID}}/pause,
/push/{{.ID}}/resume, /push/{{.ID}}/test, /push/{{.ID}}/rotate,
/push/{{.ID}}/delete) lack a hidden csrf_token field and must include one; add a
hidden input named csrf_token with the template value (e.g.
value="{{.CSRFToken}}") to every POST form, and update the handler render path
that supplies this template to include CSRFToken in the data map so the token is
available to the template.

---

Duplicate comments:
In `@internal/inspector/templates/tokens.tmpl.html`:
- Around line 3-37: Add the same hidden CSRF input pattern used in me.tmpl.html
to the token creation and revoke forms: include a hidden input named
"csrf_token" with the template value (e.g. {{.CSRFToken}}) inside the <form
method="post" action="/tokens/create" ...> block and inside the per-token revoke
form that posts to "/tokens/{{.ID}}/revoke"; update the tokens.tmpl.html
template to render that hidden field wherever the CSRF middleware will validate
requests so both the create (form action="/tokens/create") and revoke
(action="/tokens/{{.ID}}/revoke") handlers receive the token.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6848db70-0ace-4544-ac8f-512b1e165b94

📥 Commits

Reviewing files that changed from the base of the PR and between dfa5b60 and 2546a8c.

📒 Files selected for processing (44)
  • .gitignore
  • CLAUDE.md
  • README.md
  • cmd/hooks/main.go
  • docs/accounts.md
  • docs/operations.md
  • docs/quickstart.md
  • docs/security.md
  • internal/admin/users_test.go
  • internal/auth/sessions.go
  • internal/inspector/inspector.go
  • internal/inspector/inspector_audit.go
  • internal/inspector/inspector_audit_test.go
  • internal/inspector/inspector_me.go
  • internal/inspector/inspector_me_push.go
  • internal/inspector/inspector_me_push_test.go
  • internal/inspector/inspector_me_test.go
  • internal/inspector/inspector_session_test.go
  • internal/inspector/inspector_test.go
  • internal/inspector/inspector_users.go
  • internal/inspector/inspector_users_test.go
  • internal/inspector/templates/detail.tmpl.html
  • internal/inspector/templates/header.tmpl.html
  • internal/inspector/templates/index.tmpl.html
  • internal/inspector/templates/login.tmpl.html
  • internal/inspector/templates/me.tmpl.html
  • internal/inspector/templates/me_push.tmpl.html
  • internal/inspector/templates/push.tmpl.html
  • internal/inspector/templates/tokens.tmpl.html
  • internal/inspector/templates/users.tmpl.html
  • internal/me/api_test.go
  • internal/server/server.go
  • internal/server/server_test.go
  • internal/store/users_store_test.go
  • internal/tokens/middleware.go
  • internal/tokens/middleware_test.go
  • internal/tokens/tokens_test.go
  • internal/webpages/pages.go
  • internal/webpages/pages_test.go
  • internal/webpages/templates/device_approve.tmpl.html
  • internal/webpages/templates/device_done.tmpl.html
  • internal/webpages/templates/device_lookup.tmpl.html
  • internal/webpages/templates/login.tmpl.html
  • internal/webpages/templates/signup.tmpl.html
💤 Files with no reviewable changes (1)
  • internal/inspector/templates/login.tmpl.html

Comment thread CLAUDE.md
Comment on lines +137 to +179
mux.Handle("POST /events/{source}/{sequence}/replay", wrap(in.replay))
mux.Handle("GET /tokens", wrap(in.tokensList))
mux.Handle("POST /tokens/create", wrap(in.tokensCreate))
mux.Handle("POST /tokens/{id}/revoke", wrap(in.tokensRevoke))
mux.Handle("GET /push", wrap(in.pushList))
mux.Handle("POST /push/create", wrap(in.pushCreate))
mux.Handle("POST /push/{id}/pause", wrap(in.pushPause))
mux.Handle("POST /push/{id}/resume", wrap(in.pushResume))
mux.Handle("POST /push/{id}/test", wrap(in.pushTest))
mux.Handle("POST /push/{id}/rotate", wrap(in.pushRotate))
mux.Handle("POST /push/{id}/delete", wrap(in.pushDelete))

// /me is the user self-service page. Mutations run through the shared
// CSRF middleware so the inspector and /api/me/* enforce the same
// double-submit + Origin contract.
csrf := func(h http.Handler) http.Handler {
return web.Middleware(web.CSRFConfig{}, h)
}
mux.Handle("GET /inspector/me", wrap(in.meIndex))
mux.Handle("POST /inspector/me/tokens", wrapH(csrf(http.HandlerFunc(in.meCreateToken))))
mux.Handle("POST /inspector/me/tokens/{id}/revoke", wrapH(csrf(http.HandlerFunc(in.meRevokeToken))))

// /inspector/me/push (task 11.7) — user-owned push-subscription view
// mirroring /inspector/push without the owner column. Mutations share
// the CSRF middleware so the same double-submit + Origin contract
// applies as elsewhere on /inspector/me.
mux.Handle("GET /inspector/me/push", wrap(in.mePushIndex))
mux.Handle("POST /inspector/me/push/{id}/pause", wrapH(csrf(http.HandlerFunc(in.mePushPause))))
mux.Handle("POST /inspector/me/push/{id}/resume", wrapH(csrf(http.HandlerFunc(in.mePushResume))))
mux.Handle("POST /inspector/me/push/{id}/test", wrapH(csrf(http.HandlerFunc(in.mePushTest))))
mux.Handle("POST /inspector/me/push/{id}/rotate", wrapH(csrf(http.HandlerFunc(in.mePushRotate))))
mux.Handle("POST /inspector/me/push/{id}/delete", wrapH(csrf(http.HandlerFunc(in.mePushDelete))))

// /inspector/audit (task 11.6): admin-only HTML view of the audit log.
mux.Handle("GET /inspector/audit", wrap(in.auditList))

// /inspector/users (task 11.5): admin-only user table + invite form
// + per-row deactivate/reactivate/reset-password/edit. Mutations run
// through the same CSRF middleware as /inspector/me.
mux.Handle("GET /inspector/users", wrap(in.usersList))
mux.Handle("POST /inspector/users/invite", wrapH(csrf(http.HandlerFunc(in.usersInvite))))
mux.Handle("POST /inspector/users/{id}/deactivate", wrapH(csrf(http.HandlerFunc(in.usersDeactivate))))
mux.Handle("POST /inspector/users/{id}/reactivate", wrapH(csrf(http.HandlerFunc(in.usersReactivate))))
mux.Handle("POST /inspector/users/{id}/reset-password", wrapH(csrf(http.HandlerFunc(in.usersResetPassword))))
mux.Handle("POST /inspector/users/{id}/update", wrapH(csrf(http.HandlerFunc(in.usersUpdate))))
}

// requireAdmin enforces admin access for an inspector request.
//
// Authentication sources, in order:
// 1. hooks_session cookie (task 11.12): if present and the user is admin,
// allow. If the user is non-admin, GET redirects to /inspector/me;
// non-GET returns 403.
// 2. legacy hooks_inspector_token cookie (task 11.11): plaintext bearer
// token in a cookie; admin scope required.
//
// Outcomes when no auth is present:
// - GET → 302 to /login?next=<path> (task 11.10).
// - non-GET → 401.
//
// Lookup failures from a non-auth source (DB unreachable, etc.) → 503 so
// operators don't mistake an outage for a bad token.
mux.Handle("GET /me", wrap(in.meIndex))
mux.Handle("POST /me/tokens", wrapH(csrf(http.HandlerFunc(in.meCreateToken))))
mux.Handle("POST /me/tokens/{id}/revoke", wrapH(csrf(http.HandlerFunc(in.meRevokeToken))))

// /me/push — user-owned push-subscription view mirroring /push without
// the owner column.
mux.Handle("GET /me/push", wrap(in.mePushIndex))
mux.Handle("POST /me/push/{id}/pause", wrapH(csrf(http.HandlerFunc(in.mePushPause))))
mux.Handle("POST /me/push/{id}/resume", wrapH(csrf(http.HandlerFunc(in.mePushResume))))
mux.Handle("POST /me/push/{id}/test", wrapH(csrf(http.HandlerFunc(in.mePushTest))))
mux.Handle("POST /me/push/{id}/rotate", wrapH(csrf(http.HandlerFunc(in.mePushRotate))))
mux.Handle("POST /me/push/{id}/delete", wrapH(csrf(http.HandlerFunc(in.mePushDelete))))

// /audit: admin-only HTML view of the audit log.
mux.Handle("GET /audit", wrap(in.auditList))

// /users: admin-only user table + invite form + per-row
// deactivate/reactivate/reset-password/edit. Mutations run through
// the same CSRF middleware as /me.
mux.Handle("GET /users", wrap(in.usersList))
mux.Handle("POST /users/invite", wrapH(csrf(http.HandlerFunc(in.usersInvite))))
mux.Handle("POST /users/{id}/deactivate", wrapH(csrf(http.HandlerFunc(in.usersDeactivate))))
mux.Handle("POST /users/{id}/reactivate", wrapH(csrf(http.HandlerFunc(in.usersReactivate))))
mux.Handle("POST /users/{id}/reset-password", wrapH(csrf(http.HandlerFunc(in.usersResetPassword))))
mux.Handle("POST /users/{id}/update", wrapH(csrf(http.HandlerFunc(in.usersUpdate))))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Protect the admin POST routes with the same CSRF middleware as /me/* and /users/*.

These handlers are now session-authenticated, but /events/.../replay, /tokens/*, and /push/* still bypass web.Middleware. That leaves token mint/revoke, push mutation, and replay actions triggerable by a cross-site form while an admin is logged in. The new session-only auth model makes this a real CSRF path.

Suggested fix
 func (in *Inspector) Register(mux *http.ServeMux) {
 	wrap := func(h http.HandlerFunc) http.Handler {
 		if in.Sessions == nil {
 			return h
 		}
 		return in.Sessions.Middleware(h)
 	}
 	// wrapH is the same as wrap but accepts an already-composed Handler
 	// (e.g. one already wrapped in CSRF middleware).
 	wrapH := func(h http.Handler) http.Handler {
 		if in.Sessions == nil {
 			return h
 		}
 		return in.Sessions.Middleware(h)
 	}
+	csrf := func(h http.Handler) http.Handler {
+		return web.Middleware(web.CSRFConfig{}, h)
+	}
 
 	mux.Handle("GET /assets/stylesheets/", http.StripPrefix("/assets/stylesheets/", http.FileServer(http.FS(in.staticSub))))
 	mux.Handle("GET /logout", wrap(in.logout))
 	mux.Handle("GET /{$}", wrap(in.index))
 	mux.Handle("GET /events/{source}/{sequence}", wrap(in.detail))
-	mux.Handle("POST /events/{source}/{sequence}/replay", wrap(in.replay))
+	mux.Handle("POST /events/{source}/{sequence}/replay", wrapH(csrf(http.HandlerFunc(in.replay))))
 	mux.Handle("GET /tokens", wrap(in.tokensList))
-	mux.Handle("POST /tokens/create", wrap(in.tokensCreate))
-	mux.Handle("POST /tokens/{id}/revoke", wrap(in.tokensRevoke))
+	mux.Handle("POST /tokens/create", wrapH(csrf(http.HandlerFunc(in.tokensCreate))))
+	mux.Handle("POST /tokens/{id}/revoke", wrapH(csrf(http.HandlerFunc(in.tokensRevoke))))
 	mux.Handle("GET /push", wrap(in.pushList))
-	mux.Handle("POST /push/create", wrap(in.pushCreate))
-	mux.Handle("POST /push/{id}/pause", wrap(in.pushPause))
-	mux.Handle("POST /push/{id}/resume", wrap(in.pushResume))
-	mux.Handle("POST /push/{id}/test", wrap(in.pushTest))
-	mux.Handle("POST /push/{id}/rotate", wrap(in.pushRotate))
-	mux.Handle("POST /push/{id}/delete", wrap(in.pushDelete))
+	mux.Handle("POST /push/create", wrapH(csrf(http.HandlerFunc(in.pushCreate))))
+	mux.Handle("POST /push/{id}/pause", wrapH(csrf(http.HandlerFunc(in.pushPause))))
+	mux.Handle("POST /push/{id}/resume", wrapH(csrf(http.HandlerFunc(in.pushResume))))
+	mux.Handle("POST /push/{id}/test", wrapH(csrf(http.HandlerFunc(in.pushTest))))
+	mux.Handle("POST /push/{id}/rotate", wrapH(csrf(http.HandlerFunc(in.pushRotate))))
+	mux.Handle("POST /push/{id}/delete", wrapH(csrf(http.HandlerFunc(in.pushDelete))))
 
 	// /me is the user self-service page. Mutations run through the shared
 	// CSRF middleware so the inspector and /api/me/* enforce the same
 	// double-submit + Origin contract.
-	csrf := func(h http.Handler) http.Handler {
-		return web.Middleware(web.CSRFConfig{}, h)
-	}
 	mux.Handle("GET /me", wrap(in.meIndex))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/inspector/inspector.go` around lines 137 - 179, The listed admin
POST/critical routes (in.replay, in.tokensList, in.tokensCreate,
in.tokensRevoke, in.pushList, in.pushCreate, in.pushPause, in.pushResume,
in.pushTest, in.pushRotate, in.pushDelete and their /me and /users equivalents)
are not using the CSRF middleware; wrap these handlers the same way as /me and
/users by passing them through the csrf helper (csrf := func(h http.Handler)
http.Handler { return web.Middleware(web.CSRFConfig{}, h) }) and registering
them with wrapH(csrf(http.HandlerFunc(...))) or wrapH(csrf(...)) so POST
mutating routes (replay, tokens create/revoke, push
create/pause/resume/test/rotate/delete) are protected.

Comment thread README.md
- CLAUDE.md token-kinds section claimed listener and PAT tokens authorize
  the inspector. They don't anymore — the inspector is session-cookie only.
- README troubleshooting tip told users to paste the admin token into the
  browser. That hasn't worked since the legacy /inspector/login form was
  removed; the inspector now signs you in with email/password.
- README's Render walkthrough never told you to claim an admin account
  before signing in. Added a one-paragraph nudge in step 4 pointing back
  at the signup URL from step 2.
SonarCloud's "Duplication on New Code" gate flagged the post-rename
redirect targets "/users" (4×) and "/me/push" (4×) as duplicated literals.
Extract usersPath and mePushPath constants in their respective handler
files; same playbook as the prior runImageDetached extraction in
dockertest.
…gate

After the /inspector → / rename, Sonar's "Duplication on New Code" check
flagged 26-44% duplication on inspector_me_test.go, inspector_me_push_test.go,
and inspector_users_test.go. The duplication was real — every test built
its own http.Request/Do/ReadAll boilerplate inline.

Add three helpers on sessionFixture:
- getBody(t, path)              — GET, returns (resp, body string)
- postForm(t, path, form)       — POST form-urlencoded
- postCSRF(t, path, form)       — postForm + Origin header (for CSRF gates)

Then collapse the matching call sites. Same playbook as the prior
runImageDetached extraction in dockertest.
Test files routinely reuse the same NewRequest → Do → ReadAll → Close
boilerplate per test case (DAMP > DRY for tests), and Sonar's CPD has
been blocking the merge gate on this. Production code is still subject
to CPD.
Replaces sonar-project.properties (which only applies to CI-based analysis
runs that invoke sonar-scanner). This repo uses SonarCloud's automatic
analysis path, which reads .sonarcloud.properties from the default branch
on every PR.
@sonarqubecloud
Copy link
Copy Markdown

@aaronbrethorst aaronbrethorst merged commit 54146b4 into main May 10, 2026
7 checks passed
@aaronbrethorst aaronbrethorst deleted the landing-page branch May 10, 2026 04:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant