Prevent wrong matches in getRelativePath #16479

PVince81 · 2015-05-20T16:33:10Z

Before this fix, the root "/files" with path "/files_trashbin" would
return "_trashbin" as relative path...

Please review @icewind1991 @schiesbn

I'm still having doubts whether this quickfix is safe or will break other parts of OC...

I raised #16478 to look into a more extensive fix.

PVince81 · 2015-05-20T16:34:12Z

The alternative would be to just detect these cases and log a warning and carry on... at least for 8.1.
My worry is about code that assumes that the return value has a leading slash. It doesn't always have it...

PVince81 · 2015-05-20T16:49:59Z

I wonder what could go wrong if someone ever had a real "_trashbin" folder in their home storage...

nickvergessen · 2015-05-21T08:21:02Z

tests/lib/files/view.php

Mind adding array('/files', '/files/', '/'), aswell?

I tried adding more cases initially, but they failed. This is because response we are getting will sometimes have a trailing slash and sometimes not. It's a real mess and I didn't want to "enforce" the expected behavior here until we fix it and decide what the API contract is for that method, as part of #16478

I'm not sure about adding test cases that asserts that the behavior is still as buggy as it was before, basically giving a "wrong" answer.

Well the case I mentioned is hardcoded before your patch:

if (rtrim($path,'/') === rtrim($this->fakeRoot, '/')) { return '/'; }

so that should always work (until someone breaks it)

Ah yes, then it makes sense. I'll add it. (I'm a bit blinded by my 😡 regarding this method)

also empty fakeRoot should be easily testable

nickvergessen · 2015-05-21T08:29:56Z

Test coverage for the patch is good and the patch looks fine aswell.

The alternative would be to just detect these cases and log a warning and carry on... at least for 8.1.

To not break anything I'd not backport it, but only fix in master. So I agree with that.

PVince81 · 2015-05-21T08:33:32Z

I agree, this PR should only go to 8.1

nickvergessen · 2015-05-21T09:11:29Z

lib/private/files/view.php

@PVince81 Should we use strlen($root) here? That would fix:
https://github.com/owncloud/core/pull/16479/files#diff-82902132462e0bedddfef05bdebb9373R855

I'd rather not take this risk. When I tried it it changed the behavior from "having a slash in the response" to "not having a slash in the response" in some cases. Keep in mind that some code that uses this API rely insanely on such slash to exist (or not exist). There is risk of breakage.

Okay, option 2 would be strlen(rtrim($this->fakeRoot, '/')) but I guess this is all 💩 until we freeze the method to one behaviour.

Yeah... I'd say just leave it. I tried to keep this PR to the minimum to just avoid the bad match, nothing more.

PVince81 · 2015-05-26T10:31:54Z

Note: this PR isn't needed any more for #16475

If we want to be on the safe side, we could just defer to 8.2

MorrisJobke · 2015-07-09T20:51:28Z

@PVince81 State of this?

PVince81 · 2015-07-10T08:17:13Z

At this point, need to continue with the proper fix on top of this PR. (too late for quickfix anyway)

PVince81 · 2015-08-24T14:56:17Z

Rebased.

scrutinizer-notifier · 2015-08-24T14:56:22Z

A new inspection was created.

PVince81 · 2015-08-24T14:57:35Z

I'd still have to have this quickfix in as it would potentially solve #18509 and #18524

The proper fix is much more risky.

MorrisJobke · 2015-08-24T18:15:15Z

I did some sometests and all went good 👍

nickvergessen · 2015-08-25T07:25:59Z

Mind also fixing getPath() ?

PVince81 · 2015-08-25T07:54:15Z

@nickvergessen separately then. Since @icewind1991 is off then I'll have a look myself.

PVince81 · 2015-08-25T07:56:42Z

@nickvergessen what's left to fix in getPath ? getPath uses getRelativePath and I thought this would fix it already ?

PVince81 · 2015-08-25T11:22:15Z

I hereby 👍 the commit from @nickvergessen for the unit tests.

Needs a second thumbs up for the original fix.

PVince81 · 2015-09-16T13:01:48Z

@nickvergessen anything left to be done here ? Does it fix #18509 ?

icewind1991 · 2015-09-16T13:18:35Z

👍

nickvergessen · 2015-09-16T13:42:15Z

@PVince81 works for the activity app atm

Before this fix, the root "/files" with path "/files_trashbin" would return "_trashbin" as relative path...

PVince81 · 2015-09-22T09:34:57Z

Rebased to wake up CI.

MorrisJobke · 2015-09-23T13:43:35Z

@PVince81 OCI fails :(

nickvergessen · 2015-09-24T07:38:08Z

Testergebnis (6 fehlgeschlagene Tests / +6)

Test\BackgroundJob\JobList::testAddRemove.testAddRemove with data set #0
Test\BackgroundJob\JobList::testAddRemove.testAddRemove with data set #1
Test\BackgroundJob\JobList::testAddRemove.testAddRemove with data set #2
Test\BackgroundJob\JobList::testAddRemove.testAddRemove with data set #3
Test\BackgroundJob\JobList::testAddRemove.testAddRemove with data set #4
Test\BackgroundJob\JobList::testRemoveDifferentArgument.testRemoveDifferentArgument with data set #0

Failing OCI is unlikely to be related:

Failed asserting that OCA\Files_Trashbin\BackgroundJob\ExpireTrash Object (...) is an instance of class "\Test\BackgroundJob\TestJob"

Also fixes #18509

👍

MorrisJobke · 2015-09-24T08:23:34Z

@DeepDiver1975 Merge or retrigger CI?

…atches Prevent wrong matches in getRelativePath

PVince81 added the 3 - To Review label May 20, 2015

PVince81 mentioned this pull request May 20, 2015

File restore from trash to ext storage appears twice in web UI #16475

Closed

nickvergessen reviewed May 21, 2015
View reviewed changes

PVince81 added this to the 8.2-next milestone May 26, 2015

PVince81 added 2 - Developing and removed 3 - To Review labels Jul 10, 2015

This was referenced Aug 24, 2015

view->getPath($id) returns strange path for delete files #18509

Closed

Bail out when trying to access trashed remote share #18524

Closed

PVince81 force-pushed the core-fixgetrelativepathwrongmatches branch from d7951e7 to f7e6f22 Compare August 24, 2015 14:56

PVince81 added 3 - To Review and removed 2 - Developing labels Aug 24, 2015

Vincent Petry and others added 2 commits September 22, 2015 11:34

Prevent wrong matches in getRelativePath

b9cd5bc

Before this fix, the root "/files" with path "/files_trashbin" would return "_trashbin" as relative path...

Add more tests for relativePath

d26c49b

PVince81 force-pushed the core-fixgetrelativepathwrongmatches branch from f7e6f22 to d26c49b Compare September 22, 2015 09:34

DeepDiver1975 added a commit that referenced this pull request Sep 24, 2015

Merge pull request #16479 from owncloud/core-fixgetrelativepathwrongm…

df75c17

…atches Prevent wrong matches in getRelativePath

DeepDiver1975 merged commit df75c17 into master Sep 24, 2015

DeepDiver1975 deleted the core-fixgetrelativepathwrongmatches branch September 24, 2015 08:25

lock bot locked as resolved and limited conversation to collaborators Aug 10, 2019

Prevent wrong matches in getRelativePath #16479

Prevent wrong matches in getRelativePath #16479

Uh oh!

Conversation

PVince81 commented May 20, 2015

Uh oh!

PVince81 commented May 20, 2015

Uh oh!

PVince81 commented May 20, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickvergessen commented May 21, 2015

Uh oh!

PVince81 commented May 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 commented May 26, 2015

Uh oh!

MorrisJobke commented Jul 9, 2015

Uh oh!

PVince81 commented Jul 10, 2015

Uh oh!

PVince81 commented Aug 24, 2015

Uh oh!

scrutinizer-notifier commented Aug 24, 2015

Uh oh!

PVince81 commented Aug 24, 2015

Uh oh!

MorrisJobke commented Aug 24, 2015

Uh oh!

nickvergessen commented Aug 25, 2015

Uh oh!

PVince81 commented Aug 25, 2015

Uh oh!

PVince81 commented Aug 25, 2015

Uh oh!

PVince81 commented Aug 25, 2015

Uh oh!

PVince81 commented Sep 16, 2015

Uh oh!

icewind1991 commented Sep 16, 2015

Uh oh!

nickvergessen commented Sep 16, 2015

Uh oh!

PVince81 commented Sep 22, 2015

Uh oh!

MorrisJobke commented Sep 23, 2015

Uh oh!

nickvergessen commented Sep 24, 2015

Uh oh!

MorrisJobke commented Sep 24, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants