refactor: replace custom CSRF and rate-limiting with off-the-shelf libraries
All checks were successful
check / check (push) Successful in 4s
All checks were successful
check / check (push) Successful in 4s
Replace custom CSRF middleware with gorilla/csrf and custom rate-limiting middleware with go-chi/httprate, as requested in code review. CSRF changes: - Replace session-based CSRF tokens with gorilla/csrf cookie-based double-submit pattern (HMAC-authenticated cookies) - Keep same form field name (csrf_token) for template compatibility - Keep same route exclusions (webhook/API routes) - In dev mode, mark requests as plaintext HTTP to skip Referer check Rate limiting changes: - Replace custom token-bucket rate limiter with httprate sliding-window counter (per-IP, 5 POST requests/min on login endpoint) - Remove custom IP extraction (httprate.KeyByRealIP handles X-Forwarded-For, X-Real-IP, True-Client-IP) - Remove custom cleanup goroutine (httprate manages its own state) Kept as-is: - SSRF prevention code (internal/delivery/ssrf.go) — application-specific - CSRFToken() wrapper function — handlers unchanged Updated README security section and architecture overview to reflect library choices.
This commit is contained in:
@@ -27,20 +27,19 @@ func TestCSRF_GETSetsToken(t *testing.T) {
|
||||
handler.ServeHTTP(w, req)
|
||||
|
||||
assert.NotEmpty(t, gotToken, "CSRF token should be set in context on GET")
|
||||
assert.Len(t, gotToken, csrfTokenLength*2, "CSRF token should be hex-encoded 32 bytes")
|
||||
}
|
||||
|
||||
func TestCSRF_POSTWithValidToken(t *testing.T) {
|
||||
t.Parallel()
|
||||
m, _ := testMiddleware(t, config.EnvironmentDev)
|
||||
|
||||
// Use a separate handler for the GET to capture the token
|
||||
// Capture the token from a GET request
|
||||
var token string
|
||||
getHandler := m.CSRF()(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
|
||||
csrfMiddleware := m.CSRF()
|
||||
getHandler := csrfMiddleware(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
|
||||
token = CSRFToken(r)
|
||||
}))
|
||||
|
||||
// GET to establish the session and capture token
|
||||
getReq := httptest.NewRequest(http.MethodGet, "/form", nil)
|
||||
getW := httptest.NewRecorder()
|
||||
getHandler.ServeHTTP(getW, getReq)
|
||||
@@ -49,14 +48,13 @@ func TestCSRF_POSTWithValidToken(t *testing.T) {
|
||||
require.NotEmpty(t, cookies)
|
||||
require.NotEmpty(t, token)
|
||||
|
||||
// POST handler that tracks whether it was called
|
||||
// POST with valid token and cookies from the GET response
|
||||
var called bool
|
||||
postHandler := m.CSRF()(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {
|
||||
postHandler := csrfMiddleware(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {
|
||||
called = true
|
||||
}))
|
||||
|
||||
// POST with valid token
|
||||
form := url.Values{csrfFormField: {token}}
|
||||
form := url.Values{"csrf_token": {token}}
|
||||
postReq := httptest.NewRequest(http.MethodPost, "/form", strings.NewReader(form.Encode()))
|
||||
postReq.Header.Set("Content-Type", "application/x-www-form-urlencoded")
|
||||
for _, c := range cookies {
|
||||
@@ -73,23 +71,21 @@ func TestCSRF_POSTWithoutToken(t *testing.T) {
|
||||
t.Parallel()
|
||||
m, _ := testMiddleware(t, config.EnvironmentDev)
|
||||
|
||||
// GET handler to establish session
|
||||
getHandler := m.CSRF()(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {
|
||||
// no-op — just establishes session
|
||||
}))
|
||||
csrfMiddleware := m.CSRF()
|
||||
|
||||
// GET to establish the CSRF cookie
|
||||
getHandler := csrfMiddleware(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {}))
|
||||
getReq := httptest.NewRequest(http.MethodGet, "/form", nil)
|
||||
getW := httptest.NewRecorder()
|
||||
getHandler.ServeHTTP(getW, getReq)
|
||||
cookies := getW.Result().Cookies()
|
||||
|
||||
// POST handler that tracks whether it was called
|
||||
// POST without CSRF token
|
||||
var called bool
|
||||
postHandler := m.CSRF()(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {
|
||||
postHandler := csrfMiddleware(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {
|
||||
called = true
|
||||
}))
|
||||
|
||||
// POST without CSRF token
|
||||
postReq := httptest.NewRequest(http.MethodPost, "/form", nil)
|
||||
postReq.Header.Set("Content-Type", "application/x-www-form-urlencoded")
|
||||
for _, c := range cookies {
|
||||
@@ -107,24 +103,22 @@ func TestCSRF_POSTWithInvalidToken(t *testing.T) {
|
||||
t.Parallel()
|
||||
m, _ := testMiddleware(t, config.EnvironmentDev)
|
||||
|
||||
// GET handler to establish session
|
||||
getHandler := m.CSRF()(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {
|
||||
// no-op — just establishes session
|
||||
}))
|
||||
csrfMiddleware := m.CSRF()
|
||||
|
||||
// GET to establish the CSRF cookie
|
||||
getHandler := csrfMiddleware(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {}))
|
||||
getReq := httptest.NewRequest(http.MethodGet, "/form", nil)
|
||||
getW := httptest.NewRecorder()
|
||||
getHandler.ServeHTTP(getW, getReq)
|
||||
cookies := getW.Result().Cookies()
|
||||
|
||||
// POST handler that tracks whether it was called
|
||||
// POST with wrong CSRF token
|
||||
var called bool
|
||||
postHandler := m.CSRF()(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {
|
||||
postHandler := csrfMiddleware(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {
|
||||
called = true
|
||||
}))
|
||||
|
||||
// POST with wrong CSRF token
|
||||
form := url.Values{csrfFormField: {"invalid-token-value"}}
|
||||
form := url.Values{"csrf_token": {"invalid-token-value"}}
|
||||
postReq := httptest.NewRequest(http.MethodPost, "/form", strings.NewReader(form.Encode()))
|
||||
postReq.Header.Set("Content-Type", "application/x-www-form-urlencoded")
|
||||
for _, c := range cookies {
|
||||
@@ -156,20 +150,8 @@ func TestCSRF_GETDoesNotValidate(t *testing.T) {
|
||||
assert.True(t, called, "GET requests should pass through CSRF middleware")
|
||||
}
|
||||
|
||||
func TestCSRFToken_NoContext(t *testing.T) {
|
||||
func TestCSRFToken_NoMiddleware(t *testing.T) {
|
||||
t.Parallel()
|
||||
req := httptest.NewRequest(http.MethodGet, "/", nil)
|
||||
assert.Empty(t, CSRFToken(req), "CSRFToken should return empty string when no token in context")
|
||||
}
|
||||
|
||||
func TestGenerateCSRFToken(t *testing.T) {
|
||||
t.Parallel()
|
||||
token, err := generateCSRFToken()
|
||||
require.NoError(t, err)
|
||||
assert.Len(t, token, csrfTokenLength*2, "token should be hex-encoded")
|
||||
|
||||
// Verify uniqueness
|
||||
token2, err := generateCSRFToken()
|
||||
require.NoError(t, err)
|
||||
assert.NotEqual(t, token, token2, "each generated token should be unique")
|
||||
assert.Empty(t, CSRFToken(req), "CSRFToken should return empty string when middleware has not run")
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user