Proxito: add CORS headers only on public versions #10737
Conversation
Add `Access-Control-Allow-Origin: *` and `Access-Control-Allow-Methods: OPTIONS, GET` HTTP headers when serving a PUBLIC version. This is required for DocDiff addon to be able to download the original version of the page and compare the DOMs to show the visual differences. Closes readthedocs/addons#93
readthedocs/proxito/middleware.py
Outdated
| privacy_level=PUBLIC, | ||
| ).exists() | ||
| if allow_cors: | ||
| response.headers["Access-Control-Allow-Origin"] = "*" |
There was a problem hiding this comment.
Instead of using "*" here we could have some logic here to only support *.readthedocs.build
Limiting the possible Access-Control-Allow-Origin values to a set of allowed origins requires code on the server side to check the value of the Origin request header, compare that to a list of allowed origins, and then if the Origin value is in the list, set the Access-Control-Allow-Origin value to the same value as the Origin value.
(from https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Origin)
There was a problem hiding this comment.
I don't think we require this since these versions are all public, but we can implement it in a following iteration if we consider.
There was a problem hiding this comment.
If we are going to enable this on .com, I'd prefer if we start the most restricted we can.
There was a problem hiding this comment.
Agreed -- we definitely want to err on the side of caution here given the security considerations that have kept this disabled in the past.
There was a problem hiding this comment.
We should add these headers to the response that serve files from docs only, proxied APIs shouldn't have these headers, specially on .com.
We probably need a way for each view to opt-in into adding these headers. But think that only a couple of views pass down the path_project_slug/path_version_slug attributes, and we aren't setting the access-control-allow-credentials header here, so should probably be safe.
readthedocs/proxito/middleware.py
Outdated
| privacy_level=PUBLIC, | ||
| ).exists() | ||
| if allow_cors: | ||
| response.headers["Access-Control-Allow-Origin"] = "*" |
There was a problem hiding this comment.
Agreed -- we definitely want to err on the side of caution here given the security considerations that have kept this disabled in the past.
| privacy_level=PUBLIC, | ||
| ).exists() | ||
| if allow_cors: | ||
| response.headers["Access-Control-Allow-Origin"] = "*.readthedocs.build" |
There was a problem hiding this comment.
@ericholscher I don't think you can use a wildcard within the origin, see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Origin#directives.
Only a single origin can be specified. If the server supports clients from multiple origins, it must return the origin for the specific client making the request.
This type of wildcard needs to be implemented in the server, something like:
if is_valid_origing(origin):
response.headers["Access-Control-Allow-Origin"] = originThere was a problem hiding this comment.
I think having this to * is fine for .org (github and gitlab pages have it to this value, for public pages at least). Maybe we can just start by allowing CORS on .org only.
- Only add CORS headers on community site - Explicit host on `Access-Control-Allow-Origin` - Only query the database if the host ends with `RTD_EXTERNAL_VERSION_DOMAIN` - Add more tests Continuation of #10737
* Proxito: update CORS settings - Only add CORS headers on community site - Explicit host on `Access-Control-Allow-Origin` - Only query the database if the host ends with `RTD_EXTERNAL_VERSION_DOMAIN` - Add more tests Continuation of #10737 * Add `Vary: Origin` header * Use Django internals to patch `Vary` header. * Use the `Origin` header from request to check for the allowed domain * Update tests to use `origin` header * Allow cross-origin requests for public versions' docs --------- Co-authored-by: Santos Gallegos <[email protected]>
* Proxito: update CORS settings - Only add CORS headers on community site - Explicit host on `Access-Control-Allow-Origin` - Only query the database if the host ends with `RTD_EXTERNAL_VERSION_DOMAIN` - Add more tests Continuation of #10737 * Add `Vary: Origin` header * Use Django internals to patch `Vary` header. * Use the `Origin` header from request to check for the allowed domain * Update tests to use `origin` header * Allow cross-origin requests for public versions' docs --------- Co-authored-by: Santos Gallegos <[email protected]>
* Proxito: update CORS settings - Only add CORS headers on community site - Explicit host on `Access-Control-Allow-Origin` - Only query the database if the host ends with `RTD_EXTERNAL_VERSION_DOMAIN` - Add more tests Continuation of #10737 * Add `Vary: Origin` header * Use Django internals to patch `Vary` header. * Use the `Origin` header from request to check for the allowed domain * Update tests to use `origin` header * Allow cross-origin requests for public versions' docs --------- Co-authored-by: Santos Gallegos <[email protected]>
Add
Access-Control-Allow-Origin: *andAccess-Control-Allow-Methods: OPTIONS, GETHTTP headers when serving a PUBLIC version.This is required for DocDiff addon to be able to download the original version of the page and compare the DOMs to show the visual differences.
Closes readthedocs/addons#93
Closes #8769