Merge pull request #21880 from vespa-engine/mpolden/disallow-relative-path

Disallow relative paths and specs

2 files changed, 53 insertions, 37 deletions

diff --git a/container-core/src/main/java/com/yahoo/restapi/Path.java b/container-core/src/main/java/com/yahoo/restapi/Path.java
index 45312f8ec15..b96488c6781 100644
--- a/container-core/src/main/java/com/yahoo/restapi/Path.java
+++ b/container-core/src/main/java/com/yahoo/restapi/Path.java
@@ -3,15 +3,16 @@ package com.yahoo.restapi;
 
 import java.net.URI;
 import java.net.URLDecoder;
-import java.net.URLEncoder;
 import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Objects;
+import java.util.function.Function;
 import java.util.stream.Stream;
 
 /**
- * A path which is able to match strings containing bracketed placeholders and return the
+ * A normalized path which is able to match strings containing bracketed placeholders and return the
  * values given at the placeholders. The path is split on '/', and each part is then URL decoded.
  * 
  * E.g a path /a/1/bar/fuz/baz/%62%2f
@@ -23,11 +24,8 @@ import java.util.stream.Stream;
  * If the path spec ends with /{*}, it will match urls with any rest path.
  * The rest path (not including the trailing slash) will be available as getRest().
  * 
- * Note that for convenience in common use this has state which changes as a side effect of each matches
- * invocation. It is therefore for single thread use.
- *
- * An optional prefix can be used to match the path spec against an alternative path. This
- * is used when you have alternative paths mapped to the same resource.
+ * Note that for convenience in common use this has state which changes as a side effect of each
+ * {@link Path#matches(String)} invocation. It is therefore for single thread use.
  *
  * @author bratseth
  */
@@ -35,7 +33,6 @@ public class Path {
 
     // This path
     private final String pathString;
-    private final String optionalPrefix;
     private final String[] elements;
 
     // Info about the last match
@@ -43,21 +40,13 @@ public class Path {
     private String rest = "";
 
     public Path(URI uri) {
-        this(uri, "");
-    }
-
-    // TODO (freva): Remove, used by factory
-    public Path(URI uri, String optionalPrefix) {
-        this.optionalPrefix = optionalPrefix;
-        this.pathString = uri.getRawPath();
-        this.elements = Stream.of(this.pathString.split("/"))
-                              .map(part -> URLDecoder.decode(part, StandardCharsets.UTF_8))
-                              .toArray(String[]::new);
+        this.pathString = requireNormalized(uri).getRawPath();
+        this.elements = splitAbsolutePath(pathString, (part) -> URLDecoder.decode(part, StandardCharsets.UTF_8));
     }
 
     private boolean matchesInner(String pathSpec) {
         values.clear();
-        String[] specElements = pathSpec.split("/");
+        String[] specElements = splitAbsolutePath(pathSpec, Function.identity());
         boolean matchPrefix = false;
         if (specElements.length > 1 && specElements[specElements.length-1].equals("{*}")) {
             matchPrefix = true;
@@ -99,13 +88,11 @@ public class Path {
      *
      * This will NOT match empty path elements.
      *
-     * @param pathSpec the path string to match to this
+     * @param pathSpec the literal path string to match to this
      * @return true if the string matches, false otherwise
      */
     public boolean matches(String pathSpec) {
-        if (matchesInner(pathSpec)) return true;
-        if (optionalPrefix.isEmpty()) return false;
-        return matchesInner(optionalPrefix + pathSpec);
+        return matchesInner(pathSpec);
     }
 
     /**
@@ -130,5 +117,21 @@ public class Path {
     public String toString() {
         return "path '" + String.join("/", elements) + "'";
     }
+
+    private static URI requireNormalized(URI uri) {
+        Objects.requireNonNull(uri);
+        if (!uri.normalize().equals(uri)) throw new IllegalArgumentException("Expected normalized URI, got '" + uri + "'");
+        return uri;
+    }
+
+    private static String[] splitAbsolutePath(String path, Function<String, String> partParser) {
+        String[] parts = Stream.of(path.split("/"))
+                               .map(partParser)
+                               .toArray(String[]::new);
+        for (var part : parts) {
+            if (part.equals("..")) throw new IllegalArgumentException("Expected absolute path, got '" + path + "'");
+        }
+        return parts;
+    }
     
 }
diff --git a/container-core/src/test/java/com/yahoo/restapi/PathTest.java b/container-core/src/test/java/com/yahoo/restapi/PathTest.java
index 865c8976c56..5cbf80ff2ad 100644
--- a/container-core/src/test/java/com/yahoo/restapi/PathTest.java
+++ b/container-core/src/test/java/com/yahoo/restapi/PathTest.java
@@ -8,6 +8,7 @@ import java.net.URI;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
 
 /**
  * @author bratseth
@@ -15,19 +16,6 @@ import static org.junit.Assert.assertEquals;
 public class PathTest {
 
     @Test
-    public void testWithPrefix() {
-        // Test that a path with a prefix matches spec without the prefix
-        Path path = new Path(URI.create("/ball/a/1/bar/fuz"), "/ball");
-        assertTrue(path.matches("/a/{foo}/bar/{b}"));
-        assertEquals("1", path.get("foo"));
-        assertEquals("fuz", path.get("b"));
-
-        // Also test that prefix does not cause false matches
-        assertFalse(path.matches("/ball/a/{foo}/zoo/{b}"));
-    }
-
-
-    @Test
     public void testPath() {
         assertFalse(new Path(URI.create("")).matches("/a/{foo}/bar/{b}"));
         assertFalse(new Path(URI.create("///")).matches("/a/{foo}/bar/{b}"));
@@ -77,8 +65,8 @@ public class PathTest {
     @Test
     public void testUrlEncodedPath() {
         assertTrue(new Path(URI.create("/a/%62/c")).matches("/a/b/c"));
-        assertTrue(new Path(URI.create("/a/%2e%2e/c")).matches("/a/../c"));
         assertFalse(new Path(URI.create("/a/b%2fc")).matches("/a/b/c"));
+        assertFalse(new Path(URI.create("/foo")).matches("/foo/bar/%2e%2e"));
 
         Path path = new Path(URI.create("/%61/%2f/%63"));
         assertTrue(path.matches("/a/{slash}/{c}"));
@@ -86,4 +74,29 @@ public class PathTest {
         assertEquals("c", path.get("c"));
     }
 
+    @Test
+    public void testInvalidPaths() {
+        assertInvalid(URI.create("/foo/../bar"));
+        assertInvalid(URI.create("/foo/%2e%2e/bar"));
+        assertInvalidPathSpec(URI.create("/foo/bar"), "/foo/bar/..");
+        assertInvalidPathSpec(URI.create("/foo/bar"), "/foo/../bar");
+    }
+
+    private void assertInvalid(URI uri) {
+        try {
+            new Path(uri);
+            fail("Expected exception");
+        } catch (IllegalArgumentException ignored) {
+        }
+    }
+
+    private void assertInvalidPathSpec(URI uri, String pathSpec) {
+        try {
+            Path path = new Path(uri);
+            path.matches(pathSpec);
+            fail("Expected exception");
+        } catch (IllegalArgumentException ignored) {
+        }
+    }
+
 }