search

go-gitea / gitea

Git with a cup of tea! Painless self-hosted all-in-one software development service, including Git hosting, code review, team collaboration, package registry and CI/CD
https://gitea.com
MIT License
45.02k stars 5.49k forks source link

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

Closed fnetX closed 2 years ago

fnetX commented 2 years ago

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:

Screenshots

No response

wxiaoguang commented 2 years ago

And possible this one:

My proposal (https://github.com/go-gitea/gitea/issues/17282#issuecomment-950305936) is saving a draft in local storage.

wxiaoguang commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

davidak commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

The PR to fix the bug: