Changes from gocardless/client-library-templates

src/main/java/com/gocardless/http/UrlFormatter.java

@@ -17,12 +17,65 @@ HttpUrl formatUrl(String template, Map<String, String> pathParams,
         String path = template;
         for (Map.Entry<String, String> entry : pathParams.entrySet()) {
             path = path.replace(":" + entry.getKey(),
-                    urlPathSegmentEscaper().escape(entry.getValue()));
+                    escapePathParam(entry.getKey(), entry.getValue()));
         }
-        HttpUrl.Builder builder = baseUrl.resolve(path).newBuilder();
+        HttpUrl.Builder builder = resolveAgainstBaseUrl(path).newBuilder();
         for (Map.Entry<String, Object> param : queryParams.entrySet()) {
             builder.addQueryParameter(param.getKey(), param.getValue().toString());
         }
         return builder.build();
     }
+
+    /**
+     * Resolves a path against the base URL, rejecting one that would leave its origin - an absolute
+     * or scheme-relative path, which {@link okhttp3.HttpUrl#resolve} otherwise follows like a
+     * browser follows a link, replacing the origin while the auth header stays attached.
+     *
+     * <p>
+     * Checked on the resolved URL rather than the raw string, since {@code resolve} accepts
+     * authority syntax (e.g. backslashes) that {@link okhttp3.HttpUrl#parse} would reject. Dot
+     * segments are left alone: they resolve against the base URL and can't leave its origin.
+     */
+    private HttpUrl resolveAgainstBaseUrl(String path) {
+        HttpUrl resolved = baseUrl.resolve(path);
+        if (resolved == null) {
+            throw new IllegalArgumentException(
+                    "Invalid request path '" + path + "': not a valid URL path");
+        }
+        if (!resolved.scheme().equals(baseUrl.scheme()) || !resolved.host().equals(baseUrl.host())
+                || resolved.port() != baseUrl.port()) {
+            throw new IllegalArgumentException("Invalid request path '" + path
+                    + "': a path may not specify a scheme or a host, only a location relative "
+                    + "to the configured base URL");
+        }
+        return resolved;
+    }
+
+    /**
+     * Escapes a value before it is interpolated into a request path. A path parameter is a single
+     * path segment, so values that could move the request to a different endpoint - path
+     * separators, control characters, '.', '..' (escaping can't make these safe - resolve() strips
+     * them regardless), and empty values - are rejected instead.
+     */
+    private static String escapePathParam(String key, String value) {
+        if (value == null || value.isEmpty()) {
+            throw new IllegalArgumentException("No value provided for URL parameter '" + key + "'");
+        }
+        if (value.equals(".") || value.equals("..")) {
+            throw new IllegalArgumentException("Invalid value for URL parameter '" + key + "': '"
+                    + value + "' would change which endpoint the request is sent to");
+        }
+        for (int i = 0; i < value.length(); i++) {
+            if (isForbiddenPathParamChar(value.charAt(i))) {
+                throw new IllegalArgumentException("Invalid value for URL parameter '" + key
+                        + "': '" + value + "' contains a character that is not allowed in a path "
+                        + "segment");
+            }
+        }
+        return urlPathSegmentEscaper().escape(value);
+    }
+
+    private static boolean isForbiddenPathParamChar(char c) {
+        return c == '/' || c == '?' || c == '#' || c < 0x20 || c == 0x7f;
+    }
 }

src/test/java/com/gocardless/http/UrlFormatterTest.java

@@ -1,6 +1,7 @@
 package com.gocardless.http;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 import com.google.common.collect.ImmutableMap;
 import java.util.Map;
@@ -72,12 +73,66 @@ public void shouldAddQueryParams() {
     }
 
     @Test
