Skip to content

Commit

Permalink
Issue #6473 - normalize only once
Browse files Browse the repository at this point in the history 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
  • Loading branch information
@lachlan-roberts
lachlan-roberts committed Jun 27, 2021
1 parent a3effb1 commit ab49940
Show file tree
Hide file tree
Showing 29 changed files with 534 additions and 260 deletions.
17 changes: 17 additions & 0 deletions jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -214,4 +214,21 @@ public EnumSet<HttpComplianceSection> sections()
{
return _sections;
}

public static String checkUriCompliance(HttpCompliance compliance, HttpURI uri)
{
if (uri.hasUtf16Encoding() && (compliance == null || compliance.sections().contains(HttpComplianceSection.NO_UTF16_ENCODINGS)))
return "UTF16 encoding not supported";
if (uri.hasAmbiguousSegment() && (compliance == null || compliance.sections().contains(HttpComplianceSection.NO_AMBIGUOUS_PATH_SEGMENTS)))
return "Ambiguous segment in URI";
if (uri.hasAmbiguousEmptySegment() && (compliance == null || compliance.sections().contains(HttpComplianceSection.NO_AMBIGUOUS_EMPTY_SEGMENT)))
return "Ambiguous empty segment in URI";
if (uri.hasAmbiguousSeparator() && (compliance == null || compliance.sections().contains(HttpComplianceSection.NO_AMBIGUOUS_PATH_SEPARATORS)))
return "Ambiguous separator in URI";
if (uri.hasAmbiguousParameter() && (compliance == null || compliance.sections().contains(HttpComplianceSection.NO_AMBIGUOUS_PATH_PARAMETERS)))
return "Ambiguous path parameter in URI";
if (uri.hasAmbiguousEncoding() && (compliance == null || compliance.sections().contains(HttpComplianceSection.NO_AMBIGUOUS_PATH_ENCODING)))
return "Ambiguous path encoding in URI";
return null;
}
}
20 changes: 19 additions & 1 deletion jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,22 @@ public HttpURI(HttpURI uri)
_emptySegment = false;
}

public HttpURI(HttpURI schemeHostPort, HttpURI uri)
{
_scheme = schemeHostPort._scheme;
_user = schemeHostPort._user;
_host = schemeHostPort._host;
_port = schemeHostPort._port;
_path = uri._path;
_param = uri._param;
_query = uri._query;
_fragment = uri._fragment;
_uri = uri._uri;
_decodedPath = uri._decodedPath;
_violations.addAll(uri._violations);
_emptySegment = false;
}

public HttpURI(String uri)
{
_port = -1;
Expand Down Expand Up @@ -506,6 +522,8 @@ else if (c == '/')
{
switch (encodedValue)
{
case 0:
throw new IllegalArgumentException("Illegal character in path");
case '/':
_violations.add(Violation.SEPARATOR);
break;
Expand Down Expand Up @@ -677,7 +695,7 @@ else if (c == '/')
}
else if (_path != null)
{
String canonical = URIUtil.canonicalPath(_path);
String canonical = URIUtil.canonicalEncodedPath(_path);
if (canonical == null)
throw new BadMessageException("Bad URI");
_decodedPath = URIUtil.decodePath(canonical);
Expand Down
8 changes: 8 additions & 0 deletions jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,11 @@ public void testParse()
uri.parse("http://foo/bar");
assertThat(uri.getHost(), is("foo"));
assertThat(uri.getPath(), is("/bar"));

// We do allow nulls if not encoded. This can be used for testing 2nd line of defence.
uri.parse("http://fo\000/bar");
assertThat(uri.getHost(), is("fo\000"));
assertThat(uri.getPath(), is("/bar"));
}

@Test
Expand Down Expand Up @@ -316,6 +321,7 @@ public static Stream<Arguments> decodePathTests()
// encoded paths
{"/f%6f%6F/bar", "/foo/bar", EnumSet.noneOf(Violation.class)},
{"/f%u006f%u006F/bar", "/foo/bar", EnumSet.of(Violation.UTF16)},
{"/f%u0001%u0001/bar", "/f\001\001/bar", EnumSet.of(Violation.UTF16)},

// illegal paths
{"//host/../path/info", null, EnumSet.noneOf(Violation.class)},
Expand All @@ -325,6 +331,8 @@ public static Stream<Arguments> decodePathTests()
{"/path/%2/F/info", null, EnumSet.noneOf(Violation.class)},
{"/path/%/info", null, EnumSet.noneOf(Violation.class)},
{"/path/%u000X/info", null, EnumSet.noneOf(Violation.class)},
{"/path/Fo%u0000/info", null, EnumSet.noneOf(Violation.class)},
{"/path/Fo%00/info", null, EnumSet.noneOf(Violation.class)},

// ambiguous dot encodings
{"scheme://host/path/%2e/info", "/path/./info", EnumSet.of(Violation.SEGMENT)},
Expand Down
15 changes: 5 additions & 10 deletions jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyWebAppContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
import org.eclipse.jetty.servlet.ServletHolder;
import org.eclipse.jetty.servlet.ServletMapping;
import org.eclipse.jetty.util.StringUtil;
import org.eclipse.jetty.util.URIUtil;
import org.eclipse.jetty.util.log.Log;
import org.eclipse.jetty.util.log.Logger;
import org.eclipse.jetty.util.resource.Resource;
Expand Down Expand Up @@ -451,16 +450,12 @@ public Resource getResource(String uriInContext) throws MalformedURLException
// If no regular resource exists check for access to /WEB-INF/lib or /WEB-INF/classes
if ((resource == null || !resource.exists()) && uriInContext != null && _classes != null)
{
String uri = URIUtil.canonicalPath(uriInContext);
if (uri == null)
return null;

try
{
// Replace /WEB-INF/classes with candidates for the classpath
if (uri.startsWith(WEB_INF_CLASSES_PREFIX))
if (uriInContext.startsWith(WEB_INF_CLASSES_PREFIX))
{
if (uri.equalsIgnoreCase(WEB_INF_CLASSES_PREFIX) || uri.equalsIgnoreCase(WEB_INF_CLASSES_PREFIX + "/"))
if (uriInContext.equalsIgnoreCase(WEB_INF_CLASSES_PREFIX) || uriInContext.equalsIgnoreCase(WEB_INF_CLASSES_PREFIX + "/"))
{
//exact match for a WEB-INF/classes, so preferentially return the resource matching the web-inf classes
//rather than the test classes
Expand All @@ -476,7 +471,7 @@ else if (_testClasses != null)
int i = 0;
while (res == null && (i < _webInfClasses.size()))
{
String newPath = StringUtil.replace(uri, WEB_INF_CLASSES_PREFIX, _webInfClasses.get(i).getPath());
String newPath = StringUtil.replace(uriInContext, WEB_INF_CLASSES_PREFIX, _webInfClasses.get(i).getPath());
res = Resource.newResource(newPath);
if (!res.exists())
{
Expand All @@ -487,11 +482,11 @@ else if (_testClasses != null)
return res;
}
}
else if (uri.startsWith(WEB_INF_LIB_PREFIX))
else if (uriInContext.startsWith(WEB_INF_LIB_PREFIX))
{
// Return the real jar file for all accesses to
// /WEB-INF/lib/*.jar
String jarName = StringUtil.strip(uri, WEB_INF_LIB_PREFIX);
String jarName = StringUtil.strip(uriInContext, WEB_INF_LIB_PREFIX);
if (jarName.startsWith("/") || jarName.startsWith("\\"))
jarName = jarName.substring(1);
if (jarName.length() == 0)
Expand Down
4 changes: 2 additions & 2 deletions jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/RedirectUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,14 @@ public static String toRedirectURL(final HttpServletRequest request, String loca
if (location.startsWith("/"))
{
// absolute in context
location = URIUtil.canonicalEncodedPath(location);
location = URIUtil.canonicalURI(location);
}
else
{
// relative to request
String path = request.getRequestURI();
String parent = (path.endsWith("/")) ? path : URIUtil.parentPath(path);
location = URIUtil.canonicalPath(URIUtil.addEncodedPaths(parent, location));
location = URIUtil.canonicalURI(URIUtil.addEncodedPaths(parent, location));
if (!location.startsWith("/"))
url.append('/');
}
Expand Down
18 changes: 3 additions & 15 deletions jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/ValidUrlRuleTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@

package org.eclipse.jetty.rewrite.handler;

import org.eclipse.jetty.http.HttpURI;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertEquals;
Expand Down Expand Up @@ -65,7 +65,7 @@ public void testInvalidUrlWithReason() throws Exception
{
_rule.setCode("405");
_rule.setReason("foo");
_request.setURIPathQuery("/%00/");
_request.setURIPathQuery("/%01/");

String result = _rule.matchAndApply(_request.getRequestURI(), _request, _response);

Expand All @@ -78,20 +78,8 @@ public void testInvalidJsp() throws Exception
{
_rule.setCode("405");
_rule.setReason("foo");
_request.setURIPathQuery("/jsp/bean1.jsp%00");

String result = _rule.matchAndApply(_request.getRequestURI(), _request, _response);

assertEquals(405, _response.getStatus());
assertEquals("foo", _response.getReason());
}

@Test
public void testInvalidShamrock() throws Exception
{
_rule.setCode("405");
_rule.setReason("foo");
_request.setURIPathQuery("/jsp/shamrock-%00%E2%98%98.jsp");
_request.setHttpURI(new HttpURI("/jsp/bean1.jsp\000"));

String result = _rule.matchAndApply(_request.getRequestURI(), _request, _response);

Expand Down
35 changes: 30 additions & 5 deletions jetty-server/src/main/java/org/eclipse/jetty/server/Dispatcher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import java.io.IOException;
import java.util.HashSet;
import java.util.Objects;
import java.util.Set;
import javax.servlet.DispatcherType;
import javax.servlet.RequestDispatcher;
Expand All @@ -30,7 +31,9 @@
import javax.servlet.http.HttpServletResponse;

import org.eclipse.jetty.http.BadMessageException;
import org.eclipse.jetty.http.HttpCompliance;
import org.eclipse.jetty.http.HttpURI;
import org.eclipse.jetty.io.Connection;
import org.eclipse.jetty.server.handler.ContextHandler;
import org.eclipse.jetty.util.Attributes;
import org.eclipse.jetty.util.MultiMap;
Expand Down Expand Up @@ -86,7 +89,7 @@ public void error(ServletRequest request, ServletResponse response) throws Servl
@Override
public void include(ServletRequest request, ServletResponse response) throws ServletException, IOException
{
Request baseRequest = Request.getBaseRequest(request);
Request baseRequest = Objects.requireNonNull(Request.getBaseRequest(request));

if (!(request instanceof HttpServletRequest))
request = new ServletRequestHttpWrapper(request);
Expand All @@ -106,6 +109,10 @@ public void include(ServletRequest request, ServletResponse response) throws Ser
}
else
{
Objects.requireNonNull(_uri);
// Check any URI violations against the compliance for this request
checkUriViolations(_uri, baseRequest);

IncludeAttributes attr = new IncludeAttributes(old_attr);

attr._requestURI = _uri.getPath();
Expand Down Expand Up @@ -133,7 +140,7 @@ public void include(ServletRequest request, ServletResponse response) throws Ser

protected void forward(ServletRequest request, ServletResponse response, DispatcherType dispatch) throws ServletException, IOException
{
Request baseRequest = Request.getBaseRequest(request);
Request baseRequest = Objects.requireNonNull(Request.getBaseRequest(request));
Response baseResponse = baseRequest.getResponse();
baseResponse.resetForForward();

Expand Down Expand Up @@ -161,6 +168,10 @@ protected void forward(ServletRequest request, ServletResponse response, Dispatc
}
else
{
Objects.requireNonNull(_uri);
// Check any URI violations against the compliance for this request
checkUriViolations(_uri, baseRequest);

ForwardAttributes attr = new ForwardAttributes(old_attr);

//If we have already been forwarded previously, then keep using the established
Expand All @@ -184,9 +195,8 @@ protected void forward(ServletRequest request, ServletResponse response, Dispatc
attr._servletPath = old_servlet_path;
}

HttpURI uri = new HttpURI(old_uri.getScheme(), old_uri.getHost(), old_uri.getPort(),
_uri.getPath(), _uri.getParam(), _uri.getQuery(), _uri.getFragment());

// Combine old and new URIs.
HttpURI uri = new HttpURI(old_uri, _uri);
baseRequest.setHttpURI(uri);

baseRequest.setContextPath(_contextHandler.getContextPath());
Expand Down Expand Up @@ -245,6 +255,21 @@ protected void forward(ServletRequest request, ServletResponse response, Dispatc
}
}

private static void checkUriViolations(HttpURI uri, Request baseRequest)
{
if (uri.hasViolations())
{
HttpChannel channel = baseRequest.getHttpChannel();
Connection connection = channel == null ? null : channel.getConnection();
HttpCompliance compliance = connection instanceof HttpConnection
? ((HttpConnection)connection).getHttpCompliance()
: channel != null ? channel.getConnector().getBean(HttpCompliance.class) : null;
String illegalState = HttpCompliance.checkUriCompliance(compliance, uri);
if (illegalState != null)
throw new IllegalStateException(illegalState);
}
}

@Override
public String toString()
{
Expand Down
20 changes: 3 additions & 17 deletions jetty-server/src/main/java/org/eclipse/jetty/server/Request.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@
import org.eclipse.jetty.http.BadMessageException;
import org.eclipse.jetty.http.HostPortHttpField;
import org.eclipse.jetty.http.HttpCompliance;
import org.eclipse.jetty.http.HttpComplianceSection;
import org.eclipse.jetty.http.HttpCookie;
import org.eclipse.jetty.http.HttpField;
import org.eclipse.jetty.http.HttpFields;
Expand Down Expand Up @@ -1833,23 +1832,10 @@ public void setMetaData(org.eclipse.jetty.http.MetaData.Request request)
? ((HttpConnection)connection).getHttpCompliance()
: _channel != null ? _channel.getConnector().getBean(HttpCompliance.class) : null;

if (uri.hasUtf16Encoding() && (compliance == null || compliance.sections().contains(HttpComplianceSection.NO_UTF16_ENCODINGS)))
throw new BadMessageException("UTF16 % encoding not supported");

ambiguous = uri.isAmbiguous();
if (ambiguous)
{
if (uri.hasAmbiguousSegment() && (compliance == null || compliance.sections().contains(HttpComplianceSection.NO_AMBIGUOUS_PATH_SEGMENTS)))
throw new BadMessageException("Ambiguous segment in URI");
if (uri.hasAmbiguousEmptySegment() && (compliance == null || compliance.sections().contains(HttpComplianceSection.NO_AMBIGUOUS_EMPTY_SEGMENT)))
throw new BadMessageException("Ambiguous empty segment in URI");
if (uri.hasAmbiguousSeparator() && (compliance == null || compliance.sections().contains(HttpComplianceSection.NO_AMBIGUOUS_PATH_SEPARATORS)))
throw new BadMessageException("Ambiguous separator in URI");
if (uri.hasAmbiguousParameter() && (compliance == null || compliance.sections().contains(HttpComplianceSection.NO_AMBIGUOUS_PATH_PARAMETERS)))
throw new BadMessageException("Ambiguous path parameter in URI");
if (uri.hasAmbiguousEncoding() && (compliance == null || compliance.sections().contains(HttpComplianceSection.NO_AMBIGUOUS_PATH_ENCODING)))
throw new BadMessageException("Ambiguous path encoding in URI");
}
String badMessage = HttpCompliance.checkUriCompliance(compliance, uri);
if (badMessage != null)
throw new BadMessageException(badMessage);
}

_originalURI = uri.isAbsolute() && request.getHttpVersion() != HttpVersion.HTTP_2 ? uri.toString() : uri.getPathQuery();
Expand Down
4 changes: 2 additions & 2 deletions jetty-server/src/main/java/org/eclipse/jetty/server/Response.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -547,14 +547,14 @@ public void sendRedirect(int code, String location, boolean consumeAll) throws I
if (location.startsWith("/"))
{
// absolute in context
location = URIUtil.canonicalEncodedPath(location);
location = URIUtil.canonicalURI(location);
}
else
{
// relative to request
String path = _channel.getRequest().getRequestURI();
String parent = (path.endsWith("/")) ? path : URIUtil.parentPath(path);
location = URIUtil.canonicalEncodedPath(URIUtil.addEncodedPaths(parent, location));
location = URIUtil.canonicalURI(URIUtil.addEncodedPaths(parent, location));
if (location != null && !location.startsWith("/"))
buf.append('/');
}
Expand Down
Loading

0 comments on commit ab49940

Please sign in to comment.