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 folder name followed by a number creates fatal error #25479

Closed
tflidd opened this Issue Jul 14, 2016 · 20 comments

Comments

Projects
None yet
7 participants
@tflidd
Contributor

tflidd commented Jul 14, 2016

Steps to reproduce

  1. Create user1 and user2
  2. user1 shares folder share with user2
  3. user1 syncs share with desktop client
  4. user2 creates folder today 12:00

Expected behaviour

folder today 12:00 is synced or if colons are impossible in the OS it should be ignored.

Actual behaviour

Internal server error

Logs

Web server error log

2016/07/14 12:14:47 [error] 14916#0: *1128 FastCGI sent in stderr: "PHP message: PHP Fatal error:  Unsupported operand types in /var/www/owncloud/3rdparty/sabre/uri/lib/functions.php on line 181" while reading response header from upstream, client: 192.168.2.131, server: _, request: "PROPFIND /remote.php/webdav/share/today%2012:00 HTTP/1.1", upstream: "fastcgi://unix:/var/run/php5-fpm.sock:", host: "192.168.2.6"

reported by user on the forum: https://forum.owncloud.org/viewtopic.php?f=38&t=37741 (OC 9.0.3, client 2.2.2 on OS X), more details not known (yet)

I verified this on a rPI2:

Server configuration

Operating system: Raspbian 8

Web server: Nginx

Database: Mysql

PHP version: php 5.6

ownCloud version: 9.0.3

Updated from an older ownCloud or fresh install: 9.0.0

Where did you install ownCloud from: repo

Signing status (ownCloud 9.0 and above): good

It's a basic setup, no 3rd-party apps, no encryption, no proxy, no external storage.

Client 2.2.2 shows error (Ubuntu 16.04/OS X).

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 14, 2016

Member

Hmmmm, indeed. I thought it was php-fpm specific but it also happens in my mod_php instance.

Member

PVince81 commented Jul 14, 2016

Hmmmm, indeed. I thought it was php-fpm specific but it also happens in my mod_php instance.

@PVince81 PVince81 added this to the 9.0.5 milestone Jul 14, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 14, 2016

Member

It used to work in 8.1 => regression. I suspect it could be due to a Sabre lib update where URL handling might be different.

Will try and find where it broke.

Member

PVince81 commented Jul 14, 2016

It used to work in 8.1 => regression. I suspect it could be due to a Sabre lib update where URL handling might be different.

Will try and find where it broke.

@PVince81 PVince81 added the regression label Jul 14, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 14, 2016

Member

stable8.2 is fine, stable9 is broken

Member

PVince81 commented Jul 14, 2016

stable8.2 is fine, stable9 is broken

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 14, 2016

Member

Indeed, it's the update to Sabre 3 eacb24c

CC @DeepDiver1975

Let's see what needs adjusting

Member

PVince81 commented Jul 14, 2016

Indeed, it's the update to Sabre 3 eacb24c

CC @DeepDiver1975

Let's see what needs adjusting

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 14, 2016

Member

Hmmmm... I tried writing a test but the test will properly URL encode the colon.
However the desktop client doesn't.

Member

PVince81 commented Jul 14, 2016

Hmmmm... I tried writing a test but the test will properly URL encode the colon.
However the desktop client doesn't.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 14, 2016

Member

Reproducible with a non-encoded colon:

% curl -D - -X MKCOL http://admin:admin@localhost/owncloud/remote.php/webdav/colon%2010:14

It seems Sabre URI uses parse_url from PHP and that one doesn't seem to like colons.

Ohhhh, it's even weirder: it's only with colon followed by numbers !

This works:

% curl -D - -X MKCOL http://admin:admin@localhost/owncloud/remote.php/webdav/colon%2010:abc

Found this: https://bugs.php.net/bug.php?id=55511

Member

PVince81 commented Jul 14, 2016

Reproducible with a non-encoded colon:

% curl -D - -X MKCOL http://admin:admin@localhost/owncloud/remote.php/webdav/colon%2010:14

It seems Sabre URI uses parse_url from PHP and that one doesn't seem to like colons.

Ohhhh, it's even weirder: it's only with colon followed by numbers !

This works:

% curl -D - -X MKCOL http://admin:admin@localhost/owncloud/remote.php/webdav/colon%2010:abc

Found this: https://bugs.php.net/bug.php?id=55511

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 14, 2016

Member

This is with php5-5.6.23-1.1.x86_64

@DeepDiver1975 do you see this behavior with PHP7 ?

Member

PVince81 commented Jul 14, 2016

This is with php5-5.6.23-1.1.x86_64

@DeepDiver1975 do you see this behavior with PHP7 ?

@PVince81 PVince81 changed the title from Colon in folder name creates fatal error to Colon in folder name followed by a number creates fatal error Jul 14, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 14, 2016

Member

Two solutions:

  1. Patch + send PR upstream in Sabre URI https://github.com/fruux/sabre-uri/blob/master/lib/functions.php#L188 to also encode ":" when not encoded (CC @evert)

and/or

  1. Ask the clients to encode colons in URLs
Member

PVince81 commented Jul 14, 2016

Two solutions:

  1. Patch + send PR upstream in Sabre URI https://github.com/fruux/sabre-uri/blob/master/lib/functions.php#L188 to also encode ":" when not encoded (CC @evert)

and/or

  1. Ask the clients to encode colons in URLs
@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Jul 14, 2016

