Fix #5226 by adding CSRF checking to api reqToken and add CSRF to the POST header for deadline

[zeripath]

#4840 prevents CSRF attacks by preventing POSTs to API addresses without being logged in by Token or with BASIC auth. It did not add capability for providing a CSRF token.

This PR adds CSRF token checking in to the reqToken function within routers/api/v1/api.go and then adjusts the way deadlines are POSTed to pass in the CSRF token in as a header.

Fixes #5226, #5249

@beeonthego as far as I understand this would still prevent the CSRF attacks. Is this correct?

Edit: make clear #4840 was preventing CSRF attacks not XSS

[techknowlogick]

ping @cezar97 as well

[codecov-io]

Codecov Report

Merging #5250 into master will decrease coverage by <.01%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #5250      +/-   ##
==========================================
- Coverage   37.64%   37.63%   -0.01%     
==========================================
  Files         310      310              
  Lines       46042    46054      +12     
==========================================
+ Hits        17331    17334       +3     
- Misses      26235    26245      +10     
+ Partials     2476     2475       -1

Impacted Files	Coverage Δ
modules/context/api.go	50.81% <0%> (-7.68%)	⬇️
routers/api/v1/api.go	74.2% <71.42%> (-0.64%)	⬇️
modules/process/manager.go	76.81% <0%> (-4.35%)	⬇️
models/repo_indexer.go	50.84% <0%> (+2.54%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57a8440...5c906d0. Read the comment docs.

[zeripath]

Thanks for the clarification. As it stands the CSRF token will only be checked if there is no BASIC Auth or token. I should actually check that definitely means that the user is logged in with a session, however I'd be surprised if csrf.Validate would pass if there was no session.

[zeripath]

Hmm ok looking again, I think there needs to be a check that the user is signed in before testing the CSRF, as it's possible to have a correct CSRF and session but not be logged in.

Edit: I've just changed the PR to account for this. Now reqToken requires Basic, Apitoken or if not one of those requires the user to be signed in and a valid CSRF.

[zeripath]

@cezar97 :

@zeripath if the user is required to be logged in too, user and repo searching don't work when logged out?
I'm referring to #5249 (comment) .

Those GET urls aren't protected by reqToken so aren't affected by this change. There are a few GET urls which are protected by reqToken - however, I think these are pure API urls. (Edit: These all appear unused by the web UI. Apart from POST /api/v1/markdown, POST /api/v1/repos/:owner/:name/issues/:id/deadline appears to be the only POST to a API url.)

I do think it may be worth rationalising and refactoring the API req* functions though. I don't think reqToken is a very good name for what it is doing. Also, I suspect that routes protected by reqAdmin are CSRF exploitable... (Edit: they are - I've updated my PR to include reqToken protection on these.)

[zeripath]

Hmm would it be helpful to go through the API urls systematically looking at where there might still be CSRF issues?

[zeripath]

For clarity, @cezar97, the reqAdmin routes weren't being protected by reqToken and should have been. So I added that as a second commit.

[kolaente]

Only POST API routes are vulnerable, as GET ones follow the best practices and don't run any action, just display information that can't be retrieved by an attacker anyway.

No, things like the repo search can expose private repos. Also these ones are dependent on a user session.

[techknowlogick]

Looks great! 🎉

@kolaente would you be able to review again?

[kolaente]

@cezar97 OK, I guess it doesn't make much of a difference if these are protected by CSRF or not.
Thanks for clarifying!

[kolaente]

Thanks!

[techknowlogick]

Thanks for PR @zeripath. Now that this is merged, are you able to backport the commit to release/v1.6 branch?