From cbf8d8e22f43e535e7ae01f57638e2e5af41a390 Mon Sep 17 00:00:00 2001 From: Florian Loch Date: Mon, 1 Feb 2021 12:22:58 +0100 Subject: [PATCH 1/4] Fix wrong error being reported when token missing in request --- csrf.go | 19 +++++++++++++++++-- csrf_test.go | 42 ++++++++++++++++++++++++++++++++++++++++++ helpers.go | 11 ++++++++--- 3 files changed, 67 insertions(+), 5 deletions(-) diff --git a/csrf.go b/csrf.go index f21e0a2..41b5aee 100644 --- a/csrf.go +++ b/csrf.go @@ -282,8 +282,23 @@ func (cs *csrf) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - // Retrieve the combined token (pad + masked) token and unmask it. - requestToken := unmask(cs.requestToken(r)) + // Retrieve the combined token (pad + masked) token... + maskedToken, err := cs.requestToken(r) + + if err != nil { + r = envError(r, ErrBadToken) + cs.opts.ErrorHandler.ServeHTTP(w, r) + return + } + + if maskedToken == nil { + r = envError(r, ErrNoToken) + cs.opts.ErrorHandler.ServeHTTP(w, r) + return + } + + // ... and unmask it. + requestToken := unmask(maskedToken) // Compare the request token against the real token if !compareTokens(requestToken, realToken) { diff --git a/csrf_test.go b/csrf_test.go index 0841d5f..a97fb10 100644 --- a/csrf_test.go +++ b/csrf_test.go @@ -374,6 +374,48 @@ func TestWithReferer(t *testing.T) { } } +// Requests without a token should fail with ErrNoToken. +func TestNoTokenProvided(t *testing.T) { + var finalErr error + + s := http.NewServeMux() + p := Protect(testKey, ErrorHandler(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + finalErr = FailureReason(r) + })))(s) + + var token string + s.Handle("/", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + token = Token(r) + })) + + // Obtain a CSRF cookie via a GET request. + r, err := http.NewRequest("GET", "http://www.gorillatoolkit.org/", nil) + if err != nil { + t.Fatal(err) + } + + rr := httptest.NewRecorder() + p.ServeHTTP(rr, r) + + // POST the token back in the header. + r, err = http.NewRequest("POST", "http://www.gorillatoolkit.org/", nil) + if err != nil { + t.Fatal(err) + } + + setCookie(rr, r) + // By accident we use the wrong header name for the token... + r.Header.Set("X-CSRF-nekot", token) + r.Header.Set("Referer", "http://www.gorillatoolkit.org/") + + rr = httptest.NewRecorder() + p.ServeHTTP(rr, r) + + if finalErr != nil && finalErr != ErrNoToken { + t.Fatalf("middleware failed to return correct error: got '%v' want '%v'", finalErr, ErrNoToken) + } +} + func setCookie(rr *httptest.ResponseRecorder, r *http.Request) { r.Header.Set("Cookie", rr.Header().Get("Set-Cookie")) } diff --git a/helpers.go b/helpers.go index 3dacfd2..c19dc47 100644 --- a/helpers.go +++ b/helpers.go @@ -105,7 +105,7 @@ func unmask(issued []byte) []byte { // requestToken returns the issued token (pad + masked token) from the HTTP POST // body or HTTP header. It will return nil if the token fails to decode. -func (cs *csrf) requestToken(r *http.Request) []byte { +func (cs *csrf) requestToken(r *http.Request) ([]byte, error) { // 1. Check the HTTP header first. issued := r.Header.Get(cs.opts.RequestHeader) @@ -123,14 +123,19 @@ func (cs *csrf) requestToken(r *http.Request) []byte { } } + // Return nil (equivalent to empty byte slice) if no token was found + if issued == "" { + return nil, nil + } + // Decode the "issued" (pad + masked) token sent in the request. Return a // nil byte slice on a decoding error (this will fail upstream). decoded, err := base64.StdEncoding.DecodeString(issued) if err != nil { - return nil + return nil, err } - return decoded + return decoded, nil } // generateRandomBytes returns securely generated random bytes. From b7383bd21dd55ed5ec81ff2d9acc5c17bde7c7e8 Mon Sep 17 00:00:00 2001 From: Florian Loch Date: Mon, 1 Feb 2021 12:41:39 +0100 Subject: [PATCH 2/4] Remove a condition that never becomes true --- csrf.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/csrf.go b/csrf.go index 41b5aee..d3ee77f 100644 --- a/csrf.go +++ b/csrf.go @@ -274,14 +274,6 @@ func (cs *csrf) ServeHTTP(w http.ResponseWriter, r *http.Request) { } } - // If the token returned from the session store is nil for non-idempotent - // ("unsafe") methods, call the error handler. - if realToken == nil { - r = envError(r, ErrNoToken) - cs.opts.ErrorHandler.ServeHTTP(w, r) - return - } - // Retrieve the combined token (pad + masked) token... maskedToken, err := cs.requestToken(r) From 8f705b8afb46dbc9cf273549918a7a5ab2e32e0c Mon Sep 17 00:00:00 2001 From: Florian Loch Date: Mon, 1 Feb 2021 12:48:51 +0100 Subject: [PATCH 3/4] Add myself to the list of AUTHORS assuming this is good style --- AUTHORS | 1 + 1 file changed, 1 insertion(+) diff --git a/AUTHORS b/AUTHORS index 4e84c37..e8d6043 100644 --- a/AUTHORS +++ b/AUTHORS @@ -2,6 +2,7 @@ # Please keep the list sorted. adiabatic +Florian D. Loch Google LLC (https://opensource.google.com) jamesgroat Joshua Carp From 3a2922cbdd9f684879ceadbfc18f91b7a5f9eb98 Mon Sep 17 00:00:00 2001 From: Florian Loch Date: Sun, 11 Apr 2021 17:49:38 +0200 Subject: [PATCH 4/4] Fix minor style issue --- csrf.go | 1 - 1 file changed, 1 deletion(-) diff --git a/csrf.go b/csrf.go index d3ee77f..21223d4 100644 --- a/csrf.go +++ b/csrf.go @@ -276,7 +276,6 @@ func (cs *csrf) ServeHTTP(w http.ResponseWriter, r *http.Request) { // Retrieve the combined token (pad + masked) token... maskedToken, err := cs.requestToken(r) - if err != nil { r = envError(r, ErrBadToken) cs.opts.ErrorHandler.ServeHTTP(w, r)