fix: setup wizard mangles the hostname and probes the wrong url - #759
Open
solracsf wants to merge 3 commits into
Open
fix: setup wizard mangles the hostname and probes the wrong url#759solracsf wants to merge 3 commits into
solracsf wants to merge 3 commits into
Conversation
… url `ltrim($base, 'http://')` passes a character list, not a prefix, so it strips any leading h, t, p, : or / of whatever follows as well. `http://push.example.com` came out as `https://ush.example.com`, and that url is what gets saved as `base_endpoint` and handed to the clients. The same method then probed `$base`, the plain http url it already had, rather than the https one it had just built, and returned `$base` from both branches of the try/catch. So the https preference the method exists for never applied on a first call, and on a second call it returned the cached (mangled) https url instead, making `getBaseUrl()` answer differently depending on how many times it had been called within the same command. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
`toHttps` was tested but `getBaseUrl` was not, which left the actual bug this branch fixes, probing the wrong url and answering differently on a second call, with no test pinning it. Drive it through `getProxiedBase()` with a mocked client that records what it was asked for. Four of the six assertions fail against master, including one that catches both halves at once: the first call returns `http://push.example.com/push` and the second `https://ush.example.com/push`. `str_starts_with` is case sensitive, so `HTTP://host` fell through to the default arm and came out as `https://HTTP://host`, and a protocol relative `//host` became `https:////host`. Compare the scheme case insensitively and give `//host` its own arm. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
`toHttps` was four `str_starts_with` arms doing what an anchored optional scheme group does in one line, and it now normalises an uppercase scheme rather than passing it through, which is what the caller wanted anyway. That also lets `getBaseUrl` drop its own `str_starts_with($base, 'https://')`: comparing the converted url against the original says the same thing and stays correct for a scheme in any case. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
ltrim($base, 'http://')passes a character list, not a prefix, so it also strips anyleading h, t or p of the host:
http://push.example.combecomeshttps://ush.example.com. That url is what gets saved asbase_endpointand handed toclients.
The same method probed
$base, the http url it already had, rather than the https oneit had just built, and returned
$basefrom both branches of the try/catch. So thehttps preference never applied on a first call, and a second call returned the cached
mangled url instead.
SetupWizardBaseUrlTestdrives this throughgetProxiedBase()with a mocked client.Four of its six assertions fail on master.