diff options
6 files changed, 39 insertions, 55 deletions
diff --git a/container-core/src/main/java/com/yahoo/restapi/HttpURL.java b/container-core/src/main/java/com/yahoo/restapi/HttpURL.java index a876aeea6b5..c4cc575940d 100644 --- a/container-core/src/main/java/com/yahoo/restapi/HttpURL.java +++ b/container-core/src/main/java/com/yahoo/restapi/HttpURL.java @@ -151,10 +151,10 @@ public class HttpURL { } /** Require that the given string (possibly decoded multiple times) contains no {@code '/'}, and isn't either of {@code "", ".", ".."}. */ - public static void requirePathSegment(String value) { + public static String requirePathSegment(String value) { while ( ! value.equals(value = decode(value, UTF_8))); require( ! value.contains("/"), value, "path segment decoded cannot contain '/'"); - Path.requireNonNormalizable(value); + return Path.requireNonNormalizable(value); } private static void requireNothing(String value) { } 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 e80d2f81693..8a494d47be4 100644 --- a/container-core/src/main/java/com/yahoo/restapi/Path.java +++ b/container-core/src/main/java/com/yahoo/restapi/Path.java @@ -2,14 +2,10 @@ package com.yahoo.restapi; import java.net.URI; -import java.net.URLDecoder; -import java.nio.charset.StandardCharsets; -import java.util.Arrays; import java.util.HashMap; +import java.util.List; import java.util.Map; -import java.util.Objects; -import java.util.function.Function; -import java.util.stream.Stream; +import java.util.function.Consumer; /** * A normalized path which is able to match strings containing bracketed placeholders and return the @@ -32,48 +28,47 @@ import java.util.stream.Stream; public class Path { // This path - private final String pathString; - private final String[] elements; + private final HttpURL.Path path; // Info about the last match private final Map<String, String> values = new HashMap<>(); - private String rest = ""; + private HttpURL.Path rest; + /** Creates a new Path for matching the given URI against patterns, which uses {@link HttpURL#requirePathSegment} as a segment validator. */ public Path(URI uri) { - this.pathString = requireNormalized(uri).getRawPath(); - this.elements = splitAbsolutePath(pathString, (part) -> URLDecoder.decode(part, StandardCharsets.UTF_8)); + this.path = HttpURL.Path.parse(uri.getRawPath()); + } + + /** Creates a new Path for matching the given URI against patterns, with the given path segment validator. */ + public Path(URI uri, Consumer<String> validator) { + this.path = HttpURL.Path.parse(uri.getRawPath(), validator); } private boolean matchesInner(String pathSpec) { values.clear(); - String[] specElements = splitAbsolutePath(pathSpec, Function.identity()); + List<String> specElements = HttpURL.Path.parse(pathSpec).segments(); boolean matchPrefix = false; - if (specElements.length > 1 && specElements[specElements.length-1].equals("{*}")) { + if (specElements.size() > 1 && specElements.get(specElements.size() - 1).equals("{*}")) { matchPrefix = true; - specElements = Arrays.copyOf(specElements, specElements.length-1); + specElements = specElements.subList(0, specElements.size() - 1); } if (matchPrefix) { - if (this.elements.length < specElements.length) return false; + if (path.segments().size() < specElements.size()) return false; } else { // match exact - if (this.elements.length != specElements.length) return false; + if (path.segments().size() != specElements.size()) return false; } - for (int i = 0; i < specElements.length; i++) { - if (specElements[i].startsWith("{") && specElements[i].endsWith("}")) // placeholder - values.put(specElements[i].substring(1, specElements[i].length()-1), elements[i]); - else if ( ! specElements[i].equals(this.elements[i])) + for (int i = 0; i < specElements.size(); i++) { + if (specElements.get(i).startsWith("{") && specElements.get(i).endsWith("}")) // placeholder + values.put(specElements.get(i).substring(1, specElements.get(i).length() - 1), path.segments().get(i)); + else if ( ! specElements.get(i).equals(path.segments().get(i))) return false; } if (matchPrefix) { - StringBuilder rest = new StringBuilder(); - for (int i = specElements.length; i < this.elements.length; i++) - rest.append(elements[i]).append("/"); - if ( ! pathString.endsWith("/") && rest.length() > 0) - rest.setLength(rest.length() - 1); - this.rest = rest.toString(); + this.rest = path.skip(specElements.size()); } return true; @@ -107,27 +102,14 @@ public class Path { * Returns the rest of the last matched path. * This is always the empty string (never null) unless the path spec ends with {*} */ - public String getRest() { return rest; } + public String getRest() { + String raw = rest.raw(); + return raw.isEmpty() ? raw : raw.substring(1); + } @Override public String toString() { - return "path '" + String.join("/", elements) + "'"; + return path.toString(); } - 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 5cbf80ff2ad..065b02be0e4 100644 --- a/container-core/src/test/java/com/yahoo/restapi/PathTest.java +++ b/container-core/src/test/java/com/yahoo/restapi/PathTest.java @@ -6,6 +6,7 @@ import org.junit.Test; import java.net.URI; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.assertEquals; import static org.junit.Assert.fail; @@ -65,10 +66,12 @@ public class PathTest { @Test public void testUrlEncodedPath() { assertTrue(new Path(URI.create("/a/%62/c")).matches("/a/b/c")); - assertFalse(new Path(URI.create("/a/b%2fc")).matches("/a/b/c")); - assertFalse(new Path(URI.create("/foo")).matches("/foo/bar/%2e%2e")); + assertFalse(new Path(URI.create("/a/b%2fc"), __ -> { }).matches("/a/b/c")); + assertThrows("path segments cannot be \"\", \".\", or \"..\", but got: '..'", + IllegalArgumentException.class, + () -> new Path(URI.create("/foo")).matches("/foo/bar/%2e%2e")); - Path path = new Path(URI.create("/%61/%2f/%63")); + Path path = new Path(URI.create("/%61/%2f/%63"), __ -> { }); assertTrue(path.matches("/a/{slash}/{c}")); assertEquals("/", path.get("slash")); assertEquals("c", path.get("c")); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/AthenzRoleFilter.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/AthenzRoleFilter.java index 21e803800f5..a7472ced09c 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/AthenzRoleFilter.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/AthenzRoleFilter.java @@ -4,12 +4,10 @@ package com.yahoo.vespa.hosted.controller.restapi.filter; import com.auth0.jwt.JWT; import com.google.inject.Inject; import com.yahoo.config.provision.ApplicationName; -import com.yahoo.config.provision.Environment; -import com.yahoo.config.provision.RegionName; import com.yahoo.config.provision.SystemName; import com.yahoo.config.provision.TenantName; -import com.yahoo.config.provision.Zone; import com.yahoo.config.provision.zone.ZoneId; +import com.yahoo.jdisc.Response; import com.yahoo.jdisc.http.filter.DiscFilterRequest; import com.yahoo.jdisc.http.filter.security.base.JsonSecurityRequestFilterBase; import com.yahoo.restapi.Path; @@ -90,6 +88,7 @@ public class AthenzRoleFilter extends JsonSecurityRequestFilterBase { } catch (Exception e) { logger.log(Level.INFO, () -> "Exception mapping Athenz principal to roles: " + Exceptions.toMessageString(e)); + return Optional.of(new ErrorResponse(Response.Status.FORBIDDEN, "Access denied")); } return Optional.empty(); } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java index f94f87b0f46..581c0160640 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java @@ -502,7 +502,7 @@ public class ApplicationApiTest extends ControllerContainerTest { // Get content/../foo tester.assertResponse(request("/application/v4/tenant/tenant2/application/application1/instance/default/environment/dev/region/us-east-1/content/%2E%2E%2Ffoo", GET).userIdentity(USER_ID), - "{\"error-code\":\"FORBIDDEN\",\"message\":\"Access denied\"}", 403); + accessDenied, 403); // Get content - root tester.assertResponse(request("/application/v4/tenant/tenant2/application/application1/instance/default/environment/dev/region/us-east-1/content/", GET).userIdentity(USER_ID), "{\"path\":\"/\"}"); @@ -1671,7 +1671,7 @@ public class ApplicationApiTest extends ControllerContainerTest { tester.assertResponse(request(serviceApi + "/storagenode-awe3slno6mmq2fye191y324jl/state%2Fv1%2F..%2F..%2Fdocument%2Fv1%2F", GET) .userIdentity(USER_ID) .oAuthCredentials(OKTA_CREDENTIALS), - "{\"error-code\":\"FORBIDDEN\",\"message\":\"Access denied\"}", + accessDenied, 403); } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/os/OsApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/os/OsApiTest.java index 7d17e97e66b..cf4deb7b4bf 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/os/OsApiTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/os/OsApiTest.java @@ -125,7 +125,7 @@ public class OsApiTest extends ControllerContainerTest { // Error: Cancel firmware checks in an empty set of zones. assertResponse(new Request("http://localhost:8080/os/v1/firmware/dev/", "", Request.Method.DELETE), - "{\"error-code\":\"NOT_FOUND\",\"message\":\"No zones at path '/os/v1/firmware/dev'\"}", 404); + "{\"error-code\":\"NOT_FOUND\",\"message\":\"No zones at path '/os/v1/firmware/dev/'\"}", 404); assertFalse("Actions are logged to audit log", tester.controller().auditLogger().readLog().entries().isEmpty()); } |