http-client-branch: fix asFileDownload handler and add test http-client-branch
authorchegar
Thu, 15 Feb 2018 14:10:27 +0000
branchhttp-client-branch
changeset 56132 c8a1eccbc719
parent 56131 99f144742013
child 56133 be9720a556c7
http-client-branch: fix asFileDownload handler and add test
src/java.net.http/share/classes/jdk/internal/net/http/ResponseBodyHandlers.java
test/jdk/java/net/httpclient/AsFileDownloadTest.java
test/jdk/java/net/httpclient/http2/server/Http2TestServerConnection.java
--- a/src/java.net.http/share/classes/jdk/internal/net/http/ResponseBodyHandlers.java	Thu Feb 15 12:31:14 2018 +0000
+++ b/src/java.net.http/share/classes/jdk/internal/net/http/ResponseBodyHandlers.java	Thu Feb 15 14:10:27 2018 +0000
@@ -26,11 +26,13 @@
 package jdk.internal.net.http;
 
 import java.io.IOException;
+import java.io.UncheckedIOException;
 import java.net.URI;
 import java.nio.file.OpenOption;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.security.AccessControlContext;
+import java.util.List;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ConcurrentMap;
 import java.util.function.Function;
@@ -39,8 +41,10 @@
 import java.net.http.HttpResponse;
 import java.net.http.HttpResponse.BodyHandler;
 import java.net.http.HttpResponse.BodySubscriber;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 import jdk.internal.net.http.ResponseSubscribers.PathSubscriber;
