Refactor CSRF protection modules, make sure CSRF tokens can be up-to-date. #19337

wxiaoguang · 2022-04-06T13:13:30Z

The CSRF protection modules have been broken for a while. For example, comment said: // Csrfer maps CSRF to each request. If this request is a Get request, it will generate a new token., however, it doesn't!! That's why a lot of users complain about their work lost.

Related issues:

And maybe more.

This PR does a refactoring to the CSRF related code, removes most unnecessary functions.

It introduces ParseCsrfToken to parse the generated token's issue time, and regenerate the token every minute.

And it fixes some incorrect code in old code, for example: ctx.Data["CsrfToken"] = html.EscapeString(ctx.csrf.GetToken()), the EscapeString is incorrect.

The error handling logic in Validate has been simplified, remove many duplicate code.

Totally this PR is +119 −195

routers/api/v1/api.go

modules/web/middleware/cookie.go

modules/context/csrf.go

zeripath

I wonder if it would make sense to move this into a separate package?

zeripath · 2022-04-06T18:46:08Z

modules/context/xsrf.go

 // It is exported so clients may set cookie timeouts that match generated tokens.
-const Timeout = 24 * time.Hour
+const CsrfTokenTimeout = 24 * time.Hour
+const CsrfTokenRegenerationDuration = 1 * time.Minute


This seems far too frequent. Perhaps this should be hourly?

This seems far too frequent. Perhaps this should be hourly?

It doesn't matter, it's just a simple hash when generating the token, doesn't cost more resources than other logic.

And the generated token is valid for the next 24 hours, so there should be no problem.

Indeed we can keep generating new tokens on every request, there will still be no performance problem. I just introduced this mechanism in case someone doesn't like generating the tokens (and sending cookies) again and again.

zeripath · 2022-04-06T18:49:00Z

modules/context/csrf.go

