diff --git a/src/main/java/com/gocardless/http/UrlFormatter.java b/src/main/java/com/gocardless/http/UrlFormatter.java index 3b3f5c2e..80e6e543 100644 --- a/src/main/java/com/gocardless/http/UrlFormatter.java +++ b/src/main/java/com/gocardless/http/UrlFormatter.java @@ -17,12 +17,65 @@ HttpUrl formatUrl(String template, Map pathParams, String path = template; for (Map.Entry 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 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. + * + *

+ * 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; + } } diff --git a/src/test/java/com/gocardless/http/UrlFormatterTest.java b/src/test/java/com/gocardless/http/UrlFormatterTest.java index 25684d7d..096d16d2 100644 --- a/src/test/java/com/gocardless/http/UrlFormatterTest.java +++ b/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 pathParams = ImmutableMap.of("bar", "bar/lah"); Map 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 pathParams = ImmutableMap.of("bar", "?limit=500"); + Map queryParams = ImmutableMap.of(); + assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams)) + .isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void shouldRejectFragmentMarkerInPathParam() { + String template = "/foo/:bar"; + Map pathParams = ImmutableMap.of("bar", "ID123#x"); + Map queryParams = ImmutableMap.of(); + assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams)) + .isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void shouldRejectControlCharacterInPathParam() { + String template = "/foo/:bar"; + Map pathParams = ImmutableMap.of("bar", "ID123\n"); + Map queryParams = ImmutableMap.of(); + assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams)) + .isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void shouldRejectDotPathParam() { + String template = "/foo/:bar"; + Map pathParams = ImmutableMap.of("bar", "."); + Map queryParams = ImmutableMap.of(); + assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams)) + .isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void shouldRejectDoubleDotPathParam() { + String template = "/foo/:bar"; + Map pathParams = ImmutableMap.of("bar", ".."); + Map queryParams = ImmutableMap.of(); + assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams)) + .isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void shouldRejectEmptyPathParam() { + String template = "/foo/:bar"; + Map pathParams = ImmutableMap.of("bar", ""); + Map 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 pathParams = ImmutableMap.of(); + Map queryParams = ImmutableMap.of(); + assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams)) + .isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void shouldRejectSchemeRelativeUrlAsPath() { + String template = "//elsewhere.example.com/capture"; + Map pathParams = ImmutableMap.of(); + Map 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 pathParams = ImmutableMap.of(); + Map queryParams = ImmutableMap.of(); + assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams)) + .isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void shouldRejectAbsoluteUrlOnADifferentPortOfTheSameHost() { + String template = "http://example.com:8080/capture"; + Map pathParams = ImmutableMap.of(); + Map 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 pathParams = ImmutableMap.of(); + Map queryParams = ImmutableMap.of(); + HttpUrl result = urlFormatter.formatUrl(template, pathParams, queryParams); + assertThat(result.toString()).isEqualTo("http://example.com/other"); + } }