Skip to content

Commit 6e026e5

Browse files
Ekaterina VergizovaRealCLanger
Ekaterina Vergizova
authored and
RealCLanger
committed
8302475: Enhance HTTP client file downloading
Reviewed-by: mbalao Backport-of: 1d26da2ef83de0c76f3c4b85c98c6c30d2e3aaf3
1 parent 7fc5e31 commit 6e026e5

File tree

2 files changed

+128
-36
lines changed

2 files changed

+128
-36
lines changed

‎src/java.net.http/share/classes/jdk/internal/net/http/ResponseBodyHandlers.java‎

Lines changed: 107 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@
4545
import java.net.http.HttpResponse.BodyHandler;
4646
import java.net.http.HttpResponse.ResponseInfo;
4747
import java.net.http.HttpResponse.BodySubscriber;
48+
import java.util.Set;
4849
import java.util.regex.Matcher;
4950
import java.util.regex.Pattern;
5051
import jdk.internal.net.http.ResponseSubscribers.PathSubscriber;
@@ -229,16 +230,120 @@ private FileDownloadBodyHandler(Path directory,
229230
static final String DISPOSITION_TYPE = "attachment;";
230231

231232
/** The "filename" parameter. */
232-
static final Pattern FILENAME = Pattern.compile("filename\\s*=", CASE_INSENSITIVE);
233+
static final Pattern FILENAME = Pattern.compile("filename\\s*=\\s*", CASE_INSENSITIVE);
233234

234235
static final List<String> PROHIBITED = List.of(".", "..", "", "~" , "|");
235236

237+
// Characters disallowed in token values
238+
239+
static final Set<Character> NOT_ALLOWED_IN_TOKEN = Set.of(
240+
'(', ')', '<', '>', '@',
241+
',', ';', ':', '\\', '"',
242+
'/', '[', ']', '?', '=',
243+
'{', '}', ' ', '\t');
244+
245+
static boolean allowedInToken(char c) {
246+
if (NOT_ALLOWED_IN_TOKEN.contains(c))
247+
return false;
248+
// exclude CTL chars <= 31, == 127, or anything >= 128
249+
return isTokenText(c);
250+
}
251+
236252
static final UncheckedIOException unchecked(ResponseInfo rinfo,
237253
String msg) {
238254
String s = String.format("%s in response [%d, %s]", msg, rinfo.statusCode(), rinfo.headers());
239255
return new UncheckedIOException(new IOException(s));
240256
}
241257

258+
static final UncheckedIOException unchecked(String msg) {
259+
return new UncheckedIOException(new IOException(msg));
260+
}
261+
262+
// Process a "filename=" parameter, which is either a "token"
263+
// or a "quoted string". If a token, it is terminated by a
264+
// semicolon or the end of the string.
265+
// If a quoted string (surrounded by "" chars then the closing "
266+
// terminates the name.
267+
// quoted strings may contain quoted-pairs (eg embedded " chars)
268+
269+
static String processFilename(String src) throws UncheckedIOException {
270+
if ("".equals(src))
271+
return src;
272+
if (src.charAt(0) == '\"') {
273+
return processQuotedString(src.substring(1));
274+
} else {
275+
return processToken(src);
276+
}
277+
}
278+
279+
static boolean isTokenText(char c) throws UncheckedIOException {
280+
return c > 31 && c < 127;
281+
}
282+
283+
static boolean isQuotedStringText(char c) throws UncheckedIOException {
284+
return c > 31;
285+
}
286+
287+
static String processQuotedString(String src) throws UncheckedIOException {
288+
boolean inqpair = false;
289+
int len = src.length();
290+
StringBuilder sb = new StringBuilder();
291+
292+
for (int i=0; i<len; i++) {
293+
char c = src.charAt(i);
294+
if (!isQuotedStringText(c)) {
295+
throw unchecked("Illegal character");
296+
}
297+
if (c == '\"') {
298+
if (!inqpair) {
299+
return sb.toString();
300+
} else {
301+
sb.append(c);
302+
}
303+
} else if (c == '\\') {
304+
if (!inqpair) {
305+
inqpair = true;
306+
continue;
307+
} else {
308+
// the quoted char is '\'
309+
sb.append(c);
310+
}
311+
} else {
312+
sb.append(c);
313+
}
314+
if (inqpair) {
315+
inqpair = false;
316+
}
317+
}
318+
// not terminated by "
319+
throw unchecked("Invalid quoted string");
320+
}
321+
322+
static String processToken(String src) throws UncheckedIOException {
323+
int end = 0;
324+
int len = src.length();
325+
boolean whitespace = false;
326+
327+
for (int i=0; i<len; i++) {
328+
char c = src.charAt(i);
329+
if (c == ';') {
330+
break;
331+
}
332+
if (c == ' ' || c == '\t') {
333+
// WS only until ; or end of string
334+
whitespace = true;
335+
continue;
336+
}
337+
end++;
338+
if (whitespace || !allowedInToken(c)) {
339+
String msg = whitespace ? "whitespace must be followed by a semicolon"
340+
: c + " is not allowed in a token";
341+
throw unchecked(msg);
342+
}
343+
}
344+
return src.substring(0, end);
345+
}
346+
242347
@Override
243348
public BodySubscriber<Path> apply(ResponseInfo responseInfo) {
244349
String dispoHeader = responseInfo.headers().firstValue("Content-Disposition")
@@ -256,13 +361,7 @@ public BodySubscriber<Path> apply(ResponseInfo responseInfo) {
256361
}
257362
int n = matcher.end();
258363

259-
int semi = dispoHeader.substring(n).indexOf(";");
260-
String filenameParam;
261-
if (semi < 0) {
262-
filenameParam = dispoHeader.substring(n);
263-
} else {
264-
filenameParam = dispoHeader.substring(n, n + semi);
265-
}
364+
String filenameParam = processFilename(dispoHeader.substring(n));
266365

267366
// strip all but the last path segment
268367
int x = filenameParam.lastIndexOf("/");
@@ -276,19 +375,6 @@ public BodySubscriber<Path> apply(ResponseInfo responseInfo) {
276375

277376
filenameParam = filenameParam.trim();
278377

279-
if (filenameParam.startsWith("\"")) { // quoted-string
280-
if (!filenameParam.endsWith("\"") || filenameParam.length() == 1) {
281-
throw unchecked(responseInfo,
282-
"Badly quoted Content-Disposition filename parameter");
283-
}
284-
filenameParam = filenameParam.substring(1, filenameParam.length() -1 );
285-
} else { // token,
286-
if (filenameParam.contains(" ")) { // space disallowed
287-
throw unchecked(responseInfo,
288-
"unquoted space in Content-Disposition filename parameter");
289-
}
290-
}
291-
292378
if (PROHIBITED.contains(filenameParam)) {
293379
throw unchecked(responseInfo,
294380
"Prohibited Content-Disposition filename parameter:"

‎test/jdk/java/net/httpclient/AsFileDownloadTest.java‎

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424
/*
2525
* @test
2626
* @summary Basic test for ofFileDownload
27-
* @bug 8196965
27+
* @bug 8196965 8302475
2828
* @modules java.base/sun.net.www.http
2929
* java.net.http/jdk.internal.net.http.common
3030
* java.net.http/jdk.internal.net.http.frame
@@ -127,18 +127,18 @@ public class AsFileDownloadTest {
127127
{ "024", "attachment; filename=me.txt; filename*=utf-8''you.txt", "me.txt" },
128128
{ "025", "attachment; filename=\"m y.txt\"; filename*=utf-8''you.txt", "m y.txt" },
129129

130-
{ "030", "attachment; filename=foo/file1.txt", "file1.txt" },
131-
{ "031", "attachment; filename=foo/bar/file2.txt", "file2.txt" },
132-
{ "032", "attachment; filename=baz\\file3.txt", "file3.txt" },
133-
{ "033", "attachment; filename=baz\\bar\\file4.txt", "file4.txt" },
134-
{ "034", "attachment; filename=x/y\\file5.txt", "file5.txt" },
135-
{ "035", "attachment; filename=x/y\\file6.txt", "file6.txt" },
136-
{ "036", "attachment; filename=x/y\\z/file7.txt", "file7.txt" },
137-
{ "037", "attachment; filename=x/y\\z/\\x/file8.txt", "file8.txt" },
138-
{ "038", "attachment; filename=/root/file9.txt", "file9.txt" },
139-
{ "039", "attachment; filename=../file10.txt", "file10.txt" },
140-
{ "040", "attachment; filename=..\\file11.txt", "file11.txt" },
141-
{ "041", "attachment; filename=foo/../../file12.txt", "file12.txt" },
130+
{ "030", "attachment; filename=\"foo/file1.txt\"", "file1.txt" },
131+
{ "031", "attachment; filename=\"foo/bar/file2.txt\"", "file2.txt" },
132+
{ "032", "attachment; filename=\"baz\\\\file3.txt\"", "file3.txt" },
133+
{ "033", "attachment; filename=\"baz\\\\bar\\\\file4.txt\"", "file4.txt" },
134+
{ "034", "attachment; filename=\"x/y\\\\file5.txt\"", "file5.txt" },
135+
{ "035", "attachment; filename=\"x/y\\\\file6.txt\"", "file6.txt" },
136+
{ "036", "attachment; filename=\"x/y\\\\z/file7.txt\"", "file7.txt" },
137+
{ "037", "attachment; filename=\"x/y\\\\z/\\\\x/file8.txt\"", "file8.txt" },
138+
{ "038", "attachment; filename=\"/root/file9.txt\"", "file9.txt" },
139+
{ "039", "attachment; filename=\"../file10.txt\"", "file10.txt" },
140+
{ "040", "attachment; filename=\"..\\\\file11.txt\"", "file11.txt" },
141+
{ "041", "attachment; filename=\"foo/../../file12.txt\"", "file12.txt" },
142142
};
143143

144144
@DataProvider(name = "positive")
@@ -177,18 +177,24 @@ void test(String uriString, String contentDispositionValue, String expectedFilen
177177
CREATE, TRUNCATE_EXISTING, WRITE);
178178
HttpResponse<Path> response = client.send(request, bh);
179179

180+
Path body = response.body();
180181
out.println("Got response: " + response);
181-
out.println("Got body Path: " + response.body());
182+
out.println("Got body Path: " + body);
182183
String fileContents = new String(Files.readAllBytes(response.body()), UTF_8);
183184
out.println("Got body: " + fileContents);
184185

185186
assertEquals(response.statusCode(),200);
186-
assertEquals(response.body().getFileName().toString(), expectedFilename);
187+
assertEquals(body.getFileName().toString(), expectedFilename);
187188
assertTrue(response.headers().firstValue("Content-Disposition").isPresent());
188189
assertEquals(response.headers().firstValue("Content-Disposition").get(),
189190
contentDispositionValue);
190191
assertEquals(fileContents, "May the luck of the Irish be with you!");
191192

193+
if (!body.toAbsolutePath().startsWith(tempDir.toAbsolutePath())) {
194+
System.out.println("Tempdir = " + tempDir.toAbsolutePath());
195+
System.out.println("body = " + body.toAbsolutePath());
196+
throw new AssertionError("body in wrong location");
197+
}
192198
// additional checks unrelated to file download
193199
caseInsensitivityOfHeaders(request.headers());
194200
caseInsensitivityOfHeaders(response.headers());

0 commit comments

Comments
 (0)