From 8973ab9000320875a4c86f6c378d4d94317c1c2a Mon Sep 17 00:00:00 2001 From: habibie11 Date: Wed, 20 May 2026 20:20:38 +0700 Subject: [PATCH 1/4] Tingkatkan keamanan upload tema --- .../Controllers/BackEnd/ThemesController.php | 241 ++++++++++++++++-- routes/web.php | 7 +- .../Security/ThemeUploadSecurityTest.php | 125 +++++++++ tests/Unit/ThemesControllerSecurityTest.php | 132 ++++++++++ 4 files changed, 486 insertions(+), 19 deletions(-) create mode 100644 tests/Feature/Security/ThemeUploadSecurityTest.php create mode 100644 tests/Unit/ThemesControllerSecurityTest.php diff --git a/app/Http/Controllers/BackEnd/ThemesController.php b/app/Http/Controllers/BackEnd/ThemesController.php index e953cd487..1ea816496 100644 --- a/app/Http/Controllers/BackEnd/ThemesController.php +++ b/app/Http/Controllers/BackEnd/ThemesController.php @@ -11,6 +11,29 @@ class ThemesController extends BackEndController { + /** + * Dangerous PHP functions that are blocked in hooks.php. + */ + private const DANGEROUS_FUNCTIONS = [ + 'system', 'exec', 'passthru', 'shell_exec', 'popen', 'proc_open', + 'pcntl_exec', 'assert', 'create_function', 'eval', + 'file_put_contents', 'file_get_contents', 'fopen', 'fwrite', 'fputs', + 'unlink', 'mkdir', 'rmdir', 'rename', 'copy', 'chmod', 'chown', + 'symlink', 'link', 'tmpfile', 'move_uploaded_file', + 'extract', 'parse_str', 'putenv', + 'ini_set', 'ini_alter', + 'header', 'setcookie', + 'define', 'defined', + 'base64_decode', 'urldecode', + ]; + + /** + * Dangerous PHP file extensions that are rejected inside ZIP archives. + */ + private const DANGEROUS_EXTENSIONS = [ + 'php', 'phtml', 'php3', 'php4', 'php5', 'pht', 'inc', 'phar', + ]; + public function index() { $page_title = 'Tema'; @@ -66,12 +89,6 @@ public function upload() ]); } - // Use FileUploadService for secure file upload - $fileUploadService = new \App\Services\FileUploadService(); - - // Define allowed MIME types for zip files - $allowedMimes = \App\Services\FileUploadService::getAllowedMimes('archive'); - // Upload file securely to temp directory $path = $fileUploadService->uploadSecure($file, 'framework/themes', $allowedMimes, 51200); // 50MB max @@ -83,6 +100,9 @@ public function upload() $zip = new \ZipArchive; if ($zip->open("$filePath/$fileName") === true) { + // === PHASE 2: Scan ZIP for dangerous PHP files BEFORE extraction === + $this->scanZipForPhp($zip); + $extractedPath = base_path('themes/extracted'); $zip->extractTo($extractedPath); $zip->close(); @@ -104,12 +124,21 @@ public function upload() scan_themes(); - // Load theme hooks - $this->loadThemeHooks($composerData['name']); + // === PHASE 1: REMOVED auto-execution of loadThemeHooks === + // Hooks are now only loaded manually via activate() by super-admin + // after security review of hooks.php content. + $themeName = $composerData['name']; + $userId = auth()->id(); + $userEmail = auth()->user()?->email ?? 'unknown'; + Log::warning("Theme uploaded (hooks NOT auto-loaded): theme={$themeName}, user_id={$userId}, email={$userEmail}", [ + 'action' => 'theme_upload', + 'theme' => $themeName, + 'user_id' => $userId, + ]); return response()->json([ 'status' => 'success', - 'message' => 'Tema berhasil diunggah dan siap digunakan', + 'message' => 'Tema berhasil diunggah. Review hooks.php melalui menu aktivasi tema sebelum mengaktifkan.', ]); } else { File::deleteDirectory($extractedPath); @@ -118,7 +147,15 @@ public function upload() } } } catch (\Exception $e) { - Log::error('File upload failed: ' . $e->getMessage()); + Log::error('Theme upload failed: ' . $e->getMessage(), [ + 'action' => 'theme_upload_failed', + 'user_id' => auth()->id(), + 'error' => $e->getMessage(), + ]); + return response()->json([ + 'status' => 'error', + 'message' => 'Tema gagal diunggah: ' . $e->getMessage(), + ]); } return response()->json([ @@ -168,19 +205,189 @@ private function clearThemeCache() } /** - * Load theme hooks/filters + * Load theme hooks/filters with token-based security validation. + * + * hooks.php is validated using token_get_all() before inclusion. + * Only allowed constructs are permitted: + * - Function declarations (function xyz() { ... }) + * - Return statement at top level + * - Array declarations + * - use() for closures / imports + * + * Any call to dangerous functions (system, exec, eval, file I/O, etc.) + * will cause the hooks file to be REJECTED. */ private function loadThemeHooks(string $themePath) { $hooksFile = base_path("themes/{$themePath}/hooks.php"); - if (file_exists($hooksFile)) { - try { - include_once $hooksFile; - Log::info("Theme hooks loaded: {$themePath}"); - } catch (\Exception $e) { - Log::error("Failed to load theme hooks: {$e->getMessage()}"); + if (! file_exists($hooksFile)) { + return; + } + + // === PHASE 3: Token-based validation === + $source = file_get_contents($hooksFile); + if ($source === false || trim($source) === '') { + Log::warning("Theme hooks file empty or unreadable: {$themePath}"); + return; + } + + $validationResult = $this->validateHooksSource($source, $themePath); + if (! $validationResult['valid']) { + Log::critical("Theme hooks REJECTED: {$themePath} — {$validationResult['reason']}", [ + 'action' => 'theme_hooks_rejected', + 'theme' => $themePath, + 'reason' => $validationResult['reason'], + 'details' => $validationResult['details'] ?? [], + ]); + throw new \RuntimeException("hooks.php mengandung kode berbahaya: {$validationResult['reason']}"); + } + + // Safe to include after token validation + try { + include_once $hooksFile; + Log::info("Theme hooks loaded (validated): {$themePath}", [ + 'action' => 'theme_hooks_loaded', + 'theme' => $themePath, + ]); + } catch (\Exception $e) { + Log::error("Failed to load theme hooks: {$e->getMessage()}", [ + 'action' => 'theme_hooks_error', + 'theme' => $themePath, + 'error' => $e->getMessage(), + ]); + } + } + + /** + * Validate hooks.php source code using token_get_all(). + * + * Returns ['valid' => bool, 'reason' => string, 'details' => array] + */ + private function validateHooksSource(string $source, string $themeName): array + { + $tokens = @token_get_all($source); + // token_get_all() tokenizes without syntax validation; it will tokenize + // anything. We detect syntax problems via T_BAD_CHARACTER token. + if (! is_array($tokens)) { + return ['valid' => false, 'reason' => 'hooks.php cannot be parsed', 'details' => []]; + } + + $count = count($tokens); + $dangerousCalls = []; + $hasInlineHtml = false; + $hasBadCharacter = false; + + for ($i = 0; $i < $count; $i++) { + if (! is_array($tokens[$i])) { + continue; + } + + // Detect syntax errors via T_BAD_CHARACTER + if ($tokens[$i][0] === T_BAD_CHARACTER) { + $hasBadCharacter = true; } + + // Reject inline HTML — hooks.php should be pure PHP + if ($tokens[$i][0] === T_INLINE_HTML) { + $content = trim((string) $tokens[$i][1]); + if ($content !== '') { + $hasInlineHtml = true; + } + } + + // Detect eval() — T_EVAL is a language construct token, not T_STRING + if ($tokens[$i][0] === T_EVAL) { + $dangerousCalls[] = [ + 'function' => 'eval', + 'line' => $tokens[$i][2], + ]; + continue; + } + + // Look for function calls: T_STRING followed by '(' + if ($tokens[$i][0] === T_STRING) { + $funcName = strtolower((string) $tokens[$i][1]); + + // Check if the next non-whitespace token is '(' + $nextIdx = $i + 1; + while ($nextIdx < $count && is_array($tokens[$nextIdx]) && $tokens[$nextIdx][0] === T_WHITESPACE) { + $nextIdx++; + } + + if ($nextIdx < $count && ! is_array($tokens[$nextIdx]) && $tokens[$nextIdx] === '(') { + // Skip if this T_STRING is part of a function/method definition + // Check previous token context — if preceded by T_FUNCTION, it's a definition, not a call + $prevIdx = $i - 1; + while ($prevIdx >= 0 && is_array($tokens[$prevIdx]) && $tokens[$prevIdx][0] === T_WHITESPACE) { + $prevIdx--; + } + $isDefinition = ($prevIdx >= 0 && is_array($tokens[$prevIdx]) && $tokens[$prevIdx][0] === T_FUNCTION); + + if (! $isDefinition && in_array($funcName, self::DANGEROUS_FUNCTIONS, true)) { + $dangerousCalls[] = [ + 'function' => $funcName, + 'line' => $tokens[$i][2], + ]; + } + } + } + } + + if ($hasBadCharacter) { + return [ + 'valid' => false, + 'reason' => 'hooks.php contains syntax errors', + 'details' => [], + ]; + } + + if ($hasInlineHtml) { + return [ + 'valid' => false, + 'reason' => 'hooks.php must contain only PHP code, no HTML', + 'details' => [], + ]; + } + + if (! empty($dangerousCalls)) { + return [ + 'valid' => false, + 'reason' => 'Dangerous function calls detected: ' . implode(', ', array_column($dangerousCalls, 'function')), + 'details' => $dangerousCalls, + ]; + } + + return ['valid' => true, 'reason' => '', 'details' => []]; + } + + /** + * Scan ZIP archive for files with dangerous PHP extensions. + * Must be called BEFORE extractTo(). + * + * @throws \RuntimeException if dangerous files are detected. + */ + private function scanZipForPhp(\ZipArchive $zip): void + { + $dangerousFiles = []; + + for ($i = 0; $i < $zip->numFiles; $i++) { + $filename = $zip->getNameIndex($i); + $ext = strtolower(pathinfo($filename, PATHINFO_EXTENSION)); + + if (in_array($ext, self::DANGEROUS_EXTENSIONS, true)) { + $dangerousFiles[] = $filename; + } + } + + if (! empty($dangerousFiles)) { + $list = implode(', ', $dangerousFiles); + Log::critical("Dangerous files detected in theme ZIP: {$list}", [ + 'action' => 'theme_zip_blocked', + 'files' => $dangerousFiles, + 'user_id' => auth()->id(), + ]); + throw new \RuntimeException("Arsip mengandung file PHP berbahaya: {$list}"); } } diff --git a/routes/web.php b/routes/web.php index 94636e0c7..36a60f133 100644 --- a/routes/web.php +++ b/routes/web.php @@ -966,11 +966,14 @@ Route::get('activate/{themes}', 'activate')->name('setting.themes.activate'); Route::get('rescan', 'rescan')->name('setting.themes.rescan'); Route::post('clear-cache', 'clearCache')->name('setting.themes.clear-cache'); - // post to-upload - Route::post('upload', 'upload')->name('setting.themes.upload'); Route::delete('destroy/{themes}', 'destroy')->name('setting.themes.destroy'); }); + // Theme upload — restricted to super-admin only (CRITICAL security boundary) + Route::group(['prefix' => 'themes', 'controller' => ThemesController::class, 'middleware' => ['role:super-admin']], function () { + Route::post('upload', 'upload')->name('setting.themes.upload'); + }); + Route::group(['prefix' => 'aplikasi', 'controller' => AplikasiController::class, 'middleware' => ['action_permission:access.setting.aplikasi']], function () { Route::get('/', 'index')->name('setting.aplikasi.index'); Route::get('/edit/{aplikasi}', 'edit')->name('setting.aplikasi.edit'); diff --git a/tests/Feature/Security/ThemeUploadSecurityTest.php b/tests/Feature/Security/ThemeUploadSecurityTest.php new file mode 100644 index 000000000..c22380d6e --- /dev/null +++ b/tests/Feature/Security/ThemeUploadSecurityTest.php @@ -0,0 +1,125 @@ +seed(RoleSpatieSeeder::class); + + $this->superAdmin = User::factory()->create(); + $this->superAdmin->assignRole('super-admin'); + + $this->adminWebsite = User::factory()->create(); + $this->adminWebsite->assignRole('administrator-website'); + }); + + // ── Phase 4: Route permission ── + + test('super_admin_can_access_upload_page', function () { + $r = $this->actingAs($this->superAdmin)->get(route('setting.themes.index')); + $r->assertStatus(200); + }); + + test('administrator_website_forbidden_from_upload', function () { + $file = UploadedFile::fake()->create('theme.zip', 100, 'application/zip'); + $r = $this->actingAs($this->adminWebsite) + ->post(route('setting.themes.upload'), ['file' => $file]); + $r->assertStatus(403); + }); + + // ── Phase 2: ZIP content scanning ── + + test('upload_with_php_in_zip_is_rejected', function () { + $file = makeZipWithPhp(); + $r = $this->actingAs($this->superAdmin) + ->post(route('setting.themes.upload'), ['file' => $file]); + $r->assertJson(['status' => 'error']); + }); + + // ── Phase 3: Activate validates hooks ── + + test('activate_with_clean_hooks_succeeds', function () { + // Ensure default theme exists in DB and on disk + $hooksDir = base_path('themes/default'); + if (! is_dir($hooksDir)) { + mkdir($hooksDir, 0755, true); + } + + $hooksFile = $hooksDir . '/hooks.php'; + file_put_contents($hooksFile, ' 'default'], + [ + 'vendor' => 'opendk', + 'version' => '1.0', + 'description' => 'Default', + 'path' => $hooksDir, + 'system' => true, + 'active' => 0, + ] + ); + + $r = $this->actingAs($this->superAdmin) + ->get(route('setting.themes.activate', $theme)); + $r->assertStatus(302); + + @unlink($hooksFile); + }); + + test('activate_with_malicious_hooks_is_rejected', function () { + $hooksDir = base_path('themes/default'); + if (! is_dir($hooksDir)) { + mkdir($hooksDir, 0755, true); + } + + $hooksFile = $hooksDir . '/hooks.php'; + file_put_contents($hooksFile, ' 'default'], + [ + 'vendor' => 'opendk', + 'version' => '1.0', + 'description' => 'Default', + 'path' => $hooksDir, + 'system' => true, + 'active' => 0, + ] + ); + + $r = $this->actingAs($this->superAdmin) + ->get(route('setting.themes.activate', $theme)); + + // Malicious hooks should cause a 500 error, not a clean redirect + $r->assertStatus(500); + + @unlink($hooksFile); + }); + + // ── Helpers ── + + function makeZipWithPhp(): UploadedFile + { + $path = sys_get_temp_dir() . DIRECTORY_SEPARATOR . uniqid() . '.zip'; + $zip = new ZipArchive(); + $zip->open($path, ZipArchive::CREATE); + $zip->addFromString('evil-theme/composer.json', '{"name":"evil"}'); + $zip->addFromString('evil-theme/theme.json', '{"api_version":"v1"}'); + $zip->addFromString('evil-theme/hooks.php', 'close(); + + return new UploadedFile($path, 'theme.zip', 'application/zip', null, true); + } + +})->group('security', 'theme-upload'); \ No newline at end of file diff --git a/tests/Unit/ThemesControllerSecurityTest.php b/tests/Unit/ThemesControllerSecurityTest.php new file mode 100644 index 000000000..b994ac314 --- /dev/null +++ b/tests/Unit/ThemesControllerSecurityTest.php @@ -0,0 +1,132 @@ +controller = new ThemesController(); +}); + +// ── scanZipForPhp tests ── + +test('scanZipForPhp rejects ZIP containing .php file', function () { + $zipPath = sys_get_temp_dir() . '/' . uniqid() . '.zip'; + $z = new ZipArchive(); + $z->open($zipPath, ZipArchive::CREATE); + $z->addFromString('evil.php', 'close(); + + $z = new ZipArchive(); + $z->open($zipPath); + + $ref = new ReflectionClass($this->controller); + $m = $ref->getMethod('scanZipForPhp'); + + expect(fn () => $m->invoke($this->controller, $z)) + ->toThrow(RuntimeException::class); + + $z->close(); + @unlink($zipPath); +}); + +test('scanZipForPhp allows clean ZIP without PHP files', function () { + $zipPath = sys_get_temp_dir() . '/' . uniqid() . '.zip'; + $z = new ZipArchive(); + $z->open($zipPath, ZipArchive::CREATE); + $z->addFromString('theme.json', '{}'); + $z->addFromString('style.css', 'body{}'); + $z->close(); + + $z = new ZipArchive(); + $z->open($zipPath); + + $ref = new ReflectionClass($this->controller); + $m = $ref->getMethod('scanZipForPhp'); + $m->invoke($this->controller, $z); + + expect(true)->toBeTrue(); // no exception = pass + + $z->close(); + @unlink($zipPath); +}); + +test('scanZipForPhp rejects PHP file in subfolder', function () { + $zipPath = sys_get_temp_dir() . '/' . uniqid() . '.zip'; + $z = new ZipArchive(); + $z->open($zipPath, ZipArchive::CREATE); + $z->addFromString('includes/backdoor.php', 'close(); + + $z = new ZipArchive(); + $z->open($zipPath); + + $ref = new ReflectionClass($this->controller); + $m = $ref->getMethod('scanZipForPhp'); + + expect(fn () => $m->invoke($this->controller, $z)) + ->toThrow(RuntimeException::class); + + $z->close(); + @unlink($zipPath); +}); + +// ── validateHooksSource tests ── + +test('validateHooksSource rejects system() call', function () { + $r = invokePrivate('validateHooksSource', ['toBeFalse(); +}); + +test('validateHooksSource rejects exec() call', function () { + $r = invokePrivate('validateHooksSource', ['toBeFalse(); +}); + +test('validateHooksSource rejects eval() call', function () { + $r = invokePrivate('validateHooksSource', ['toBeFalse(); +}); + +test('validateHooksSource rejects file_put_contents() call', function () { + $r = invokePrivate('validateHooksSource', ['toBeFalse(); +}); + +test('validateHooksSource rejects shell_exec() call', function () { + $r = invokePrivate('validateHooksSource', ['toBeFalse(); +}); + +test('validateHooksSource allows harmless function declaration', function () { + $r = invokePrivate('validateHooksSource', ['toBeTrue(); +}); + +test('validateHooksSource allows array return', function () { + $r = invokePrivate('validateHooksSource', [' "b"];', 'clean']); + expect($r['valid'])->toBeTrue(); +}); + +test('validateHooksSource rejects inline HTML', function () { + $r = invokePrivate('validateHooksSource', ['

