Fix http redirects #365
Fix http redirects #365
Conversation
|
Thats great, thanks @pietroalbini and @HeroicKatora. We have been aware of the issue since we started using CloudFront but we weren't able to provide a solution for it. This is so useful. |
|
like I said in the discord; I am only skeptical about dotenv, we are already doing same thing with systemd, it's reading our environment variables from (~/.cratesfyi.env) and passing it to our process. So I don't think we need dotenv. |
All the docs.rs redirects are absolute, including the protocol and the host name. This is fine if docs.rs is exposed directly on the Internet, but the production instance sits behind CloudFront, which terminates TLS connections. This means the application thinks it's serving HTTP requests instead of HTTPS ones, and it issues wrong redirects. This fixes the issue by taking the Cloudfront-Forwarded-Proto header into account when building the redirect URL.
pietroalbini
force-pushed
the
fix-http-redirects
branch
from
7a0b8ed to
b4de77c
Compare
|
Moved the dotenv commit in its own PR: #366 I still think it's useful but we don't need to block this PR on it. |
|
Awesome, thanks again! |
|
|
||
| // Only include the port if it's needed | ||
| let port = req.url.port(); | ||
| if port == 80 { |
There was a problem hiding this comment.
Shouldn't this be 80 or 443?
There was a problem hiding this comment.
Uh, probably yes, but it shouldn't cause any issue with our production setup, so fixing it is not that high priority.
There was a problem hiding this comment.
I think only 80 is fine since CloudFront is sending requests to 80.
All the docs.rs redirects are absolute, including the protocol and the host name. This is fine if docs.rs is exposed directly on the Internet, but the production instance sits behind CloudFront which terminates TLS connections. This means the application thinks it's serving HTTP requests instead of HTTPS ones, and it issues wrong redirects.
This PR fixes the issue by taking the Cloudfront-Forwarded-Proto header into account when building the redirect URL. This also adds
dotenvto the project. It's not necessary but it's handy when you want to set environment variables and you aren't in the vagrant VM.Fixes #363, and thanks @HeroicKatora for investigating this!