-    public void shouldEncodePathParam() {
+    public void shouldRejectSlashInPathParam() {
         String template = "/foo/:bar";
         Map<String, String> pathParams = ImmutableMap.of("bar", "bar/lah");
         Map<String, Object> queryParams = ImmutableMap.of();
-        HttpUrl result = urlFormatter.formatUrl(template, pathParams, queryParams);
-        assertThat(result.toString()).isEqualTo("http://example.com/foo/bar%2Flah");
+        assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams))
+                .isInstanceOf(IllegalArgumentException.class);
+    }
+
+    @Test
+    public void shouldRejectQuestionMarkInPathParam() {
+        String template = "/foo/:bar";
+        Map<String, String> pathParams = ImmutableMap.of("bar", "?limit=500");
+        Map<String, Object> queryParams = ImmutableMap.of();
+        assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams))
+                .isInstanceOf(IllegalArgumentException.class);
+    }
+
+    @Test
+    public void shouldRejectFragmentMarkerInPathParam() {
+        String template = "/foo/:bar";
+        Map<String, String> pathParams = ImmutableMap.of("bar", "ID123#x");
+        Map<String, Object> queryParams = ImmutableMap.of();
+        assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams))
+                .isInstanceOf(IllegalArgumentException.class);
+    }
+
+    @Test
+    public void shouldRejectControlCharacterInPathParam() {
+        String template = "/foo/:bar";
+        Map<String, String> pathParams = ImmutableMap.of("bar", "ID123\n");
+        Map<String, Object> queryParams = ImmutableMap.of();
+        assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams))
+                .isInstanceOf(IllegalArgumentException.class);
+    }
+
+    @Test
+    public void shouldRejectDotPathParam() {
+        String template = "/foo/:bar";
+        Map<String, String> pathParams = ImmutableMap.of("bar", ".");
+        Map<String, Object> queryParams = ImmutableMap.of();
+        assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams))
+                .isInstanceOf(IllegalArgumentException.class);
+    }
+
+    @Test
+    public void shouldRejectDoubleDotPathParam() {
+        String template = "/foo/:bar";
+        Map<String, String> pathParams = ImmutableMap.of("bar", "..");
+        Map<String, Object> queryParams = ImmutableMap.of();
+        assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams))
+                .isInstanceOf(IllegalArgumentException.class);
+    }
+
+    @Test
+    public void shouldRejectEmptyPathParam() {
+        String template = "/foo/:bar";
+        Map<String, String> pathParams = ImmutableMap.of("bar", "");
+        Map<String, Object> queryParams = ImmutableMap.of();
+        assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams))
+                .isInstanceOf(IllegalArgumentException.class);
     }
 
     @Test
@@ -98,4 +153,55 @@ public void shouldSupportBaseUrlWithPath() {
         HttpUrl result = urlFormatter.formatUrl(template, pathParams, queryParams);
         assertThat(result.toString()).isEqualTo("http://example.com/direct/debit/ID123");
     }
+    // An absolute or scheme-relative path would replace the configured base URL while the
+    // Authorization header is still attached, handing the token to whichever host it names.
+
+    @Test
+    public void shouldRejectAbsoluteUrlAsPath() {
+        String template = "http://elsewhere.example.com/capture";
+        Map<String, String> pathParams = ImmutableMap.of();
+        Map<String, Object> queryParams = ImmutableMap.of();
+        assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams))
+                .isInstanceOf(IllegalArgumentException.class);
+    }
+
+    @Test
+    public void shouldRejectSchemeRelativeUrlAsPath() {
+        String template = "//elsewhere.example.com/capture";
+        Map<String, String> pathParams = ImmutableMap.of();
+        Map<String, Object> queryParams = ImmutableMap.of();
+        assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams))
+                .isInstanceOf(IllegalArgumentException.class);
+    }
+
+    @Test
+    public void shouldRejectBackslashAuthorityAsPath() {
+        // HttpUrl#resolve reads backslashes as slashes, so this is authority syntax that a
+        // check on the raw string would be likely to miss.
+        String template = "\\\\elsewhere.example.com/capture";
+        Map<String, String> pathParams = ImmutableMap.of();
+        Map<String, Object> queryParams = ImmutableMap.of();
+        assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams))
+                .isInstanceOf(IllegalArgumentException.class);
+    }
+
+    @Test
+    public void shouldRejectAbsoluteUrlOnADifferentPortOfTheSameHost() {
+        String template = "http://example.com:8080/capture";
+        Map<String, String> pathParams = ImmutableMap.of();
+        Map<String, Object> queryParams = ImmutableMap.of();
+        assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams))
+                .isInstanceOf(IllegalArgumentException.class);
+    }
+
+    @Test
+    public void shouldKeepDotSegmentsOnTheConfiguredBaseUrl() {
+        // Dot segments resolve against the base URL, so they can reach another path on the
+        // same origin but cannot leave it. They are allowed through.
+        String template = "/foo/../other";
+        Map<String, String> pathParams = ImmutableMap.of();
+        Map<String, Object> queryParams = ImmutableMap.of();
+        HttpUrl result = urlFormatter.formatUrl(template, pathParams, queryParams);
+        assertThat(result.toString()).isEqualTo("http://example.com/other");
+    }
 }