http: avoid generating double slashes in url#5325
Merged
pks-t merged 1 commit intolibgit2:masterfrom Jan 6, 2020
Merged
Conversation
pks-t
reviewed
Dec 13, 2019
Member
pks-t
left a comment
There was a problem hiding this comment.
I think this makes a lot of sense, so I'm 👍
| size_t pathlen = strlen(path); | ||
| /* If path already ends in /, remove the leading slash from service_url */ | ||
| if (pathlen > 0 && path[pathlen - 1] == '/') | ||
| service_url++; |
Member
There was a problem hiding this comment.
if (git__suffixcmp(path, "/"))
service_url++;
We could also check if service_url actually starts with a "/" as kind of a sanity check even though currently this is always the case.
if (git__suffixcmp(path, "/") && git__prefixcmp(s->service_url, "/"))
service_url++;
d2723e0 to
48e0397
Compare
Contributor
Author
|
I can't tell why the CI failed. It appears it may have to do with fuzzing leaking a fd, which I think is unlikely to be caused by this PR, but I'm not sure. |
josharian
added a commit
to josharian/libgit2
that referenced
this pull request
Dec 13, 2019
Some servers leave the query parameters intact in the Location header when responding with a redirect. The service_suffix removal check as written assumed that the server removed them. Handle both cases. Along with PR libgit2#5325, this fixes libgit2#5321.
Prior to this change, given a remote url with a trailing slash, such as http://localhost/a/, service requests would contain a double slash: http://localhost/a//info/refs?service=git-receive-pack. Detect and prevent that. Updates libgit2#5321
48e0397 to
05c1fb8
Compare
Contributor
Author
|
Looks like the CI failure was a flake; I re-ran with the same code and it passed. |
Member
|
Yep, we have tests that actually hit the network and are thus a bit flaky. Apologies for the inconvenience. |
pks-t
approved these changes
Jan 6, 2020
Member
pks-t
left a comment
There was a problem hiding this comment.
Looks good to me, thanks a lot for this fix!
josharian
added a commit
to josharian/git2go
that referenced
this pull request
Jan 8, 2020
to pick up libgit2/libgit2#5325 Merge commit '69df156d02d5733bcb981960fde9b4d8e185ff73' into v28
josharian
added a commit
to josharian/libgit2
that referenced
this pull request
Jan 9, 2020
Some servers leave the query parameters intact in the Location header when responding with a redirect. The service_suffix removal check as written assumed that the server removed them. Handle both cases. Along with PR libgit2#5325, this fixes libgit2#5321. There are two new tests. The first already passed; the second previously failed.
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
3 participants
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.
Prior to this change, given a remote url with a trailing slash,
such as http://localhost/a/, service requests would contain a
double slash: http://localhost/a//info/refs?service=git-receive-pack.
Prevent that.
I was unable to test the winhttp.c changes locally. I trust that CI will cover that.
Updates #5321
cc @ethomson