From b6a8f8027e491a94fe91372388f8bb9ad6daf2fd Mon Sep 17 00:00:00 2001 From: Jonas Hungershausen Date: Tue, 31 Dec 2024 14:19:12 +0100 Subject: [PATCH] fix: properly setup CSRF errors --- driver/registry_default.go | 2 +- .../strategy/idfirst/strategy_login_test.go | 3 +- selfservice/strategy/profile/strategy_test.go | 121 +++++++------ x/.snapshots/TestSnapshotCsrfErrors.json | 162 ++++++++++++++++++ x/nosurf.go | 82 +++++---- x/nosurf_test.go | 85 ++++++--- 6 files changed, 337 insertions(+), 118 deletions(-) create mode 100644 x/.snapshots/TestSnapshotCsrfErrors.json diff --git a/driver/registry_default.go b/driver/registry_default.go index 464f7881f626..0c455e6fcc92 100644 --- a/driver/registry_default.go +++ b/driver/registry_default.go @@ -826,7 +826,7 @@ func (m *RegistryDefault) WithCSRFTokenGenerator(cg x.CSRFToken) { func (m *RegistryDefault) GenerateCSRFToken(r *http.Request) string { if m.csrfTokenGenerator == nil { - m.csrfTokenGenerator = x.DefaultCSRFToken + m.csrfTokenGenerator = x.DefaultCSRFTokenGenerator } return m.csrfTokenGenerator(r) } diff --git a/selfservice/strategy/idfirst/strategy_login_test.go b/selfservice/strategy/idfirst/strategy_login_test.go index d8c31bdeb786..e5056f1b45ba 100644 --- a/selfservice/strategy/idfirst/strategy_login_test.go +++ b/selfservice/strategy/idfirst/strategy_login_test.go @@ -236,8 +236,7 @@ func TestCompleteLogin(t *testing.T) { actual, res := testhelpers.LoginMakeRequest(t, false, false, f, browserClient, values.Encode()) assert.EqualValues(t, http.StatusOK, res.StatusCode) - assertx.EqualAsJSON(t, x.ErrInvalidCSRFToken, - json.RawMessage(actual), "%s", actual) + assertx.EqualAsJSON(t, x.ErrInvalidCSRFTokenServerNoCookies, json.RawMessage(actual), "%s", actual) }) t.Run("case=should fail because of missing CSRF token/type=spa", func(t *testing.T) { diff --git a/selfservice/strategy/profile/strategy_test.go b/selfservice/strategy/profile/strategy_test.go index be9b448a0215..ef0a825a993c 100644 --- a/selfservice/strategy/profile/strategy_test.go +++ b/selfservice/strategy/profile/strategy_test.go @@ -85,7 +85,8 @@ func TestStrategyTraits(t *testing.T) { ui := testhelpers.NewSettingsUIEchoServer(t, reg) _ = testhelpers.NewErrorTestServer(t, reg) - publicTS, _ := testhelpers.NewKratosServer(t, reg) + publicRouter := x.NewRouterPublic() + publicTS, _ := testhelpers.NewKratosServerWithRouters(t, reg, publicRouter, x.NewRouterAdmin()) browserIdentity1 := newIdentityWithPassword("john-browser@doe.com") apiIdentity1 := newIdentityWithPassword("john-api@doe.com") @@ -122,75 +123,85 @@ func TestStrategyTraits(t *testing.T) { }) }) - t.Run("description=should fail to post data if CSRF is invalid/type=browser", func(t *testing.T) { - setUnprivileged(t) + t.Run("csrf issues", func(t *testing.T) { + reg.WithCSRFHandler(x.NewCSRFHandler(publicRouter, reg)) + reg.WithCSRFTokenGenerator(x.DefaultCSRFTokenGenerator) - f := testhelpers.InitializeSettingsFlowViaBrowser(t, browserUser1, false, publicTS) + t.Cleanup(func() { + reg.WithCSRFHandler(new(x.FakeCSRFHandler)) + reg.WithCSRFTokenGenerator(x.FakeCSRFTokenGenerator) + }) - actual, res := testhelpers.SettingsMakeRequest(t, false, false, f, browserUser1, - url.Values{"traits.booly": {"true"}, "csrf_token": {"invalid"}, "method": {"profile"}}.Encode()) - assert.EqualValues(t, http.StatusOK, res.StatusCode, "should return a 400 error because CSRF token is not set\n\t%s", actual) - assertx.EqualAsJSON(t, x.ErrInvalidCSRFToken, json.RawMessage(actual), "%s", actual) - }) + t.Run("description=should fail to post data if CSRF is invalid/type=browser", func(t *testing.T) { + setUnprivileged(t) - t.Run("description=should fail to post data if CSRF is invalid/type=spa", func(t *testing.T) { - setUnprivileged(t) + f := testhelpers.InitializeSettingsFlowViaBrowser(t, browserUser1, false, publicTS) - f := testhelpers.InitializeSettingsFlowViaBrowser(t, browserUser1, true, publicTS) + actual, res := testhelpers.SettingsMakeRequest(t, false, false, f, browserUser1, + url.Values{"traits.booly": {"true"}, "csrf_token": {"invalid"}, "method": {"profile"}}.Encode()) + assert.EqualValues(t, http.StatusOK, res.StatusCode, "should return a 400 error because CSRF token is not set\n\t%s", actual) + assertx.EqualAsJSON(t, x.ErrInvalidCSRFToken, json.RawMessage(actual), "%s", actual) + }) - actual, res := testhelpers.SettingsMakeRequest(t, false, true, f, browserUser1, - testhelpers.EncodeFormAsJSON(t, true, url.Values{"traits.booly": {"true"}, "csrf_token": {"invalid"}, "method": {"profile"}})) - assert.EqualValues(t, http.StatusForbidden, res.StatusCode, "should return a 400 error because CSRF token is not set\n\t%s", actual) - assertx.EqualAsJSON(t, x.ErrInvalidCSRFToken, json.RawMessage(gjson.Get(actual, "error").Raw), "%s", actual) - }) + t.Run("description=should fail to post data if CSRF is invalid/type=spa", func(t *testing.T) { + setUnprivileged(t) - t.Run("description=should not fail because of CSRF token but because of unprivileged/type=api", func(t *testing.T) { - setUnprivileged(t) + f := testhelpers.InitializeSettingsFlowViaBrowser(t, browserUser1, true, publicTS) - f := testhelpers.InitializeSettingsFlowViaAPI(t, apiUser1, publicTS) + actual, res := testhelpers.SettingsMakeRequest(t, false, true, f, browserUser1, + testhelpers.EncodeFormAsJSON(t, true, url.Values{"traits.booly": {"true"}, "csrf_token": {"invalid"}, "method": {"profile"}})) + assert.EqualValues(t, http.StatusForbidden, res.StatusCode, "should return a 400 error because CSRF token is not set\n\t%s", actual) + assertx.EqualAsJSON(t, x.ErrInvalidCSRFToken, json.RawMessage(gjson.Get(actual, "error").Raw), "%s", actual) + }) - actual, res := testhelpers.SettingsMakeRequest(t, true, false, f, apiUser1, `{"traits.booly":true,"method":"profile","csrf_token":"`+x.FakeCSRFToken+`"}`) - require.Len(t, res.Cookies(), 1) - assert.Equal(t, "ory_kratos_continuity", res.Cookies()[0].Name) - assert.EqualValues(t, http.StatusForbidden, res.StatusCode) - assert.Contains(t, gjson.Get(actual, "error.reason").String(), "login session is too old", actual) - }) + t.Run("description=should not fail because of CSRF token but because of unprivileged/type=api", func(t *testing.T) { + setUnprivileged(t) - t.Run("case=should fail with correct CSRF error cause/type=api", func(t *testing.T) { - setPrivileged(t) + f := testhelpers.InitializeSettingsFlowViaAPI(t, apiUser1, publicTS) + + actual, res := testhelpers.SettingsMakeRequest(t, true, false, f, apiUser1, `{"traits.booly":true,"method":"profile","csrf_token":"`+x.FakeCSRFToken+`"}`) + require.Len(t, res.Cookies(), 1) + assert.Equal(t, "ory_kratos_continuity", res.Cookies()[0].Name) + assert.EqualValues(t, http.StatusForbidden, res.StatusCode) + assert.Contains(t, gjson.Get(actual, "error.reason").String(), "login session is too old", actual) + }) - for k, tc := range []struct { - mod func(http.Header) - exp string - }{ - { - mod: func(h http.Header) { - h.Add("Cookie", "name=bar") + t.Run("case=should fail with correct CSRF error cause/type=api", func(t *testing.T) { + setPrivileged(t) + + for k, tc := range []struct { + mod func(http.Header) + exp string + }{ + { + mod: func(h http.Header) { + h.Add("Cookie", "name=bar") + }, + exp: "The HTTP Request Header included the \\\"Cookie\\\" key", }, - exp: "The HTTP Request Header included the \\\"Cookie\\\" key", - }, - { - mod: func(h http.Header) { - h.Add("Origin", "www.bar.com") + { + mod: func(h http.Header) { + h.Add("Origin", "www.bar.com") + }, + exp: "The HTTP Request Header included the \\\"Origin\\\" key", }, - exp: "The HTTP Request Header included the \\\"Origin\\\" key", - }, - } { - t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) { - f := testhelpers.InitializeSettingsFlowViaAPI(t, apiUser1, publicTS) + } { + t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) { + f := testhelpers.InitializeSettingsFlowViaAPI(t, apiUser1, publicTS) - req := testhelpers.NewRequest(t, true, "POST", f.Ui.Action, bytes.NewBufferString(`{"traits.booly":true,"method":"profile","csrf_token":"invalid"}`)) - tc.mod(req.Header) + req := testhelpers.NewRequest(t, true, "POST", f.Ui.Action, bytes.NewBufferString(`{"traits.booly":true,"method":"profile","csrf_token":"invalid"}`)) + tc.mod(req.Header) - res, err := apiUser1.Do(req) - require.NoError(t, err) - defer res.Body.Close() + res, err := apiUser1.Do(req) + require.NoError(t, err) + defer res.Body.Close() - actual := string(ioutilx.MustReadAll(res.Body)) - assert.EqualValues(t, http.StatusBadRequest, res.StatusCode) - assert.Contains(t, actual, tc.exp, "%s", actual) - }) - } + actual := string(ioutilx.MustReadAll(res.Body)) + assert.EqualValues(t, http.StatusBadRequest, res.StatusCode) + assert.Contains(t, actual, tc.exp, "%s", actual) + }) + } + }) }) t.Run("description=hydrate the proper fields", func(t *testing.T) { diff --git a/x/.snapshots/TestSnapshotCsrfErrors.json b/x/.snapshots/TestSnapshotCsrfErrors.json new file mode 100644 index 000000000000..f433965241af --- /dev/null +++ b/x/.snapshots/TestSnapshotCsrfErrors.json @@ -0,0 +1,162 @@ +[ + { + "Id": "ErrInvalidCSRFToken", + "Err": { + "id": "security_csrf_violation", + "code": 403, + "status": "Forbidden", + "reason": "Please retry the flow and optionally clear your cookies. The request was rejected to protect you from Cross-Site-Request-Forgery (CSRF) which could cause account takeover, leaking personal information, and other serious security issues.", + "details": { + "docs": "https://www.ory.sh/docs/kratos/debug/csrf", + "hint": "" + }, + "message": "The request was rejected to protect you from Cross-Site-Request-Forgery (CSRF) which could cause account takeover, leaking personal information, and other serious security issues." + } + }, + { + "Id": "ErrInvalidCSRFTokenAJAX", + "Err": { + "id": "security_csrf_violation", + "code": 403, + "status": "Forbidden", + "reason": "Please retry the flow and optionally clear your cookies. The request was rejected to protect you from Cross-Site-Request-Forgery (CSRF) which could cause account takeover, leaking personal information, and other serious security issues.", + "details": { + "docs": "https://www.ory.sh/docs/kratos/debug/csrf", + "hint": "We detected an AJAX call, please ensure that CORS is enabled and configured correctly, and that your AJAX code sends cookies and has credentials enabled. For further debugging, check your browser's network tab to see what cookies are included or excluded." + }, + "message": "The request was rejected to protect you from Cross-Site-Request-Forgery (CSRF) which could cause account takeover, leaking personal information, and other serious security issues." + } + }, + { + "Id": "ErrInvalidCSRFTokenAJAXCookieMissing", + "Err": { + "id": "security_csrf_violation", + "code": 403, + "status": "Forbidden", + "reason": "Please retry the flow and optionally clear your cookies. The request was rejected to protect you from Cross-Site-Request-Forgery (CSRF) which could cause account takeover, leaking personal information, and other serious security issues.", + "details": { + "docs": "https://www.ory.sh/docs/kratos/debug/csrf", + "hint": "We detected an AJAX call, please ensure that CORS is enabled and configured correctly, and that your AJAX code sends cookies and has credentials enabled. For further debugging, check your browser's network tab to see what cookies are included or excluded.", + "reject_reason": "The HTTP cookie header was set but did not include the anti-CSRF cookie." + }, + "message": "The request was rejected to protect you from Cross-Site-Request-Forgery (CSRF) which could cause account takeover, leaking personal information, and other serious security issues." + } + }, + { + "Id": "ErrInvalidCSRFTokenAJAXNoCookies", + "Err": { + "id": "security_csrf_violation", + "code": 403, + "status": "Forbidden", + "reason": "Please retry the flow and optionally clear your cookies. The request was rejected to protect you from Cross-Site-Request-Forgery (CSRF) which could cause account takeover, leaking personal information, and other serious security issues.", + "details": { + "docs": "https://www.ory.sh/docs/kratos/debug/csrf", + "hint": "We detected an AJAX call, please ensure that CORS is enabled and configured correctly, and that your AJAX code sends cookies and has credentials enabled. For further debugging, check your browser's network tab to see what cookies are included or excluded.", + "reject_reason": "The HTTP cookie header is empty or not set." + }, + "message": "The request was rejected to protect you from Cross-Site-Request-Forgery (CSRF) which could cause account takeover, leaking personal information, and other serious security issues." + } + }, + { + "Id": "ErrInvalidCSRFTokenAJAXTokenMismatch", + "Err": { + "id": "security_csrf_violation", + "code": 403, + "status": "Forbidden", + "reason": "Please retry the flow and optionally clear your cookies. The request was rejected to protect you from Cross-Site-Request-Forgery (CSRF) which could cause account takeover, leaking personal information, and other serious security issues.", + "details": { + "docs": "https://www.ory.sh/docs/kratos/debug/csrf", + "hint": "We detected an AJAX call, please ensure that CORS is enabled and configured correctly, and that your AJAX code sends cookies and has credentials enabled. For further debugging, check your browser's network tab to see what cookies are included or excluded.", + "reject_reason": "The HTTP cookie header was set and a CSRF token was sent but they do not match. We recommend deleting all cookies for this domain and retrying the flow." + }, + "message": "The request was rejected to protect you from Cross-Site-Request-Forgery (CSRF) which could cause account takeover, leaking personal information, and other serious security issues." + } + }, + { + "Id": "ErrInvalidCSRFTokenAJAXTokenNotSent", + "Err": { + "id": "security_csrf_violation", + "code": 403, + "status": "Forbidden", + "reason": "Please retry the flow and optionally clear your cookies. The request was rejected to protect you from Cross-Site-Request-Forgery (CSRF) which could cause account takeover, leaking personal information, and other serious security issues.", + "details": { + "docs": "https://www.ory.sh/docs/kratos/debug/csrf", + "hint": "The anti-CSRF cookie was found but the CSRF token was not included in the HTTP request body (csrf_token) nor in the HTTP header (X-CSRF-Token)." + }, + "message": "The request was rejected to protect you from Cross-Site-Request-Forgery (CSRF) which could cause account takeover, leaking personal information, and other serious security issues." + } + }, + { + "Id": "ErrInvalidCSRFTokenServer", + "Err": { + "id": "security_csrf_violation", + "code": 403, + "status": "Forbidden", + "reason": "Please retry the flow and optionally clear your cookies. The request was rejected to protect you from Cross-Site-Request-Forgery (CSRF) which could cause account takeover, leaking personal information, and other serious security issues.", + "details": { + "docs": "https://www.ory.sh/docs/kratos/debug/csrf", + "hint": "We detected a regular browser or server-side call. To debug browser calls check your browser's network tab to see what cookies are included or excluded. If you are calling from a server ensure that the appropriate cookies are being forwarded and that the SDK method is called correctly." + }, + "message": "The request was rejected to protect you from Cross-Site-Request-Forgery (CSRF) which could cause account takeover, leaking personal information, and other serious security issues." + } + }, + { + "Id": "ErrInvalidCSRFTokenServerCookieMissing", + "Err": { + "id": "security_csrf_violation", + "code": 403, + "status": "Forbidden", + "reason": "Please retry the flow and optionally clear your cookies. The request was rejected to protect you from Cross-Site-Request-Forgery (CSRF) which could cause account takeover, leaking personal information, and other serious security issues.", + "details": { + "docs": "https://www.ory.sh/docs/kratos/debug/csrf", + "hint": "We detected a regular browser or server-side call. To debug browser calls check your browser's network tab to see what cookies are included or excluded. If you are calling from a server ensure that the appropriate cookies are being forwarded and that the SDK method is called correctly.", + "reject_reason": "The HTTP cookie header was set but did not include the anti-CSRF cookie." + }, + "message": "The request was rejected to protect you from Cross-Site-Request-Forgery (CSRF) which could cause account takeover, leaking personal information, and other serious security issues." + } + }, + { + "Id": "ErrInvalidCSRFTokenServerNoCookies", + "Err": { + "id": "security_csrf_violation", + "code": 403, + "status": "Forbidden", + "reason": "Please retry the flow and optionally clear your cookies. The request was rejected to protect you from Cross-Site-Request-Forgery (CSRF) which could cause account takeover, leaking personal information, and other serious security issues.", + "details": { + "docs": "https://www.ory.sh/docs/kratos/debug/csrf", + "hint": "We detected a regular browser or server-side call. To debug browser calls check your browser's network tab to see what cookies are included or excluded. If you are calling from a server ensure that the appropriate cookies are being forwarded and that the SDK method is called correctly.", + "reject_reason": "The HTTP cookie header is empty or not set." + }, + "message": "The request was rejected to protect you from Cross-Site-Request-Forgery (CSRF) which could cause account takeover, leaking personal information, and other serious security issues." + } + }, + { + "Id": "ErrInvalidCSRFTokenServerTokenMismatch", + "Err": { + "id": "security_csrf_violation", + "code": 403, + "status": "Forbidden", + "reason": "Please retry the flow and optionally clear your cookies. The request was rejected to protect you from Cross-Site-Request-Forgery (CSRF) which could cause account takeover, leaking personal information, and other serious security issues.", + "details": { + "docs": "https://www.ory.sh/docs/kratos/debug/csrf", + "hint": "We detected a regular browser or server-side call. To debug browser calls check your browser's network tab to see what cookies are included or excluded. If you are calling from a server ensure that the appropriate cookies are being forwarded and that the SDK method is called correctly.", + "reject_reason": "The HTTP cookie header was set and a CSRF token was sent but they do not match. We recommend deleting all cookies for this domain and retrying the flow." + }, + "message": "The request was rejected to protect you from Cross-Site-Request-Forgery (CSRF) which could cause account takeover, leaking personal information, and other serious security issues." + } + }, + { + "Id": "ErrInvalidCSRFTokenServerTokenNotSent", + "Err": { + "id": "security_csrf_violation", + "code": 403, + "status": "Forbidden", + "reason": "Please retry the flow and optionally clear your cookies. The request was rejected to protect you from Cross-Site-Request-Forgery (CSRF) which could cause account takeover, leaking personal information, and other serious security issues.", + "details": { + "docs": "https://www.ory.sh/docs/kratos/debug/csrf", + "hint": "The anti-CSRF cookie was found but the CSRF token was not included in the HTTP request body (csrf_token) nor in the HTTP header (X-CSRF-Token)." + }, + "message": "The request was rejected to protect you from Cross-Site-Request-Forgery (CSRF) which could cause account takeover, leaking personal information, and other serious security issues." + } + } +] diff --git a/x/nosurf.go b/x/nosurf.go index ce49f01d3cdd..8275d5a122c4 100644 --- a/x/nosurf.go +++ b/x/nosurf.go @@ -9,6 +9,8 @@ import ( "fmt" "net/http" + "github.com/golang/gddo/httputil" + "github.com/ory/kratos/text" "github.com/ory/kratos/driver/config" @@ -21,13 +23,21 @@ import ( "github.com/ory/x/stringsx" ) +func newInvalidCsrfTokenError(hint string) *herodot.DefaultError { + return &herodot.DefaultError{ + IDField: text.ErrIDCSRF, + CodeField: http.StatusForbidden, + StatusField: http.StatusText(http.StatusForbidden), + ReasonField: "Please retry the flow and optionally clear your cookies. The request was rejected to protect you from Cross-Site-Request-Forgery (CSRF) which could cause account takeover, leaking personal information, and other serious security issues.", + DebugField: "", + DetailsField: map[string]interface{}{"docs": "https://www.ory.sh/docs/kratos/debug/csrf", "hint": hint}, + ErrorField: "The request was rejected to protect you from Cross-Site-Request-Forgery (CSRF) which could cause account takeover, leaking personal information, and other serious security issues.", + } +} + var ( - ErrInvalidCSRFToken = herodot.ErrForbidden. - WithID(text.ErrIDCSRF). - WithError("the request was rejected to protect you from Cross-Site-Request-Forgery"). - WithDetail("docs", "https://www.ory.sh/kratos/docs/debug/csrf"). - WithReason("Please retry the flow and optionally clear your cookies. The request was rejected to protect you from Cross-Site-Request-Forgery (CSRF) which could cause account takeover, leaking personal information, and other serious security issues.") - ErrGone = herodot.DefaultError{ + ErrInvalidCSRFToken = newInvalidCsrfTokenError("") + ErrGone = herodot.DefaultError{ CodeField: http.StatusGone, StatusField: http.StatusText(http.StatusGone), ReasonField: "", @@ -37,29 +47,31 @@ var ( } ) -const noCookie = "The HTTP Cookie Header is empty or not set." -const cookieMissing = "The HTTP Cookie Header was set but did not include the anti-CSRF cookie." -const tokenNotSent = "The anti-CSRF cookie was found but the CSRF token was not included in the HTTP request body (" + nosurf.CookieName + ") nor in the HTTP Header (" + nosurf.HeaderName + ")." -const tokenMismatch = "The HTTP Cookie Header was set and a CSRF token was sent but they do not match. We recommend deleting all cookies for this domain and retrying the flow." +const ( + noCookie = "The HTTP cookie header is empty or not set." + cookieMissing = "The HTTP cookie header was set but did not include the anti-CSRF cookie." + tokenNotSent = "The anti-CSRF cookie was found but the CSRF token was not included in the HTTP request body (" + nosurf.CookieName + ") nor in the HTTP header (" + nosurf.HeaderName + ")." + tokenMismatch = "The HTTP cookie header was set and a CSRF token was sent but they do not match. We recommend deleting all cookies for this domain and retrying the flow." +) var ( - ErrInvalidCSRFTokenAJAX = ErrInvalidCSRFToken. - WithDetail("hint", "We detected an AJAX call, please ensure that CORS is enabled and configured correctly, and that your AJAX code sends cookies and has credentials enabled. For further debugging, check your Browser's Network Tab to see what cookies are included or excluded.") + hintAjaxCallDetection = "We detected an AJAX call, please ensure that CORS is enabled and configured correctly, and that your AJAX code sends cookies and has credentials enabled. For further debugging, check your browser's network tab to see what cookies are included or excluded." + ErrInvalidCSRFTokenAJAX = newInvalidCsrfTokenError(hintAjaxCallDetection) - ErrInvalidCSRFTokenAJAXNoCookies = ErrInvalidCSRFTokenAJAX.WithDetail("reject_reason", noCookie) - ErrInvalidCSRFTokenAJAXCookieMissing = ErrInvalidCSRFTokenAJAX.WithDetail("reject_reason", cookieMissing) - ErrInvalidCSRFTokenAJAXTokenNotSent = ErrInvalidCSRFToken.WithDetail("hint", tokenNotSent) - ErrInvalidCSRFTokenAJAXTokenMismatch = ErrInvalidCSRFTokenAJAX.WithDetail("reject_reason", tokenMismatch) + ErrInvalidCSRFTokenAJAXNoCookies = newInvalidCsrfTokenError(hintAjaxCallDetection).WithDetail("reject_reason", noCookie) + ErrInvalidCSRFTokenAJAXCookieMissing = newInvalidCsrfTokenError(hintAjaxCallDetection).WithDetail("reject_reason", cookieMissing) + ErrInvalidCSRFTokenAJAXTokenMismatch = newInvalidCsrfTokenError(hintAjaxCallDetection).WithDetail("reject_reason", tokenMismatch) + ErrInvalidCSRFTokenAJAXTokenNotSent = newInvalidCsrfTokenError(tokenNotSent) ) var ( - ErrInvalidCSRFTokenServer = ErrInvalidCSRFToken. - WithDetail("hint", "We detected a regular browser or server-side call. To debug browser calls check your Browser's Network Tab to see what cookies are included or excluded. If you are calling from a server ensure that the appropriate cookies are being forwarded and that the SDK method is called correctly.") + hintServerDetection = "We detected a regular browser or server-side call. To debug browser calls check your browser's network tab to see what cookies are included or excluded. If you are calling from a server ensure that the appropriate cookies are being forwarded and that the SDK method is called correctly." + ErrInvalidCSRFTokenServer = newInvalidCsrfTokenError(hintServerDetection) - ErrInvalidCSRFTokenServerNoCookies = ErrInvalidCSRFTokenServer.WithDetail("reject_reason", noCookie) - ErrInvalidCSRFTokenServerCookieMissing = ErrInvalidCSRFTokenServer.WithDetail("reject_reason", cookieMissing) - ErrInvalidCSRFTokenServerTokenNotSent = ErrInvalidCSRFToken.WithDetail("hint", tokenNotSent) - ErrInvalidCSRFTokenServerTokenMismatch = ErrInvalidCSRFTokenAJAX.WithDetail("reject_reason", tokenMismatch) + ErrInvalidCSRFTokenServerNoCookies = newInvalidCsrfTokenError(hintServerDetection).WithDetail("reject_reason", noCookie) + ErrInvalidCSRFTokenServerCookieMissing = newInvalidCsrfTokenError(hintServerDetection).WithDetail("reject_reason", cookieMissing) + ErrInvalidCSRFTokenServerTokenMismatch = newInvalidCsrfTokenError(hintServerDetection).WithDetail("reject_reason", tokenMismatch) + ErrInvalidCSRFTokenServerTokenNotSent = newInvalidCsrfTokenError(tokenNotSent) ) type CSRFTokenGeneratorProvider interface { @@ -70,7 +82,7 @@ type CSRFToken func(r *http.Request) string const CSRFTokenName = "csrf_token" -func DefaultCSRFToken(r *http.Request) string { +func DefaultCSRFTokenGenerator(r *http.Request) string { return nosurf.Token(r) } @@ -130,13 +142,15 @@ type CSRFProvider interface { func CSRFCookieName(reg interface { config.Provider -}, r *http.Request) string { +}, r *http.Request, +) string { return "csrf_token_" + fmt.Sprintf("%x", sha256.Sum256([]byte(reg.Config().SelfPublicURL(r.Context()).String()))) } func NosurfBaseCookieHandler(reg interface { config.Provider -}) func(w http.ResponseWriter, r *http.Request) http.Cookie { +}, +) func(w http.ResponseWriter, r *http.Request) http.Cookie { return func(w http.ResponseWriter, r *http.Request) http.Cookie { secure := reg.Config().CookieSecure(r.Context()) @@ -173,9 +187,10 @@ func NosurfBaseCookieHandler(reg interface { func CSRFErrorReason(r *http.Request, reg interface { config.Provider -}) error { - // Is it an AJAX request? - isAjax := len(r.Header.Get("Origin")) == 0 +}, +) error { + secFetchMode := r.Header.Get("Sec-Fetch-Mode") + isAjax := secFetchMode == "cors" || secFetchMode == "no-cors" || httputil.NegotiateContentType(r, []string{"application/json"}, "text/html") == "application/json" if len(r.Header.Get("Cookie")) == 0 { if isAjax { @@ -187,7 +202,7 @@ func CSRFErrorReason(r *http.Request, reg interface { return errors.WithStack(ErrInvalidCSRFTokenAJAXCookieMissing) } return errors.WithStack(ErrInvalidCSRFTokenServerCookieMissing) - } else if len(r.Form.Get("csrf_token")+r.Header.Get(nosurf.HeaderName)) == 0 { + } else if len(r.Form.Get(nosurf.FormFieldName)+r.Header.Get(nosurf.HeaderName)) == 0 { if isAjax { return errors.WithStack(ErrInvalidCSRFTokenAJAXTokenNotSent) } @@ -204,7 +219,8 @@ func CSRFFailureHandler(reg interface { config.Provider LoggingProvider WriterProvider -}) http.HandlerFunc { +}, +) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { err := CSRFErrorReason(r, reg) reg.Logger(). @@ -227,7 +243,8 @@ func NewCSRFHandler( config.Provider LoggingProvider WriterProvider - }) *nosurf.CSRFHandler { + }, +) *nosurf.CSRFHandler { n := nosurf.New(router) n.SetBaseCookieFunc(NosurfBaseCookieHandler(reg)) @@ -241,7 +258,8 @@ func NewTestCSRFHandler(router http.Handler, reg interface { WriterProvider LoggingProvider config.Provider -}) *nosurf.CSRFHandler { +}, +) *nosurf.CSRFHandler { n := NewCSRFHandler(router, reg) reg.WithCSRFHandler(n) reg.WithCSRFTokenGenerator(nosurf.Token) diff --git a/x/nosurf_test.go b/x/nosurf_test.go index a889f0070050..0e22d547f643 100644 --- a/x/nosurf_test.go +++ b/x/nosurf_test.go @@ -10,12 +10,14 @@ import ( "net/http/httptest" "net/url" "regexp" + "slices" "strings" "testing" "github.com/tidwall/gjson" "github.com/ory/x/assertx" + "github.com/ory/x/snapshotx" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -88,7 +90,7 @@ func TestNosurfBaseCookieErrorHandler(t *testing.T) { newAjaxRequest := func() *http.Request { req := httptest.NewRequest("GET", "https://foo/bar", nil) - req.Header.Set("Origin", "foo") + req.Header.Set("Sec-Fetch-Mode", "cors") return req } newBrowserRequest := func() *http.Request { @@ -100,73 +102,100 @@ func TestNosurfBaseCookieErrorHandler(t *testing.T) { expectError(t, x.ErrInvalidCSRFTokenAJAXNoCookies, newAjaxRequest()) }) - t.Run("source=ajax", func(t *testing.T) { - expectError(t, x.ErrInvalidCSRFTokenAJAXNoCookies, newBrowserRequest()) + t.Run("source=browser", func(t *testing.T) { + expectError(t, x.ErrInvalidCSRFTokenServerNoCookies, newBrowserRequest()) }) }) - t.Run("case=ajax with cookie but without csrf cookie", func(t *testing.T) { - test := func(t *testing.T, req *http.Request) { + t.Run("case=with cookie header but without csrf cookie", func(t *testing.T) { + test := func(t *testing.T, req *http.Request, err error) { req.Header.Set("Cookie", "foo=bar;") - expectError(t, x.ErrInvalidCSRFTokenAJAXNoCookies, req) + expectError(t, err, req) } t.Run("source=ajax", func(t *testing.T) { - test(t, newAjaxRequest()) + test(t, newAjaxRequest(), x.ErrInvalidCSRFTokenAJAXCookieMissing) }) - t.Run("source=ajax", func(t *testing.T) { - test(t, newBrowserRequest()) + t.Run("source=browser", func(t *testing.T) { + test(t, newBrowserRequest(), x.ErrInvalidCSRFTokenServerCookieMissing) }) }) t.Run("case=ajax with correct cookie but token was not sent in header", func(t *testing.T) { - test := func(t *testing.T, req *http.Request) { + test := func(t *testing.T, req *http.Request, err error) { req.Header.Set("Cookie", x.CSRFCookieName(reg, req)+"=bar;") - expectError(t, x.ErrInvalidCSRFTokenAJAXTokenNotSent, req) + expectError(t, err, req) } t.Run("source=ajax", func(t *testing.T) { - test(t, newAjaxRequest()) + test(t, newAjaxRequest(), x.ErrInvalidCSRFTokenAJAXTokenNotSent) }) - t.Run("source=ajax", func(t *testing.T) { - test(t, newBrowserRequest()) + t.Run("source=browser", func(t *testing.T) { + test(t, newBrowserRequest(), x.ErrInvalidCSRFTokenServerTokenNotSent) }) }) t.Run("case=ajax with correct cookie and token in header but they do not match", func(t *testing.T) { - test := func(t *testing.T, req *http.Request) { + test := func(t *testing.T, req *http.Request, err error) { req.Header.Set(nosurf.HeaderName, "bar") req.Header.Set("Cookie", x.CSRFCookieName(reg, req)+"=bar;") - expectError(t, x.ErrInvalidCSRFTokenAJAXTokenMismatch, req) + expectError(t, err, req) } t.Run("source=ajax", func(t *testing.T) { - test(t, newAjaxRequest()) + test(t, newAjaxRequest(), x.ErrInvalidCSRFTokenAJAXTokenMismatch) }) - t.Run("source=ajax", func(t *testing.T) { - test(t, newBrowserRequest()) + t.Run("source=browser", func(t *testing.T) { + test(t, newBrowserRequest(), x.ErrInvalidCSRFTokenServerTokenMismatch) }) }) - t.Run("case=ajax with correct cookie and token in body but they do not match", func(t *testing.T) { - test := func(t *testing.T, req *http.Request) { + t.Run("case=with correct cookie and token in body but they do not match", func(t *testing.T) { + test := func(t *testing.T, req *http.Request, err error) { req.Header.Set("Accept", "application/x-www-form-urlencoded") + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") req.Header.Set("Cookie", x.CSRFCookieName(reg, req)+"=bar;") - expectError(t, x.ErrInvalidCSRFTokenAJAXTokenMismatch, req) + req.ParseForm() + expectError(t, err, req) } t.Run("source=ajax", func(t *testing.T) { - req := httptest.NewRequest("POST", "https://foo/bar", strings.NewReader(url.Values{nosurf.CookieName: {"bar"}}.Encode())) - req.Header.Set("Origin", "foo") - test(t, req) + req := httptest.NewRequest("POST", "https://foo/bar", strings.NewReader(url.Values{nosurf.FormFieldName: {"bar"}}.Encode())) + req.Header.Set("Sec-Fetch-Mode", "cors") + test(t, req, x.ErrInvalidCSRFTokenAJAXTokenMismatch) }) - t.Run("source=ajax", func(t *testing.T) { - req := httptest.NewRequest("POST", "https://foo/bar", strings.NewReader(url.Values{nosurf.CookieName: {"bar"}}.Encode())) - test(t, req) + t.Run("source=browser", func(t *testing.T) { + req := httptest.NewRequest("POST", "https://foo/bar", strings.NewReader(url.Values{nosurf.FormFieldName: {"bar"}}.Encode())) + test(t, req, x.ErrInvalidCSRFTokenServerTokenMismatch) }) }) } + +func TestSnapshotCsrfErrors(t *testing.T) { + type Snapshot = struct { + Id string + Err error + } + + errors := []Snapshot{ + {Id: "ErrInvalidCSRFToken", Err: x.ErrInvalidCSRFToken}, + {Id: "ErrInvalidCSRFTokenAJAX", Err: x.ErrInvalidCSRFTokenAJAX}, + {Id: "ErrInvalidCSRFTokenAJAXNoCookies", Err: x.ErrInvalidCSRFTokenAJAXNoCookies}, + {Id: "ErrInvalidCSRFTokenAJAXCookieMissing", Err: x.ErrInvalidCSRFTokenAJAXCookieMissing}, + {Id: "ErrInvalidCSRFTokenAJAXTokenMismatch", Err: x.ErrInvalidCSRFTokenAJAXTokenMismatch}, + {Id: "ErrInvalidCSRFTokenAJAXTokenNotSent", Err: x.ErrInvalidCSRFTokenAJAXTokenNotSent}, + {Id: "ErrInvalidCSRFTokenServer", Err: x.ErrInvalidCSRFTokenServer}, + {Id: "ErrInvalidCSRFTokenServerNoCookies", Err: x.ErrInvalidCSRFTokenServerNoCookies}, + {Id: "ErrInvalidCSRFTokenServerCookieMissing", Err: x.ErrInvalidCSRFTokenServerCookieMissing}, + {Id: "ErrInvalidCSRFTokenServerTokenMismatch", Err: x.ErrInvalidCSRFTokenServerTokenMismatch}, + {Id: "ErrInvalidCSRFTokenServerTokenNotSent", Err: x.ErrInvalidCSRFTokenServerTokenNotSent}, + } + slices.SortFunc(errors, func(a Snapshot, b Snapshot) int { + return strings.Compare(a.Id, b.Id) + }) + snapshotx.SnapshotT(t, errors) +}