-import static jdk.internal.net.http.common.Utils.unchecked;
+import static java.util.regex.Pattern.CASE_INSENSITIVE;
 
 public final class ResponseBodyHandlers {
 
@@ -124,7 +128,7 @@
     // Similar to Path body handler, but for file download. Supports setting ACC.
     public static class FileDownloadBodyHandler implements UntrustedBodyHandler<Path> {
         private final Path directory;
-        private final OpenOption[]openOptions;
+        private final OpenOption[] openOptions;
         private volatile AccessControlContext acc;
 
         public FileDownloadBodyHandler(Path directory, OpenOption... openOptions) {
@@ -137,25 +141,85 @@
             this.acc = acc;
         }
 
+        /** The "attachment" disposition-type and separator. */
+        static final String DISPOSITION_TYPE = "attachment;";
+
+        /** The "filename" parameter. */
+        static final Pattern FILENAME = Pattern.compile("filename\\s*=", CASE_INSENSITIVE);
+
+        static final List<String> PROHIBITED = List.of(".", "..", "", "~" , "|");
+
+        static final UncheckedIOException unchecked(int code,
+                                                    HttpHeaders headers,
+                                                    String msg) {
+            String s = String.format("%s in response [%d, %s]", msg, code, headers);
+            return new UncheckedIOException(new IOException(s));
+        }
+
         @Override
         public BodySubscriber<Path> apply(int statusCode, HttpHeaders headers) {
             String dispoHeader = headers.firstValue("Content-Disposition")
-                    .orElseThrow(() -> unchecked(new IOException("No Content-Disposition")));
-            if (!dispoHeader.startsWith("attachment;")) {
-                throw unchecked(new IOException("Unknown Content-Disposition type"));
+                    .orElseThrow(() -> unchecked(statusCode, headers,
+                            "No Content-Disposition header"));
+
+            if (!dispoHeader.regionMatches(true, // ignoreCase
+                                           0, DISPOSITION_TYPE,
+                                           0, DISPOSITION_TYPE.length())) {
+                throw unchecked(statusCode, headers, "Unknown Content-Disposition type");
+            }
+
+            Matcher matcher = FILENAME.matcher(dispoHeader);
+            if (!matcher.find()) {
+                throw unchecked(statusCode, headers,
+                          "Bad Content-Disposition filename parameter");
             }
-            int n = dispoHeader.indexOf("filename=");
-            if (n == -1) {
-                throw unchecked(new IOException("Bad Content-Disposition type"));
+            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);
+            }
+
+            // strip all but the last path segment
+            int x = filenameParam.lastIndexOf("/");
+            if (x != -1) {
+                filenameParam = filenameParam.substring(x+1);
             }
-            int lastsemi = dispoHeader.lastIndexOf(';');
-            String disposition;
-            if (lastsemi < n) {
-                disposition = dispoHeader.substring(n + 9);
-            } else {
-                disposition = dispoHeader.substring(n + 9, lastsemi);
+            x = filenameParam.lastIndexOf("\\");
+            if (x != -1) {
+                filenameParam = filenameParam.substring(x+1);
             }
-            Path file = Paths.get(directory.toString(), disposition);
+
+            filenameParam = filenameParam.trim();
+
+            if (filenameParam.startsWith("\"")) {  // quoted-string
+                if (!filenameParam.endsWith("\"") || filenameParam.length() == 1) {
+                    throw unchecked(statusCode, headers,
+                            "Badly quoted Content-Disposition filename parameter");
+                }
+                filenameParam = filenameParam.substring(1, filenameParam.length() -1 );
+            } else {  // token,
+                if (filenameParam.contains(" ")) {  // space disallowed
+                    throw unchecked(statusCode, headers,
+                            "unquoted space in Content-Disposition filename parameter");
+                }
+            }
+
+            if (PROHIBITED.contains(filenameParam)) {
+                throw unchecked(statusCode, headers,
+                        "Prohibited Content-Disposition filename parameter:"
+                                + filenameParam);
+            }
+
+            Path file = Paths.get(directory.toString(), filenameParam);
+
+            if (!file.startsWith(directory)) {
+                throw unchecked(statusCode, headers,
+                        "Resulting file, " + file.toString() + ", outside of given directory");
+            }
 
             PathSubscriber bs = (PathSubscriber)asFileImpl(file, openOptions);
             bs.setAccessControlContext(acc);
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/jdk/java/net/httpclient/AsFileDownloadTest.java	Thu Feb 15 14:10:27 2018 +0000
@@ -0,0 +1,346 @@
+/*
+ * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @summary Basic test for asFileDownload
+ * @bug 8196965
+ * @modules java.base/sun.net.www.http
+ *          java.net.http/jdk.internal.net.http.common
+ *          java.net.http/jdk.internal.net.http.frame
+ *          java.net.http/jdk.internal.net.http.hpack
+ *          java.logging
+ *          jdk.httpserver
+ * @library /lib/testlibrary /test/lib http2/server
+ * @build Http2TestServer
+ * @build jdk.testlibrary.SimpleSSLContext
+ * @build jdk.test.lib.Platform
+ * @build jdk.test.lib.util.FileUtils
+ * @run testng/othervm AsFileDownloadTest
+ */
+
+import com.sun.net.httpserver.HttpExchange;
+import com.sun.net.httpserver.HttpHandler;
+import com.sun.net.httpserver.HttpServer;
+import com.sun.net.httpserver.HttpsConfigurator;
+import com.sun.net.httpserver.HttpsServer;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.io.UncheckedIOException;
+import java.net.InetSocketAddress;
+import java.net.URI;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import javax.net.ssl.SSLContext;
+import java.net.http.HttpClient;
+import java.net.http.HttpRequest;
+import java.net.http.HttpResponse;
+import java.net.http.HttpResponse.BodyHandler;
+import jdk.testlibrary.SimpleSSLContext;
+import jdk.test.lib.util.FileUtils;
+import org.testng.annotations.AfterTest;
+import org.testng.annotations.BeforeTest;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+import static java.lang.System.out;
+import static java.net.http.HttpRequest.BodyPublisher.fromString;
+import static java.net.http.HttpResponse.BodyHandler.asFileDownload;
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.nio.file.StandardOpenOption.*;
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.fail;
+
+public class AsFileDownloadTest {
+
+    SSLContext sslContext;
+    HttpServer httpTestServer;         // HTTP/1.1    [ 4 servers ]
+    HttpsServer httpsTestServer;       // HTTPS/1.1
+    Http2TestServer http2TestServer;   // HTTP/2 ( h2c )
+    Http2TestServer https2TestServer;  // HTTP/2 ( h2  )
+    String httpURI;
+    String httpsURI;
+    String http2URI;
+    String https2URI;
+
+    Path tempDir;
+
+    static final String[][] contentDispositionValues = new String[][] {
+          // URI query     Content-Type header value         Expected filename
+            { "001", "Attachment; filename=example001.html", "example001.html" },
+            { "002", "attachment; filename=example002.html", "example002.html" },
+            { "003", "ATTACHMENT; filename=example003.html", "example003.html" },
+            { "004", "attAChment; filename=example004.html", "example004.html" },
+            { "005", "attachmeNt; filename=example005.html", "example005.html" },
+
+            { "006", "attachment; Filename=example006.html", "example006.html" },
+            { "007", "attachment; FILENAME=example007.html", "example007.html" },
+            { "008", "attachment; fileName=example008.html", "example008.html" },
+            { "009", "attachment; fIlEnAMe=example009.html", "example009.html" },
+
+            { "010", "attachment; filename=Example010.html", "Example010.html" },
+            { "011", "attachment; filename=EXAMPLE011.html", "EXAMPLE011.html" },
+            { "012", "attachment; filename=eXample012.html", "eXample012.html" },
+            { "013", "attachment; filename=example013.HTML", "example013.HTML" },
+            { "014", "attachment; filename  =eXaMpLe014.HtMl", "eXaMpLe014.HtMl"},
+
+            { "015", "attachment; filename=a",               "a"               },
+            { "016", "attachment; filename= b",              "b"               },
+            { "017", "attachment; filename=  c",             "c"               },
+            { "018", "attachment; filename=    d",           "d"               },
+            { "019", "attachment; filename=e  ; filename*=utf-8''eee.txt",  "e"},
+            { "020", "attachment; filename*=utf-8''fff.txt; filename=f",    "f"},
+            { "021", "attachment;  filename=g",              "g"               },
+            { "022", "attachment;   filename= h",            "h"               },
+
+            { "023", "attachment; filename=\"space name\"",                       "space name" },
+            { "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" },
+    };
+
+    @DataProvider(name = "positive")
+    public Object[][] positive() {
+        List<Object[]> list = new ArrayList<>();
+
+        Arrays.asList(contentDispositionValues).stream()
+                .map(e -> new Object[] {httpURI +  "?" + e[0], e[1], e[2]})
+                .forEach(list::add);
+        Arrays.asList(contentDispositionValues).stream()
+                .map(e -> new Object[] {httpsURI +  "?" + e[0], e[1], e[2]})
+                .forEach(list::add);
+        Arrays.asList(contentDispositionValues).stream()
+                .map(e -> new Object[] {http2URI +  "?" + e[0], e[1], e[2]})
+                .forEach(list::add);
+        Arrays.asList(contentDispositionValues).stream()
+                .map(e -> new Object[] {https2URI +  "?" + e[0], e[1], e[2]})
+                .forEach(list::add);
+
+        return list.stream().toArray(Object[][]::new);
+    }
+
+    @Test(dataProvider = "positive")
+    void test(String uriString, String contentDispositionValue, String expectedFilename)
+        throws Exception
+    {
+        HttpClient client = HttpClient.newBuilder().sslContext(sslContext).build();
+
+        URI uri = URI.create(uriString);
+        HttpRequest request = HttpRequest.newBuilder(uri)
+                .POST(fromString("May the luck of the Irish be with you!"))
+                .build();
+
+        BodyHandler bh = asFileDownload(tempDir.resolve(uri.getPath().substring(1)),
+                                        CREATE, TRUNCATE_EXISTING, WRITE);
+        HttpResponse<Path> response = client.send(request, bh);
+
+        out.println("Got response: " + response);
+        out.println("Got body Path: " + response.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);
+        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!");
+    }
+
+    // --- Negative
+
+    static final String[][] contentDispositionBADValues = new String[][] {
+            // URI query     Content-Type header value
+            { "100", ""                                    },  // empty
+            { "101", "filename=example.html"               },  // no attachment
+            { "102", "attachment; filename=space name"     },  // unquoted with space
+            { "103", "attachment; filename="               },  // empty filename param
+            { "104", "attachment; filename=\""             },  // single quote
+            { "105", "attachment; filename=\"\""           },  // empty quoted
+            { "106", "attachment; filename=."              },  // dot
+            { "107", "attachment; filename=.."             },  // dot dot
+            { "108", "attachment; filename=\".."           },  // badly quoted dot dot
+            { "109", "attachment; filename=\"..\""         },  // quoted dot dot
+            { "110", "attachment; filename=\"bad"          },  // badly quoted
+            { "111", "attachment; filename=\"bad;"         },  // badly quoted with ';'
+            { "112", "attachment; filename=\"bad ;"        },  // badly quoted with ' ;'
+            { "113", "attachment; filename*=utf-8''xx.txt "},  // no "filename" param
+    };
+
+    @DataProvider(name = "negative")
+    public Object[][] negative() {
+        List<Object[]> list = new ArrayList<>();
+
+        Arrays.asList(contentDispositionBADValues).stream()
+                .map(e -> new Object[] {httpURI +  "?" + e[0], e[1]})
+                .forEach(list::add);
+        Arrays.asList(contentDispositionBADValues).stream()
+                .map(e -> new Object[] {httpsURI +  "?" + e[0], e[1]})
+                .forEach(list::add);
+        Arrays.asList(contentDispositionBADValues).stream()
+                .map(e -> new Object[] {http2URI +  "?" + e[0], e[1]})
+                .forEach(list::add);
+        Arrays.asList(contentDispositionBADValues).stream()
+                .map(e -> new Object[] {https2URI +  "?" + e[0], e[1]})
+                .forEach(list::add);
+
+        return list.stream().toArray(Object[][]::new);
+    }
+
+    @Test(dataProvider = "negative")
+    void negativeTest(String uriString, String contentDispositionValue)
+            throws Exception
+    {
+        HttpClient client = HttpClient.newBuilder().sslContext(sslContext).build();
+
+        URI uri = URI.create(uriString);
+        HttpRequest request = HttpRequest.newBuilder(uri)
+                .POST(fromString("Does not matter"))
+                .build();
+
+        BodyHandler bh = asFileDownload(tempDir, CREATE, TRUNCATE_EXISTING, WRITE);
+        try {
+            HttpResponse<Path> response = client.send(request, bh);
+            fail("UNEXPECTED response: " + response + ", path:" + response.body());
+        } catch (UncheckedIOException | IOException ioe) {
+            System.out.println("Caught expected: " + ioe);
+        }
+    }
+
+    // -- Infrastructure
+
+    @BeforeTest
+    public void setup() throws Exception {
+        tempDir = Paths.get("asFileDownloadTest.tmp.dir");
+        FileUtils.deleteFileIfExistsWithRetry(tempDir);
+        Files.createDirectory(tempDir);
+        // Unique dirs per test run, based on the URI path
+        Files.createDirectories(tempDir.resolve("http1/afdt/"));
+        Files.createDirectories(tempDir.resolve("https1/afdt/"));
+        Files.createDirectories(tempDir.resolve("http2/afdt/"));
+        Files.createDirectories(tempDir.resolve("https2/afdt/"));
+
+        sslContext = new SimpleSSLContext().get();
+        if (sslContext == null)
+            throw new AssertionError("Unexpected null sslContext");
+
+        InetSocketAddress sa = new InetSocketAddress(0);
+        httpTestServer = HttpServer.create(sa, 0);
+        httpTestServer.createContext("/http1/afdt", new Http1FileDispoHandler());
+        httpURI = "http://127.0.0.1:" + httpTestServer.getAddress().getPort() + "/http1/afdt";
+
+        httpsTestServer = HttpsServer.create(sa, 0);
+        httpsTestServer.setHttpsConfigurator(new HttpsConfigurator(sslContext));
+        httpsTestServer.createContext("/https1/afdt", new Http1FileDispoHandler());
+        httpsURI = "https://127.0.0.1:" + httpsTestServer.getAddress().getPort() + "/https1/afdt";
+
+        http2TestServer = new Http2TestServer("127.0.0.1", false, 0);
+        http2TestServer.addHandler(new Http2FileDispoHandler(), "/http2/afdt");
+        int port = http2TestServer.getAddress().getPort();
+        http2URI = "http://127.0.0.1:" + port + "/http2/afdt";
+
+        https2TestServer = new Http2TestServer("127.0.0.1", true, 0);
+        https2TestServer.addHandler(new Http2FileDispoHandler(), "/https2/afdt");
+        port = https2TestServer.getAddress().getPort();
+        https2URI = "https://127.0.0.1:" + port + "/https2/afdt";
+
+        httpTestServer.start();
+        httpsTestServer.start();
+        http2TestServer.start();
+        https2TestServer.start();
+    }
+
+    @AfterTest
+    public void teardown() throws Exception {
+        httpTestServer.stop(0);
+        httpsTestServer.stop(0);
+        http2TestServer.stop();
+        https2TestServer.stop();
+    }
+
+    static String contentDispositionValueFromURI(URI uri) {
+        String queryIndex = uri.getQuery();
+        String[][] values;
+        if (queryIndex.startsWith("0"))  // positive tests start with '0'
+            values = contentDispositionValues;
+        else if (queryIndex.startsWith("1"))  // negative tests start with '1'
+            values = contentDispositionBADValues;
+        else
+            throw new AssertionError("SERVER: UNEXPECTED query:" + queryIndex);
+
+        return Arrays.asList(values).stream()
+                .filter(e -> e[0].equals(queryIndex))
+                .map(e -> e[1])
+                .findFirst()
+                .orElseThrow();
+    }
+
+    static class Http1FileDispoHandler implements HttpHandler {
+        @Override
+        public void handle(HttpExchange t) throws IOException {
+            try (InputStream is = t.getRequestBody();
+                 OutputStream os = t.getResponseBody()) {
+                byte[] bytes = is.readAllBytes();
+
+                t.getResponseHeaders().set("Content-Disposition",
+                        contentDispositionValueFromURI(t.getRequestURI()));
+
+                t.sendResponseHeaders(200, bytes.length);
+                os.write(bytes);
+            }
+        }
+    }
+
+    static class Http2FileDispoHandler implements Http2Handler {
+        @Override
+        public void handle(Http2TestExchange t) throws IOException {
+            try (InputStream is = t.getRequestBody();
+                 OutputStream os = t.getResponseBody()) {
+                byte[] bytes = is.readAllBytes();
+
+                t.getResponseHeaders().addHeader("Content-Disposition",
+                        contentDispositionValueFromURI(t.getRequestURI()));
+
+                t.sendResponseHeaders(200, bytes.length);
+                os.write(bytes);
+            }
+        }
+    }
+}
--- a/test/jdk/java/net/httpclient/http2/server/Http2TestServerConnection.java	Thu Feb 15 12:31:14 2018 +0000
+++ b/test/jdk/java/net/httpclient/http2/server/Http2TestServerConnection.java	Thu Feb 15 14:10:27 2018 +0000
@@ -473,7 +473,7 @@
         if (!tokens[2].equals("HTTP/1.1")) {
             throw new IOException("bad request line");
         }
-        URI uri = null;
+        URI uri;
         try {
             uri = new URI(tokens[1]);
         } catch (URISyntaxException e) {
@@ -487,7 +487,11 @@
         headers.setHeader(":method", tokens[0]);
         headers.setHeader(":scheme", "http"); // always in this case
         headers.setHeader(":authority", host);
-        headers.setHeader(":path", uri.getPath());
+        String path = uri.getPath();
+        if (uri.getQuery() != null)
+            path = path + "?" + uri.getQuery();
+        headers.setHeader(":path", path);
+
         Queue q = new Queue(sentinel);
         byte[] body = getRequestBody(request);
         addHeaders(getHeaders(request.headers), headers);