[runtime][java][skills] Bound download and extraction size - #1091
Draft
pranavshuklaa wants to merge 30 commits into
Draft
[runtime][java][skills] Bound download and extraction size#1091pranavshuklaa wants to merge 30 commits into
pranavshuklaa wants to merge 30 commits into
Conversation
Generated-by: Codex (GPT-5)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked issue: #1072
Note on branch base
This branch is built on top of #1005 (commit 5eb91e7), which is still open.
Until #1005 merges, this PR's diff will include #1005's commits in addition
to mine. My changes are isolated to the most recent commit on this branch
SkillMaterializer.java and its tests. I'll rebase onto main once #1005 lands,
at which point the diff will show only these changes.
Purpose of change
This change adds resource bounds for skill archives materialized from URL
sources.
A skill archive downloaded from a remote URL can otherwise consume an
unbounded amount of disk space during download and ZIP extraction, a
malicious or compromised source could exhaust disk space via an oversized
download or a zip bomb. This PR adds explicit limits around both stages and
ensures the limits are enforced using the actual data being processed, not
just metadata the source controls.
The proposed limits are:
These values are proposed for discussion, we can adjust based on sense of realistic skill archive sizes.
Download protection
The download path now:
Content-Lengthwhen available and rejects archivesalready declared larger than the download limit, before reading any body
bytes.
stream and rejects the download before writing bytes that would cross the
limit.
even when
Content-Lengthis missing, zero, or understated by the server.rejected, rather than relying solely on the JVM shutdown hook.
This closes the gap where a server could bypass a
Content-Length-onlycheck simply by omitting or lying about the header, declared size is only
ever used as a cheap early exit, never as the actual enforcement.
ZIP extraction protection
The extraction path applies layered validation:
before any bytes are extracted.
central directory as an early, cheap rejection where that metadata is
present.
cumulatively across the archive, rejecting extraction before writing bytes
that would cross either limit.
any extraction begins.
to the existing JVM shutdown hook fallback.
As with the download path, declared ZIP metadata is attacker-controlled and
is only used as an early exit ,the actual decompressed byte count during
extraction is the real enforcement. This is deliberate: a crafted archive
can declare an entry as 1 byte while its actual DEFLATE stream expands to
gigabytes (a zip bomb), so trusting
ZipEntry.getSize()alone would not besufficient.
Smoke test
SkillMaterializerSmokeTestis included alongside the unit test coverage.It exercises the same core scenarios as the unit tests but with verbose
console output, useful for manually verifying the enforcement and cleanup
behavior end-to-end against the real HTTP and ZIP code paths. Happy to
remove it before merge as you'd rather keep only the unit tests
flagging it now since it's additive rather than required.
Tests
Added unit tests in
SkillMaterializerTestcovering:Content-Lengthand missing
Content-Lengthmetadata so the declared size passes but the actual DEFLATE stream exceeds
the limit, proving the byte counter, not the metadata check, is doing the
enforcement
traversal check too, not just the new size checks)
SkillMaterializerSmokeTestcovers the same core scenarios with readableconsole output for manual verification.
Verified locally:
SkillMaterializerTest— all tests passingSkillMaterializerSmokeTest— all tests passingAPI
No
Documentation
doc-neededdoc-not-neededdoc-includedWas this patch authored or co-authored using generative AI tooling?
Generated-by: Generated-by: ChatGPT (GPT-5.6 Luna)