Build/fix no prefix #1089
Merged
Build/fix no prefix #1089
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
jamilbk
reviewed
Nov 3, 2022
apps/fz_common/lib/fz_net.ex
Outdated
| def to_complete_url(str) when is_binary(str) do | ||
| case URI.parse(str) do | ||
| %{host: nil, scheme: nil} -> "https://" <> str | ||
| %{scheme: nil} -> "https" <> str |
There was a problem hiding this comment.
Out of curiosity, when does this happen?
There was a problem hiding this comment.
In an edge case a user types //hostname
There was a problem hiding this comment.
I see -- maybe we shouldn't boot up if EXTERNAL_URL is invalid.
There was a problem hiding this comment.
Oh just did some testing, caddy doesn't support that so will remove it
Pull Request Test Coverage Report for Build 9ee2a9120174389ddc768d7a0ceeeca9fba63877-PR-1089
💛 - Coveralls |
* Fix schemeless external URLs; error on invalid ones * use different dockerfile for linux vs non-linux * Use conditional EXTERNAL_URL defaults * suppress empty warning * postgres volume location * Use inline Caddyfile
jamilbk
approved these changes
Nov 4, 2022
apps/fz_common/lib/fz_net.ex
Outdated
| def to_complete_url(str) when is_binary(str) do | ||
| case URI.parse(str) do | ||
| %{host: nil, scheme: nil} -> "https://" <> str | ||
| %{scheme: nil} -> "https" <> str |
There was a problem hiding this comment.
I see -- maybe we shouldn't boot up if EXTERNAL_URL is invalid.
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.
Fixes #1086 #1073
For #1073 the problem was docker doing dnat changing the destination IP and without a HOST header that would cause a cert problem (No cert for the dnatted IP) and a no site route (the route was generated for the original destination IP not the dnatted one). The fix was to put the container in
network_mode: "host"so no dnat ocurred.For #1086 the the problem was that if you didn't add
https://to theEXTERNAL_URLit'd cause a bug where theiporhostwas taken as a path instead ofhostwhen usingURI.parsewhich in turn would cause the Route helpers to generate the static routes incorrectly. The solution is trying to detect if it's a correctly generated url and if not add thehttps://.