Skip to content

Commit 47e1a01

Browse files
joakimejoakime
authored
Issue #13368 - Bad UrlParameterDecoder.parse() return value. (#13369)
* Introduce new ComplianceTest in jetty-server. * UrlParameterDecoder.parse() had bad return value. * UrlEncoded.decodeUtf8To() had bad return value. * Enhancing UrlParameterDecoderTest to test for return value of parse() * Enhancing UrlEncodedUtf8Test to also test for decodeUtf8To() return value.
1 parent 8a59b30 commit 47e1a01

File tree

5 files changed

+315
-67
lines changed

5 files changed

+315
-67
lines changed

‎jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ComplianceTest.java‎

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
//
2+
// ========================================================================
3+
// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others.
4+
//
5+
// This program and the accompanying materials are made available under the
6+
// terms of the Eclipse Public License v. 2.0 which is available at
7+
// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0
8+
// which is available at https://www.apache.org/licenses/LICENSE-2.0.
9+
//
10+
// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
11+
// ========================================================================
12+
//
13+
14+
package org.eclipse.jetty.server;
15+
16+
import java.util.ArrayList;
17+
import java.util.List;
18+
import java.util.Map;
19+
import java.util.Queue;
20+
import java.util.function.Consumer;
21+
import java.util.stream.Stream;
22+
23+
import org.eclipse.jetty.http.ComplianceViolation;
24+
import org.eclipse.jetty.http.HttpTester;
25+
import org.eclipse.jetty.http.UriCompliance;
26+
import org.eclipse.jetty.io.Content;
27+
import org.eclipse.jetty.util.BlockingArrayQueue;
28+
import org.eclipse.jetty.util.Callback;
29+
import org.eclipse.jetty.util.Fields;
30+
import org.eclipse.jetty.util.component.LifeCycle;
31+
import org.junit.jupiter.api.AfterEach;
32+
import org.junit.jupiter.params.ParameterizedTest;
33+
import org.junit.jupiter.params.provider.Arguments;
34+
import org.junit.jupiter.params.provider.MethodSource;
35+
36+
import static org.hamcrest.MatcherAssert.assertThat;
37+
import static org.hamcrest.Matchers.containsString;
38+
import static org.junit.jupiter.api.Assertions.assertEquals;
39+
40+
/**
41+
* Test various {@link org.eclipse.jetty.http.ComplianceViolation} behaviors with actual requests.
42+
*/
43+
public class ComplianceTest
44+
{
45+
private Server server;
46+
private LocalConnector localConnector;
47+
48+
@AfterEach
49+
public void stopServer()
50+
{
51+
LifeCycle.stop(server);
52+
}
53+
54+
protected void startServer(Consumer<Server> serverConsumer) throws Exception
55+
{
56+
server = new Server();
57+
localConnector = new LocalConnector(server);
58+
server.addConnector(localConnector);
59+
60+
if (serverConsumer != null)
61+
serverConsumer.accept(server);
62+
63+
server.start();
64+
}
65+
66+
public static Stream<Arguments> queryComplianceCases()
67+
{
68+
List<Arguments> cases = new ArrayList<>();
69+
for (UriCompliance compliance : List.of(UriCompliance.DEFAULT, UriCompliance.LEGACY, UriCompliance.RFC3986))
70+
{
71+
cases.add(Arguments.of(compliance, "query=blocked=%2F%2F%E6%84%9B%22",
72+
Map.of("query", "blocked=//愛\"")));
73+
cases.add(Arguments.of(compliance, "query=buster=1752874099305",
74+
Map.of("query", "buster=1752874099305")));
75+
cases.add(Arguments.of(compliance, "query=startup=1",
76+
Map.of("query", "startup=1")));
77+
}
78+
return cases.stream();
79+
}
80+
81+
@ParameterizedTest
82+
@MethodSource("queryComplianceCases")
83+
public void testQueryCompliance(UriCompliance compliance, String rawQuery, Map<String, String> expectedMap) throws Exception
84+
{
85+
Queue<ComplianceViolation.Event> events = new BlockingArrayQueue<>();
86+
87+
ComplianceViolation.Listener listener = new ComplianceViolation.Listener()
88+
{
89+
@Override
90+
public void onComplianceViolation(ComplianceViolation.Event event)
91+
{
92+
new RuntimeException("Huh?").printStackTrace(System.err);
93+
events.add(event);
94+
}
95+
};
96+
97+
startServer(server ->
98+
{
99+
localConnector.getContainedBeans(HttpConfiguration.class)
100+
.forEach(httpConfig ->
101+
{
102+
httpConfig.setUriCompliance(compliance);
103+
httpConfig.addComplianceViolationListener(listener);
104+
});
105+
106+
server.setHandler(new Handler.Abstract()
107+
{
108+
@Override
109+
public boolean handle(Request request, Response response, Callback callback)
110+
{
111+
try
112+
{
113+
StringBuilder body = new StringBuilder();
114+
body.append("raw-path-query=").append(request.getHttpURI().getPathQuery());
115+
Fields fields = Request.getParameters(request);
116+
for (Fields.Field field : fields)
117+
{
118+
body.append("\nfield[").append(field.getName());
119+
body.append("]=").append(field.getValues());
120+
}
121+
Content.Sink.write(response, true, body.toString(), callback);
122+
return true;
123+
}
124+
catch (Exception e)
125+
{
126+
throw new RuntimeException(e);
127+
}
128+
}
129+
});
130+
});
131+
132+
String rawRequest = """
133+
GET /test?%s HTTP/1.1
134+
Host: local
135+
Connection: close
136+
137+
""".formatted(rawQuery);
138+
139+
String rawResponse = localConnector.getResponse(rawRequest);
140+
HttpTester.Response response = HttpTester.parseResponse(rawResponse);
141+
assertEquals(200, response.getStatus());
142+
String responseBody = response.getContent();
143+
assertThat(responseBody, containsString(rawQuery));
144+
expectedMap.forEach((key, value) -> assertThat(responseBody, containsString("field[%s]=[%s]".formatted(key, value))));
145+
146+
// Shouldn't see any compliance violation events
147+
assertEquals(0, events.size(), () ->
148+
{
149+
StringBuilder msg = new StringBuilder();
150+
events.forEach(event -> msg.append("event:").append(event).append("\n"));
151+
return msg.toString();
152+
});
153+
}
154+
}

‎jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/UrlEncoded.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -368,7 +368,7 @@ public static boolean decodeUtf8To(String query, int offset, int length, BiConsu
368368
UrlParameterDecoder decoder = new UrlParameterDecoder(charsetStringBuilder, adder, -1, -1, allowBadUtf8, allowBadPercent, allowTruncatedUtf8);
369369
try
370370
{
371-
return decoder.parse(query, offset, length);
371+
return !decoder.parse(query, offset, length);
372372
}
373373
catch (IOException e)
374374
{

‎jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/UrlParameterDecoder.java‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ class UrlParameterDecoder
3939
private String name;
4040
private int keyCount;
4141
private int charCount;
42+
private boolean codingError = false;
4243

4344
public UrlParameterDecoder(CharsetStringBuilder charsetStringBuilder, BiConsumer<String, String> newFieldAdder)
4445
{
@@ -152,6 +153,7 @@ public boolean parse(Reader reader) throws IOException
152153

153154
private boolean parseCompletely(CharIterator iter) throws IOException
154155
{
156+
codingError = false;
155157
int i;
156158
while ((i = iter.next()) >= 0)
157159
{
@@ -164,7 +166,7 @@ private boolean parseCompletely(CharIterator iter) throws IOException
164166
}
165167

166168
complete();
167-
return builder.hasCodingErrors();
169+
return codingError;
168170
}
169171

170172
/**
@@ -223,6 +225,7 @@ else if (!StringUtil.isEmpty(str))
223225
}
224226
catch (NumberFormatException e)
225227
{
228+
codingError = true;
226229
boolean replaced = builder.replaceIncomplete();
227230
if (replaced && !allowBadEncoding || !allowBadPercent)
228231
throw new IllegalArgumentException(notValidPctEncoding((char)hi, (char)lo));
@@ -273,19 +276,22 @@ private boolean handleIncompletePctEncoding(int hi)
273276
{
274277
if (builder.replaceIncomplete())
275278
{
279+
codingError = true;
276280
if (!allowBadEncoding || !allowBadPercent)
277281
throw new IllegalArgumentException(notValidPctEncoding((char)hi, (char)0));
278282
return false;
279283
}
280284
else if (allowBadPercent)
281285
{
286+
codingError = true;
282287
builder.append('%');
283288
if (hi != -1)
284289
builder.append((char)hi);
285290
return true;
286291
}
287292
else
288293
{
294+
codingError = true;
289295
throw new IllegalArgumentException(notValidPctEncoding((char)hi, (char)0));
290296
}
291297
}
@@ -295,6 +301,9 @@ private static void decodeHexByteTo(CharsetStringBuilder buffer, char hi, char l
295301
buffer.append((byte)((convertHexDigit(hi) << 4) + convertHexDigit(lo)));
296302
}
297303

304+
/**
305+
* Finish up any remaining character sequences.
306+
*/
298307
private void complete() throws CharacterCodingException
299308
{
300309
if (name != null)
@@ -320,22 +329,26 @@ private String takeBuiltString() throws CharacterCodingException
320329
if (!allowBadEncoding && !allowBadPercent && !allowTruncatedEncoding)
321330
{
322331
String result = builder.build(false);
332+
codingError |= builder.hasCodingErrors();
323333
builder.reset();
324334
return result;
325335
}
326336

327-
boolean codingError = builder.hasCodingErrors();
337+
codingError |= builder.hasCodingErrors();
328338
if (codingError && !allowBadEncoding)
329339
{
330340
return builder.build(false);
331341
}
332342

333-
if (builder.replaceIncomplete() && !allowTruncatedEncoding)
343+
boolean replaced = builder.replaceIncomplete();
344+
codingError |= replaced;
345+
if (replaced && !allowTruncatedEncoding)
334346
{
335347
return builder.build(false);
336348
}
337349

338350
String result = builder.build(true);
351+
codingError |= builder.hasCodingErrors();
339352
builder.reset();
340353
return result;
341354
}

‎jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/UrlEncodedUtf8Test.java‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@
2424
import static org.hamcrest.MatcherAssert.assertThat;
2525
import static org.hamcrest.Matchers.is;
2626
import static org.hamcrest.Matchers.notNullValue;
27+
import static org.junit.jupiter.api.Assertions.assertFalse;
28+
import static org.junit.jupiter.api.Assertions.assertTrue;
2729

2830
public class UrlEncodedUtf8Test
2931
{
@@ -45,7 +47,28 @@ public class UrlEncodedUtf8Test
4547
public void testDecodeAllowBadSequence(String query, String expectedName, String expectedValue)
4648
{
4749
Fields fields = new Fields();
48-
UrlEncoded.decodeUtf8To(query, 0, query.length(), fields::add, true, true, true);
50+
boolean ret = UrlEncoded.decodeUtf8To(query, 0, query.length(), fields::add, true, true, true);
51+
assertFalse(ret, "decodeUtf8To should have returned false to indicate a bad utf-8 encoded value");
52+
Fields.Field field = fields.get(expectedName);
53+
assertThat("Name exists", field, notNullValue());
54+
assertThat("Value", field.getValue(), is(expectedValue));
55+
}
56+
57+
@ParameterizedTest
58+
@CsvSource(delimiter = '|', useHeadersInDisplayName = false,
59+
textBlock = """
60+
# query | expectedName | expectedValue
61+
a=good | a | good
62+
b=go%2fod | b | go/od
63+
c=quot%22 | c | quot"
64+
d=fo o | d | fo o
65+
e=%25TOK%25 | e | %TOK%
66+
""")
67+
public void testDecodeValid(String query, String expectedName, String expectedValue)
68+
{
69+
Fields fields = new Fields();
70+
boolean ret = UrlEncoded.decodeUtf8To(query, 0, query.length(), fields::add, false, false, false);
71+
assertTrue(ret, "decodeUtf8To should have returned true to indicate a good utf-8 encoded value");
4972
Fields.Field field = fields.get(expectedName);
5073
assertThat("Name exists", field, notNullValue());
5174
assertThat("Value", field.getValue(), is(expectedValue));

0 commit comments

Comments
 (0)