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.

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

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

Closed
fnetX opened this issue Nov 29, 2021 · 13 comments · Fixed by #19337
Closed

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

fnetX opened this issue Nov 29, 2021 · 13 comments · Fixed by #19337
Labels

Comments

@fnetX
Copy link
Contributor

fnetX commented Nov 29, 2021

Feature Description

If you have an issue / pull comment open for a long time, submitting it will yield Bad Request: Invalid CSRF token, which is fine from a security perspective, but very annoying to users.

It seems there is currently no way to restore the submitted data. I encountered this two times very recently with pull review comments, the content is gone if you go back in your history, and you can't re-send (and maybe get via the browser console, although this is already very unintuitive, because Gitea uses a redirect to the dashboard to display this warning, instead of directly showing it on the next page).

This is very annoying to me, and I think it might be annoying to all users ever encountering this.

I can think of some "naive solutions", but I doubt all work or are secure:

  • rework the error forward or WIP: Replace easymde with textarea #15394 so that the usual browser-internal restoring of text fields works (it doesn't for me with Gitea, I think it might be related to one of these things)
  • if the user is still logged in, allow to re-submit the data in the GUI, either with an "authorize" button on the page with the valid CSRF token, or by filling back the form the user wanted to submit
  • consider allowing to disable CSRF tokens as of Replace CRSF token with SameSite=strict #11188

Screenshots

No response

@wxiaoguang
Copy link
Contributor

And possible this one:

My proposal (#17282 (comment)) is saving a draft in local storage.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Dec 7, 2021

After reading the EasyMDE source code, the autosave of EasyMDE seems not ideal, it doesn't clear expired drafts, it may lead to problems: https://stackoverflow.com/questions/13567509/what-happens-when-localstorage-is-full .

@McNetic
Copy link

McNetic commented Dec 16, 2021

I second this Request. I just lost a good hour or two of code review commentary.

Edit: I have to revise: Fortunately, all inline comments were already saved (just not visible), so I only lost the actual review comment. Still annoying, but only two minutes of work ;-)

@lunny
Copy link
Member

lunny commented Dec 16, 2021

I think save it in the frontend is a choice. So the form content will not be lost when special things happen.

@davidak
Copy link

davidak commented Mar 23, 2022

This is a very annoying issue. I had it multiple times and last time i lost 2 hours of work!

@lunny have you considered using a more modern architecture with AJAX (is that still a thing?) or Websockets to change the page content in real time, like displaying an error or adding new comments in real-time? As always, GitHubs design is great!

@lunny lunny added the type/bug label Mar 26, 2022
@lunny
Copy link
Member

lunny commented Mar 26, 2022

This is a very annoying issue. I had it multiple times and last time i lost 2 hours of work!

@lunny have you considered using a more modern architecture with AJAX (is that still a thing?) or Websockets to change the page content in real time, like displaying an error or adding new comments in real-time? As always, GitHubs design is great!

We should do that AJAX thing, but there is no plan currently. But we are working toward that direction. And currently I think it should be a bug. At least, it should redirect to login page but not display the CSRF token issue.

@fnetX
Copy link
Contributor Author

fnetX commented Mar 26, 2022

The issue is that you can still be logged in, but the CSRF token be expired. At least this is the scenario for me.

Another solution would be to present a confirmation page that asks the user if they still want to perform the action with the new token.

@lunny
Copy link
Member

lunny commented Mar 26, 2022

The issue is that you can still be logged in, but the CSRF token be expired. At least this is the scenario for me.

Another solution would be to present a confirmation page that asks the user if they still want to perform the action with the new token.

So the CSRF expired time have to be set longer than session expired time to avoid that.

@fnetX
Copy link
Contributor Author

fnetX commented Mar 26, 2022

The issue is that you could for example login again in the meantime - the CSRF token is expired, but you have a valid session. The login page wouldn't help in this case. It should be gracefully handled, and in case you don't have a valid session, you'll be redirected to the login page from there.

@wxiaoguang
Copy link
Contributor

I think I find the bug in CSRF module.

If I understand correctly, the CSRF token is generated every 24h, and the valid period is also 24h.

So, if a user get a CSRF token at time t, then they starts writing comment at t+23:59, and submits at t+24:01, they will meet this problem.

If it is the case, there could be a simple fix to generate the CSRF token every minute (or every 10 minutes).

@lunny
Copy link
Member

lunny commented Apr 6, 2022

I think I find the bug in CSRF module.

If I understand correctly, the CSRF token is generated every 24h, and the valid period is also 24h.

So, if a user get a CSRF token at time t, then they starts writing comment at t+23:59, and submits at t+24:01, they will meet this problem.

If it is the case, there could be a simple fix to generate the CSRF token every minute (or every 10 minutes).

Where could you find it's 24h?

@wxiaoguang
Copy link
Contributor

About: "Where could you find it's 24h?"

Two timeout durations in csrf.go and xsrf.go, one is for cookie timeout, one is for token validation timeout, they are both 24h

@wxiaoguang
Copy link
Contributor

The PR to fix the bug:

@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants