Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

URLs with double slash at the start of the path fail to be requested #231

Closed
dd32 opened this issue Aug 18, 2016 · 6 comments

Comments

Projects
None yet
4 participants
@dd32
Copy link
Collaborator

commented Aug 18, 2016

Requests_IRI currently rejects a url such as http://example.org//path/ as being valid, but accepts http://example.org/path//path/.

This seems to stem from this bit of code: https://github.com/rmccue/Requests/blob/master/library/Requests/IRI.php#L691-L694

I haven't looked at it close enough to determine if it's a logic error in the if statement, or if it's an edgecase it's specifically not attempting to support. At first I thought it was trying to catch the case where the scheme separator was picked up as the path component.

This was reported as a regression in WordPress 4.6, as we previously handled this scenario before we switched to Requests.

@rmccue

This comment has been minimized.

Copy link
Owner

commented Aug 30, 2016

I'm unsure why this code exists here; I suspect it's an intricacy in the IRI spec. I didn't write this code however, this is ported from SimplePie's IRI parser, which was written by @gsnedders. I'm wondering if it's meant to be caught at a higher level.

We can probably fix this with no real issues.

@rmccue

This comment has been minimized.

Copy link
Owner

commented Aug 30, 2016

Tracked back to this commit.

@orange-themes

This comment has been minimized.

Copy link

commented Aug 30, 2016

Seems like not only double path, we get the same error, when we try to access http://www.geoplugin.net/json.gp?ip=8.8.8.8

The response is like http://grab.by/Sq3i and this happened direct after wp update to 4.6

@rmccue

This comment has been minimized.

Copy link
Owner

commented Aug 30, 2016

@orange-themes I think this is unrelated; might be #232 if the server you're connecting to checks referer headers.

@gsnedders

This comment has been minimized.

Copy link

commented Aug 30, 2016

@rmccue Pretty sure I just misread the RFC, because this should totally work. More seriously, we probably just want to follow this given it roughly matches what browsers do.

@rmccue

This comment has been minimized.

Copy link
Owner

commented Aug 30, 2016

@gsnedders Thanks for the confirmation; my guess is it was just a double-check on higher-level normalisation. Will fix now :)

@rmccue rmccue closed this in #240 Sep 18, 2016

@rmccue rmccue added this to the 1.7 milestone Sep 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.