From 4ae3d8f2cd3ec6e18fdf60e0ddf495bf43b5950f Mon Sep 17 00:00:00 2001 From: Michael McMahon Date: Tue, 21 Mar 2023 17:10:57 +0000 Subject: [PATCH] 8302475: Enhance HTTP client file downloading Reviewed-by: dfuchs, rhalade --- .../net/http/ResponseBodyHandlers.java | 128 +++++++++++++++--- .../net/httpclient/AsFileDownloadTest.java | 37 ++--- 2 files changed, 128 insertions(+), 37 deletions(-) diff --git a/src/java.net.http/share/classes/jdk/internal/net/http/ResponseBodyHandlers.java b/src/java.net.http/share/classes/jdk/internal/net/http/ResponseBodyHandlers.java index 329f183ce7a..66d89ae1fc5 100644 --- a/src/java.net.http/share/classes/jdk/internal/net/http/ResponseBodyHandlers.java +++ b/src/java.net.http/share/classes/jdk/internal/net/http/ResponseBodyHandlers.java @@ -45,6 +45,7 @@ import java.net.http.HttpResponse; import java.net.http.HttpResponse.BodyHandler; import java.net.http.HttpResponse.ResponseInfo; import java.net.http.HttpResponse.BodySubscriber; +import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; import jdk.internal.net.http.ResponseSubscribers.PathSubscriber; @@ -229,16 +230,120 @@ public final class ResponseBodyHandlers { static final String DISPOSITION_TYPE = "attachment;"; /** The "filename" parameter. */ - static final Pattern FILENAME = Pattern.compile("filename\\s*=", CASE_INSENSITIVE); + static final Pattern FILENAME = Pattern.compile("filename\\s*=\\s*", CASE_INSENSITIVE); static final List PROHIBITED = List.of(".", "..", "", "~" , "|"); + // Characters disallowed in token values + + static final Set NOT_ALLOWED_IN_TOKEN = Set.of( + '(', ')', '<', '>', '@', + ',', ';', ':', '\\', '"', + '/', '[', ']', '?', '=', + '{', '}', ' ', '\t'); + + static boolean allowedInToken(char c) { + if (NOT_ALLOWED_IN_TOKEN.contains(c)) + return false; + // exclude CTL chars <= 31, == 127, or anything >= 128 + return isTokenText(c); + } + static final UncheckedIOException unchecked(ResponseInfo rinfo, String msg) { String s = String.format("%s in response [%d, %s]", msg, rinfo.statusCode(), rinfo.headers()); return new UncheckedIOException(new IOException(s)); } + static final UncheckedIOException unchecked(String msg) { + return new UncheckedIOException(new IOException(msg)); + } + + // Process a "filename=" parameter, which is either a "token" + // or a "quoted string". If a token, it is terminated by a + // semicolon or the end of the string. + // If a quoted string (surrounded by "" chars then the closing " + // terminates the name. + // quoted strings may contain quoted-pairs (eg embedded " chars) + + static String processFilename(String src) throws UncheckedIOException { + if ("".equals(src)) + return src; + if (src.charAt(0) == '\"') { + return processQuotedString(src.substring(1)); + } else { + return processToken(src); + } + } + + static boolean isTokenText(char c) throws UncheckedIOException { + return c > 31 && c < 127; + } + + static boolean isQuotedStringText(char c) throws UncheckedIOException { + return c > 31; + } + + static String processQuotedString(String src) throws UncheckedIOException { + boolean inqpair = false; + int len = src.length(); + StringBuilder sb = new StringBuilder(); + + for (int i=0; i apply(ResponseInfo responseInfo) { String dispoHeader = responseInfo.headers().firstValue("Content-Disposition") @@ -256,13 +361,7 @@ public final class ResponseBodyHandlers { } int n = matcher.end(); - int semi = dispoHeader.substring(n).indexOf(";"); - String filenameParam; - if (semi < 0) { - filenameParam = dispoHeader.substring(n); - } else { - filenameParam = dispoHeader.substring(n, n + semi); - } + String filenameParam = processFilename(dispoHeader.substring(n)); // strip all but the last path segment int x = filenameParam.lastIndexOf("/"); @@ -276,19 +375,6 @@ public final class ResponseBodyHandlers { filenameParam = filenameParam.trim(); - if (filenameParam.startsWith("\"")) { // quoted-string - if (!filenameParam.endsWith("\"") || filenameParam.length() == 1) { - throw unchecked(responseInfo, - "Badly quoted Content-Disposition filename parameter"); - } - filenameParam = filenameParam.substring(1, filenameParam.length() -1 ); - } else { // token, - if (filenameParam.contains(" ")) { // space disallowed - throw unchecked(responseInfo, - "unquoted space in Content-Disposition filename parameter"); - } - } - if (PROHIBITED.contains(filenameParam)) { throw unchecked(responseInfo, "Prohibited Content-Disposition filename parameter:" diff --git a/test/jdk/java/net/httpclient/AsFileDownloadTest.java b/test/jdk/java/net/httpclient/AsFileDownloadTest.java index b8ba2894b59..d191c8f4d4a 100644 --- a/test/jdk/java/net/httpclient/AsFileDownloadTest.java +++ b/test/jdk/java/net/httpclient/AsFileDownloadTest.java @@ -71,7 +71,7 @@ import static org.testng.Assert.fail; /* * @test * @summary Basic test for ofFileDownload - * @bug 8196965 + * @bug 8196965 8302475 * @library /test/lib /test/jdk/java/net/httpclient/lib * @build jdk.httpclient.test.lib.http2.Http2TestServer jdk.test.lib.net.SimpleSSLContext * jdk.test.lib.Platform jdk.test.lib.util.FileUtils @@ -126,18 +126,18 @@ public class AsFileDownloadTest { { "024", "attachment; filename=me.txt; filename*=utf-8''you.txt", "me.txt" }, { "025", "attachment; filename=\"m y.txt\"; filename*=utf-8''you.txt", "m y.txt" }, - { "030", "attachment; filename=foo/file1.txt", "file1.txt" }, - { "031", "attachment; filename=foo/bar/file2.txt", "file2.txt" }, - { "032", "attachment; filename=baz\\file3.txt", "file3.txt" }, - { "033", "attachment; filename=baz\\bar\\file4.txt", "file4.txt" }, - { "034", "attachment; filename=x/y\\file5.txt", "file5.txt" }, - { "035", "attachment; filename=x/y\\file6.txt", "file6.txt" }, - { "036", "attachment; filename=x/y\\z/file7.txt", "file7.txt" }, - { "037", "attachment; filename=x/y\\z/\\x/file8.txt", "file8.txt" }, - { "038", "attachment; filename=/root/file9.txt", "file9.txt" }, - { "039", "attachment; filename=../file10.txt", "file10.txt" }, - { "040", "attachment; filename=..\\file11.txt", "file11.txt" }, - { "041", "attachment; filename=foo/../../file12.txt", "file12.txt" }, + { "030", "attachment; filename=\"foo/file1.txt\"", "file1.txt" }, + { "031", "attachment; filename=\"foo/bar/file2.txt\"", "file2.txt" }, + { "032", "attachment; filename=\"baz\\\\file3.txt\"", "file3.txt" }, + { "033", "attachment; filename=\"baz\\\\bar\\\\file4.txt\"", "file4.txt" }, + { "034", "attachment; filename=\"x/y\\\\file5.txt\"", "file5.txt" }, + { "035", "attachment; filename=\"x/y\\\\file6.txt\"", "file6.txt" }, + { "036", "attachment; filename=\"x/y\\\\z/file7.txt\"", "file7.txt" }, + { "037", "attachment; filename=\"x/y\\\\z/\\\\x/file8.txt\"", "file8.txt" }, + { "038", "attachment; filename=\"/root/file9.txt\"", "file9.txt" }, + { "039", "attachment; filename=\"../file10.txt\"", "file10.txt" }, + { "040", "attachment; filename=\"..\\\\file11.txt\"", "file11.txt" }, + { "041", "attachment; filename=\"foo/../../file12.txt\"", "file12.txt" }, }; @DataProvider(name = "positive") @@ -178,19 +178,24 @@ public class AsFileDownloadTest { BodyHandler bh = ofFileDownload(tempDir.resolve(uri.getPath().substring(1)), CREATE, TRUNCATE_EXISTING, WRITE); HttpResponse response = client.send(request, bh); - + Path body = response.body(); out.println("Got response: " + response); - out.println("Got body Path: " + response.body()); + out.println("Got body Path: " + body); String fileContents = new String(Files.readAllBytes(response.body()), UTF_8); out.println("Got body: " + fileContents); assertEquals(response.statusCode(), 200); - assertEquals(response.body().getFileName().toString(), expectedFilename); + assertEquals(body.getFileName().toString(), expectedFilename); assertTrue(response.headers().firstValue("Content-Disposition").isPresent()); assertEquals(response.headers().firstValue("Content-Disposition").get(), contentDispositionValue); assertEquals(fileContents, "May the luck of the Irish be with you!"); + if (!body.toAbsolutePath().startsWith(tempDir.toAbsolutePath())) { + System.out.println("Tempdir = " + tempDir.toAbsolutePath()); + System.out.println("body = " + body.toAbsolutePath()); + throw new AssertionError("body in wrong location"); + } // additional checks unrelated to file download caseInsensitivityOfHeaders(request.headers()); caseInsensitivityOfHeaders(response.headers());