Skip to content

Commit 915f690

Browse files
EmyrkEmyrk
authored
chore: fix csrf error message on empty session header (coder#14018)
* chore: fix csrf error message on empty session header A more detailed error message was added to catch mismatched session tokens. This error was mistakenly applying to all CSRF failures.
1 parent 2279441 commit 915f690

File tree

2 files changed

+78
-1
lines changed

2 files changed

+78
-1
lines changed

‎coderd/httpmw/csrf.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,9 @@ func CSRF(secureCookie bool) func(next http.Handler) http.Handler {
2222
mw.SetBaseCookie(http.Cookie{Path: "/", HttpOnly: true, SameSite: http.SameSiteLaxMode, Secure: secureCookie})
2323
mw.SetFailureHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
2424
sessCookie, err := r.Cookie(codersdk.SessionTokenCookie)
25-
if err == nil && r.Header.Get(codersdk.SessionTokenHeader) != sessCookie.Value {
25+
if err == nil &&
26+
r.Header.Get(codersdk.SessionTokenHeader) != "" &&
27+
r.Header.Get(codersdk.SessionTokenHeader) != sessCookie.Value {
2628
// If a user is using header authentication and cookie auth, but the values
2729
// do not match, the cookie value takes priority.
2830
// At the very least, return a more helpful error to the user.

‎coderd/httpmw/csrf_test.go

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package httpmw_test
33
import (
44
"context"
55
"net/http"
6+
"net/http/httptest"
67
"testing"
78

89
"github.com/justinas/nosurf"
@@ -69,3 +70,77 @@ func TestCSRFExemptList(t *testing.T) {
6970
})
7071
}
7172
}
73+
74+
// TestCSRFError verifies the error message returned to a user when CSRF
75+
// checks fail.
76+
//
77+
//nolint:bodyclose // Using httptest.Recorders
78+
func TestCSRFError(t *testing.T) {
79+
t.Parallel()
80+
81+
// Hard coded matching CSRF values
82+
const csrfCookieValue = "JXm9hOUdZctWt0ZZGAy9xiS/gxMKYOThdxjjMnMUyn4="
83+
const csrfHeaderValue = "KNKvagCBEHZK7ihe2t7fj6VeJ0UyTDco1yVUJE8N06oNqxLu5Zx1vRxZbgfC0mJJgeGkVjgs08mgPbcWPBkZ1A=="
84+
// Use a url with "/api" as the root, other routes bypass CSRF.
85+
const urlPath = "https://coder.com/api/v2/hello"
86+
87+
var handler http.Handler = http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) {
88+
writer.WriteHeader(http.StatusOK)
89+
})
90+
handler = httpmw.CSRF(false)(handler)
91+
92+
// Not testing the error case, just providing the example of things working
93+
// to base the failure tests off of.
94+
t.Run("ValidCSRF", func(t *testing.T) {
95+
t.Parallel()
96+
97+
req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, urlPath, nil)
98+
require.NoError(t, err)
99+
100+
req.AddCookie(&http.Cookie{Name: codersdk.SessionTokenCookie, Value: "session_token_value"})
101+
req.AddCookie(&http.Cookie{Name: nosurf.CookieName, Value: csrfCookieValue})
102+
req.Header.Add(nosurf.HeaderName, csrfHeaderValue)
103+
104+
rec := httptest.NewRecorder()
105+
handler.ServeHTTP(rec, req)
106+
resp := rec.Result()
107+
require.Equal(t, http.StatusOK, resp.StatusCode)
108+
})
109+
110+
// The classic CSRF failure returns the generic error.
111+
t.Run("MissingCSRFHeader", func(t *testing.T) {
112+
t.Parallel()
113+
114+
req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, urlPath, nil)
115+
require.NoError(t, err)
116+
117+
req.AddCookie(&http.Cookie{Name: codersdk.SessionTokenCookie, Value: "session_token_value"})
118+
req.AddCookie(&http.Cookie{Name: nosurf.CookieName, Value: csrfCookieValue})
119+
120+
rec := httptest.NewRecorder()
121+
handler.ServeHTTP(rec, req)
122+
resp := rec.Result()
123+
require.Equal(t, http.StatusBadRequest, resp.StatusCode)
124+
require.Contains(t, rec.Body.String(), "Something is wrong with your CSRF token.")
125+
})
126+
127+
// Include the CSRF cookie, but not the CSRF header value.
128+
// Including the 'codersdk.SessionTokenHeader' will bypass CSRF only if
129+
// it matches the cookie. If it does not, we expect a more helpful error.
130+
t.Run("MismatchedHeaderAndCookie", func(t *testing.T) {
131+
t.Parallel()
132+
133+
req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, urlPath, nil)
134+
require.NoError(t, err)
135+
136+
req.AddCookie(&http.Cookie{Name: codersdk.SessionTokenCookie, Value: "session_token_value"})
137+
req.AddCookie(&http.Cookie{Name: nosurf.CookieName, Value: csrfCookieValue})
138+
req.Header.Add(codersdk.SessionTokenHeader, "mismatched_value")
139+
140+
rec := httptest.NewRecorder()
141+
handler.ServeHTTP(rec, req)
142+
resp := rec.Result()
143+
require.Equal(t, http.StatusBadRequest, resp.StatusCode)
144+
require.Contains(t, rec.Body.String(), "CSRF error encountered. Authentication via")
145+
})
146+
}

0 commit comments

Comments
 (0)