fix(import): cap GitHub zip expansion before inflate - #3679
Conversation
Reuse the skill-sync unzip filter so file count and originalSize limits apply before unzipSync inflates a 25MB archive. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
@SebTardif is attempting to deploy a commit to the OpenClaw Foundation Team on Vercel. A member of the Team first needs to authorize it. |
|
Codex review: blocked before merge. Reviewed September 11, 2026, 10:13 AM ET / 14:13 UTC. ClawSweeper reviewWhat this changesAdds ZIP-header limits before GitHub import decompression and a regression test for rejecting oversized files. Regression provenancePossible regression — suspected (reviewed change). No predecessor PR is attributed. Merge readiness⛔ Blocked before merge - 3 items remain The hardening remains necessary on main and in v0.23.3, but the patch introduces a compatibility regression for repositories containing oversized, unselected files. Priority: P2 Review scores
Verification
How this fits togetherClawHub imports selected skill files from a signed-in user's public GitHub repositories. Archive extraction feeds candidate discovery, file previews, and publishing. flowchart TD
A[Signed-in user] --> B[Owned public repository check]
B --> C[Download bounded GitHub archive]
C --> D[Check entry headers]
D --> E[Decompress accepted entries]
D --> F[Reject over-limit archive]
E --> G[Preview and select skill files]
G --> H[Publish skill]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Apply limits before decompression while preserving selective import's oversized-file skipping, and document that distinction from complete-content Skill Sync verification. Do we have a high-confidence way to reproduce the issue? Yes, from source: main inflates every ZIP entry before checking limits, and a valid skill alongside a compressible file over 10 MiB demonstrates the patch's skip-to-error regression. No reviewer-side runtime reproduction was executed. Is this the best way to solve the issue? Not as written: the pre-inflate filter is the right layer, but returning false for oversized entries provides the same allocation protection without breaking selective imports; post-inflate checks alone cannot prevent excessive allocation. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against cbfee7343ddc. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
What Problem This Solves
Authenticated Convex GitHub import (
previewGitHubImport,previewGitHubImportCandidate,importGitHubSkill) callsunzipSync(zipBytes)with no filter, then checks file count / single-file / total size after the archive has already been inflated.fetchGitHubZipBytesonly caps the compressed download at 25MB. A highly compressible 25MB GitHub zip can expand far past the 80MB unzipped budget inside the Convex action before those post-checks run.Sibling
convex/githubSkillSync.tsalready usescreateZipEntryLimitFiltersooriginalSizeis enforced before inflate. This change applies the same filter togithubImport.tsunzipToEntries.Evidence
Live
bunagainst productionunzipToEntrieswith a zip whose uncompressedoriginalSizeis 10MB+1:Same-repo:
githubSkillSyncunzip filter (createZipEntryLimitFilter). F006 #3612 is the same file's blob-store leak, not unzip bounds.Real behavior proof
C:\tmp\wt-ch-f012at786ce7d.zipSynccontainingmodel.binof length10 * 1024 * 1024 + 1and called__test.unzipToEntries.THREW ConvexError: Repo archive contains a file that is too large.Tracker
Ref #3678
That issue stays open if this PR is closed without landing on main.