diff --git a/src/main/java/com/checkout/AbstractClient.java b/src/main/java/com/checkout/AbstractClient.java index 774fcb10..24b2a27a 100644 --- a/src/main/java/com/checkout/AbstractClient.java +++ b/src/main/java/com/checkout/AbstractClient.java @@ -8,7 +8,8 @@ public abstract class AbstractClient { private static final char[] HEX = "0123456789ABCDEF".toCharArray(); private static final String INVALID_PATH_PARAMETER = - "Invalid path parameter: it must be valid text, not null, empty, '.' or '..'"; + "Invalid path parameter: it must be valid text, not null, empty, '.' or '..', " + + "and must not contain '/', '\\' or ASCII control characters"; protected final ApiClient apiClient; protected final CheckoutConfiguration configuration; @@ -37,8 +38,9 @@ protected SdkAuthorization sdkAuthorization() { * * @param pathParams the path segments, in order * @return the request path, relative to the base URI - * @throws CheckoutArgumentException if a parameter is null, empty, "." or "..", or is not valid - * UTF-16 text (an unpaired surrogate) + * @throws CheckoutArgumentException if a parameter is null, empty, "." or "..", contains "/", "\\" or + * an ASCII control character (U+0000 to U+001F, U+007F), or is not + * valid UTF-16 text (an unpaired surrogate) */ protected static String buildPath(final String... pathParams) { final StringBuilder path = new StringBuilder(); @@ -53,11 +55,13 @@ protected static String buildPath(final String... pathParams) { /** * Percent-encodes a path parameter as UTF-8, keeping only the RFC 3986 unreserved characters - * (letters, digits, "-", ".", "_" and "~"), so the result is exactly one path segment. + * (letters, digits, "-", ".", "_" and "~"), so the result is exactly one path segment. "/", "\\" and + * ASCII control characters are refused rather than encoded: the API decodes "%2F" and "%5C" back + * into separators, so an encoded value could still reach another endpoint. */ private static String encodePathSegment(final String pathParam) { if (pathParam == null || pathParam.isEmpty() || ".".equals(pathParam) || "..".equals(pathParam) - || hasUnpairedSurrogate(pathParam)) { + || hasForbiddenCharacter(pathParam) || hasUnpairedSurrogate(pathParam)) { throw new CheckoutArgumentException(INVALID_PATH_PARAMETER); } final StringBuilder encoded = new StringBuilder(pathParam.length()); @@ -77,6 +81,16 @@ private static boolean isUnreserved(final int c) { || c == '-' || c == '.' || c == '_' || c == '~'; } + private static boolean hasForbiddenCharacter(final String value) { + for (int i = 0; i < value.length(); i++) { + final char c = value.charAt(i); + if (c == '/' || c == '\\' || c <= 0x1F || c == 0x7F) { + return true; + } + } + return false; + } + private static boolean hasUnpairedSurrogate(final String value) { for (int i = 0; i < value.length(); i++) { final char c = value.charAt(i); diff --git a/src/main/java/com/checkout/ApacheHttpClientTransport.java b/src/main/java/com/checkout/ApacheHttpClientTransport.java index 774cf9b1..eab3fe58 100644 --- a/src/main/java/com/checkout/ApacheHttpClientTransport.java +++ b/src/main/java/com/checkout/ApacheHttpClientTransport.java @@ -64,9 +64,15 @@ class ApacheHttpClientTransport implements Transport { private static final String PURPOSE = "purpose"; private static final String PATH = "path"; private static final String INVALID_REQUEST_PATH = "Invalid request path"; - private static final Pattern ENCODED_QUERY = Pattern.compile("[A-Za-z0-9._~*+%=&-]+"); + private static final String INVALID_BASE_URI = "Invalid base URI: it must be an absolute http or https URI with a host"; + private static final Pattern ENCODED_QUERY = Pattern.compile("(?:[A-Za-z0-9._~*+=&-]|%[0-9A-Fa-f]{2})+"); + private static final Pattern ENCODED_DOT = Pattern.compile("%2e", Pattern.CASE_INSENSITIVE); + private static final Pattern ENCODED_SEGMENT = Pattern.compile("(?:[A-Za-z0-9._~-]|%[0-9A-Fa-f]{2})+"); + private static final Pattern ENCODED_SEPARATOR_OR_CONTROL = + Pattern.compile("%(?:2F|5C|[01][0-9A-F]|7F)", Pattern.CASE_INSENSITIVE); private final URI baseUri; + private final String baseDirectory; private final CloseableHttpClient httpClient; private final Executor executor; private final TransportConfiguration transportConfiguration; @@ -83,7 +89,8 @@ class ApacheHttpClientTransport implements Transport { final CheckoutConfiguration configuration ) { CheckoutUtils.validateParams("baseUri", baseUri, "httpClientBuilder", httpClientBuilder, "executor", executor); - this.baseUri = withRootPath(baseUri); + this.baseUri = normalisedBaseUri(baseUri); + this.baseDirectory = directoryOf(this.baseUri); this.httpClient = httpClientBuilder .setRedirectStrategy(new CustomAwsRedirectStrategy()) .build(); @@ -386,9 +393,11 @@ private HttpUriRequest buildRequest(final ClientOperation clientOperation, final /** * Resolves a request path against the base URI. The path must be relative and made of non-empty - * segments, none of them "." or ".." (also when percent-encoded or followed by ";"). It may end with - * an already encoded query ("?name=value&..." made only of unreserved characters, "%", "+", "=" - * and "&"), and never with a "#". The resolved URL must keep the scheme, host, port and base path + * segments of unreserved characters and "%XX" escapes, none of them "." or ".." (also when + * percent-encoded) and none containing an encoded "/", "\\" or ASCII control character, which the + * API would decode back into a separator. It may end with + * an already encoded query ("?name=value&..." made only of unreserved characters, "*", "+", "=", + * "&" and "%" followed by two hex digits), and never with a "#". The resolved URL must keep the scheme, host, port and base path * of the base URI. * * @param path the request path, as built by {@code AbstractClient.buildPath} @@ -409,12 +418,27 @@ String getRequestUrl(final String path) { } /** - * A base URI with no path ("https://api.example.com") resolves "payments" to the host + * Checks that the base URI is absolute http or https with an authority, and normalises its path. A + * base URI with no path ("https://api.example.com") resolves "payments" to the host * "api.example.compayments"; the root path "/" keeps the request on the configured host. */ - private static URI withRootPath(final URI baseUri) { + private static URI normalisedBaseUri(final URI baseUri) { + final String scheme = baseUri.getScheme(); + if (scheme == null || baseUri.isOpaque() || baseUri.getRawAuthority() == null + || !("http".equalsIgnoreCase(scheme) || "https".equalsIgnoreCase(scheme))) { + throw new CheckoutArgumentException(INVALID_BASE_URI); + } final String path = baseUri.getRawPath(); - return baseUri.isAbsolute() && !baseUri.isOpaque() && (path == null || path.isEmpty()) ? baseUri.resolve("/") : baseUri; + return path == null || path.isEmpty() ? baseUri.resolve("/") : baseUri.normalize(); + } + + /** + * The part of the base path that {@link URI#resolve(String)} keeps, read from a resolved probe so it + * matches resolve exactly (it drops the last segment and collapses empty ones). + */ + private static String directoryOf(final URI baseUri) { + final String probe = baseUri.resolve("probe").getRawPath(); + return probe.substring(0, probe.length() - "probe".length()); } private static void validateRequestPath(final String path) { @@ -427,7 +451,8 @@ private static void validateRequestPath(final String path) { } final String pathOnly = queryStart >= 0 ? path.substring(0, queryStart) : path; for (final String segment : pathOnly.split("/", -1)) { - if (isEmptyOrDotSegment(segment)) { + if (isEmptyOrDotSegment(segment) || !ENCODED_SEGMENT.matcher(segment).matches() + || ENCODED_SEPARATOR_OR_CONTROL.matcher(segment).find()) { throw new CheckoutArgumentException(INVALID_REQUEST_PATH); } } @@ -435,21 +460,18 @@ private static void validateRequestPath(final String path) { private static boolean isEmptyOrDotSegment(final String segment) { final int parameters = segment.indexOf(';'); - final String name = (parameters >= 0 ? segment.substring(0, parameters) : segment).replaceAll("(?i)%2e", "."); + final String name = ENCODED_DOT.matcher(parameters >= 0 ? segment.substring(0, parameters) : segment).replaceAll("."); return name.isEmpty() || ".".equals(name) || "..".equals(name); } /** - * True when the URL has the scheme, host and port of the base URI and a path under the base URI's - * directory, the part of its path that {@link URI#resolve(String)} keeps. + * True when the URL has the scheme and authority of the base URI and a path under the base URI's + * directory. The raw authority is compared because {@link URI#getHost()} is null for host names + * that are not valid DNS names, such as those with "_". */ private boolean isUnderBaseUri(final URI resolved) { - final String basePath = baseUri.getRawPath() == null ? "" : baseUri.getRawPath(); - final String baseDirectory = basePath.substring(0, basePath.lastIndexOf('/') + 1); return baseUri.getScheme().equalsIgnoreCase(resolved.getScheme()) - && baseUri.getHost() != null - && baseUri.getHost().equalsIgnoreCase(resolved.getHost()) - && baseUri.getPort() == resolved.getPort() + && baseUri.getRawAuthority().equalsIgnoreCase(resolved.getRawAuthority()) && resolved.getRawPath() != null && resolved.getRawPath().startsWith(baseDirectory) && resolved.getRawFragment() == null; diff --git a/src/test/java/com/checkout/AbstractClientBuildPathTest.java b/src/test/java/com/checkout/AbstractClientBuildPathTest.java index 02a85f76..225d08e1 100644 --- a/src/test/java/com/checkout/AbstractClientBuildPathTest.java +++ b/src/test/java/com/checkout/AbstractClientBuildPathTest.java @@ -17,15 +17,10 @@ class AbstractClientBuildPathTest { private static Stream encodedValues() { return Stream.of( Arguments.of("pay_123", "payments/pay_123"), - Arguments.of("pay_1/refunds", "payments/pay_1%2Frefunds"), - Arguments.of("../payments/pay_1", "payments/..%2Fpayments%2Fpay_1"), Arguments.of("pay_1?x=1", "payments/pay_1%3Fx%3D1"), Arguments.of("pay_1#x", "payments/pay_1%23x"), Arguments.of("pay_1%2Frefunds", "payments/pay_1%252Frefunds"), - Arguments.of("pay_1\\refunds", "payments/pay_1%5Crefunds"), Arguments.of("my secret", "payments/my%20secret"), - Arguments.of("pay_1\r\nX: y", "payments/pay_1%0D%0AX%3A%20y"), - Arguments.of("pay_1\u0000", "payments/pay_1%00"), Arguments.of("clé", "payments/cl%C3%A9"), Arguments.of("a.b-c_d~e", "payments/a.b-c_d~e"), Arguments.of("...", "payments/..."), @@ -46,9 +41,19 @@ void shouldKeepPlainPathsUnchanged() { @ParameterizedTest @NullSource - @ValueSource(strings = {"", ".", "..", "\ud800", "a\udc00b", "\ud83d"}) + @ValueSource(strings = {"", ".", "..", "\ud800", "a\udc00b", "\ud83d", + "pay_1/refunds", "../payments/pay_1", "pay_1\\refunds", "pay_1\r\nX: y", "pay_1\u0000", + "pay_1\t", "pay_1\u001f", "pay_1\u007f"}) void shouldRefuseInvalidPathParameters(final String value) { - assertThrows(CheckoutArgumentException.class, () -> AbstractClient.buildPath("payments", value, "refunds")); + final CheckoutArgumentException exception = assertThrows(CheckoutArgumentException.class, + () -> AbstractClient.buildPath("payments", value, "refunds")); + assertEquals("Invalid path parameter: it must be valid text, not null, empty, '.' or '..', " + + "and must not contain '/', '\\' or ASCII control characters", exception.getMessage()); + } + + @Test + void shouldKeepCharactersOutsideTheForbiddenSetEncoded() { + assertEquals("payments/pay_1%C2%80%20%25", AbstractClient.buildPath("payments", "pay_1\u0080 %")); } } diff --git a/src/test/java/com/checkout/ApacheHttpClientTransportRequestUrlTest.java b/src/test/java/com/checkout/ApacheHttpClientTransportRequestUrlTest.java index 6d4ffcf5..03dccdb2 100644 --- a/src/test/java/com/checkout/ApacheHttpClientTransportRequestUrlTest.java +++ b/src/test/java/com/checkout/ApacheHttpClientTransportRequestUrlTest.java @@ -89,7 +89,7 @@ void shouldResolveValidRequestPaths() { final ApacheHttpClientTransport transport = transport(SANDBOX); assertEquals("https://api.sandbox.checkout.com/payments/pay_123", transport.getRequestUrl(VALID_PATH)); - assertEquals("https://api.sandbox.checkout.com/payments/pay_1%2Frefunds", transport.getRequestUrl("payments/pay_1%2Frefunds")); + assertEquals("https://api.sandbox.checkout.com/payments/pay_1%252Frefunds", transport.getRequestUrl("payments/pay_1%252Frefunds")); assertEquals("https://api.sandbox.checkout.com/payments/...", transport.getRequestUrl("payments/...")); assertEquals("https://api.sandbox.checkout.com/ideal-external/issuers", transport.getRequestUrl("ideal-external/issuers")); assertEquals("https://api.sandbox.checkout.com/event-types?version=v2%26x%3D1%23y", @@ -109,6 +109,47 @@ void shouldResolveUnderCustomBaseUris() { transport(URI.create("https://proxy.example/checkout")).getRequestUrl(VALID_PATH)); } + @Test + void shouldResolveUnderBaseUrisWithUnderscoreHostsOrUnnormalisedPaths() { + assertEquals("https://proxy_host.example/payments/pay_123", + transport(URI.create("https://proxy_host.example/")).getRequestUrl(VALID_PATH)); + assertEquals("http://checkout_proxy:8080/api/payments/pay_123", + transport(URI.create("http://checkout_proxy:8080/api/")).getRequestUrl(VALID_PATH)); + assertEquals("https://proxy.example/b/payments/pay_123", + transport(URI.create("https://proxy.example/a/../b/")).getRequestUrl(VALID_PATH)); + assertEquals("https://proxy.example/payments/pay_123", + transport(URI.create("https://proxy.example//")).getRequestUrl(VALID_PATH)); + } + + @ParameterizedTest + @ValueSource(strings = {"api.example.com/", "/checkout/", "mailto:x@example.com", "https:///payments/"}) + void shouldRefuseBaseUrisThatAreNotAbsoluteWithAHost(final String baseUri) { + assertThrows(CheckoutArgumentException.class, () -> transport(URI.create(baseUri))); + } + + @Test + void shouldNotSendTheAuthorizationHeaderWhenFollowingARedirect() { + final List targetAuthorization = Collections.synchronizedList(new ArrayList<>()); + server.createContext("/redirect", exchange -> { + exchange.getResponseHeaders().add("Location", "/target"); + exchange.sendResponseHeaders(307, -1); + exchange.close(); + }); + server.createContext("/target", exchange -> { + targetAuthorization.add(String.valueOf(exchange.getRequestHeaders().getFirst("Authorization"))); + final byte[] body = "{}".getBytes(StandardCharsets.UTF_8); + exchange.getResponseHeaders().add("Content-Type", "application/json"); + exchange.sendResponseHeaders(200, body.length); + try (OutputStream out = exchange.getResponseBody()) { + out.write(body); + } + }); + + localTransport().invokeSync(ClientOperation.GET, "redirect", authorization, null, null, Collections.emptyMap()); + + assertEquals(Collections.singletonList("null"), targetAuthorization, "the redirect was followed without credentials"); + } + @ParameterizedTest @NullSource @ValueSource(strings = { @@ -127,6 +168,21 @@ void shouldResolveUnderCustomBaseUris() { "event-types?version=v2#x", "event-types?version=../x", "event-types?version=a?b", + "event-types?version=%zz", + "payments/%2F..%2Fworkflows", + "payments/a%2fb", + "payments/pay_1%2Frefunds", + "payments/%5C..%5Cx", + "payments/a%00b", + "payments/a%0D%0Ab", + "payments/a%7Fb", + "payments/a%zzb", + "payments/a%", + "p\\..\\x", + "payments/a b", + "payments/a:b", + "event-types?version=%2", + "event-types?version=v2%", "a?b/../c", "payments/../x?version=v2", "payments/pay_1#refunds",