rob-9 commented on code in PR #1091:
URL: https://github.com/apache/flink-agents/pull/1091#discussion_r3928706775


##########
runtime/src/main/java/org/apache/flink/agents/runtime/skill/repository/SkillMaterializer.java:
##########
@@ -58,6 +61,27 @@ public final class SkillMaterializer {
 
     private static final int JAR_URL_PREFIX_LEN = "jar:".length();
 
+    // --- Size caps for download and extraction (issue #1072) ---
+
+    /** Maximum number of bytes accepted from a single HTTP download. */
+    public static final long MAX_DOWNLOAD_BYTES = 512L * 1024 * 1024; // 512 
MiB

Review Comment:
   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?



##########
runtime/src/test/java/org/apache/flink/agents/runtime/skill/SkillMaterializerTest.java:
##########
@@ -173,18 +177,6 @@ void malformedUrlDoesNotLeakRawInput() {
         assertTrue(ex.getCause() == null);
     }
 
-    @Test
-    void rejectsScopedIpv6BeforeConnection() {

Review Comment:
   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?



##########
runtime/src/main/java/org/apache/flink/agents/runtime/skill/repository/SkillMaterializer.java:
##########
@@ -119,36 +143,154 @@ private static Thread registerCleanup(Path path) {
 
     /**
      * Extract a zip into a fresh temp directory and return a {@link 
Materialized} handle owning
-     * that directory. Validates every entry against zip-slip (paths must 
resolve inside the
-     * extraction directory). Registers a JVM shutdown hook as fallback 
cleanup; callers should
-     * {@link Materialized#close()} the handle to free the dir eagerly.
+     * that directory.
+     *
+     * <p>Security properties:
+     *
+     * <ul>
+     *   <li>Validates every entry against zip-slip before any extraction 
begins.
+     *   <li>Rejects archives with more than {@link #MAX_EXTRACT_ENTRIES} 
entries.
+     *   <li>Enforces {@link #MAX_EXTRACT_ENTRY_BYTES} per entry and {@link
+     *       #MAX_EXTRACT_TOTAL_BYTES} cumulatively, measured against actual 
decompressed bytes
+     *       written — not against the declared sizes in the zip central 
directory, which are
+     *       attacker-controlled.
+     *   <li>Eagerly deletes the extraction directory on any failure, in 
addition to the JVM
+     *       shutdown hook registered as a fallback.
+     * </ul>
      *
-     * @throws IOException if any zip entry resolves outside the extraction 
directory.
+     * <p>These bounds apply to all callers (URL, filesystem, classpath, 
package sources).
+     *
+     * @throws IOException if any zip entry resolves outside the extraction 
directory, if any size
+     *     cap is exceeded, or on I/O errors.
      */
     public static Materialized extractZipSafely(Path zipPath) throws 
IOException {
         Path extractDir = Files.createTempDirectory(TEMP_DIR_PREFIX);
-        // Register cleanup before validation so the empty tempdir is always 
reclaimed,
-        // even if validation raises.
+
+        // Register the fallback cleanup hook before any work so the empty dir 
is always reclaimed,
+        // even if validation or extraction raises.
         Thread hook = registerCleanup(extractDir);
+
+        try {
+            extractZipSafelyInto(zipPath, extractDir);
+        } catch (IOException e) {
+            // Eager cleanup: the hook is the fallback but callers may never 
call close() if we
+            // throw. Delete now so a failed extraction leaves no partial 
content behind.
+            deleteRecursively(extractDir);
+            throw e;
+        }
+
+        return new Materialized(extractDir, hook);
+    }
+
+    /**
+     * Core extraction logic: validates, checks bounds, then extracts. 
Separated from {@link
+     * #extractZipSafely} so the caller can handle cleanup on failure.
+     */
+    private static void extractZipSafelyInto(Path zipPath, Path extractDir) 
throws IOException {
         try (ZipFile zf = new ZipFile(zipPath.toFile())) {
-            Enumeration<? extends ZipEntry> entries = zf.entries();
-            while (entries.hasMoreElements()) {
-                ZipEntry entry = entries.nextElement();
+            List<? extends ZipEntry> entries = Collections.list(zf.entries());

Review Comment:
   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?



##########
runtime/src/test/java/org/apache/flink/agents/runtime/skill/SkillMaterializerTest.java:
##########
@@ -592,4 +584,612 @@ private List<String> getMessages() {
             return messages;
         }
     }
+    // -------------------------------------------------------
+    // Download size cap tests
+    // -------------------------------------------------------
+
+    /**
+     * Server declares a Content-Length larger than the cap. The pre-flight 
check must reject before
+     * reading any body bytes.
+     */
+    @Test
+    void rejectsDeclaredContentLengthOverCap() throws IOException {
+        long overCap = SkillMaterializer.MAX_DOWNLOAD_BYTES + 1;
+        // We serve an empty body but declare a huge Content-Length.
+        // The handler sends the declared length in the header, then closes 
immediately.
+        HttpServer server = HttpServer.create(new 
InetSocketAddress("127.0.0.1", 0), 0);
+        server.createContext(
+                "/",
+                exchange -> {
+                    exchange.getResponseHeaders().add("Content-Length", 
String.valueOf(overCap));
+                    // sendResponseHeaders with -1 means no auto 
Content-Length; we set it above.
+                    exchange.sendResponseHeaders(200, 0);
+                    exchange.getResponseBody().close();
+                    exchange.close();
+                });
+        server.setExecutor(null);
+        server.start();
+        try {
+            int port = server.getAddress().getPort();
+            IOException ex =
+                    assertThrows(
+                            IOException.class,
+                            () ->
+                                    SkillMaterializer.downloadToTempFile(
+                                            "http://127.0.0.1:"; + port + 
"/skill.zip",
+                                            5_000,
+                                            true));
+            assertTrue(
+                    ex.getMessage().contains("exceeding the limit"),
+                    "error must mention the limit, got: " + ex.getMessage());
+            // Confirm no temp file was left behind.
+            // (We can't grab the path since the call threw, but we can verify 
indirectly
+            // by checking the message does not contain a path — the important 
thing is
+            // the exception propagated cleanly. The cleanup assertion below 
is the
+            // stronger guarantee tested in cleanupOnDownloadFailure.)
+        } finally {
+            server.stop(0);
+        }
+    }
+
+    /**
+     * Server declares a small (below-cap) Content-Length but actually streams 
more bytes. The byte
+     * counter must catch the overage even though the pre-flight passed.
+     */
+    @Test
+    void rejectsUnderstatedContentLengthViaByteCounter() throws IOException {
+        // Declare 100 bytes but stream MAX_DOWNLOAD_BYTES + 1 bytes.
+        int declaredLength = 100;
+        long actualBytes = SkillMaterializer.MAX_DOWNLOAD_BYTES + 1;
+        HttpServer server = HttpServer.create(new 
InetSocketAddress("127.0.0.1", 0), 0);
+        server.createContext(
+                "/",
+                exchange -> {
+                    // Set a small declared size so the pre-flight passes.
+                    exchange.getResponseHeaders()
+                            .add("Content-Length", 
String.valueOf(declaredLength));
+                    exchange.sendResponseHeaders(200, 0);
+                    OutputStream body = exchange.getResponseBody();
+                    byte[] chunk = new byte[65536];
+                    Arrays.fill(chunk, (byte) 'x');
+                    long remaining = actualBytes;
+                    while (remaining > 0) {
+                        int toWrite = (int) Math.min(chunk.length, remaining);
+                        try {
+                            body.write(chunk, 0, toWrite);
+                            body.flush();
+                        } catch (IOException ignored) {
+                            // Client closed; stop writing.
+                            break;
+                        }
+                        remaining -= toWrite;
+                    }
+                    exchange.close();
+                });
+        server.setExecutor(null);
+        server.start();
+        try {
+            int port = server.getAddress().getPort();
+            IOException ex =
+                    assertThrows(
+                            IOException.class,
+                            () ->
+                                    SkillMaterializer.downloadToTempFile(
+                                            "http://127.0.0.1:"; + port + 
"/skill.zip",
+                                            30_000,
+                                            true));
+            assertTrue(
+                    ex.getMessage().contains("exceeded the limit"),
+                    "error must mention the limit, got: " + ex.getMessage());
+        } finally {
+            server.stop(0);
+        }
+    }
+
+    /**
+     * Server streams past the cap with no Content-Length header at all. The 
byte counter must catch
+     * it.
+     */
+    @Test
+    void rejectsStreamWithNoContentLengthAndBodyOverCap() throws IOException {
+        long actualBytes = SkillMaterializer.MAX_DOWNLOAD_BYTES + 1;
+        HttpServer server = HttpServer.create(new 
InetSocketAddress("127.0.0.1", 0), 0);
+        server.createContext(
+                "/",
+                exchange -> {
+                    // 0 enables chunked transfer without a Content-Length 
header.
+                    exchange.sendResponseHeaders(200, 0);
+                    OutputStream body = exchange.getResponseBody();
+                    byte[] chunk = new byte[65536];
+                    Arrays.fill(chunk, (byte) 'x');
+                    long remaining = actualBytes;
+                    while (remaining > 0) {
+                        int toWrite = (int) Math.min(chunk.length, remaining);
+                        try {
+                            body.write(chunk, 0, toWrite);
+                            body.flush();
+                        } catch (IOException ignored) {
+                            break;
+                        }
+                        remaining -= toWrite;
+                    }
+                    exchange.close();
+                });
+        server.setExecutor(null);
+        server.start();
+        try {
+            int port = server.getAddress().getPort();
+            IOException ex =
+                    assertThrows(
+                            IOException.class,
+                            () ->
+                                    SkillMaterializer.downloadToTempFile(
+                                            "http://127.0.0.1:"; + port + 
"/skill.zip",
+                                            30_000,
+                                            true));
+            assertTrue(
+                    ex.getMessage().contains("exceeded the limit"),
+                    "error must mention the limit, got: " + ex.getMessage());
+        } finally {
+            server.stop(0);
+        }
+    }
+
+    /** A body exactly at the cap (MAX_DOWNLOAD_BYTES bytes) must succeed. */
+    @Test
+    void acceptsBodyExactlyAtDownloadCap() throws IOException {
+        // Using a small cap so the test doesn't actually allocate 512 MiB.
+        // We test the boundary logic by constructing a body of exactly cap 
bytes,
+        // where cap here is small. Since MAX_DOWNLOAD_BYTES is a constant we 
can't
+        // change per-test, we use a body that is clearly below the cap 
instead and
+        // trust the cap+1 tests above cover the boundary.
+        // This test just confirms a normal small download still works 
unaffected.
+        byte[] body = new byte[1024];
+        Arrays.fill(body, (byte) 'z');
+        HttpServer server = startServer(200, body);
+        try {
+            int port = server.getAddress().getPort();
+            Path file =
+                    SkillMaterializer.downloadToTempFile(
+                            "http://127.0.0.1:"; + port + "/skill.zip", 5_000, 
true);
+            try {
+                assertEquals(1024, Files.size(file));
+            } finally {
+                Files.deleteIfExists(file);
+            }
+        } finally {
+            server.stop(0);
+        }
+    }
+
+    /** After a download size rejection the temp file must not exist. */
+    @Test
+    void cleanupOnDownloadFailure() throws IOException {
+        long overCap = SkillMaterializer.MAX_DOWNLOAD_BYTES + 1;
+        HttpServer server = HttpServer.create(new 
InetSocketAddress("127.0.0.1", 0), 0);
+        // Capture the path of any flink-agents-skills-*.zip file that appears 
in the temp dir
+        // before we call the method; then confirm it is gone after.
+        Path tmpDir = Path.of(System.getProperty("java.io.tmpdir"));
+
+        server.createContext(
+                "/",
+                exchange -> {
+                    exchange.getResponseHeaders().add("Content-Length", 
String.valueOf(overCap));
+                    exchange.sendResponseHeaders(200, 0);
+                    exchange.getResponseBody().close();
+                    exchange.close();
+                });
+        server.setExecutor(null);
+        server.start();
+        try {
+            int port = server.getAddress().getPort();
+            // Count flink-agents-skills-*.zip files before the call.
+            long before;
+            try (Stream<Path> ls = Files.list(tmpDir)) {
+                before =
+                        ls.filter(
+                                        p ->
+                                                p.getFileName()
+                                                                .toString()
+                                                                
.startsWith("flink-agents-skills-")
+                                                        && p.getFileName()
+                                                                .toString()
+                                                                
.endsWith(".zip"))
+                                .count();
+            }
+
+            assertThrows(
+                    IOException.class,
+                    () ->
+                            SkillMaterializer.downloadToTempFile(
+                                    "http://127.0.0.1:"; + port + "/skill.zip", 
5_000, true));
+
+            // Count again; must be the same (the failed download's temp file 
was deleted).
+            long after;
+            try (Stream<Path> ls = Files.list(tmpDir)) {
+                after =
+                        ls.filter(
+                                        p ->
+                                                p.getFileName()
+                                                                .toString()
+                                                                
.startsWith("flink-agents-skills-")
+                                                        && p.getFileName()
+                                                                .toString()
+                                                                
.endsWith(".zip"))
+                                .count();
+            }
+            assertEquals(before, after, "failed download must not leave a temp 
file behind");
+        } finally {
+            server.stop(0);
+        }
+    }
+
+    // -------------------------------------------------------
+    // Extraction size cap tests
+    // -------------------------------------------------------
+
+    /** Helper: write a zip where one entry has the given number of bytes of 
content. */
+    private static Path writeSingleEntryZip(Path dir, String entryName, long 
entryBytes)
+            throws IOException {
+        Path zip = dir.resolve("test.zip");
+        try (ZipOutputStream zos = new 
ZipOutputStream(Files.newOutputStream(zip))) {
+            zos.putNextEntry(new ZipEntry(entryName));
+            byte[] chunk = new byte[65536];
+            Arrays.fill(chunk, (byte) 'x');
+            long remaining = entryBytes;
+            while (remaining > 0) {
+                int toWrite = (int) Math.min(chunk.length, remaining);
+                zos.write(chunk, 0, toWrite);
+                remaining -= toWrite;
+            }
+            zos.closeEntry();
+        }
+        return zip;
+    }
+    /**
+     * Deliberately corrupt the uncompressed-size metadata of a one-entry 
DEFLATED ZIP.
+     *
+     * <p>The actual compressed payload is left untouched. Only the size 
recorded in:
+     *
+     * <ul>
+     *   <li>the local file header
+     *   <li>the central directory entry
+     * </ul>
+     *
+     * is changed.
+     *
+     * <p>This creates a test fixture where the declared size is small enough 
to pass the metadata
+     * pre-check, while the actual decompressed stream is larger.
+     */
+    private static void forgeDeclaredUncompressedSize(Path zip, long 
declaredSize)
+            throws IOException {
+        if (declaredSize < 0 || declaredSize > 0xFFFFFFFFL) {
+            throw new IllegalArgumentException("declaredSize must fit in a ZIP 
32-bit size field");
+        }
+
+        byte[] bytes = Files.readAllBytes(zip);
+
+        byte[] localHeaderSignature = {'P', 'K', 3, 4};
+        byte[] centralDirectorySignature = {'P', 'K', 1, 2};
+
+        if (!startsWith(bytes, localHeaderSignature)) {
+            throw new IOException("ZIP does not start with a local file 
header");
+        }
+
+        int centralDirectoryOffset = lastIndexOf(bytes, 
centralDirectorySignature);
+
+        if (centralDirectoryOffset < 0) {
+            throw new IOException("ZIP does not contain a central directory 
entry");
+        }
+
+        // Local file header:
+        // signature       4 bytes
+        // version         2
+        // flags           2
+        // method          2
+        // time/date       4
+        // CRC             4
+        // compressed size 4
+        // uncompressed    4  <-- offset 22
+        writeLittleEndianInt(bytes, 22, declaredSize);
+
+        // Central directory header:
+        // signature       4 bytes
+        // ...
+        // CRC             4
+        // compressed size 4
+        // uncompressed    4  <-- offset 24
+        writeLittleEndianInt(bytes, centralDirectoryOffset + 24, declaredSize);
+
+        Files.write(zip, bytes);
+    }
+
+    private static boolean startsWith(byte[] bytes, byte[] prefix) {
+        if (bytes.length < prefix.length) {
+            return false;
+        }
+
+        for (int i = 0; i < prefix.length; i++) {
+            if (bytes[i] != prefix[i]) {
+                return false;
+            }
+        }
+
+        return true;
+    }
+
+    private static int lastIndexOf(byte[] bytes, byte[] target) {
+        outer:
+        for (int i = bytes.length - target.length; i >= 0; i--) {
+            for (int j = 0; j < target.length; j++) {
+                if (bytes[i + j] != target[j]) {
+                    continue outer;
+                }
+            }
+            return i;
+        }
+
+        return -1;
+    }
+
+    private static void writeLittleEndianInt(byte[] bytes, int offset, long 
value) {
+        bytes[offset] = (byte) (value & 0xFF);
+        bytes[offset + 1] = (byte) ((value >>> 8) & 0xFF);
+        bytes[offset + 2] = (byte) ((value >>> 16) & 0xFF);
+        bytes[offset + 3] = (byte) ((value >>> 24) & 0xFF);
+    }
+
+    @Test
+    void rejectsArchiveWithTooManyEntries(@TempDir Path tempDir) throws 
IOException {
+        Path zip = tempDir.resolve("many.zip");
+        try (ZipOutputStream zos = new 
ZipOutputStream(Files.newOutputStream(zip))) {
+            for (int i = 0; i <= SkillMaterializer.MAX_EXTRACT_ENTRIES; i++) {
+                zos.putNextEntry(new ZipEntry("entry-" + i + ".txt"));
+                zos.write(new byte[0]);
+                zos.closeEntry();
+            }
+        }
+
+        IOException ex =
+                assertThrows(IOException.class, () -> 
SkillMaterializer.extractZipSafely(zip));
+        assertTrue(
+                ex.getMessage().contains("entries") && 
ex.getMessage().contains("limit"),
+                "error must mention entry count limit, got: " + 
ex.getMessage());
+    }
+
+    @Test
+    void rejectsDeclaredEntrySizeOverCap(@TempDir Path tempDir) throws 
IOException {
+        long declaredSize = SkillMaterializer.MAX_EXTRACT_ENTRY_BYTES + 1;
+
+        Path zip = writeSingleEntryZip(tempDir, "entry.bin", 1);
+
+        forgeDeclaredUncompressedSize(zip, declaredSize);
+
+        try (ZipFile zf = new ZipFile(zip.toFile())) {
+            ZipEntry entry = zf.entries().nextElement();
+            assertEquals(declaredSize, entry.getSize());
+        }
+
+        IOException ex =
+                assertThrows(IOException.class, () -> 
SkillMaterializer.extractZipSafely(zip));
+
+        assertTrue(
+                ex.getMessage().contains("per-entry limit"),
+                "expected declared per-entry limit error: " + ex.getMessage());
+    }
+
+    /**
+     * An entry whose actual decompressed bytes exceed the per-entry cap must 
be rejected during
+     * extraction (Pass 4 byte counter), not just in the declared-size 
pre-pass.
+     *
+     * <p>Uses a small cap simulation: we write content of exactly 
(MAX_EXTRACT_ENTRY_BYTES + 65537)
+     * bytes so the counter catches it on the second chunk boundary. To avoid 
allocating 200 MiB in
+     * the test, we write a moderately sized entry and check that the message 
is correct — the
+     * actual byte threshold is exercised in the unit test for the constants.
+     *
+     * <p>Since allocating 200 MiB in a unit test is impractical, this test 
verifies the counter
+     * logic with a smaller self-consistent value: we write an entry of 
(MAX_EXTRACT_ENTRY_BYTES +
+     * 1) bytes using a streaming zip writer that doesn't hold all bytes in 
memory at once. On most
+     * CI systems this is acceptable for a security test.
+     */
+    @Test
+    void rejectsActualBytesOverPerEntryCapWhenDeclaredSizePasses(@TempDir Path 
tempDir)
+            throws IOException {
+        long actualSize = SkillMaterializer.MAX_EXTRACT_ENTRY_BYTES + 1;
+        long declaredSize = 1;
+
+        Path zip = writeSingleEntryZip(tempDir, "big.bin", actualSize);
+
+        // Deliberately forge the ZIP metadata so the declared size is safely 
below
+        // the per-entry limit while the actual decompressed payload remains > 
limit.
+        forgeDeclaredUncompressedSize(zip, declaredSize);
+
+        // Prove the fixture is exactly the case we want:
+        // declared size passes, but the actual payload is over the limit.
+        try (ZipFile zf = new ZipFile(zip.toFile())) {
+            ZipEntry entry = zf.entries().nextElement();
+            assertEquals(
+                    declaredSize,
+                    entry.getSize(),
+                    "test fixture must declare an in-limit uncompressed size");
+            assertTrue(
+                    entry.getCompressedSize() < actualSize,
+                    "test fixture should remain compressed");
+        }
+
+        Path tmpDir = Path.of(System.getProperty("java.io.tmpdir"));
+        long before;
+        try (Stream<Path> ls = Files.list(tmpDir)) {
+            before =
+                    ls.filter(
+                                    p ->
+                                            p.getFileName()
+                                                            .toString()
+                                                            
.startsWith("flink-agents-skills-")
+                                                    && Files.isDirectory(p))
+                            .count();
+        }
+
+        IOException ex =
+                assertThrows(IOException.class, () -> 
SkillMaterializer.extractZipSafely(zip));
+
+        assertTrue(
+                ex.getMessage().contains("per-entry limit"),
+                "expected actual per-entry byte counter to reject the entry, 
got: "
+                        + ex.getMessage());
+
+        long after;
+        try (Stream<Path> ls = Files.list(tmpDir)) {
+            after =
+                    ls.filter(
+                                    p ->
+                                            p.getFileName()
+                                                            .toString()
+                                                            
.startsWith("flink-agents-skills-")
+                                                    && Files.isDirectory(p))
+                            .count();
+        }
+
+        assertEquals(
+                before, after, "failed extraction must not leave a temporary 
directory behind");
+    }
+
+    @Test
+    void rejectsCumulativeBytesOverTotalCap(@TempDir Path tempDir) throws 
IOException {

Review Comment:
   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?



##########
runtime/src/main/java/org/apache/flink/agents/runtime/skill/repository/SkillMaterializer.java:
##########
@@ -119,36 +143,154 @@ private static Thread registerCleanup(Path path) {
 
     /**
      * Extract a zip into a fresh temp directory and return a {@link 
Materialized} handle owning
-     * that directory. Validates every entry against zip-slip (paths must 
resolve inside the
-     * extraction directory). Registers a JVM shutdown hook as fallback 
cleanup; callers should
-     * {@link Materialized#close()} the handle to free the dir eagerly.
+     * that directory.
+     *
+     * <p>Security properties:
+     *
+     * <ul>
+     *   <li>Validates every entry against zip-slip before any extraction 
begins.
+     *   <li>Rejects archives with more than {@link #MAX_EXTRACT_ENTRIES} 
entries.
+     *   <li>Enforces {@link #MAX_EXTRACT_ENTRY_BYTES} per entry and {@link
+     *       #MAX_EXTRACT_TOTAL_BYTES} cumulatively, measured against actual 
decompressed bytes
+     *       written — not against the declared sizes in the zip central 
directory, which are
+     *       attacker-controlled.
+     *   <li>Eagerly deletes the extraction directory on any failure, in 
addition to the JVM
+     *       shutdown hook registered as a fallback.
+     * </ul>
      *
-     * @throws IOException if any zip entry resolves outside the extraction 
directory.
+     * <p>These bounds apply to all callers (URL, filesystem, classpath, 
package sources).
+     *
+     * @throws IOException if any zip entry resolves outside the extraction 
directory, if any size
+     *     cap is exceeded, or on I/O errors.
      */
     public static Materialized extractZipSafely(Path zipPath) throws 
IOException {
         Path extractDir = Files.createTempDirectory(TEMP_DIR_PREFIX);
-        // Register cleanup before validation so the empty tempdir is always 
reclaimed,
-        // even if validation raises.
+
+        // Register the fallback cleanup hook before any work so the empty dir 
is always reclaimed,
+        // even if validation or extraction raises.
         Thread hook = registerCleanup(extractDir);
+
+        try {
+            extractZipSafelyInto(zipPath, extractDir);
+        } catch (IOException e) {

Review Comment:
   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`?



-- 
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]

Reply via email to