Image pipeline #109 — ImageEngine foundation (libvips + capability detection)#117
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughViene introdotta la classe ModifichePipeline di imaging con libvips, HEIC/HEIF e JPEG-XL
Diagrammi di sequenzasequenceDiagram
participant UploadService
participant ImageEngine
participant Vips as jcupitt/vips
participant Imagick
UploadService->>ImageEngine: encode(src, dest, targetW, fmt, quality)
ImageEngine->>ImageEngine: capabilities() — verifica vips
alt vips disponibile
ImageEngine->>Vips: thumbnail(src, targetW)
ImageEngine->>Vips: writeToFile(dest, opts)
Vips-->>ImageEngine: file scritto
ImageEngine->>ImageEngine: optimize(dest, format)
ImageEngine-->>UploadService: true
else vips non disponibile
alt format === 'jxl'
ImageEngine->>Imagick: Genera PNG temporaneo
ImageEngine->>ImageEngine: run('cjxl', argv)
ImageEngine-->>UploadService: true/false
else fallback non disponibile
ImageEngine-->>UploadService: false
end
UploadService->>Imagick: resize e salva (fallback)
Imagick-->>UploadService: variante scritta
end
Stima dello sforzo di revisione🎯 4 (Complesso) | ⏱️ ~75 minuti Issue potenzialmente collegate
PR potenzialmente correlate
Poesia
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.2.2)PHPStan was skipped because the config uses disallowed 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 |
#109 completata su questo branchSecondo incremento (commit
Verifica end-to-end (host senza vips): Attivazione produttiva: @coderabbitai review |
|
🐇 ✨ ✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
…y detection (#109) First increment of the modern image pipeline (#109): - New App\Services\Imaging\ImageEngine: capability-detected encoder. - libvips fast path (jcupitt/vips): shrink-on-load, low memory; emits jpeg/webp/avif/jxl when the build supports them. - capabilities(): detects vips, HEIC/HEIF read, AVIF/JPEG-XL write, and optimizer binaries (jpegoptim/pngquant/cwebp/avifenc). - optional post-encode optimization via proc_open argv-array (no shell). - images:generate tries ImageEngine::encode() first, falling back to the existing Imagick/GD path when vips is unavailable: zero behaviour change on hosts without vips (verified: encode() returns false then fallback). - Hardened orphan-variant deletion with a realpath confinement under public/media (path-traversal guard). Activation needs composer require jcupitt/vips + php-vips/libvips built with heif+aom (AVIF) and libjxl (JPEG-XL). Without them, behaviour is unchanged. HEIC import and next-gen output formats follow on this branch. Refs #109
…, diagnostics (#109) Completes the modern image pipeline on top of the ImageEngine foundation. Ingestion / UploadService: - Accept HEIC/HEIF uploads, gated on ImageEngine capabilities (heif_read) so we never store an original we can't turn into web variants. ISO-BMFF 'ftyp' magic check; HEIC-aware dimension reading via Imagick pingImage (headers only — decompression-bomb safe) since getimagesize() can't read HEIC. Originals keep .heic/.heif; variants stay jpg/webp/avif. - Wire ImageEngine fast path into resizeWithImagick / resizeWithImagickOrGd (libvips first, Imagick/GD fallback unchanged). Variant generation: - images:generate emits JPEG-XL when enabled in settings AND the build can write it (capability-gated; libvips engine). Order unchanged for the existing avif/webp/jpg. Settings: - image.formats/quality gain a jxl entry (default OFF); admin settings form + controller parse fmt_jxl/q_jxl. Diagnostics: - New "Imaging Engine" panel: active engine, libvips/Imagick/GD, HEIC read, AVIF/JPEG-XL write, detected optimizer binaries. Security (per project policy — fix findings even when pre-existing): - Confined orphan-variant + favicon deletions with realpath guards. - Hardened the images:generate exec (realpath-validated console path, escapeshellarg, app-private reused log under storage/logs). composer.json: suggest jcupitt/vips, ext-vips, ext-imagick, spatie/image-optimizer (all optional; behaviour unchanged without them). Verified end-to-end on a vips-less host: images:generate produced 15/15 variants (avif/webp/jpg) via the Imagick fallback, 0 errors. Refs #109
fabiodalez-dev
force-pushed
the
feat/image-pipeline
branch
from
fb9ab10 to
02db946
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/Services/UploadService.php (1)
436-444: 💤 Low valueDoppio tentativo ridondante di
ImageEngine::encode().Quando
resizeWithImagickOrGd()fallisce il fast path (linea 440) e Imagick è disponibile, chiamaresizeWithImagick()che tenta di nuovoImageEngine::encode()(linea 402). Questo secondo tentativo è garantito fallire perché le condizioni sono identiche.Considerare di rimuovere il fast path da
resizeWithImagick()poiché è già gestito dal chiamante, oppure aggiungere un parametro per saltare il tentativo vips.♻️ Proposta per evitare il doppio tentativo
Opzione 1: Rimuovere il fast path da
resizeWithImagick():private function resizeWithImagick(string $src, string $dest, int $targetW, string $format, int $quality): bool { - // Fast path (`#109`): libvips when available; falls through to Imagick - // (below) when vips is absent or can't handle this format. - if (\App\Services\Imaging\ImageEngine::encode($src, $dest, $targetW, $format, $quality)) { - return true; - } try { self::applyImagickLimits();Opzione 2: Aggiungere un parametro opzionale:
-private function resizeWithImagick(string $src, string $dest, int $targetW, string $format, int $quality): bool +private function resizeWithImagick(string $src, string $dest, int $targetW, string $format, int $quality, bool $tryVips = true): bool { - if (\App\Services\Imaging\ImageEngine::encode($src, $dest, $targetW, $format, $quality)) { + if ($tryVips && \App\Services\Imaging\ImageEngine::encode($src, $dest, $targetW, $format, $quality)) { return true; }🤖 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 `@app/Services/UploadService.php` around lines 436 - 444, There is a redundant call to ImageEngine::encode() that occurs twice with identical parameters. The first attempt happens in resizeWithImagickOrGd() method, and when it fails, the method calls resizeWithImagick() which attempts ImageEngine::encode() again at line 402, guaranteed to fail with the same parameters. Remove the ImageEngine::encode() fast path attempt from inside the resizeWithImagick() method since this fast path is already handled by the caller resizeWithImagickOrGd() before delegating to resizeWithImagick(). This eliminates the redundant second attempt that will always fail.
🤖 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 `@app/Views/admin/settings.twig`:
- Around line 46-51: The JPEG-XL format section in the settings template
contains hardcoded English strings that should use the trans() function for
internationalization consistency with other format options. Replace the
hardcoded description "next-gen format; generated only when the server's libvips
is built with libjxl. Browser support is still limited, so it is opt-in." with
{{ trans('admin.settings.jxl_desc') }} and replace the hardcoded label "JPEG-XL
quality" with {{ trans('admin.settings.jxl_quality') }} to maintain proper i18n
practices throughout the template.
---
Nitpick comments:
In `@app/Services/UploadService.php`:
- Around line 436-444: There is a redundant call to ImageEngine::encode() that
occurs twice with identical parameters. The first attempt happens in
resizeWithImagickOrGd() method, and when it fails, the method calls
resizeWithImagick() which attempts ImageEngine::encode() again at line 402,
guaranteed to fail with the same parameters. Remove the ImageEngine::encode()
fast path attempt from inside the resizeWithImagick() method since this fast
path is already handled by the caller resizeWithImagickOrGd() before delegating
to resizeWithImagick(). This eliminates the redundant second attempt that will
always fail.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 40cc6bb9-b6f1-4824-b68f-343125d20f29
📒 Files selected for processing (8)
app/Controllers/Admin/DiagnosticsController.phpapp/Controllers/Admin/SettingsController.phpapp/Services/Imaging/ImageEngine.phpapp/Services/SettingsService.phpapp/Services/UploadService.phpapp/Tasks/ImagesGenerateCommand.phpapp/Views/admin/settings.twigcomposer.json
Code reviewBranch: Found 26 findings across all lanes:
Deep lane — correctness & security✓ Auto-fixable (13)
Cross-cutting group G1: F006 + F007 + F009 — Fix all three HEIC-capability gaps against a single decode/read surface rather than piecemeal. Land the F008 centralization first by adding ImageEngine::dimensions() (vips Image::newFromFile sequential for HEIF, then Imagick ping, then null) and ImageEngine::previewJpeg() (HEIC-capable sm encode), mirroring the vips-OR-Imagick union already asserted by capabilities()['heif_read']. Then: (F007) route readImageDimensions' HEIF branch through ImageEngine::dimensions so a vips-only host returns real dims instead of null; (F006) replace the bare getimagesize() post-orientation re-read with readImageDimensions($dest,$mime), updating dims only when non-null; (F009) when generateJpegPreview returns null for an accepted HEIC, fall back to ImageEngine::previewJpeg to produce the sm jpg synchronously and insert the sm variant row. Doing them together guarantees the gate and the readers/encoders agree on one HEIC-decode notion across the dimension-read, orientation re-read, and preview paths. Details and fix proposalsF003 — When vips is available, resizeWithImagickOrGd() calls ImageEngine::encode() and on failure delegates to resizeWithImagick(), which unconditionally calls ImageEngine::encode() a second time; the redundant call always returns false (same deterministic conditions) but adds latency.File: Evidence:
Approach: Extract the Imagick-only body into a private resizeWithImagickOnly() and have resizeWithImagickOrGd's fallback call it instead of resizeWithImagick(), so the second encode() is removed while direct webp/avif callers keep their single vips fast path. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_20260617T135442Z732e76): fixed and verified F006 — After EXIF-based orientation normalization, getimagesize() is called on $dest to re-read dimensions, but if the original was a HEIC file the call returns false and the stored width/height remain pre-normalization.File: Evidence:
Approach: Replace the direct getimagesize() re-read with readImageDimensions($dest,$mime), updating $width/$height only when non-null. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_20260617T135442Z732e76): fixed and verified F007 — readImageDimensions() reads HEIC dimensions only via Imagick (pingImage); no libvips path, so on a vips-only host (libvips+libheif, no Imagick HEIC) heif_read is true and upload admitted, yet dimension read returns null → validateImageFile() throws 'cannot read dimensions' and the HEIC upload is rejected.File: Evidence:
Approach: Add a libvips dimension-read branch to readImageDimensions for HEIF (Image::newFromFile sequential header read), preferring vips then Imagick then null, so the reader's engine set matches the gate. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_20260617T135442Z732e76): fixed and verified F008 — HEIC acceptance is gated on capabilities()['heif_read'] (true when EITHER vips-with-libheif OR Imagick-HEIC), but the dimension reader and the immediate-preview path require Imagick/GD respectively — the writer-gate and consumers disagree on what 'can decode HEIC' means.File: Evidence:
Approach: Centralize HEIC capability: add ImageEngine::dimensions() and ImageEngine::previewJpeg() (vips-backed, header/thumbnail-on-load) and route readImageDimensions and generateJpegPreview through them, keeping heif_read as the single gate. Subsumes F007 and F009. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_20260617T135442Z732e76): fixed and verified F009 — ingestAlbumUpload()'s immediate 'sm' preview uses ImagesService::generateJpegPreview() (GD-only, only image/jpeg & image/png); newly-admitted HEIC originals produce no immediate sm preview row, so a just-uploaded HEIC has no thumbnail until async images:generate runs.File: Evidence:
Approach: When generateJpegPreview returns null for an accepted HEIC, fall back to the HEIC-capable engine to produce the sm jpg synchronously (resizeWithImagickOrGd → ImageEngine::encode), then insert the sm variant row. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_20260617T135442Z732e76): fixed and verified F012 — The Imagick variant path honours STRIP_EXIF (default strip; can be disabled to retain metadata), but the libvips fast path uses ImageEngine::encode()'s default $strip=true and callers never thread STRIP_EXIF into it — on vips hosts variants are always stripped regardless of STRIP_EXIF=false: a silent behaviour divergence between engines.File: Evidence:
Approach: Thread the STRIP_EXIF flag into encode()'s $strip at every fast-path call site. UploadService passes $this->envFlag('STRIP_EXIF',true) as the 6th arg at 402 and 440; ImagesGenerateCommand resolves the flag locally (getenv default true) and passes it at 183. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_20260617T135442Z732e76): fixed and verified F013 — JPEG-XL variants are fully generated (encoded, written to public/media, inserted into image_variants) when settings enable jxl and the build can write it, but no frontend template emits a type="image/jxl"
|
| Finding | Group | Outcome | phase_9_finding |
|---|---|---|---|
| F003 | FG-1 | ✓ fixed and verified | |
| F006 | FG-1 | ✓ fixed and verified | |
| F007 | FG-1 | ✓ fixed and verified | |
| F008 | FG-1 | ✓ fixed and verified | |
| F009 | FG-1 | ✓ fixed and verified | |
| F012 | FG-1 | ✓ fixed and verified | |
| F020 | FG-2 | ✓ fixed and verified | |
| F022 | FG-2 | ✓ fixed and verified | |
| F025 | FG-3 | ✓ fixed and verified | |
| F030 | FG-1 | ✓ fixed and verified |
Run fixrun_20260617T142726Z4d8b5e — 2026-06-17T15:06:18Z
- Outcomes: 4 fixed and verified
- Commits:
c9280cb
| Finding | Group | Outcome | phase_9_finding |
|---|---|---|---|
| F013 | FG-1 | ✓ fixed and verified | |
| F014 | FG-2 | ✓ fixed and verified | |
| F015 | FG-2 | ✓ fixed and verified | |
| F028 | FG-2 | ✓ fixed and verified |
Run fixrun_20260617T224418Z0d11a1 — 2026-06-17T22:47:11Z
- Outcomes: 4 fixed and verified
- Commits:
2e0efd4
| Finding | Group | Outcome | phase_9_finding |
|---|---|---|---|
| F016 | FG-1 | ✓ fixed and verified | |
| F017 | FG-2 | ✓ fixed and verified | |
| F018 | FG-2 | ✓ fixed and verified | |
| F029 | FG-2 | ✓ fixed and verified |
Applies 10 confirmed findings from the code review of #117. HEIC handling (cross-cutting group, app/Services): - Centralize HEIC dimension reads via new ImageEngine::dimensions() (libvips header/sequential, bomb-safe); readImageDimensions() now tries vips before Imagick so vips-only hosts can measure admitted HEIC instead of rejecting it (F007/F008). - Post-orientation re-read uses the HEIC-aware readImageDimensions() instead of bare getimagesize(), fixing swapped/stale dimensions for rotated HEIC originals (F006). - HEIC uploads get an immediate sm preview via the HEIC-capable variant path when the GD-only generateJpegPreview() returns null (F009). Encoder correctness (app/Services/UploadService.php, app/Tasks): - Remove the redundant second ImageEngine::encode() on the Imagick fallback by extracting resizeWithImagickOnly() (F003). - Thread STRIP_EXIF into the libvips encode() fast path so it honours STRIP_EXIF=false like the Imagick path (F012). JPEG-XL data layer (database, app/Tasks): - Allow 'jxl' in image_variants.format on both engines: widen the SQLite CHECK and MySQL ENUM, add migrate_1.4.13_{sqlite,mysql}.sql (SQLite via guarded table rebuild), regenerate template.sqlite, bump version to 1.4.13. Wrap the variant INSERT in try/catch so one bad row no longer aborts the whole images:generate run (F030). Admin UI (app/Views, storage/translations, app/Controllers): - i18n the JPEG-XL settings strings via trans() keys (F020). - Fix the quality-inputs grid to grid-cols-4 so the 4th input aligns (F022). - Diagnostics no longer reports "No capable image engine" when GD is present (F025).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@database/migrations/migrate_1.4.13_sqlite.sql`:
- Around line 9-30: The migration sequence for rebuilding the image_variants
table (CREATE TABLE image_variants_new, INSERT INTO image_variants_new, DROP
TABLE image_variants, ALTER TABLE image_variants_new RENAME TO image_variants)
lacks explicit transaction protection, leaving the database in an inconsistent
state if any step fails. Wrap the entire migration sequence between the PRAGMA
foreign_keys=OFF and PRAGMA foreign_keys=ON statements with BEGIN TRANSACTION
and COMMIT statements to ensure atomicity and prevent partial state corruption.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 07fd38e6-229b-4528-b650-8c023517ef03
📒 Files selected for processing (13)
app/Controllers/Admin/DiagnosticsController.phpapp/Services/Imaging/ImageEngine.phpapp/Services/UploadService.phpapp/Tasks/ImagesGenerateCommand.phpapp/Views/admin/settings.twigdatabase/migrations/migrate_1.4.13_mysql.sqldatabase/migrations/migrate_1.4.13_sqlite.sqldatabase/schema.mysql.sqldatabase/schema.sqlite.sqldatabase/template.sqlitestorage/translations/en_admin.jsonstorage/translations/it_admin.jsonversion.json
✅ Files skipped from review due to trivial changes (4)
- version.json
- database/migrations/migrate_1.4.13_mysql.sql
- storage/translations/it_admin.json
- storage/translations/en_admin.json
🚧 Files skipped from review as they are similar to previous changes (4)
- app/Views/admin/settings.twig
- app/Controllers/Admin/DiagnosticsController.php
- app/Tasks/ImagesGenerateCommand.php
- app/Services/UploadService.php
| PRAGMA foreign_keys=OFF; | ||
|
|
||
| CREATE TABLE image_variants_new ( | ||
| id INTEGER PRIMARY KEY AUTOINCREMENT, | ||
| image_id INTEGER NOT NULL, | ||
| variant TEXT NOT NULL, | ||
| format TEXT NOT NULL CHECK(format IN ('avif', 'webp', 'jpg', 'jxl')), | ||
| path TEXT NOT NULL, | ||
| width INTEGER NOT NULL, | ||
| height INTEGER NOT NULL, | ||
| size_bytes INTEGER NOT NULL, | ||
| UNIQUE(image_id, variant, format), | ||
| FOREIGN KEY (image_id) REFERENCES images(id) ON DELETE CASCADE | ||
| ); | ||
|
|
||
| INSERT INTO image_variants_new (id, image_id, variant, format, path, width, height, size_bytes) SELECT id, image_id, variant, format, path, width, height, size_bytes FROM image_variants; | ||
|
|
||
| DROP TABLE image_variants; | ||
|
|
||
| ALTER TABLE image_variants_new RENAME TO image_variants; | ||
|
|
||
| PRAGMA foreign_keys=ON; |
There was a problem hiding this comment.
Rendi atomica la migrazione di rebuild della tabella.
Tra Line 9 e Line 30, la sequenza CREATE → INSERT → DROP → RENAME non è protetta da una transazione esplicita. Se fallisce a metà, puoi lasciare il DB in stato intermedio (tabella rinominata a metà o assente).
💡 Patch proposta
PRAGMA foreign_keys=OFF;
+BEGIN IMMEDIATE;
CREATE TABLE image_variants_new (
@@
ALTER TABLE image_variants_new RENAME TO image_variants;
+COMMIT;
PRAGMA foreign_keys=ON;🤖 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 `@database/migrations/migrate_1.4.13_sqlite.sql` around lines 9 - 30, The
migration sequence for rebuilding the image_variants table (CREATE TABLE
image_variants_new, INSERT INTO image_variants_new, DROP TABLE image_variants,
ALTER TABLE image_variants_new RENAME TO image_variants) lacks explicit
transaction protection, leaving the database in an inconsistent state if any
step fails. Wrap the entire migration sequence between the PRAGMA
foreign_keys=OFF and PRAGMA foreign_keys=ON statements with BEGIN TRANSACTION
and COMMIT statements to ensure atomicity and prevent partial state corruption.
Walkthrough decisions
Walking the Full skip set scope: of 4 non-auto-eligible finding(s), 4 promoted, 0 skipped, 0 stopped. Promoted findings will be picked up by the next Promoted
Decisions log: this comment is append-only audit — never edited in place. Each |
Applies 4 findings promoted via /adamsreview:walkthrough. - F013: serve JPEG-XL to capable browsers. buildResponsiveSources() gains a 'jxl' bucket (avif > jxl > webp > jpg), and every <picture> template emits a <source type="image/jxl"> after avif when jxl variants exist (guarded exactly like avif, so jxl-disabled — the default — renders no empty source). Generation unchanged; LCP/Early Hints single-format preload stays on avif. - F014: drop dead optimization paths. Remove the unreachable pngquant branch from ImageEngine::optimize() and stop detecting/surfacing opt_pngquant/opt_cwebp/opt_avifenc (never invoked); keep jpegoptim. - F028: ImageEngine::optimize() is now private static (only the internal encode() pipeline calls it). - F015: correct the encode() docstring — $format (not the $dest extension) selects the encoder.
- Add MediaAccessSecurityTest: 4 comprehensive tests locking in the access-control invariants for NSFW-only, password-only, combined NSFW+password albums, plus the cross-cutting boundaries (per-album scoping, unpublished 404, path-traversal rejection, downloads-disabled original fallback). Each asserts the core invariant: a gated visitor may receive a generic blur placeholder but NEVER the real sharp variant bytes, while an authorized viewer gets the real bytes from no-store private storage. Access-control code is byte-identical to main; these tests guard it against regression. - MigrationsTest: seed image_variants in its pre-1.4.13 shape so the new jxl-widening migration's table rebuild applies, and assert format='jxl' is accepted post-migration.
The home/about/cookie/license/privacy editors under /admin/pages had no way back to the page list except the browser button. Add a "← Back" btn-secondary linking to /admin/pages in each editor's header action group, matching the pattern already used in pages/galleries.twig (trans key admin.pages.back, already present in both locales).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/Services/Imaging/ImageEngine.php (2)
115-115:⚠️ Potential issue | 🟠 MajorPropagare
$stripanche ajpegoptim.
encode(..., $strip: false)preserva i metadati tramite vips (riga 179), maoptimize()(riga 115) esegue semprejpegoptim --strip-all(riga 164), annullando il parametro$strip. Passare$stripaoptimize()e usare--strip-nonequando$stripèfalse,--strip-allquando ètrue.Fix suggerito: propaga `$strip` all'optimizer
- self::optimize($dest, $format); + self::optimize($dest, $format, $strip); @@ - private static function optimize(string $path, string $format): void + private static function optimize(string $path, string $format, bool $strip): void @@ if (($format === 'jpeg' || $format === 'jpg') && $caps['opt_jpegoptim']) { - self::run(['jpegoptim', '--strip-all', '--quiet', $path]); + self::run(['jpegoptim', $strip ? '--strip-all' : '--strip-none', '--quiet', $path]); }🤖 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 `@app/Services/Imaging/ImageEngine.php` at line 115, The `$strip` parameter passed to the `encode` method is not being propagated to the `optimize()` method call on line 115, which causes jpegoptim to always strip all metadata regardless of the `$strip` setting. Modify the `optimize()` method signature to accept the `$strip` parameter, then pass it from the `encode()` method. Inside the `optimize()` method (around line 164 where jpegoptim is executed), conditionally use the `--strip-none` flag when `$strip` is false and `--strip-all` when `$strip` is true, ensuring metadata stripping behavior is consistent with the original parameter intent.
267-269:⚠️ Potential issue | 🟠 MajorAggiungi un timeout al processo optimizer per evitare blocchi.
proc_close()è una chiamata bloccante che attende la terminazione del processo figlio. Sejpegoptimrimane sospeso, blocca completamente il worker durante l'upload poichéImageEngine::encode()è invocato direttamente nel flusso diUploadService. Sebbene il codice menzioni che l'ottimizzazione è "opportunistica", non implementa alcun meccanismo di timeout o terminazione forzata. Considera di usareproc_terminate()con un timeout oppureproc_get_status()per verificare lo stato senza bloccare.🤖 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 `@app/Services/Imaging/ImageEngine.php` around lines 267 - 269, The proc_close() call in the ImageEngine class is blocking and can hang indefinitely if the jpegoptim process doesn't terminate, causing the entire upload worker to freeze. Replace the blocking proc_close() call with a timeout mechanism that uses proc_get_status() to monitor the process state and proc_terminate() to force terminate it if it exceeds the timeout threshold. This ensures the opportunistic optimization process does not block the upload flow when the child process becomes stuck or unresponsive.
🧹 Nitpick comments (2)
app/Views/frontend/home_parallax.twig (1)
179-187: ⚡ Quick winApplica escaping HTML attribute ai
srcsetnel blocco<picture>.In Line 183 (e anche Line 180/186),
srcsetviene renderizzato senzae('html_attr'). È meglio uniformare l’escaping per hardening output e coerenza con gli altri template.Diff proposto
{% if variants_avif %} - <source type="image/avif" srcset="{{ variants_avif|join(', ') }}" sizes="(min-width: 1200px) 33vw, (min-width: 768px) 50vw, 100vw"> + <source type="image/avif" srcset="{{ variants_avif|join(', ')|e('html_attr') }}" sizes="(min-width: 1200px) 33vw, (min-width: 768px) 50vw, 100vw"> {% endif %} {% if variants_jxl %} - <source type="image/jxl" srcset="{{ variants_jxl|join(', ') }}" sizes="(min-width: 1200px) 33vw, (min-width: 768px) 50vw, 100vw"> + <source type="image/jxl" srcset="{{ variants_jxl|join(', ')|e('html_attr') }}" sizes="(min-width: 1200px) 33vw, (min-width: 768px) 50vw, 100vw"> {% endif %} {% if variants_webp %} - <source type="image/webp" srcset="{{ variants_webp|join(', ') }}" sizes="(min-width: 1200px) 33vw, (min-width: 768px) 50vw, 100vw"> + <source type="image/webp" srcset="{{ variants_webp|join(', ')|e('html_attr') }}" sizes="(min-width: 1200px) 33vw, (min-width: 768px) 50vw, 100vw"> {% endif %}🤖 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 `@app/Views/frontend/home_parallax.twig` around lines 179 - 187, The srcset attributes in the picture element source blocks for variants_avif, variants_jxl, and variants_webp are missing HTML attribute escaping. Apply the e('html_attr') filter to each srcset value by piping the join() result through e('html_attr') in all three source elements to ensure consistent and secure output escaping across the template.app/Services/Imaging/ImageEngine.php (1)
70-75: ⚡ Quick winAllinea il contratto
$format/$destprima della scrittura.Il PHPDoc dice che
$destnon influenza il formato, mawriteToFile()in php-vips sceglie il saver dal suffisso del filename; le opzioni sono solo inoltrate al saver selezionato. Con un path temporaneo o un’estensione non allineata a$format, il contratto pubblico può produrre un formato diverso da quello atteso o fallire. (libvips.github.io)♻️ Possibile fix: valida che il suffisso sia coerente con il formato
- * `@param` string $dest Destination path; only where the file is written - * (it does not influence the output format). + * `@param` string $dest Destination path; php-vips selects the saver from + * this suffix, so it must match $format. @@ - * `@param` string $format 'jpeg' | 'webp' | 'avif' | 'jxl' — selects the - * encoder (passed to writeOptions); this argument, - * not the $dest extension, decides the output format. + * `@param` string $format 'jpeg' | 'webp' | 'avif' | 'jxl' — selects the + * encoder options and is validated against $dest. @@ $targetW = max(1, $targetW); $quality = max(1, min(100, $quality)); + if (!self::destinationMatchesFormat($dest, $format)) { + return false; + } @@ private static function writeOptions(string $format, int $quality, bool $strip): array { return match ($format) { @@ }; } + + private static function destinationMatchesFormat(string $dest, string $format): bool + { + $ext = strtolower(pathinfo($dest, PATHINFO_EXTENSION)); + + return match ($format) { + 'jpeg', 'jpg' => $ext === 'jpg' || $ext === 'jpeg', + 'webp' => $ext === 'webp', + 'avif' => $ext === 'avif', + 'jxl' => $ext === 'jxl', + default => false, + }; + }Also applies to: 109-109
🤖 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 `@app/Services/Imaging/ImageEngine.php` around lines 70 - 75, The PHPDoc states that the destination path `$dest` does not influence the output format, but php-vips's `writeToFile()` method actually selects the encoder based on the filename suffix, creating a contract violation. Add validation in the method where `$format` and `$dest` parameters are used (before calling `writeToFile()`) to ensure the file extension in `$dest` matches the requested `$format` parameter. If there is a mismatch between the extension and the format, either throw an exception or adjust the destination path to use the correct extension for the specified format.
🤖 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 `@app/Services/CacheWarmService.php`:
- Around line 884-885: The `processImageSourcesBatch()` method at line 884 was
updated to include 'jxl' in the sources array initialization, but the
`buildImageSources()` method at line 1007 still initializes sources without
'jxl'. This creates an inconsistency between the two paths for handling image
sources. Update the sources array initialization in `buildImageSources()` to
include 'jxl' key alongside 'avif', 'webp', and 'jpg', matching the
implementation in `processImageSourcesBatch()` so both methods handle JXL
support consistently.
In `@tests/Services/MediaAccessSecurityTest.php`:
- Line 232: The traversal path security test on line 232 uses
assertGreaterThanOrEqual(400, ...) which is too permissive and allows any status
code from 400 onwards (including 500 errors) to pass. Replace this with an
explicit assertion that checks for exactly 403, which is the expected security
response when a path traversal attempt is blocked. This ensures the test
validates the correct security behavior from the controller and prevents masking
potential regressions where an unexpected error code might be returned.
---
Outside diff comments:
In `@app/Services/Imaging/ImageEngine.php`:
- Line 115: The `$strip` parameter passed to the `encode` method is not being
propagated to the `optimize()` method call on line 115, which causes jpegoptim
to always strip all metadata regardless of the `$strip` setting. Modify the
`optimize()` method signature to accept the `$strip` parameter, then pass it
from the `encode()` method. Inside the `optimize()` method (around line 164
where jpegoptim is executed), conditionally use the `--strip-none` flag when
`$strip` is false and `--strip-all` when `$strip` is true, ensuring metadata
stripping behavior is consistent with the original parameter intent.
- Around line 267-269: The proc_close() call in the ImageEngine class is
blocking and can hang indefinitely if the jpegoptim process doesn't terminate,
causing the entire upload worker to freeze. Replace the blocking proc_close()
call with a timeout mechanism that uses proc_get_status() to monitor the process
state and proc_terminate() to force terminate it if it exceeds the timeout
threshold. This ensures the opportunistic optimization process does not block
the upload flow when the child process becomes stuck or unresponsive.
---
Nitpick comments:
In `@app/Services/Imaging/ImageEngine.php`:
- Around line 70-75: The PHPDoc states that the destination path `$dest` does
not influence the output format, but php-vips's `writeToFile()` method actually
selects the encoder based on the filename suffix, creating a contract violation.
Add validation in the method where `$format` and `$dest` parameters are used
(before calling `writeToFile()`) to ensure the file extension in `$dest` matches
the requested `$format` parameter. If there is a mismatch between the extension
and the format, either throw an exception or adjust the destination path to use
the correct extension for the specified format.
In `@app/Views/frontend/home_parallax.twig`:
- Around line 179-187: The srcset attributes in the picture element source
blocks for variants_avif, variants_jxl, and variants_webp are missing HTML
attribute escaping. Apply the e('html_attr') filter to each srcset value by
piping the join() result through e('html_attr') in all three source elements to
ensure consistent and secure output escaping across the template.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 59465a2c-319c-443f-8c47-3e7edb670b1c
📒 Files selected for processing (26)
app/Controllers/Admin/DiagnosticsController.phpapp/Controllers/Frontend/PageController.phpapp/Services/CacheWarmService.phpapp/Services/ImageVariantsService.phpapp/Services/Imaging/ImageEngine.phpapp/Views/frontend/_album_card.twigapp/Views/frontend/_album_gallery.twigapp/Views/frontend/_gallery_content.twigapp/Views/frontend/_gallery_creative_layout.twigapp/Views/frontend/_gallery_item.twigapp/Views/frontend/_gallery_magazine_content.twigapp/Views/frontend/_gallery_masonry_portfolio.twigapp/Views/frontend/_gallery_wall_scroll.twigapp/Views/frontend/_image_item.twigapp/Views/frontend/_image_item_masonry.twigapp/Views/frontend/_lqip_macros.twigapp/Views/frontend/_picture.twigapp/Views/frontend/album.twigapp/Views/frontend/gallery.twigapp/Views/frontend/gallery_magazine.twigapp/Views/frontend/home/_infinite_gallery.twigapp/Views/frontend/home_gallery.twigapp/Views/frontend/home_modern.twigapp/Views/frontend/home_parallax.twigtests/Database/MigrationsTest.phptests/Services/MediaAccessSecurityTest.php
💤 Files with no reviewable changes (1)
- app/Controllers/Admin/DiagnosticsController.php
| $sources = ['avif' => [], 'jxl' => [], 'webp' => [], 'jpg' => []]; | ||
| $variants = $variantsByImage[(int) $image['id']] ?? []; |
There was a problem hiding this comment.
Allinea anche il path single-image al supporto JXL.
Qui hai aggiornato processImageSourcesBatch(), ma a Line 1007 buildImageSources() inizializza ancora sources senza jxl. Questo crea output incoerente tra i due percorsi e può impedire l’emissione di JXL nei flussi che passano da processImageSources().
Diff proposto
private function buildImageSources(array $image, array $variants): array
{
$sources = [
'avif' => [],
+ 'jxl' => [],
'webp' => [],
'jpg' => [],
];Also applies to: 1007-1011
🤖 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 `@app/Services/CacheWarmService.php` around lines 884 - 885, The
`processImageSourcesBatch()` method at line 884 was updated to include 'jxl' in
the sources array initialization, but the `buildImageSources()` method at line
1007 still initializes sources without 'jxl'. This creates an inconsistency
between the two paths for handling image sources. Update the sources array
initialization in `buildImageSources()` to include 'jxl' key alongside 'avif',
'webp', and 'jpg', matching the implementation in `processImageSourcesBatch()`
so both methods handle JXL support consistently.
| new Response(), | ||
| ['path' => '../../../etc/passwd'] | ||
| ); | ||
| self::assertGreaterThanOrEqual(400, $traversal->getStatusCode(), 'traversal path must not resolve to a file'); |
There was a problem hiding this comment.
Restringi l’asserzione del traversal a 403 per non mascherare regressioni
In Line 232 l’asserzione >= 400 lascia passare anche un 500; per un test di sicurezza sul path traversal conviene verificare esplicitamente 403 (comportamento atteso del controller).
Diff suggerita
- self::assertGreaterThanOrEqual(400, $traversal->getStatusCode(), 'traversal path must not resolve to a file');
+ self::assertSame(403, $traversal->getStatusCode(), 'traversal path must be rejected with 403');📝 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.
| self::assertGreaterThanOrEqual(400, $traversal->getStatusCode(), 'traversal path must not resolve to a file'); | |
| self::assertSame(403, $traversal->getStatusCode(), 'traversal path must be rejected with 403'); |
🤖 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/Services/MediaAccessSecurityTest.php` at line 232, The traversal path
security test on line 232 uses assertGreaterThanOrEqual(400, ...) which is too
permissive and allows any status code from 400 onwards (including 500 errors) to
pass. Replace this with an explicit assertion that checks for exactly 403, which
is the expected security response when a path traversal attempt is blocked. This
ensures the test validates the correct security behavior from the controller and
prevents masking potential regressions where an unexpected error code might be
returned.
The F013 template work emitted <source type="image/jxl"> URLs, but the serving layer rejected the .jxl format, so those URLs 404'd on every album (all /media/ traffic routes through MediaController). Wire jxl through the full serve path: - EXT_TO_MIME gains jxl => image/jxl. - serveProtected format whitelist, servePublic filename regex + allowedMimes, serveResolvedFile + serveStaticFile allowedMimes all accept jxl. - ProtectedMediaStorage::mediaBasename accepts .jxl so protected jxl variants resolve/quarantine like the others. - Strict DB-path MIME gate: libmagic frequently can't identify JPEG-XL, which would spuriously fail the magic-byte cross-check. Verify the JXL signature directly (bare codestream 0xFF0A or the ISO-BMFF "JXL " box) instead of trusting finfo — keeps the strict guarantee on every libmagic version. A .jxl file whose bytes aren't JXL is still rejected. Also harden ProtectedMediaStorage deletions: route all 7 unlink() sites through a new confinedUnlink() that realpath-confines the target to the public/media or storage/protected-media root (defence-in-depth on top of mediaBasename()'s filename whitelist). Adds a JPEG-XL serving test (valid jxl → 200 image/jxl; spoofed bytes → 403) to MediaAccessSecurityTest.
) JPEG-XL generation previously required a libvips build with libjxl, which is rarer than the standalone cjxl (libjxl reference) encoder. Add a cjxl fallback so jxl variants are actually produced on far more hosts: - ImageEngine::capabilities()['jxl_write'] is now true when libvips can write jxl OR (Imagick is present AND cjxl is on PATH). - ImageEngine::encode() routes jxl to a new encodeJxlViaCjxl() when vips is absent or its build can't write jxl: Imagick produces the resized, metadata-stripped PNG intermediate, cjxl transcodes it to .jxl (argv proc_open, no shell). - ImagesGenerateCommand derives jxl variant dimensions from the source aspect ratio (getimagesize() can't read .jxl, which would otherwise store height=0). Verified end-to-end on a cjxl host: images:generate now emits real .jxl variants (FF0A codestream, correct dims), MediaController serves them as image/jxl, and NSFW/password albums quarantine them to protected-media. Adds ImageEngineJxlTest (skips where no jxl encoder exists).
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@app/Views/admin/pages/about.twig`:
- Line 11: The anchor tag with `target="_blank"` on the link to "{{ base_path
}}/about" is missing the security attribute `rel="noopener noreferrer"`, which
protects against reverse-tabnabbing attacks. Add `rel="noopener noreferrer"` to
the anchor tag that contains the `target="_blank"` attribute and the external
link icon.
- Line 11: The link in the view page button is hardcoded to `/about` instead of
using the configurable slug. Replace the hardcoded path `/about` in the href
attribute with the configurable `about_slug` variable that is available in this
form. Use proper Twig syntax to reference the slug variable so that custom slug
configurations are respected and the button opens the correct page.
In `@app/Views/admin/pages/license.twig`:
- Line 11: The external link on line 11 uses target="_blank" without the
rel="noopener noreferrer" attribute, which is a security vulnerability. Add the
rel="noopener noreferrer" attribute to the anchor tag that contains the license
page link (identified by class="btn-secondary" and target="_blank") to properly
harden the external link and prevent security risks from the opened page
accessing the window object of the opener.
In `@app/Views/admin/pages/privacy.twig`:
- Line 11: Add the rel="noopener noreferrer" attribute to the anchor tag in the
privacy policy view link. Locate the anchor element with class="btn-secondary"
that has target="_blank" attribute pointing to the privacy policy page, and
include rel="noopener noreferrer" to prevent tabnabbing security vulnerabilities
when the link opens in a new tab.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7e2c436-a6a0-431a-bbf7-a2a1040f4e49
📒 Files selected for processing (11)
app/Controllers/Frontend/MediaController.phpapp/Services/Imaging/ImageEngine.phpapp/Services/ProtectedMediaStorage.phpapp/Tasks/ImagesGenerateCommand.phpapp/Views/admin/pages/about.twigapp/Views/admin/pages/cookie.twigapp/Views/admin/pages/home.twigapp/Views/admin/pages/license.twigapp/Views/admin/pages/privacy.twigtests/Services/ImageEngineJxlTest.phptests/Services/MediaAccessSecurityTest.php
✅ Files skipped from review due to trivial changes (1)
- app/Views/admin/pages/cookie.twig
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Tasks/ImagesGenerateCommand.php
| <a href="{{ base_path }}/about" class="btn-secondary" target="_blank"><i class="fas fa-external-link-alt mr-2"></i>{{ trans('admin.pages.view_page') }}</a> | ||
| <div class="flex items-center gap-3"> | ||
| <a href="{{ base_path }}/admin/pages" class="btn-secondary"><i class="fas fa-arrow-left mr-2"></i>{{ trans('admin.pages.back') }}</a> | ||
| <a href="{{ base_path }}/about" class="btn-secondary" target="_blank"><i class="fas fa-external-link-alt mr-2"></i>{{ trans('admin.pages.view_page') }}</a> |
There was a problem hiding this comment.
Aggiungi rel al link con target="_blank".
A Line 11 manca rel="noopener noreferrer": è una protezione standard contro reverse-tabnabbing.
🤖 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 `@app/Views/admin/pages/about.twig` at line 11, The anchor tag with
`target="_blank"` on the link to "{{ base_path }}/about" is missing the security
attribute `rel="noopener noreferrer"`, which protects against reverse-tabnabbing
attacks. Add `rel="noopener noreferrer"` to the anchor tag that contains the
`target="_blank"` attribute and the external link icon.
Usa lo slug configurato per “view page”, non un path hardcoded.
A Line 11 il link punta sempre a /about, ma in questo form lo slug è configurabile (about_slug). Con slug personalizzato il pulsante apre la pagina sbagliata.
Diff proposto
- <a href="{{ base_path }}/about" class="btn-secondary" target="_blank"><i class="fas fa-external-link-alt mr-2"></i>{{ trans('admin.pages.view_page') }}</a>
+ <a href="{{ base_path }}/{{ settings['about.slug']|default('about') }}" class="btn-secondary" target="_blank" rel="noopener noreferrer"><i class="fas fa-external-link-alt mr-2"></i>{{ trans('admin.pages.view_page') }}</a>📝 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.
| <a href="{{ base_path }}/about" class="btn-secondary" target="_blank"><i class="fas fa-external-link-alt mr-2"></i>{{ trans('admin.pages.view_page') }}</a> | |
| <a href="{{ base_path }}/{{ settings['about.slug']|default('about') }}" class="btn-secondary" target="_blank" rel="noopener noreferrer"><i class="fas fa-external-link-alt mr-2"></i>{{ trans('admin.pages.view_page') }}</a> |
🤖 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 `@app/Views/admin/pages/about.twig` at line 11, The link in the view page
button is hardcoded to `/about` instead of using the configurable slug. Replace
the hardcoded path `/about` in the href attribute with the configurable
`about_slug` variable that is available in this form. Use proper Twig syntax to
reference the slug variable so that custom slug configurations are respected and
the button opens the correct page.
| <a href="{{ base_path }}/{{ settings['license.slug']|default('license') }}" class="btn-secondary" target="_blank"><i class="fas fa-external-link-alt mr-2"></i>{{ trans('admin.pages.view_page') }}</a> | ||
| <div class="flex items-center gap-3"> | ||
| <a href="{{ base_path }}/admin/pages" class="btn-secondary"><i class="fas fa-arrow-left mr-2"></i>{{ trans('admin.pages.back') }}</a> | ||
| <a href="{{ base_path }}/{{ settings['license.slug']|default('license') }}" class="btn-secondary" target="_blank"><i class="fas fa-external-link-alt mr-2"></i>{{ trans('admin.pages.view_page') }}</a> |
There was a problem hiding this comment.
Completa l’hardening del link esterno con rel.
A Line 11 il link usa target="_blank" ma senza rel="noopener noreferrer".
🤖 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 `@app/Views/admin/pages/license.twig` at line 11, The external link on line 11
uses target="_blank" without the rel="noopener noreferrer" attribute, which is a
security vulnerability. Add the rel="noopener noreferrer" attribute to the
anchor tag that contains the license page link (identified by
class="btn-secondary" and target="_blank") to properly harden the external link
and prevent security risks from the opened page accessing the window object of
the opener.
| <a href="{{ base_path }}/{{ settings['privacy.slug']|default('privacy-policy') }}" class="btn-secondary" target="_blank"><i class="fas fa-external-link-alt mr-2"></i>{{ trans('admin.pages.view_page') }}</a> | ||
| <div class="flex items-center gap-3"> | ||
| <a href="{{ base_path }}/admin/pages" class="btn-secondary"><i class="fas fa-arrow-left mr-2"></i>{{ trans('admin.pages.back') }}</a> | ||
| <a href="{{ base_path }}/{{ settings['privacy.slug']|default('privacy-policy') }}" class="btn-secondary" target="_blank"><i class="fas fa-external-link-alt mr-2"></i>{{ trans('admin.pages.view_page') }}</a> |
There was a problem hiding this comment.
Aggiungi rel="noopener noreferrer" al link “view page”.
A Line 11 c’è target="_blank" senza attributo rel, con rischio di tabnabbing.
🤖 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 `@app/Views/admin/pages/privacy.twig` at line 11, Add the rel="noopener
noreferrer" attribute to the anchor tag in the privacy policy view link. Locate
the anchor element with class="btn-secondary" that has target="_blank" attribute
pointing to the privacy policy page, and include rel="noopener noreferrer" to
prevent tabnabbing security vulnerabilities when the link opens in a new tab.
Walkthrough decisions
Walking the Full skip set scope: of 5 non-auto-eligible finding(s), 4 promoted, 1 skipped. Promoted findings will be picked up by the next Promoted
Skipped
Decisions log: append-only audit — never edited in place. Current state: see the main review comment and |
Comment/message-only, no logic change:
- F017: resizeWithImagick() comment described an inline Imagick body
"below" that no longer exists after the F003 refactor — it now
delegates to resizeWithImagickOnly(). Reworded to match.
- F018: $allowed comment said "variants are jpg/webp/avif as usual";
jxl is now a real generated+served variant — updated to include it.
- F029: the HEIC-unsupported RuntimeException leaked which imaging
libraries are present ("no libheif / Imagick HEIC delegate") in the
client JSON response. The client now gets a generic message; the
detail is logged server-side via Logger::warning. Acceptance logic
unchanged.
- F016: strengthened the nosemgrep rationale on ImageEngine::run() —
it's private static and every caller passes a constant binary name
plus an app-controlled path, so a future user-influenced argv would
be caught in review.
Adds 23 reusable, capability-guarded tests over the changed surface (skip cleanly where libvips/cjxl/GD are absent so CI stays portable): - ImageEngineTest (10): capabilities() contract + caching, jxl_write / heif_read backing-encoder consistency, encode() fast-path/fallback contract, JPEG-XL-via-cjxl encode (valid signature, no upscale, degenerate-input clamping), dimensions() vips/no-vips contract. - MediaControllerMediaTypeTest (9): JPEG/JPEG-XL served with correct MIME, ISO-BMFF + codestream JXL signatures accepted, spoofed / truncated / empty .jxl rejected by the strict magic-byte gate, unknown extension never resolves, public jxl cacheable, protected jxl quarantined + no-store. - ProtectedMediaStorageTest (+4): .jxl basename resolves and quarantines for protected albums / stays public for public ones, confinedUnlink removes jxl inside the media roots, and traversal dbPaths never resolve or delete. Together with this branch's MediaAccessSecurityTest (5), ImageEngineJxl Test (1) and the MigrationsTest jxl coverage, the changed code is locked in against regression.
Cover the capability-detected engine (libvips fast path + Imagick/GD fallback), HEIC/HEIF import for iPhone photos, and opt-in JPEG-XL generation+serving (libvips+libjxl or cjxl): variant tree, quality table, format/HEIC settings, and the headline feature line.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@tests/Services/ImageEngineTest.php`:
- Around line 174-176: The tempnam() function can return false on failure, but
the code does not check for this before calling unlink() and concatenating with
the $ext variable, which will cause a TypeError. Add a check after the tempnam()
call to verify it did not return false. If it does return false, throw an
appropriate exception or skip the test. Only proceed with the unlink() call and
string concatenation if tempnam() successfully returns a valid file path string.
- Around line 91-105: The testEncodedJxlNeverUpscalesBeyondSource method
currently only verifies that encoding succeeds and produces a valid JXL file,
but does not actually assert that the output dimensions were not upscaled. To
fix this, after the ImageEngine::encode call successfully creates the JXL file
at $dest, add logic to read the actual dimensions of the encoded JXL output and
assert that the width is less than or equal to the source width of 40 pixels.
This will ensure that a regression where upscaling occurs would properly fail
the test.
In `@tests/Services/MediaControllerMediaTypeTest.php`:
- Around line 32-56: The MEDIA_XSENDFILE environment variable affects the
behavior of MediaController::streamFile() and causes test flakiness when it is
set, resulting in empty body assertions failing at Line 74. In the setUp()
method, save the original value of the MEDIA_XSENDFILE environment variable from
both $_ENV and getenv(), then unset it to ensure deterministic test behavior. In
the tearDown() method, restore the original MEDIA_XSENDFILE value if it was
previously set, ensuring the environment is cleaned up properly after each test
run.
- Around line 103-120: The test assertions are too loose and should verify exact
status codes instead of accepting any 4xx or 5xx response. In the
testEmptyJxlFileRejected method, replace the assertGreaterThanOrEqual(400) call
with an assertion that checks for the specific 403 status code to match the
strict validation gate used for other invalid JXL files. In the
testUnknownExtensionDoesNotResolve method, replace the
assertGreaterThanOrEqual(400) call with an assertion that checks for the
specific 404 status code as indicated by the comment that the unknown .gif
extension must return exactly 404, never stream.
In `@tests/Services/ProtectedMediaStorageTest.php`:
- Around line 224-233: The test testTraversalDbPathNeverResolvesOrDeletes
currently uses inputs (passwd, hosts) that would be rejected solely by the
basename regex validation, not actually testing the `..` traversal guard in
mediaBasename(). Add test assertions with paths containing `..` sequences but
otherwise valid basenames (for example /media/../media/{id}_md.jxl format) to
properly exercise the str_contains($normalized, '..') check in the
mediaBasename() method, ensuring the traversal prevention logic is genuinely
being tested.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5fb871cd-0e62-4e82-8ec3-984c43590c80
📒 Files selected for processing (6)
README.mdapp/Services/Imaging/ImageEngine.phpapp/Services/UploadService.phptests/Services/ImageEngineTest.phptests/Services/MediaControllerMediaTypeTest.phptests/Services/ProtectedMediaStorageTest.php
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- app/Services/Imaging/ImageEngine.php
- app/Services/UploadService.php
| public function testEncodedJxlNeverUpscalesBeyondSource(): void | ||
| { | ||
| if (!ImageEngine::capabilities()['jxl_write']) { | ||
| self::markTestSkipped('No JPEG-XL encoder on this host.'); | ||
| } | ||
| // Source is 40px wide; asking for 1000 must NOT upscale. We can't read | ||
| // jxl dims with getimagesize, so assert via a re-encode round-trip is | ||
| // out of scope — instead assert the encode succeeds and the file is a | ||
| // valid, non-empty jxl (the 'size'=>'down' guard is what prevents the | ||
| // upscale; this proves the small-source path doesn't error). | ||
| $dest = $this->tmpPath('.jxl'); | ||
| self::assertTrue(ImageEngine::encode($this->makePng(40, 30), $dest, 1000, 'jxl', 70)); | ||
| self::assertGreaterThan(0, (int) filesize($dest)); | ||
| self::assertTrue($this->isJxl($dest)); | ||
| } |
There was a problem hiding this comment.
Il test “never upscale” non verifica realmente l’assenza di upscale.
Qui stai verificando solo successo encode + firma JXL; una regressione che upscala comunque passerebbe. Conviene limitare questo test ai casi in cui puoi leggere le dimensioni e asserire davvero width <= sourceWidth.
Diff proposto
public function testEncodedJxlNeverUpscalesBeyondSource(): void
{
- if (!ImageEngine::capabilities()['jxl_write']) {
- self::markTestSkipped('No JPEG-XL encoder on this host.');
+ $caps = ImageEngine::capabilities();
+ if (!$caps['jxl_write']) {
+ self::markTestSkipped('No JPEG-XL encoder on this host.');
+ }
+ if (!$caps['vips']) {
+ self::markTestSkipped('Dimension verification for JXL requires vips on this host.');
}
- // Source is 40px wide; asking for 1000 must NOT upscale. We can't read
- // jxl dims with getimagesize, so assert via a re-encode round-trip is
- // out of scope — instead assert the encode succeeds and the file is a
- // valid, non-empty jxl (the 'size'=>'down' guard is what prevents the
- // upscale; this proves the small-source path doesn't error).
+
$dest = $this->tmpPath('.jxl');
self::assertTrue(ImageEngine::encode($this->makePng(40, 30), $dest, 1000, 'jxl', 70));
- self::assertGreaterThan(0, (int) filesize($dest));
- self::assertTrue($this->isJxl($dest));
+ $dims = ImageEngine::dimensions($dest);
+ self::assertNotNull($dims);
+ self::assertLessThanOrEqual(40, $dims[0]);
}🤖 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/Services/ImageEngineTest.php` around lines 91 - 105, The
testEncodedJxlNeverUpscalesBeyondSource method currently only verifies that
encoding succeeds and produces a valid JXL file, but does not actually assert
that the output dimensions were not upscaled. To fix this, after the
ImageEngine::encode call successfully creates the JXL file at $dest, add logic
to read the actual dimensions of the encoded JXL output and assert that the
width is less than or equal to the source width of 40 pixels. This will ensure
that a regression where upscaling occurs would properly fail the test.
| $p = tempnam(sys_get_temp_dir(), 'cimaise_iet_'); | ||
| @unlink($p); | ||
| $p .= $ext; |
There was a problem hiding this comment.
Gestisci il fallimento di tempnam() prima di usarne il risultato.
Se tempnam() ritorna false, qui rischi un TypeError su unlink()/concatenazione e il test fallisce in modo intermittente.
Diff proposto
private function tmpPath(string $ext): string
{
$p = tempnam(sys_get_temp_dir(), 'cimaise_iet_');
+ self::assertNotFalse($p, 'tempnam() failed creating a temporary path');
`@unlink`($p);
$p .= $ext;
$this->tmp[] = $p;
return $p;
}🧰 Tools
🪛 ast-grep (0.43.0)
[info] 174-174: Avoid unsafe call to unlink
Context: unlink($p)
Note: [CWE-73] External Control of File Name or Path.
(avoid-unlink)
🤖 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/Services/ImageEngineTest.php` around lines 174 - 176, The tempnam()
function can return false on failure, but the code does not check for this
before calling unlink() and concatenating with the $ext variable, which will
cause a TypeError. Add a check after the tempnam() call to verify it did not
return false. If it does return false, throw an appropriate exception or skip
the test. Only proceed with the unlink() call and string concatenation if
tempnam() successfully returns a valid file path string.
| protected function setUp(): void | ||
| { | ||
| $_SERVER['SCRIPT_NAME'] = '/index.php'; | ||
| $_SESSION = []; | ||
| $this->nextId = random_int(6_100_000, 6_900_000); | ||
| $this->dbFile = sys_get_temp_dir() . '/cimaise_mediatype_' . uniqid('', true) . '.sqlite'; | ||
| $this->db = new Database(null, null, $this->dbFile, null, null, 'utf8mb4', 'utf8mb4_unicode_ci', true); | ||
| $this->db->pdo()->exec( | ||
| 'CREATE TABLE albums (id INTEGER PRIMARY KEY, slug TEXT, is_published INTEGER NOT NULL DEFAULT 1, | ||
| is_nsfw INTEGER NOT NULL DEFAULT 0, password_hash TEXT, allow_downloads INTEGER NOT NULL DEFAULT 1); | ||
| CREATE TABLE images (id INTEGER PRIMARY KEY, album_id INTEGER NOT NULL, original_path TEXT, mime TEXT); | ||
| CREATE TABLE image_variants (id INTEGER PRIMARY KEY AUTOINCREMENT, image_id INTEGER NOT NULL, | ||
| variant TEXT NOT NULL, format TEXT NOT NULL, path TEXT NOT NULL, width INTEGER, height INTEGER, size_bytes INTEGER);' | ||
| ); | ||
| } | ||
|
|
||
| protected function tearDown(): void | ||
| { | ||
| $_SESSION = []; | ||
| unset($this->db); | ||
| foreach (array_merge($this->createdFiles, [$this->dbFile, $this->dbFile . '-wal', $this->dbFile . '-shm']) as $f) { | ||
| // nosemgrep: test-fixture cleanup; $f is a path this test created. | ||
| @unlink($f); // nosemgrep | ||
| } | ||
| } |
There was a problem hiding this comment.
Isola MEDIA_XSENDFILE per rendere deterministica la verifica del body.
MediaController::streamFile() delega lo streaming quando MEDIA_XSENDFILE è impostato; in quel caso l’assertion sul body esatto alla Linea 74 può fallire con un body vuoto. Salva, unsetta e ripristina $_ENV['MEDIA_XSENDFILE']/getenv('MEDIA_XSENDFILE') in setUp()/tearDown().
🧰 Tools
🪛 ast-grep (0.43.0)
[error] 36-36: Avoid pseudo-random numbers
Context: uniqid('', true)
Note: [CWE-338] Use of Cryptographically Weak Pseudo-Random Number Generator (PRNG).
(no-pseudo-random-php)
[info] 53-53: Avoid unsafe call to unlink
Context: unlink($f)
Note: [CWE-73] External Control of File Name or Path.
(avoid-unlink)
🤖 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/Services/MediaControllerMediaTypeTest.php` around lines 32 - 56, The
MEDIA_XSENDFILE environment variable affects the behavior of
MediaController::streamFile() and causes test flakiness when it is set,
resulting in empty body assertions failing at Line 74. In the setUp() method,
save the original value of the MEDIA_XSENDFILE environment variable from both
$_ENV and getenv(), then unset it to ensure deterministic test behavior. In the
tearDown() method, restore the original MEDIA_XSENDFILE value if it was
previously set, ensuring the environment is cleaned up properly after each test
run.
| public function testEmptyJxlFileRejected(): void | ||
| { | ||
| $id = $this->insertImage(); | ||
| $this->writeRawVariant($id, 'md', 'jxl', ''); | ||
| self::assertGreaterThanOrEqual(400, $this->servePublic($id, 'md', 'jxl')->getStatusCode()); | ||
| } | ||
|
|
||
| public function testUnknownExtensionDoesNotResolve(): void | ||
| { | ||
| // The public /media/{path} filename whitelist is jpg|webp|avif|jxl|png; | ||
| // a .gif (or anything else) must 404, never stream. | ||
| $id = $this->insertImage(); | ||
| $r = $this->controller()->servePublic( | ||
| (new ServerRequestFactory())->createServerRequest('GET', "/media/{$id}_sm.gif"), | ||
| new Response(), | ||
| ['path' => "{$id}_sm.gif"] | ||
| ); | ||
| self::assertGreaterThanOrEqual(400, $r->getStatusCode()); |
There was a problem hiding this comment.
Rendi esatti gli status attesi invece di accettare qualunque 4xx/5xx.
assertGreaterThanOrEqual(400) farebbe passare anche un 500. Qui il file JXL vuoto dovrebbe seguire lo stesso strict gate degli altri JXL non validi (403), mentre il commento del caso .gif dice esplicitamente che deve essere 404.
Proposta di assertion più stretta
- self::assertGreaterThanOrEqual(400, $this->servePublic($id, 'md', 'jxl')->getStatusCode());
+ self::assertSame(403, $this->servePublic($id, 'md', 'jxl')->getStatusCode());
@@
- self::assertGreaterThanOrEqual(400, $r->getStatusCode());
+ self::assertSame(404, $r->getStatusCode());🤖 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/Services/MediaControllerMediaTypeTest.php` around lines 103 - 120, The
test assertions are too loose and should verify exact status codes instead of
accepting any 4xx or 5xx response. In the testEmptyJxlFileRejected method,
replace the assertGreaterThanOrEqual(400) call with an assertion that checks for
the specific 403 status code to match the strict validation gate used for other
invalid JXL files. In the testUnknownExtensionDoesNotResolve method, replace the
assertGreaterThanOrEqual(400) call with an assertion that checks for the
specific 404 status code as indicated by the comment that the unknown .gif
extension must return exactly 404, never stream.
| public function testTraversalDbPathNeverResolvesOrDeletes(): void | ||
| { | ||
| // mediaBasename() rejects any path containing '..' or outside /media/, | ||
| // so confinedUnlink/resolveVariantPath can never escape the media dirs. | ||
| $storage = new ProtectedMediaStorage($this->db); | ||
| self::assertNull($storage->resolveVariantPath('/media/../../etc/passwd', true)); | ||
| self::assertNull($storage->resolveVariantPath('/etc/passwd', false)); | ||
| // deleteVariantCopies on a traversal path is a no-op that returns false | ||
| // (no basename ⇒ nothing deleted), and obviously touches nothing outside. | ||
| self::assertFalse($storage->deleteVariantCopies('/media/../../../etc/hosts')); |
There was a problem hiding this comment.
Usa un traversal con basename valido per coprire davvero la guardia ...
Gli input attuali (passwd, hosts) verrebbero respinti anche solo dalla regex del basename; il test passerebbe anche rimuovendo il controllo str_contains($normalized, '..') in ProtectedMediaStorage::mediaBasename(). Aggiungi almeno un path con .. ma con basename altrimenti valido, ad esempio /media/../media/{$id}_md.jxl.
Proposta di rinforzo del test
$storage = new ProtectedMediaStorage($this->db);
+ $validLookingTraversal = '/media/../media/' . $this->nextId . '_md.jxl';
+
+ self::assertNull($storage->resolveVariantPath($validLookingTraversal, true));
+ self::assertFalse($storage->deleteVariantCopies($validLookingTraversal));
self::assertNull($storage->resolveVariantPath('/media/../../etc/passwd', true));
self::assertNull($storage->resolveVariantPath('/etc/passwd', 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 `@tests/Services/ProtectedMediaStorageTest.php` around lines 224 - 233, The
test testTraversalDbPathNeverResolvesOrDeletes currently uses inputs (passwd,
hosts) that would be rejected solely by the basename regex validation, not
actually testing the `..` traversal guard in mediaBasename(). Add test
assertions with paths containing `..` sequences but otherwise valid basenames
(for example /media/../media/{id}_md.jxl format) to properly exercise the
str_contains($normalized, '..') check in the mediaBasename() method, ensuring
the traversal prevention logic is genuinely being tested.
1 participant
Primo incremento di #109 (pipeline immagine moderna).
Cosa fa
App\Services\Imaging\ImageEngine: encoder con rilevamento capacità.jcupitt/vips): shrink-on-load, bassa RAM; produce jpeg/webp/avif/jxl quando il build li supporta.capabilities(): rileva vips, lettura HEIC/HEIF, scrittura AVIF/JPEG-XL, e i binari optimizer (jpegoptim/pngquant/cwebp/avifenc).proc_openin forma argv-array (niente shell → niente command injection).images:generateprovaImageEngine::encode()per primo e ricade sul path Imagick/GD esistente quando vips non c'è → zero cambi di comportamento senza vips (verificato:encode()ritornafalsee si usa il fallback).realpath()sottopublic/media(guardia path-traversal).Attivazione
composer require jcupitt/vips+ estensione php-vips / libvips con heif+aom (AVIF) e libjxl (JPEG-XL). Senza queste dipendenze il comportamento resta identico a prima.Sanity (host attuale, senza vips)
caps = {vips:false, imagick:true, heif_read:true (Imagick), avif_write:true (Imagick), jxl_write:false, ...}→ fallback corretto.Prossimi step sulla stessa issue
composer.json(suggest/require) + smoke test.Closes #109
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes