Ensure GetCSRF doesn't return an empty token

[wolfogre]

Since page templates keep changing, some pages that contained forms with CSRF token no longer have them.

It leads to some calls of GetCSRF returning an empty string, which fails the tests. Like

gitea/tests/integration/attachment_test.go

Lines 62 to 63 in 3269b04

	// this test is not right because it just doesn't pass the CSRF validation
	createAttachment(t, session, "user2/repo1", "image.png", generateImg(), http.StatusBadRequest)

The test did try to get the CSRF token and provided it, but it was empty.

[wolfogre]

Not yet completed, I want to see CICD fail first, then succeed with new commits.

[wolfogre]

I believe it's worth being backported to follow #32069.

[wxiaoguang]

I do not think this change is right.

The problem is that TestCreateAnonymousAttachment should use an "anonymous" csrf token to test the "anonymous" request.

Hmm ... I see the "anonymous" crsf token is provided by "user/login" now ....

[wolfogre]

I have a new idea to improve it, maybe after this PR.

Since it doesn't matter where the test code gets the CSRF token to send requests, why not

-func GetCSRF(t testing.TB, session *TestSession, urlStr string) string
+func GetCSRF(t testing.TB, session *TestSession) string {
	// try "/user/login" to get token
	// if it returns `SeeOther`, so it's logged in, try "/user/settings" again
}

/user/login and /user/settings pages should be stable enough, and developers don't need to decide which URL to get the token any longer.

[wxiaoguang]

I have a new idea to improve it, maybe after this PR.

Since it doesn't matter where the test code gets the CSRF token to send requests, why not

That's not the initial idea of Gitea's CSRF token design, see this:

Ideally, the CSRF token was designed to be related to current page (even the current form/action), but nobody did so.

[wolfogre]

Ideally, the CSRF token was designed to be related to current page (even the current form/action), but nobody did so.

Oh, thanks, I understand now.

[KN4CK3R]

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

[wxiaoguang]

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

[KN4CK3R]

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.

[wxiaoguang]

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"

[delvh]

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

[KN4CK3R]

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.

[wxiaoguang]

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

[wxiaoguang]

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

[wxiaoguang]

@KN4CK3R

a separate PR
-> Refactor CSRF token #32216