From 6275d1bc5000a09793574696cb18cc5b5c4b07d6 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sun, 22 Sep 2024 12:57:03 +0200 Subject: [PATCH] Refactor CSRF protector (#32057) (fix forgejo tests) Fix the tests unique to Forgejo that are impacted by the refactor. --- services/context/csrf.go | 7 ++-- tests/integration/links_test.go | 24 +++++++++--- tests/integration/oauth_test.go | 66 ++++++++++++++++++--------------- 3 files changed, 59 insertions(+), 38 deletions(-) diff --git a/services/context/csrf.go b/services/context/csrf.go index 5890d53f42..e0518a499b 100644 --- a/services/context/csrf.go +++ b/services/context/csrf.go @@ -30,8 +30,9 @@ import ( ) const ( - CsrfHeaderName = "X-Csrf-Token" - CsrfFormName = "_csrf" + CsrfHeaderName = "X-Csrf-Token" + CsrfFormName = "_csrf" + CsrfErrorString = "Invalid CSRF token." ) // CSRFProtector represents a CSRF protector and is used to get the current token and validate the token. @@ -144,7 +145,7 @@ func (c *csrfProtector) validateToken(ctx *Context, token string) { c.DeleteCookie(ctx) // currently, there should be no access to the APIPath with CSRF token. because templates shouldn't use the `/api/` endpoints. // FIXME: distinguish what the response is for: HTML (web page) or JSON (fetch) - http.Error(ctx.Resp, "Invalid CSRF token.", http.StatusBadRequest) + http.Error(ctx.Resp, CsrfErrorString, http.StatusBadRequest) } } diff --git a/tests/integration/links_test.go b/tests/integration/links_test.go index 68d7008e02..e9ad933b24 100644 --- a/tests/integration/links_test.go +++ b/tests/integration/links_test.go @@ -12,6 +12,7 @@ import ( "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/test" + forgejo_context "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" @@ -190,11 +191,6 @@ func TestRedirectsWebhooks(t *testing.T) { {from: "/user/settings/hooks/" + kind + "/new", to: "/user/login", verb: "GET"}, {from: "/admin/system-hooks/" + kind + "/new", to: "/user/login", verb: "GET"}, {from: "/admin/default-hooks/" + kind + "/new", to: "/user/login", verb: "GET"}, - {from: "/user2/repo1/settings/hooks/" + kind + "/new", to: "/", verb: "POST"}, - {from: "/admin/system-hooks/" + kind + "/new", to: "/", verb: "POST"}, - {from: "/admin/default-hooks/" + kind + "/new", to: "/", verb: "POST"}, - {from: "/user2/repo1/settings/hooks/1", to: "/", verb: "POST"}, - {from: "/admin/hooks/1", to: "/", verb: "POST"}, } for _, info := range redirects { req := NewRequest(t, info.verb, info.from) @@ -202,6 +198,24 @@ func TestRedirectsWebhooks(t *testing.T) { assert.EqualValues(t, path.Join(setting.AppSubURL, info.to), test.RedirectURL(resp), info.from) } } + + for _, kind := range []string{"forgejo", "gitea"} { + csrf := []struct { + from string + verb string + }{ + {from: "/user2/repo1/settings/hooks/" + kind + "/new", verb: "POST"}, + {from: "/admin/hooks/1", verb: "POST"}, + {from: "/admin/system-hooks/" + kind + "/new", verb: "POST"}, + {from: "/admin/default-hooks/" + kind + "/new", verb: "POST"}, + {from: "/user2/repo1/settings/hooks/1", verb: "POST"}, + } + for _, info := range csrf { + req := NewRequest(t, info.verb, info.from) + resp := MakeRequest(t, req, http.StatusBadRequest) + assert.Contains(t, resp.Body.String(), forgejo_context.CsrfErrorString) + } + } } func TestRepoLinks(t *testing.T) { diff --git a/tests/integration/oauth_test.go b/tests/integration/oauth_test.go index 0d5e9a0472..f385b99e46 100644 --- a/tests/integration/oauth_test.go +++ b/tests/integration/oauth_test.go @@ -11,6 +11,7 @@ import ( "fmt" "io" "net/http" + "net/http/httptest" "net/url" "strings" "testing" @@ -24,6 +25,7 @@ import ( api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/test" "code.gitea.io/gitea/routers/web/auth" + forgejo_context "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/tests" "github.com/markbates/goth" @@ -803,6 +805,16 @@ func TestOAuthIntrospection(t *testing.T) { }) } +func requireCookieCSRF(t *testing.T, resp http.ResponseWriter) string { + for _, c := range resp.(*httptest.ResponseRecorder).Result().Cookies() { + if c.Name == "_csrf" { + return c.Value + } + } + require.True(t, false, "_csrf not found in cookies") + return "" +} + func TestOAuth_GrantScopesReadUser(t *testing.T) { defer tests.PrepareTestEnv(t)() @@ -840,19 +852,18 @@ func TestOAuth_GrantScopesReadUser(t *testing.T) { authorizeResp := ctx.MakeRequest(t, authorizeReq, http.StatusSeeOther) authcode := strings.Split(strings.Split(authorizeResp.Body.String(), "?code=")[1], "&")[0] - htmlDoc := NewHTMLParser(t, authorizeResp.Body) grantReq := NewRequestWithValues(t, "POST", "/login/oauth/grant", map[string]string{ - "_csrf": htmlDoc.GetCSRF(), + "_csrf": requireCookieCSRF(t, authorizeResp), "client_id": app.ClientID, "redirect_uri": "a", "state": "thestate", "granted": "true", }) - grantResp := ctx.MakeRequest(t, grantReq, http.StatusSeeOther) - htmlDocGrant := NewHTMLParser(t, grantResp.Body) + grantResp := ctx.MakeRequest(t, grantReq, http.StatusBadRequest) + assert.NotContains(t, grantResp.Body.String(), forgejo_context.CsrfErrorString) accessTokenReq := NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{ - "_csrf": htmlDocGrant.GetCSRF(), + "_csrf": requireCookieCSRF(t, authorizeResp), "grant_type": "authorization_code", "client_id": app.ClientID, "client_secret": app.ClientSecret, @@ -921,19 +932,18 @@ func TestOAuth_GrantScopesFailReadRepository(t *testing.T) { authorizeResp := ctx.MakeRequest(t, authorizeReq, http.StatusSeeOther) authcode := strings.Split(strings.Split(authorizeResp.Body.String(), "?code=")[1], "&")[0] - htmlDoc := NewHTMLParser(t, authorizeResp.Body) grantReq := NewRequestWithValues(t, "POST", "/login/oauth/grant", map[string]string{ - "_csrf": htmlDoc.GetCSRF(), + "_csrf": requireCookieCSRF(t, authorizeResp), "client_id": app.ClientID, "redirect_uri": "a", "state": "thestate", "granted": "true", }) - grantResp := ctx.MakeRequest(t, grantReq, http.StatusSeeOther) - htmlDocGrant := NewHTMLParser(t, grantResp.Body) + grantResp := ctx.MakeRequest(t, grantReq, http.StatusBadRequest) + assert.NotContains(t, grantResp.Body.String(), forgejo_context.CsrfErrorString) accessTokenReq := NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{ - "_csrf": htmlDocGrant.GetCSRF(), + "_csrf": requireCookieCSRF(t, authorizeResp), "grant_type": "authorization_code", "client_id": app.ClientID, "client_secret": app.ClientSecret, @@ -1000,19 +1010,18 @@ func TestOAuth_GrantScopesReadRepository(t *testing.T) { authorizeResp := ctx.MakeRequest(t, authorizeReq, http.StatusSeeOther) authcode := strings.Split(strings.Split(authorizeResp.Body.String(), "?code=")[1], "&")[0] - htmlDoc := NewHTMLParser(t, authorizeResp.Body) grantReq := NewRequestWithValues(t, "POST", "/login/oauth/grant", map[string]string{ - "_csrf": htmlDoc.GetCSRF(), + "_csrf": requireCookieCSRF(t, authorizeResp), "client_id": app.ClientID, "redirect_uri": "a", "state": "thestate", "granted": "true", }) - grantResp := ctx.MakeRequest(t, grantReq, http.StatusSeeOther) - htmlDocGrant := NewHTMLParser(t, grantResp.Body) + grantResp := ctx.MakeRequest(t, grantReq, http.StatusBadRequest) + assert.NotContains(t, grantResp.Body.String(), forgejo_context.CsrfErrorString) accessTokenReq := NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{ - "_csrf": htmlDocGrant.GetCSRF(), + "_csrf": requireCookieCSRF(t, authorizeResp), "grant_type": "authorization_code", "client_id": app.ClientID, "client_secret": app.ClientSecret, @@ -1082,19 +1091,18 @@ func TestOAuth_GrantScopesReadPrivateGroups(t *testing.T) { authorizeResp := ctx.MakeRequest(t, authorizeReq, http.StatusSeeOther) authcode := strings.Split(strings.Split(authorizeResp.Body.String(), "?code=")[1], "&")[0] - htmlDoc := NewHTMLParser(t, authorizeResp.Body) grantReq := NewRequestWithValues(t, "POST", "/login/oauth/grant", map[string]string{ - "_csrf": htmlDoc.GetCSRF(), + "_csrf": requireCookieCSRF(t, authorizeResp), "client_id": app.ClientID, "redirect_uri": "a", "state": "thestate", "granted": "true", }) - grantResp := ctx.MakeRequest(t, grantReq, http.StatusSeeOther) - htmlDocGrant := NewHTMLParser(t, grantResp.Body) + grantResp := ctx.MakeRequest(t, grantReq, http.StatusBadRequest) + assert.NotContains(t, grantResp.Body.String(), forgejo_context.CsrfErrorString) accessTokenReq := NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{ - "_csrf": htmlDocGrant.GetCSRF(), + "_csrf": requireCookieCSRF(t, authorizeResp), "grant_type": "authorization_code", "client_id": app.ClientID, "client_secret": app.ClientSecret, @@ -1164,19 +1172,18 @@ func TestOAuth_GrantScopesReadOnlyPublicGroups(t *testing.T) { authorizeResp := ctx.MakeRequest(t, authorizeReq, http.StatusSeeOther) authcode := strings.Split(strings.Split(authorizeResp.Body.String(), "?code=")[1], "&")[0] - htmlDoc := NewHTMLParser(t, authorizeResp.Body) grantReq := NewRequestWithValues(t, "POST", "/login/oauth/grant", map[string]string{ - "_csrf": htmlDoc.GetCSRF(), + "_csrf": requireCookieCSRF(t, authorizeResp), "client_id": app.ClientID, "redirect_uri": "a", "state": "thestate", "granted": "true", }) - grantResp := ctx.MakeRequest(t, grantReq, http.StatusSeeOther) - htmlDocGrant := NewHTMLParser(t, grantResp.Body) + grantResp := ctx.MakeRequest(t, grantReq, http.StatusBadRequest) + assert.NotContains(t, grantResp.Body.String(), forgejo_context.CsrfErrorString) accessTokenReq := NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{ - "_csrf": htmlDocGrant.GetCSRF(), + "_csrf": requireCookieCSRF(t, authorizeResp), "grant_type": "authorization_code", "client_id": app.ClientID, "client_secret": app.ClientSecret, @@ -1260,19 +1267,18 @@ func TestOAuth_GrantScopesReadPublicGroupsWithTheReadScope(t *testing.T) { authorizeResp := ctx.MakeRequest(t, authorizeReq, http.StatusSeeOther) authcode := strings.Split(strings.Split(authorizeResp.Body.String(), "?code=")[1], "&")[0] - htmlDoc := NewHTMLParser(t, authorizeResp.Body) grantReq := NewRequestWithValues(t, "POST", "/login/oauth/grant", map[string]string{ - "_csrf": htmlDoc.GetCSRF(), + "_csrf": requireCookieCSRF(t, authorizeResp), "client_id": app.ClientID, "redirect_uri": "a", "state": "thestate", "granted": "true", }) - grantResp := ctx.MakeRequest(t, grantReq, http.StatusSeeOther) - htmlDocGrant := NewHTMLParser(t, grantResp.Body) + grantResp := ctx.MakeRequest(t, grantReq, http.StatusBadRequest) + assert.NotContains(t, grantResp.Body.String(), forgejo_context.CsrfErrorString) accessTokenReq := NewRequestWithValues(t, "POST", "/login/oauth/access_token", map[string]string{ - "_csrf": htmlDocGrant.GetCSRF(), + "_csrf": requireCookieCSRF(t, authorizeResp), "grant_type": "authorization_code", "client_id": app.ClientID, "client_secret": app.ClientSecret,