Member

@DeepDiver1975 do you see this behavior with PHP7 ?

let me try this ...

Member

DeepDiver1975 commented Jul 14, 2016

@DeepDiver1975 do you see this behavior with PHP7 ?

let me try this ...

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Jul 14, 2016

Member

same with php7

Member

DeepDiver1975 commented Jul 14, 2016

same with php7

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 14, 2016

Member

Can a regexp expert tell me how to adjust https://github.com/fruux/sabre-uri/blob/master/lib/functions.php#L188 to also include the colon ? All my attempts failed...

These never seem to match the colon:
'/[^[:ascii:]:]/u',, '/[^[:ascii:]\:]/u', '/[^[:ascii:]\\:]/u',

Member

PVince81 commented Jul 14, 2016

Can a regexp expert tell me how to adjust https://github.com/fruux/sabre-uri/blob/master/lib/functions.php#L188 to also include the colon ? All my attempts failed...

These never seem to match the colon:
'/[^[:ascii:]:]/u',, '/[^[:ascii:]\:]/u', '/[^[:ascii:]\\:]/u',

@guruz

This comment has been minimized.

Show comment
Hide comment
@guruz

guruz Jul 14, 2016

Contributor

don't include the colon, it's where to body stores 💩

Contributor

guruz commented Jul 14, 2016

don't include the colon, it's where to body stores 💩

@Xenopathic

This comment has been minimized.

Show comment
Hide comment
@Xenopathic

Xenopathic Jul 14, 2016

Member

@PVince81 The regexes you've posted are negated by the ^, try '/(?:[^[:ascii:]]|:)/u' instead (the?:` makes the group non-capturing, and it either matches a non-ASCII character or a colon)

Member

Xenopathic commented Jul 14, 2016

@PVince81 The regexes you've posted are negated by the ^, try '/(?:[^[:ascii:]]|:)/u' instead (the?:` makes the group non-capturing, and it either matches a non-ASCII character or a colon)

@evert

This comment has been minimized.

Show comment
Hide comment
@evert

evert Jul 14, 2016

This would also escape the colon in the schema.

I think that PHP bug should be reopened though with better clarification and lining up with what rfc3986 says about this.

If we can get the PHP guys to confirm that it is indeed a bug, I think that that would be a better basis for creating the workaround for sabre/uri.

Make sure that whatever change you're making doesn't create incorrect result when the intention of the uri is to actually include a tcp port.

evert commented Jul 14, 2016

This would also escape the colon in the schema.

I think that PHP bug should be reopened though with better clarification and lining up with what rfc3986 says about this.

If we can get the PHP guys to confirm that it is indeed a bug, I think that that would be a better basis for creating the workaround for sabre/uri.

Make sure that whatever change you're making doesn't create incorrect result when the intention of the uri is to actually include a tcp port.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Aug 17, 2016

Member

Any volunteer to report this upstream on the PHP bugtracker ? Possibly someone who already has an account there.

Member

PVince81 commented Aug 17, 2016

Any volunteer to report this upstream on the PHP bugtracker ? Possibly someone who already has an account there.

@PVince81 PVince81 modified the milestones: 9.0.6, 9.0.5 Sep 21, 2016

@PVince81 PVince81 modified the milestones: 9.0.7, 9.0.6 Oct 20, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Nov 30, 2016

Member

Workaround would be to have the clients url encode the colon.

But it seems indeed to be a PHP bug, see https://bugs.php.net/bug.php?id=55511

Member

PVince81 commented Nov 30, 2016

Workaround would be to have the clients url encode the colon.

But it seems indeed to be a PHP bug, see https://bugs.php.net/bug.php?id=55511

@PVince81 PVince81 modified the milestones: 9.0.8, 9.0.7 Nov 30, 2016

@PVince81 PVince81 self-assigned this Dec 1, 2016

@PVince81 PVince81 added the blue-ticket label Dec 1, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Dec 1, 2016

Member

Apparently the web UI properly encodes the colon, probably because we use encodeURIComponent which does that. That's why it does work in the web UI.

Member

PVince81 commented Dec 1, 2016

Apparently the web UI properly encodes the colon, probably because we use encodeURIComponent which does that. That's why it does work in the web UI.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Dec 1, 2016

Member

Instead of patching Sabre\URI maybe patching Sabre\Request::getPath instead to pre-encode the colon so that when we pass it to Sabre\URI::parse it will work...

Member

PVince81 commented Dec 1, 2016

Instead of patching Sabre\URI maybe patching Sabre\Request::getPath instead to pre-encode the colon so that when we pass it to Sabre\URI::parse it will work...

@PVince81 PVince81 referenced this issue Dec 6, 2016

Merged

Fix colon followed by number #26773

7 of 13 tasks complete
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Dec 6, 2016

Member

WIP PR here that adds a test to reproduce the issue: #26773

The final fix will require upgrading sabre/uri after sabre-io/uri#9 is released

Member

PVince81 commented Dec 6, 2016

WIP PR here that adds a test to reproduce the issue: #26773

The final fix will require upgrading sabre/uri after sabre-io/uri#9 is released

@JKawohl

This comment has been minimized.

Show comment
Hide comment
@JKawohl

JKawohl Dec 8, 2016

Contributor

This is also related: owncloud/enterprise#1692

Contributor

JKawohl commented Dec 8, 2016

This is also related: owncloud/enterprise#1692

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment