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

Colon ':' in urls causing problems #9

Closed
sii opened this issue Jun 16, 2016 · 14 comments
Closed

Colon ':' in urls causing problems #9

sii opened this issue Jun 16, 2016 · 14 comments
Labels

Comments

@sii
Copy link

sii commented Jun 16, 2016

Relating to this issue:
owncloud/core#23740

Tested on php 7.0.4 on Ubuntu 16.04.

The parse function in lib/functions.php has trouble with colon characters in the wrong place in the uri.
The parse function in turn uses php's parse_url function which is what is failing under certain (odd) circumstances.

If there is a colon character as the second to last character in a uri phps parse_url function fails to parse the uri. When phps parse_url function fails and returns false, sabres parse function also fails.
It's also worth noting that phps parse_url works if you move the colon anywhere else in the string.

Here's how this can be reproduced:

<?php
print "Works:\n";
var_dump(parse_url('/webdav/test3'));
print "\nFails:\n";
var_dump(parse_url('/webdav/test:3'));
?>

I'm not sure how or where this is most appropriate to fix.

Thanks

@evert
Copy link
Member

evert commented Jun 20, 2016

Hi!

Could you log in this as a php bug? If they acknowledge it we can think of building a workaround.

@evert evert added the bug label Jun 20, 2016
@evert
Copy link
Member

evert commented Jun 22, 2016

I think part of the problem is that :3 is probably being picked up as a port. Changing 3 to a cause it to succeed.

I think the fact that it starts with a / should have caused parse_url to have picked this up as a relative reference though, because / is not allowed in a uri scheme anyway.

@e-alfred
Copy link

Will this bug be solved anytime soon? Projects like Owncloud are still being affected by this:

owncloud/core#23740

@staabm
Copy link
Member

staabm commented Aug 10, 2016

@e-alfred feel free to provide a unit test and bugfix

@evert
Copy link
Member

evert commented Aug 10, 2016

The earlier comments still stand. Bug report to the php project would be a great first start.

@e-alfred
Copy link

e-alfred commented Aug 10, 2016

Here is a bug report about it: https://bugs.php.net/bug.php?id=55511

They say it is not a bug on their side.

From owncloud/core#25479.

@staabm
Copy link
Member

staabm commented Aug 10, 2016

sounds like a colon needs to be encoded when part of the url-path

@PVince81
Copy link

but then it was said that the spec was superseded and it doesn't need to be encoded any more:

The quoted spec has been superseded by RFC 3986 and that does allow colons in the path.

@PVince81
Copy link

@evert wait for a PHP-side fix or put a workaround in Sabre ?

@evert
Copy link
Member

evert commented Dec 1, 2016

I'm cool with a workaround in sabre/uri.

@PVince81
Copy link

PVince81 commented Dec 1, 2016

I tried @Xenopathic's regexp '/(?:[^[:ascii:]]|:)/u', and it seems to match colons fine and also non-ascii chars. I'll prepare a PR.

@PVince81
Copy link

PVince81 commented Dec 1, 2016

Argh, looks like this would break other cases where the URL is absolute: "http://..." would also encode the colon. And since we haven't parsed the URL yet we can't know which part is the path... Chicken-egg.

Another is then to do another regexp that only replaces the colon only if it appears after any first slash in the string.

What do you think ?

@staabm
Copy link
Member

staabm commented Dec 1, 2016

Another is then to do another regexp that only replaces the colon only if it appears after any first slash in the string.

not sure we support this, but colons can also appear as a username:password pair within the host portion.

@PVince81
Copy link

PVince81 commented Dec 4, 2016

PR here #12

evert added a commit that referenced this issue Dec 6, 2016
This is a new implementation of PHP's parse_url. It's likely not
perfect, but it catches specific cases that the built-in function does
not support.

Fixes Issue #9
Fixes Issue #11

@felixfbecker / @sii / @PVince81 Does this sufficiently solve your
problem?
evert added a commit that referenced this issue Dec 7, 2016
This is a new implementation of PHP's parse_url. It's likely not
perfect, but it catches specific cases that the built-in function does
not support.

Fixes Issue #9
Fixes Issue #11

@felixfbecker / @sii / @PVince81 Does this sufficiently solve your
problem?
@evert evert closed this as completed Dec 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants