Skip to content

fix(review): bound fallback artifact inflation#4178

Merged
JSONbored merged 5 commits into
mainfrom
codex/fix-vulnerability-in-visual-capture-fallback
Jul 10, 2026
Merged

fix(review): bound fallback artifact inflation#4178
JSONbored merged 5 commits into
mainfrom
codex/fix-vulnerability-in-visual-capture-fallback

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • The fallback workflow artifact reader previously inflated method-8 (DEFLATE) ZIP entries into a full ArrayBuffer with no decompressed-size limit, allowing crafted small archives to inflate to very large buffers and cause worker memory exhaustion.
  • The backend only applied a compressed-artifact cap, so a highly-compressible (ZIP-bomb) PNG could bypass that protection and be accepted and stored as image/png.

Description

  • Enforce a per-entry decompressed size cap by threading a maxEntryBytes budget into parseZipEntries and by checking the central-directory's declared uncompressed size before inflating. (
    modified: src/review/visual/actions-fallback.ts)
  • Replace the one-shot Response(...).arrayBuffer() inflation with a streaming reader (stream.getReader()) in inflateRawRaw that aborts and returns null if the decompressed output exceeds the configured limit. (modified: src/review/visual/actions-fallback.ts)
  • Validate extracted .png entries by checking the PNG magic bytes and ignore entries whose decompressed bytes do not start with a PNG signature before accepting them as shots. (modified: src/review/visual/actions-fallback.ts)
  • Add regression tests and update fixtures to cover declared-oversized entries, streamed-DEFLATE overflow, and invalid-PNG payloads, plus update test ZIP fixtures to include PNG signatures. (modified: test/unit/actions-fallback.test.ts)

Testing

  • Ran npx vitest run test/unit/actions-fallback.test.ts and the focused suite passed (62 tests passed).
  • Ran npm run typecheck and the TypeScript check completed successfully.
  • Ran git diff --check with no issues reported.
  • Attempted coverage: npx vitest run --coverage --pool=forks test/unit/actions-fallback.test.ts executed the focused tests successfully but coverage report generation failed in this environment with TypeError: jsTokens is not a function; a full npm run test:coverage run did not complete in this environment due to unrelated long-running tests/timeouts.
  • npm audit --audit-level=moderate could not be completed here due to the registry audit endpoint returning HTTP 403 in this environment.

Codex Task

@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.46154% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.10%. Comparing base (efcd25d) to head (795def4).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/review/visual/actions-fallback.ts 88.46% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4178      +/-   ##
==========================================
- Coverage   94.11%   94.10%   -0.01%     
==========================================
  Files         432      432              
  Lines       38348    38369      +21     
  Branches    13979    13983       +4     
==========================================
+ Hits        36091    36109      +18     
- Misses       1600     1602       +2     
- Partials      657      658       +1     
Files with missing lines Coverage Δ
src/review/visual/actions-fallback.ts 98.25% <88.46%> (-1.75%) ⬇️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@gittensory-orb gittensory-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.5x multiplier. label Jul 8, 2026
@gittensory-orb

gittensory-orb Bot commented Jul 8, 2026

Copy link
Copy Markdown

Caution

🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥

🛑 Gittensory review result - fixes required

Review updated: 2026-07-10 08:40:39 UTC

3 files · 1 AI reviewer · 2 blockers · readiness 93/100 · CI failing · unstable

🛑 Suggested Action - Manual Review

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.

Review summary
This PR closes a real ZIP-bomb path in the fallback artifact reader: it threads a maxEntryBytes budget into parseZipEntries, checks the central-directory's declared uncompressedSize before inflating, replaces the one-shot arrayBuffer() inflate with a streaming reader that aborts once decompressed output exceeds the budget, and validates extracted .png entries against the real PNG magic bytes before accepting them as shots. The new tests exercise the real paths (declared-oversize entry via the actual uncompressedSize field offset, streamed-DEFLATE overflow via a genuinely compressible payload, and an invalid-PNG payload) rather than fabricating unreachable states, and the change is narrowly scoped to the vulnerable module plus its test file with no schema/migration surface.

Nits — 5 non-blocking
  • The PR description doesn't link or close an eligible open issue for this security fix — worth confirming against repo convention even though the change itself is sound.
  • src/review/visual/actions-fallback.ts: the central-directory uncompressedSize field offset (+24) and the PNG signature length (8) are magic numbers not backed by named constants, unlike the file's existing CENTRAL_DIR_HEADER_SIZE-style constants.
  • MAX_ARTIFACT_BYTES now implicitly serves double duty as both the overall-artifact cap and the default per-entry cap (via the `??` fallback in parseZipEntries) — a comment clarifying that dual role would help avoid someone changing one without realizing it affects the other.
  • The file is now ~423 lines, over the apparent ~400-line local convention — not blocking, but worth a look if more ZIP-parsing logic gets added later.
  • Add a comment or small test confirming fetchFallbackArtifactShots's call to parseZipEntries relies on the default maxEntryBytes (the diff doesn't show that call site passing an options object), so a future refactor can't silently drop the cap.

Why this is blocked

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.

CI checks failing

  • codecov/patch — 88.46% of diff hit (target 99.00%)
Signal Result Evidence
Code review ❌ 2 blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 48 registered-repo PR(s), 40 merged, 334 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 48 PR(s), 334 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 48 PR(s), 334 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@gittensory-orb gittensory-orb Bot added the manual-review Gittensor contributor context label Jul 8, 2026
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 8, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
gittensory-ui 795def4 Commit Preview URL

Branch Preview URL
Jul 10 2026, 08:26 AM

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@JSONbored JSONbored force-pushed the codex/fix-vulnerability-in-visual-capture-fallback branch from 47472c5 to 8c7e1d4 Compare July 9, 2026 08:47
JSONbored added 4 commits July 9, 2026 23:59
…gic bytes

This PR's new isPng() check (src/review/visual/actions-fallback.ts) correctly
rejects the plain-text fixture entries ("desktop-bytes" etc.) this sibling
test file uses -- its own local buildZip() fixture wasn't updated alongside
test/unit/actions-fallback.test.ts's pngBytes() helper. Adds the same helper
here and updates the one byte-exact assertion that decoded the stored object
as text to compare the raw bytes instead.
@JSONbored JSONbored merged commit 40b89ec into main Jul 10, 2026
11 of 12 checks passed
@JSONbored JSONbored deleted the codex/fix-vulnerability-in-visual-capture-fallback branch July 10, 2026 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aardvark codex gittensor:bug Gittensor-scored bug fix — scores a 0.5x multiplier. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant