Sign up
View stargazersSign up

fix(screenshots): reject corrupt PNGs during validation#23169

merged2
main
GitHub fork: r13/a027-reject-corrupt-pngs-during-validation
opened20d agobyOonly21mil
2 files changed
View on GitHubTwo-way sync enabledPR last imported 10d ago

Problem

--validate-only can call a corrupt mobile screenshot "upload-ready". The validator checks dimensions and RGB format using fixed header offsets, but never decodes the image data. A PNG truncated inside IDAT or with a damaged checksum can pass.

On main, --validate-only accepts a PNG truncated to 44 bytes in both the iPhone 6.9-inch and Pixel capture sets. Complete PNGs should pass, and corrupt files should identify the failing asset and exit nonzero.

Change

Decode each asset with the existing PNG.sync.read before returning validation success. Report decoder errors with the asset label and preserve the underlying error as the cause. Keep the original dimension, bit-depth, color-type, alpha, file-size, aspect-ratio and screenshot-count checks. Validation does not rewrite the input.

The focused tests use generated RGB PNGs. They cover an intact file in an offset byte view, metadata-only files, truncation inside IDAT and a damaged IDAT checksum. They exercise real external-format parsing without timers, mocks or devices.

Scope and approval

This fixes an obvious false validation success in existing screenshot tooling, under the contribution guide's exception for a very small, focused bug fix. No prior issue or discussion approval is claimed. Only the validator and its existing test file change.

Validation-only returns before app, server, Metro, simulator or emulator setup. No product UI or screenshot pixels change. The CLI evidence uses generated RGB files at 1320×2868 and 1080×1920, not app captures. No production app build is needed for this path.

Prior PRs: #4014, by @juliusmarminge, introduced the harness and validator. No earlier PR attempting this integrity fix was found. #14254, by @Bil0000, is separate Pen-export work and is not reused here; its maintainer deferral concerns that feature.

Verification

Linux, Node 24.22.0:

  • vp test run scripts/mobile-showcase.test.ts --no-cache: against main's mobile-showcase.ts, the three corruption tests fail and the other four pass. All seven pass with this change.
  • node scripts/mobile-showcase.ts --validate-only --device iphone-6.9 --appearance dark --theme t3-code, repeated with --device pixel: base accepts intact, truncated and restored files with exit 0. Candidate accepts intact and restored files, but names the truncated asset and exits 1. Each capture set contains six generated screenshots.
  • tsc-rs --noEmit -p scripts/tsconfig.json: passes.
  • vp lint scripts/mobile-showcase.ts scripts/mobile-showcase.test.ts: passes, including main's current test lint rules.
  • vp fmt --check scripts/mobile-showcase.ts scripts/mobile-showcase.test.ts: passes. git diff --check and the merge check against green main also pass.

The new corruption tests fail on base, then all seven tests pass on the candidate.

PNG integrity regression test output

Base accepts truncated files. Candidate rejects them for both capture sets and passes again after restoration.

Validation-only CLI before and after

Not checked: native screenshot capture, App Store or Google Play uploads, production app builds, and the full repository suite. This command path neither captures nor uploads screenshots.

Generated with GPT-6.1 Sol in T3 Code Nightly

Checks

23 successful · 11 skipped
All checks passed
7c7d32a

Added 2 commits

  • test(screenshots): cover PNG image-data integrity7c7d32a
  • fix(screenshots): reject corrupt PNGs during validatione5ae7f3
CcoderabbitaiBot20d ago
Synced from GitHub

Warning

Review limit reached

Only developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing.

Next included review available in 14 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 4970288d-c063-45f1-bcca-4f64e240c03d
📥 Commits

Reviewing files that changed from the base of the PR and between b5a226e49c52ecab4ddc2d678a7012d97e651a6a and 7c7d32ae02ce301a22c8921bee6caed7a86212e4.

📒 Files selected for processing: 2
  • scripts/mobile-showcase.test.ts
  • scripts/mobile-showcase.ts
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

MmacroscopeappBot20d ago
Synced from GitHub

Approvability

Verdict: Approved at 7c7d32a

Macroscope's review found this PR approvable — This is a small, self-contained fix in host-side screenshot tooling: validation now decodes PNG data and rejects truncated or corrupt assets while preserving valid asset handling. The accompanying regression tests cover the new validation cases without affecting product runtime.

You can add or adjust custom eligibility rules. Learn more.

Mmaria12d ago

Note

Written by gpt-6.1-sol on behalf of Maria

Verified in a real dev environment

Verified unchanged head 7c7d32ae02ce301a22c8921bee6caed7a86212e4 on this Linux Boat VM. No code changes or pushes were needed.

  • Installed and ran github/main (32ecd71726) in /home/user/main, then stopped it before running the exact PR head with isolated .t3 state. Both used real vp run dev servers.
  • Used repository Playwright with /usr/bin/google-chrome to add a real git project, create threads, and submit real Codex gpt-6.1-sol turns through the web composer and local provider proxy. The response rendered in both clients. The PR provider log confirms turn/start used gpt-6.1-sol and completed successfully.
  • Exercised the changed user entry point: node scripts/mobile-showcase.ts --validate-only --device <iphone-6.9|pixel> --appearance dark --theme t3-code. This is host-side screenshot tooling, not chat attachment validation; it returns before server, Metro, or device setup.
  • Used an actual web-client screenshot after the main Codex turn, encoded as RGB at 1320×2868 for the iPhone slot and resized to 1080×1920 for the Pixel slot. Six copies populated each expected capture set. These are browser captures, not native screenshots or generated demo UI.
  • Main incorrectly exited 0 and called both a 44-byte truncated PNG and a PNG with a damaged IDAT checksum "upload-ready", for both device slots. The PR exits 1 for both corrupt variants and names the failing asset (iphone-6.9/thread.png or pixel/thread.png).
  • Intact and restored files pass on both revisions. Wrong dimensions, alpha, and missing scenes still fail with their existing diagnostics. SHA-256 comparison confirms validation did not change any of the 12 intact files.
  • Ran vp test run scripts/mobile-showcase.test.ts through blacksmith-check: all 7 tests passed. Existing head CI reports 27 successful checks, 0 pending, 0 failing.

The transcript captures the before/after CLI results. The short video shows the real PR web composer and Codex response. Captures were inspected; the video was trimmed to remove startup frames.

main accepts corrupt PNGs; PR rejects them while preserving neighboring validation

real Codex gpt-6.1-sol turn through the PR web client

Not verified

Native iOS/Android capture, simulator/emulator setup, Electron, macOS/Windows, App Store/Google Play uploads, and other providers were not exercised. The changed validation-only path runs on Linux without these services; browser captures prove PNG validation, not native capture or store acceptance. No UI changes are part of this PR. The full repository suite was not run.