[api] Make bundled Java compilation opt-in and restrict remote URL loading - #3875
Conversation
…ading Adds configuration controls around two loading behaviors, with the previous behavior available via opt-out: - ClassLoaderUtils.compileJavaClass: compiling bundled .java sources at model load is now opt-in via DJL_COMPILE_JAVA / -Dai.djl.compile_java. Models that ship precompiled .class/.jar files or set a translator programmatically are unaffected. - Utils.openUrl: remote fetches are limited to public http(s) destinations; redirects are resolved explicitly and each hop is checked against the same rule. Local-resource schemes (file:, jar:) are unrestricted. Other schemes are unsupported. Restore previous behavior with DJL_ALLOW_INSECURE_URL / -Dai.djl.allow_insecure_url. - SimpleUrlRepository: download() and the content-length HEAD probe now go through Utils.openUrl so both paths share the same URL handling. Adds UrlAccessAndCompilationTest covering the new defaults and both flags.
The JVM caches open jar files, so a handle can remain on the temp jar after the stream is closed. Files.deleteIfExists then fails on Windows with FileSystemException. Fall back to deleteOnExit for cleanup.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3875 +/- ##
=============================================
+ Coverage 60.41% 72.28% +11.86%
- Complexity 6235 7771 +1536
=============================================
Files 705 705
Lines 34984 35099 +115
Branches 3819 3852 +33
=============================================
+ Hits 21137 25372 +4235
+ Misses 12243 7969 -4274
- Partials 1604 1758 +154 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Compiling a bundled .java translator is now opt-in, so the test cases that load a translator from a classes folder (and from a jar built out of that compiled output) must set ai.djl.compile_java for the duration of those assertions.
AbstractRepository.download resolved the artifact URI from repository metadata and opened it directly. An absolute URI there bypasses base-URI resolution, so it was not subject to the URL handling applied elsewhere. Route it through Utils.openUrl so every download path shares the same rule. Adds RepositoryDownloadTest covering absolute item URIs.
The redirect handling in openUrl was not actually exercised: the existing test used a loopback first hop, which is rejected by the destination check before any connection is made, so the redirect loop never ran. Makes the redirect helper package-private with the host check as a parameter so it can be driven against a local test server, and adds cases for: a redirect being followed, a redirect target being re-checked and refused, a redirect to an unsupported scheme, a redirect with no Location header, exceeding the hop limit, the opt-out path, and unknown-host resolution. Also covers the SimpleUrlRepository download and content-length entry points. Drops a redundant assertion-only test.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Tightens security defaults around model loading by making bundled .java compilation opt-in and restricting remote URL loading to public http(s) destinations, while retaining prior behavior behind explicit flags.
Changes:
- Make bundled
.javacompilation at model-load time opt-in viaDJL_COMPILE_JAVA/ai.djl.compile_java. - Restrict
Utils.openUrlto public http(s) with explicit, bounded redirect resolution; allow localfile:/jar:schemes; add opt-outDJL_ALLOW_INSECURE_URL. - Route repository download paths through shared URL handling and add tests covering the new behaviors and flags.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| integration/src/main/java/ai/djl/integration/tests/model_zoo/CustomTranslatorTest.java | Updates integration test to enable new opt-in compilation flag for bundled .java translator scenarios. |
| api/src/test/java/ai/djl/util/UrlAccessAndCompilationTest.java | Adds unit tests for URL restrictions, redirect validation, and dynamic compilation opt-in. |
| api/src/test/java/ai/djl/repository/RepositoryDownloadTest.java | Adds tests ensuring repository download paths reject non-public/unsupported artifact URIs. |
| api/src/main/java/ai/djl/util/Utils.java | Implements secure URL opening with public-host enforcement and explicit redirect handling; adds opt-out flag and host validation helper. |
| api/src/main/java/ai/djl/util/ClassLoaderUtils.java | Disables bundled source compilation by default; adds opt-in flag method and logging. |
| api/src/main/java/ai/djl/repository/SimpleUrlRepository.java | Routes downloads via Utils.openUrl; tightens HEAD probe behavior for content-length checks. |
| api/src/main/java/ai/djl/repository/AbstractRepository.java | Routes artifact downloads via Utils.openUrl to enforce consistent URL handling. |
Suppressed comments (1)
integration/src/main/java/ai/djl/integration/tests/model_zoo/CustomTranslatorTest.java:1
- This test mutates a global JVM system property. If the integration test suite is ever run with parallel methods/classes enabled, this can cause cross-test interference. Consider isolating the property mutation using a shared lock (e.g., synchronize around the set/clear + assertions that depend on it) or configuring these tests to run single-threaded where the suite is defined.
/*
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| public static boolean isPublicHost(String host) { | ||
| if (host == null || host.isEmpty()) { | ||
| return false; | ||
| } | ||
| try { | ||
| InetAddress[] addresses = InetAddress.getAllByName(host); | ||
| if (addresses.length == 0) { | ||
| return false; | ||
| } | ||
| for (InetAddress addr : addresses) { | ||
| if (addr.isLoopbackAddress() | ||
| || addr.isLinkLocalAddress() | ||
| || addr.isSiteLocalAddress() | ||
| || addr.isAnyLocalAddress() | ||
| || addr.isMulticastAddress()) { | ||
| return false; | ||
| } | ||
| } | ||
| } catch (UnknownHostException e) { | ||
| return false; | ||
| } | ||
| return new BufferedInputStream(url.openStream()); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Good catch — confirmed and fixed in dcb78a6.
InetAddress#isSiteLocalAddress() only covers the deprecated fec0::/10, so ULA addresses were treated as public. Verified before/after:
fd00::1 siteLocal=false linkLocal=false loopback=false => ALLOWED (before)
fc00::1 siteLocal=false linkLocal=false loopback=false => ALLOWED (before)
fe80::1 linkLocal=true => blocked
::1 loopback=true => blocked
Added an explicit fc00::/7 check ((bytes[0] & 0xFE) == 0xFC on a 16-byte address) plus testIpv6UniqueLocalAddressIsNotPublic, which covers fd00::1, fc00::1, fdff:ffff::1, fe80::1, ::1, and asserts a public IPv6 address is still allowed. Confirmed the test fails with the new check removed.
| conn = (HttpURLConnection) uri.toURL().openConnection(); | ||
| if (!Utils.isInsecureUrlAllowed()) { | ||
| conn.setInstanceFollowRedirects(false); | ||
| } | ||
| conn.setRequestMethod("HEAD"); | ||
| int code = conn.getResponseCode(); |
There was a problem hiding this comment.
Agreed, fixed in dcb78a6. The HEAD probe now resolves redirects explicitly instead of just disabling them, so a public 3xx still yields a content length.
Refactored to a single shared path: Utils.openHttpConnection(url, method, headers) performs the bounded, per-hop-validated redirect resolution and returns the final connection. openUrl uses it with GET; getContentLength() uses it with HEAD.
| private long getContentLength() throws IOException { | ||
| String scheme = uri.getScheme(); | ||
| if ("http".equalsIgnoreCase(scheme) || "https".equalsIgnoreCase(scheme)) { | ||
| // Apply the same rule as Utils.openUrl to the HEAD probe: reject non-public | ||
| // destinations and do not auto-follow redirects (a 302 could otherwise reach an | ||
| // internal target such as the instance metadata endpoint). | ||
| if (!Utils.isInsecureUrlAllowed() && !Utils.isPublicHost(uri.getHost())) { | ||
| throw new IOException("Blocked request to non-public address: " + uri.getHost()); | ||
| } | ||
| HttpURLConnection conn = null; | ||
| try { | ||
| resolved = true; | ||
| conn = (HttpURLConnection) uri.toURL().openConnection(); | ||
| if (!Utils.isInsecureUrlAllowed()) { | ||
| conn.setInstanceFollowRedirects(false); | ||
| } | ||
| conn.setRequestMethod("HEAD"); | ||
| int code = conn.getResponseCode(); |
There was a problem hiding this comment.
You're right, the description did not match the code — getContentLength() was building its own connection and only duplicating the host check. Fixed the code rather than the wording in dcb78a6: both the download and the HEAD probe now go through the shared Utils.openHttpConnection(...), so the described behavior is what actually happens.
| URL current = url; | ||
| for (int i = 0; i <= MAX_REDIRECTS; ++i) { | ||
| String protocol = current.getProtocol(); | ||
| if (!"http".equalsIgnoreCase(protocol) && !"https".equalsIgnoreCase(protocol)) { | ||
| throw new IOException("Blocked redirect to unsupported URL protocol: " + protocol); | ||
| } |
There was a problem hiding this comment.
I don't think this is off by one — iteration i = 0 issues the initial request, not a redirect, so i <= MAX_REDIRECTS gives 1 request + at most 5 redirect hops, which matches MAX_REDIRECTS = 5 and the >5 message.
Traced against testTooManyRedirectsIsRejected (a chain where every hop redirects):
i=0 GET /hop0 -> 302 (redirect 1 resolved)
...
i=4 GET /hop4 -> 302 (redirect 5 resolved)
i=5 GET /hop5 -> 302 -> loop exits, throws "Too many redirects (>5)"
So /hop6 is never fetched: 6 requests total = 1 initial + 5 follows. Changing the bound to i < MAX_REDIRECTS would actually introduce an off-by-one (only 4 redirects followed). Since the intent wasn't obvious, I added a clarifying comment in dcb78a6: // One initial request plus at most MAX_REDIRECTS redirect hops. Happy to rename the constant if you'd still prefer that.
| // Resolve relative redirects against the current URL, then re-validate the target. | ||
| current = new URL(current, location); | ||
| } | ||
| throw new IOException("Too many redirects (>" + MAX_REDIRECTS + ") for URL: " + url); |
There was a problem hiding this comment.
I don't think this is off by one — iteration i = 0 issues the initial request, not a redirect, so i <= MAX_REDIRECTS gives 1 request + at most 5 redirect hops, which matches MAX_REDIRECTS = 5 and the >5 message.
Traced against testTooManyRedirectsIsRejected (a chain where every hop redirects):
i=0 GET /hop0 -> 302 (redirect 1 resolved)
...
i=4 GET /hop4 -> 302 (redirect 5 resolved)
i=5 GET /hop5 -> 302 -> loop exits, throws "Too many redirects (>5)"
So /hop6 is never fetched: 6 requests total = 1 initial + 5 follows. Changing the bound to i < MAX_REDIRECTS would actually introduce an off-by-one (only 4 redirects followed). Since the intent wasn't obvious, I added a clarifying comment in dcb78a6: // One initial request plus at most MAX_REDIRECTS redirect hops. Happy to rename the constant if you'd still prefer that.
| } finally { | ||
| System.clearProperty("ai.djl.compile_java"); | ||
| } |
There was a problem hiding this comment.
Reasonable concern. The mutation is already wrapped in try/finally, and the integration suite runs methods sequentially today, so there's no interference in the current configuration. I've left it as-is to keep the change small, but happy to add a lock or pin these methods to single-threaded execution if you'd rather guard against the suite being parallelized later.
| @Test | ||
| public void testInsecureOptOutAllowsFileProtocol() throws IOException { | ||
| System.setProperty("ai.djl.allow_insecure_url", "true"); | ||
| Assert.assertTrue(Utils.isInsecureUrlAllowed()); | ||
| Path tmp = Files.createTempFile("djl-optout", ".txt"); | ||
| Files.write(tmp, "ok".getBytes(java.nio.charset.StandardCharsets.UTF_8)); | ||
| try (java.io.InputStream is = Utils.openUrl(tmp.toUri().toURL())) { | ||
| Assert.assertEquals(new String(is.readAllBytes(), "UTF-8"), "ok"); | ||
| } finally { | ||
| Files.deleteIfExists(tmp); | ||
| } | ||
| } |
There was a problem hiding this comment.
Correct on both points, fixed in dcb78a6 — that test was a leftover from an earlier revision where file: was gated. Since file: is now allowed either way, the test proved nothing about the flag, so I removed it; the opt-out is covered by testInsecureOptOutAllowsNonPublicHttpHost, which uses an http host that is blocked by default and succeeds only with the flag set. Also switched the decode to StandardCharsets.UTF_8.
- Reject IPv6 unique local addresses (fc00::/7). InetAddress#isSiteLocalAddress only covers the deprecated fec0::/10, so fd00::/fc00:: destinations were treated as public. - Share one validated path for remote reads: openHttpConnection(url, method, headers) resolves redirects with a per-hop check and is now used by both openUrl (GET) and the SimpleUrlRepository content-length probe (HEAD). The HEAD probe previously built its own connection, so redirects to a public target were no longer resolved and the size came back unknown. - Clarify the hop bound: one initial request plus at most MAX_REDIRECTS hops. - Drop a test that claimed to exercise the opt-out flag but used file:, which is allowed either way; the flag is covered by the http case instead. Use StandardCharsets.UTF_8 rather than a string literal.
There was a problem hiding this comment.
🟡 Changes recommended
The new openHttpConnection(...) path currently bypasses offline/insecure-URL semantics for direct callers and bundled-Java compilation logs WARN unconditionally, both of which can cause incorrect behavior/noisy logs in common flows.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
api/src/main/java/ai/djl/util/ClassLoaderUtils.java:256
compileJavaClass(...)now logs a WARN and returns before checking whether theclasses/directory exists or contains any.javasources. SinceServingTranslatorFactorycalls this unconditionally during translator discovery, this will emit a warning for most model loads (including ones with only precompiled.class/.jartranslators), creating noisy logs and obscuring real warnings.
integration/src/main/java/ai/djl/integration/tests/model_zoo/CustomTranslatorTest.java:155
- This test sets
ai.djl.compile_javaand then unconditionally clears it, which can break other integration tests (or local runs) when the property was already set before this method ran. Preserving and restoring the prior value avoids cross-test interference while keeping the opt-in behavior under test.
System.setProperty("ai.djl.compile_java", "true");
try {
// load translator from classes folder
runImageClassification(Application.UNDEFINED, null, "MyTranslator");
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
| public static HttpURLConnection openHttpConnection( | ||
| URL url, String method, Map<String, String> headers) throws IOException { | ||
| return openHttpConnection(url, method, headers, Utils::isPublicHost); | ||
| } |
There was a problem hiding this comment.
Correct on both counts, and this was a regression from the previous commit — fixed in eb2f663.
When the HEAD probe moved into the shared helper, the isInsecureUrlAllowed() checks that getContentLength() previously had were dropped, so the probe stayed blocked even with DJL_ALLOW_INSECURE_URL=true, and the offline guard didn't apply to it either.
Rather than re-adding the checks at each call site, both now live in openHttpConnection(url, method, headers):
- offline mode throws before any connection is opened;
- with the opt-out set, it returns a plain connection for the requested method;
- otherwise it takes the validated redirect-resolving path.
openUrl now delegates to it instead of repeating the same logic, so GET and HEAD cannot drift apart again. Added testOptOutAppliesToHeadConnection, which asserts the probe is blocked by default for a loopback host and succeeds once the flag is set.
Separately, thanks for the note about compileJavaClass logging unconditionally — also fixed in eb2f663. The gate ran before the directory and file scan, so every model load warned even with no bundled sources. It now scans first: missing directory logs at debug, a directory with no .java is silent, and the warning is emitted only when sources are actually present (including the file count).
openHttpConnection was introduced as the shared path for remote reads but did not apply the offline-mode guard or the opt-out flag, so a caller using it directly (the content-length probe) would still open a connection in offline mode and would stay blocked even with DJL_ALLOW_INSECURE_URL set. Both checks now live in the shared helper, and openUrl delegates to it instead of repeating them. Also only report a skipped compilation when bundled sources are actually present. The check ran before the directory and file scan, so every model load logged a warning even when there were no .java sources to compile.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
| if (!hostAllowed.test(current.getHost())) { | ||
| throw new IOException( | ||
| "Blocked request to non-public address: " + current.getHost()); | ||
| } | ||
| HttpURLConnection conn = (HttpURLConnection) current.openConnection(); | ||
| conn.setInstanceFollowRedirects(false); |
There was a problem hiding this comment.
Fair point, and it's a real limitation rather than something I can fully close here. Pinning the connection to a validated address means connecting by IP and carrying the original host in the Host header, which then has to be reconciled with TLS SNI and certificate verification — more surgery than belongs in this change, and easy to get subtly wrong.
Documented it explicitly on isPublicHost in a672534 so the gap is stated rather than implied:
Note that this resolves the host name and the subsequent connection resolves it again, so a name that resolves to an allowed address here could resolve to a different address when the connection is made. Pinning the connection to a validated address would be needed to close that gap; environments that require it should restrict egress at the network layer.
Happy to look at pinning as a follow-up if you think it's worth carrying.
| try (InputStream is = new BufferedInputStream(fileUri.toURL().openStream())) { | ||
| // The artifact URI comes from the repository metadata and may be absolute, so route it | ||
| // through Utils.openUrl to apply the same URL handling as the rest of the download paths. | ||
| try (InputStream is = Utils.openUrl(fileUri.toURL())) { |
There was a problem hiding this comment.
You're right, and this one was my mistake — openUrl returns conn.getInputStream() for http, which is unbuffered, so routing these paths through it silently dropped the BufferedInputStream they had.
Fixed in a672534 by buffering inside openUrl rather than at each call site, so this path and SimpleUrlRepository.download both get it back and any future caller does too. openUrl already buffered the file:/jar: branch, so this also makes the two branches consistent.
| // Route through Utils.openUrl so the download is subject to the same URL handling | ||
| // (scheme restriction, non-public address blocking, redirect re-validation) instead of | ||
| // calling openStream() directly, which would bypass them. | ||
| try (InputStream is = Utils.openUrl(uri.toURL())) { |
There was a problem hiding this comment.
Same root cause as the AbstractRepository comment and fixed in the same commit (a672534): openUrl now wraps the http stream in a BufferedInputStream, so this call site keeps the buffering it had before. Handling it inside openUrl rather than here keeps the two download paths from drifting apart again.
| dir); | ||
| return; | ||
| } | ||
| JavaCompiler compiler = ToolProvider.getSystemJavaCompiler(); |
There was a problem hiding this comment.
Good catch — fixed in a672534. ToolProvider.getSystemJavaCompiler() returns null on a JRE, and the generic catch (Throwable) turned the resulting NPE into a "Failed to compile bundled java file" warning that pointed nowhere useful.
It now checks explicitly and logs that a JDK is required, including the file count and directory, then returns. Worth noting the null-compiler risk predates this change; it was just easier to hit once the file scan moved ahead of the enablement check.
| public void testRedirectToNonPublicBlockedByDefault() throws IOException { | ||
| // End-to-end through openUrl: a server that 302-redirects to a non-public destination is | ||
| // refused. Note the first hop here is itself non-public, so this asserts the entry check; | ||
| // the redirect re-validation itself is covered by testRedirectToDisallowedHostIsRejected. | ||
| com.sun.net.httpserver.HttpServer server = | ||
| com.sun.net.httpserver.HttpServer.create( | ||
| new java.net.InetSocketAddress("127.0.0.1", 0), 0); | ||
| server.createContext( | ||
| "/", | ||
| exchange -> { | ||
| exchange.getResponseHeaders() | ||
| .add( | ||
| "Location", | ||
| "http://169.254.169.254/latest/meta-data/iam/" | ||
| + "meta-data/artifact.tar.gz"); | ||
| exchange.sendResponseHeaders(302, -1); | ||
| exchange.close(); | ||
| }); | ||
| server.start(); | ||
| try { | ||
| int port = server.getAddress().getPort(); | ||
| URL redirecting = new URL("http://127.0.0.1:" + port + "/model.tar.gz"); | ||
| IOException ex = | ||
| Assert.expectThrows(IOException.class, () -> Utils.openUrl(redirecting)); | ||
| Assert.assertTrue( | ||
| ex.getMessage().contains("Blocked request to non-public address"), | ||
| "expected non-public-address block, got: " + ex.getMessage()); | ||
| } finally { | ||
| server.stop(0); | ||
| } |
There was a problem hiding this comment.
Agreed — the name promised more than the assertion delivered. Went with your option (a) plus a stronger assertion, in a672534.
Renamed to testNonPublicHostBlockedBeforeConnecting, and instead of only describing the behavior in a comment the test now counts requests reaching the listener and asserts hits == 0, so "refused before any connection" is actually verified. Redirect re-validation stays covered by testRedirectToDisallowedHostIsRejected, which allows the first hop and confirms the Location target is re-checked.
Review follow-ups: - Restore buffering on remote reads. Routing the repository downloads through Utils.openUrl dropped the BufferedInputStream those paths previously used, so openUrl now buffers the http stream and both callers get it back. - Handle a null system java compiler explicitly. On a JRE, ToolProvider returns null and the opt-in path threw an NPE that surfaced as a misleading "Failed to compile" warning. - Document that the destination check resolves the host name while the connection resolves it again, so a name can point elsewhere at connect time. - Do not widen the public API more than needed: only openHttpConnection is used from another package, so the flag and host-check helpers are package-private. - Use getContentLengthLong so archives larger than 2 GiB report a real size. - Do not leak the connection if the response status cannot be read. - Rename the loopback test to say what it asserts, and assert the listener is never contacted rather than only describing it in a comment.
Follow-ups from a second review pass: - Do not degrade silently when bundled sources are skipped. ServingTranslatorFactory resolves the translator from the compiled output, and with no class to find it fell through to the default (no-op) translator, so a model shipping libs/classes/*.java kept serving with different behavior and only a log line. compileJavaClass now reports the skip to the caller instead. - Only follow real redirects. 300, 304 and 305 were treated as redirects, so a conditional request answered with 304 failed with "no Location header". - Do not carry credentials across an origin change. Headers were re-sent on every hop, so an Authorization header for one host followed a redirect to another. Authorization, Cookie and Proxy-Authorization are dropped when scheme, host or port change. - Surface error responses instead of leaking the connection: a 4xx/5xx now disconnects and reports the status code rather than failing later in getInputStream. - Distinguish an unresolvable host from a private one in the log, so a name that simply does not resolve is not reported as a blocked address. - SimpleUrlRepository marks the metadata resolved only once it is built; it was set before the probe, so a failure left the repository permanently returning no metadata. - Tests: assert the bundled static initializer never ran, assert the real cause rather than an empty resource list, and skip the compilation cases when DJL_COMPILE_JAVA is set in the environment (the variable takes precedence over the system property).
Throwing on 4xx in the shared helper changed the meaning of a non-200 response: the content-length probe treats it as an unknown size and continues, so a repository whose HEAD returns 404 stopped reporting its resource at all. The helper now returns the connection for any non-redirect status, and openUrl releases the connection if opening the stream fails, which is where the leak actually was.
|
Follow-up from a self-review pass over the full diff (commits
Also restored the Test suite is 27 tests across |
docs/create_serving_ready_model.md lists compiling .java files in libs/classes at load time as one of the supported ways to ship a translator. That path is now opt-in, so document the flag and point at shipping a precompiled .class/.jar as the option that needs no flag.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
| compiler.run(null, null, null, files); | ||
| } catch (Throwable e) { | ||
| logger.warn("Failed to compile bundled java file", e); |
There was a problem hiding this comment.
Agreed, fixed in f5ec16a — and this is the same failure mode I fixed for the disabled case, so leaving it here was inconsistent. run(...) returning non-zero meant the compile failed, findImplementation then found nothing, and ServingTranslatorFactory fell back to a default translator: different inference results with nothing in the log to explain it.
It now logs at error with the file count, the directory and the exit code. Compiler diagnostics still go to stderr since run(null, null, null, ...) inherits it.
One deliberate choice worth flagging rather than burying: I log instead of throwing, even though the disabled path throws. A model directory can legitimately contain .java files that aren't the translator and don't compile standalone while a working .class/.jar sits alongside them — throwing would turn that from working into a hard load failure. Since compilation is already explicitly opted into at that point, an error log seemed the right side to err on. Happy to switch it to a throw if you'd rather be strict.
| private static boolean sameOrigin(URL a, URL b) { | ||
| return a.getProtocol().equalsIgnoreCase(b.getProtocol()) | ||
| && a.getHost().equalsIgnoreCase(b.getHost()) | ||
| && a.getPort() == b.getPort(); |
There was a problem hiding this comment.
Good catch, real bug, fixed in f5ec16a. URL#getPort() returns -1 for an implicit port, so https://host/a → https://host:443/b compared -1 against 443, was classified cross-origin, and dropped the credential headers on a hop that never left the origin.
Now compared via an effectivePort helper that substitutes getDefaultPort() when the port is implicit.
Added testSameOriginTreatsImplicitAndExplicitDefaultPortAsEqual, covering both http/80 and https/443 plus the genuinely-different port, scheme and host cases. I confirmed it fails on the previous code (expected [true] but found [false]) before taking the fix.
| private static Map<String, String> withoutCredentials(Map<String, String> headers) { | ||
| if (headers.isEmpty()) { | ||
| return headers; | ||
| } | ||
| Map<String, String> copy = new ConcurrentHashMap<>(); | ||
| for (Map.Entry<String, String> entry : headers.entrySet()) { | ||
| String name = entry.getKey(); | ||
| if (!"Authorization".equalsIgnoreCase(name) | ||
| && !"Cookie".equalsIgnoreCase(name) | ||
| && !"Proxy-Authorization".equalsIgnoreCase(name)) { | ||
| copy.put(name, entry.getValue()); | ||
| } | ||
| } | ||
| return copy; | ||
| } |
There was a problem hiding this comment.
Correct, and it would have thrown NPE — fixed in f5ec16a.
Rather than swap the map implementation I removed the copy entirely: the credential headers are now skipped where the request properties are set, guarded by a stripCredentials flag set when a hop leaves the origin. That keeps the previous semantics (once dropped, they stay dropped for later hops), passes a caller's null value straight through to addRequestProperty exactly as before, and avoids allocating a map per redirect. It also sidesteps the PMD UseConcurrentHashMap rule, which fires on a plain HashMap here.
Added testCrossOriginRedirectDropsCredentialsAndAllowsNullHeaderValue, which records what the final hop actually received and asserts Authorization is absent while a non-credential header still arrives, with a null-valued header in the map. Verified it throws NPE on the previous code. Also added testSameOriginRedirectKeepsCredentials as the counterpart — that direction had no coverage at all before.
| // Compiling a bundled .java translator is opt-in; enable it for the cases below, which | ||
| // exercise that path (the jar case is built from the compiled output). | ||
| System.setProperty("ai.djl.compile_java", "true"); | ||
| try { | ||
| // load translator from classes folder | ||
| runImageClassification(Application.UNDEFINED, null, "MyTranslator"); | ||
|
|
||
| Path jarFile = libsDir.resolve("example.jar"); | ||
| ZipUtils.zip(classesDir, jarFile, false); | ||
| Utils.deleteQuietly(classesDir); | ||
|
|
||
| // load translator from jar file | ||
| runImageClassification(Application.UNDEFINED, null, "MyTranslator"); | ||
| } finally { | ||
| System.clearProperty("ai.djl.compile_java"); | ||
| } |
There was a problem hiding this comment.
Correct, fixed in f5ec16a. Utils.getenv(name, def) returns the environment value whenever it is set, so DJL_COMPILE_JAVA=false makes the System.setProperty call below it a no-op and the assertions then fail with a confusing wrong-translator error.
Now skips with a message naming the value when the variable is set to anything that doesn't parse true.
Worth noting this was mine to catch: I had already added exactly this guard to the two unit tests in UrlAccessAndCompilationTest and did not carry it across to the one other test that depends on the same flag.
| } | ||
| } | ||
| } catch (UnknownHostException e) { | ||
| logger.warn("Cannot resolve host, treating as not public: {}", host, e); |
There was a problem hiding this comment.
Agreed, changed in f5ec16a. The message now logs the host without the throwable — the host name is the actionable part and a resolver failure stack trace is the same three frames every time, so at one per bad host it was pure noise.
Kept it at warn rather than dropping to debug: a load failing because a name doesn't resolve is something an operator should see, and the message now says what happened without the trace.
- sameOrigin compared URL#getPort directly, which is -1 for an implicit port, so a redirect that only makes the default port explicit was treated as cross-origin and dropped credential headers. Compare effective ports instead. - withoutCredentials copied the header map into a ConcurrentHashMap, which rejects null values, so a caller-supplied null header value threw during redirect handling. The copy is gone entirely: credential headers are now skipped when the request properties are set, which also avoids an allocation per hop. - compileJavaClass ignored the compiler exit code, so a failed compile surfaced later as a missing class or as a fallback to a default translator. Log the failure with the directory and exit code. - Dropped the stack trace from the unresolved-host warning; the host name is the actionable part and the trace repeats per bad host. - CustomTranslatorTest set the system property, but the environment variable takes precedence, so an environment pinning DJL_COMPILE_JAVA off would fail the test for an unrelated reason. Skip in that case, matching the guard already used in the unit tests.
- The map recording headers seen by the server was a plain HashMap written on the handler thread and read on the test thread. Use a concurrent map. - Absent headers were stored as the string "null" and asserted against that literal. Record only headers that were actually sent and assert on presence, which is what the test means. - File#list returns null rather than throwing, so the no-sources assertion could NPE. Use Files.list.
Asserting that Proxy-Authorization does not cross an origin passed for the wrong reason: HttpURLConnection treats it as a restricted header and never transmits it, so the check held whether or not the strip worked. Assert over a real connection only on the headers the JDK actually sends, prove they are sent at all with a same-origin control, and cover all three names directly in testCredentialHeadersAreRecognized.
A jar: URL nests another URL, as jar:<nested>!/<entry>, and the connection fetches that nested URL -- but the outer URL reports protocol "jar" with an empty host, so jar:http://host/x.jar was never seen by the destination rule and reached the network. Only a nested file: URL is a purely local read, so that is now the condition for allowing jar:, and the comment claiming jar: performs no network fetch was wrong. compileJavaClass no longer throws when sources are present and the opt-in is absent: - The exception was unchecked and escaped the declared exceptions on the model-load path, so callers catching ModelException/IOException missed it and BaseModelLoader's TranslateException handling was bypassed. - It fired on the mere presence of a .java file, so a model shipping a usable .class or .jar alongside one stopped loading. That included a model previously loaded with the opt-in, since javac writes the .class next to the source inside the DJL cache. The decision now sits in ServingTranslatorFactory, which raises a TranslateException only when no implementation was found and the only candidates were skipped sources, reported by hasSkippedJavaSources. Also: - Files.walk reports traversal errors from the terminal operation as an unchecked UncheckedIOException, so narrowing master's catch turned an unreadable subdirectory into a failed model load. Catch both. - Bound the connect phase: the redirect loop opens up to six connections, multiplying the OS SYN timeout on a black-holed host. Read timeouts are left alone since artifact downloads are legitimately slow. - Skip the content-length probe in offline mode and report an unknown size, so a cached URL-loaded model still loads with DJL_OFFLINE=true. - Assert on the message in the blocked-by-default tests. A bare assertThrows(IOException) also passed unpatched, where ftp: gave UnknownHostException and loopback gave ConnectException. - Save and restore the ai.djl.* properties the build forwards, and clear ai.djl.offline for tests that assert on the destination message, so the suite does not change meaning under 'gradlew --offline'.
There was a problem hiding this comment.
🟡 Changes recommended
It introduces a new public openHttpConnection API that can NPE on null headers and an integration test that does not restore a preexisting system property value, both of which can cause hard-to-diagnose failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
integration/src/main/java/ai/djl/integration/tests/model_zoo/CustomTranslatorTest.java:172
- This test unconditionally clears
ai.djl.compile_javainfinally. If the property was already set when the integration suite started (e.g., forwarded from the build), this test will silently change the global JVM state for subsequent tests. Save the previous value and restore it instead of clearing unconditionally.
System.setProperty("ai.djl.compile_java", "true");
try {
// load translator from classes folder
runImageClassification(Application.UNDEFINED, null, "MyTranslator");
Path jarFile = libsDir.resolve("example.jar");
ZipUtils.zip(classesDir, jarFile, false);
Utils.deleteQuietly(classesDir);
// load translator from jar file
runImageClassification(Application.UNDEFINED, null, "MyTranslator");
} finally {
System.clearProperty("ai.djl.compile_java");
}
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
| public static HttpURLConnection openHttpConnection( | ||
| URL url, String method, Map<String, String> headers) throws IOException { | ||
| if (isOfflineMode()) { | ||
| throw new IOException("Offline mode is enabled."); | ||
| } | ||
| if (isInsecureUrlAllowed()) { | ||
| HttpURLConnection conn = (HttpURLConnection) url.openConnection(); | ||
| conn.setRequestMethod(method); | ||
| for (Map.Entry<String, String> entry : headers.entrySet()) { | ||
| conn.addRequestProperty(entry.getKey(), entry.getValue()); | ||
| } | ||
| return conn; | ||
| } | ||
| return openHttpConnection(url, method, headers, Utils::isPublicHost); | ||
| } |
There was a problem hiding this comment.
Agreed, fixed in 3e17cae. Normalised in the public overload, ahead of both the opt-out branch and the delegation to the validated path, so a null map is handled once regardless of which route the call takes. The package-private overload is only reachable through that call site, so I left it without a duplicate guard.
I went with treating null as empty rather than IllegalArgumentException: "no headers" is a reasonable thing for a caller to mean, and the method already has a natural representation for it.
Two things worth stating rather than leaving implicit:
- This also widens
openUrl(URL, Map), which delegates here —openUrl(url, null)previously threwNullPointerExceptionand now behaves as an empty map. Strictly more permissive, so no existing caller can be affected, but it is a behavior change beyond the reported one. - Added
testNullHeadersAreTreatedAsEmpty, covering both routes (rejected on the destination by default, request actually issued with the opt-out set). I confirmed it fails on the previous code withNullPointerException: Cannot invoke "java.util.Map.entrySet()"before taking the fix.
I deliberately did not add the same guard to hasSkippedJavaSources(Path), the other new public method here: it passes the path to Files.isDirectory, and every java.nio.file.Files method plus compileJavaClass itself already throw NullPointerException on a null path. Guarding only this one would be the inconsistent choice. Happy to revisit if you would rather both be lenient.
openHttpConnection is new public API and iterated the header map directly, so a null argument surfaced as a NullPointerException from inside the redirect loop rather than as something a caller could act on. Normalise it once in the public overload, which covers both the opt-out branch and the validated path. The package-private overload is only reachable through that call site, so it needs no separate guard. Note this also widens openUrl(URL, Map), which delegates here: passing null headers previously threw NullPointerException and now behaves as an empty map. That is strictly more permissive, so no existing caller can be affected. Also correct the grammar in the directory log message that moved into findJavaSources. The identical string in scanDirectory is left alone to keep the diff to the code this change touches.
The header map is read in two independent places -- the validated redirect loop and the opt-out branch -- and only the second was covered. The package-private overload is called directly by tests, so it should not depend on a caller having normalised the argument; it now does so itself. The test that came with the previous commit claimed to exercise both routes but did not. Its default-path call is rejected on the destination before any header is read, so that half passed with or without the fix, and the loop production traffic actually takes was never reached with a null map. It now goes through the host-check seam with a permissive predicate so the loop body runs, and the comment says which calls do and do not exercise the argument. Verified by removing each guard independently: either one alone makes the test fail with NullPointerException, so neither branch is covered by accident.
The previous revision allowed file: on the protocol name alone and allowed jar: whose nested protocol was file. Neither is sufficient: - file://<host>/path is not a local read. The JDK's file handler falls back to FTP for any authority that is not empty, localhost or a ~ user reference, so such a URL performs an outbound fetch to <host>. Verified: openConnection() on file://10.0.0.5/x returns FtpURLConnection. - jar:file://<host>/x.jar!/entry therefore fetches over the network while satisfying a nested-protocol-name check, which is the same hole the nested check was added to close. Both are now judged on the destination: a file: URL must carry no remote authority, and a jar: URL is resolved to its nested URL and checked recursively. This also repairs a regression the nested-protocol-name check introduced. A classpath resource inside a Spring Boot 3.2+ executable jar has the form jar:nested:/app.jar/!BOOT-INF/lib/engine.jar!/jnilib/pytorch.properties, and pytorch LibUtils reads it through openUrl and wraps any IOException in an AssertionError. Rejecting every nested protocol other than file broke engine initialisation there for a read that touches no network. Container schemes that address a resource inside the running application (nested:, vfs:, wsjar:, bundleresource:) are local reads and are allowed; only remote nested protocols and file: with an authority are refused. Verified by removing each check independently: either one alone makes the corresponding test fail.
…ects Reverts the loud failure added for a skipped compile. The ticket asked for the compilation to be gated behind an opt-in; failing the load as well was beyond that and broke two configurations that previously worked: a model shipping a usable .class or .jar alongside a .java, and a model whose .java is incidental to a translator resolved elsewhere. The skip is now reported at error level and the load proceeds, so ClassLoaderUtils no longer needs hasSkippedJavaSources and ServingTranslatorFactory is back to its original shape. Redirect handling, both reachable through the loop this change introduced: - Refuse a hop that drops https. HttpURLConnection will not follow a scheme-changing redirect, so resolving hops explicitly reintroduced the downgrade, and nothing downstream would notice because SimpleUrlRepository never records a checksum. - Refuse a 3xx that is not followed. getInputStream() succeeds for any status below 400, so returning the connection let a 300 or 305 negotiation page be written out as the model or the native library. isPublicHost also now rejects RFC 6598 carrier-grade NAT (100.64.0.0/10), which is standard Kubernetes secondary pod CIDR space and so addresses internal services. Other reserved ranges are deliberately excluded: no deployment serves an internal endpoint from unallocated or benchmarking space, so blocking them adds surface without removing risk. Tests: neutralise DJL_ALLOW_INSECURE_URL, which otherwise disables everything these tests assert; make the unreadable-directory test discriminating rather than passing as root; and fix two of my own tests that only failed off this machine -- one built a file: URL by concatenating a Windows absolute path, and one pointed an https URL at a plaintext listener and hung in the TLS handshake, since only the connect phase is bounded.
DJL 0.37.0 (deepjavalibrary/djl#3875) makes compilation of bundled .java sources opt-in, gated on DJL_COMPILE_JAVA / -Dai.djl.compile_java. WorkflowTest.testFunctions loads functions.json, which declares the custom function "oid" backed by workflows/libs/classes/.../OtherIdentityWF.java, a bundled source compiled at load time. With compilation now off by default the class is never produced, findImplementation returns null, and the test fails with BadWorkflowException: Could not load function oid. Enable the system property for the duration of the test, mirroring the fix DJL applied to its own CustomTranslatorTest in the same commit. The environment variable takes precedence over the system property, so skip rather than fail if an environment explicitly pins DJL_COMPILE_JAVA off. Test-only: this does not change the serving default, so the arbitrary code execution path #3875 closed stays closed.
Description
Adds configuration controls around two loading behaviors and tightens their defaults. Every previous behavior remains available, but from this release it has to be enabled explicitly — the defaults changed, so an existing deployment that relies on one of the behaviors below needs to set the corresponding flag.
1. Compiling bundled
.javasources at model load is now opt-inClassLoaderUtils.compileJavaClasspreviously compiled and loaded any.javasources found under a model'slib/classes/directory whenever a model was loaded. This now requires an explicit opt-in. When sources are present and the opt-in is absent, the skip is reported at error level naming the flag and the load proceeds, so a model that resolves its translator from a precompiled.class/.jar, or that does not need a bundled translator at all, is unaffected.2. Remote URL loading is limited to public http(s) destinations
Utils.openUrlnow:http/httpsto public destinations, rejecting loopback, link-local, RFC 1918, IPv6 unique-local and RFC 6598 carrier-grade NAT (100.64.0.0/10, standard Kubernetes pod CIDR space);HttpURLConnectionauto-follow) and applies the same destination rule to each hop, with a bounded hop count;https, whichHttpURLConnectionalso refuses;Authorization,CookieandProxy-Authorizationwhen a redirect crosses to a different scheme, host or port;file:only with no remote authority, because the JDK's file handler falls back to FTP forfile://<host>/pathand so performs an outbound fetch;jar:only when the URL it nests is itself a local read, checked recursively, since ajar:URL reports protocoljarwith an empty host and reveals nothing about its destination. Container schemes addressing a resource inside the running application (nested:for a Spring Boot executable jar,vfs:,wsjar:,bundleresource:) are local reads and remain allowed;3.
SimpleUrlRepositoryandAbstractRepositoryshare the same URL handlingSimpleUrlRepository.download(), its content-lengthHEADprobe, andAbstractRepository.download()(for absolute artifact URIs declared in repository metadata) previously issued their own connections. All three now go through the shared entry point, so the download paths and the probe behave consistently.What changes for existing deployments
.javaunderlib/classes/DJL_COMPILE_JAVA=trueor-Dai.djl.compile_java=truehttp(s)load from a host on a private network — an internal artifact mirror on an RFC 1918 address, a Kubernetes in-cluster service address, a VPC endpoint reached by private DNS, or alocalhostserverBlocked request to non-public address: <host>DJL_ALLOW_INSECURE_URL=trueor-Dai.djl.allow_insecure_url=trueftp:or another customURLStreamHandlerscheme passed toUtils.openUrlBlocked request using unsupported URL protocol: <scheme>Too many redirectsDJL_ALLOW_INSECURE_URLis deliberately a single all-or-nothing switch: setting it restores the entire previous path (no destination rule, no explicit redirect resolution, no scheme restriction, no header handling). There is no per-host allowlist. If that turns out to be too coarse for real deployments — for example an operator who wants one internal mirror reachable without disabling the rest — a narrowerDJL_ALLOWED_HOSTS-style control would be the natural follow-up, and I'm happy to add it here instead if you'd prefer.Both flags follow the existing
DJL_OFFLINE/ai.djl.offlineconvention (environment variable first, then system property).docs/create_serving_ready_model.mddocumented the bundled-.javapath as a supported way to ship a translator and has been updated to record the opt-in.Impact on djl-serving and LMI containers
Traced through djl-serving and validated in a running LMI container (model load plus inference), because that is where most of this library's remote loading actually happens.
Not affected — the standard LMI path does not reach the changed code. An LMI container runs
engine=Python, so the model download is performed byhuggingface_hubin the Python process, not by this library. Confirmed in a live container: the load is driven bydjl_python.lmi_vllm.vllm_async_serviceand vLLM's own logs showhuggingface_hubperforming the fetch.s3://is equally unaffected — it is served by its ownRepositoryFactoryand never callsopenUrl. A 31B model loaded toHealthy, returned correct tokens, and throughput matched the unpatched baseline within run-to-run noise.The plugin and endpoint results above were re-checked against the current revision. The model-load and throughput figures come from an earlier revision of this branch; the destination rule they exercise is unchanged since, but they are not a measurement of the final code.
Affected — one operator path. Registering a model from an
http(s)URL whose host resolves to a private address:Such a URL ending in
.tar.gz/.tgz/.zip/.taris routed toSimpleUrlRepository, whose content-lengthHEADprobe now applies the destination rule, so the registration fails withBlocked request to non-public address: <host>before any download starts. This covers an internal artifact mirror on an RFC 1918 address, a Kubernetes in-cluster service address (a ClusterIP is always10.x), a VPC endpoint reached by private DNS, and alocalhostserver during development. Note the rule applies to the resolved address, sohttp://models.internal.corp/m.tar.gzis included when that name resolves privately.A URL without an archive extension is routed to
RpcRepositoryinstead, whosedownloadperforms no fetch, so that shape is unaffected.Also affected: bundled
.javatranslators, but not via LMI.ServingTranslatorFactoryis only reached by the Java engine, so an LMI (Python-engine) deployment does not hit the compilation opt-in. A Java-engine djl-serving model shipping.javaunderlib/classes/does, and so does a workflow whosefuncsship.java.Regression found and fixed during that validation: the plugin loader reads each plugin's
plugin.definitionthroughopenUrlon ajar:URL. An earlier revision of this change blocked all non-http(s) schemes, which made all bundled plugins fail to load — the management console and KServe v2 endpoints returned 404 while the core inference routes kept working. Allowing local reads fixes it, and re-verified against the current revision: all 6 plugins load (everyplugin.definitionis ajar:file:URL with no authority), zero scheme blocks at startup, and/consoleand/v2/health/readyboth return 200. This is whyfile:andjar:are judged on their destination rather than refused outright.Not affected
No API changes to existing methods; one new public method (
Utils.openHttpConnection) so callers that only need response metadata share the same handling.These loading flows behave exactly as before:
file:paths (LocalRepository);s3://andgs://(handled by their ownRepositoryFactoryimplementations, which never callopenUrl);http(s)archives, thedjl://model zoo, and Hugging Face;jar:classpath resources — including the way djl-serving reads each plugin'splugin.definition;https://publish.djl.ai/...;.classor.jartranslators, or supplying one programmatically viaCriteria.optTranslator(...).Testing
UrlAccessAndCompilationTest(21 tests) andRepositoryDownloadTest(6 tests) covering the new defaults, both flags, explicit redirect resolution against a real local HTTP server (302 followed, disallowed destination, unsupported scheme, missingLocation, hop limit), the local-scheme cases, and the repository download paths.verifyJava,checkstyleMain/Test,pmdMain/Test,javadocall pass; all modules compile.