Proposal: limit api routes to access by token/basic auth only

[beeonthego]

As pointed out in issue #4703 (content deleted now), it is a csrf vulnerability to allow users to access api routes using session cookie without second check.

Currently the CSRF middleware only checks POST requests, and skips API routes. In addition, the reqToken function in routers/api/v1/api.go accepts all signed in users, including authenticated with session cookie. This means a script can make a POST/DELETE request to api routes while user is logged in (cookie present), without user's knowledge or consent. This is not desirable in multi tenant environment.

We have implemented some extra checks internally to limit api routes to access by token/basic auth only, and prohibit access by cookie when the route requires token. This does not affect public routes.

We have searched the code base and haven't found any client side Javascript code accessing protected api routes with cookie. But maybe there are cases we are not aware.
Is this the right approach? Will a PR like this break someone's code in the pipeline (if merged)?

[lafriks]

There is repo/org search in user dashboard that uses it. If that would be changed to not use API, I agree

[beeonthego]

Thanks for the insight. I thought I may have missed something. I did.

Here are the JS functions and api routes I have found in compiled JS file in user dashboard. The reqToken middleware was not present when registering routes. That is why I thought these were public routes. However I have just looked at the handler function and the access check is actually in the function.

searchUsers: /api/v1/users/search (public ?)
searchRepositories: /api/v1/repos/search (access check in function)
searchURL: /api/v1/repos/search (access check in function)
apiSettings: /api/v1/topics/search (access check in function)

Is it better to inject an access token to the dashboard page, and send the token back when making API requests? this will be similar to how csrf token is handled now. As long as we don't allow dashboard to load in cross site iFrame, it should be safe, as there is no content generated by other users in dashboard.

your thoughts?

[beeonthego]

I have just submitted PR #4840 to address a critical security issue reported in issue #4357 . It has passed all tests except two db tests. When token is enforced on API routes, reqToken will do its job as its name suggests, ask for a token. It will reject requests if token is not provided.

The current tests look for records and the records are not supposed to be there when token is enforced on API routes. Could someone review and rewrite the tests?

IMHO, API routes are designed for consumption by apps or remote servers, and private routes are protected with a key or token. Accepting authentication by cookie on API routes opens a can of worms.

POST requests on UI routes check the second factor of CSRF token. The current API routes are not covered by csrf handler. If someone has discovered a creative way to bypass markdown parser/sanitizer and run their code, they can make requests to the API routes using someone else's cookie. This is not an issue in a single user/team environment. things are different when there are multiple users.

[lunny]

closed by #4840