handle s3rver launched with ip other than localhost #421
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
(for example docker environment), now we don't only test for ctx.hostname being 127.0.0.1, but any ip other when using docker, you would end up with an ip in for example 172.12.0.6, and vhost.js would have erroneously replaced ctx.path by `/undefined/$bucketname`
kherock
requested changes
Mar 25, 2019
lib/middleware/vhost.js
Outdated
| @@ -15,7 +17,7 @@ module.exports = () => | |||
| if (bucket) { | |||
| ctx.path = `/${bucket}${ctx.path}`; | |||
| } | |||
| } else if (ctx.hostname !== "localhost" && ctx.hostname !== "127.0.0.1") { | |||
| } else if (ctx.hostname !== "localhost" && !net.isIP(ctx.hostname)) { | |||
| // otherwise attempt to distinguish virtual host-style requests | |||
| ctx.path = `/${bucket}${ctx.path}`; | |||
There was a problem hiding this comment.
Thanks for this! Though this doesn't totally fix the underlying issue. I believe this line should also read
ctx.path = `/${ctx.hostname}${ctx.path}`Regarding tests, I'm realizing the existing test should be named should reach the server with a bucket . The behavior fixed in this PR should probably add a couple tests for vhost-style and IP-style requests.vhost subdomain
|
I was worried my description of the testing suite fixes needed might not have been too clear, so I went ahead and made the changes myself. Again, thanks for pointing this out! |
kherock
approved these changes
Mar 25, 2019
allan-simon
commented
Mar 25, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(for example docker environment), now we don't only test for ctx.hostname being 127.0.0.1, but any ip
other when using docker, you would end up with an ip in for example 172.12.0.6, and vhost.js would
have erroneously replaced ctx.path by
/undefined/$bucketname