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

Switch URL protocol for resource links to HTTPS #1

Closed
dpilafian opened this issue Apr 21, 2020 · 9 comments
Closed

Switch URL protocol for resource links to HTTPS #1

dpilafian opened this issue Apr 21, 2020 · 9 comments
Labels
bug Something isn't working fixed

Comments

@dpilafian
Copy link

The parent project has an issue filed to make the resource links the same protocol as the request.

Change URL protocol in response data based on whether the request was made over HTTPS:
phalt#66

Since the swapi.dev server uses HTTPS (it even redirects HTTP to HTTPS) and the world has pretty much switched over to HTTPS, it probably makes sense to just do a simple straight switch to HTTPS. There's little value in making the response intelligent based on the protocol of the request.

Currently the next field in responses uses the HTTP protocol:

GET https://swapi.dev/api/vehicles/?format=json

{
   "count": 39,
   "next": "http://swapi.dev/api/vehicles/?page=2&format=json",
   "previous": null,
   "results": [
      {
         "name": "Sand Crawler",
         "model": "Digger Crawler",
         "manufacturer": "Corellia Mining Corporation",
         "cost_in_credits": "150000",
         "length": "36.8 ",
         "max_atmosphering_speed": "30",

Following the next link can result in errors like:

Fetch API cannot load http://swapi.dev/api/vehicles/?page=2&format=json due to access control 

This problem would be resolved by using HTTPS in the next field:

   "next": "https://swapi.dev/api/vehicles/?page=2&format=json",
dpilafian added a commit to dna-engine/data-dashboard that referenced this issue Apr 21, 2020
@neoreddog
Copy link

neoreddog commented May 4, 2020

I think this is the case for any url provided through the API.

Tested with the following requests:
https://swapi.dev/api/people/
https://swapi.dev/api/people/?page=1
https://swapi.dev/api/planets/
https://swapi.dev/api/films/
https://swapi.dev/api/starships/

@EmmaDawsonDev
Copy link

I am running into the same problem. I first make a fetch request to retrieve page 1 data which works fine but then when I try to use the urls contained within that data to make additional fetch requests I'm getting failures because they are all http instead of https. Would be great if this could be updated.

@Juriy
Copy link
Owner

Juriy commented Jun 16, 2021

Help would be highly appreciated with this issue. In my current setup I have nginx in front of SWAPI application. NginX is terminating SSL, so Django that runs SWAPI thinks that it is requested via HTTP. That's why (I think) the links appear broken. The problem is - I have very little knowledge of Django and I am not sure if I can configure protocol for the links in JSON responses explicitly.

@Juriy Juriy added bug Something isn't working help wanted Extra attention is needed labels Jun 16, 2021
@sploitfaze
Copy link

sploitfaze commented Jun 17, 2021

Hello, @Juriy. I am really glad to see your activity on this project. :)

To make Django use HTTPS instead of HTTP, set HTTPS=on in environment variables.

@Juriy
Copy link
Owner

Juriy commented Jun 17, 2021

@leonidpodriz thanks for the hint! The thing is that I want Django to stay on HTTP, since it is NGINX that is dealing with HTTPS and SSL termination. Then it acts as a reverse proxy, and passes the request to Django app over plain HTTP. So it is fine for Django to be on HTTP ( it is not exposed externally ) but somehow we need to let it know that it is behind a reverse proxy, and the real protocol that clients use is HTTPS.

image

This StackOverflow question seems to be relevant. I will experiment with USE_X_FORWARDED_HOST later tonight.
https://stackoverflow.com/questions/32631903/django-rest-framework-reverse-relationship-breaks-when-behind-proxy

@sploitfaze
Copy link

@Juriy Well, you're right. But after HTTPS=on Django will stay on plain HTTP and reverse proxy will works correct.

Proofs from local testing:
Screenshot 2021-06-17 at 17 28 12

This environment variable is uses only to build absolute URLs.
I had search where HTTPS variable is using and not found more places despite this:
https://github.com/django/django/blob/b626c289ccf9cc487f97d91c2a45cac096d9d0c7/django/http/request.py#L135

Django has a few more ways to make it return HTTPS URLs, but this way is the easiest and impact only one function (see above), not the whole application.

@Juriy
Copy link
Owner

Juriy commented Jun 17, 2021

Thank you for the hint, let me test it on prod-like first and I'll update prod right after.

@sploitfaze
Copy link

sploitfaze commented Jun 17, 2021

Hey, don't spend your time for my "hint". :) I got wrong, because this won't be work when Nginx connects to Django thorough WSGI.

In my environment, rewritten "SWAPI" handle HTTPS well. I think it is related to this: https://docs.djangoproject.com/en/3.2/releases/1.11.22/

I propose the next solution:
To the Nginx configuration add proxy_set_header 'X_FORWARDED_PROTO' 'https'; That's works. :)

@Juriy Juriy removed the help wanted Extra attention is needed label Jun 17, 2021
@Juriy
Copy link
Owner

Juriy commented Jun 17, 2021

Fixed.

Basically I was overthinking it a little 🙂
For the historical purposes: original configuration already had a config line that would make Django realize that request was originally secure and it is behind the reverse proxy:

SECURE_PROXY_SSL_HEADER = ('HTTP_X_FORWARDED_PROTO', 'https')

https://docs.djangoproject.com/en/1.8/ref/settings/#secure-proxy-ssl-header

All I had to do is make Nginx send this header. Now this was very close to what @leonidpodriz suggested, except for a minor difference in syntax for config.

location /api {
    proxy_set_header X-Forwarded-Proto $scheme;
    proxy_pass "http://localhost:8000/api";
}

Thanks for help!

@Juriy Juriy closed this as completed Jun 17, 2021
@Juriy Juriy added the fixed label Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed
Projects
None yet
Development

No branches or pull requests

5 participants