diff --git a/.github/user_bug_report.md b/.github/user_bug_report.md index 9e5f2a5..4b92a8e 100755 --- a/.github/user_bug_report.md +++ b/.github/user_bug_report.md @@ -8,9 +8,11 @@ assignees: '' --- # Bug Description + A clear and concise description of what the bug is. # How to Reproduce + Steps to reproduce the behavior: 1. Go to '...' @@ -19,16 +21,20 @@ Steps to reproduce the behavior: 4. See error # Expected Behavior + A clear and concise description of what you expected to happen. # Screenshots + If applicable, add screenshots to help explain your problem. # System Information - - System: [e.g. PC or iPhone11, ...] - - OS: [e.g. iOS] - - Browser [e.g. chrome, safari] - - KarakaVersion [e.g. 22] + +- System: [e.g. PC or iPhone11, ...] +- OS: [e.g. iOS] +- Browser [e.g. chrome, safari] +- KarakaVersion [e.g. 22] # Additional Information + Add any other context about the problem here. diff --git a/Controller/ApiController.php b/Controller/ApiController.php index 01ddbf6..1d8bf4d 100755 --- a/Controller/ApiController.php +++ b/Controller/ApiController.php @@ -269,7 +269,7 @@ final class ApiController extends Controller array $fileNames = [], array $files = [], int $account = 0, - string $basePath = '/Modules/Media/Files', + string $basePath = '', string $virtualPath = '', string $password = '', string $encryptionKey = '', @@ -286,8 +286,6 @@ final class ApiController extends Controller $outputDir = ''; $absolute = false; - // @todo sandatize $basePath, we don't know if it might be relative! - if ($pathSettings === PathSettings::RANDOM_PATH) { $outputDir = self::createMediaPath($basePath); } elseif ($pathSettings === PathSettings::FILE_PATH) { @@ -297,6 +295,10 @@ final class ApiController extends Controller return []; } + if (!Guard::isSafePath($outputDir, __DIR__ . '/../../../')) { + return []; + } + $upload = new UploadFile(); $upload->outputDir = $outputDir; $upload->preserveFileName = empty($fileNames) || \count($fileNames) === \count($files); @@ -428,11 +430,11 @@ final class ApiController extends Controller ] ); - $app?->moduleManager->get('Admin')->createAccountModelPermission( + $app?->moduleManager->get('Admin', 'Api')->createAccountModelPermission( new AccountPermission( $account, $app->unitId, - $app->appName, + $app->appId, self::NAME, self::NAME, PermissionCategory::MEDIA, @@ -576,19 +578,17 @@ final class ApiController extends Controller $media->setPath((string) ($request->getData('path') ?? $media->getPath())); $media->setVirtualPath(\urldecode((string) ($request->getData('virtualpath') ?? $media->getVirtualPath()))); - // @todo: implement a security check to ensure the user is allowed to write to the file. Right now you could overwrite ANY file with a malicious $path - if ($id === 0 - && $media instanceof NullMedia - && \is_file($fullPath = __DIR__ . '/../Files' . ($path = \urldecode($request->getData('path')))) - && \stripos(FileUtils::absolute(__DIR__ . '/../Files/'), FileUtils::absolute($fullPath)) === 0 + if ($media instanceof NullMedia + || !$this->app->accountManager->get($request->header->account)->hasPermission( + PermissionType::MODIFY, + $this->app->unitId, + $this->app->appId, + self::NAME, + PermissionCategory::MEDIA, + $request->header->account + ) ) { - $name = \explode('.', \basename($path)); - - $media->name = $name[0]; - $media->extension = $name[\count($name) - 1] ?? ''; - $media->setVirtualPath(\dirname($path)); - $media->setPath('/Modules/Media/Files/' . \ltrim($path, '\\/')); - $media->isAbsolute = false; + return $media; } if ($request->hasData('content')) { @@ -695,8 +695,8 @@ final class ApiController extends Controller private function validateReferenceCreate(RequestAbstract $request) : array { $val = []; - if (($val['parent'] = (empty($request->getData('parent')) && empty($request->getData('virtualpath')))) - || ($val['source'] = (empty($request->getData('source')) && empty($request->getData('child')))) + if (($val['parent'] = (!$request->hasData('parent') && !$request->hasData('virtualpath'))) + || ($val['source'] = (!$request->hasData('source') && !$request->hasData('child'))) ) { return $val; } @@ -779,7 +779,7 @@ final class ApiController extends Controller private function validateCollectionCreate(RequestAbstract $request) : array { $val = []; - if (($val['name'] = empty($request->getData('name')))) { + if (($val['name'] = !$request->hasData('name'))) { return $val; } @@ -812,7 +812,7 @@ final class ApiController extends Controller $outputDir = ''; $basePath = __DIR__ . '/../../../Modules/Media/Files'; - if (empty($request->getData('path'))) { + if (!$request->hasData('path')) { $outputDir = self::createMediaPath($basePath); } else { $outputDir = $basePath . '/' . \ltrim($request->getData('path'), '\\/'); @@ -958,7 +958,7 @@ final class ApiController extends Controller $outputDir = ''; $basePath = __DIR__ . '/../../../Modules/Media/Files'; - if (empty($request->getData('path'))) { + if (!$request->hasData('path')) { $outputDir = self::createMediaPath($basePath); } else { if (\stripos( @@ -1053,7 +1053,7 @@ final class ApiController extends Controller && !$this->app->accountManager->get($request->header->account)->hasPermission( PermissionType::READ, $this->app->unitId, - $this->app->appName, + $this->app->appId, self::NAME, PermissionCategory::MEDIA, $media->getId() @@ -1247,7 +1247,7 @@ final class ApiController extends Controller private function validateMediaTypeCreate(RequestAbstract $request) : array { $val = []; - if (($val['name'] = empty($request->getData('name'))) + if (($val['name'] = !$request->hasData('name')) ) { return $val; } @@ -1297,7 +1297,7 @@ final class ApiController extends Controller $type = new MediaType(); $type->name = $request->getDataString('name') ?? ''; - if (!empty($request->getData('title'))) { + if ($request->hasData('title')) { $type->setL11n($request->getDataString('title') ?? '', $request->getData('lang') ?? $request->getLanguage()); } @@ -1316,8 +1316,8 @@ final class ApiController extends Controller private function validateMediaTypeL11nCreate(RequestAbstract $request) : array { $val = []; - if (($val['title'] = empty($request->getData('title'))) - || ($val['type'] = empty($request->getData('type'))) + if (($val['title'] = !$request->hasData('title')) + || ($val['type'] = !$request->hasData('type')) ) { return $val; } diff --git a/Controller/BackendController.php b/Controller/BackendController.php index b0669de..1724387 100755 --- a/Controller/BackendController.php +++ b/Controller/BackendController.php @@ -82,7 +82,7 @@ final class BackendController extends Controller ->hasPermission( PermissionType::READ, $this->app->unitId, - $this->app->appName, + $this->app->appId, self::NAME, PermissionCategory::MEDIA, ); @@ -94,7 +94,7 @@ final class BackendController extends Controller ->groups($this->app->accountManager->get($request->header->account)->getGroupIds()) ->account($request->header->account) ->units([null, $this->app->unitId]) - ->apps([null, 'Api', $this->app->appName]) + ->apps([null, 'Api', $this->app->appId]) ->modules([null, self::NAME]) ->categories([null, PermissionCategory::MEDIA]) ->permission(PermissionType::READ)