pranavshuklaa opened a new pull request, #1091:
URL: https://github.com/apache/flink-agents/pull/1091
<!--
* Thank you very much for contributing to Flink Agents.
* Please add the relevant components in the PR title. E.g., [api],
[runtime], [java], [python], [hotfix], etc.
-->
<!-- Please link the PR to the relevant issue(s). Hotfix doesn't need this.
-->
Linked issue: #1072
### Note on branch base
This branch is built on top of #1005 (commit 5eb91e73), 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
<!-- Do not remove this section. Check the proper box only. -->
- [ ] `doc-needed` <!-- Your PR changes impact docs -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-included` <!-- Your PR already contains the necessary
documentation updates -->
### Was this patch authored or co-authored using generative AI tooling?
<!-- Do not remove this section. Check the proper box only. -->
- [x] Yes
- [ ] No
`Generated-by: Generated-by: ChatGPT (GPT-5.6 Luna)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
