Enforce token on api routes [fixed critical security issue #4357]

[beeonthego]

Check and make sure an authentication has been made using token or basic auth in reqToken handler.

When a user logs into Drone using gitea password, the current integration with Drone depends on basic auth to authenticate a Gitea user and fetches/creates an access token with the name drone. So this PR treats a valid basic auth header as the equivalent of api token, in order for Drone integration continue to work.

The user dashboard uses a few API routes for searching user/repo. All these requests use GET methods, and return results depending on whether the user has signed in, including token and other methods. These routes do not use reqToken handler, and will continue to work as they are now.

Please review and comment if changes are required. It is highly appreciated if it can be merged soon to have API routes covered.

[lunny]

I don't think it's necessary.

[beeonthego]

@lunny yes I agree the first expression can be removed. the result is the same.

the logic in the current test suites needs review and change, otherwise this pr won't pass all tests.

In fact when token is enforced, some tests should be added to make sure cookie can not access protected api routes.

[beeonthego]

I assume the maintainers will review the access policy and modify test suites if agreeing to the policy of enforcing token in reqToken handler. I will not do anything about the test suites.

Please let me know if there is any other action required from me. This PR is about fixing critical security issues, and is easy to review. It is highly appreciated if it can be given some priority.

[techknowlogick]

From a preliminary review this looks good, however now a lot of the tests fail, and the GPG test has a panic

Edit: From posting this, I now see the post above mine. Sadly this won't be merged unless the tests pass (as the GitHub policies that are set up disable the button)

[beeonthego]

Can someone review the access policy and test suite, prepare and merge a PR so a token is enforced in API routes? This issue has been around for a long time now, and the fix is easy. I don't want to waste time on test suites, if the maintainers eventually decide "no, we will keep things as they are now".

[techknowlogick]

I'm a maintainer and can promise you that Cezar97's reports will always be something that I listen to.

Speaking of being a maintainer, I have the ability to push to your branch, I can work on the tests, however I like to ask permission before I make changes to someones branch, so is it ok if I work on your branch to resolve these failing tests?

[beeonthego]

@techknowlogick yes please do so and enforce token policy on API routes asap. If you need to, I can add your public ssh key to my repo so you have full access to that branch.

I have done internal tests and confirmed that browsers automatically send cookie along with cross site requests (GET and POST). Modern browsers have built-in same origin security policy and do not expose responses of certain content type to JavaScript. However such protection may not cover json/jsonp response. API routes accept cookie alone, skip csrf token check, and respond with json, and thus are not covered by browsers' same origin policy protection.

API Routes are vulnerable to malicious scripts on another site, no XSS required.

This may lead to theft/deletion of private code. Please do act quickly.
Malicious scripts do not have direct access to victim's cookie, but can take advantage of browsers' behaviour and perform GET/POST actions from another site, using victim's cookie indirectly.

[beeonthego]

@techknowlogick I think all maintainers can already make changes to the forked branch as I have enabled "Allow edits from maintainers". Please let me know if any action is required from me. thank you.

[techknowlogick]

🎉 Woooo! Tests now pass!

(again, I'm so sorry for all of the commits, I'm still on mobile and am using the web editor which is extra difficult because it doesn't exist in mobile version of GitHub)

[beeonthego]

@techknowlogick Thank you for the hard work! I don't mind the commit emails at all. These serve as time sheets to show you have spent 6 hours debugging and fixing the tests. Well done. Highly appreciated.

[codecov-io]

Codecov Report

Merging #4840 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4840      +/-   ##
==========================================
+ Coverage   37.32%   37.38%   +0.06%     
==========================================
  Files         305      305              
  Lines       45219    45221       +2     
==========================================
+ Hits        16876    16907      +31     
+ Misses      25905    25873      -32     
- Partials     2438     2441       +3

Impacted Files	Coverage Δ
modules/auth/auth.go	55.24% <100%> (+2.73%)	⬆️
routers/api/v1/api.go	75.37% <100%> (ø)	⬆️
models/repo_list.go	56.37% <0%> (-1.35%)	⬇️
modules/process/manager.go	81.15% <0%> (+4.34%)	⬆️
modules/auth/user_form.go	28.57% <0%> (+7.14%)	⬆️
routers/user/setting/applications.go	48.88% <0%> (+48.88%)	⬆️

---

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 387a4b0...40a2419. Read the comment docs.

[lunny]

Some ajax requests on the UI used /api/v1/**, some page and feature will be broken if this PR merged. refs:
https://github.com/go-gitea/gitea/blob/master/public/js/index.js#L1459
https://github.com/go-gitea/gitea/blob/master/public/js/index.js#L1486
https://github.com/go-gitea/gitea/blob/master/public/js/index.js#L2087
https://github.com/go-gitea/gitea/blob/master/public/js/index.js#L2507
https://github.com/go-gitea/gitea/blob/master/public/js/index.js#L2630
That routes have to be replaced before this PR merged.

[beeonthego]

@lunny As I wrote in the comment when submitting the PR, API routes used by front end ajax requests do not have reqToken middleware, and are not affected. These routes will continue to work, are all GET requests performing no action to change state or modify data. These routes and handler functions can be addressed with another PR. what do you think?

[techknowlogick]

@beeonthego oh my, I didn't even realize that was the amount of time I spent 😬 I would've been faster if only I wasn't on mobile.

[lunny]

@beeonthego that makes sense.

[beeonthego]

@techknowlogick It took me hours to trace functions in different modules, search the JavaScript to see the potential impact, and finally come up with 3 lines of code. Time goes by faster in front of screens, big or small. Thank you for the hard work on the tests. We now have one less thing to worry about. Cheers!

[ghost]

@techknowlogick @beeonthego new test needs to get updated too, is good otherwise

[techknowlogick]

@beeonthego would you be able to backport this to the release/v1.5 branch? If not let me know and I'll do it.

[beeonthego]

@techknowlogick could you backport it to release/v1.5? thank you.