Skip to content

Commit 5a1b284

Browse files
authored
fix(security): use session-based CSRF storage for persistence (#392)
## Summary - **Root cause fix**: CSRF middleware was using its own internal in-memory storage, causing tokens to fail validation - The v1.1.2 fix corrected the expiration duration but didn't address the underlying storage issue - Now uses session-based CSRF storage which persists tokens in the session store (BBolt when `PersistSessions=true`) ## Root Cause Analysis The CSRF middleware was configured without a `Session` parameter, which caused it to use its own internal `memory.New()` storage. This meant: 1. CSRF tokens were stored in a separate in-memory storage from the session store 2. Tokens were lost on container restart 3. The token lookup failed with "csrf token not found" errors ## Changes 1. **`internal/web/server.go`**: Pass `sessionStore` to `createCSRFConfig()` and configure CSRF with `Session` and `SessionKey` parameters 2. **`internal/web/cookie_security_test.go`**: Update tests for session-based CSRF (requires session cookie, not separate csrf_ cookie) ## Test plan - [x] All existing CSRF tests pass - [x] Full test suite passes - [x] Linter passes with 0 issues - [ ] Test on production after deployment 🤖 Generated with [Claude Code](https://claude.com/claude-code)
2 parents 323b91f + 88dc5d0 commit 5a1b284

File tree

2 files changed

+37
-28
lines changed

2 files changed

+37
-28
lines changed

internal/web/cookie_security_test.go

Lines changed: 30 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ func TestCookieSecurityWithHTTPS(t *testing.T) {
5050
}
5151

5252
// Test CSRF handler creation doesn't panic
53-
csrfHandler := createCSRFConfig(opts)
53+
csrfHandler := createCSRFConfig(opts, sessionStore)
5454
if csrfHandler == nil {
5555
t.Fatal("Expected CSRF handler, got nil")
5656
}
@@ -89,7 +89,7 @@ func TestCookieSecurityWithHTTP(t *testing.T) {
8989
}
9090

9191
// Test CSRF handler creation doesn't panic
92-
csrfHandler := createCSRFConfig(opts)
92+
csrfHandler := createCSRFConfig(opts, sessionStore)
9393
if csrfHandler == nil {
9494
t.Fatal("Expected CSRF handler, got nil")
9595
}
@@ -147,15 +147,15 @@ func TestCookieSecureConfiguration(t *testing.T) {
147147
}
148148

149149
// Test CSRF handler creation with configuration
150-
csrfHandler := createCSRFConfig(opts)
150+
csrfHandler := createCSRFConfig(opts, sessionStore)
151151
if csrfHandler == nil {
152152
t.Fatal("Expected CSRF handler, got nil")
153153
}
154154
})
155155
}
156156
}
157157

