Repository navigation
[api] Make bundled Java compilation opt-in and restrict remote URL loading #3875
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
f164627
f83c5e7
a82030c
6416ca9
f0b9088
dcb78a6
eb2f663
a672534
4e5ac22
0b0ad82
7d915fc
f5ec16a
eab4095
ac57f07
54f5dec
9860b07
3e17cae
cafc6cb
ef1f7cd
d637802
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,6 @@ | |
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import java.io.BufferedInputStream; | ||
| import java.io.IOException; | ||
| import java.io.InputStream; | ||
| import java.net.HttpURLConnection; | ||
|
|
@@ -102,7 +101,10 @@ public List<MRL> getResources() { | |
| protected void download(Path tmp, URI baseUri, Artifact.Item item, Progress progress) | ||
| throws IOException { | ||
| logger.debug("Downloading artifact: {} ...", uri); | ||
| try (InputStream is = new BufferedInputStream(uri.toURL().openStream())) { | ||
| // 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())) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same root cause as the |
||
| save(is, tmp, item, progress); | ||
| } | ||
| } | ||
|
|
@@ -140,8 +142,9 @@ private long getContentLength() throws IOException { | |
| HttpURLConnection conn = null; | ||
| try { | ||
| resolved = true; | ||
| conn = (HttpURLConnection) uri.toURL().openConnection(); | ||
| conn.setRequestMethod("HEAD"); | ||
| // Route the HEAD probe through the same handling as the download path, so | ||
| // redirects are resolved explicitly and every hop is checked. | ||
| conn = Utils.openHttpConnection(uri.toURL(), "HEAD", Collections.emptyMap()); | ||
| int code = conn.getResponseCode(); | ||
|
Comment on lines
140
to
155
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, the description did not match the code — |
||
| if (code != 200) { | ||
| logger.debug("Failed detect content length, error code: {}", code); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,9 @@ | |
| import java.io.IOException; | ||
| import java.io.InputStream; | ||
| import java.net.HttpURLConnection; | ||
| import java.net.InetAddress; | ||
| import java.net.URL; | ||
| import java.net.UnknownHostException; | ||
| import java.nio.charset.StandardCharsets; | ||
| import java.nio.file.FileVisitOption; | ||
| import java.nio.file.Files; | ||
|
|
@@ -39,6 +41,7 @@ | |
| import java.util.Map; | ||
| import java.util.Objects; | ||
| import java.util.Scanner; | ||
| import java.util.function.Predicate; | ||
| import java.util.regex.Matcher; | ||
| import java.util.regex.Pattern; | ||
| import java.util.stream.Collectors; | ||
|
|
@@ -49,6 +52,8 @@ public final class Utils { | |
|
|
||
| private static final Logger logger = LoggerFactory.getLogger(Utils.class); | ||
|
|
||
| private static final int MAX_REDIRECTS = 5; | ||
|
|
||
| public static final String[] EMPTY_ARRAY = new String[0]; | ||
|
|
||
| private Utils() {} | ||
|
|
@@ -524,18 +529,160 @@ public static InputStream openUrl(URL url) throws IOException { | |
| * @throws IOException if an I/O exception occurs | ||
| */ | ||
| public static InputStream openUrl(URL url, Map<String, String> headers) throws IOException { | ||
| // Remote fetches are limited to public http(s) destinations by default, and redirects are | ||
| // resolved explicitly so each hop is checked against the same rule. Local-resource schemes | ||
| // (file:, jar:) are unrestricted, since they do not perform a network fetch. Other schemes | ||
| // are not supported. Set DJL_ALLOW_INSECURE_URL=true (or -Dai.djl.allow_insecure_url=true) | ||
| // to restore the previous unrestricted behavior. | ||
| boolean insecureAllowed = isInsecureUrlAllowed(); | ||
| String protocol = url.getProtocol(); | ||
| if ("http".equalsIgnoreCase(protocol) || "https".equalsIgnoreCase(protocol)) { | ||
| if (isOfflineMode()) { | ||
| throw new IOException("Offline mode is enabled."); | ||
| } | ||
| HttpURLConnection conn = (HttpURLConnection) url.openConnection(); | ||
| if (insecureAllowed) { | ||
| HttpURLConnection conn = (HttpURLConnection) url.openConnection(); | ||
| for (Map.Entry<String, String> entry : headers.entrySet()) { | ||
| conn.addRequestProperty(entry.getKey(), entry.getValue()); | ||
| } | ||
| return conn.getInputStream(); | ||
| } | ||
| return openHttpConnection(url, "GET", headers).getInputStream(); | ||
| } | ||
| // Local-resource schemes and full-opt-out use the legacy direct stream. | ||
| if (insecureAllowed | ||
| || "file".equalsIgnoreCase(protocol) | ||
| || "jar".equalsIgnoreCase(protocol)) { | ||
| return new BufferedInputStream(url.openStream()); | ||
| } | ||
| throw new IOException( | ||
| "Blocked request using unsupported URL protocol: " | ||
| + protocol | ||
| + ". Set DJL_ALLOW_INSECURE_URL=true to override."); | ||
| } | ||
|
|
||
| /** | ||
| * Opens an http(s) connection using the specified request method, resolving redirects | ||
| * explicitly so that every hop is checked against the same destination rule. | ||
| * | ||
| * <p>This is the shared entry point for remote reads: {@link #openUrl(URL, Map)} uses it for | ||
| * {@code GET}, and callers that only need response metadata can use it for {@code HEAD}. The | ||
| * caller owns the returned connection and should disconnect it when finished. | ||
| * | ||
| * @param url the URL to open | ||
| * @param method the HTTP request method, for example {@code GET} or {@code HEAD} | ||
| * @param headers HTTP headers | ||
| * @return the connection for the final, validated hop | ||
| * @throws IOException if an I/O exception occurs or a hop is not allowed | ||
| */ | ||
| public static HttpURLConnection openHttpConnection( | ||
| URL url, String method, Map<String, String> headers) throws IOException { | ||
| return openHttpConnection(url, method, headers, Utils::isPublicHost); | ||
| } | ||
|
Comment on lines
+668
to
+687
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Rather than re-adding the checks at each call site, both now live in
Separately, thanks for the note about
Comment on lines
+668
to
+687
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Two things worth stating rather than leaving implicit:
I deliberately did not add the same guard to |
||
|
|
||
| /** | ||
| * Opens an http(s) connection, resolving redirects explicitly so that every hop is checked | ||
| * against {@code hostAllowed} rather than being followed by the connection itself. | ||
| * | ||
| * <p>The host check is a parameter so the redirect handling can be exercised against a local | ||
| * test server. | ||
| * | ||
| * @param url the URL to open | ||
| * @param method the HTTP request method | ||
| * @param headers HTTP headers | ||
| * @param hostAllowed the check applied to the host of every hop | ||
| * @return the connection for the final, validated hop | ||
| * @throws IOException if an I/O exception occurs or a hop is not allowed | ||
| */ | ||
| static HttpURLConnection openHttpConnection( | ||
| URL url, String method, Map<String, String> headers, Predicate<String> hostAllowed) | ||
| throws IOException { | ||
| URL current = url; | ||
| // One initial request plus at most MAX_REDIRECTS redirect hops. | ||
| 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); | ||
| } | ||
|
Comment on lines
+709
to
+717
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is off by one — iteration Traced against So |
||
| if (!hostAllowed.test(current.getHost())) { | ||
| throw new IOException( | ||
| "Blocked request to non-public address: " + current.getHost()); | ||
| } | ||
| HttpURLConnection conn = (HttpURLConnection) current.openConnection(); | ||
| conn.setInstanceFollowRedirects(false); | ||
|
Comment on lines
+718
to
+723
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Documented it explicitly on
Happy to look at pinning as a follow-up if you think it's worth carrying. |
||
| conn.setRequestMethod(method); | ||
| for (Map.Entry<String, String> entry : headers.entrySet()) { | ||
| conn.addRequestProperty(entry.getKey(), entry.getValue()); | ||
| } | ||
| return conn.getInputStream(); | ||
| int code = conn.getResponseCode(); | ||
| if (code < 300 || code >= 400) { | ||
| return conn; | ||
| } | ||
| // Redirect: re-validate the target on the next loop iteration instead of letting | ||
| // HttpURLConnection follow it blindly (which would defeat the host check above). | ||
| String location = conn.getHeaderField("Location"); | ||
| conn.disconnect(); | ||
| if (location == null) { | ||
| throw new IOException("Redirect (" + code + ") with no Location header"); | ||
| } | ||
| // 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); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is off by one — iteration Traced against So |
||
| } | ||
|
|
||
| /** | ||
| * Returns whether insecure (non-public) URL access is explicitly allowed. | ||
| * | ||
| * @return true if {@code DJL_ALLOW_INSECURE_URL} / {@code ai.djl.allow_insecure_url} is set | ||
| * true | ||
| */ | ||
| public static boolean isInsecureUrlAllowed() { | ||
| String mode = | ||
| getenv("DJL_ALLOW_INSECURE_URL", System.getProperty("ai.djl.allow_insecure_url")); | ||
| return Boolean.parseBoolean(mode); | ||
| } | ||
|
|
||
| /** | ||
| * Returns whether a host resolves exclusively to public (non-internal) IP addresses. | ||
| * | ||
| * <p>A host is rejected if it is empty or if any resolved address is loopback, link-local, | ||
| * site-local (RFC 1918 private), wildcard, multicast, or an IPv6 unique local address | ||
| * (fc00::/7). The latter is checked explicitly because {@link InetAddress#isSiteLocalAddress()} | ||
| * only covers the deprecated fec0::/10 range. | ||
| * | ||
| * @param host the host name to validate | ||
| * @return true if every resolved address is a public address | ||
| */ | ||
| 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() | ||
| || isIpv6UniqueLocal(addr)) { | ||
| return false; | ||
| } | ||
| } | ||
| } catch (UnknownHostException e) { | ||
| return false; | ||
| } | ||
| return new BufferedInputStream(url.openStream()); | ||
| return true; | ||
| } | ||
|
|
||
| private static boolean isIpv6UniqueLocal(InetAddress addr) { | ||
| byte[] bytes = addr.getAddress(); | ||
| // fc00::/7 - the first seven bits are 1111110 | ||
| return bytes.length == 16 && (bytes[0] & 0xFE) == 0xFC; | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| /* | ||
| * Copyright 2026 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"). You may not use this file except in compliance | ||
| * with the License. A copy of the License is located at | ||
| * | ||
| * http://aws.amazon.com/apache2.0/ | ||
| * | ||
| * or in the "license" file accompanying this file. This file is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES | ||
| * OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions | ||
| * and limitations under the License. | ||
| */ | ||
| package ai.djl.repository; | ||
|
|
||
| import org.testng.Assert; | ||
| import org.testng.annotations.Test; | ||
|
|
||
| import java.io.IOException; | ||
| import java.net.URI; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Path; | ||
|
|
||
| /** Tests the URL handling applied when downloading artifacts declared by repository metadata. */ | ||
| public class RepositoryDownloadTest { | ||
|
|
||
| @Test | ||
| public void testNonPublicArtifactUriIsRejected() throws IOException { | ||
| // An artifact URI declared in repository metadata may be absolute, which bypasses base-URI | ||
| // resolution. It must still be subject to the same destination rule as the other download | ||
| // paths, so a non-public destination is refused. | ||
| IOException e = downloadItemUri("http://169.254.169.254/latest/meta-data/x.tar.gz"); | ||
| Assert.assertTrue( | ||
| e.getMessage().contains("non-public"), "unexpected message: " + e.getMessage()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testPrivateArtifactUriIsRejected() throws IOException { | ||
| IOException e = downloadItemUri("http://10.0.0.5/model.tar.gz"); | ||
| Assert.assertTrue( | ||
| e.getMessage().contains("non-public"), "unexpected message: " + e.getMessage()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testLoopbackArtifactUriIsRejected() throws IOException { | ||
| IOException e = downloadItemUri("http://127.0.0.1:1/model.tar.gz"); | ||
| Assert.assertTrue( | ||
| e.getMessage().contains("non-public"), "unexpected message: " + e.getMessage()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testUnsupportedSchemeArtifactUriIsRejected() throws IOException { | ||
| IOException e = downloadItemUri("ftp://ftp.example.com/model.tar.gz"); | ||
| Assert.assertTrue( | ||
| e.getMessage().contains("URL protocol"), "unexpected message: " + e.getMessage()); | ||
| } | ||
|
|
||
| /** | ||
| * Invokes the shared artifact-download path with an absolute item URI and returns the thrown | ||
| * exception. | ||
| */ | ||
| private IOException downloadItemUri(String itemUri) throws IOException { | ||
| Path tmp = Files.createTempDirectory("djl-download-probe"); | ||
| try { | ||
| Repository repository = | ||
| new RemoteRepository( | ||
| "test", URI.create("https://resources.djl.ai/test-models/")); | ||
| Artifact.Item item = new Artifact.Item(); | ||
| item.setName("data"); | ||
| item.setUri(itemUri); | ||
|
|
||
| return Assert.expectThrows( | ||
| IOException.class, | ||
| () -> | ||
| ((AbstractRepository) repository) | ||
| .download(tmp, URI.create(""), item, null)); | ||
| } finally { | ||
| Files.deleteIfExists(tmp); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void testSimpleUrlRepositoryRejectsNonPublicHost() { | ||
| // The content-length HEAD probe is a second entry point into the same download flow, so it | ||
| // applies the same destination rule. | ||
| SimpleUrlRepository repository = | ||
| new SimpleUrlRepository( | ||
| "test", | ||
| URI.create("http://169.254.169.254/latest/meta-data/x.tar.gz"), | ||
| "x.tar.gz"); | ||
| // getResources() resolves metadata, which performs the HEAD probe. | ||
| Assert.assertTrue(repository.getResources().isEmpty()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSimpleUrlRepositoryDownloadRejectsNonPublicHost() throws IOException { | ||
| SimpleUrlRepository repository = | ||
| new SimpleUrlRepository( | ||
| "test", URI.create("http://10.0.0.5/model.tar.gz"), "model.tar.gz"); | ||
| Path tmp = Files.createTempDirectory("djl-simpleurl-probe"); | ||
| try { | ||
| Artifact.Item item = new Artifact.Item(); | ||
| item.setName("data"); | ||
| item.setUri("http://10.0.0.5/model.tar.gz"); | ||
| IOException e = | ||
| Assert.expectThrows( | ||
| IOException.class, | ||
| () -> repository.download(tmp, URI.create(""), item, null)); | ||
| Assert.assertTrue( | ||
| e.getMessage().contains("non-public"), "unexpected: " + e.getMessage()); | ||
| } finally { | ||
| Files.deleteIfExists(tmp); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, and this one was my mistake —
openUrlreturnsconn.getInputStream()for http, which is unbuffered, so routing these paths through it silently dropped theBufferedInputStreamthey had.Fixed in a672534 by buffering inside
openUrlrather than at each call site, so this path andSimpleUrlRepository.downloadboth get it back and any future caller does too.openUrlalready buffered thefile:/jar:branch, so this also makes the two branches consistent.