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

CORS problems around API #6154

Closed
humitos opened this issue Sep 9, 2019 · 10 comments
Closed

CORS problems around API #6154

humitos opened this issue Sep 9, 2019 · 10 comments
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code Needed: design decision A core team decision is required

Comments

@humitos
Copy link
Member

humitos commented Sep 9, 2019

We currently support JSONP on APIv2 in the Footer HTML endpoint only.

Supporting JSONP on APIv3 will allow to query the API from different custom domains.

Suggested at #6152

@humitos humitos added Improvement Minor improvement to code Accepted Accepted issue on our roadmap labels Sep 9, 2019
@stsewd
Copy link
Member

stsewd commented Sep 9, 2019

We shouldn't use jsonp for new code, we should use a CORS solution instead. And for public and read only endpoints.

@stsewd stsewd added the Needed: design decision A core team decision is required label Sep 9, 2019
@humitos
Copy link
Member Author

humitos commented Sep 9, 2019

@stsewd

We shouldn't use jsonp for new code

Why not?

we should use a CORS solution instead

How is that solution?

@stsewd
Copy link
Member

stsewd commented Sep 9, 2019

jsonp is to get around CORS, we are opening too much with that, I linked this in other of our projects (private). You can read the warning on this project for example https://jpadilla.github.io/django-rest-framework-jsonp/.

This lib is for handling CORS on django https://github.com/adamchainz/django-cors-headers and it's recommended from django rest framework https://www.django-rest-framework.org/topics/ajax-csrf-cors/#cors

@humitos
Copy link
Member Author

humitos commented Sep 11, 2019

Thanks @stsewd for the data!

We already have django-cors-headers installed and it seems that we have been using it already for Ads. Although, I think we can improve its settings and enable it on the API as well.

@stsewd stsewd changed the title Support JSONP on APIv3 CORS around API Sep 18, 2019
@stsewd stsewd changed the title CORS around API CORS problems around API Sep 18, 2019
@humitos
Copy link
Member Author

humitos commented Jun 29, 2021

I'm closing this issue because it's old and it does not adds value to the current situation. We have talked about CORS recently and we are working on the changes required.

@humitos humitos closed this as completed Jun 29, 2021
@jobisoft
Copy link

jobisoft commented Jul 3, 2021

This was closed without pointing to the current discussion. Where can I follow the progress?

I am not able to access the API from <projectslug>.readthedocs.io and also not from the custom domain setup for the project, and I find no public reference to CORS related hiccups besides this closed issue. Any help is appreciated.

@humitos
Copy link
Member Author

humitos commented Jul 5, 2021

Hi @jobisoft

This was closed without pointing to the current discussion. Where can I follow the progress?

The discussion wasn't public. We will update the documentation for our API to reflect these decisions soon.

I am not able to access the API from <projectslug>.readthedocs.io and also not from the custom domain setup for the project, and I find no public reference to CORS related hiccups besides this closed issue. Any help is appreciated.

What API endpoints are you using?

You should probably use the Proxied API that we designed for this particular case: instead of hitting readthedocs.org/api/ you should use <projectslug>.readthedocs.io/_/api/ (or just /_/api/ without caring where your RTD site is hosted --.readthedocs.io or custom domain). Please, let me know if this works for you.

@jobisoft
Copy link

jobisoft commented Jul 5, 2021

@humitos : Thanks for your response. The endpoint I am currently using is this:
https://readthedocs.org/api/v2/version/?project__slug=thunderbird-webextension-apis&active=true

The page itself is here:
https://thunderbird-webextensions.readthedocs.io/en/latest/

But I cannot seem to get the proxy endpoint running:
https://thunderbird-webextensions.readthedocs.io/_/api/v2/version/?project__slug=thunderbird-webextension-apis&active=true

I also tried v3 at
https://thunderbird-webextensions.readthedocs.io/_/api/v3/projects/thunderbird-webextension-apis/versions/

Could you point me to documentation regarding proxy API? It definitely would solve my issues.

Thanks for your time!

@stsewd
Copy link
Member

stsewd commented Jul 6, 2021

@jobisoft we only proxy APIs that are commonly used from docs sites, like search, embed and footer. We won't be able to proxy all endpoints as they depend on the codebase from the main site. We are going to add a better (public) footer api for users to use #8052

@jobisoft
Copy link

jobisoft commented Jul 6, 2021

Thanks for your support, really appreciated.

My use case is the popular version warning extension, which can add banners to a readthedocs page and inform users, if they are looking at an outdated version. The beauty of the API usage there: All old versions could add pointers to all other available versions without being manually updated each time a new version has been released.

I solved this now by manually including a static json file in my "latest" version, which holds the information I would get back from the API. Since all old versions now fetch the file from the "latest" version, they will always be up to date again and I only have to update my static json. Maybe that helps others in the meantime.

Looking forward to the upcoming changes in the footer API!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code Needed: design decision A core team decision is required
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants