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

Disallow path traversals in file view #14342

Merged
merged 2 commits into from Feb 19, 2015

Conversation

@LukasReschke
Copy link
Member

commented Feb 18, 2015

This prevents a misusage of \OC\Files\View by calling it with user-supplied input. In such cases an exception is now thrown.

Also I added some basic PHPDocs to that class as my IDE was crying "FIX ME" ;-)

cc @icewind1991 Please review

@ghost

This comment has been minimized.

Copy link

commented Feb 18, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/9731/
Test PASSed.

@nickvergessen
nickvergessen reviewed Feb 18, 2015
View changes
lib/private/files/view.php Outdated
public function filemtime($path) {
return $this->basicOperation('filemtime', $path);
}

/**
* @param $path

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Feb 18, 2015

Contributor

missing type

@nickvergessen
nickvergessen reviewed Feb 18, 2015
View changes
tests/lib/files/view.php Outdated
@@ -894,4 +894,22 @@ public function testDeleteFailKeepCache() {
$this->assertFalse($view->unlink('foo.txt'));
$this->assertTrue($cache->inCache('foo.txt'));
}


This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Feb 18, 2015

Contributor

double new line

@nickvergessen
nickvergessen reviewed Feb 18, 2015
View changes
tests/lib/files/view.php Outdated
function directoryTraversalProvider() {
return [
['../test/'],
['..\test\my/../folder'],

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Feb 18, 2015

Contributor

you are escaping the t and m here, if they receive a meaning once ;)
To be sure just \\ double them all teh time

This comment has been minimized.

Copy link
@LukasReschke

LukasReschke Feb 18, 2015

Author Member

Indeed 🙈

@icewind1991

This comment has been minimized.

Copy link
Member

commented Feb 18, 2015

Makes sense 👍

This prevents a misusage of \OC\Files\View by calling it with user-supplied input. In such cases an exception is now thrown.
@LukasReschke LukasReschke force-pushed the disallow-path-traversals-in-file-view branch to 46ca0fa Feb 18, 2015
@LukasReschke

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2015

@nickvergessen Adjusted.

@ghost

This comment has been minimized.

Copy link

commented Feb 18, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/9740/
Test PASSed.

@scrutinizer-notifier

This comment has been minimized.

Copy link

commented Feb 18, 2015

The inspection completed: 27 new issues, 2 updated code elements

@DeepDiver1975 DeepDiver1975 added this to the 8.1-next milestone Feb 19, 2015
@DeepDiver1975

This comment has been minimized.

Copy link
Member

commented Feb 19, 2015

👍

DeepDiver1975 added a commit that referenced this pull request Feb 19, 2015
…ile-view

Disallow path traversals in file view
@DeepDiver1975 DeepDiver1975 merged commit 84eb00e into master Feb 19, 2015
1 check passed
1 check passed
default Merged build finished.
Details
@DeepDiver1975 DeepDiver1975 deleted the disallow-path-traversals-in-file-view branch Feb 19, 2015
@DeepDiver1975

This comment has been minimized.

Copy link
Member

commented Feb 19, 2015

@LukasReschke I assume we want this on all stable branches?

@LukasReschke

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2015

Not quite sure about this as it is "just" an hardening and you never know which third-party apps rely on which behaviour.

Our shipped apps should work okay though.

I'd say we keep this for 8.1 for now. - With a 3 month release schedule this will reach the users anyways soon :)

@lock lock bot locked as resolved and limited conversation to collaborators Aug 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.