Tingkatkan keamanan upload tema by habibie11 · Pull Request #1558 · OpenSID/OpenDK

habibie11 · 2026-05-20T13:22:38Z

📋 Ringkasan

Fitur upload tema memiliki celah keamanan kritis yang memungkinkan penyerang mengeksekusi kode PHP sewenang-wenang di server. Penyerang yang memiliki akses ke halaman upload tema dapat mengunggah file ZIP berisi hooks.php berbahaya yang otomatis dieksekusi melalui include_once setelah ekstraksi.

Attack Chain (Sebelum Fix)

1. POST /setting/themes/upload  →  upload ZIP berisi hooks.php berbahaya
2. ZIP diekstrak ke themes/{name}/
3. loadThemeHooks() dipanggil otomatis setelah upload
4. include_once hooks.php  →  eksekusi kode penyerang di server

🛡️ Remediasi — 4 Lapis Pertahanan

Phase 1: Hapus Auto-Eksekusi `loadThemeHooks()` dari `upload()`

File: app/Http/Controllers/BackEnd/ThemesController.php

Baris $this->loadThemeHooks($composerData['name']) dihapus dari method upload(). Hooks hanya dimuat saat tema diaktifkan secara eksplisit melalui activate(), bukan otomatis saat upload.

Phase 2: Pemindaian Konten ZIP Sebelum Ekstraksi

Method baru: scanZipForPhp(\ZipArchive $zip): void

Memindai semua entri ZIP sebelum extractTo() dipanggil
Menolak file dengan ekstensi berbahaya: php, phtml, php3, php4, php5, pht, inc, phar
Melempar RuntimeException jika file berbahaya terdeteksi
Mencatat ke audit log (Log::critical)

Phase 3: Validasi Sumber `hooks.php` Berbasis Token

Method baru: validateHooksSource(string $source, string $themeName): array

loadThemeHooks() kini memvalidasi isi file hooks.php menggunakan token_get_all() sebelum include_once. Deteksi meliputi:

Kategori	Contoh yang Ditolak
Eksekusi perintah	`system()`, `exec()`, `shell_exec()`, `passthru()`, `popen()`, `proc_open()`
Evaluasi kode	`eval()`, `assert()`, `create_function()`
I/O file	`file_put_contents()`, `file_get_contents()`, `fopen()`, `unlink()`, dll.
Modifikasi environment	`ini_set()`, `putenv()`, `header()`, `setcookie()`
Offuscation	`base64_decode()`, `urldecode()`
Inline HTML	Kode PHP yang bercampur dengan HTML
Language construct	`T_EVAL` token langsung terdeteksi

Fungsi deklarasi (function nama()) tetap diizinkan — hanya pemanggilan fungsi (nama(...)) yang diperiksa.

Phase 4: Isolasi Route Upload ke `super-admin` Saja

File: routes/web.php

Route setting.themes.upload dipisahkan dari grup tema umum dan diberi middleware role:super-admin:

// Sebelumnya: upload dapat diakses oleh semua role dengan akses themes
Route::group(['prefix' => 'themes', 'middleware' => ['action_permission:access.setting.themes']], ...);

// Sekarang: upload hanya bisa diakses oleh super-admin
Route::group(['prefix' => 'themes', 'middleware' => ['role:super-admin']], function () {
    Route::post('upload', 'upload')->name('setting.themes.upload');
});

🧪 Test Coverage — 16/16 Passing

Unit Tests: `tests/Unit/ThemesControllerSecurityTest.php` (11 tests)

scanZipForPhp() — 3 tests:

✅ ZIP berisi .php → RuntimeException
✅ ZIP bersih (theme.json + style.css) → lolos
✅ PHP di subfolder → RuntimeException

validateHooksSource() — 8 tests:

✅ Tolak: system(), exec(), eval(), file_put_contents(), shell_exec()
✅ Terima: deklarasi fungsi, array return
✅ Tolak: inline HTML

Feature Tests: `tests/Feature/Security/ThemeUploadSecurityTest.php` (5 tests)

✅ super-admin dapat akses halaman upload
✅ administrator-website ditolak (403)
✅ Upload ZIP berisi hooks.php → error response
✅ Aktivasi dengan hooks bersih → sukses (302)
✅ Aktivasi dengan hooks berbahaya (passthru) → 500 error

