Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ensure GetCSRF doesn't return an empty token #32130

Merged
merged 11 commits into from
Sep 30, 2024
Merged

Ensure GetCSRF doesn't return an empty token #32130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2cbc7d6
test: fix GetCSRF
wolfogre Sep 25, 2024
74cf5a8
test: fix TestCreateAnonymousAttachment
wolfogre Sep 25, 2024
3b07e1f
test: drop csrf token for GET requests
wolfogre Sep 25, 2024
10b2d63
chore: use /explore/repos
wolfogre Sep 25, 2024
9338b41
Revert "chore: use /explore/repos"
wolfogre Sep 25, 2024
60b4259
chore: use different urls
wolfogre Sep 25, 2024
58bc786
chore: improve createAttachment
wolfogre Sep 25, 2024
4c4233d
fix lint
wxiaoguang Sep 25, 2024
0bea254
Merge branch 'main' into bugfix/get_csrf
GiteaBot Sep 28, 2024
7d4d180
Merge branch 'main' into bugfix/get_csrf
GiteaBot Sep 28, 2024
58630f8
Merge branch 'main' into bugfix/get_csrf
GiteaBot Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions tests/integration/attachment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ func generateImg() bytes.Buffer {
return buff
}

func createAttachment(t *testing.T, session *TestSession, repoURL, filename string, buff bytes.Buffer, expectedStatus int) string {
func createAttachment(t *testing.T, session *TestSession, csrf, repoURL, filename string, buff bytes.Buffer, expectedStatus int) string {
body := &bytes.Buffer{}

// Setup multi-part
Expand All @@ -41,8 +41,6 @@ func createAttachment(t *testing.T, session *TestSession, repoURL, filename stri
err = writer.Close()
assert.NoError(t, err)

csrf := GetCSRF(t, session, repoURL)

req := NewRequestWithBody(t, "POST", repoURL+"/issues/attachments", body)
req.Header.Add("X-Csrf-Token", csrf)
req.Header.Add("Content-Type", writer.FormDataContentType())
Expand All @@ -59,15 +57,14 @@ func createAttachment(t *testing.T, session *TestSession, repoURL, filename stri
func TestCreateAnonymousAttachment(t *testing.T) {
defer tests.PrepareTestEnv(t)()
session := emptyTestSession(t)
// this test is not right because it just doesn't pass the CSRF validation
createAttachment(t, session, "user2/repo1", "image.png", generateImg(), http.StatusBadRequest)
createAttachment(t, session, GetCSRF(t, session, "/user/login"), "user2/repo1", "image.png", generateImg(), http.StatusSeeOther)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if someone looking at this code in two months knows why an attachment test calls a login endpoint...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It reads as this: when TestCreateAnonymousAttachment, it needs to GetCSRF from /user/login, it sounds reasonable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the CSRF token is not action related why can't we have a GetCSRF method without passed url? If /user/login is a redirect call /user/settings. One of both requests contains a token.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is user-related.

And I guess just no one has interest to work on #32130 (comment) , and no one has the explicitly said that "that FIXME could be removed and Gitea will always use a common CSRF token for current user"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what @KN4CK3R wants to say is we should document exactly that with a comment…

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's user-related, that's the reason why there is the session.

func GetCSRF(t testing.TB, session *TestSession) string {
	t.Helper()
	
	req := NewRequest(t, "GET", "/user/login")
	resp := session.MakeRequest(t, req, NoExpectedStatus)
	
	if resp.Code == http.StatusSeeOther {
		req = NewRequest(t, "GET", "/user/settings")
		resp = session.MakeRequest(t, req, http.StatusOk)
	}
	
	doc := NewHTMLParser(t, resp.Body)
	return doc.GetCSRF()
}

If we decide to use action-related CSRF tokens the GetCSRF method with an url must change too because then a single page can contain multiple forms with different tokens but doc.GetCSRF() currently just returns the first token.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would slow down many tests by using the extra request.

In most cases, the test writer clearly knows what CSRF token they would like to use: anonymous, or a signed-in user.

If you prefer to do so, I think it needs 2 functions:

GetCSRFForAnonymous
GetCSRFForCurrentUser

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(while these improvements could be in a separate PR but not in this PR's scope)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KN4CK3R

a separate PR
-> Refactor CSRF token #32216

}

func TestCreateIssueAttachment(t *testing.T) {
defer tests.PrepareTestEnv(t)()
const repoURL = "user2/repo1"
session := loginUser(t, "user2")
uuid := createAttachment(t, session, repoURL, "image.png", generateImg(), http.StatusOK)
uuid := createAttachment(t, session, GetCSRF(t, session, repoURL), repoURL, "image.png", generateImg(), http.StatusOK)

req := NewRequest(t, "GET", repoURL+"/issues/new")
resp := session.MakeRequest(t, req, http.StatusOK)
Expand Down
7 changes: 6 additions & 1 deletion tests/integration/integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import (

"github.com/PuerkitoBio/goquery"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/xeipuuv/gojsonschema"
)

Expand Down Expand Up @@ -486,12 +487,16 @@ func VerifyJSONSchema(t testing.TB, resp *httptest.ResponseRecorder, schemaFile
}

// GetCSRF returns CSRF token from body
// If it fails, it means the CSRF token is not found in the response body returned by the url with the given session.
// In this case, you should find a better url to get it.
func GetCSRF(t testing.TB, session *TestSession, urlStr string) string {
t.Helper()
req := NewRequest(t, "GET", urlStr)
resp := session.MakeRequest(t, req, http.StatusOK)
doc := NewHTMLParser(t, resp.Body)
return doc.GetCSRF()
csrf := doc.GetCSRF()
require.NotEmpty(t, csrf)
return csrf
}

// GetCSRFFrom returns CSRF token from body
Expand Down
4 changes: 0 additions & 4 deletions tests/integration/org_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,9 +204,7 @@ func TestTeamSearch(t *testing.T) {
var results TeamSearchResults

session := loginUser(t, user.Name)
csrf := GetCSRF(t, session, "/"+org.Name)
req := NewRequestf(t, "GET", "/org/%s/teams/-/search?q=%s", org.Name, "_team")
req.Header.Add("X-Csrf-Token", csrf)
wxiaoguang marked this conversation as resolved.
Show resolved Hide resolved
resp := session.MakeRequest(t, req, http.StatusOK)
DecodeJSON(t, resp, &results)
assert.NotEmpty(t, results.Data)
Expand All @@ -217,8 +215,6 @@ func TestTeamSearch(t *testing.T) {
// no access if not organization member
user5 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5})
session = loginUser(t, user5.Name)
csrf = GetCSRF(t, session, "/"+org.Name)
req = NewRequestf(t, "GET", "/org/%s/teams/-/search?q=%s", org.Name, "team")
req.Header.Add("X-Csrf-Token", csrf)
session.MakeRequest(t, req, http.StatusNotFound)
}
Loading