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

Proxito: redirect http->https for public domains #10142

Merged
merged 4 commits into from
Mar 14, 2023
Merged

Proxito: redirect http->https for public domains #10142

merged 4 commits into from
Mar 14, 2023

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Mar 13, 2023

No description provided.

@stsewd stsewd requested a review from a team as a code owner March 13, 2023 18:53
@stsewd stsewd requested a review from humitos March 13, 2023 18:53
@stsewd stsewd requested a review from ericholscher March 13, 2023 18:54
Comment on lines 268 to 275
if (
unresolved_domain.is_from_public_domain
or unresolved_domain.is_from_external_domain
):
if settings.PUBLIC_DOMAIN_USES_HTTPS and not request.is_secure():
# Redirect HTTP -> HTTPS (302) for public domains.
return RedirectType.http_to_https

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should probably move the https redirects to the middleware, since we want all traffic to redirect to https, not just doc pages.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

But we need to take custom domains into consideration, we can't redirect everything

Copy link
Member Author

Choose a reason for hiding this comment

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

or at leas in .org, in .com we were redirecting all custom domains.

Copy link
Member

Choose a reason for hiding this comment

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

I think now that we have automatic SSL configuration, we should be able to auto-redirect to SSL everywhere? I think we probably want to migrate to only serving on SSL, so this seems like a reasonable step to take? But I agree, we want to make sure we're taking into account the domain.ssl variable so we don't break things in the transition.

Copy link
Member

Choose a reason for hiding this comment

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

We can probably inherit from the Django one and modify it a little to consider domain.ssl. I like re-using starndard Django stuffs :)

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.

Seems like we have some additional design questions here. I'm fine doing this just for proxito for now, since doing it everywhere might be a larger decision, and have weird side outcomes?

readthedocs/proxito/views/serve.py Outdated Show resolved Hide resolved
readthedocs/proxito/views/serve.py Outdated Show resolved Hide resolved
Comment on lines 268 to 275
if (
unresolved_domain.is_from_public_domain
or unresolved_domain.is_from_external_domain
):
if settings.PUBLIC_DOMAIN_USES_HTTPS and not request.is_secure():
# Redirect HTTP -> HTTPS (302) for public domains.
return RedirectType.http_to_https

Copy link
Member

Choose a reason for hiding this comment

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

@stsewd
Copy link
Member Author

stsewd commented Mar 13, 2023

Seems like we have some additional design questions here. I'm fine doing this just for proxito for now, since doing it everywhere might be a larger decision, and have weird side outcomes?

The current nginx implementation redirects all requests (from public domains for .org and public and custom domains on .com), not just docs pages #10144

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.

This looks good to me 👍

@stsewd stsewd merged commit 830b899 into main Mar 14, 2023
@stsewd stsewd deleted the https-redirect branch March 14, 2023 23:20
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.

3 participants