fix: PNG/APNG CRC verification, GIF bounds check, adjustBrightness LUT hoisting, PAM docs#106
Conversation
Agent-Logs-Url: https://github.com/cross-org/image/sessions/776097d4-a68d-484d-9cb0-802fc49ff8bc Co-authored-by: Hexagon <419737+Hexagon@users.noreply.github.com>
|
@copilot have a look at the ci failure |
There was a problem hiding this comment.
Pull request overview
This PR addresses several low-severity TODO findings by improving image decoding correctness/robustness, reducing a small recurring allocation in image processing, and clarifying PAM decoder limitations.
Changes:
- Added CRC32 verification for PNG/APNG chunk parsing during decode and metadata extraction, and updated the existing security test PNG to include a correct IHDR CRC.
- Hardened GIF background color selection by replacing
|| 0fallbacks with an explicit palette-bounds check. - Hoisted
adjustBrightness()clamp LUT to module scope to avoid per-call reconstruction, and documented PAM decoder’s supported subset.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/utils/security.test.ts | Updates crafted PNG test bytes to include a correct IHDR CRC so the oversized-dimension check remains exercised. |
| src/utils/image_processing.ts | Hoists brightness clamp LUT to module scope and updates adjustBrightness() to use it. |
| src/utils/gif_decoder.ts | Adds explicit bounds checking for background palette index lookups. |
| src/formats/png.ts | Verifies CRC for each parsed chunk in decode() and extractMetadata(). |
| src/formats/apng.ts | Verifies CRC for each parsed chunk in APNG first-pass decode and extractMetadata(). |
| src/formats/pam.ts | Adds JSDoc describing the supported PAM subset and expected errors for unsupported variants. |
| TODO.md | Marks the addressed TODO items as fixed and summarizes the implemented fixes. |
| CHANGELOG.md | Documents the new CRC verification, GIF bounds check, and LUT hoisting behavior changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pos += 4; | ||
|
|
||
| // Verify CRC (covers chunk type + chunk data) | ||
| if (storedCrc !== this.crc32(data.slice(typePos, typePos + 4 + length))) { |
There was a problem hiding this comment.
CRC verification uses data.slice(...), which copies the chunk bytes for every chunk during metadata extraction. Prefer data.subarray(typePos, typePos + 4 + length) (with an upfront bounds check) to avoid extra allocations and reduce overhead.
| if (storedCrc !== this.crc32(data.slice(typePos, typePos + 4 + length))) { | |
| if ( | |
| storedCrc !== this.crc32(data.subarray(typePos, typePos + 4 + length)) | |
| ) { |
| // Use bitwise OR for fast rounding (equivalent to Math.round for positive numbers) | ||
| const adjustInt = (adjust + 0.5) | 0; |
There was a problem hiding this comment.
adjustInt is computed with (adjust + 0.5) | 0, which only matches Math.round for positive values. Since amount can be negative, this yields incorrect rounding for negative brightness adjustments (e.g., -10.2 rounds to -9), which slightly skews results. Use Math.round(adjust) (or a sign-aware bitwise rounding) to keep brightness adjustment symmetric for negative/positive amounts.
| // Use bitwise OR for fast rounding (equivalent to Math.round for positive numbers) | |
| const adjustInt = (adjust + 0.5) | 0; | |
| // Use symmetric rounding so negative and positive brightness adjustments behave consistently | |
| const adjustInt = Math.round(adjust); |
| const chunkData = data.slice(pos, pos + length); | ||
| pos += length; | ||
| pos += 4; // Skip CRC | ||
| const storedCrc = this.readUint32(data, pos); | ||
| pos += 4; |
There was a problem hiding this comment.
In PNGFormat.decode's chunk loop there is no bounds check that pos + length + 4 is within data.length before slicing chunk data and reading the stored CRC. Because readUint32 treats out-of-range reads as zero, a truncated chunk can be misclassified as a CRC failure (and can even be bypassed if the computed CRC happens to be 0), and chunkData can be shorter than the declared length. Add an explicit bounds check (similar to extractMetadata's if (pos + length + 4 > data.length) ...) and throw a clear truncation/invalid chunk error before CRC verification.
| pos += 4; | ||
|
|
||
| // Verify CRC (covers chunk type + chunk data) | ||
| if (storedCrc !== this.crc32(data.slice(typePos, typePos + 4 + length))) { |
There was a problem hiding this comment.
CRC verification currently computes this.crc32(data.slice(typePos, typePos + 4 + length)), which allocates a new Uint8Array for every chunk. Since crc32 only reads, prefer data.subarray(typePos, typePos + 4 + length) (after bounds validation) to avoid per-chunk copies during metadata extraction.
| if (storedCrc !== this.crc32(data.slice(typePos, typePos + 4 + length))) { | |
| if ( | |
| storedCrc !== this.crc32(data.subarray(typePos, typePos + 4 + length)) | |
| ) { |
| while (pos < data.length) { | ||
| const length = this.readUint32(data, pos); | ||
| pos += 4; | ||
| const typePos = pos; | ||
| const type = String.fromCharCode( |
There was a problem hiding this comment.
APNGFormat.decodeFrames first-pass chunk parsing does not validate that there are at least 8 bytes for length+type, nor that pos + length + 4 is within data.length before slicing chunk data and reading the stored CRC. With readUint32's out-of-range behavior, truncated/corrupt inputs can be mis-parsed and can produce misleading CRC errors (or be bypassed if CRC reads as 0). Add the same bounds checks used in extractMetadata before reading type/data/CRC, and throw a clear truncation/invalid chunk error.
Agent-Logs-Url: https://github.com/cross-org/image/sessions/3acd24c2-8377-47d2-b224-8ddfe866eabb Co-authored-by: Hexagon <419737+Hexagon@users.noreply.github.com>
|
@copilot apply changes based on the comments in this thread |
… Math.round Agent-Logs-Url: https://github.com/cross-org/image/sessions/556e4fe7-2181-4380-adf9-67252e62634b Co-authored-by: Hexagon <419737+Hexagon@users.noreply.github.com>
Agent-Logs-Url: https://github.com/cross-org/image/sessions/556e4fe7-2181-4380-adf9-67252e62634b Co-authored-by: Hexagon <419737+Hexagon@users.noreply.github.com>
Applied all 5 review comments in commits 54bf326 and e583259:
All 626 tests pass. |
adjustBrightnessclamp LUT to module-level constant (performance)|| 0with explicit bounds check (robustness)deno fmt --checksubarrayinstead ofslicefor CRC verification in png.ts and apng.tsMath.roundfor negative-safe rounding in adjustBrightness