[runtime][java][skills] Bound download and extraction size

[pranavshuklaa]

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:

• 512 MiB maximum downloaded archive size
• 200 MiB maximum size of a single extracted entry
• 1 GiB maximum total extracted size
• 10,000 maximum number of extracted entries

These values are proposed for discussion, we can adjust based on sense of realistic skill archive sizes.

Download protection

The download path now:

• Checks the declared Content-Length when available and rejects archives
  already declared larger than the download limit, before reading any body
  bytes.
• Independently tracks the number of bytes actually read from the response
  stream and rejects the download before writing bytes that would cross the
  limit.
• Treats the streamed byte count as authoritative, so the protection holds
  even when Content-Length is missing, zero, or understated by the server.
• Deletes the temporary file immediately if the download fails or is
  rejected, rather than relying solely on the JVM shutdown hook.

This closes the gap where a server could bypass a Content-Length-only
check 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:

• Rejects archives with more than the configured maximum number of entries,
  before any bytes are extracted.
• Checks declared per-entry and cumulative uncompressed sizes from the ZIP
  central directory as an early, cheap rejection where that metadata is
  present.
• Independently tracks actual decompressed bytes written per entry and
  cumulatively across the archive, rejecting extraction before writing bytes
  that would cross either limit.
• Preserves the existing zip-slip (path traversal) validation, run before
  any extraction begins.
• Deletes the extraction directory immediately on any failure, in addition
  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 be
sufficient.

Smoke test

SkillMaterializerSmokeTest is 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 SkillMaterializerTest covering:

• Download size limit via declared Content-Length
• Download size limit via actual streamed byte count, including understated
  and missing Content-Length
• Download failure cleanup (temp file removed on rejection)
• ZIP entry count limit
• Per-entry declared size limit (metadata pre-check)
• Per-entry actual decompressed size limit, using a fixture with forged ZIP
  metadata 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
• Cumulative extraction size limit
• Extraction failure cleanup (temp directory removed on rejection)
• Zip-slip rejection cleanup (confirms cleanup covers the pre-existing path
  traversal check too, not just the new size checks)
• Happy-path extraction unaffected by the new checks

SkillMaterializerSmokeTest covers the same core scenarios with readable
console output for manual verification.

Verified locally:

• SkillMaterializerTest — all tests passing
• SkillMaterializerSmokeTest — all tests passing

API

No

Documentation

• doc-needed
• doc-not-needed
• doc-included

Was this patch authored or co-authored using generative AI tooling?

• Yes
• No

Generated-by: Generated-by: ChatGPT (GPT-5.6 Luna)

[rob-9]

these limits currently only exist on Java side. #1072 requires aligned Java and Python enforcement, but Python still uses unbounded shutil.copyfileobj(...) and ZipFile.extractall(...). could you add the corresponding Python limits and regression tests so Skills.from_url(...) does not retain the original vulnerability?

[rob-9]

the path here deletes the directory but leaves hook registered, so every rejected archive retains a shutdown-hook thread until JVM exit.

it also only cataches IOException: for example, an entry containing NUL makes Path.resolve(...) throw InvalidPathException, bypassing this cleanup and leaving the temp directory until shutdown.

could we create the Materialized handle before extraction and call close() for both IOException and RuntimeException?

[rob-9]

the entry-count check at line 202 happens only after Collections.list(...) has enumerated and retained every entry.

memory consumption before rejection therefore remains proportional to the attacker-controlled entry count rather than the 10,000-entry limit. could we check zf.size() before constructing the list, or stop during bounded enumeration, so no more than the permitted entries are materialized?

[rob-9]

was this deletion intentional? this direct downloadToTempFile(...) regression test was added in 5eb91e73 following the scoped-IPv6 downloader issue raised in #1005.

the surviving tests cover validation through the API and URLSkillRepository, but none directly preserve this downloader contract. could we retain the test since its removal is unrelated to the resource-bound change?

[rob-9]

this fixture's declared uncompressed total already exceeds the limit, so it is rejected by the metadata precheck before extraction begins.

the assertion confirms that path by expecting total uncompressed size; the authoritative totalWritten guard instead reports total extracted size. could the fixture understate its declared sizes, or use injectable test limits, so the test actually exercises cumulative decompressed-byte enforcement?

[rob-9]

with code style check, I believe there's an issue with SkillMaterializerSmokeTest.java missing a header.