Skip to content

Repository navigation

Commit c3f12e9

Browse files
gocardless-ci-robot[bot]gocardless-ci-robot[bot]
authored
Merge pull request #232 from gocardless/template-changes
Changes from gocardless/client-library-templates
2 parents 874c555 + 12bf5d4 commit c3f12e9

2 files changed

Lines changed: 164 additions & 5 deletions

File tree

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

Lines changed: 55 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,65 @@ HttpUrl formatUrl(String template, Map<String, String> pathParams,
1717
String path = template;
1818
for (Map.Entry<String, String> entry : pathParams.entrySet()) {
1919
path = path.replace(":" + entry.getKey(),
20-
urlPathSegmentEscaper().escape(entry.getValue()));
20+
escapePathParam(entry.getKey(), entry.getValue()));
2121
}
22-
HttpUrl.Builder builder = baseUrl.resolve(path).newBuilder();
22+
HttpUrl.Builder builder = resolveAgainstBaseUrl(path).newBuilder();
2323
for (Map.Entry<String, Object> param : queryParams.entrySet()) {
2424
builder.addQueryParameter(param.getKey(), param.getValue().toString());
2525
}
2626
return builder.build();
2727
}
28+
29+
/**
30+
* Resolves a path against the base URL, rejecting one that would leave its origin - an absolute
31+
* or scheme-relative path, which {@link okhttp3.HttpUrl#resolve} otherwise follows like a
32+
* browser follows a link, replacing the origin while the auth header stays attached.
33+
*
34+
* <p>
35+
* Checked on the resolved URL rather than the raw string, since {@code resolve} accepts
36+
* authority syntax (e.g. backslashes) that {@link okhttp3.HttpUrl#parse} would reject. Dot
37+
* segments are left alone: they resolve against the base URL and can't leave its origin.
38+
*/
39+
private HttpUrl resolveAgainstBaseUrl(String path) {
40+
HttpUrl resolved = baseUrl.resolve(path);
41+
if (resolved == null) {
42+
throw new IllegalArgumentException(
43+
"Invalid request path '" + path + "': not a valid URL path");
44+
}
45+
if (!resolved.scheme().equals(baseUrl.scheme()) || !resolved.host().equals(baseUrl.host())
46+
|| resolved.port() != baseUrl.port()) {
47+
throw new IllegalArgumentException("Invalid request path '" + path
48+
+ "': a path may not specify a scheme or a host, only a location relative "
49+
+ "to the configured base URL");
50+
}
51+
return resolved;
52+
}
53+
54+
/**
55+
* Escapes a value before it is interpolated into a request path. A path parameter is a single
56+
* path segment, so values that could move the request to a different endpoint - path
57+
* separators, control characters, '.', '..' (escaping can't make these safe - resolve() strips
58+
* them regardless), and empty values - are rejected instead.
59+
*/
60+
private static String escapePathParam(String key, String value) {
61+
if (value == null || value.isEmpty()) {
62+
throw new IllegalArgumentException("No value provided for URL parameter '" + key + "'");
63+
}
64+
if (value.equals(".") || value.equals("..")) {
65+
throw new IllegalArgumentException("Invalid value for URL parameter '" + key + "': '"
66+
+ value + "' would change which endpoint the request is sent to");
67+
}
68+
for (int i = 0; i < value.length(); i++) {
69+
if (isForbiddenPathParamChar(value.charAt(i))) {
70+
throw new IllegalArgumentException("Invalid value for URL parameter '" + key
71+
+ "': '" + value + "' contains a character that is not allowed in a path "
72+
+ "segment");
73+
}
74+
}
75+
return urlPathSegmentEscaper().escape(value);
76+
}
77+
78+
private static boolean isForbiddenPathParamChar(char c) {
79+
return c == '/' || c == '?' || c == '#' || c < 0x20 || c == 0x7f;
80+
}
2881
}

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

Lines changed: 109 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package com.gocardless.http;
22

33
import static org.assertj.core.api.Assertions.assertThat;
4+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
45

56
import com.google.common.collect.ImmutableMap;
67
import java.util.Map;
@@ -72,12 +73,66 @@ public void shouldAddQueryParams() {
7273
}
7374

