This is an automated email from the ASF dual-hosted git repository. luigidemasi pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/camel-kit.git
commit ac688d442ef8c3228158b3b09f68e98bcb91b319 Author: Andrea Cosentino <[email protected]> AuthorDate: Mon Nov 2 09:38:04 2026 +0100 Fix #137: validate every redirect hop's transport before following it Address review feedback. The client used HttpClient.Redirect.NORMAL, which follows HTTP->HTTP redirects (including loopback -> non-loopback host) without re-checking the scheme, so a local mirror could redirect the manifest, archive or release-discovery request to a remote cleartext backend. Switch to Redirect.NEVER and follow redirects manually in the shared send() path, calling requireFetchableUri() on each hop before the request, bounded to five hops, preserving the response deadline and body cleanup, and carrying Accept. HTTPS redirects (used by GitHub release assets) are unaffected. Add tests: a loopback -> non-loopback-http redirect is refused and a loopback -> loopback redirect is followed; plus independent rejection tests for the release-discovery endpoint, the discovered manifest URL, and the resolved archive URL -- the three guard sites the earlier single test missed. Co-authored-by: Claude Opus 4.8 <[email protected]> --- .../camel/kit/knowledge/mcp/IndexDownloader.java | 40 +++++- .../kit/knowledge/mcp/IndexDownloaderTest.java | 147 +++++++++++++++++++++ 2 files changed, 184 insertions(+), 3 deletions(-) diff --git a/knowledge/mcp/src/main/java/org/apache/camel/kit/knowledge/mcp/IndexDownloader.java b/knowledge/mcp/src/main/java/org/apache/camel/kit/knowledge/mcp/IndexDownloader.java index ce7c1723..ec940a26 100644 --- a/knowledge/mcp/src/main/java/org/apache/camel/kit/knowledge/mcp/IndexDownloader.java +++ b/knowledge/mcp/src/main/java/org/apache/camel/kit/knowledge/mcp/IndexDownloader.java @@ -86,8 +86,10 @@ public class IndexDownloader { private final Duration manifestTimeout; private final Duration assetTimeout; private final ObjectMapper mapper = new ObjectMapper(); + // NEVER, not NORMAL: redirects are followed manually in send() so each hop's transport is validated + // (requireFetchableUri) before it is fetched. NORMAL would follow a loopback->non-loopback-http hop first. private final HttpClient httpClient = HttpClient.newBuilder() - .followRedirects(HttpClient.Redirect.NORMAL) + .followRedirects(HttpClient.Redirect.NEVER) .connectTimeout(CONNECT_TIMEOUT) .build(); @@ -601,9 +603,41 @@ public class IndexDownloader { } private HttpResponse<InputStream> send(HttpRequest request) throws IOException, InterruptedException { + final int maxRedirects = 5; long deadline = System.nanoTime() + request.timeout().orElseThrow().toNanos(); - return httpClient.send(request, info -> HttpResponse.BodySubscribers.mapping( - HttpResponse.BodySubscribers.ofPublisher(), publisher -> new TimedBody(publisher, deadline))); + HttpRequest current = request; + for (int hop = 0;; hop++) { + HttpResponse<InputStream> response = httpClient.send(current, info -> HttpResponse.BodySubscribers.mapping( + HttpResponse.BodySubscribers.ofPublisher(), publisher -> new TimedBody(publisher, deadline))); + int status = response.statusCode(); + if (status != 301 && status != 302 && status != 303 && status != 307 && status != 308) { + return response; + } + // Validate the redirect target's transport BEFORE following it; a 304 is not a redirect and + // is returned above for the conditional-manifest path. + String target = response.headers().firstValue("Location").orElse(null); + response.body().close(); + if (hop >= maxRedirects) { + throw new IOException("Too many index redirects starting from " + request.uri()); + } + if (target == null) { + throw new IOException("Index redirect without a Location header from " + current.uri()); + } + URI next; + try { + next = current.uri().resolve(target); + } catch (IllegalArgumentException e) { + throw new IOException("Invalid index redirect target: " + target, e); + } + requireFetchableUri(next); + long remaining = deadline - System.nanoTime(); + if (remaining <= 0) { + throw new HttpTimeoutException("Index redirect handling timed out from " + request.uri()); + } + HttpRequest.Builder builder = HttpRequest.newBuilder(next).timeout(Duration.ofNanos(remaining)).GET(); + current.headers().firstValue("Accept").ifPresent(accept -> builder.header("Accept", accept)); + current = builder.build(); + } } /** Bounded HTTP buffering; only the resolving thread reads or writes index files. */ diff --git a/knowledge/mcp/src/test/java/org/apache/camel/kit/knowledge/mcp/IndexDownloaderTest.java b/knowledge/mcp/src/test/java/org/apache/camel/kit/knowledge/mcp/IndexDownloaderTest.java index 64d189a2..24707d10 100644 --- a/knowledge/mcp/src/test/java/org/apache/camel/kit/knowledge/mcp/IndexDownloaderTest.java +++ b/knowledge/mcp/src/test/java/org/apache/camel/kit/knowledge/mcp/IndexDownloaderTest.java @@ -205,6 +205,153 @@ class IndexDownloaderTest { assertTrue(refused, "A remote http manifest must be refused over cleartext: " + e); } + /** Asserts a cleartext-http refusal somewhere in the exception's cause chain. */ + private static void assertRefusedCleartext(Throwable thrown) { + for (Throwable t = thrown; t != null; t = t.getCause()) { + if (t.getMessage() != null && t.getMessage().contains("cleartext http")) { + return; + } + } + fail("Expected a cleartext-http refusal in the cause chain, got: " + thrown); + } + + /** A manifest-less config that drives release discovery against the given endpoint. */ + private IndexResolverConfig discoveryConfig(String releasesUrl) { + cacheDir = tmp.resolve("cache"); + return new IndexResolverConfig() { + @Override + public Optional<String> path() { + return Optional.empty(); + } + + @Override + public Optional<String> url() { + return Optional.empty(); + } + + @Override + public String releasesUrl() { + return releasesUrl; + } + + @Override + public Optional<String> version() { + return Optional.empty(); + } + + @Override + public String cacheDir() { + return cacheDir.toString(); + } + }; + } + + @Test + void manifestRedirectToNonLoopbackHttpIsRejected() throws Exception { + publishDir = tmp.resolve("publish"); + Files.createDirectories(publishDir); + cacheDir = tmp.resolve("cache"); + HttpServer redirector = HttpServer.create(new InetSocketAddress("127.0.0.1", 0), 0); + redirector.createContext("/", exchange -> { + try (exchange) { + exchange.getResponseHeaders().set("Location", "http://example.org/index.json";); + exchange.sendResponseHeaders(302, -1); + } + }); + redirector.start(); + try { + String url = "http://127.0.0.1:"; + redirector.getAddress().getPort() + "/index.json"; + assertRefusedCleartext(assertThrows(IOException.class, () -> downloader(null, url).resolve())); + assertFalse(Files.exists(cacheDir.resolve("current")), + "a rejected loopback->remote-http redirect must not activate an index"); + } finally { + redirector.stop(0); + } + } + + @Test + void loopbackHttpRedirectIsFollowed() throws Exception { + publish("2026.07.09", "redirect-followed", null); + HttpServer backend = HttpServer.create(new InetSocketAddress("127.0.0.1", 0), 0); + backend.createContext("/", exchange -> { + try (exchange) { + String name = exchange.getRequestURI().getPath().endsWith("index.json") + ? "index.json" + : "knowledge-index.zip"; + byte[] body = Files.readAllBytes(publishDir.resolve(name)); + exchange.sendResponseHeaders(200, body.length); + exchange.getResponseBody().write(body); + } + }); + backend.start(); + int backendPort = backend.getAddress().getPort(); + HttpServer redirector = HttpServer.create(new InetSocketAddress("127.0.0.1", 0), 0); + redirector.createContext("/", exchange -> { + try (exchange) { + exchange.getResponseHeaders() + .set("Location", "http://127.0.0.1:"; + backendPort + exchange.getRequestURI().getPath()); + exchange.sendResponseHeaders(302, -1); + } + }); + redirector.start(); + try { + String url = "http://127.0.0.1:"; + redirector.getAddress().getPort() + "/index.json"; + IndexDownloader.ResolvedIndex resolved = downloader(null, url).resolve(); + assertEquals("redirect-followed", Files.readString(resolved.dir().resolve("segments_1")), + "a loopback->loopback http redirect must be followed"); + } finally { + redirector.stop(0); + backend.stop(0); + } + } + + @Test + void releasesEndpointOverNonLoopbackHttpIsRejected() { + IndexDownloader downloader = new IndexDownloader(discoveryConfig("http://example.org/releases";)); + assertRefusedCleartext(assertThrows(IOException.class, () -> downloader.resolve())); + assertFalse(Files.exists(cacheDir.resolve("current"))); + } + + @Test + void discoveredManifestUrlOverNonLoopbackHttpIsRejected() throws Exception { + HttpServer releases = HttpServer.create(new InetSocketAddress("127.0.0.1", 0), 0); + releases.createContext("/", exchange -> { + try (exchange) { + byte[] body = ("[{\"tag_name\":\"knowledge-index-v1-2026.07.09-000000\",\"draft\":false," + + "\"prerelease\":false,\"assets\":[{\"name\":\"index.json\"," + + "\"browser_download_url\":\"http://example.org/index.json\"}]}]";) + .getBytes(java.nio.charset.StandardCharsets.UTF_8); + exchange.sendResponseHeaders(200, body.length); + exchange.getResponseBody().write(body); + } + }); + releases.start(); + try { + String releasesUrl = "http://127.0.0.1:"; + releases.getAddress().getPort() + "/releases"; + IndexDownloader downloader = new IndexDownloader(discoveryConfig(releasesUrl)); + assertRefusedCleartext(assertThrows(IOException.class, () -> downloader.resolve())); + assertFalse(Files.exists(cacheDir.resolve("current"))); + } finally { + releases.stop(0); + } + } + + @Test + void resolvedArchiveUrlOverNonLoopbackHttpIsRejected() throws Exception { + publishDir = tmp.resolve("publish"); + Files.createDirectories(publishDir); + cacheDir = tmp.resolve("cache"); + Files.writeString(publishDir.resolve("index.json"), new ObjectMapper().createObjectNode() + .put("version", "2026.07.09") + .put("sha256", "0".repeat(64)) + .put("asset", "http://example.org/knowledge-index.zip";) + .put("embeddingModel", "granite-embedding-small-english-r2-q8").toString()); + String url = "file://" + publishDir.resolve("index.json"); + + assertRefusedCleartext(assertThrows(IOException.class, () -> downloader(null, url).resolve())); + assertFalse(Files.exists(cacheDir.resolve("2026.07.09")), "a rejected archive URL must not be installed"); + } + @Test void newVersionReplacesOldAndPrunes() throws Exception { String url = publish("2026.07.01", "old-bytes", null);
