fix(web): send CSRF token with bulk toolbar POSTs - #659
Merged
Conversation
Two pre-existing breakages made every local commit fail before any hook ran: - tekwizely/pre-commit-golang v1.0.0-rc.1 ships `go-imports`, not the configured `go-fmt-goimports`, so pre-commit aborted environment setup with "hook not present in repository". - .prettierrc.json still loaded prettier-plugin-tailwindcss and prettier-plugin-go-template, leftovers from the removed bun/Tailwind toolchain; with no package.json in the repo the mirrors-prettier hook cannot resolve them and errors out on every run. Assisted-by: claude-code:claude-fable-5 Agent-Session: https://claude.ai/code/session_01D41i7TcscCHnuQ22AJzr4S Agent-Host: 0493f0 Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
The v2.13.1 bump (#657) updated the tool version without re-running go mod tidy, leaving go.mod/go.sum out of sync with the new dependency graph. The pre-commit go-mod-tidy hook now flags this on every local commit. Tidied under the pinned go1.27.0 toolchain; build and tests pass, including the e2e-tagged package. Assisted-by: claude-code:claude-fable-5 Agent-Session: https://claude.ai/code/session_01D41i7TcscCHnuQ22AJzr4S Agent-Host: 0493f0 Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
The bulk-select toolbar (v2-bulk.js) built its POST form with only target_dn + extras — no csrf_token field — and the list pages it lives on never rendered a token anywhere in their DOM. Every toolbar action (add members / add to group / remove from group / disable / delete, across all three scopes) therefore died in the CSRF middleware with 403 "CSRF token validation failed", regardless of deployment setup. Fix: the three list templates now expose the session's CSRF token as a data-csrf attribute on main[data-bulk-scope], and submitForm() adds it to the generated form as a hidden csrf_token input — the same token/extractor contract the server-rendered drawer forms already use. Why no test caught it: setupFullTestApp registers routes without the CSRF middleware, and the e2e bulk test drives the drawer's Delete form (server-rendered, token embedded), never the JS-built toolbar form. New coverage closes both halves: - internal/web/bulk_csrf_test.go runs the list GET + bulk POST behind the real CSRF middleware in production order and asserts the page-exposed token round-trips (verified red on the unfixed tree: "missing csrf token in form"). - internal/e2e/bulk_toolbar_csrf_test.go drives the actual toolbar in Chromium: checkbox → "Add members…" → prompt → POST, then asserts the membership change landed (verified red with only the JS fix reverted, green with it). Fixes #652 Assisted-by: claude-code:claude-fable-5 Agent-Session: https://claude.ai/code/session_01D41i7TcscCHnuQ22AJzr4S Agent-Host: 0493f0 Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
github-actions
Bot
added
dependencies
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
Contributor
|
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because the head coverage (66.97%) is below the target coverage (70.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #659 +/- ##
==========================================
+ Coverage 74.07% 74.21% +0.14%
==========================================
Files 36 36
Lines 3151 3157 +6
==========================================
+ Hits 2334 2343 +9
+ Misses 817 814 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Review follow-up on the #652 fix: - e2e: wrap the "Add members…" click in ExpectNavigation. The page is already on /groups when the click fires, so the previous WaitForURL("**/groups") matched the current URL and resolved before the POST navigation — the 403 assertion could run against the pre-POST DOM and the follow-up Navigate could abort the in-flight POST on a slow runner. - v2-bulk.js: console.error when main[data-bulk-scope] carries no data-csrf token, so a regression to the silent pre-fix 403 is diagnosable from the browser console. console.error for error conditions matches v2-graph.js / v2-palette.js. - unit: new TestBulkToolbar_ListPageIsNeverServedFromCache pins the setupRoutes invariant that list pages (which now embed a session-scoped token) are never STORED by the template cache; the test harness gains templateCacheMiddleware to mirror production. Assisted-by: claude-code:claude-fable-5 Agent-Session: https://claude.ai/code/session_01D41i7TcscCHnuQ22AJzr4S Agent-Host: 0493f0 Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
The new toolbar test leaves its seeded group behind (its action under test is add-members, not delete), and later tests in the package pick groups dynamically: in CI, TestAddRemoveGroupMembership selected the stale bulk-toolbar-csrf-* group (sorts before "developers") and failed with LDAP error 20 "Attribute Or Value Exists" because testuser1 was already a member. Remove the group via ldapdelete in t.Cleanup. Verified locally by running the polluter + victim pair in CI order: both pass with the cleanup in place. Assisted-by: claude-code:claude-fable-5 Agent-Session: https://claude.ai/code/session_01D41i7TcscCHnuQ22AJzr4S Agent-Host: 0493f0 Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Second adversarial review round; every finding verified by injecting the defect it targets: - e2e membership assertion was vacuous: the group detail page renders "testuser1" in the add-user datalist of UNASSIGNED users too, so a silently failed add (per-entry LDAP error, 303 redirect, no flash) kept the test green. Now asserts the member tag's remove-form input, which renders only for actual members — confirmed red against a handler mutant that suppresses AddUserToGroup, green when restored. - The cache-refresh poll loops (here and in bulk_delete_test) called WaitFor-style helpers carrying the 30s page timeout per miss against a server-rendered, post-load-static DOM — the "45s, 1s ticks" loop really made ~2 attempts and could overrun or spuriously skip. Poll Locator.Count() instead. - Cache-invariant guard was timing-coupled: with the harness's 100ms TTL, a stored-but-expired entry also reads as MISS, silently passing the very RenderWithCache mutation the guard exists to catch. TTL is now minutes-scale. - Token-exposure and never-cached guards covered /groups only while /users and /computers received identical plumbing. Both guards now loop over all three list pages; emptying the token argument in either extra handler reddens the guard at that path (verified). Assisted-by: claude-code:claude-fable-5 Agent-Session: https://claude.ai/code/session_01D41i7TcscCHnuQ22AJzr4S Agent-Host: 0493f0 Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Third review round returned no majors; one minor closed here: the
three-page token-exposure loop asserted each token non-empty but
POSTed only the last one, so a wrong-but-non-empty token on a single
page (hardcoded string in one handler) survived. CSRF storage is
session-backed — one token per session — so the loop now requires all
three pages to render the identical token. Verified red against a
hardcoded-token mutant in the computers handler ("token diverged on
/computers"), green restored.
Assisted-by: claude-code:claude-fable-5
Agent-Session: https://claude.ai/code/session_01D41i7TcscCHnuQ22AJzr4S
Agent-Host: 0493f0
Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
The previous cleanup deleted the group directly in LDAP, leaving a ghost in the app's 30s cache. In CI timing that ghost became the first entry in TestAddRemoveGroupMembership's addable-groups datalist (bulk-toolbar-csrf-* sorts before developers); adding testuser1 to the nonexistent group failed silently, left the user with zero memberships, and cascaded into TestAddGroupAsNonAdminShowsFlash. The earlier "benign by cache ordering" assessment was wrong — it held only by refresh-timing luck. Now the test's final step deletes the group via the bulk "Delete groups" toolbar action, which removes it from LDAP and the app cache in one stroke — and exercises submitForm's second groups-scope action through the same CSRF plumbing this PR fixes. The direct ldapdelete remains as a failure-path backstop and now waits out one cache refresh cycle so a ghost cannot poison later tests even when this test dies early. Verified: full e2e suite green locally (101s), including the polluter followed by both previously cascading tests in CI order. Assisted-by: claude-code:claude-fable-5 Agent-Session: https://claude.ai/code/session_01D41i7TcscCHnuQ22AJzr4S Agent-Host: 0493f0 Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
|
CybotTM
marked this pull request as ready for review
August 25, 2026 23:46
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
github-merge-queue
Bot
removed this pull request from the merge queue due to a conflict with the base branch
Aug 25, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #652.
Root cause. The bulk-select toolbar (
v2-bulk.js) builds its POST form dynamically with onlytarget_dn+ extras — it never included acsrf_tokenfield, and the list pages it lives on never rendered a token anywhere in their DOM, so the JS had no token to send. Every toolbar action (add members, add to group, remove from group, disable, delete — all three scopes) therefore died in Fiber's CSRF middleware with 403 "CSRF token validation failed". The reporter's nginx setup was innocent: the failure is environment-independent and matches the screenshot URL/groups/bulk?action=add-membersexactly.Fix. The three list templates now expose the session's CSRF token as a
data-csrfattribute onmain[data-bulk-scope], andsubmitForm()adds it to the generated form as a hiddencsrf_tokeninput — the same token/extractor contract (form:csrf_token) the server-rendered drawer forms already use; a missing token now logs aconsole.errorinstead of failing silently. List pages are not affected by the template cache: only graph views are ever stored (RenderWithCache); a note insetupRoutesrecords that constraint and a test pins it.Why no test caught it.
setupFullTestAppregisters routes without the CSRF middleware, and the existing e2e bulk test drives the drawer's Delete form, which is server-rendered with an embedded token — the JS-built toolbar form was never exercised.Tests. Every guard was verified against an injected defect, not just green on the fixed tree:
internal/web/bulk_csrf_test.go— behind the real CSRF middleware in production order (RequireAuth→ csrf → cache middleware): all three list pages must expose the identical session token asdata-csrf(emptying the token argument in any handler reddens the guard at that path; a hardcoded wrong-but-non-empty token reddens the identity check), the token round-trips through a bulk POST, a token-less POST still 403s, and list pages are never served from the template cache (minutes-scale harness TTL so an expired entry can't fake a MISS).internal/e2e/bulk_toolbar_csrf_test.go— drives the actual toolbar in Chromium against OpenLDAP: checkbox → "Add members…" → prompt → POST (ExpectNavigation-wrapped), then asserts the member tag's remove-form input — which renders only for actual members, so a silently failed LDAP add is caught (verified red against a handler mutant suppressingAddUserToGroup, red with only the JS fix reverted, and the unmutated test green both locally and in CI's e2e job at 608e188: https://github.com/netresearch/ldap-manager/actions/runs/32909180989/job/97999653848). The disposable group is deleted int.Cleanupso later tests aren't polluted.Three adversarial review passes ran over this branch; the third returned no major findings. All findings from all rounds (vacuous waits, a vacuous membership assertion, timing-coupled cache guard, single-page coverage, token-identity gap, e2e state pollution, silent missing-token path) are fixed and mutation-verified. The cache-refresh poll loops here and in
bulk_delete_test.gonow pollLocator.Count()instead of burning the 30s page timeout per miss.Also in this PR — two pre-existing breakages of the local commit gate, fixed because they blocked committing anything: the pre-commit config referenced the nonexistent hook id
go-fmt-goimports(the pinned rev shipsgo-imports), and.prettierrc.jsonstill loaded Tailwind-era plugins that no longer resolve (nopackage.jsonin the repo). Plusgo mod tidydrift left behind by the golangci-lint v2.13.1 bump (#657), which the now-workinggo-mod-tidyhook flags.Assisted by claude-code:claude-fable-5 — Session