158-
// TestCSRFConfigurationAcceptsOpts verifies CSRF handler accepts options parameter
158+
// TestCSRFConfigurationAcceptsOpts verifies CSRF handler accepts options and session store parameters
159159
func TestCSRFConfigurationAcceptsOpts(t *testing.T) {
160160
opts := &options.Opts{
161161
LDAP: ldap.Config{
@@ -176,14 +176,20 @@ func TestCSRFConfigurationAcceptsOpts(t *testing.T) {
176176
PoolAcquireTimeout: 10 * time.Second,
177177
}
178178

179-
// Verify CSRF handler creation accepts opts parameter (fixes the signature change)
180-
csrfHandler := createCSRFConfig(opts)
179+
// Create session store for CSRF middleware
180+
sessionStore := createSessionStore(opts)
181+
if sessionStore == nil {
182+
t.Fatal("Expected session store, got nil")
183+
}
184+
185+
// Verify CSRF handler creation accepts opts and sessionStore parameters
186+
csrfHandler := createCSRFConfig(opts, sessionStore)
181187
if csrfHandler == nil {
182188
t.Fatal("Expected CSRF handler, got nil")
183189
}
184190

185191
// Handler created successfully - type is fiber.Handler (internal Fiber type)
186-
t.Log("CSRF handler created successfully with opts parameter")
192+
t.Log("CSRF handler created successfully with opts and sessionStore parameters")
187193
}
188194

189195
// TestCSRFTokenValidation verifies that CSRF tokens are properly validated on POST requests.
@@ -210,13 +216,15 @@ func TestCSRFTokenValidation(t *testing.T) {
210216
PoolAcquireTimeout: 10 * time.Second,
211217
}
212218

213-
// Create a test Fiber app with CSRF middleware
214-
f := fiber.New()
215-
csrfHandler := createCSRFConfig(opts)
219+
// Create session store first for CSRF middleware
216220
sessionStore := session.New(session.Config{
217221
Storage: memory.New(),
218222
})
219223

224+
// Create a test Fiber app with CSRF middleware
225+
f := fiber.New()
226+
csrfHandler := createCSRFConfig(opts, sessionStore)
227+
220228
// Test endpoint that returns CSRF token on GET and validates on POST
221229
f.All("/test-csrf", *csrfHandler, func(c *fiber.Ctx) error {
222230
sess, err := sessionStore.Get(c)
@@ -293,7 +301,8 @@ func TestCSRFTokenValidation(t *testing.T) {
293301
})
294302

295303
t.Run("POST with valid CSRF token succeeds", func(t *testing.T) {
296-
// Step 1: GET to obtain CSRF token and cookie
304+
// Step 1: GET to obtain CSRF token and session cookie
305+
// With session-based CSRF, the token is stored in the session
297306
getReq := httptest.NewRequest("GET", "/test-csrf", nil)
298307
getResp, err := f.Test(getReq)
299308
if err != nil {
@@ -313,25 +322,21 @@ func TestCSRFTokenValidation(t *testing.T) {
313322
}
314323
csrfToken := tokenMatch[1]
315324

316-
// Extract CSRF cookie
317-
var csrfCookie *http.Cookie
318-
for _, cookie := range getResp.Cookies() {
319-
if strings.HasPrefix(cookie.Name, "csrf_") {
320-
csrfCookie = cookie
321-
322-
break
323-
}
324-
}
325-
326-
if csrfCookie == nil {
327-
t.Fatal("CSRF cookie not found in response")
325+
// Extract all cookies (session cookie contains the CSRF token with session-based CSRF)
326+
cookies := getResp.Cookies()
327+
if len(cookies) == 0 {
328+
t.Fatal("No cookies found in response (session cookie required for session-based CSRF)")
328329
}
329330

330-
// Step 2: POST with valid CSRF token and cookie
331+
// Step 2: POST with valid CSRF token and all cookies (including session cookie)
331332
postReq := httptest.NewRequest("POST", "/test-csrf",
332333
strings.NewReader("csrf_token="+csrfToken+"&data=test"))
333334
postReq.Header.Set("Content-Type", "application/x-www-form-urlencoded")
334-
postReq.AddCookie(csrfCookie)
335+
336+
// Add all cookies from GET response (session-based CSRF needs the session cookie)
337+
for _, cookie := range cookies {
338+
postReq.AddCookie(cookie)
339+
}
335340

336341
postResp, err := f.Test(postReq)
337342
if err != nil {

internal/web/server.go

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ func NewApp(opts *options.Opts) (*App, error) {
119119
sessionStore := createSessionStore(opts)
120120
templateCache := NewTemplateCache(DefaultTemplateCacheConfig())
121121
f := createFiberApp()
122-
csrfHandler := *createCSRFConfig(opts)
122+
csrfHandler := *createCSRFConfig(opts, sessionStore)
123123

124124
// Load asset manifest for cache-busted files
125125
manifestPath := "internal/web/static/manifest.json"
@@ -181,7 +181,9 @@ func setupMiddleware(f *fiber.App) {
181181
}
182182

183183
// createCSRFConfig creates and returns CSRF middleware configuration
184-
func createCSRFConfig(opts *options.Opts) *fiber.Handler {
184+
// Uses session-based storage to ensure CSRF tokens persist across requests
185+
// and survive container restarts when PersistSessions is enabled.
186+
func createCSRFConfig(opts *options.Opts, sessionStore *session.Store) *fiber.Handler {
185187
csrfHandler := csrf.New(csrf.Config{
186188
KeyLookup: "form:csrf_token",
187189
CookieName: "csrf_",
@@ -190,7 +192,9 @@ func createCSRFConfig(opts *options.Opts) *fiber.Handler {
190192
CookieHTTPOnly: true,
191193
Expiration: time.Hour,
192194
KeyGenerator: csrf.ConfigDefault.KeyGenerator,
193-
ContextKey: "token", // Store token in c.Locals("token") for template access
195+
Session: sessionStore, // Use session-based CSRF storage for persistence
196+
SessionKey: "csrf_token", // Key to store CSRF token in session
197+
ContextKey: "token", // Store token in c.Locals("token") for template access
194198
ErrorHandler: func(c *fiber.Ctx, err error) error {
195199
log.Warn().Err(err).Msg("CSRF validation failed")
196200
c.Status(fiber.StatusForbidden)

0 commit comments

Comments
 (0)