Improve CORS configuration #6168
Conversation
The goal of this commit is to allow access to our resources from different hosts: * everything coming from `*.readthedocs.io` * everything hitting any URL under `/api/` if it's safe method, no matter the host where the request comes from Compared with the previous implementation, this reduces the number of queries to our database since we are not checking that the host where the request was made is a known Domain for us. Besides, this opens access to `/api/v3` as well.
humitos
force-pushed
the
humitos/cors-settings
branch
from
b53569d to
731b4cd
Compare
|
I didn't test this locally, but I have some concerns about removing the domain check. So, api v2 doesn't require authentication, because it only lists public stuff. But for api v3 we require auth because it lists private stuff. So, I think for endpoints that return private things, we should only allow if the domain is from a known origin. |
|
|
||
| return False | ||
| return all([ | ||
| request.method in SAFE_METHODS, |
There was a problem hiding this comment.
We define this setting CORS_ALLOW_METHODS, do we really need to check this again here?
There was a problem hiding this comment.
I think these settings are confusing to me as well.
I understand that CORS_ALLOW_METHODS is used for the pre-flight request (OPTIONS) and it's to add the header Access-Control-Allow-Methods so after the browser can decide if will execute the final request or block it.
The signal is checked independently of these settings. If it returns True the origin is allowed.
In the end, CORS_ALLOW_METHODS is not considered when allowing it or denying it, it's just to "inform" the browser on the pre-flight request.
This chunk of code is useful to understand this behavior: https://github.com/adamchainz/django-cors-headers/blob/master/corsheaders/middleware.py#L123-L146
Hopefully users aren't putting their private auth tokens into public webpages to make authenticated requests. |
@davidfischer but don't we use their session if they are logged in? |
|
Currently, not for APIv3: However, if we ever do that, we should be very careful for CORS. |
|
@stsewd @davidfischer good questions were risen here! Let's see if we are all in the same page with these changes. Authentication via session is not enabled in APIv3 and it was only considered for very specific endpoints in case it would needed (i.e. Ads if ever migrated to v3): https://docs.readthedocs.io/en/latest/api/v3.html#session I'm not planning to enable session authentication on v3 for now and I do not see a good reason to do it at this point. Do we have one that I'm missing?
This is a good point. Although, as David said, people won't use their own private token to build a public webpage. However, we plan to implement scope-based token at some point but we will still want to allow these requests coming from "unknown" (not in our DB) domains --again, the example of a landing page. This will give the user the ability to expose only the things they want. I do think that we should not allow not safe methods or we can allow them if they are a known combination of domain+project. @davidfischer there is still something that I'm not 100% sure to be right. This is the Ads endpoint at |
|
@humitos maybe do we need public endpoints for api v3? People using this for landing pages, not sure if they want to put a personal token there. And I think we use sessions for api v2, if the user has a session we return more things, like private repos, well that doesn't matter much now that privacy levels are deprecated. |
Some time ago, we decided that we didn't want to expose public endpoints for different reasons. Although, this will be solved by scope-based tokens. People will use a specific token with specific permissions ("read active versions" or just "read everything from a project" --we can play with the granularity here) for this, not personal tokens. Summarizing, "the landing page" (in another domain) use case is still not supported on APIv2 nor APIv3.
This is a good point to check. Maybe it's not too important on community, but probably it's something we need to take care on corporate. We may don't want to allow CORS all over APIv2, but only in APIv3? Or we want to disable CORS on APIv2 for corporate? |
|
@humitos with scoped toke permissions, we still ask the user to put that token to the public, so anyone could use it. Not sure if that is really important, but it feels weird exposing a token like that, even just for read/public actions. Or we can ask users to enter the domains where they are going use the read/public endpoints. So they can only be used for that site only. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
humitos
added
Needed: design decision
|
We are now allowing Session Authentication on APIv3 (this was mainly added for new templates). I'm not sure how risky it is to allow safe/unsafe methods from known/unknown domains. Even if we known the domain and it's a safe method there could be a malicious Javascript that fetches private data (in .com specifically) without the user concern. |
|
Closing this PR. We have changed our CORS setting recently. |
The goal of this commit is to allow access to our resources from different hosts:
*.readthedocs.io/api/if it's safe method, no matter the host where the request comes fromCompared with the previous implementation, this reduces the number of queries to our database since we are not checking that the host where the request was made is a known Domain for us.
Besides, this opens access to
/api/v3as well.Be careful when reviewing this PR since it may be opening our API too widely. I'd appreciate comments on this.
Closes #6154