+		// If cookie token presents, re-use existing unexpired token, else generate a new one.
+		if issueTime, ok := ParseCsrfToken(x.Token); ok {
+			dur := time.Since(issueTime)
+			if dur >= -CsrfTokenRegenerationDuration && dur <= CsrfTokenRegenerationDuration {


Can this really be negative?

Can this really be negative?

For example, the server time changed a lot? Personally I prefer to check the [-duration, duration] range

ps: if issueTime can be guaranteed to be a monotonic clock, then we do not check the negative range. Checking the negative is not wrong because the issueTime might come from other packages and be refactored.

If you prefer to remove the negative check, it might can be done. However, I think it requires the code to store the mono clock in token, instead of a simple UnixNano ..... currently, issueTime := time.Unix(0, nanos) is a non-monotonic clock

wxiaoguang · 2022-04-07T02:32:41Z

I wonder if it would make sense to move this into a separate package?

What's the preferred package name and package path?

silverwind · 2022-04-07T15:57:33Z

What's the preferred package name and package path?

modules/csrf maybe? Not sure if that's what @zeripath means.

wxiaoguang · 2022-04-07T16:48:00Z

What's the preferred package name and package path?

modules/csrf maybe? Not sure if that's what @zeripath means.

I do not think csrf can be a proper package name. CSRF means Cross site request forgery, it's an attack vector/method. The package is protecting against CSRF attack.

silverwind · 2022-04-07T16:51:15Z

It's code to handle CSRF, seems pretty clear to me. And a short name is easy to work with.

wxiaoguang · 2022-04-07T19:22:03Z

TBH, I still think the name csrf is not ideal enough.

And this PR has done what it should do. Moving code into a new package is too much for this PR. Reasons:

If we do moving code and changing code at the same time, this PR would become more difficult to be reviewed, and it may introduce potential bugs.
Moving code into a new package is not an easy work at the moment. Because modules/context.go need modules/csrf.go and csrf also needs context. Without more related refactoring, there will be a cycle-import error, the downside of Golang. For reason 1, it's not ideal to make these more argument renaming/changing refactoring into this PR.

@zeripath @silverwind

modules/context/xsrf.go

* giteaofficial/main: (22 commits) Add logic to switch between source/rendered on Markdown (go-gitea#19356) Fixed registry host value. (go-gitea#19363) [skip ci] Updated translations via Crowdin Allow package linking to private repository (go-gitea#19348) Use "main" as default branch name (go-gitea#19354) Move milestone to models/issues/ (go-gitea#19278) Refactor CSRF protection modules, make sure CSRF tokens can be up-to-date. (go-gitea#19337) Remove dependent on session auth for api/v1 routers (go-gitea#19321) API: Search Issues, dont show 500 if filter result in empty list (go-gitea#19244) [skip ci] Updated translations via Crowdin Never use /api/v1 from Gitea UI Pages (go-gitea#19318) [skip ci] Updated translations via Crowdin Show ssh command directly in template instead of i18n translation (go-gitea#19335) Package registry changes (go-gitea#19305) [skip ci] Updated translations via Crowdin Add `ENABLE_SSH_LOG` to debugging problems (go-gitea#19316) Warn on SSH connection for incorrect configuration (go-gitea#19317) escape fake link Allow custom redirect for landing page (go-gitea#19324) [skip ci] Updated translations via Crowdin ...

…date. (go-gitea#19337) Do a refactoring to the CSRF related code, remove most unnecessary functions. Parse the generated token's issue time, regenerate the token every a few minutes.

wxiaoguang force-pushed the refactor-csrf branch from f0023a5 to c6da09f Compare April 6, 2022 13:18

wxiaoguang added type/bug type/refactoring Existing code has been cleaned up. There should be no new functionality. labels Apr 6, 2022

wxiaoguang added this to the 1.17.0 milestone Apr 6, 2022

wxiaoguang linked an issue Apr 6, 2022 that may be closed by this pull request

Invalid CSRF token issues - unable to recover submitted data #17850

Closed

wxiaoguang mentioned this pull request Apr 6, 2022

Invalid CSRF token issues - unable to recover submitted data #17850

Closed

wxiaoguang force-pushed the refactor-csrf branch from c6da09f to e3aa6b4 Compare April 6, 2022 13:25

lunny reviewed Apr 6, 2022

View reviewed changes

routers/api/v1/api.go Outdated Show resolved Hide resolved

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 6, 2022

Refactor CSRF

9c5b27b

wxiaoguang force-pushed the refactor-csrf branch from e3aa6b4 to 9c5b27b Compare April 6, 2022 13:29

delvh reviewed Apr 6, 2022

View reviewed changes

wxiaoguang added 2 commits April 6, 2022 21:43

use middleware.DeleteCSRFCookie to replace duplicate code

abc4e6d

refactor

70f8710

wxiaoguang force-pushed the refactor-csrf branch from 69e6236 to 70f8710 Compare April 6, 2022 14:02

delvh approved these changes Apr 6, 2022

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 6, 2022

wxiaoguang mentioned this pull request Apr 6, 2022

[Backport] Fix invalid CSRF token bug, make sure CSRF tokens can be up-to-date #19338

Merged

zeripath reviewed Apr 6, 2022

View reviewed changes

fine tune and add comments

850a9fd

This comment was marked as off-topic.

Sign in to view

wxiaoguang requested a review from zeripath April 7, 2022 09:39

Merge branch 'main' into refactor-csrf

8570784

wxiaoguang requested a review from lunny April 8, 2022 02:01

Merge branch 'main' into refactor-csrf

ce08711

lunny reviewed Apr 8, 2022

View reviewed changes

modules/context/xsrf.go Show resolved Hide resolved

lunny approved these changes Apr 8, 2022

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 8, 2022

lunny and others added 2 commits April 8, 2022 10:39

Merge branch 'main' into refactor-csrf

f5c980b

fine tune CSRF response, tested

909e82d

6543 approved these changes Apr 8, 2022

View reviewed changes

6543 and others added 3 commits April 8, 2022 06:23

Merge branch 'main' into refactor-csrf

6db3cc3

add csrf test

c55c5ea

add a comment for APIPath with CSRF token

b1babfe

wxiaoguang merged commit 84ceaa9 into go-gitea:main Apr 8, 2022

wxiaoguang deleted the refactor-csrf branch April 8, 2022 05:21

wxiaoguang mentioned this pull request Apr 8, 2022

Show login form and redo action if successful login when trying to comment on an issue where session has timed out. #17282

Closed

go-gitea locked and limited conversation to collaborators May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor CSRF protection modules, make sure CSRF tokens can be up-to-date. #19337

Refactor CSRF protection modules, make sure CSRF tokens can be up-to-date. #19337

wxiaoguang commented Apr 6, 2022 •

edited

Loading

zeripath left a comment

zeripath Apr 6, 2022

wxiaoguang Apr 7, 2022 •

edited

Loading

zeripath Apr 6, 2022

wxiaoguang Apr 7, 2022

wxiaoguang Apr 7, 2022 •

edited

Loading

wxiaoguang commented Apr 7, 2022

This comment was marked as off-topic.

silverwind commented Apr 7, 2022 •

edited

Loading

wxiaoguang commented Apr 7, 2022

silverwind commented Apr 7, 2022

wxiaoguang commented Apr 7, 2022

Refactor CSRF protection modules, make sure CSRF tokens can be up-to-date. #19337

Refactor CSRF protection modules, make sure CSRF tokens can be up-to-date. #19337

Conversation

wxiaoguang commented Apr 6, 2022 • edited Loading

zeripath left a comment

Choose a reason for hiding this comment

zeripath Apr 6, 2022

Choose a reason for hiding this comment

wxiaoguang Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

zeripath Apr 6, 2022

Choose a reason for hiding this comment

wxiaoguang Apr 7, 2022

Choose a reason for hiding this comment

wxiaoguang Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

wxiaoguang commented Apr 7, 2022

This comment was marked as off-topic.

silverwind commented Apr 7, 2022 • edited Loading

wxiaoguang commented Apr 7, 2022

silverwind commented Apr 7, 2022

wxiaoguang commented Apr 7, 2022

wxiaoguang commented Apr 6, 2022 •

edited

Loading

wxiaoguang Apr 7, 2022 •

edited

Loading

wxiaoguang Apr 7, 2022 •

edited

Loading

silverwind commented Apr 7, 2022 •

edited

Loading