x

toBeFalse(); +}); + +// ── Helper ── + +function invokePrivate(string $method, array $args): mixed +{ + // Access via the controller stored in $this (Pest test closure binding) + $controller = test()->controller ?? new ThemesController(); + $ref = new ReflectionClass($controller); + $m = $ref->getMethod($method); + return $m->invokeArgs($controller, $args); +} \ No newline at end of file From a1916ee9bc6ab51bef1dd311bc7005fbe2d9fdd7 Mon Sep 17 00:00:00 2001 From: habibie11 Date: Thu, 21 May 2026 17:38:54 +0700 Subject: [PATCH 2/4] perbaikan review --- .../Controllers/BackEnd/ThemesController.php | 194 +++++++++++++----- .../Security/ThemeUploadSecurityTest.php | 156 ++++++++++++-- 2 files changed, 287 insertions(+), 63 deletions(-) diff --git a/app/Http/Controllers/BackEnd/ThemesController.php b/app/Http/Controllers/BackEnd/ThemesController.php index 1ea816496..94899c6ac 100644 --- a/app/Http/Controllers/BackEnd/ThemesController.php +++ b/app/Http/Controllers/BackEnd/ThemesController.php @@ -17,6 +17,9 @@ class ThemesController extends BackEndController private const DANGEROUS_FUNCTIONS = [ 'system', 'exec', 'passthru', 'shell_exec', 'popen', 'proc_open', 'pcntl_exec', 'assert', 'create_function', 'eval', + // BUG FIX: call_user_func / call_user_func_array bypass (Severity: Critical) + // These were missing and allowed bypassing the entire token-based validation. + 'call_user_func', 'call_user_func_array', 'file_put_contents', 'file_get_contents', 'fopen', 'fwrite', 'fputs', 'unlink', 'mkdir', 'rmdir', 'rename', 'copy', 'chmod', 'chown', 'symlink', 'link', 'tmpfile', 'move_uploaded_file', @@ -51,8 +54,19 @@ public function activate(Themes $themes) // Clear all theme API cache $this->clearThemeCache(); - // Load theme hooks if exists - $this->loadThemeHooks($themes->name); + // OBS FIX: Wrap loadThemeHooks in try/catch so a bad hooks.php yields a + // user-friendly redirect with an error flash instead of an HTTP 500. + try { + $this->loadThemeHooks($themes->name); + } catch (\RuntimeException $e) { + Log::critical('Theme hooks rejected during activation', [ + 'theme' => $themes->name, + 'reason' => $e->getMessage(), + ]); + + return redirect()->route('setting.themes.index') + ->with('error', 'Tema diaktifkan tetapi hooks.php ditolak: ' . $e->getMessage()); + } return redirect()->route('setting.themes.index') ->with('success', 'Tema berhasil diaktifkan. Cache API telah dibersihkan.'); @@ -99,52 +113,70 @@ public function upload() $filePath = storage_path('framework/themes'); $zip = new \ZipArchive; - if ($zip->open("$filePath/$fileName") === true) { - // === PHASE 2: Scan ZIP for dangerous PHP files BEFORE extraction === - $this->scanZipForPhp($zip); - - $extractedPath = base_path('themes/extracted'); - $zip->extractTo($extractedPath); - $zip->close(); - - $folderTheme = explode('.', $fileName)[0]; - $composerPath = "$extractedPath/$folderTheme/composer.json"; - - if (file_exists($composerPath)) { - $composerData = json_decode(file_get_contents($composerPath), true); - - // Validate theme structure - $this->validateThemeStructure($extractedPath, $folderTheme); - - $newFolder = base_path('themes/' . $composerData['name']); - - if (File::move("$extractedPath/$folderTheme", $newFolder)) { - File::deleteDirectory($extractedPath); - File::deleteDirectory($filePath); - - scan_themes(); - - // === PHASE 1: REMOVED auto-execution of loadThemeHooks === - // Hooks are now only loaded manually via activate() by super-admin - // after security review of hooks.php content. - $themeName = $composerData['name']; - $userId = auth()->id(); - $userEmail = auth()->user()?->email ?? 'unknown'; - Log::warning("Theme uploaded (hooks NOT auto-loaded): theme={$themeName}, user_id={$userId}, email={$userEmail}", [ - 'action' => 'theme_upload', - 'theme' => $themeName, - 'user_id' => $userId, - ]); - - return response()->json([ - 'status' => 'success', - 'message' => 'Tema berhasil diunggah. Review hooks.php melalui menu aktivasi tema sebelum mengaktifkan.', - ]); - } else { - File::deleteDirectory($extractedPath); - File::deleteDirectory($filePath); + + // BUG FIX (Low): Wrap ZIP usage in try/finally so the handle is always + // closed even when scanZipForPhp() or validateThemeStructure() throws. + $zipOpened = false; + try { + if ($zip->open("$filePath/$fileName") === true) { + $zipOpened = true; + + // === PHASE 2: Scan ZIP for dangerous PHP files BEFORE extraction === + $this->scanZipForPhp($zip); + + // BUG FIX (High): Validate ZIP entries for path traversal sequences + // before extraction to prevent writing files outside themes/extracted/. + $this->validateZipEntryPaths($zip); + + $extractedPath = base_path('themes/extracted'); + $zip->extractTo($extractedPath); + $zip->close(); + $zipOpened = false; + + $folderTheme = explode('.', $fileName)[0]; + $composerPath = "$extractedPath/$folderTheme/composer.json"; + + if (file_exists($composerPath)) { + $composerData = json_decode(file_get_contents($composerPath), true); + + // Validate theme structure + $this->validateThemeStructure($extractedPath, $folderTheme); + + $newFolder = base_path('themes/' . $composerData['name']); + + if (File::move("$extractedPath/$folderTheme", $newFolder)) { + File::deleteDirectory($extractedPath); + File::deleteDirectory($filePath); + + scan_themes(); + + // === PHASE 1: REMOVED auto-execution of loadThemeHooks === + // Hooks are now only loaded manually via activate() by super-admin + // after security review of hooks.php content. + $themeName = $composerData['name']; + $userId = auth()->id(); + $userEmail = auth()->user()?->email ?? 'unknown'; + Log::warning("Theme uploaded (hooks NOT auto-loaded): theme={$themeName}, user_id={$userId}, email={$userEmail}", [ + 'action' => 'theme_upload', + 'theme' => $themeName, + 'user_id' => $userId, + ]); + + return response()->json([ + 'status' => 'success', + 'message' => 'Tema berhasil diunggah. Review hooks.php melalui menu aktivasi tema sebelum mengaktifkan.', + ]); + } else { + File::deleteDirectory($extractedPath); + File::deleteDirectory($filePath); + } } } + } finally { + // BUG FIX (Low): Ensure the ZIP handle is always released. + if ($zipOpened) { + $zip->close(); + } } } catch (\Exception $e) { Log::error('Theme upload failed: ' . $e->getMessage(), [ @@ -152,9 +184,10 @@ public function upload() 'user_id' => auth()->id(), 'error' => $e->getMessage(), ]); + // BUG FIX (Medium): Return a generic message to the user; detail is logged above. return response()->json([ 'status' => 'error', - 'message' => 'Tema gagal diunggah: ' . $e->getMessage(), + 'message' => 'Tema gagal diunggah. Silakan periksa log untuk detail.', ]); } @@ -332,6 +365,23 @@ private function validateHooksSource(string $source, string $themeName): array } } } + + // BUG FIX (High): Detect variable function calls, e.g. $f('id') or $obj->method(). + // The token sequence is T_VARIABLE followed by '(' — the old scanner only checked + // T_STRING, so `$f = 'system'; $f('id');` would silently pass validation. + if ($tokens[$i][0] === T_VARIABLE) { + $nextIdx = $i + 1; + while ($nextIdx < $count && is_array($tokens[$nextIdx]) && $tokens[$nextIdx][0] === T_WHITESPACE) { + $nextIdx++; + } + + if ($nextIdx < $count && ! is_array($tokens[$nextIdx]) && $tokens[$nextIdx] === '(') { + $dangerousCalls[] = [ + 'function' => 'variable_function_call:' . $tokens[$i][1], + 'line' => $tokens[$i][2], + ]; + } + } } if ($hasBadCharacter) { @@ -407,8 +457,19 @@ private function validateThemeStructure(string $path, string $folder) } } - // Validate theme.json - $themeConfig = json_decode(file_get_contents("$path/$folder/theme.json"), true); + // OBS FIX: Add null-safety for file_get_contents and json_decode. + // file_get_contents() can return false on failure; json_decode() returns null + // for malformed JSON — both would cause silent failures without this check. + $themeJsonPath = "$path/$folder/theme.json"; + $themeJsonRaw = file_get_contents($themeJsonPath); + if ($themeJsonRaw === false) { + throw new \Exception("Tidak dapat membaca theme.json dari tema"); + } + + $themeConfig = json_decode($themeJsonRaw, true); + if (! is_array($themeConfig)) { + throw new \Exception("theme.json tidak mengandung JSON yang valid"); + } if (!isset($themeConfig['api_version'])) { Log::warning("Tema tidak mendefinisikan api_version, gunakan v1 sebagai default"); @@ -417,6 +478,41 @@ private function validateThemeStructure(string $path, string $folder) return true; } + /** + * Validate that no ZIP entry contains path traversal sequences. + * Must be called BEFORE extractTo(). + * + * BUG FIX (High): scanZipForPhp() only checked file extensions. A crafted ZIP + * with entries like "../../../storage/framework/cache/data.txt" could overwrite + * arbitrary files outside themes/extracted/ upon extraction. + * + * @throws \RuntimeException if any entry contains ".." or starts with "/". + */ + private function validateZipEntryPaths(\ZipArchive $zip): void + { + $traversalEntries = []; + + for ($i = 0; $i < $zip->numFiles; $i++) { + $filename = $zip->getNameIndex($i); + + // Reject entries that start with '/' (absolute path) + // or contain '..' (directory traversal) + if (str_starts_with($filename, '/') || str_contains($filename, '..')) { + $traversalEntries[] = $filename; + } + } + + if (! empty($traversalEntries)) { + $list = implode(', ', $traversalEntries); + Log::critical("Path traversal detected in theme ZIP: {$list}", [ + 'action' => 'theme_zip_traversal_blocked', + 'entries' => $traversalEntries, + 'user_id' => auth()->id(), + ]); + throw new \RuntimeException("Arsip mengandung entri path traversal berbahaya: {$list}"); + } + } + /** * Clear API cache manually (untuk admin) */ diff --git a/tests/Feature/Security/ThemeUploadSecurityTest.php b/tests/Feature/Security/ThemeUploadSecurityTest.php index c22380d6e..5d3248ad6 100644 --- a/tests/Feature/Security/ThemeUploadSecurityTest.php +++ b/tests/Feature/Security/ThemeUploadSecurityTest.php @@ -46,6 +46,15 @@ $r->assertJson(['status' => 'error']); }); + // ── BUG 3 FIX: ZIP path traversal ── + + test('upload_with_path_traversal_in_zip_is_rejected', function () { + $file = makeZipWithPathTraversal(); + $r = $this->actingAs($this->superAdmin) + ->post(route('setting.themes.upload'), ['file' => $file]); + $r->assertJson(['status' => 'error']); + }); + // ── Phase 3: Activate validates hooks ── test('activate_with_clean_hooks_succeeds', function () { @@ -61,12 +70,12 @@ $theme = Themes::firstOrCreate( ['name' => 'default'], [ - 'vendor' => 'opendk', - 'version' => '1.0', + 'vendor' => 'opendk', + 'version' => '1.0', 'description' => 'Default', - 'path' => $hooksDir, - 'system' => true, - 'active' => 0, + 'path' => $hooksDir, + 'system' => true, + 'active' => 0, ] ); @@ -77,7 +86,8 @@ @unlink($hooksFile); }); - test('activate_with_malicious_hooks_is_rejected', function () { + // OBS 1 FIX: activate() sekarang redirect 302 + flash error, bukan HTTP 500 + test('activate_with_malicious_hooks_redirects_with_error', function () { $hooksDir = base_path('themes/default'); if (! is_dir($hooksDir)) { mkdir($hooksDir, 0755, true); @@ -89,20 +99,121 @@ $theme = Themes::firstOrCreate( ['name' => 'default'], [ - 'vendor' => 'opendk', - 'version' => '1.0', + 'vendor' => 'opendk', + 'version' => '1.0', 'description' => 'Default', - 'path' => $hooksDir, - 'system' => true, - 'active' => 0, + 'path' => $hooksDir, + 'system' => true, + 'active' => 0, ] ); $r = $this->actingAs($this->superAdmin) ->get(route('setting.themes.activate', $theme)); - // Malicious hooks should cause a 500 error, not a clean redirect - $r->assertStatus(500); + // OBS 1 FIX: Malicious hooks now produce a friendly 302 redirect with a + // session error flash, not an HTTP 500, improving UX without hiding the failure. + $r->assertStatus(302); + $r->assertSessionHas('error'); + + @unlink($hooksFile); + }); + + // ── BUG 1 FIX: call_user_func / call_user_func_array bypass ── + + test('hooks_with_call_user_func_is_rejected', function () { + $hooksDir = base_path('themes/default'); + if (! is_dir($hooksDir)) { + mkdir($hooksDir, 0755, true); + } + + $hooksFile = $hooksDir . '/hooks.php'; + // Previously this bypassed validation because call_user_func was not in DANGEROUS_FUNCTIONS + file_put_contents($hooksFile, ' 'default'], + [ + 'vendor' => 'opendk', + 'version' => '1.0', + 'description' => 'Default', + 'path' => $hooksDir, + 'system' => true, + 'active' => 0, + ] + ); + + $r = $this->actingAs($this->superAdmin) + ->get(route('setting.themes.activate', $theme)); + + // BUG 1 FIX: call_user_func is now in DANGEROUS_FUNCTIONS → rejected + $r->assertStatus(302); + $r->assertSessionHas('error'); + + @unlink($hooksFile); + }); + + test('hooks_with_call_user_func_array_is_rejected', function () { + $hooksDir = base_path('themes/default'); + if (! is_dir($hooksDir)) { + mkdir($hooksDir, 0755, true); + } + + $hooksFile = $hooksDir . '/hooks.php'; + file_put_contents($hooksFile, ' 'default'], + [ + 'vendor' => 'opendk', + 'version' => '1.0', + 'description' => 'Default', + 'path' => $hooksDir, + 'system' => true, + 'active' => 0, + ] + ); + + $r = $this->actingAs($this->superAdmin) + ->get(route('setting.themes.activate', $theme)); + + // BUG 1 FIX: call_user_func_array is now in DANGEROUS_FUNCTIONS → rejected + $r->assertStatus(302); + $r->assertSessionHas('error'); + + @unlink($hooksFile); + }); + + // ── BUG 2 FIX: Variable function call bypass ── + + test('hooks_with_variable_function_call_is_rejected', function () { + $hooksDir = base_path('themes/default'); + if (! is_dir($hooksDir)) { + mkdir($hooksDir, 0755, true); + } + + $hooksFile = $hooksDir . '/hooks.php'; + // Previously: $f = 'system'; $f('id'); would pass validation (T_VARIABLE + '(' not detected) + file_put_contents($hooksFile, ' 'default'], + [ + 'vendor' => 'opendk', + 'version' => '1.0', + 'description' => 'Default', + 'path' => $hooksDir, + 'system' => true, + 'active' => 0, + ] + ); + + $r = $this->actingAs($this->superAdmin) + ->get(route('setting.themes.activate', $theme)); + + // BUG 2 FIX: T_VARIABLE followed by '(' is now detected → rejected + $r->assertStatus(302); + $r->assertSessionHas('error'); @unlink($hooksFile); }); @@ -112,7 +223,7 @@ function makeZipWithPhp(): UploadedFile { $path = sys_get_temp_dir() . DIRECTORY_SEPARATOR . uniqid() . '.zip'; - $zip = new ZipArchive(); + $zip = new ZipArchive(); $zip->open($path, ZipArchive::CREATE); $zip->addFromString('evil-theme/composer.json', '{"name":"evil"}'); $zip->addFromString('evil-theme/theme.json', '{"api_version":"v1"}'); @@ -122,4 +233,21 @@ function makeZipWithPhp(): UploadedFile return new UploadedFile($path, 'theme.zip', 'application/zip', null, true); } + /** + * BUG 3 FIX: Helper — creates a ZIP with a path traversal entry. + */ + function makeZipWithPathTraversal(): UploadedFile + { + $path = sys_get_temp_dir() . DIRECTORY_SEPARATOR . uniqid() . '.zip'; + $zip = new ZipArchive(); + $zip->open($path, ZipArchive::CREATE); + $zip->addFromString('evil-theme/composer.json', '{"name":"evil"}'); + $zip->addFromString('evil-theme/theme.json', '{"api_version":"v1"}'); + // Attempt to write outside themes/extracted/ via path traversal + $zip->addFromString('../../../storage/logs/pwned.txt', 'pwned'); + $zip->close(); + + return new UploadedFile($path, 'theme.zip', 'application/zip', null, true); + } + })->group('security', 'theme-upload'); \ No newline at end of file From 7936a5784d8d0ef89db4353cb8a7ff4757f31108 Mon Sep 17 00:00:00 2001 From: habibie11 Date: Fri, 22 May 2026 09:24:03 +0700 Subject: [PATCH 3/4] Refactor ThemesController: Ekstraksi logika bisnis dan validasi ke layanan terpisah Commit ini memindahkan logika bisnis manajemen tema ke ThemeService, validasi keamanan ke ThemeHooksValidator, dan menggunakan CacheService untuk pengelolaan cache agar controller lebih bersih dan terstruktur. --- .../Controllers/BackEnd/ThemesController.php | 447 +----------------- app/Services/ThemeHooksValidator.php | 253 ++++++++++ app/Services/ThemeService.php | 164 +++++++ 3 files changed, 434 insertions(+), 430 deletions(-) create mode 100644 app/Services/ThemeHooksValidator.php create mode 100644 app/Services/ThemeService.php diff --git a/app/Http/Controllers/BackEnd/ThemesController.php b/app/Http/Controllers/BackEnd/ThemesController.php index 94899c6ac..785958352 100644 --- a/app/Http/Controllers/BackEnd/ThemesController.php +++ b/app/Http/Controllers/BackEnd/ThemesController.php @@ -4,38 +4,19 @@ use App\Http\Controllers\BackEndController; use App\Models\Themes; -use Illuminate\Support\Facades\Cache; -use Illuminate\Support\Facades\File; +use App\Services\ThemeService; use Illuminate\Support\Facades\Log; -use Illuminate\Support\Facades\Artisan; class ThemesController extends BackEndController { - /** - * Dangerous PHP functions that are blocked in hooks.php. - */ - private const DANGEROUS_FUNCTIONS = [ - 'system', 'exec', 'passthru', 'shell_exec', 'popen', 'proc_open', - 'pcntl_exec', 'assert', 'create_function', 'eval', - // BUG FIX: call_user_func / call_user_func_array bypass (Severity: Critical) - // These were missing and allowed bypassing the entire token-based validation. - 'call_user_func', 'call_user_func_array', - 'file_put_contents', 'file_get_contents', 'fopen', 'fwrite', 'fputs', - 'unlink', 'mkdir', 'rmdir', 'rename', 'copy', 'chmod', 'chown', - 'symlink', 'link', 'tmpfile', 'move_uploaded_file', - 'extract', 'parse_str', 'putenv', - 'ini_set', 'ini_alter', - 'header', 'setcookie', - 'define', 'defined', - 'base64_decode', 'urldecode', - ]; + protected ThemeService $themeService; - /** - * Dangerous PHP file extensions that are rejected inside ZIP archives. - */ - private const DANGEROUS_EXTENSIONS = [ - 'php', 'phtml', 'php3', 'php4', 'php5', 'pht', 'inc', 'phar', - ]; + public function __construct(ThemeService $themeService) + { + $this->themeService = $themeService; + // In Laravel, if parent has no constructor this is fine, if it does, + // we might need parent::__construct(), but usually it's injected. + } public function index() { @@ -48,16 +29,8 @@ public function index() public function activate(Themes $themes) { - Themes::where('active', 1)->update(['active' => 0]); - $themes->update(['active' => 1]); - - // Clear all theme API cache - $this->clearThemeCache(); - - // OBS FIX: Wrap loadThemeHooks in try/catch so a bad hooks.php yields a - // user-friendly redirect with an error flash instead of an HTTP 500. try { - $this->loadThemeHooks($themes->name); + $this->themeService->activate($themes); } catch (\RuntimeException $e) { Log::critical('Theme hooks rejected during activation', [ 'theme' => $themes->name, @@ -74,10 +47,7 @@ public function activate(Themes $themes) public function reScan() { - scan_themes(); - - // Clear cache after rescan - $this->clearThemeCache(); + $this->themeService->reScan(); return redirect()->route('setting.themes.index') ->with('success', 'Tema berhasil dipindai ulang'); @@ -87,114 +57,30 @@ public function upload() { try { $file = request()->file('file'); - - // Use FileUploadService for secure file upload - $fileUploadService = new \App\Services\FileUploadService(); - // Define allowed MIME types for zip files - $allowedMimes = \App\Services\FileUploadService::getAllowedMimes('archive'); - - // Validate file type - $fileMimeType = $file->getMimeType(); - if (!in_array($fileMimeType, $allowedMimes)) { + if (!$file) { return response()->json([ 'status' => 'error', - 'message' => 'File harus berformat .zip', + 'message' => 'File tema tidak ditemukan', ]); } - // Upload file securely to temp directory - $path = $fileUploadService->uploadSecure($file, 'framework/themes', $allowedMimes, 51200); // 50MB max - - // Extract filename from path - $fileName = basename($path); + $result = $this->themeService->installFromZip($file); - // Get the full file path for further processing - $filePath = storage_path('framework/themes'); - - $zip = new \ZipArchive; - - // BUG FIX (Low): Wrap ZIP usage in try/finally so the handle is always - // closed even when scanZipForPhp() or validateThemeStructure() throws. - $zipOpened = false; - try { - if ($zip->open("$filePath/$fileName") === true) { - $zipOpened = true; - - // === PHASE 2: Scan ZIP for dangerous PHP files BEFORE extraction === - $this->scanZipForPhp($zip); - - // BUG FIX (High): Validate ZIP entries for path traversal sequences - // before extraction to prevent writing files outside themes/extracted/. - $this->validateZipEntryPaths($zip); + return response()->json($result); - $extractedPath = base_path('themes/extracted'); - $zip->extractTo($extractedPath); - $zip->close(); - $zipOpened = false; - - $folderTheme = explode('.', $fileName)[0]; - $composerPath = "$extractedPath/$folderTheme/composer.json"; - - if (file_exists($composerPath)) { - $composerData = json_decode(file_get_contents($composerPath), true); - - // Validate theme structure - $this->validateThemeStructure($extractedPath, $folderTheme); - - $newFolder = base_path('themes/' . $composerData['name']); - - if (File::move("$extractedPath/$folderTheme", $newFolder)) { - File::deleteDirectory($extractedPath); - File::deleteDirectory($filePath); - - scan_themes(); - - // === PHASE 1: REMOVED auto-execution of loadThemeHooks === - // Hooks are now only loaded manually via activate() by super-admin - // after security review of hooks.php content. - $themeName = $composerData['name']; - $userId = auth()->id(); - $userEmail = auth()->user()?->email ?? 'unknown'; - Log::warning("Theme uploaded (hooks NOT auto-loaded): theme={$themeName}, user_id={$userId}, email={$userEmail}", [ - 'action' => 'theme_upload', - 'theme' => $themeName, - 'user_id' => $userId, - ]); - - return response()->json([ - 'status' => 'success', - 'message' => 'Tema berhasil diunggah. Review hooks.php melalui menu aktivasi tema sebelum mengaktifkan.', - ]); - } else { - File::deleteDirectory($extractedPath); - File::deleteDirectory($filePath); - } - } - } - } finally { - // BUG FIX (Low): Ensure the ZIP handle is always released. - if ($zipOpened) { - $zip->close(); - } - } } catch (\Exception $e) { Log::error('Theme upload failed: ' . $e->getMessage(), [ 'action' => 'theme_upload_failed', 'user_id' => auth()->id(), 'error' => $e->getMessage(), ]); - // BUG FIX (Medium): Return a generic message to the user; detail is logged above. + return response()->json([ 'status' => 'error', 'message' => 'Tema gagal diunggah. Silakan periksa log untuk detail.', ]); } - - return response()->json([ - 'status' => 'error', - 'message' => 'Tema gagal diunggah', - ]); } public function destroy(Themes $themes) @@ -208,317 +94,18 @@ public function destroy(Themes $themes) $themes->delete(); // Clear cache - $this->clearThemeCache(); + $this->themeService->clearCache(); return redirect()->route('setting.themes.index') ->with('success', 'Tema berhasil dihapus'); } - /** - * Clear all theme-related cache - */ - private function clearThemeCache() - { - // Clear specific cache keys - Cache::forget('active_theme'); - $store = Cache::getStore(); - if (method_exists($store, 'tags')) { - // Tagged cache supported, clear tag and exit to avoid duplicate calls below - Cache::tags(['theme_api'])->flush(); - Log::info('Theme cache cleared via tags'); - } else { - // Alternative: Clear by prefix - $keys = Cache::get('theme_api:*'); - foreach ($keys ?? [] as $key) { - Cache::forget($key); - } - } - - Log::info('Theme cache cleared'); - } - - /** - * Load theme hooks/filters with token-based security validation. - * - * hooks.php is validated using token_get_all() before inclusion. - * Only allowed constructs are permitted: - * - Function declarations (function xyz() { ... }) - * - Return statement at top level - * - Array declarations - * - use() for closures / imports - * - * Any call to dangerous functions (system, exec, eval, file I/O, etc.) - * will cause the hooks file to be REJECTED. - */ - private function loadThemeHooks(string $themePath) - { - $hooksFile = base_path("themes/{$themePath}/hooks.php"); - - if (! file_exists($hooksFile)) { - return; - } - - // === PHASE 3: Token-based validation === - $source = file_get_contents($hooksFile); - if ($source === false || trim($source) === '') { - Log::warning("Theme hooks file empty or unreadable: {$themePath}"); - return; - } - - $validationResult = $this->validateHooksSource($source, $themePath); - if (! $validationResult['valid']) { - Log::critical("Theme hooks REJECTED: {$themePath} — {$validationResult['reason']}", [ - 'action' => 'theme_hooks_rejected', - 'theme' => $themePath, - 'reason' => $validationResult['reason'], - 'details' => $validationResult['details'] ?? [], - ]); - throw new \RuntimeException("hooks.php mengandung kode berbahaya: {$validationResult['reason']}"); - } - - // Safe to include after token validation - try { - include_once $hooksFile; - Log::info("Theme hooks loaded (validated): {$themePath}", [ - 'action' => 'theme_hooks_loaded', - 'theme' => $themePath, - ]); - } catch (\Exception $e) { - Log::error("Failed to load theme hooks: {$e->getMessage()}", [ - 'action' => 'theme_hooks_error', - 'theme' => $themePath, - 'error' => $e->getMessage(), - ]); - } - } - - /** - * Validate hooks.php source code using token_get_all(). - * - * Returns ['valid' => bool, 'reason' => string, 'details' => array] - */ - private function validateHooksSource(string $source, string $themeName): array - { - $tokens = @token_get_all($source); - // token_get_all() tokenizes without syntax validation; it will tokenize - // anything. We detect syntax problems via T_BAD_CHARACTER token. - if (! is_array($tokens)) { - return ['valid' => false, 'reason' => 'hooks.php cannot be parsed', 'details' => []]; - } - - $count = count($tokens); - $dangerousCalls = []; - $hasInlineHtml = false; - $hasBadCharacter = false; - - for ($i = 0; $i < $count; $i++) { - if (! is_array($tokens[$i])) { - continue; - } - - // Detect syntax errors via T_BAD_CHARACTER - if ($tokens[$i][0] === T_BAD_CHARACTER) { - $hasBadCharacter = true; - } - - // Reject inline HTML — hooks.php should be pure PHP - if ($tokens[$i][0] === T_INLINE_HTML) { - $content = trim((string) $tokens[$i][1]); - if ($content !== '') { - $hasInlineHtml = true; - } - } - - // Detect eval() — T_EVAL is a language construct token, not T_STRING - if ($tokens[$i][0] === T_EVAL) { - $dangerousCalls[] = [ - 'function' => 'eval', - 'line' => $tokens[$i][2], - ]; - continue; - } - - // Look for function calls: T_STRING followed by '(' - if ($tokens[$i][0] === T_STRING) { - $funcName = strtolower((string) $tokens[$i][1]); - - // Check if the next non-whitespace token is '(' - $nextIdx = $i + 1; - while ($nextIdx < $count && is_array($tokens[$nextIdx]) && $tokens[$nextIdx][0] === T_WHITESPACE) { - $nextIdx++; - } - - if ($nextIdx < $count && ! is_array($tokens[$nextIdx]) && $tokens[$nextIdx] === '(') { - // Skip if this T_STRING is part of a function/method definition - // Check previous token context — if preceded by T_FUNCTION, it's a definition, not a call - $prevIdx = $i - 1; - while ($prevIdx >= 0 && is_array($tokens[$prevIdx]) && $tokens[$prevIdx][0] === T_WHITESPACE) { - $prevIdx--; - } - $isDefinition = ($prevIdx >= 0 && is_array($tokens[$prevIdx]) && $tokens[$prevIdx][0] === T_FUNCTION); - - if (! $isDefinition && in_array($funcName, self::DANGEROUS_FUNCTIONS, true)) { - $dangerousCalls[] = [ - 'function' => $funcName, - 'line' => $tokens[$i][2], - ]; - } - } - } - - // BUG FIX (High): Detect variable function calls, e.g. $f('id') or $obj->method(). - // The token sequence is T_VARIABLE followed by '(' — the old scanner only checked - // T_STRING, so `$f = 'system'; $f('id');` would silently pass validation. - if ($tokens[$i][0] === T_VARIABLE) { - $nextIdx = $i + 1; - while ($nextIdx < $count && is_array($tokens[$nextIdx]) && $tokens[$nextIdx][0] === T_WHITESPACE) { - $nextIdx++; - } - - if ($nextIdx < $count && ! is_array($tokens[$nextIdx]) && $tokens[$nextIdx] === '(') { - $dangerousCalls[] = [ - 'function' => 'variable_function_call:' . $tokens[$i][1], - 'line' => $tokens[$i][2], - ]; - } - } - } - - if ($hasBadCharacter) { - return [ - 'valid' => false, - 'reason' => 'hooks.php contains syntax errors', - 'details' => [], - ]; - } - - if ($hasInlineHtml) { - return [ - 'valid' => false, - 'reason' => 'hooks.php must contain only PHP code, no HTML', - 'details' => [], - ]; - } - - if (! empty($dangerousCalls)) { - return [ - 'valid' => false, - 'reason' => 'Dangerous function calls detected: ' . implode(', ', array_column($dangerousCalls, 'function')), - 'details' => $dangerousCalls, - ]; - } - - return ['valid' => true, 'reason' => '', 'details' => []]; - } - - /** - * Scan ZIP archive for files with dangerous PHP extensions. - * Must be called BEFORE extractTo(). - * - * @throws \RuntimeException if dangerous files are detected. - */ - private function scanZipForPhp(\ZipArchive $zip): void - { - $dangerousFiles = []; - - for ($i = 0; $i < $zip->numFiles; $i++) { - $filename = $zip->getNameIndex($i); - $ext = strtolower(pathinfo($filename, PATHINFO_EXTENSION)); - - if (in_array($ext, self::DANGEROUS_EXTENSIONS, true)) { - $dangerousFiles[] = $filename; - } - } - - if (! empty($dangerousFiles)) { - $list = implode(', ', $dangerousFiles); - Log::critical("Dangerous files detected in theme ZIP: {$list}", [ - 'action' => 'theme_zip_blocked', - 'files' => $dangerousFiles, - 'user_id' => auth()->id(), - ]); - throw new \RuntimeException("Arsip mengandung file PHP berbahaya: {$list}"); - } - } - - /** - * Validate theme structure for API compatibility - */ - private function validateThemeStructure(string $path, string $folder) - { - $requiredFiles = [ - 'composer.json', - 'theme.json', - ]; - - foreach ($requiredFiles as $file) { - if (!file_exists("$path/$folder/$file")) { - throw new \Exception("File {$file} tidak ditemukan di tema"); - } - } - - // OBS FIX: Add null-safety for file_get_contents and json_decode. - // file_get_contents() can return false on failure; json_decode() returns null - // for malformed JSON — both would cause silent failures without this check. - $themeJsonPath = "$path/$folder/theme.json"; - $themeJsonRaw = file_get_contents($themeJsonPath); - if ($themeJsonRaw === false) { - throw new \Exception("Tidak dapat membaca theme.json dari tema"); - } - - $themeConfig = json_decode($themeJsonRaw, true); - if (! is_array($themeConfig)) { - throw new \Exception("theme.json tidak mengandung JSON yang valid"); - } - - if (!isset($themeConfig['api_version'])) { - Log::warning("Tema tidak mendefinisikan api_version, gunakan v1 sebagai default"); - } - - return true; - } - - /** - * Validate that no ZIP entry contains path traversal sequences. - * Must be called BEFORE extractTo(). - * - * BUG FIX (High): scanZipForPhp() only checked file extensions. A crafted ZIP - * with entries like "../../../storage/framework/cache/data.txt" could overwrite - * arbitrary files outside themes/extracted/ upon extraction. - * - * @throws \RuntimeException if any entry contains ".." or starts with "/". - */ - private function validateZipEntryPaths(\ZipArchive $zip): void - { - $traversalEntries = []; - - for ($i = 0; $i < $zip->numFiles; $i++) { - $filename = $zip->getNameIndex($i); - - // Reject entries that start with '/' (absolute path) - // or contain '..' (directory traversal) - if (str_starts_with($filename, '/') || str_contains($filename, '..')) { - $traversalEntries[] = $filename; - } - } - - if (! empty($traversalEntries)) { - $list = implode(', ', $traversalEntries); - Log::critical("Path traversal detected in theme ZIP: {$list}", [ - 'action' => 'theme_zip_traversal_blocked', - 'entries' => $traversalEntries, - 'user_id' => auth()->id(), - ]); - throw new \RuntimeException("Arsip mengandung entri path traversal berbahaya: {$list}"); - } - } - /** * Clear API cache manually (untuk admin) */ public function clearCache() { - $this->clearThemeCache(); + $this->themeService->clearCache(); return redirect()->route('setting.themes.index') ->with('success', 'Cache API tema berhasil dibersihkan'); diff --git a/app/Services/ThemeHooksValidator.php b/app/Services/ThemeHooksValidator.php new file mode 100644 index 000000000..c01788542 --- /dev/null +++ b/app/Services/ThemeHooksValidator.php @@ -0,0 +1,253 @@ +numFiles; $i++) { + $filename = $zip->getNameIndex($i); + $ext = strtolower(pathinfo($filename, PATHINFO_EXTENSION)); + + if (in_array($ext, self::DANGEROUS_EXTENSIONS, true)) { + $dangerousFiles[] = $filename; + } + } + + if (! empty($dangerousFiles)) { + $list = implode(', ', $dangerousFiles); + Log::critical("Dangerous files detected in theme ZIP: {$list}", [ + 'action' => 'theme_zip_blocked', + 'files' => $dangerousFiles, + 'user_id' => auth()->id(), + ]); + throw new \RuntimeException("Arsip mengandung file PHP berbahaya: {$list}"); + } + } + + /** + * Validate that no ZIP entry contains path traversal sequences. + * Must be called BEFORE extractTo(). + * + * @throws \RuntimeException if any entry contains ".." or starts with "/". + */ + public function validateZipEntryPaths(\ZipArchive $zip): void + { + $traversalEntries = []; + + for ($i = 0; $i < $zip->numFiles; $i++) { + $filename = $zip->getNameIndex($i); + + // Reject entries that start with '/' (absolute path) + // or contain '..' (directory traversal) + if (str_starts_with($filename, '/') || str_contains($filename, '..')) { + $traversalEntries[] = $filename; + } + } + + if (! empty($traversalEntries)) { + $list = implode(', ', $traversalEntries); + Log::critical("Path traversal detected in theme ZIP: {$list}", [ + 'action' => 'theme_zip_traversal_blocked', + 'entries' => $traversalEntries, + 'user_id' => auth()->id(), + ]); + throw new \RuntimeException("Arsip mengandung entri path traversal berbahaya: {$list}"); + } + } + + /** + * Load theme hooks/filters with token-based security validation. + */ + public function loadHooks(string $themePath): void + { + $hooksFile = base_path("themes/{$themePath}/hooks.php"); + + if (! file_exists($hooksFile)) { + return; + } + + // === PHASE 3: Token-based validation === + $source = file_get_contents($hooksFile); + if ($source === false || trim($source) === '') { + Log::warning("Theme hooks file empty or unreadable: {$themePath}"); + return; + } + + $validationResult = $this->validateSource($source, $themePath); + if (! $validationResult['valid']) { + Log::critical("Theme hooks REJECTED: {$themePath} — {$validationResult['reason']}", [ + 'action' => 'theme_hooks_rejected', + 'theme' => $themePath, + 'reason' => $validationResult['reason'], + 'details' => $validationResult['details'] ?? [], + ]); + throw new \RuntimeException("hooks.php mengandung kode berbahaya: {$validationResult['reason']}"); + } + + // Safe to include after token validation + try { + include_once $hooksFile; + Log::info("Theme hooks loaded (validated): {$themePath}", [ + 'action' => 'theme_hooks_loaded', + 'theme' => $themePath, + ]); + } catch (\Exception $e) { + Log::error("Failed to load theme hooks: {$e->getMessage()}", [ + 'action' => 'theme_hooks_error', + 'theme' => $themePath, + 'error' => $e->getMessage(), + ]); + } + } + + /** + * Validate hooks.php source code using token_get_all(). + * + * Returns ['valid' => bool, 'reason' => string, 'details' => array] + */ + public function validateSource(string $source, string $themeName): array + { + $tokens = @token_get_all($source); + if (! is_array($tokens)) { + return ['valid' => false, 'reason' => 'hooks.php cannot be parsed', 'details' => []]; + } + + $count = count($tokens); + $dangerousCalls = []; + $hasInlineHtml = false; + $hasBadCharacter = false; + + for ($i = 0; $i < $count; $i++) { + if (! is_array($tokens[$i])) { + continue; + } + + // Detect syntax errors via T_BAD_CHARACTER + if ($tokens[$i][0] === T_BAD_CHARACTER) { + $hasBadCharacter = true; + } + + // Reject inline HTML — hooks.php should be pure PHP + if ($tokens[$i][0] === T_INLINE_HTML) { + $content = trim((string) $tokens[$i][1]); + if ($content !== '') { + $hasInlineHtml = true; + } + } + + // Detect eval() — T_EVAL is a language construct token, not T_STRING + if ($tokens[$i][0] === T_EVAL) { + $dangerousCalls[] = [ + 'function' => 'eval', + 'line' => $tokens[$i][2], + ]; + continue; + } + + // Look for function calls: T_STRING followed by '(' + if ($tokens[$i][0] === T_STRING) { + $funcName = strtolower((string) $tokens[$i][1]); + + // Check if the next non-whitespace token is '(' + $nextIdx = $i + 1; + while ($nextIdx < $count && is_array($tokens[$nextIdx]) && $tokens[$nextIdx][0] === T_WHITESPACE) { + $nextIdx++; + } + + if ($nextIdx < $count && ! is_array($tokens[$nextIdx]) && $tokens[$nextIdx] === '(') { + // Skip if this T_STRING is part of a function/method definition + $prevIdx = $i - 1; + while ($prevIdx >= 0 && is_array($tokens[$prevIdx]) && $tokens[$prevIdx][0] === T_WHITESPACE) { + $prevIdx--; + } + $isDefinition = ($prevIdx >= 0 && is_array($tokens[$prevIdx]) && $tokens[$prevIdx][0] === T_FUNCTION); + + if (! $isDefinition && in_array($funcName, self::DANGEROUS_FUNCTIONS, true)) { + $dangerousCalls[] = [ + 'function' => $funcName, + 'line' => $tokens[$i][2], + ]; + } + } + } + + // BUG FIX (High): Detect variable function calls, e.g. $f('id') or $obj->method(). + if ($tokens[$i][0] === T_VARIABLE) { + $nextIdx = $i + 1; + while ($nextIdx < $count && is_array($tokens[$nextIdx]) && $tokens[$nextIdx][0] === T_WHITESPACE) { + $nextIdx++; + } + + if ($nextIdx < $count && ! is_array($tokens[$nextIdx]) && $tokens[$nextIdx] === '(') { + $dangerousCalls[] = [ + 'function' => 'variable_function_call:' . $tokens[$i][1], + 'line' => $tokens[$i][2], + ]; + } + } + } + + if ($hasBadCharacter) { + return [ + 'valid' => false, + 'reason' => 'hooks.php contains syntax errors', + 'details' => [], + ]; + } + + if ($hasInlineHtml) { + return [ + 'valid' => false, + 'reason' => 'hooks.php must contain only PHP code, no HTML', + 'details' => [], + ]; + } + + if (! empty($dangerousCalls)) { + return [ + 'valid' => false, + 'reason' => 'Dangerous function calls detected: ' . implode(', ', array_column($dangerousCalls, 'function')), + 'details' => $dangerousCalls, + ]; + } + + return ['valid' => true, 'reason' => '', 'details' => []]; + } +} diff --git a/app/Services/ThemeService.php b/app/Services/ThemeService.php new file mode 100644 index 000000000..2ceaa888e --- /dev/null +++ b/app/Services/ThemeService.php @@ -0,0 +1,164 @@ +validator = $validator; + $this->cacheService = $cacheService; + $this->fileUploadService = $fileUploadService; + } + + public function activate(Themes $themes): void + { + Themes::where('active', 1)->update(['active' => 0]); + $themes->update(['active' => 1]); + + $this->clearCache(); + + $this->validator->loadHooks($themes->name); + } + + public function reScan(): void + { + scan_themes(); + $this->clearCache(); + } + + public function clearCache(): void + { + $this->cacheService->removeCacheByKey('active_theme'); + $store = \Illuminate\Support\Facades\Cache::getStore(); + if (method_exists($store, 'tags')) { + \Illuminate\Support\Facades\Cache::tags(['theme_api'])->flush(); + Log::info('Theme cache cleared via tags'); + } else { + $this->cacheService->removeCachePrefix('theme_api'); + Log::info('Theme cache cleared via prefix'); + } + } + + public function installFromZip(UploadedFile $file): array + { + $allowedMimes = FileUploadService::getAllowedMimes('archive'); + + $fileMimeType = $file->getMimeType(); + if (!in_array($fileMimeType, $allowedMimes)) { + return [ + 'status' => 'error', + 'message' => 'File harus berformat .zip', + ]; + } + + $path = $this->fileUploadService->uploadSecure($file, 'framework/themes', $allowedMimes, 51200); + + $fileName = basename($path); + $filePath = storage_path('framework/themes'); + + $zip = new \ZipArchive; + $zipOpened = false; + + try { + if ($zip->open("$filePath/$fileName") === true) { + $zipOpened = true; + + $this->validator->scanZipForPhp($zip); + $this->validator->validateZipEntryPaths($zip); + + $extractedPath = base_path('themes/extracted'); + $zip->extractTo($extractedPath); + $zip->close(); + $zipOpened = false; + + $folderTheme = explode('.', $fileName)[0]; + $composerPath = "$extractedPath/$folderTheme/composer.json"; + + if (file_exists($composerPath)) { + $composerData = json_decode(file_get_contents($composerPath), true); + + $this->validateThemeStructure($extractedPath, $folderTheme); + + $newFolder = base_path('themes/' . $composerData['name']); + + if (File::move("$extractedPath/$folderTheme", $newFolder)) { + File::deleteDirectory($extractedPath); + File::deleteDirectory($filePath); + + scan_themes(); + + $themeName = $composerData['name']; + $userId = auth()->id(); + $userEmail = auth()->user()?->email ?? 'unknown'; + Log::warning("Theme uploaded (hooks NOT auto-loaded): theme={$themeName}, user_id={$userId}, email={$userEmail}", [ + 'action' => 'theme_upload', + 'theme' => $themeName, + 'user_id' => $userId, + ]); + + return [ + 'status' => 'success', + 'message' => 'Tema berhasil diunggah. Review hooks.php melalui menu aktivasi tema sebelum mengaktifkan.', + ]; + } else { + File::deleteDirectory($extractedPath); + File::deleteDirectory($filePath); + } + } + } + } finally { + if ($zipOpened) { + $zip->close(); + } + } + + return [ + 'status' => 'error', + 'message' => 'Tema gagal diunggah (struktur arsip tidak valid atau file hilang)', + ]; + } + + private function validateThemeStructure(string $path, string $folder): bool + { + $requiredFiles = [ + 'composer.json', + 'theme.json', + ]; + + foreach ($requiredFiles as $file) { + if (!file_exists("$path/$folder/$file")) { + throw new \Exception("File {$file} tidak ditemukan di tema"); + } + } + + $themeJsonPath = "$path/$folder/theme.json"; + $themeJsonRaw = file_get_contents($themeJsonPath); + if ($themeJsonRaw === false) { + throw new \Exception("Tidak dapat membaca theme.json dari tema"); + } + + $themeConfig = json_decode($themeJsonRaw, true); + if (! is_array($themeConfig)) { + throw new \Exception("theme.json tidak mengandung JSON yang valid"); + } + + if (!isset($themeConfig['api_version'])) { + Log::warning("Tema tidak mendefinisikan api_version, gunakan v1 sebagai default"); + } + + return true; + } +} From de7a99baf3f29f53d8bf5eefcfc16894b9305fe4 Mon Sep 17 00:00:00 2001 From: habibie11 Date: Fri, 22 May 2026 10:44:36 +0700 Subject: [PATCH 4/4] fix test --- tests/Unit/ThemesControllerSecurityTest.php | 52 +++++++-------------- 1 file changed, 16 insertions(+), 36 deletions(-) diff --git a/tests/Unit/ThemesControllerSecurityTest.php b/tests/Unit/ThemesControllerSecurityTest.php index b994ac314..abde0f5e9 100644 --- a/tests/Unit/ThemesControllerSecurityTest.php +++ b/tests/Unit/ThemesControllerSecurityTest.php @@ -1,18 +1,17 @@ controller = new ThemesController(); + $this->validator = new ThemeHooksValidator(); }); // ── scanZipForPhp tests ── @@ -27,10 +26,7 @@ $z = new ZipArchive(); $z->open($zipPath); - $ref = new ReflectionClass($this->controller); - $m = $ref->getMethod('scanZipForPhp'); - - expect(fn () => $m->invoke($this->controller, $z)) + expect(fn () => $this->validator->scanZipForPhp($z)) ->toThrow(RuntimeException::class); $z->close(); @@ -48,9 +44,7 @@ $z = new ZipArchive(); $z->open($zipPath); - $ref = new ReflectionClass($this->controller); - $m = $ref->getMethod('scanZipForPhp'); - $m->invoke($this->controller, $z); + $this->validator->scanZipForPhp($z); expect(true)->toBeTrue(); // no exception = pass @@ -68,10 +62,7 @@ $z = new ZipArchive(); $z->open($zipPath); - $ref = new ReflectionClass($this->controller); - $m = $ref->getMethod('scanZipForPhp'); - - expect(fn () => $m->invoke($this->controller, $z)) + expect(fn () => $this->validator->scanZipForPhp($z)) ->toThrow(RuntimeException::class); $z->close(); @@ -81,52 +72,41 @@ // ── validateHooksSource tests ── test('validateHooksSource rejects system() call', function () { - $r = invokePrivate('validateHooksSource', ['validator->validateSource('toBeFalse(); }); test('validateHooksSource rejects exec() call', function () { - $r = invokePrivate('validateHooksSource', ['validator->validateSource('toBeFalse(); }); test('validateHooksSource rejects eval() call', function () { - $r = invokePrivate('validateHooksSource', ['validator->validateSource('toBeFalse(); }); test('validateHooksSource rejects file_put_contents() call', function () { - $r = invokePrivate('validateHooksSource', ['validator->validateSource('toBeFalse(); }); test('validateHooksSource rejects shell_exec() call', function () { - $r = invokePrivate('validateHooksSource', ['validator->validateSource('toBeFalse(); }); test('validateHooksSource allows harmless function declaration', function () { - $r = invokePrivate('validateHooksSource', ['validator->validateSource('toBeTrue(); }); test('validateHooksSource allows array return', function () { - $r = invokePrivate('validateHooksSource', [' "b"];', 'clean']); + $r = $this->validator->validateSource(' "b"];', 'clean'); expect($r['valid'])->toBeTrue(); }); test('validateHooksSource rejects inline HTML', function () { - $r = invokePrivate('validateHooksSource', ['

x

validator->validateSource('

x

toBeFalse(); -}); - -// ── Helper ── - -function invokePrivate(string $method, array $args): mixed -{ - // Access via the controller stored in $this (Pest test closure binding) - $controller = test()->controller ?? new ThemesController(); - $ref = new ReflectionClass($controller); - $m = $ref->getMethod($method); - return $m->invokeArgs($controller, $args); -} \ No newline at end of file +}); \ No newline at end of file