fix(review): harden photo reorder against data loss + failure-path drift

Addresses ce-code-review findings on the photo-editor media-API work:

- P0 (#1): PhotoRenumberer now renumbers EVERY on-disk image, using the
  client manifest only as preferred ORDER and appending any omitted image
  at the end. A stale/incomplete `order` (e.g. a second browser tab)
  previously left an unlisted photo at a target slot for phase-2's
  rename() to silently overwrite — verified data loss, now impossible.
  The reorder route inherits the guard; create/reconcile is unchanged.
- P2 (#3): unique per-call token in the .reorder-tmp-* name so two
  concurrent renumbers on one folder can't collide and clobber bytes.
- P3 (#7): de-duplicate the manifest so a repeated name can't shift/drop
  a photo.
- P2 (#2): applyReorder + doDelete split the two failure stages — a failed
  refresh AFTER a committed reorder/delete no longer reverts to a stale or
  ghost state, it reconciles to disk. A DELETE 404 is treated as success
  so a retried ghost cell converges.
- P2 (#4): both custom routes call requirePermission('api.pages.write')
  so the GHSA-x7hm API-key scope cap applies (owner already holds it, so
  the owner-only behaviour is unchanged).
- P3 (#8): refresh stale comments (photo-01..NN; drop editLoadPhotos ref).

PhotoRenumberer's 7-case unit suite still passes and the data-loss repro
now preserves all bytes. Assets rebuilt.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

plugins/cache-on-save/classes/PhotoRenumberer.php

@@ -18,6 +18,14 @@ namespace Grav\Plugin\Shared;
  * renamed. A crafted manifest entry naming the entry `.md`, a `.gpx`, or a
  * `.meta.yaml` sidecar is silently skipped — it can never be renamed or clobbered.
  * This guard lives here (not only in the callers) so every caller inherits it.
+ *
+ * Completeness: renumber() ALWAYS renumbers every image already in $dir, not just
+ * the manifest subset. $names only supplies the preferred ORDER; any on-disk image
+ * the manifest omits is appended at the end. This makes an incomplete/stale
+ * manifest (e.g. a second browser tab whose list predates a change) harmless —
+ * without it, an unlisted image left sitting at a target slot would be silently
+ * OVERWRITTEN (destroyed) by the second-phase rename. The reorder route trusts a
+ * client-supplied list, so this guard is what keeps it from losing photos.
  */
 class PhotoRenumberer
 {
@@ -26,13 +34,16 @@ class PhotoRenumberer
     private const IMAGE_EXTS = ['jpg', 'jpeg', 'png', 'gif', 'webp', 'heic', 'heif'];
 
     /**
-     * Rename the image files named in $names to photo-01..NN in that order, in $dir.
+     * Renumber every image in $dir to photo-01..NN, using $names as the preferred
+     * order and appending any unlisted on-disk images at the end.
      *
      * Two-phase via temp names so a target (photo-02.jpg) can't clobber a
      * not-yet-moved source of the same name (e.g. a straight swap or the
      * un-padded photo-N → photo-0N normalisation pass). Non-image and missing
-     * files in $names are skipped and do not consume an index, so the surviving
-     * images are always numbered contiguously from 01.
+     * files in $names are skipped and do not consume an index; a repeated name is
+     * counted once. Because every on-disk image becomes a target (see the
+     * completeness note on the class), the surviving images are numbered
+     * contiguously from 01 and no untouched file is ever overwritten.
      *
      * Idempotent: a file already at its correct padded name is left untouched,
      * so re-running with the same manifest (e.g. an auto-retried reorder) is a
@@ -40,14 +51,18 @@ class PhotoRenumberer
      */
     public static function renumber(string $dir, array $names): void
     {
-        // Keep only real image files, in the requested order — this is both the
-        // security filter and what determines the pad width.
+        // Keep only real image files, in the requested order, de-duplicated —
+        // this is both the security filter and what determines the pad width.
         $targets = [];
+        $seen = [];
         foreach ($names as $name) {
             if (!is_string($name) || $name === '') {
                 continue;
             }
             $base = basename(str_replace('\\', '/', $name));
+            if (isset($seen[$base])) {
+                continue; // a repeated name must not consume a second index
+            }
             $src = $dir . DIRECTORY_SEPARATOR . $base;
             if (!is_file($src)) {
                 continue; // not on disk — skip (idempotent for auto-retry)
@@ -56,11 +71,43 @@ class PhotoRenumberer
             if (!in_array($ext, self::IMAGE_EXTS, true)) {
                 continue; // never rename the entry .md, a .gpx, or a sidecar
             }
+            $seen[$base] = true;
             $targets[] = [$src, $ext ?: 'jpg'];
         }
 
+        // Completeness guard: append EVERY other image already in $dir that the
+        // manifest didn't list (natural name order), so an incomplete/stale
+        // manifest can't leave an unlisted image at a target slot for phase-2 to
+        // overwrite. Hidden files ('.'-prefixed temp/sidecar) are never targets.
+        $extra = [];
+        foreach (@scandir($dir) ?: [] as $f) {
+            if ($f === '' || $f[0] === '.' || isset($seen[$f])) {
+                continue;
+            }
+            $p = $dir . DIRECTORY_SEPARATOR . $f;
+            if (!is_file($p)) {
+                continue;
+            }
+            $ext = strtolower(pathinfo($f, PATHINFO_EXTENSION));
+            if (!in_array($ext, self::IMAGE_EXTS, true)) {
+                continue;
+            }
+            $extra[$f] = [$p, $ext ?: 'jpg'];
+        }
+        if ($extra) {
+            uksort($extra, 'strnatcasecmp');
+            foreach ($extra as $t) {
+                $targets[] = $t;
+            }
+        }
+
         $width = max(2, strlen((string) count($targets)));
 
+        // Unique per-call token in the temp name so two concurrent renumbers on
+        // the same folder can't collide on a shared '.reorder-tmp-N' path and
+        // overwrite one photo's bytes.
+        $token = bin2hex(random_bytes(4));
+
         $planned = [];
         $i = 1;
         foreach ($targets as [$src, $ext]) {
@@ -70,7 +117,7 @@ class PhotoRenumberer
                 $i++;
                 continue; // already correctly named — leave it
             }
-            $tmp = $dir . DIRECTORY_SEPARATOR . '.reorder-tmp-' . $i . '.' . $ext;
+            $tmp = $dir . DIRECTORY_SEPARATOR . '.reorder-tmp-' . $token . '-' . $i . '.' . $ext;
             @rename($src, $tmp);
             $planned[] = [$tmp, $final];
             $i++;

plugins/entry-actions/classes/EntryActionsApiController.php

@@ -35,6 +35,11 @@ class EntryActionsApiController extends AbstractApiController
     {
         // Authenticated OWNER only (KTD8). getUser() throws 401 for anonymous.
         $user = $this->getUser($request);
+        // Enforce the API-key scope cap (GHSA-x7hm) with the SAME permission the
+        // stock media/page-write endpoints require. The owner already holds it
+        // (their add/delete media uploads pass it), so this only caps a scoped
+        // key — it never blocks the legitimate owner.
+        $this->requirePermission($request, 'api.pages.write');
         if (!EntryScopeGuard::isOwnerUser($this->grav, $user)) {
             throw new ForbiddenException('Only the site owner can delete journal entries.');
         }
@@ -73,12 +78,16 @@ class EntryActionsApiController extends AbstractApiController
      * Filename safety is defence in depth: unsafe segments (containing '/' or '..')
      * are dropped here, and PhotoRenumberer only ever renames files that already
      * exist as image media in the folder — so a crafted order body can never touch
-     * the entry .md, a .gpx or a .meta.yaml sidecar.
+     * the entry .md, a .gpx or a .meta.yaml sidecar. An incomplete `order` (e.g. a
+     * stale second tab) is safe too: PhotoRenumberer renumbers every on-disk image,
+     * appending any the manifest omits, so no photo is lost — `order` only sorts.
      */
     public function reorderPhotos(ServerRequestInterface $request): ResponseInterface
     {
         // Authenticated OWNER only (KTD8). getUser() throws 401 for anonymous.
         $user = $this->getUser($request);
+        // Enforce the API-key scope cap (GHSA-x7hm) — see deleteEntry above.
+        $this->requirePermission($request, 'api.pages.write');
         if (!EntryScopeGuard::isOwnerUser($this->grav, $user)) {
             throw new ForbiddenException('Only the site owner can reorder entry photos.');
         }

themes/intotheeast/js/src/post-form.js

@@ -155,7 +155,7 @@ function initPhotoConversion() {
     // current visual order into a hidden input. Its name is a TOP-LEVEL POST key
     // ("photo_order", not "data[...]") so Grav's form never captures it into the
     // page data — the server reads it straight from $_POST and renames the
-    // copied files photo-1..N to match, with nothing leaking into frontmatter.
+    // copied files photo-01..NN to match, with nothing leaking into frontmatter.
     form.addEventListener('submit', function () {
         if (!orderPond) return;
         var files = orderPond.getFiles();
@@ -277,7 +277,7 @@ function initPhotoConversion() {
             if (pond && !pond._heicHooked) {
                 pond._heicHooked = true;
                 // allowReorder: drag thumbnails to set the order. The server
-                // (cache-on-save) renames the copied files photo-1..N in the
+                // (cache-on-save) renames the copied files photo-01..NN in the
                 // submitted order so the published entry honours it (entry media
                 // is filename-ordered; hero = first).
                 pond.setOptions({ beforeAddFile: makeBeforeAddFile(pond), allowReorder: true, itemInsertLocation: 'after' });
@@ -745,10 +745,10 @@ function editSetContent(value) {
 //
 // The photos/FilePond control is deliberately EXCLUDED: FilePond reads the
 // disabled state of its underlying input when the form plugin creates it and
-// never re-enables (removing its browse button, so the owner can't add photos on
-// edit — U7). Photos are also safe to leave live during the D1 prefill window:
-// they load additively (editLoadPhotos), not by overwrite, so early interaction
-// can't be clobbered the way an empty text field could.
+// never re-enables. In edit mode FilePond is decommissioned anyway — the live
+// photo editor (initPhotoEditor) hides it and manages add/delete/reorder against
+// the media API independently of this form's Save — so its disabled state during
+// the D1 prefill window is irrelevant.
 function editFormDisabled(form, disabled) {
     var els = form.querySelectorAll('input, textarea, select, button');
     Array.prototype.forEach.call(els, function (el) {
@@ -951,14 +951,18 @@ function initPhotoEditor(route) {
         var lastGood = photos.slice();
         setBusy(true);
         setStatus('Saving order…');
-        reorder(next)
-            .then(function () { return mediaList(); })
-            .then(function (list) { setStatus(''); render(list); })
-            .catch(function () {
-                setStatus('Couldn’t save the new order — reverted. Try again.', true);
-                render(lastGood); // revert the SortableJS move to last-known-good
-            })
-            .then(function () { setBusy(false); });
+        // Split the two failure stages: a failed SAVE reverts the drag; a failed
+        // REFRESH after a successful save must NOT revert (the order did persist)
+        // — show the saved order and reconcile on the next op.
+        reorder(next).then(function () {
+            return mediaList().then(
+                function (list) { setStatus(''); render(list); },
+                function () { setStatus(''); render(next); } // saved; DOM already shows it
+            );
+        }, function () {
+            setStatus('Couldn’t save the new order — reverted. Try again.', true);
+            render(lastGood); // revert the SortableJS move to last-known-good
+        }).then(function () { setBusy(false); });
     }
 
     // ✕ swaps the cell to an inline "Delete? [Confirm] [Cancel]" (option a).
@@ -985,17 +989,26 @@ function initPhotoEditor(route) {
 
     function doDelete(name) {
         var lastGood = photos.slice();
+        var remaining = photos.filter(function (p) { return p !== name; });
         setBusy(true);
         setStatus('Deleting…');
-        apiOk('/api/v1/pages' + route + '/media/' + encodeURIComponent(name), { method: 'DELETE' })
-            .then(function (ok) {
-                if (!ok) throw new Error('delete failed');
-                // Renumber the survivors so cover=first stays correct (idempotent).
-                var remaining = photos.filter(function (p) { return p !== name; });
-                return (remaining.length ? reorder(remaining) : Promise.resolve()).then(mediaList);
-            })
-            .then(function (list) { setStatus(''); render(list); })
-            .catch(function () {
+        // A 404 means the file is already gone — treat it as success so retrying
+        // a ghost cell converges instead of looping on "couldn't delete".
+        fetch('/api/v1/pages' + route + '/media/' + encodeURIComponent(name), { credentials: 'include', method: 'DELETE' })
+            .then(function (r) {
+                if (!(r.ok || r.status === 204 || r.status === 404)) throw new Error('delete failed');
+                // Deleted. Renumber survivors (cover=first), then refresh. A failure
+                // AFTER this point must NOT resurrect the deleted photo — show the
+                // survivor set, never lastGood.
+                return (remaining.length ? reorder(remaining) : Promise.resolve()).then(mediaList).then(
+                    function (list) { setStatus(''); render(list); },
+                    function () {
+                        setStatus('Photo deleted, but refreshing the list failed — reload if it looks off.', true);
+                        render(remaining);
+                    }
+                );
+            }, function () {
+                // The DELETE request itself failed — nothing changed on disk.
                 setStatus('Couldn’t delete that photo. Try again.', true);
                 render(lastGood);
             })