Redirect HTTP -> HTTPS for custom domains #6882
Conversation
- Only redirect if the appropriate flag is set on the Domain (default is not to redirect) - Use a 302 redirect for now. Ideally this is a 301 redirect, but redirecting other people's domains with a 301 is always a bit risky.
readthedocs/proxito/middleware.py
Outdated
| if domain.https and not request.is_secure(): | ||
| # Redirect HTTP -> HTTPS (302) for this custom domain | ||
| log.debug('Proxito CNAME HTTPS Redirect: host=%s', host) | ||
| return redirect('https://%s%s' % (host, request.get_full_path())) |
There was a problem hiding this comment.
we should use f-strings or .format here
readthedocs/proxito/middleware.py
Outdated
| if domain.https and not request.is_secure(): | ||
| # Redirect HTTP -> HTTPS (302) for this custom domain | ||
| log.debug('Proxito CNAME HTTPS Redirect: host=%s', host) | ||
| return redirect('https://{}{}'.format(host, request.get_full_path())) |
There was a problem hiding this comment.
If possible, I'd like to see a custom RTD header on here so we know where this request comes from similar to
Perhaps X-RTD-HTTPS-Redirect
There was a problem hiding this comment.
I think I'll do X-RTD-Redirect and the value will be https. I might use the same header with the value canonicalize when doing the domain canonicalization.
|
Hold off on merging this. I'm considering a refactor here that might make it easier to canonicalize the domain as well. I might move the actual redirection to |
|
I put an alternative implementation (based on this one) in #6905. That one uses the regular resolver code to actually perform the redirect and I think I like that solution a bit better overall. I'm open to feedback on either/both though. If we want to roll out the HTTP -> HTTPS change separately from canonicalizing the domain, we can merge this, roll this out, and merge that one in the future. |
|
I'm going to close this in favor of #6905 which seems closer to rolling out. |
Fixes #4641