Fix: upload themes#1620
Open
pandigresik wants to merge 6 commits into
Open
Conversation
Contributor
|
🔄 AI PR Review sedang antri di server...
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request: Fix upload themes validation and path issues
Description
Fix multiple issues in theme upload flow: invalid file uploads were not being validated before processing, temporary extraction path used the wrong storage disk, uploaded filename hash did not match the folder inside zip, and
scanZipForPhpincorrectly blocked Blade template files. Also updates.gitignoreto properly exclude non-default theme directories.Changes made:
ThemesController.php): Added$file->isValid()check before processing upload to catch corrupted or failed uploads early and return a specific error messageThemeService.php): Fixed storage path fromstorage_path('framework/themes')toStorage::disk('public')->path('framework/themes')so the temporary zip file is found during extractionThemeService.php): Replacedexplode('.', $fileName)[0]folder guessing with recursivefindComposerJson()search to handle renamed files fromuploadSecureThemeHooksValidator.php): Added.blade.phpskip inscanZipForPhpso legitimate Blade template files are not blocked.gitignore): Uncommented/themes/*rule to ignore non-default theme directoriesReason for change:
upload_max_filesize), no error was returned to the user, causing a generic failure laterstorage_path()returns the default Laravel storage path, but the file was actually stored on the public disk viaFileUploadService, so the zip could not be found for extractionuploadSecurerenames files to{timestamp}_{random}.zip, so deriving folder name from filename produced a non-existent pathscanZipForPhprejected all.phpfiles including safe.blade.phptemplates, preventing valid themes from being uploadedthemes/opendk/buenwere not excluded from version controlImpact of change:
✅ Better Error Messages: Users now see specific error messages for invalid file uploads and missing
composer.json✅ Reliable Extraction: Zip files are now found and extracted from the correct disk path; folder name auto-detected regardless of uploaded filename
✅ Blade Templates Allowed: Legitimate
.blade.phpfiles pass through while dangerous.phpfiles are still blocked✅ Cleanup: Temporary files are always cleaned up on failure
✅ Version Control: Only the default theme is tracked; other themes are properly ignored
Related Issue
#1619
Steps to Reproduce
Before fix (problem):
composer.jsonAfter fix (solution):
Testing on related features:
.blade.phpfiles) ✅ WorkinguploadSecure✅ Folder auto-detected correctlyscanZipForPhpblocks dangerous.phpbut allows.blade.php✅ WorkingChecklist
Technical Details
Technical Explanation
ThemesController.php (
uploadmethod):$file->isValid()check after the null check, returning a JSON error with$file->getErrorMessage()if the upload failed during PHP processingThemeService.php (
installFromZipmethod):$filePath = storage_path('framework/themes')to$filePath = Storage::disk('public')->path('framework/themes')—FileUploadService::uploadSecure()stores files on the public disk, so the path must resolve to the same disk$folderTheme = explode('.', $fileName)[0]to guess the extracted folder name, butuploadSecurerenames the file to{timestamp}_{random}.zip, causing a mismatch. Replaced with newfindComposerJson()method that recursively searches forcomposer.jsonin the extracted directory usingRecursiveDirectoryIterator, making folder detection reliable regardless of uploaded filenameThemeHooksValidator.php (
scanZipForPhpmethod):str_ends_with($filename, '.blade.php')check to skip Blade template files before checking against dangerous extensions. Blade files are safe — they are rendered by Laravel's template engine and cannot execute arbitrary PHP functions likeexec,system,eval.gitignore:
/themes/*rule so that only!/themes/opendk/default/is tracked; other themes likethemes/opendk/buenare properly ignoredConfiguration changes
.gitignore: Enabled/themes/*ignore patternDependencies added
No new dependencies
Testing
Manual Testing
Automated Testing
Breaking Changes
None
Migration Guide
Not required
References
Video
simplescreenrecorder-2026-07-08_16.36.32.mp4