📊 Ringkasan Perubahan

File	Status	Deskripsi
`app/Http/Controllers/BackEnd/ThemesController.php`	Modified	4 phase remediation + constants + audit logging
`routes/web.php`	Modified	Isolasi route upload ke `role:super-admin`
`tests/Unit/ThemesControllerSecurityTest.php`	New	11 unit test untuk `scanZipForPhp` & `validateHooksSource`
`tests/Feature/Security/ThemeUploadSecurityTest.php`	New	5 feature test untuk endpoint upload & activate

🚀 Cara Menjalankan Test

# Unit test (11)
php vendor/bin/pest tests/Unit/ThemesControllerSecurityTest.php

# Feature test (5)
php vendor/bin/pest tests/Feature/Security/ThemeUploadSecurityTest.php

# Semua test keamanan
php vendor/bin/pest --group=security

# Semua test tema
php vendor/bin/pest --group=theme-upload

⚠️ Breaking Changes

loadThemeHooks() tidak lagi dipanggil otomatis setelah upload — hanya saat activate(). Ini perubahan perilaku yang disengaja.
Route upload sekarang terbatas pada role super-admin. Administrator-website tidak lagi dapat mengunggah tema.
hooks.php yang menggunakan fungsi apa pun dalam daftar DANGEROUS_FUNCTIONS akan ditolak saat aktivasi tema.

github-actions · 2026-05-20T13:22:57Z

🔄 AI PR Review sedang antri di server...

Proses review akan segera dimulai di background — hasil akan muncul sebagai komentar setelah selesai.
Powered by CrewAI · PR #1558

pandigresik · 2026-05-21T06:39:48Z

Code Review: Theme Upload Security Hardening (dev-1555 vs dev)

Overview

Branch ini menambahkan security hardening pada upload tema: pembatasan route super-admin, scanning konten ZIP sebelum ekstraksi, validasi hooks.php berbasis token, dan penghapusan pemuatan hook otomatis saat upload. Arah keamanan sudah tepat, namun ada beberapa bypass vector dan bug yang perlu diperbaiki.

BUG — `call_user_func` / `call_user_func_array` bypass di `validateHooksSource` (Severity: Critical)

File: app/Http/Controllers/BackEnd/ThemesController.php:17-28

call_user_func dan call_user_func_array tidak ada di DANGEROUS_FUNCTIONS. Penyerang bisa mengeksekusi kode arbitrer di hooks.php:

<?php call_user_func('system', 'id');
<?php call_user_func_array('passthru', ['id']);

