fix(client): security hardening

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);

src/main/java/com/checkout/ApacheHttpClientTransport.java

@@ -64,9 +64,15 @@
     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 @@
             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 @@
 
     /**
      * 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 @@
     }
 
     /**
-     * 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,29 +451,27 @@
         }
         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);
             }
         }
     }
 
     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;

src/test/java/com/checkout/AbstractClientBuildPathTest.java

@@ -17,15 +17,10 @@ class AbstractClientBuildPathTest {
     private static Stream<Arguments> 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 %"));
     }
 
 }

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<String> 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",