Conversation
📝 WalkthroughWalkthroughThis PR adds a CKEditor 5 based rich text editor to the Vue frontend, replacing the prior in-component CKEditor setup used by CkEditorField. It introduces a CkEditor.vue wrapper with configurable toolbar/plugins/upload, an EditorAssetPicker modal for browsing existing assets, an upload adapter, and an asset browser plugin. Backend support is added via a new EditorUploadService, DTOs, and EditorUploadController exposing upload/assets endpoints, plus config binding and a composer dependency. Tests and README documentation are added accordingly. Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CkEditor as CkEditor.vue
participant Picker as EditorAssetPicker
participant Adapter as EditorUploadAdapter
participant Controller as EditorUploadController
participant Service as EditorUploadService
User->>CkEditor: open editor / drag image
CkEditor->>Adapter: upload(file)
Adapter->>Controller: POST /editor/upload
Controller->>Service: storeImage(file)
Service-->>Controller: EditorUploadResult
Controller-->>Adapter: {url, fileName}
Adapter-->>CkEditor: insert uploaded image
User->>CkEditor: click "Browse files"
CkEditor->>Picker: open, loadAssets()
Picker->>Controller: GET /editor/assets
Controller->>Service: listAssets()
Service-->>Controller: EditorAssetItem[]
Controller-->>Picker: items
User->>Picker: select asset
Picker-->>CkEditor: emit select(asset)
CkEditor->>CkEditor: insertAsset into document
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
tests/Integration/Controller/EditorUploadControllerTest.php (1)
70-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid reusing
$uploadedFilevariable name for a file path.Line 70 reassigns
$uploadedFile(previously anUploadedFileobject) to a string path. This shadowing is confusing and could lead to bugs if the original object is referenced later. Use a distinct variable name.♻️ Proposed refactor: rename variable
- $uploadedFile = $projectDir . '/public/uploadimages/ckeditor5/' . $payload['fileName']; - if (is_file($uploadedFile)) { - unlink($uploadedFile); + $uploadedFilePath = $projectDir . '/public/uploadimages/ckeditor5/' . $payload['fileName']; + if (is_file($uploadedFilePath)) { + unlink($uploadedFilePath); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Integration/Controller/EditorUploadControllerTest.php` around lines 70 - 73, The test cleanup code is reusing $uploadedFile for a string path after it already referred to an UploadedFile object, which is confusing and can lead to accidental misuse. In EditorUploadControllerTest, rename the path variable to something distinct like an uploaded file path name and keep $uploadedFile reserved for the UploadedFile instance, updating the is_file/unlink cleanup block accordingly.src/Service/EditorUploadService.php (1)
91-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTighten visibility of internal helper methods.
buildRelativeUrl()andgetTargetDirectory()are public but only used within the service and its tests. Making them private reduces the public API surface and prevents accidental misuse.♻️ Proposed refactor: make helpers private
- public function buildRelativeUrl(string $fileName): string + private function buildRelativeUrl(string $fileName): string { return sprintf( '/%s/%s/%s', trim($this->editorImagesDir, '/'), self::STORAGE_SUBDIRECTORY, ltrim($fileName, '/') ); } - public function getTargetDirectory(): string + private function getTargetDirectory(): string { return rtrim($this->projectDir, '/') . '/public/' . trim($this->editorImagesDir, '/') . '/' . self::STORAGE_SUBDIRECTORY; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Service/EditorUploadService.php` around lines 91 - 108, Both EditorUploadService helper methods are too broadly exposed for internal-only behavior. Change buildRelativeUrl() and getTargetDirectory() in EditorUploadService to private, and update any direct test usage to exercise them through the service’s public methods or adjust test setup as needed so the internal helpers are no longer part of the public API.tests/Unit/Service/EditorUploadServiceTest.php (1)
79-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared
removePathhelper to a trait or base class.The
removePath()method is duplicated verbatim inEditorUploadControllerTest.php. Extracting it to a shared trait or test base class would eliminate the duplication.♻️ Proposed refactor: extract to trait
+<?php + +declare(strict_types=1); + +namespace PhpList\WebFrontend\Tests\Support; + +trait TempDirCleanupTrait +{ + private function removePath(string $path): void + { + if (is_file($path) || is_link($path)) { + unlink($path); + return; + } + + if (!is_dir($path)) { + return; + } + + foreach (scandir($path) as $item) { + if ($item === '.' || $item === '..') { + continue; + } + $this->removePath($path . DIRECTORY_SEPARATOR . $item); + } + + rmdir($path); + } +}Then in both test files:
+use PhpList\WebFrontend\Tests\Support\TempDirCleanupTrait; + final class EditorUploadServiceTest extends TestCase { + use TempDirCleanupTrait; + - private function removePath(string $path): void - { - // ... duplicated method body - } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/Service/EditorUploadServiceTest.php` around lines 79 - 100, The removePath() helper is duplicated in EditorUploadServiceTest and EditorUploadControllerTest, so extract it into a shared test trait or base class and reuse that single implementation from both tests. Move the recursive filesystem cleanup logic into the shared helper, then update each test class to import or extend it so the duplicated private method can be removed.assets/editor/CkEditor.vue (1)
177-200: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout to the asset-loading fetch.
loadAssetshas no timeout orAbortController. If the backend is slow or unresponsive,assetLoadingstaystrueindefinitely and the picker shows a perpetual spinner with no recourse for the user.♻️ Proposed refactor with AbortController
const loadAssets = async () => { assetLoading.value = true; assetError.value = ''; + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 15000); try { const response = await fetch(props.assetsEndpoint, { headers: { 'X-Requested-With': 'XMLHttpRequest', }, credentials: props.withCredentials ? 'include' : 'same-origin', + signal: controller.signal, }); if (!response.ok) { throw new Error(`Failed to load assets (${response.status})`); } const payload = await response.json(); assetItems.value = Array.isArray(payload?.items) ? payload.items : []; } catch (error) { - assetError.value = error?.message || 'Failed to load assets.'; + assetError.value = error?.name === 'AbortError' + ? 'Asset request timed out.' + : error?.message || 'Failed to load assets.'; } finally { + clearTimeout(timeoutId); assetLoading.value = false; } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets/editor/CkEditor.vue` around lines 177 - 200, The loadAssets async flow in CkEditor.vue has no timeout or abort handling, so a slow request can leave assetLoading stuck on and the picker spinning forever. Update loadAssets to use an AbortController (or equivalent timeout mechanism) around the fetch call, cancel the request after a reasonable limit, and handle the abort/error case by setting assetError while always clearing assetLoading in finally.tests/Unit/assets/editor/uploadAdapter.spec.js (1)
6-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests cover the happy and error paths correctly.
The XHR mock pattern is clean — storing listeners by event name and allowing manual triggering gives precise control over the async flow. The success test correctly awaits
Promise.resolve()to letloader.filesettle before firingload, and the error test verifies both default endpoint behavior anderror.messageextraction.The main gaps are
abort()(no test calls it or verifiesxhr.abort()is invoked) and theerrorevent (network-level failure path). Adding these would round out coverage for the adapter's failure modes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/assets/editor/uploadAdapter.spec.js` around lines 6 - 78, The EditorUploadAdapter spec covers upload success and backend error response, but it is missing failure-path coverage for abort and network errors. Extend the existing EditorUploadAdapter tests to exercise adapter.abort() and verify that xhr.abort() is called, and add a case that triggers the XHR error event to confirm the adapter rejects correctly on transport-level failure; use the existing sendMock/listeners setup and the adapter.xhr instance so the new assertions stay aligned with the current test pattern.tests/Unit/assets/editor/CkEditor.spec.js (1)
95-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExisting tests cover the core contract well.
V-model sync, config shape (
licenseKey,toolbar,htmlSupport),openAssetPickerexposure, andreadonly→disabledmapping are all verified correctly against the component'seditorConfigcomputed andisDisabledcomputed.The stub's
.readybutton is available but no test triggers it, sohandleReadyside effects — upload adapter installation, readonly sync, andeditorRefassignment — remain untested. Consider adding a case that clicks.readyand assertsinstallUploadAdapterwas called (e.g., by spying oneditor.plugins.get('FileRepository').createUploadAdapter) and thateditorRefis populated forinsertAssetuse.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/assets/editor/CkEditor.spec.js` around lines 95 - 140, The CkEditor tests cover props and v-model, but they do not exercise the handleReady path exposed by the stub’s ready control. Add a test that clicks the .ready button on the mounted CkEditor wrapper and verify the handleReady side effects: editorRef is set, readonly state is synced, and installUploadAdapter runs by spying on editor.plugins.get('FileRepository').createUploadAdapter or the equivalent upload adapter hook.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@assets/editor/EditorAssetPicker.vue`:
- Around line 55-110: The filtered asset table in EditorAssetPicker.vue has no
empty-state, so when filteredItems is empty it shows only the header row and
looks broken. Update the table rendering around the v-for on filteredItems to
conditionally show a single full-width tbody row with a “No assets found”
message when there are no results, while keeping the existing item rows and
select button behavior intact.
- Around line 1-9: The modal in EditorAssetPicker.vue already has the dialog
semantics but lacks keyboard dismissal and focus handling. Add an Escape-key
handler on the overlay div that closes the picker when open, and in the <script
setup> for EditorAssetPicker, wire basic focus management on open so keyboard
users land inside the dialog instead of needing to tab to the Close button.
In `@src/Service/EditorUploadService.php`:
- Around line 128-139: Block SVG handling in EditorUploadService to prevent
stored XSS: guessExtensionFromMime currently maps image/svg+xml to svg, and
storeImage accepts any image/* MIME, so SVGs can be uploaded and served inline.
Update storeImage to explicitly reject image/svg+xml before extension guessing,
and remove or bypass the svg case in guessExtensionFromMime so only safe raster
formats are stored.
- Around line 26-47: The storeImage method in EditorUploadService currently
validates only upload status and MIME type, but it never enforces an
application-level size limit. Add a file-size check near the start of
storeImage, before any filesystem work or move call, and reject oversized
uploads by throwing a RuntimeException with a clear message. Use the existing
UploadedFile object and keep the new limit consistent with the upload policy
used by this service.
---
Nitpick comments:
In `@assets/editor/CkEditor.vue`:
- Around line 177-200: The loadAssets async flow in CkEditor.vue has no timeout
or abort handling, so a slow request can leave assetLoading stuck on and the
picker spinning forever. Update loadAssets to use an AbortController (or
equivalent timeout mechanism) around the fetch call, cancel the request after a
reasonable limit, and handle the abort/error case by setting assetError while
always clearing assetLoading in finally.
In `@src/Service/EditorUploadService.php`:
- Around line 91-108: Both EditorUploadService helper methods are too broadly
exposed for internal-only behavior. Change buildRelativeUrl() and
getTargetDirectory() in EditorUploadService to private, and update any direct
test usage to exercise them through the service’s public methods or adjust test
setup as needed so the internal helpers are no longer part of the public API.
In `@tests/Integration/Controller/EditorUploadControllerTest.php`:
- Around line 70-73: The test cleanup code is reusing $uploadedFile for a string
path after it already referred to an UploadedFile object, which is confusing and
can lead to accidental misuse. In EditorUploadControllerTest, rename the path
variable to something distinct like an uploaded file path name and keep
$uploadedFile reserved for the UploadedFile instance, updating the
is_file/unlink cleanup block accordingly.
In `@tests/Unit/assets/editor/CkEditor.spec.js`:
- Around line 95-140: The CkEditor tests cover props and v-model, but they do
not exercise the handleReady path exposed by the stub’s ready control. Add a
test that clicks the .ready button on the mounted CkEditor wrapper and verify
the handleReady side effects: editorRef is set, readonly state is synced, and
installUploadAdapter runs by spying on
editor.plugins.get('FileRepository').createUploadAdapter or the equivalent
upload adapter hook.
In `@tests/Unit/assets/editor/uploadAdapter.spec.js`:
- Around line 6-78: The EditorUploadAdapter spec covers upload success and
backend error response, but it is missing failure-path coverage for abort and
network errors. Extend the existing EditorUploadAdapter tests to exercise
adapter.abort() and verify that xhr.abort() is called, and add a case that
triggers the XHR error event to confirm the adapter rejects correctly on
transport-level failure; use the existing sendMock/listeners setup and the
adapter.xhr instance so the new assertions stay aligned with the current test
pattern.
In `@tests/Unit/Service/EditorUploadServiceTest.php`:
- Around line 79-100: The removePath() helper is duplicated in
EditorUploadServiceTest and EditorUploadControllerTest, so extract it into a
shared test trait or base class and reuse that single implementation from both
tests. Move the recursive filesystem cleanup logic into the shared helper, then
update each test class to import or extend it so the duplicated private method
can be removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 98d609e8-1d6e-46fd-ade2-82794eb5b2f3
📒 Files selected for processing (21)
README.mdassets/editor/CkEditor.vueassets/editor/EditorAssetPicker.vueassets/editor/assetBrowserPlugin.jsassets/editor/index.tsassets/editor/plugins.tsassets/editor/toolbar.tsassets/editor/uploadAdapter.tsassets/vue/components/base/CkEditorField.vuecomposer.jsonconfig/services.ymlsrc/Controller/EditorUploadController.phpsrc/Dto/EditorAssetItem.phpsrc/Dto/EditorUploadResult.phpsrc/Service/EditorUploadService.phptests/Integration/Controller/EditorUploadControllerTest.phptests/Unit/Service/EditorUploadServiceTest.phptests/Unit/assets/editor/CkEditor.spec.jstests/Unit/assets/editor/uploadAdapter.spec.jstests/Unit/assets/vue/components/base/CkEditorField.spec.jsvitest.config.mjs
Comment on lines
+1
to
+9
| <template> | ||
| <Teleport to="body"> | ||
| <div | ||
| v-if="open" | ||
| class="fixed inset-0 z-50 flex items-center justify-center bg-slate-950/50 p-4" | ||
| role="dialog" | ||
| aria-modal="true" | ||
| aria-labelledby="editor-asset-picker-title" | ||
| > |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add Escape-to-close and basic focus management for the modal.
The dialog has correct ARIA attributes but is missing keyboard dismissal (@keydown.escape) and focus management. Keyboard-only users cannot close the modal without tabbing to the Close button. At minimum, add an Escape handler on the overlay div.
♿ Proposed fix for Escape key and focus management
<div
v-if="open"
class="fixed inset-0 z-50 flex items-center justify-center bg-slate-950/50 p-4"
role="dialog"
aria-modal="true"
aria-labelledby="editor-asset-picker-title"
+ `@keydown.escape`="$emit('close')"
+ tabindex="-1"
+ ref="dialogRef"
>And in <script setup>, add focus handling on open:
<script setup>
-import { computed } from 'vue'
+import { computed, ref, watch, nextTick } from 'vue'
+const dialogRef = ref(null)
+
+watch(() => props.open, async (isOpen) => {
+ if (isOpen) {
+ await nextTick()
+ dialogRef.value?.focus()
+ }
+})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <template> | |
| <Teleport to="body"> | |
| <div | |
| v-if="open" | |
| class="fixed inset-0 z-50 flex items-center justify-center bg-slate-950/50 p-4" | |
| role="dialog" | |
| aria-modal="true" | |
| aria-labelledby="editor-asset-picker-title" | |
| > | |
| <template> | |
| <Teleport to="body"> | |
| <div | |
| v-if="open" | |
| class="fixed inset-0 z-50 flex items-center justify-center bg-slate-950/50 p-4" | |
| role="dialog" | |
| aria-modal="true" | |
| aria-labelledby="editor-asset-picker-title" | |
| `@keydown.escape`="$emit('close')" | |
| tabindex="-1" | |
| ref="dialogRef" | |
| > |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@assets/editor/EditorAssetPicker.vue` around lines 1 - 9, The modal in
EditorAssetPicker.vue already has the dialog semantics but lacks keyboard
dismissal and focus handling. Add an Escape-key handler on the overlay div that
closes the picker when open, and in the <script setup> for EditorAssetPicker,
wire basic focus management on open so keyboard users land inside the dialog
instead of needing to tab to the Close button.
Comment on lines
+55
to
+110
| <table v-else class="w-full table-fixed border-separate border-spacing-0"> | ||
| <thead class="sticky top-0 bg-slate-50"> | ||
| <tr class="text-left text-xs font-semibold uppercase tracking-wide text-slate-500"> | ||
| <th class="w-24 border-b border-slate-200 px-4 py-3">Preview</th> | ||
| <th class="border-b border-slate-200 px-4 py-3">Name</th> | ||
| <th class="w-32 border-b border-slate-200 px-4 py-3">Type</th> | ||
| <th class="w-28 border-b border-slate-200 px-4 py-3">Size</th> | ||
| <th class="w-40 border-b border-slate-200 px-4 py-3">Updated</th> | ||
| <th class="w-28 border-b border-slate-200 px-4 py-3"></th> | ||
| </tr> | ||
| </thead> | ||
| <tbody> | ||
| <tr | ||
| v-for="item in filteredItems" | ||
| :key="item.url" | ||
| class="border-b border-slate-100 text-sm text-slate-700 hover:bg-slate-50" | ||
| > | ||
| <td class="px-4 py-3 align-top"> | ||
| <img | ||
| v-if="item.isImage" | ||
| :src="item.url" | ||
| :alt="item.fileName" | ||
| class="h-14 w-14 rounded border border-slate-200 object-cover" | ||
| > | ||
| <div | ||
| v-else | ||
| class="flex h-14 w-14 items-center justify-center rounded border border-slate-200 bg-slate-50 text-xs font-semibold uppercase text-slate-500" | ||
| > | ||
| {{ extensionLabel(item.fileName) }} | ||
| </div> | ||
| </td> | ||
| <td class="px-4 py-3 align-top"> | ||
| <p class="break-all font-medium text-slate-900">{{ item.fileName }}</p> | ||
| <p class="break-all text-xs text-slate-500">{{ item.url }}</p> | ||
| </td> | ||
| <td class="px-4 py-3 align-top text-xs uppercase text-slate-500"> | ||
| {{ item.mimeType }} | ||
| </td> | ||
| <td class="px-4 py-3 align-top text-sm text-slate-600"> | ||
| {{ formatBytes(item.size) }} | ||
| </td> | ||
| <td class="px-4 py-3 align-top text-sm text-slate-600"> | ||
| {{ formatDate(item.modifiedAt) }} | ||
| </td> | ||
| <td class="px-4 py-3 align-top text-right"> | ||
| <button | ||
| type="button" | ||
| class="rounded-lg bg-ext-wf1 px-3 py-2 text-sm font-medium text-white hover:bg-ext-wf3" | ||
| @click="$emit('select', item)" | ||
| > | ||
| Insert | ||
| </button> | ||
| </td> | ||
| </tr> | ||
| </tbody> | ||
| </table> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add an empty-state message when the filtered list is empty.
When the search yields no results, the table renders only headers with no body rows, which looks broken. A simple "No assets found" row improves clarity.
✨ Proposed fix for empty state
<table v-else class="w-full table-fixed border-separate border-spacing-0">
<thead class="sticky top-0 bg-slate-50">
<tr class="text-left text-xs font-semibold uppercase tracking-wide text-slate-500">
<th class="w-24 border-b border-slate-200 px-4 py-3">Preview</th>
<th class="border-b border-slate-200 px-4 py-3">Name</th>
<th class="w-32 border-b border-slate-200 px-4 py-3">Type</th>
<th class="w-28 border-b border-slate-200 px-4 py-3">Size</th>
<th class="w-40 border-b border-slate-200 px-4 py-3">Updated</th>
<th class="w-28 border-b border-slate-200 px-4 py-3"></th>
</tr>
</thead>
<tbody>
+ <tr v-if="filteredItems.length === 0">
+ <td colspan="6" class="px-4 py-8 text-center text-sm text-slate-500">
+ No assets found.
+ </td>
+ </tr>
<tr
v-for="item in filteredItems"
:key="item.url"
class="border-b border-slate-100 text-sm text-slate-700 hover:bg-slate-50"
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <table v-else class="w-full table-fixed border-separate border-spacing-0"> | |
| <thead class="sticky top-0 bg-slate-50"> | |
| <tr class="text-left text-xs font-semibold uppercase tracking-wide text-slate-500"> | |
| <th class="w-24 border-b border-slate-200 px-4 py-3">Preview</th> | |
| <th class="border-b border-slate-200 px-4 py-3">Name</th> | |
| <th class="w-32 border-b border-slate-200 px-4 py-3">Type</th> | |
| <th class="w-28 border-b border-slate-200 px-4 py-3">Size</th> | |
| <th class="w-40 border-b border-slate-200 px-4 py-3">Updated</th> | |
| <th class="w-28 border-b border-slate-200 px-4 py-3"></th> | |
| </tr> | |
| </thead> | |
| <tbody> | |
| <tr | |
| v-for="item in filteredItems" | |
| :key="item.url" | |
| class="border-b border-slate-100 text-sm text-slate-700 hover:bg-slate-50" | |
| > | |
| <td class="px-4 py-3 align-top"> | |
| <img | |
| v-if="item.isImage" | |
| :src="item.url" | |
| :alt="item.fileName" | |
| class="h-14 w-14 rounded border border-slate-200 object-cover" | |
| > | |
| <div | |
| v-else | |
| class="flex h-14 w-14 items-center justify-center rounded border border-slate-200 bg-slate-50 text-xs font-semibold uppercase text-slate-500" | |
| > | |
| {{ extensionLabel(item.fileName) }} | |
| </div> | |
| </td> | |
| <td class="px-4 py-3 align-top"> | |
| <p class="break-all font-medium text-slate-900">{{ item.fileName }}</p> | |
| <p class="break-all text-xs text-slate-500">{{ item.url }}</p> | |
| </td> | |
| <td class="px-4 py-3 align-top text-xs uppercase text-slate-500"> | |
| {{ item.mimeType }} | |
| </td> | |
| <td class="px-4 py-3 align-top text-sm text-slate-600"> | |
| {{ formatBytes(item.size) }} | |
| </td> | |
| <td class="px-4 py-3 align-top text-sm text-slate-600"> | |
| {{ formatDate(item.modifiedAt) }} | |
| </td> | |
| <td class="px-4 py-3 align-top text-right"> | |
| <button | |
| type="button" | |
| class="rounded-lg bg-ext-wf1 px-3 py-2 text-sm font-medium text-white hover:bg-ext-wf3" | |
| @click="$emit('select', item)" | |
| > | |
| Insert | |
| </button> | |
| </td> | |
| </tr> | |
| </tbody> | |
| </table> | |
| <table v-else class="w-full table-fixed border-separate border-spacing-0"> | |
| <thead class="sticky top-0 bg-slate-50"> | |
| <tr class="text-left text-xs font-semibold uppercase tracking-wide text-slate-500"> | |
| <th class="w-24 border-b border-slate-200 px-4 py-3">Preview</th> | |
| <th class="border-b border-slate-200 px-4 py-3">Name</th> | |
| <th class="w-32 border-b border-slate-200 px-4 py-3">Type</th> | |
| <th class="w-28 border-b border-slate-200 px-4 py-3">Size</th> | |
| <th class="w-40 border-b border-slate-200 px-4 py-3">Updated</th> | |
| <th class="w-28 border-b border-slate-200 px-4 py-3"></th> | |
| </tr> | |
| </thead> | |
| <tbody> | |
| <tr v-if="filteredItems.length === 0"> | |
| <td colspan="6" class="px-4 py-8 text-center text-sm text-slate-500"> | |
| No assets found. | |
| </td> | |
| </tr> | |
| <tr | |
| v-for="item in filteredItems" | |
| :key="item.url" | |
| class="border-b border-slate-100 text-sm text-slate-700 hover:bg-slate-50" | |
| > | |
| <td class="px-4 py-3 align-top"> | |
| <img | |
| v-if="item.isImage" | |
| :src="item.url" | |
| :alt="item.fileName" | |
| class="h-14 w-14 rounded border border-slate-200 object-cover" | |
| > | |
| <div | |
| v-else | |
| class="flex h-14 w-14 items-center justify-center rounded border border-slate-200 bg-slate-50 text-xs font-semibold uppercase text-slate-500" | |
| > | |
| {{ extensionLabel(item.fileName) }} | |
| </div> | |
| </td> | |
| <td class="px-4 py-3 align-top"> | |
| <p class="break-all font-medium text-slate-900">{{ item.fileName }}</p> | |
| <p class="break-all text-xs text-slate-500">{{ item.url }}</p> | |
| </td> | |
| <td class="px-4 py-3 align-top text-xs uppercase text-slate-500"> | |
| {{ item.mimeType }} | |
| </td> | |
| <td class="px-4 py-3 align-top text-sm text-slate-600"> | |
| {{ formatBytes(item.size) }} | |
| </td> | |
| <td class="px-4 py-3 align-top text-sm text-slate-600"> | |
| {{ formatDate(item.modifiedAt) }} | |
| </td> | |
| <td class="px-4 py-3 align-top text-right"> | |
| <button | |
| type="button" | |
| class="rounded-lg bg-ext-wf1 px-3 py-2 text-sm font-medium text-white hover:bg-ext-wf3" | |
| `@click`="$emit('select', item)" | |
| > | |
| Insert | |
| </button> | |
| </td> | |
| </tr> | |
| </tbody> | |
| </table> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@assets/editor/EditorAssetPicker.vue` around lines 55 - 110, The filtered
asset table in EditorAssetPicker.vue has no empty-state, so when filteredItems
is empty it shows only the header row and looks broken. Update the table
rendering around the v-for on filteredItems to conditionally show a single
full-width tbody row with a “No assets found” message when there are no results,
while keeping the existing item rows and select button behavior intact.
Comment on lines
+26
to
+47
| public function storeImage(UploadedFile $uploadedFile): EditorUploadResult | ||
| { | ||
| if (!$uploadedFile->isValid()) { | ||
| throw new RuntimeException($uploadedFile->getErrorMessage()); | ||
| } | ||
|
|
||
| $mimeType = (string) $uploadedFile->getMimeType(); | ||
| if (!str_starts_with($mimeType, 'image/')) { | ||
| throw new RuntimeException('Only image uploads are supported.'); | ||
| } | ||
|
|
||
| $targetDirectory = $this->getTargetDirectory(); | ||
| $this->filesystem->mkdir($targetDirectory, 0755); | ||
|
|
||
| $fileName = $this->buildFileName($uploadedFile); | ||
| $uploadedFile->move($targetDirectory, $fileName); | ||
|
|
||
| return new EditorUploadResult( | ||
| fileName: $fileName, | ||
| relativeUrl: $this->buildRelativeUrl($fileName), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Enforce a maximum file size for uploads.
storeImage has no application-level file size check. While PHP's upload_max_filesize provides a server-level backstop, an explicit check prevents large files from consuming disk and memory before the upload completes.
📏 Proposed fix: add file size validation
public function storeImage(UploadedFile $uploadedFile): EditorUploadResult
{
if (!$uploadedFile->isValid()) {
throw new RuntimeException($uploadedFile->getErrorMessage());
}
+ $maxSize = 10 * 1024 * 1024; // 10 MB
+ if ($uploadedFile->getSize() > $maxSize) {
+ throw new RuntimeException('File is too large. Maximum size is 10 MB.');
+ }
+
$mimeType = (string) mime_content_type($uploadedFile->getPathname());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Service/EditorUploadService.php` around lines 26 - 47, The storeImage
method in EditorUploadService currently validates only upload status and MIME
type, but it never enforces an application-level size limit. Add a file-size
check near the start of storeImage, before any filesystem work or move call, and
reject oversized uploads by throwing a RuntimeException with a clear message.
Use the existing UploadedFile object and keep the new limit consistent with the
upload policy used by this service.
Comment on lines
+128
to
+139
| private function guessExtensionFromMime(string $mimeType): string | ||
| { | ||
| return match ($mimeType) { | ||
| 'image/jpeg' => 'jpg', | ||
| 'image/png' => 'png', | ||
| 'image/gif' => 'gif', | ||
| 'image/webp' => 'webp', | ||
| 'image/bmp', 'image/x-ms-bmp' => 'bmp', | ||
| 'image/svg+xml' => 'svg', | ||
| default => '', | ||
| }; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
SVG uploads allow stored XSS.
guessExtensionFromMime includes 'image/svg+xml' => 'svg', and image/svg+xml passes the str_starts_with($mimeType, 'image/') check. SVG files can contain embedded <script> tags and event handlers. When served inline from the public directory, the browser will execute any embedded JavaScript — a stored XSS vector.
Either block SVG uploads explicitly or sanitize SVG content before storing.
🛡️ Proposed fix: block SVG uploads
private function guessExtensionFromMime(string $mimeType): string
{
return match ($mimeType) {
'image/jpeg' => 'jpg',
'image/png' => 'png',
'image/gif' => 'gif',
'image/webp' => 'webp',
'image/bmp', 'image/x-ms-bmp' => 'bmp',
- 'image/svg+xml' => 'svg',
default => '',
};
}Additionally, add an explicit check in storeImage:
$mimeType = (string) mime_content_type($uploadedFile->getPathname());
- if (!str_starts_with($mimeType, 'image/')) {
+ if (!str_starts_with($mimeType, 'image/') || $mimeType === 'image/svg+xml') {
throw new RuntimeException('Only image uploads are supported.');
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Service/EditorUploadService.php` around lines 128 - 139, Block SVG
handling in EditorUploadService to prevent stored XSS: guessExtensionFromMime
currently maps image/svg+xml to svg, and storeImage accepts any image/* MIME, so
SVGs can be uploaded and served inline. Update storeImage to explicitly reject
image/svg+xml before extension guessing, and remove or bypass the svg case in
guessExtensionFromMime so only safe raster formats are stored.
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
2 participants
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.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Summary
Provide a general description of the code changes in your pull request …
were there any bugs you had fixed? If so, mention them. If these bugs have open
GitHub issues, be sure to tag them here as well, to keep the conversation
linked together.
Unit test
Are your changes covered with unit tests, and do they not break anything?
You can run the existing unit tests using this command:
Code style
Have you checked that you code is well-documented and follows the PSR-2 coding
style?
You can check for this using this command:
Other Information
If there's anything else that's important and relevant to your pull
request, mention that information here. This could include benchmarks,
or other information.
If you are updating any of the CHANGELOG files or are asked to update the
CHANGELOG files by reviewers, please add the CHANGELOG entry at the top of the file.
Thanks for contributing to phpList!