Skip to content

Commit

Permalink
[UNDERTOW-2269] Handle query string properly after UNDERTOW-2206
Browse files Browse the repository at this point in the history
  • Loading branch information
baranowb committed May 24, 2023
1 parent 023e16c commit b6de9fe
Show file tree
Hide file tree
Showing 7 changed files with 216 additions and 57 deletions.
3 changes: 3 additions & 0 deletions core/src/main/java/io/undertow/UndertowMessages.java
Original file line number Diff line number Diff line change
Expand Up @@ -650,4 +650,7 @@ public interface UndertowMessages {
@Message(id = 208, value = "Failed to allocate resource")
IOException failedToAllocateResource();

@Message(id = 209, value = "Failed to encode query string '%s' with '%s' encoding.")
IllegalArgumentException failedToEncodeQueryString(final String q, final String e);

}
62 changes: 44 additions & 18 deletions core/src/main/java/io/undertow/util/QueryParameterUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,15 @@

import java.io.UnsupportedEncodingException;
import java.net.URLDecoder;
import java.net.URLEncoder;
import java.nio.charset.StandardCharsets;
import java.util.ArrayDeque;
import java.util.Deque;
import java.util.LinkedHashMap;
import java.util.Map;
import org.xnio.OptionMap;

import io.undertow.UndertowMessages;
import io.undertow.UndertowOptions;
import io.undertow.server.HttpServerExchange;

Expand All @@ -42,33 +44,42 @@ private QueryParameterUtils() {
}

