Skip to content

Commit 37b0ed4

Browse files
pitbulkpitbulk
authored
Merge pull request #94 from rembjo0/topic-redirect-signature-verifiy
Use raw url parameters in redirect signature validation
2 parents cd7bf88 + ca7b2d7 commit 37b0ed4

11 files changed

Lines changed: 516 additions & 16 deletions

File tree

‎core/src/main/java/com/onelogin/saml2/http/HttpRequest.java‎

Lines changed: 78 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,24 +10,46 @@
1010
import java.util.List;
1111
import java.util.Map;
1212
import java.util.Objects;
13+
import java.util.regex.Matcher;
14+
import java.util.regex.Pattern;
15+
16+
import org.apache.commons.lang3.StringUtils;
17+
18+
import com.onelogin.saml2.util.Util;
1319

1420
/**
1521
* Framework-agnostic representation of an HTTP request.
1622
*
1723
* @since 2.0.0
1824
*/
1925
public final class HttpRequest {
26+
27+
public static final Map<String, List<String>> EMPTY_PARAMETERS = Collections.<String, List<String>>emptyMap();
28+
2029
private final String requestURL;
2130
private final Map<String, List<String>> parameters;
31+
private final String queryString;
2232

2333
/**
2434
* Creates a new HttpRequest.
2535
*
2636
* @param requestURL the request URL (up to but not including query parameters)
2737
* @throws NullPointerException if requestURL is null
38+
* @deprecated Not providing a queryString can cause HTTP Redirect binding to fail.
2839
*/
40+
@Deprecated
2941
public HttpRequest(String requestURL) {
30-
this(requestURL, Collections.<String, List<String>>emptyMap());
42+
this(requestURL, EMPTY_PARAMETERS);
43+
}
44+
45+
/**
46+
* Creates a new HttpRequest.
47+
*
48+
* @param requestURL the request URL (up to but not including query parameters)
49+
* @param query string that is contained in the request URL after the path
50+
*/
51+
public HttpRequest(String requestURL, String queryString) {
52+
this(requestURL, EMPTY_PARAMETERS, queryString);
3153
}
3254

3355
/**
@@ -36,10 +58,25 @@ public HttpRequest(String requestURL) {
3658
* @param requestURL the request URL (up to but not including query parameters)
3759
* @param parameters the request query parameters
3860
* @throws NullPointerException if any of the parameters is null
61+
* @deprecated Not providing a queryString can cause HTTP Redirect binding to fail.
3962
*/
63+
@Deprecated
4064
public HttpRequest(String requestURL, Map<String, List<String>> parameters) {
65+
this(requestURL, parameters, null);
66+
}
67+
68+
/**
69+
* Creates a new HttpRequest.
70+
*
71+
* @param requestURL the request URL (up to but not including query parameters)
72+
* @param parameters the request query parameters
73+
* @param query string that is contained in the request URL after the path
74+
* @throws NullPointerException if any of the parameters is null
75+
*/
76+
public HttpRequest(String requestURL, Map<String, List<String>> parameters, String queryString) {
4177
this.requestURL = checkNotNull(requestURL, "requestURL");
4278
this.parameters = unmodifiableCopyOf(checkNotNull(parameters, "queryParams"));
79+
this.queryString = StringUtils.trimToEmpty(queryString);
4380
}
4481

4582
/**
@@ -58,7 +95,7 @@ public HttpRequest addParameter(String name, String value) {
5895
final Map<String, List<String>> params = new HashMap<>(parameters);
5996
params.put(name, newValues);
6097

61-
return new HttpRequest(requestURL, params);
98+
return new HttpRequest(requestURL, params, queryString);
6299
}
63100

64101
/**
@@ -72,7 +109,7 @@ public HttpRequest removeParameter(String name) {
72109
final Map<String, List<String>> params = new HashMap<>(parameters);
73110
params.remove(name);
74111

75-
return new HttpRequest(requestURL, params);
112+
return new HttpRequest(requestURL, params, queryString);
76113
}
77114

78115
/**
@@ -110,6 +147,37 @@ public Map<String, List<String>> getParameters() {
110147
return parameters;
111148
}
112149

150+
/**
151+
* Return an url encoded get parameter value
152+
* Prefer to extract the original encoded value directly from queryString since url
153+
* encoding is not canonical.
154+
*
155+
* @param name
156+
* @return the first value for the parameter, or null
157+
*/
158+
public String getEncodedParameter(String name) {
159+
Matcher matcher = Pattern.compile(Pattern.quote(name) + "=([^&#]+)").matcher(queryString);
160+
if (matcher.find()) {
161+
return matcher.group(1);
162+
} else {
163+
return Util.urlEncoder(getParameter(name));
164+
}
165+
}
166+
167+
/**
168+
* Return an url encoded get parameter value
169+
* Prefer to extract the original encoded value directly from queryString since url
170+
* encoding is not canonical.
171+
*
172+
* @param name
173+
* @param defaultValue
174+
* @return the first value for the parameter, or url encoded default value
175+
*/
176+
public String getEncodedParameter(String name, String defaultValue) {
177+
String value = getEncodedParameter(name);
178+
return (value != null ? value : Util.urlEncoder(defaultValue));
179+
}
180+
113181
@Override
114182
public boolean equals(Object o) {
115183
if (this == o) {
@@ -122,19 +190,21 @@ public boolean equals(Object o) {
122190

123191
HttpRequest that = (HttpRequest) o;
124192
return Objects.equals(requestURL, that.requestURL) &&
125-
Objects.equals(parameters, that.parameters);
193+
Objects.equals(parameters, that.parameters) &&
194+
Objects.equals(queryString, this.queryString);
126195
}
127196

128197
@Override
129198
public int hashCode() {
130-
return Objects.hash(requestURL, parameters);
199+
return Objects.hash(requestURL, parameters, queryString);
131200
}
132201

133202
@Override
134203
public String toString() {
135204
return "HttpRequest{" +
136205
"requestURL='" + requestURL + '\'' +
137206
", parameters=" + parameters +
207+
", queryString=" + queryString +
138208
'}';
139209
}
140210

@@ -146,4 +216,6 @@ private static Map<String, List<String>> unmodifiableCopyOf(Map<String, List<Str
146216

147217
return unmodifiableMap(copy);
148218
}
149-
}
219+
220+
221+
}

‎core/src/main/java/com/onelogin/saml2/logout/LogoutRequest.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -334,15 +334,15 @@ public Boolean isValid() throws Exception {
334334
if (signAlg == null || signAlg.isEmpty()) {
335335
signAlg = Constants.RSA_SHA1;
336336
}
337-
String relayState = request.getParameter("RelayState");
337+
String relayState = request.getEncodedParameter("RelayState");
338338

339-
String signedQuery = "SAMLRequest=" + Util.urlEncoder(request.getParameter("SAMLRequest"));
339+
String signedQuery = "SAMLRequest=" + request.getEncodedParameter("SAMLRequest");
340340

341341
if (relayState != null && !relayState.isEmpty()) {
342-
signedQuery += "&RelayState=" + Util.urlEncoder(relayState);
342+
signedQuery += "&RelayState=" + relayState;
343343
}
344344

345-
signedQuery += "&SigAlg=" + Util.urlEncoder(signAlg);
345+
signedQuery += "&SigAlg=" + request.getEncodedParameter("SigAlg", signAlg);
346346

347347
if (!Util.validateBinarySignature(signedQuery, Util.base64decoder(signature), cert, signAlg)) {
348348
throw new ValidationError("Signature validation failed. Logout Request rejected", ValidationError.INVALID_SIGNATURE);

‎core/src/main/java/com/onelogin/saml2/logout/LogoutResponse.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -233,14 +233,14 @@ public Boolean isValid(String requestId) {
233233
signAlg = Constants.RSA_SHA1;
234234
}
235235

236-
String signedQuery = "SAMLResponse=" + Util.urlEncoder(request.getParameter("SAMLResponse"));
236+
String signedQuery = "SAMLResponse=" + request.getEncodedParameter("SAMLResponse");
237237

238-
String relayState = request.getParameter("RelayState");
238+
String relayState = request.getEncodedParameter("RelayState");
239239
if (relayState != null && !relayState.isEmpty()) {
240-
signedQuery += "&RelayState=" + Util.urlEncoder(relayState);
240+
signedQuery += "&RelayState=" + relayState;
241241
}
242242

243-
signedQuery += "&SigAlg=" + Util.urlEncoder(signAlg);
243+
signedQuery += "&SigAlg=" + request.getEncodedParameter("SigAlg", signAlg);
244244

245245
if (!Util.validateBinarySignature(signedQuery, Util.base64decoder(signature), cert, signAlg)) {
246246
throw new ValidationError("Signature validation failed. Logout Response rejected", ValidationError.INVALID_SIGNATURE);

‎core/src/test/java/com/onelogin/saml2/http/HttpRequestTest.java‎

Lines changed: 118 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,15 @@
1010

1111
import java.util.Arrays;
1212
import java.util.Collections;
13+
import java.util.HashMap;
1314
import java.util.List;
1415
import java.util.Map;
1516

1617
import org.junit.Test;
1718

19+
import com.onelogin.saml2.test.NaiveUrlEncoder;
20+
import com.onelogin.saml2.util.Util;
21+
1822
public class HttpRequestTest {
1923
@Test
2024
public void testConstructorWithNoQueryParams() throws Exception {
@@ -55,6 +59,9 @@ public void testAddParameter() throws Exception {
5559
assertThat(request.getParameters(), equalTo(singletonMap(name, singletonList(value))));
5660
assertThat(request.getParameters(name), equalTo(singletonList(value)));
5761
assertThat(request.getParameter(name), equalTo(value));
62+
63+
final HttpRequest request2 = request.addParameter(name, value);
64+
assertThat(request2.getParameters(name), equalTo(Arrays.asList(value, value)));
5865
}
5966

6067
@Test
@@ -68,11 +75,121 @@ public void testRemoveParameter() throws Exception {
6875
assertThat(request.getParameters(), equalTo(singletonMap(name, singletonList(value))));
6976
assertThat(request.getParameters(name), equalTo(singletonList(value)));
7077
assertThat(request.getParameter(name), equalTo(value));
71-
78+
7279
request = request.removeParameter(name);
7380
assertThat(request.getRequestURL(), equalTo(url));
7481
assertTrue(request.getParameters().isEmpty());
7582
assertTrue(request.getParameters(name).isEmpty());
7683
assertNull(request.getParameter(name));
7784
}
85+
86+
@Test
87+
public void testGetEncodedParameter_encodesParametersNotOnQueryString() throws Exception {
88+
final String url = "url";
89+
final String name = "name";
90+
final String value1 = "val/1!";
91+
final String addedName = "added";
92+
final String addedValue = "added#value!";
93+
94+
final List<String> values = Arrays.asList(value1);
95+
final Map<String, List<String>> parametersMap = singletonMap(name, values);
96+
97+
final HttpRequest request = new HttpRequest(url, parametersMap).addParameter(addedName, addedValue);
98+
99+
assertThat(request.getEncodedParameter(name), equalTo(Util.urlEncoder(value1)));
100+
assertThat(request.getEncodedParameter(addedName), equalTo(Util.urlEncoder(addedValue)));
101+
}
102+
103+
@Test
104+
public void testGetEncodedParameter_prefersValueFromQueryString() throws Exception {
105+
final String url = "url";
106+
final String name = "name";
107+
final String value1 = "value1";
108+
final String urlValue1 = "onUrl1";
109+
final String queryString = name + "=" + urlValue1;
110+
111+
final List<String> values = Arrays.asList(value1);
112+
final Map<String, List<String>> parametersMap = singletonMap(name, values);
113+
114+
final HttpRequest request = new HttpRequest(url, parametersMap, queryString);
115+
116+
assertThat(request.getEncodedParameter(name), equalTo(urlValue1));
117+
assertThat(request.getParameter(name), equalTo(value1));
118+
}
119+
120+
@Test
121+
public void testGetEncodedParameter_returnsExactAsGivenInQueryString() throws Exception {
122+
final String url = "url";
123+
final String name = "name";
124+
String encodedValue1 = NaiveUrlEncoder.encode("do not alter!");
125+
final String queryString = name + "=" + encodedValue1;
126+
127+
final HttpRequest request = new HttpRequest(url, queryString);
128+
129+
assertThat(request.getEncodedParameter(name), equalTo(encodedValue1));
130+
}
131+
132+
@Test
133+
public void testGetEncodedParameter_handlesMultipleValuesOnQueryString() throws Exception {
134+
final String url = "url";
135+
final String queryString = "k1=v1&k2=v2&k3=v3";
136+
137+
final Map<String, List<String>> parametersMap = new HashMap<>();
138+
final HttpRequest request = new HttpRequest(url, parametersMap, queryString);
139+
140+
assertThat(request.getEncodedParameter("k1"), equalTo("v1"));
141+
assertThat(request.getEncodedParameter("k2"), equalTo("v2"));
142+
assertThat(request.getEncodedParameter("k3"), equalTo("v3"));
143+
}
144+
145+
@Test
146+
public void testGetEncodedParameter_stopsAtUrlFragment() throws Exception {
147+
final String url = "url";
148+
final String queryString = "first=&foo=bar#ignore";
149+
150+
final HttpRequest request = new HttpRequest(url, queryString);
151+
152+
assertThat(request.getEncodedParameter("foo"), equalTo("bar"));
153+
}
154+
155+
@Test
156+
public void testGetEncodedParameter_withDefault_usesDefaultWhenParameterMissing() throws Exception {
157+
final String url = "url";
158+
final String foobar = "foo/bar!";
159+
160+
final HttpRequest request = new HttpRequest(url);
161+
assertThat(request.getEncodedParameter("missing", foobar), equalTo(Util.urlEncoder(foobar)));
162+
}
163+
164+
165+
@Test
166+
public void testAddParameter_preservesQueryString() throws Exception {
167+
final String url = "url";
168+
final String name = "name";
169+
final String value1 = "val/1!";
170+
String encodedValue1 = NaiveUrlEncoder.encode(value1);
171+
final String queryString = name + "=" + encodedValue1;
172+
173+
final Map<String, List<String>> parametersMap = new HashMap<>();
174+
final HttpRequest request = new HttpRequest(url, parametersMap, queryString).addParameter(name, value1);
175+
176+
assertThat(request.getEncodedParameter(name), equalTo(encodedValue1));
177+
}
178+
179+
@Test
180+
public void testRemoveParameter_preservesQueryString() throws Exception {
181+
final String url = "url";
182+
final String name = "name";
183+
final String value1 = "val/1!";
184+
String encodedValue1 = NaiveUrlEncoder.encode(value1);
185+
final String queryString = name + "=" + encodedValue1;
186+
187+
final List<String> values = Arrays.asList(value1);
188+
final Map<String, List<String>> parametersMap = singletonMap(name, values);
189+
190+
final HttpRequest request = new HttpRequest(url, parametersMap, queryString).removeParameter(name);
191+
192+
assertThat(request.getEncodedParameter(name), equalTo(encodedValue1));
193+
}
194+
78195
}

‎core/src/test/java/com/onelogin/saml2/test/NaiveUrlEncodeTest.java‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
package com.onelogin.saml2.test;
2+
3+
import java.io.UnsupportedEncodingException;
4+
import java.net.URLDecoder;
5+
6+
import org.junit.Assert;
7+
import org.junit.Test;
8+
9+
import com.onelogin.saml2.util.Util;
10+
11+
public class NaiveUrlEncodeTest {
12+
13+
@Test
14+
public void testDemonstratingUrlEncodingNotCanonical () throws UnsupportedEncodingException {
15+
String theString = "Hello World!";
16+
17+
String naiveEncoded = NaiveUrlEncoder.encode(theString);
18+
String propperEncoded = Util.urlEncoder(theString);
19+
20+
Assert.assertNotEquals("Encoded versions should differ", naiveEncoded, propperEncoded);
21+
Assert.assertEquals("Decoded versions equal", URLDecoder.decode(naiveEncoded, "UTF-8"), URLDecoder.decode(propperEncoded, "UTF-8"));
22+
}
23+
24+
}

0 commit comments

Comments
 (0)