Bound preview dimensions and reject dangling SQLite sidecars

Author: mchwang

docs/implementation/read-only-review.md

@@ -48,3 +48,5 @@ Review round 4 had no inline findings but raised two summary concerns. Both were
 Review round 5 found a symlinked-ancestor escape and raw-spelling collisions in planting guards. Both regression cases failed before the fix. Demo creation/reuse now checks every ancestor (canonicalizing only the OS temporary-directory prefix); planting compares canonical identities for declared files and every path in the base/commit trees before cloning. The summary's status-styling concern had no additional concrete example; accepted cards already use the explicit label added in round 4.
 
 Review round 6 reproduced destination-inside-source mutation; planting now resolves the existing destination ancestor and rejects canonical source descendants before creating files, including `.git` and symlink aliases. The Git-environment finding was declined: `gitRaw` already passes `{cwd, env, ...}` to `execFileSync`. A new end-to-end test with inherited `GIT_DIR`/`GIT_WORK_TREE` passed before any production change. The summary's snapshot-race claim supplied no concrete interleaving; the service already checks revision/snapshot/review version before publishing a view and writes use atomic store CAS, covered by concurrent-write tests.
+
+Review round 7 reproduced oversized-dimension raster previews and dangling SQLite sidecar symlinks. Preview metadata is now parsed from the bounded buffer with image-size: at most 8,192 pixels per side, 4 million pixels per image and 16 million pixels across unique preview blobs. Unknown dimensions omit the preview; existing compressed/response byte limits remain. Sidecars use lstat so dangling links are rejected. Both new regressions failed before the fixes. The summary mentioned rename-scope/literal-path concerns without specific findings; no additional behavior was inferred from that shorthand.

git/history.ts

@@ -1,3 +1,4 @@
+import { imageSize } from 'image-size';
 import { execFileSync } from 'node:child_process';
 import { lstatSync, opendirSync } from 'node:fs';
 import { resolve as resolvePath, join } from 'node:path';
@@ -90,7 +91,7 @@ export function readHistory(repo: string, baseRef: string, headRef = 'HEAD', lim
   });
   if (expectedParent !== head) throw new Error('The base must be an ancestor of the head.');
   const blobs = new Map<string, { text: string | null; byteSize: number; preview?: string }>();
-  let previewBytes = 0;
+  let previewBytes = 0, previewPixels = 0;
   const version = (oid: string, mode: string): FileVersion | null => {
     if (/^0+$/.test(oid)) return null;
     if (mode === '160000') return { oid, mode, text: null }; // gitlink is not a local blob
@@ -108,7 +109,14 @@ export function readHistory(repo: string, baseRef: string, headRef = 'HEAD', lim
         /^GIF8[79]a$/.test(data.subarray(0,6).toString('ascii')) ? 'image/gif' :
         data.subarray(0,4).toString('ascii') === 'RIFF' && data.subarray(8,12).toString('ascii') === 'WEBP' ? 'image/webp' : null;
       if (mime && data.length <= 1024 * 1024 && previewBytes + data.length <= 4 * 1024 * 1024) {
-        preview = `data:${mime};base64,${data.toString('base64')}`; previewBytes += data.length;
+        try {
+          // Parse metadata from the already bounded buffer; never allocate decoded pixels.
+          const { width, height } = imageSize(data), pixels = width * height;
+          if (Number.isSafeInteger(pixels) && width > 0 && height > 0 && width <= 8192 && height <= 8192 && pixels <= 4_000_000 && previewPixels + pixels <= 16_000_000) {
+            preview = `data:${mime};base64,${data.toString('base64')}`;
+            previewBytes += data.length; previewPixels += pixels;
+          }
+        } catch { /* Unknown or malformed dimensions: metadata card only. */ }
       }
       blobs.set(oid, { text, byteSize: data.length, ...(preview ? { preview } : {}) });
     }

package-lock.json

@@ -18,6 +18,7 @@
     "@fontsource/ibm-plex-sans": "5.3.0",
     "ajv": "8.20.0",
     "diff": "9.0.0",
+    "image-size": "2.0.4",
     "yaml": "2.9.1"
   },
   "devDependencies": {

package.json

@@ -30,7 +30,7 @@ export function createDemo(directory: string): ReviewConfig {
     const config = JSON.parse(readFileSync(configPath, 'utf8')) as ReviewConfig;
     if (config.demo !== true || config.repository !== join(root, 'retry-service') || config.database !== join(root, 'review.sqlite')) throw new Error('Demo configuration must refer to its own fixture.');
     check(config.repository, true); check(join(config.repository, '.git'), true); check(config.database, false);
-    for (const suffix of ['-wal', '-shm']) if (existsSync(config.database + suffix)) check(config.database + suffix, false);
+    for (const suffix of ['-wal', '-shm']) if (lstatSync(config.database + suffix, { throwIfNoEntry: false })) check(config.database + suffix, false);
     return config;
   }
   if (existsSync(root)) throw new Error('Demo directory exists without a configuration. Choose a new empty path.');

scripts/demo.ts

@@ -48,3 +48,10 @@ it('rejects symlinked ancestors before creating a demo', () => {
   const base = root(); symlinkSync(base, join(base, 'alias'));
   expect(() => createDemo(join(base, 'alias', 'nested', 'demo'))).toThrow(/fixture/i);
 });
+it('rejects dangling SQLite sidecar symlinks', () => {
+ const base=root(),directory=join(base,'demo');const config=createDemo(directory);
+ for(const suffix of ['-wal','-shm']) {
+   const path=config.database+suffix;rmSync(path,{force:true});symlinkSync(join(base,'missing'+suffix),path);
+   expect(()=>createDemo(directory)).toThrow(/fixture/i);rmSync(path);
+ }
+});

test/demo.test.ts

@@ -328,3 +328,10 @@ it('rejects shallow parent rewriting before loading history', () => {
   f.write('.git/shallow', `${head}\n`);
   expect(() => readHistory(f.dir, f.base, head)).toThrow(/shallow/i);
 });
+it('omits raster previews with oversized declared dimensions', () => {
+ const f=fixture(); const base=f.git('rev-parse','HEAD');
+ const image=Buffer.from('iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8/x8AAwMCAO+/lXsAAAAASUVORK5CYII=','base64');
+ image.writeUInt32BE(100000,16);image.writeUInt32BE(100000,20);
+ f.write('oversized.png',image);const head=f.commit();
+ expect(readHistory(f.dir,base,head).final.find(file=>file.newPath==='oversized.png')?.after?.preview).toBeUndefined();
+});