public static String buildQueryString(final Map<String, Deque<String>> params) {
StringBuilder sb = new StringBuilder();
boolean first = true;
for (Map.Entry<String, Deque<String>> entry : params.entrySet()) {
if (entry.getValue().isEmpty()) {
if (first) {
first = false;
} else {
sb.append('&');
}
sb.append(entry.getKey());
sb.append('=');
} else {
for (String val : entry.getValue()) {
return QueryParameterUtils.buildQueryString(params, null);
}

public static String buildQueryString(final Map<String, Deque<String>> params, final String encoding) {
try {
StringBuilder sb = new StringBuilder();
boolean first = true;
for (Map.Entry<String, Deque<String>> entry : params.entrySet()) {
final String key = encoding != null ? URLEncoder.encode(entry.getKey(), encoding) : entry.getKey();
if (entry.getValue().isEmpty()) {
if (first) {
first = false;
} else {
sb.append('&');
}
sb.append(entry.getKey());
sb.append(key);
sb.append('=');
sb.append(val);
} else {
for (String val : entry.getValue()) {
if (first) {
first = false;
} else {
sb.append('&');
}
final String _val = encoding != null ? URLEncoder.encode(val, encoding) : val;
sb.append(key);
sb.append('=');
sb.append(_val);
}
}
}
return sb.toString();
} catch (UnsupportedEncodingException e) {
throw UndertowMessages.MESSAGES.failedToEncodeQueryString(buildQueryString(params), encoding);
}
return sb.toString();
}

/**
* Parses a query string into a map
* @param newQueryString The query string
Expand Down Expand Up @@ -146,8 +157,9 @@ public static Map<String, Deque<String>> mergeQueryParametersWithNewQueryString(
return mergeQueryParametersWithNewQueryString(queryParameters, newQueryString, StandardCharsets.UTF_8.name());
}

@Deprecated
public static Map<String, Deque<String>> mergeQueryParametersWithNewQueryString(final Map<String, Deque<String>> queryParameters, final String newQueryString, final String encoding) {

//DEPRACETED this will create duplicates
Map<String, Deque<String>> newQueryParameters = parseQueryString(newQueryString, encoding);
//according to the spec the new query parameters have to 'take precedence'
for (Map.Entry<String, Deque<String>> entry : queryParameters.entrySet()) {
Expand All @@ -160,6 +172,20 @@ public static Map<String, Deque<String>> mergeQueryParametersWithNewQueryString(
return newQueryParameters;
}

public static Map<String, Deque<String>> mergeQueryParameters(final Map<String, Deque<String>> newParams, final Map<String, Deque<String>> oldParams) {
//according to the spec the new query parameters have to 'take precedence'
for (Map.Entry<String, Deque<String>> entry : oldParams.entrySet()) {
if (!newParams.containsKey(entry.getKey())) {
newParams.put(entry.getKey(), new ArrayDeque<>(entry.getValue()));
} else {
final Deque<String> newValues = newParams.get(entry.getKey());
final Deque<String> oldValues = entry.getValue();
oldValues.stream().filter(v -> !newValues.contains(v)).forEach(v-> newValues.add(v));
}
}
return newParams;
}

public static String getQueryParamEncoding(HttpServerExchange exchange) {
String encoding = null;
OptionMap undertowOptions = exchange.getConnection().getUndertowOptions();
Expand Down
53 changes: 19 additions & 34 deletions servlet/src/main/java/io/undertow/servlet/util/DispatchUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,6 @@
*/
package io.undertow.servlet.util;

import io.undertow.server.Connectors;
import io.undertow.server.HttpServerExchange;
import io.undertow.servlet.handlers.ServletPathMatch;
import io.undertow.servlet.handlers.ServletRequestContext;
import io.undertow.servlet.spec.HttpServletRequestImpl;
import io.undertow.servlet.spec.HttpServletResponseImpl;
import io.undertow.servlet.spec.ServletContextImpl;
import io.undertow.util.ParameterLimitException;
import jakarta.servlet.ServletException;
import java.util.Deque;
import java.util.Map;

import static jakarta.servlet.AsyncContext.ASYNC_CONTEXT_PATH;
import static jakarta.servlet.AsyncContext.ASYNC_MAPPING;
import static jakarta.servlet.AsyncContext.ASYNC_PATH_INFO;
Expand All @@ -54,6 +42,17 @@
import static jakarta.servlet.RequestDispatcher.INCLUDE_REQUEST_URI;
import static jakarta.servlet.RequestDispatcher.INCLUDE_SERVLET_PATH;

import io.undertow.server.Connectors;
import io.undertow.server.HttpServerExchange;
import io.undertow.servlet.handlers.ServletPathMatch;
import io.undertow.servlet.handlers.ServletRequestContext;
import io.undertow.servlet.spec.HttpServletRequestImpl;
import io.undertow.servlet.spec.HttpServletResponseImpl;
import io.undertow.servlet.spec.ServletContextImpl;
import io.undertow.util.ParameterLimitException;
import io.undertow.util.QueryParameterUtils;
import jakarta.servlet.ServletException;

/**
* <p>Utility class to manage the dispatching parsing of the path. The methods
* fill the exchange, request and response with the needed data for the
Expand Down Expand Up @@ -210,24 +209,6 @@ public static ServletPathMatch dispatchAsync(final String path,
return pathMatch;
}

private static Map<String, Deque<String>> mergeQueryParameters(final Map<String, Deque<String>> newParams, final Map<String, Deque<String>> oldParams) {
for (Map.Entry<String, Deque<String>> entry : oldParams.entrySet()) {
Deque<String> values = newParams.get(entry.getKey());
if (values == null) {
// add all the values as new params do not contain this key
newParams.put(entry.getKey(), entry.getValue());
} else {
// merge values new params first
for (String v : entry.getValue()) {
if (!values.contains(v)) {
values.add(v);
}
}
}
}
return newParams;
}

private static String assignRequestPath(final String path, final HttpServletRequestImpl requestImpl,
final ServletContextImpl servletContext, final boolean include) throws ParameterLimitException {
final StringBuilder sb = new StringBuilder();
Expand All @@ -247,13 +228,17 @@ private static String assignRequestPath(final String path, final HttpServletRequ
exchange.setRelativePath(newRequestPath);
exchange.setRequestPath(fake.getRequestPath());
exchange.setRequestURI(fake.getRequestURI());
if (!fake.getQueryString().isEmpty()) {
exchange.setQueryString(fake.getQueryString());
}

}
// both forward and include merge parameters by spec
if (!fake.getQueryString().isEmpty()) {
requestImpl.setQueryParameters(mergeQueryParameters(fake.getQueryParameters(), exchange.getQueryParameters()));
requestImpl.setQueryParameters(QueryParameterUtils.mergeQueryParameters(fake.getQueryParameters(), exchange.getQueryParameters()));
if(!include) {
final String newQueryString = QueryParameterUtils.buildQueryString(requestImpl.getQueryParameters(), QueryParameterUtils.getQueryParamEncoding(exchange));
exchange.setQueryString(newQueryString);
exchange.getQueryParameters().clear();
exchange.getQueryParameters().putAll(requestImpl.getQueryParameters());
}
}
return newRequestPath;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ public void testDispatchAttributesEncoded() throws IOException {
HttpResponse result = client.execute(get);
Assert.assertEquals(StatusCodes.OK, result.getStatusLine().getStatusCode());
String response = HttpClientUtils.readResponse(result);
MatcherAssert.assertThat(response, CoreMatchers.containsString("wrapped: pathInfo:/path%info queryString:n3=v%253 servletPath:/pathinfo requestUri:/servletContext/pathinfo/path%25info\r\n"));
MatcherAssert.assertThat(response, CoreMatchers.containsString("wrapped: pathInfo:/path%info queryString:n1=v%251&n2=v2&n3=v%253 servletPath:/pathinfo requestUri:/servletContext/pathinfo/path%25info\r\n"));
MatcherAssert.assertThat(response, CoreMatchers.containsString("jakarta.servlet.async.request_uri:/servletContext/dispatch/dis%25patch\r\n"));
MatcherAssert.assertThat(response, CoreMatchers.containsString("jakarta.servlet.async.context_path:/servletContext\r\n"));
MatcherAssert.assertThat(response, CoreMatchers.containsString("jakarta.servlet.async.servlet_path:/dispatch\r\n"));
Expand All @@ -293,7 +293,7 @@ public void testDispatchAttributesEncodedExtension() throws IOException {
HttpResponse result = client.execute(get);
Assert.assertEquals(StatusCodes.OK, result.getStatusLine().getStatusCode());
String response = HttpClientUtils.readResponse(result);
MatcherAssert.assertThat(response, CoreMatchers.containsString("wrapped: pathInfo:null queryString:n3=v%253 servletPath:/path%info.info requestUri:/servletContext/path%25info.info\r\n"));
MatcherAssert.assertThat(response, CoreMatchers.containsString("wrapped: pathInfo:null queryString:n1=v%251&n2=v2&n3=v%253 servletPath:/path%info.info requestUri:/servletContext/path%25info.info\r\n"));
MatcherAssert.assertThat(response, CoreMatchers.containsString("jakarta.servlet.async.request_uri:/servletContext/dis%25patch.dispatch\r\n"));
MatcherAssert.assertThat(response, CoreMatchers.containsString("jakarta.servlet.async.context_path:/servletContext\r\n"));
MatcherAssert.assertThat(response, CoreMatchers.containsString("jakarta.servlet.async.servlet_path:/dis%patch.dispatch\r\n"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ public void testIncludeAggregatesQueryString() throws IOException, InterruptedEx
result = client.execute(get);
assertEquals(StatusCodes.OK, result.getStatusLine().getStatusCode());
response = HttpClientUtils.readResponse(result);
assertEquals("pathInfo:null queryString:foo=bar servletPath:/path requestUri:/servletContext/path", response);
assertEquals("pathInfo:null queryString:a=b&foo=bar servletPath:/path requestUri:/servletContext/path", response);
latch.await(30, TimeUnit.SECONDS);
//UNDERTOW-327 and UNDERTOW-1599 - make sure that the access log includes the original path and query string
assertEquals("GET /servletContext/dispatch?a=b " + protocol + " /servletContext/dispatch /dispatch", message);
Expand Down Expand Up @@ -319,7 +319,7 @@ public void testAttributesEncoded() throws IOException {
HttpResponse result = client.execute(get);
assertEquals(StatusCodes.OK, result.getStatusLine().getStatusCode());
String response = HttpClientUtils.readResponse(result);
MatcherAssert.assertThat(response, CoreMatchers.containsString("pathInfo:/path%info queryString:n3=V%253 servletPath:/path-forward requestUri:/servletContext/path-forward/path%25info\r\n"));
MatcherAssert.assertThat(response, CoreMatchers.containsString("pathInfo:/path%info queryString:n1=v%251&n2=v%252&n3=V%253 servletPath:/path-forward requestUri:/servletContext/path-forward/path%25info\r\n"));
MatcherAssert.assertThat(response, CoreMatchers.containsString("jakarta.servlet.forward.request_uri:/servletContext/dispatch/dispatch%25info\r\n"));
MatcherAssert.assertThat(response, CoreMatchers.containsString("jakarta.servlet.forward.context_path:/servletContext\r\n"));
MatcherAssert.assertThat(response, CoreMatchers.containsString("jakarta.servlet.forward.servlet_path:/dispatch\r\n"));
Expand All @@ -340,7 +340,7 @@ public void testAttributesEncodedExtension() throws IOException {
HttpResponse result = client.execute(get);
assertEquals(StatusCodes.OK, result.getStatusLine().getStatusCode());
String response = HttpClientUtils.readResponse(result);
MatcherAssert.assertThat(response, CoreMatchers.containsString("pathInfo:null queryString:n3=V%253 servletPath:/to%forward/path%info.forwardinfo requestUri:/servletContext/to%25forward/path%25info.forwardinfo\r\n"));
MatcherAssert.assertThat(response, CoreMatchers.containsString("pathInfo:null queryString:n1=v%251&n2=v%252&n3=V%253 servletPath:/to%forward/path%info.forwardinfo requestUri:/servletContext/to%25forward/path%25info.forwardinfo\r\n"));
MatcherAssert.assertThat(response, CoreMatchers.containsString("jakarta.servlet.forward.request_uri:/servletContext/dis%25patch/dispatch%25info.dispatch\r\n"));
MatcherAssert.assertThat(response, CoreMatchers.containsString("jakarta.servlet.forward.context_path:/servletContext\r\n"));
MatcherAssert.assertThat(response, CoreMatchers.containsString("jakarta.servlet.forward.servlet_path:/dis%patch/dispatch%info.dispatch\r\n"));
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
/*
* JBoss, Home of Professional Open Source.
* Copyright 2023 Red Hat, Inc., and individual contributors
* as indicated by the @author tags.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.undertow.servlet.test.dispatcher.query;

import static org.junit.Assert.assertEquals;

import java.io.IOException;

import org.apache.http.HttpResponse;
import org.apache.http.client.methods.HttpGet;
import org.junit.Assume;
import org.junit.BeforeClass;
import org.junit.Test;
import org.junit.runner.RunWith;

import io.undertow.server.handlers.PathHandler;
import io.undertow.servlet.api.DeploymentInfo;
import io.undertow.servlet.api.DeploymentManager;
import io.undertow.servlet.api.ServletContainer;
import io.undertow.servlet.api.ServletInfo;
import io.undertow.servlet.test.util.TestClassIntrospector;
import io.undertow.servlet.test.util.TestResourceLoader;
import io.undertow.testutils.DefaultServer;
import io.undertow.testutils.HttpClientUtils;
import io.undertow.testutils.TestHttpClient;
import io.undertow.util.StatusCodes;
import jakarta.servlet.ServletException;

/**
* @author baranowb
*/
@RunWith(DefaultServer.class)
public class ForwardQueryParametersTestCase {

@BeforeClass
public static void setup() throws ServletException {
// we don't run this test on h2 upgrade, as if it is run with the original request
// the protocols will not match
Assume.assumeFalse(DefaultServer.isH2upgrade());
final PathHandler root = new PathHandler();
final ServletContainer container = ServletContainer.Factory.newInstance();

DeploymentInfo builder = new DeploymentInfo().setClassLoader(ForwardQueryParametersTestCase.class.getClassLoader())
.setContextPath("/servletContext").setClassIntrospecter(TestClassIntrospector.INSTANCE)
.setDeploymentName("servletContext.war")
.setResourceManager(new TestResourceLoader(ForwardQueryParametersTestCase.class))
.addServlet(new ServletInfo("initial", ForwardingServlet.class)
.addInitParam(ForwardingServlet.PARAM_NAME, "initial")
.addInitParam(ForwardingServlet.PARAM_VALUE, "was-here")
.addInitParam(ForwardingServlet.FWD_TARGET, "/fwd1")
.addMapping("/initial"))
.addServlet(
new ServletInfo("fwd1", ForwardingServlet.class).addInitParam(ForwardingServlet.PARAM_NAME, "second")
.addInitParam(ForwardingServlet.PARAM_VALUE, "wasnt-here")
.addInitParam(ForwardingServlet.FWD_TARGET, "/fwd2")
.addMapping("/fwd1"))
.addServlet(new ServletInfo("fwd2", ForwardingServlet.class)
.addMapping("/fwd2"));

DeploymentManager manager = container.addDeployment(builder);
manager.deploy();
root.addPrefixPath(builder.getContextPath(), manager.start());
DefaultServer.setRootHandler(root);
}

@Test
public void testPathBasedInclude() throws IOException, InterruptedException {
TestHttpClient client = new TestHttpClient();
try {
HttpGet get = new HttpGet(DefaultServer.getDefaultServerURL() + "/servletContext/initial");
HttpResponse result = client.execute(get);
assertEquals(StatusCodes.OK, result.getStatusLine().getStatusCode());
final String response = HttpClientUtils.readResponse(result);
assertEquals("initial=was-here&second=wasnt-here", response);
} finally {
client.getConnectionManager().shutdown();
}
}
}

0 comments on commit b6de9fe

Please sign in to comment.