Skip to content

Commit

Permalink
Merge pull request #46571 from nextcloud/chore/migrate-to-filenameval…
Browse files Browse the repository at this point in the history
…idator

refactor: Migrate some legacy and core functions to `IFilenameValidator`
  • Loading branch information
sorbaugh authored Jul 22, 2024
2 parents c2a571e + 9716b0d commit 9ed2d3e
Show file tree
Hide file tree
Showing 5 changed files with 38 additions and 27 deletions.
3 changes: 1 addition & 2 deletions core/Controller/AvatarController.php
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,7 @@ public function postAvatar(?string $path = null): JSONResponse {
} elseif (!is_null($files)) {
if (
$files['error'][0] === 0 &&
is_uploaded_file($files['tmp_name'][0]) &&
!\OC\Files\Filesystem::isFileBlacklisted($files['tmp_name'][0])
is_uploaded_file($files['tmp_name'][0])
) {
if ($files['size'][0] > 20 * 1024 * 1024) {
return new JSONResponse(
Expand Down
6 changes: 6 additions & 0 deletions lib/private/Files/View.php
Original file line number Diff line number Diff line change
Expand Up @@ -1824,6 +1824,12 @@ private function getPartFileInfo(string $path): \OC\Files\FileInfo {
* @throws InvalidPathException
*/
public function verifyPath($path, $fileName): void {
// All of the view's functions disallow '..' in the path so we can short cut if the path is invalid
if (!Filesystem::isValidPath($path ?: '/')) {
$l = \OCP\Util::getL10N('lib');
throw new InvalidPathException($l->t('Path contains invalid segments'));
}

try {
/** @type \OCP\Files\Storage $storage */
[$storage, $internalPath] = $this->resolvePath($path);
Expand Down
29 changes: 15 additions & 14 deletions lib/private/Security/CertificateManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
*/
namespace OC\Security;

use OC\Files\Filesystem;
use OC\Files\View;
use OCP\ICertificate;
use OCP\ICertificateManager;
Expand Down Expand Up @@ -150,20 +149,19 @@ public function createCertificateBundle(): void {
* @throws \Exception If the certificate could not get added
*/
public function addCertificate(string $certificate, string $name): ICertificate {
if (!Filesystem::isValidPath($name) or Filesystem::isFileBlacklisted($name)) {
throw new \Exception('Filename is not valid');
}
$path = $this->getPathToCertificates() . 'uploads/' . $name;
$directory = dirname($path);

$this->view->verifyPath($directory, basename($path));
$this->bundlePath = null;

$dir = $this->getPathToCertificates() . 'uploads/';
if (!$this->view->file_exists($dir)) {
$this->view->mkdir($dir);
if (!$this->view->file_exists($directory)) {
$this->view->mkdir($directory);
}

try {
$file = $dir . $name;
$certificateObject = new Certificate($certificate, $name);
$this->view->file_put_contents($file, $certificate);
$this->view->file_put_contents($path, $certificate);
$this->createCertificateBundle();
return $certificateObject;
} catch (\Exception $e) {
Expand All @@ -175,14 +173,17 @@ public function addCertificate(string $certificate, string $name): ICertificate
* Remove the certificate and re-generate the certificate bundle
*/
public function removeCertificate(string $name): bool {
if (!Filesystem::isValidPath($name)) {
$path = $this->getPathToCertificates() . 'uploads/' . $name;

try {
$this->view->verifyPath(dirname($path), basename($path));
} catch (\Exception) {
return false;
}
$this->bundlePath = null;

$path = $this->getPathToCertificates() . 'uploads/';
if ($this->view->file_exists($path . $name)) {
$this->view->unlink($path . $name);
$this->bundlePath = null;
if ($this->view->file_exists($path)) {
$this->view->unlink($path);
$this->createCertificateBundle();
}
return true;
Expand Down
12 changes: 10 additions & 2 deletions lib/private/legacy/OC_Helper.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* SPDX-License-Identifier: AGPL-3.0-only
*/
use bantu\IniGetWrapper\IniGetWrapper;
use OC\Files\FilenameValidator;
use OC\Files\Filesystem;
use OCP\Files\Mount\IMountPoint;
use OCP\IBinaryFinder;
Expand Down Expand Up @@ -116,6 +117,10 @@ public static function computerFileSize(string $str): false|int|float {
* @return void
*/
public static function copyr($src, $dest) {
if (!file_exists($src)) {
return;
}

if (is_dir($src)) {
if (!is_dir($dest)) {
mkdir($dest);
Expand All @@ -126,8 +131,11 @@ public static function copyr($src, $dest) {
self::copyr("$src/$file", "$dest/$file");
}
}
} elseif (file_exists($src) && !\OC\Files\Filesystem::isFileBlacklisted($src)) {
copy($src, $dest);
} else {
$validator = \OCP\Server::get(FilenameValidator::class);
if (!$validator->isForbidden($src)) {
copy($src, $dest);
}
}
}

Expand Down
15 changes: 6 additions & 9 deletions tests/lib/Security/CertificateManagerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@

use OC\Files\View;
use OC\Security\CertificateManager;
use OCP\Files\InvalidPathException;
use OCP\IConfig;
use OCP\Security\ISecureRandom;
use PHPUnit\Framework\MockObject\MockObject;
use Psr\Log\LoggerInterface;

/**
Expand All @@ -25,12 +27,9 @@ class CertificateManagerTest extends \Test\TestCase {
use \Test\Traits\UserTrait;
use \Test\Traits\MountProviderTrait;

/** @var CertificateManager */
private $certificateManager;
/** @var String */
private $username;
/** @var ISecureRandom */
private $random;
private CertificateManager $certificateManager;
private string $username;
private ISecureRandom&MockObject $random;

protected function setUp(): void {
parent::setUp();
Expand Down Expand Up @@ -117,9 +116,7 @@ public function dangerousFileProvider() {
* @param string $filename
*/
public function testAddDangerousFile($filename) {
$this->expectException(\Exception::class);
$this->expectExceptionMessage('Filename is not valid');

$this->expectException(InvalidPathException::class);
$this->certificateManager->addCertificate(file_get_contents(__DIR__ . '/../../data/certificates/expiredCertificate.crt'), $filename);
}

Expand Down

0 comments on commit 9ed2d3e

Please sign in to comment.