Token scanner melihat call_user_func sebagai T_STRING diikuti (, tapi karena tidak ada di blocklist, validasi lolos. Ini sepenuhnya mengalahkan validasi berbasis token.

Fix: Tambahkan call_user_func dan call_user_func_array ke DANGEROUS_FUNCTIONS.

BUG — Variable function calls bypass `validateHooksSource` (Severity: High)

File: app/Http/Controllers/BackEnd/ThemesController.php:308-334

Scanner hanya mendeteksi T_STRING diikuti (. Tidak akan menangkap:

<?php $f = 'system'; $f('id');
<?php ${'sys' . 'tem'}('id');

Pola ini adalah valid PHP untuk pemanggilan fungsi dinamis. Urutan token untuk $f('id') adalah T_VARIABLE lalu (, bukan T_STRING lalu (, sehingga scanner melewatkannya.

Fix: Blokir T_VARIABLE diikuti ( di level atas.

BUG — ZIP path traversal tidak divalidasi (Severity: High)

File: app/Http/Controllers/BackEnd/ThemesController.php:370-392

scanZipForPhp memeriksa ekstensi file tapi tidak memvalidasi sequence path traversal. ZIP yang berisi:

../../../storage/framework/cache/data.php

akan lolos scan (.php akan tertangkap, tapi .txt, .json, dll tidak) lalu diekstrak di luar themes/extracted/ di baris 107. Ini memungkinkan overwrite file arbitrer di disk.

Fix: Sebelum ekstraksi, validasi bahwa tidak ada entri yang mengandung .. atau dimulai dengan /.

BUG — Exception message terekspos ke user (Severity: Medium)

File: app/Http/Controllers/BackEnd/ThemesController.php:155-158

'message' => 'Tema gagal diunggah: ' . $e->getMessage(),

Pesan exception mentah dikembalikan ke client. Ini bisa mengekspos path internal, query SQL, atau fragmen stack trace.

Fix: Kembalikan pesan generik ke user dan log detailnya (sudah dilakukan).

BUG — ZIP handle leak saat `scanZipForPhp` throw exception (Severity: Low)

File: app/Http/Controllers/BackEnd/ThemesController.php:102-108

Ketika scanZipForPhp throw exception di baris 104, resource $zip tidak pernah ditutup. Block catch di baris 149 menangani exception tapi tidak menutup archive.

Fix: Tutup ZIP sebelum throw, atau gunakan block finally.

OBSERVATION — `activate()` throw `RuntimeException` tanpa catch saat hooks buruk

File: app/Http/Controllers/BackEnd/ThemesController.php:243

Ketika validateHooksSource menolak hooks.php, loadThemeHooks throw RuntimeException yang keluar dari activate() tanpa tertangkap (baris 55). Ini menghasilkan error 500 daripada redirect dengan pesan error yang user-friendly. Test di tests/Feature/Security/ThemeUploadSecurityTest.php:107 secara eksplisit mengharapkan 500, jadi ini tampak disengaja, tapi pertimbangkan apakah redirect dengan flash error akan lebih baik untuk UX.

OBSERVATION — Duplicate prefix route group `themes`

File: routes/web.php:964-975

Dua route group terpisah berbagi prefix themes — satu dengan action_permission:access.setting.themes dan satu dengan role:super-admin. Ini berfungsi karena path-nya berbeda, tapi mudah untuk secara tidak sengaja menambahkan route ke group yang salah. Pertimbangkan untuk konsolidasi ke satu group dengan middleware gabungan di mana sesuai, atau tambahkan komentar yang menjelaskan pemisahan.

OBSERVATION — `scanZipForPhp` tidak skip entri direktori

File: app/Http/Controllers/BackEnd/ThemesController.php:374

Arsip ZIP menyertakan entri direktori (sering diakhiri dengan /). pathinfo('some-dir/', PATHINFO_EXTENSION) mengembalikan '', yang tidak akan cocok dengan ekstensi berbahaya, jadi ini bukan bug. Namun, secara eksplisit skip entri yang diakhiri dengan / akan membuat intent lebih jelas dan menghindari pemanggilan pathinfo yang tidak perlu.

Analisis Fat Controller: ThemesController (430 baris)

Kesimpulan

Controller ini terlalu gemuk dengan beberapa tanggung jawab yang bercampur. Disarankan memisahkan ke service dan class validator.

1. `ThemeService` - Extract Business Logic

Candidate methods:

activate() - aktivasi tema
reScan() - scan tema
validateThemeStructure() - validasi struktur
Logika upload/install dari upload() (extract ZIP, move folder, scan)

Alasan: Controller seharusnya hanya menangani HTTP request/response, bukan business logic seperti manipulasi file system dan validasi struktur tema.

ThemesController (remaining)
├── index()
├── activate() → ThemeService::activate()
├── reScan() → ThemeService::rescan()
├── upload() → ThemeService::installFromZip()
├── destroy()
└── clearCache() → ThemeService::clearCache()

2. `ThemeHooksValidator` - Extract Security Logic

Candidate methods:

validateHooksSource() (95 baris)
scanZipForPhp() (22 baris)
loadThemeHooks() (40 baris)
Constants: DANGEROUS_FUNCTIONS, DANGEROUS_EXTENSIONS

Alasan: Validasi token-based ini adalah domain security yang standalone dan bisa di-reuse. 117+ baris kode security yang kompleks tidak seharusnya ada di controller.

// app/Services/ThemeHooksValidator.php
class ThemeHooksValidator
{
    private const DANGEROUS_FUNCTIONS = [...];
    private const DANGEROUS_EXTENSIONS = [...];
    
    public function validateSource(string $source, string $themeName): array
    public function scanZipForPhp(\ZipArchive $zip): void
    public function loadHooks(string $themePath): void
}

3. Cache Management - Gunakan `CacheService` yang Ada

Masalah: clearThemeCache() (19 baris) mengimplementasikan logic cache yang sudah ada di CacheService:

// Sekarang (ThemesController:187-205):
Cache::forget('active_theme');
$store = Cache::getStore();
if (method_exists($store, 'tags')) {
    Cache::tags(['theme_api'])->flush();
} else {
    $keys = Cache::get('theme_api:*');
    foreach ($keys ?? [] as $key) {
        Cache::forget($key);
    }
}

// Seharusnya bisa menggunakan CacheService yang sudah ada:
$cacheService->removeCacheByKey('active_theme');
$cacheService->removeCachePrefix('theme_api');

4. Bug/Issue Tambahan yang Ditemukan

a. upload() - Fallback error tanpa konteks (line 161-164)

Jika $zip->open() gagal atau composer.json tidak ada, controller mengembalikan error generik "Tema gagal diunggah" tanpa log atau pesan spesifik. User tidak tahu kenapa upload gagal.

b. upload() - Resource leak potensial (line 101-148)

Jika exception terjadi setelah $zip->open() tapi sebelum $zip->close(), file ZIP tetap terbuka. Gunakan finally block atau try-with-resources pattern.

c. validateThemeStructure() - Double file_get_contents (line 411)

$themeConfig = json_decode(file_get_contents("$path/$folder/theme.json"), true);
// Tidak ada pengecekan jika file_get_contents return false
// Tidak ada pengecekan jika json_decode return null (invalid JSON)

d. loadThemeHooks() - Exception tidak di-rethrow (line 253-259)

} catch (\Exception $e) {
    Log::error("Failed to load theme hooks: {$e->getMessage()}");
    // Exception di-silence, hooks gagal load tapi tidak ada yang tahu
}

Estimasi Setelah Refactoring

Komponen	Sebelum	Sesudah
ThemesController	430 lines	~80 lines
ThemeService	0	~150 lines
ThemeHooksValidator	0	~120 lines
CacheService (existing)	-	reuse

Total pemisahan: ~350 baris keluar dari controller.

habibie11 · 2026-05-21T10:40:36Z

Code Review: Theme Upload Security Hardening (dev-1555 vs dev)

Overview

Branch ini menambahkan security hardening pada upload tema: pembatasan route super-admin, scanning konten ZIP sebelum ekstraksi, validasi hooks.php berbasis token, dan penghapusan pemuatan hook otomatis saat upload. Arah keamanan sudah tepat, namun ada beberapa bypass vector dan bug yang perlu diperbaiki.

BUG — call_user_func / call_user_func_array bypass di validateHooksSource (Severity: Critical)

File: app/Http/Controllers/BackEnd/ThemesController.php:17-28

call_user_func dan call_user_func_array tidak ada di DANGEROUS_FUNCTIONS. Penyerang bisa mengeksekusi kode arbitrer di hooks.php:
<?php call_user_func('system', 'id');
<?php call_user_func_array('passthru', ['id']);
Token scanner melihat call_user_func sebagai T_STRING diikuti (, tapi karena tidak ada di blocklist, validasi lolos. Ini sepenuhnya mengalahkan validasi berbasis token.

Fix: Tambahkan call_user_func dan call_user_func_array ke DANGEROUS_FUNCTIONS.

BUG — Variable function calls bypass validateHooksSource (Severity: High)

File: app/Http/Controllers/BackEnd/ThemesController.php:308-334

Scanner hanya mendeteksi T_STRING diikuti (. Tidak akan menangkap:
<?php $f = 'system'; $f('id');
<?php ${'sys' . 'tem'}('id');
Pola ini adalah valid PHP untuk pemanggilan fungsi dinamis. Urutan token untuk $f('id') adalah T_VARIABLE lalu (, bukan T_STRING lalu (, sehingga scanner melewatkannya.

Fix: Blokir T_VARIABLE diikuti ( di level atas.

BUG — ZIP path traversal tidak divalidasi (Severity: High)

File: app/Http/Controllers/BackEnd/ThemesController.php:370-392

scanZipForPhp memeriksa ekstensi file tapi tidak memvalidasi sequence path traversal. ZIP yang berisi:
../../../storage/framework/cache/data.php
akan lolos scan (.php akan tertangkap, tapi .txt, .json, dll tidak) lalu diekstrak di luar themes/extracted/ di baris 107. Ini memungkinkan overwrite file arbitrer di disk.

Fix: Sebelum ekstraksi, validasi bahwa tidak ada entri yang mengandung .. atau dimulai dengan /.

BUG — Exception message terekspos ke user (Severity: Medium)

File: app/Http/Controllers/BackEnd/ThemesController.php:155-158
'message' => 'Tema gagal diunggah: ' . $e->getMessage(),
Pesan exception mentah dikembalikan ke client. Ini bisa mengekspos path internal, query SQL, atau fragmen stack trace.

Fix: Kembalikan pesan generik ke user dan log detailnya (sudah dilakukan).

BUG — ZIP handle leak saat scanZipForPhp throw exception (Severity: Low)

File: app/Http/Controllers/BackEnd/ThemesController.php:102-108

Ketika scanZipForPhp throw exception di baris 104, resource $zip tidak pernah ditutup. Block catch di baris 149 menangani exception tapi tidak menutup archive.

Fix: Tutup ZIP sebelum throw, atau gunakan block finally.

OBSERVATION — activate() throw RuntimeException tanpa catch saat hooks buruk

File: app/Http/Controllers/BackEnd/ThemesController.php:243

Ketika validateHooksSource menolak hooks.php, loadThemeHooks throw RuntimeException yang keluar dari activate() tanpa tertangkap (baris 55). Ini menghasilkan error 500 daripada redirect dengan pesan error yang user-friendly. Test di tests/Feature/Security/ThemeUploadSecurityTest.php:107 secara eksplisit mengharapkan 500, jadi ini tampak disengaja, tapi pertimbangkan apakah redirect dengan flash error akan lebih baik untuk UX.

OBSERVATION — Duplicate prefix route group themes

File: routes/web.php:964-975

Dua route group terpisah berbagi prefix themes — satu dengan action_permission:access.setting.themes dan satu dengan role:super-admin. Ini berfungsi karena path-nya berbeda, tapi mudah untuk secara tidak sengaja menambahkan route ke group yang salah. Pertimbangkan untuk konsolidasi ke satu group dengan middleware gabungan di mana sesuai, atau tambahkan komentar yang menjelaskan pemisahan.

OBSERVATION — scanZipForPhp tidak skip entri direktori

File: app/Http/Controllers/BackEnd/ThemesController.php:374

Arsip ZIP menyertakan entri direktori (sering diakhiri dengan /). pathinfo('some-dir/', PATHINFO_EXTENSION) mengembalikan '', yang tidak akan cocok dengan ekstensi berbahaya, jadi ini bukan bug. Namun, secara eksplisit skip entri yang diakhiri dengan / akan membuat intent lebih jelas dan menghindari pemanggilan pathinfo yang tidak perlu.

Analisis Fat Controller: ThemesController (430 baris)

Kesimpulan

Controller ini terlalu gemuk dengan beberapa tanggung jawab yang bercampur. Disarankan memisahkan ke service dan class validator.

1. ThemeService - Extract Business Logic

Candidate methods:

activate() - aktivasi tema

reScan() - scan tema

validateThemeStructure() - validasi struktur

Logika upload/install dari upload() (extract ZIP, move folder, scan)

Alasan: Controller seharusnya hanya menangani HTTP request/response, bukan business logic seperti manipulasi file system dan validasi struktur tema.
ThemesController (remaining)
├── index()
├── activate() → ThemeService::activate()
├── reScan() → ThemeService::rescan()
├── upload() → ThemeService::installFromZip()
├── destroy()
└── clearCache() → ThemeService::clearCache()
2. ThemeHooksValidator - Extract Security Logic

Candidate methods:

validateHooksSource() (95 baris)

scanZipForPhp() (22 baris)

loadThemeHooks() (40 baris)

Constants: DANGEROUS_FUNCTIONS, DANGEROUS_EXTENSIONS

Alasan: Validasi token-based ini adalah domain security yang standalone dan bisa di-reuse. 117+ baris kode security yang kompleks tidak seharusnya ada di controller.
// app/Services/ThemeHooksValidator.php
class ThemeHooksValidator
{
    private const DANGEROUS_FUNCTIONS = [...];
    private const DANGEROUS_EXTENSIONS = [...];
    
    public function validateSource(string $source, string $themeName): array
    public function scanZipForPhp(\ZipArchive $zip): void
    public function loadHooks(string $themePath): void
}
3. Cache Management - Gunakan CacheService yang Ada

Masalah: clearThemeCache() (19 baris) mengimplementasikan logic cache yang sudah ada di CacheService:
// Sekarang (ThemesController:187-205):
Cache::forget('active_theme');
$store = Cache::getStore();
if (method_exists($store, 'tags')) {
    Cache::tags(['theme_api'])->flush();
} else {
    $keys = Cache::get('theme_api:*');
    foreach ($keys ?? [] as $key) {
        Cache::forget($key);
    }
}

// Seharusnya bisa menggunakan CacheService yang sudah ada:
$cacheService->removeCacheByKey('active_theme');
$cacheService->removeCachePrefix('theme_api');
4. Bug/Issue Tambahan yang Ditemukan

a. upload() - Fallback error tanpa konteks (line 161-164)

Jika $zip->open() gagal atau composer.json tidak ada, controller mengembalikan error generik "Tema gagal diunggah" tanpa log atau pesan spesifik. User tidak tahu kenapa upload gagal.

b. upload() - Resource leak potensial (line 101-148)

Jika exception terjadi setelah $zip->open() tapi sebelum $zip->close(), file ZIP tetap terbuka. Gunakan finally block atau try-with-resources pattern.

c. validateThemeStructure() - Double file_get_contents (line 411)
$themeConfig = json_decode(file_get_contents("$path/$folder/theme.json"), true);
// Tidak ada pengecekan jika file_get_contents return false
// Tidak ada pengecekan jika json_decode return null (invalid JSON)
d. loadThemeHooks() - Exception tidak di-rethrow (line 253-259)
} catch (\Exception $e) {
    Log::error("Failed to load theme hooks: {$e->getMessage()}");
    // Exception di-silence, hooks gagal load tapi tidak ada yang tahu
}
Estimasi Setelah Refactoring

Komponen Sebelum Sesudah
ThemesController 430 lines ~80 lines
ThemeService 0 ~150 lines
ThemeHooksValidator 0 ~120 lines
CacheService (existing) - reuse
Total pemisahan: ~350 baris keluar dari controller.

a1916ee

untuk memisahkan ke ThemeHooksValidator + ThemeService + menggunakan CacheService. Ini bersifat opsional dan bisa dikerjakan sebagai PR terpisah jika diperlukan atau jika ingin sekalian dikerjakan di PR ini?

pandigresik · 2026-05-21T23:43:07Z

ThemeService

sekalian saja, agar controllernya tidak terlalu gemuk. Karena PR ini perubahan pada controller tersebut

…yanan 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.

habibie11 · 2026-05-22T02:28:18Z

ThemeService

sekalian saja, agar controllernya tidak terlalu gemuk. Karena PR ini perubahan pada controller tersebut

sudah, silahkan di cek mas.

pandigresik · 2026-05-22T04:07:51Z

OK

Tingkatkan keamanan upload tema

8973ab9

habibie11 requested a review from affandii06 May 20, 2026 13:23

affandii06 assigned pandigresik May 21, 2026

affandii06 added this to the M4 OpenDK 2606 milestone May 21, 2026

perbaikan review

a1916ee

fix test

de7a99b

pandigresik approved these changes May 22, 2026

View reviewed changes

Conversation

habibie11 commented May 20, 2026

📋 Ringkasan

Attack Chain (Sebelum Fix)

🛡️ Remediasi — 4 Lapis Pertahanan

Phase 1: Hapus Auto-Eksekusi loadThemeHooks() dari upload()

Phase 2: Pemindaian Konten ZIP Sebelum Ekstraksi

Phase 3: Validasi Sumber hooks.php Berbasis Token

Phase 4: Isolasi Route Upload ke super-admin Saja

🧪 Test Coverage — 16/16 Passing

Unit Tests: tests/Unit/ThemesControllerSecurityTest.php (11 tests)

Feature Tests: tests/Feature/Security/ThemeUploadSecurityTest.php (5 tests)

📊 Ringkasan Perubahan

🚀 Cara Menjalankan Test

⚠️ Breaking Changes

Uh oh!

github-actions Bot commented May 20, 2026

Uh oh!

pandigresik commented May 21, 2026

Code Review: Theme Upload Security Hardening (dev-1555 vs dev)

Overview

BUG — call_user_func / call_user_func_array bypass di validateHooksSource (Severity: Critical)

BUG — Variable function calls bypass validateHooksSource (Severity: High)

BUG — ZIP path traversal tidak divalidasi (Severity: High)

BUG — Exception message terekspos ke user (Severity: Medium)

BUG — ZIP handle leak saat scanZipForPhp throw exception (Severity: Low)

OBSERVATION — activate() throw RuntimeException tanpa catch saat hooks buruk

OBSERVATION — Duplicate prefix route group themes

OBSERVATION — scanZipForPhp tidak skip entri direktori

Analisis Fat Controller: ThemesController (430 baris)

Kesimpulan

1. ThemeService - Extract Business Logic

2. ThemeHooksValidator - Extract Security Logic

3. Cache Management - Gunakan CacheService yang Ada

4. Bug/Issue Tambahan yang Ditemukan

Estimasi Setelah Refactoring

Uh oh!

habibie11 commented May 21, 2026

Code Review: Theme Upload Security Hardening (dev-1555 vs dev)

Overview

BUG — call_user_func / call_user_func_array bypass di validateHooksSource (Severity: Critical)

BUG — Variable function calls bypass validateHooksSource (Severity: High)

BUG — ZIP path traversal tidak divalidasi (Severity: High)

BUG — Exception message terekspos ke user (Severity: Medium)

BUG — ZIP handle leak saat scanZipForPhp throw exception (Severity: Low)

OBSERVATION — activate() throw RuntimeException tanpa catch saat hooks buruk

OBSERVATION — Duplicate prefix route group themes

OBSERVATION — scanZipForPhp tidak skip entri direktori

Analisis Fat Controller: ThemesController (430 baris)

Kesimpulan

1. ThemeService - Extract Business Logic

2. ThemeHooksValidator - Extract Security Logic

3. Cache Management - Gunakan CacheService yang Ada

4. Bug/Issue Tambahan yang Ditemukan

Estimasi Setelah Refactoring

Uh oh!

pandigresik commented May 21, 2026

Uh oh!

habibie11 commented May 22, 2026

Uh oh!

pandigresik commented May 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Phase 1: Hapus Auto-Eksekusi `loadThemeHooks()` dari `upload()`

Phase 3: Validasi Sumber `hooks.php` Berbasis Token

Phase 4: Isolasi Route Upload ke `super-admin` Saja

Unit Tests: `tests/Unit/ThemesControllerSecurityTest.php` (11 tests)

Feature Tests: `tests/Feature/Security/ThemeUploadSecurityTest.php` (5 tests)

BUG — `call_user_func` / `call_user_func_array` bypass di `validateHooksSource` (Severity: Critical)

BUG — Variable function calls bypass `validateHooksSource` (Severity: High)

BUG — ZIP handle leak saat `scanZipForPhp` throw exception (Severity: Low)

OBSERVATION — `activate()` throw `RuntimeException` tanpa catch saat hooks buruk

OBSERVATION — Duplicate prefix route group `themes`

OBSERVATION — `scanZipForPhp` tidak skip entri direktori

1. `ThemeService` - Extract Business Logic

2. `ThemeHooksValidator` - Extract Security Logic

3. Cache Management - Gunakan `CacheService` yang Ada

BUG — `call_user_func` / `call_user_func_array` bypass di `validateHooksSource` (Severity: Critical)

BUG — Variable function calls bypass `validateHooksSource` (Severity: High)

BUG — ZIP handle leak saat `scanZipForPhp` throw exception (Severity: Low)

OBSERVATION — `activate()` throw `RuntimeException` tanpa catch saat hooks buruk

OBSERVATION — Duplicate prefix route group `themes`

OBSERVATION — `scanZipForPhp` tidak skip entri direktori

1. `ThemeService` - Extract Business Logic

2. `ThemeHooksValidator` - Extract Security Logic

3. Cache Management - Gunakan `CacheService` yang Ada