Skip to content

Proxito: update CORS settings #10751

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Sep 20, 2023
Merged

Proxito: update CORS settings #10751

merged 6 commits into from
Sep 20, 2023

Conversation

humitos
Copy link
Member

@humitos humitos commented Sep 19, 2023

  • 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

- 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
Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add the origin header to vary if the origin is set per-origin instead of *, similar to

https://github.com/adamchainz/django-cors-headers/blob/24f9145c5845c7eaa4182356cb5957d6a9d5a907/src/corsheaders/middleware.py#L97-L99

see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Origin#cors_and_caching.

But again, we should be fine using *, the most important part here is that we aren't enabling this in .com yet.

@humitos
Copy link
Member Author

humitos commented Sep 19, 2023

We need to add the origin header to vary if the origin is set per-origin instead of *

I added the Vary header.

@ericholscher ericholscher added the PR: hotfix Pull request applied as hotfix to release label Sep 19, 2023
@stsewd
Copy link
Member

stsewd commented Sep 19, 2023

Pushed some changes:

  • Use * and allow this for .org only for now.
  • Use the header constants from the corsheader package.

Using * simplifies things since we don't need to add any custom logic or worry about caching, and also disallows requests that send credentials in cross site requests. I'm going to test this more on .com, but with this behavior we should be fine.

Next step I would like to add these headers per view instead of having it in the middleware, having it per-view it's more scoped, and we don't have to query the DB again.

@humitos
Copy link
Member Author

humitos commented Sep 20, 2023

Looks great! Thanks for jumping in into this PR 🚀

@humitos humitos merged commit a0285d6 into main Sep 20, 2023
@humitos humitos deleted the humitos/cors-update branch September 20, 2023 10:40
humitos added a commit that referenced this pull request Sep 20, 2023
* 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]>
humitos added a commit that referenced this pull request Sep 20, 2023
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: hotfix Pull request applied as hotfix to release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants