Skip to content
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

Allow all /api/v2/ CORS if the Domain is known #4880

Merged
merged 2 commits into from
Dec 11, 2018

Conversation

humitos
Copy link
Member

@humitos humitos commented Nov 8, 2018

Currently, if any documentation project tries to use our APIv2 from a custom domain by doing an AJAX request, it will be blocked because of CORS.

This PR adds a new checking for this by allowing CORS on all the URLs that start with /api/v2/ and the HTTP_ORIGIN header is a known Domain in our platform.

Raised at humitos/sphinx-version-warning#23

# Don't do domain checking for APIv2 when the Domain is known
if request.path_info.startswith('/api/v2/'):
domain = Domain.objects.filter(domain__icontains=host)
if domain.exists():
Copy link
Member Author

Choose a reason for hiding this comment

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

Do we want to allow to access the API no matter which domain the request comes from?

@codecov
Copy link

codecov bot commented Nov 8, 2018

Codecov Report

Merging #4880 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4880      +/-   ##
==========================================
- Coverage   76.64%   76.63%   -0.02%     
==========================================
  Files         158      158              
  Lines       10054    10062       +8     
  Branches     1271     1271              
==========================================
+ Hits         7706     7711       +5     
- Misses       2007     2011       +4     
+ Partials      341      340       -1
Impacted Files Coverage Δ
readthedocs/core/signals.py 82% <100%> (-4.67%) ⬇️
readthedocs/config/parser.py 87.5% <0%> (-12.5%) ⬇️
readthedocs/restapi/serializers.py 97.43% <0%> (-0.1%) ⬇️
readthedocs/config/config.py 98.62% <0%> (-0.03%) ⬇️
readthedocs/doc_builder/config.py 91.89% <0%> (ø) ⬆️
readthedocs/doc_builder/environments.py 87.33% <0%> (ø) ⬆️
readthedocs/projects/models.py 85.44% <0%> (+0.34%) ⬆️
readthedocs/projects/admin.py 91.5% <0%> (+0.42%) ⬆️
readthedocs/projects/tasks.py 71.02% <0%> (+0.46%) ⬆️

# Don't do domain checking for APIv2 when the Domain is known
if request.path_info.startswith('/api/v2/'):
domain = Domain.objects.filter(domain__icontains=host)
if domain.exists():
Copy link
Member

Choose a reason for hiding this comment

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

I'm -0 on allowing all API calls to all projects from all Domain's. We should still be mapping the domain -> project, so I think we can just add /api/v2/ to WHITELIST_URLS?

Copy link
Member Author

Choose a reason for hiding this comment

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

How do you connect the Domain -> Project for this case? When calling /api/v2 we don't pass the project= query string.

Copy link
Member Author

Choose a reason for hiding this comment

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

One thing we can do here is just allow safe methods for /api/v2 endpoints: GET and HEAD.

@humitos humitos requested a review from a team December 3, 2018 11:16
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

Works for me. We should include more thinking and research around CORS when we do https://github.com/rtfd/readthedocs-ops/pull/441 and document our approach.

@humitos humitos merged commit 5907064 into master Dec 11, 2018
@humitos humitos deleted the humitos/cors/allow-api-known-domains branch December 11, 2018 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants