Skip to content

Commit ff65290

Browse files
authored
docs(agents): capture CSRF and e2e-cleanup learnings from #652 (#663)
Documentation follow-up from the #652 investigation (retro outcome). Three additions to `internal/web/AGENTS.md`, each a fact that cost real debugging time because it lived only in test comments and PR text: - The "CSRF errors" pointer now names the actual failure class: a persistent 403 "CSRF token validation failed" means the POST carries no `csrf_token`; JS-built forms read the session token from `data-csrf` on `main[data-bulk-scope]`, server-rendered forms embed a hidden input. - The test-infrastructure section states that `setupFullTestApp` registers routes without the CSRF middleware — the exact coverage gap that let #652 ship — and points to `setupCSRFBulkTestApp` for exercising the production chain. - The integration-test list gains the cleanup rule: delete seeded entries through the app, not via direct `ldapdelete` — the 30s directory cache keeps a ghost that poisons later tests (seen in CI as a cascading failure of `TestAddRemoveGroupMembership`). Two pre-existing over-length bullets in the same section are re-wrapped because the markdownlint hook (MD013, 120 columns) blocks any commit touching the file. _Assisted by claude-code:claude-fable-5 — [Session](https://claude.ai/code/session_01D41i7TcscCHnuQ22AJzr4S)_
2 parents b43afd9 + 486c406 commit ff65290

2 files changed

Lines changed: 32 additions & 8 deletions

File tree

internal/AGENTS.md

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ go test -bench=. ./internal/...
6565

6666
Follow Go's standard project layout:
6767

68-
```
68+
```text
6969
internal/
7070
├── ldap/ # Domain: LDAP operations
7171
│ ├── client.go # Public API
@@ -231,7 +231,7 @@ if err := ValidateUsername(input); err != nil {
231231

232232
### Package Organization
233233

234-
```
234+
```text
235235
internal/
236236
├── ldap/ # Domain: LDAP operations
237237
├── ldap_cache/ # Domain: Caching layer
@@ -328,9 +328,12 @@ func skipIfNoLDAP(t *testing.T) {
328328

329329
**Critical gotchas:**
330330

331-
- **Use `127.0.0.1` not `localhost`**: `simple-ldap-go` treats "localhost" as a mock/example server via `isExampleServerName()`, returning fake connections with "connection to example server not available"
331+
- **Use `127.0.0.1` not `localhost`**: `simple-ldap-go` treats "localhost" as a mock/example server
332+
via `isExampleServerName()`, returning fake connections with "connection to example server not
333+
available"
332334
- **Use `net.JoinHostPort`** not `fmt.Sprintf("%s:%d")` — the latter breaks with IPv6
333-
- **Use `net.Dialer`** with a context-aware `DialContext` instead of `net.DialTimeout` — keeps network code consistent with the `noctx` expectation of threading context through HTTP clients
335+
- **Use `net.Dialer`** with a context-aware `DialContext` instead of `net.DialTimeout` — keeps
336+
network code consistent with the `noctx` expectation of threading context through HTTP clients
334337
- **Seed data with `go-ldap/ldap/v3`** directly, not through `simple-ldap-go`
335338
- **CI config**: Port 1389, domain `test.local`, baseDN `dc=test,dc=local`, admin password `admin`
336339

@@ -351,7 +354,10 @@ func skipIfNoLDAP(t *testing.T) {
351354
2. **Configuration**: See `internal/options/options.go` for struct tags and flag definitions
352355
3. **Web handlers**: Review `internal/web/AGENTS.md` for HTTP patterns
353356
4. **Testing**: Look at existing `*_test.go` files for table-driven examples
354-
5. **LDAP integration tests**: See `internal/web/ldap_integration_test.go` for real LDAP patterns
357+
5. **LDAP integration tests**: See `internal/web/ldap_integration_test.go` for real LDAP patterns.
358+
For e2e tests (`internal/e2e/`): clean up seeded entries THROUGH the app, not via direct
359+
`ldapdelete` — the 30s cache keeps a ghost that poisons later tests (details in
360+
`internal/web/AGENTS.md`, "LDAP Integration Tests")
355361
6. **Dependencies**: Use `internal/` packages for shared code, avoid circular deps
356362
7. **Build issues**: Run `make clean && make setup && make build`
357363
8. **Test failures**: Run `make test` for coverage, `make test-race` for race conditions

internal/web/AGENTS.md

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -422,6 +422,12 @@ req := httptest.NewRequest("GET", "/users", nil)
422422
resp, err := app.fiber.Test(req)
423423
```
424424

425+
**`setupFullTestApp` registers routes WITHOUT the CSRF middleware** — a POST
426+
handler test passing here proves nothing about the token contract (this gap is
427+
how issue #652 shipped). To exercise the real chain (`RequireAuth` → csrf →
428+
cache middleware), use `setupCSRFBulkTestApp` in `bulk_csrf_test.go` as the
429+
pattern.
430+
425431
### Auth Session Testing
426432

427433
Create sessions via a separate mini Fiber app that writes session cookies:
@@ -457,12 +463,20 @@ func createAuthSession(t *testing.T, store *session.Store) []*http.Cookie {
457463

458464
Integration tests use a real OpenLDAP container (`osixia/openldap:1.5.0`):
459465

460-
- `skipIfNoLDAP(t)`: Check TCP connectivity, skip the whole test if unavailable — never let a test tolerate LDAP being down silently.
466+
- `skipIfNoLDAP(t)`: Check TCP connectivity, skip the whole test if unavailable — never let a test
467+
tolerate LDAP being down silently.
461468
- Use `go-ldap/ldap/v3` directly to seed test data (OUs, users, groups).
462469
- **IMPORTANT**: Use `127.0.0.1` not `localhost``simple-ldap-go` treats localhost as a mock server.
463470
- CI service container on port 1389, domain `test.local`, baseDN `dc=test,dc=local`.
464-
- Assert the **expected** outcome for each test — either success (with a valid seeded user) or a specific error (e.g., invalid credentials). Do not OR-pattern "success or error" — that hides regressions. If the environment is unavailable, the `skipIfNoLDAP` skip is the correct path.
471+
- Assert the **expected** outcome for each test — either success (with a valid seeded user) or a
472+
specific error (e.g., invalid credentials). Do not OR-pattern "success or error" — that hides
473+
regressions. If the environment is unavailable, the `skipIfNoLDAP` skip is the correct path.
465474
- Extract helper functions to avoid `dupl` linter violations in similar test patterns.
475+
- **e2e cleanup goes THROUGH the app, not behind its back.** The app caches the directory for 30s;
476+
a test that seeds an entry via `ldapadd` and deletes it via `ldapdelete` leaves a cache ghost that
477+
poisons later tests (a stale group in the addable datalist made adds fail silently — see
478+
`internal/e2e/bulk_toolbar_csrf_test.go`). Delete via the UI/handler so LDAP and cache update together; keep
479+
direct LDAP deletion only as a failure-path backstop that waits out one refresh cycle.
466480

467481
**Key test files:**
468482

@@ -488,7 +502,11 @@ Integration tests use a real OpenLDAP container (`osixia/openldap:1.5.0`):
488502
5. **Testing**: Check `server_test.go` for `setupFullTestApp`, `ldap_integration_test.go` for LDAP tests
489503
6. **Frontend**: `static/app.css` for styles, `static/js/v2-*.js` for plain JS; no build step
490504
7. **Assets out of date**: Run `make build-assets` (regenerates templ + refreshes `static/vendor/`)
491-
8. **CSRF errors**: Check `createCSRFConfig` in server.go for configuration
505+
8. **CSRF errors**: Check `createCSRFConfig` in server.go for configuration. A persistent 403
506+
"CSRF token validation failed" on a POST usually means the request carries no `csrf_token` at
507+
all — JS-built forms must read the per-session CSRF token from `data-csrf` on `main[data-bulk-scope]`
508+
(see `submitForm` in `static/js/v2-bulk.js`; issue #652). Server-rendered forms embed it as a
509+
hidden input.
492510
9. **LDAP mock issues**: If `simple-ldap-go` returns "example server" errors, use `127.0.0.1` not `localhost`
493511

494512
## House Rules

0 commit comments

Comments
 (0)