7475
@Test
75-
public void shouldEncodePathParam() {
76+
public void shouldRejectSlashInPathParam() {
7677
String template = "/foo/:bar";
7778
Map<String, String> pathParams = ImmutableMap.of("bar", "bar/lah");
7879
Map<String, Object> queryParams = ImmutableMap.of();
79-
HttpUrl result = urlFormatter.formatUrl(template, pathParams, queryParams);
80-
assertThat(result.toString()).isEqualTo("http://example.com/foo/bar%2Flah");
80+
assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams))
81+
.isInstanceOf(IllegalArgumentException.class);
82+
}
83+
84+
@Test
85+
public void shouldRejectQuestionMarkInPathParam() {
86+
String template = "/foo/:bar";
87+
Map<String, String> pathParams = ImmutableMap.of("bar", "?limit=500");
88+
Map<String, Object> queryParams = ImmutableMap.of();
89+
assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams))
90+
.isInstanceOf(IllegalArgumentException.class);
91+
}
92+
93+
@Test
94+
public void shouldRejectFragmentMarkerInPathParam() {
95+
String template = "/foo/:bar";
96+
Map<String, String> pathParams = ImmutableMap.of("bar", "ID123#x");
97+
Map<String, Object> queryParams = ImmutableMap.of();
98+
assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams))
99+
.isInstanceOf(IllegalArgumentException.class);
100+
}
101+
102+
@Test
103+
public void shouldRejectControlCharacterInPathParam() {
104+
String template = "/foo/:bar";
105+
Map<String, String> pathParams = ImmutableMap.of("bar", "ID123\n");
106+
Map<String, Object> queryParams = ImmutableMap.of();
107+
assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams))
108+
.isInstanceOf(IllegalArgumentException.class);
109+
}
110+
111+
@Test
112+
public void shouldRejectDotPathParam() {
113+
String template = "/foo/:bar";
114+
Map<String, String> pathParams = ImmutableMap.of("bar", ".");
115+
Map<String, Object> queryParams = ImmutableMap.of();
116+
assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams))
117+
.isInstanceOf(IllegalArgumentException.class);
118+
}
119+
120+
@Test
121+
public void shouldRejectDoubleDotPathParam() {
122+
String template = "/foo/:bar";
123+
Map<String, String> pathParams = ImmutableMap.of("bar", "..");
124+
Map<String, Object> queryParams = ImmutableMap.of();
125+
assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams))
126+
.isInstanceOf(IllegalArgumentException.class);
127+
}
128+
129+
@Test
130+
public void shouldRejectEmptyPathParam() {
131+
String template = "/foo/:bar";
132+
Map<String, String> pathParams = ImmutableMap.of("bar", "");
133+
Map<String, Object> queryParams = ImmutableMap.of();
134+
assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams))
135+
.isInstanceOf(IllegalArgumentException.class);
81136
}
82137

83138
@Test
@@ -98,4 +153,55 @@ public void shouldSupportBaseUrlWithPath() {
98153
HttpUrl result = urlFormatter.formatUrl(template, pathParams, queryParams);
99154
assertThat(result.toString()).isEqualTo("http://example.com/direct/debit/ID123");
100155
}
156+
// An absolute or scheme-relative path would replace the configured base URL while the
157+
// Authorization header is still attached, handing the token to whichever host it names.
158+
159+
@Test
160+
public void shouldRejectAbsoluteUrlAsPath() {
161+
String template = "http://elsewhere.example.com/capture";
162+
Map<String, String> pathParams = ImmutableMap.of();
163+
Map<String, Object> queryParams = ImmutableMap.of();
164+
assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams))
165+
.isInstanceOf(IllegalArgumentException.class);
166+
}
167+
168+
@Test
169+
public void shouldRejectSchemeRelativeUrlAsPath() {
170+
String template = "//elsewhere.example.com/capture";
171+
Map<String, String> pathParams = ImmutableMap.of();
172+
Map<String, Object> queryParams = ImmutableMap.of();
173+
assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams))
174+
.isInstanceOf(IllegalArgumentException.class);
175+
}
176+
177+
@Test
178+
public void shouldRejectBackslashAuthorityAsPath() {
179+
// HttpUrl#resolve reads backslashes as slashes, so this is authority syntax that a
180+
// check on the raw string would be likely to miss.
181+
String template = "\\\\elsewhere.example.com/capture";
182+
Map<String, String> pathParams = ImmutableMap.of();
183+
Map<String, Object> queryParams = ImmutableMap.of();
184+
assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams))
185+
.isInstanceOf(IllegalArgumentException.class);
186+
}
187+
188+
@Test
189+
public void shouldRejectAbsoluteUrlOnADifferentPortOfTheSameHost() {
190+
String template = "http://example.com:8080/capture";
191+
Map<String, String> pathParams = ImmutableMap.of();
192+
Map<String, Object> queryParams = ImmutableMap.of();
193+
assertThatThrownBy(() -> urlFormatter.formatUrl(template, pathParams, queryParams))
194+
.isInstanceOf(IllegalArgumentException.class);
195+
}
196+
197+
@Test
198+
public void shouldKeepDotSegmentsOnTheConfiguredBaseUrl() {
199+
// Dot segments resolve against the base URL, so they can reach another path on the
200+
// same origin but cannot leave it. They are allowed through.
201+
String template = "/foo/../other";
202+
Map<String, String> pathParams = ImmutableMap.of();
203+
Map<String, Object> queryParams = ImmutableMap.of();
204+
HttpUrl result = urlFormatter.formatUrl(template, pathParams, queryParams);
205+
assertThat(result.toString()).isEqualTo("http://example.com/other");
206+
}
101207
}

0 commit comments

Comments
 (0)