Skip to content

Commit

Permalink
Fully decode #9444
Browse files Browse the repository at this point in the history
getServletPath and getPathInfo will never return an encoded path segment. Instead, they will throw an IllegalArgumentException if they are called when there is a URI with violations.
  • Loading branch information
gregw committed Mar 7, 2023
1 parent d6a7fab commit e73f747
Show file tree
Hide file tree
Showing 10 changed files with 51 additions and 53 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
import org.eclipse.jetty.http.MetaData;
import org.eclipse.jetty.http.Trailers;
import org.eclipse.jetty.io.Content;
import org.eclipse.jetty.server.handler.ErrorHandler;
import org.eclipse.jetty.server.internal.HttpChannelState;
import org.eclipse.jetty.util.Attributes;
import org.eclipse.jetty.util.Callback;
Expand Down Expand Up @@ -726,14 +725,14 @@ static long getContentBytesRead(Request request)
* {@code newPathInContext=/newPath}, the returned HttpURI is {@code http://host/ctx/newPath?a=b}.</p>
*
* @param request The request to base the new HttpURI on.
* @param newPathInContext The new path in context for the new HttpURI
* @param newEncodedPathInContext The new path in context for the new HttpURI
* @return A new immutable HttpURI with the path in context replaced, but query string and path
* parameters retained.
*/
static HttpURI newHttpURIFrom(Request request, String newPathInContext)
static HttpURI newHttpURIFrom(Request request, String newEncodedPathInContext)
{
return HttpURI.build(request.getHttpURI())
.path(URIUtil.addPaths(getContextPath(request), newPathInContext))
.path(URIUtil.addPaths(getContextPath(request), newEncodedPathInContext))
.asImmutable();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -359,22 +359,22 @@ protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws Se
{
String includedServletPath = (String)req.getAttribute(RequestDispatcher.INCLUDE_SERVLET_PATH);
boolean included = includedServletPath != null;
String pathInContext;
String decodedPathInContext;
if (included)
pathInContext = getIncludedPathInContext(req, includedServletPath, isPathInfoOnly());
decodedPathInContext = getIncludedPathInContext(req, includedServletPath, isPathInfoOnly());
else if (req instanceof ServletApiRequest apiRequest)
pathInContext = apiRequest.getServletContextRequest().getPathInContext();
decodedPathInContext = apiRequest.getServletContextRequest().getDecodedPathInContext();
else
pathInContext = URIUtil.addPaths(isPathInfoOnly() ? "/" : req.getServletPath(), req.getPathInfo());
decodedPathInContext = URIUtil.addPaths(isPathInfoOnly() ? "/" : req.getServletPath(), req.getPathInfo());

pathInContext = URIUtil.encodePath(pathInContext);
String encodedPathInContext = URIUtil.encodePath(decodedPathInContext);

if (LOG.isDebugEnabled())
LOG.debug("doGet(req={}, resp={}) pathInContext={}, included={}", req, resp, pathInContext, included);
LOG.debug("doGet(req={}, resp={}) pathInContext={}, included={}", req, resp, encodedPathInContext, included);

try
{
HttpContent content = _resourceService.getContent(pathInContext, ServletContextRequest.getServletContextRequest(req));
HttpContent content = _resourceService.getContent(encodedPathInContext, ServletContextRequest.getServletContextRequest(req));
if (LOG.isDebugEnabled())
LOG.debug("content = {}", content);

Expand All @@ -388,7 +388,7 @@ else if (req instanceof ServletApiRequest apiRequest)
* If the exception isn’t caught and handled, and the response
* hasn’t been committed, the status code MUST be set to 500.
*/
throw new FileNotFoundException(pathInContext);
throw new FileNotFoundException(encodedPathInContext);
}

// no content
Expand Down Expand Up @@ -436,9 +436,9 @@ else if (req instanceof ServletApiRequest apiRequest)
catch (InvalidPathException e)
{
if (LOG.isDebugEnabled())
LOG.debug("InvalidPathException for pathInContext: {}", pathInContext, e);
LOG.debug("InvalidPathException for pathInContext: {}", encodedPathInContext, e);
if (included)
throw new FileNotFoundException(pathInContext);
throw new FileNotFoundException(encodedPathInContext);
resp.setStatus(404);
}
}
Expand Down Expand Up @@ -485,9 +485,9 @@ private static class ServletCoreRequest extends Request.Wrapper
if (request.getDispatcherType() == DispatcherType.REQUEST)
_uri = getWrapped().getHttpURI();
else if (included)
_uri = Request.newHttpURIFrom(getWrapped(), getIncludedPathInContext(request, includedServletPath, false));
_uri = Request.newHttpURIFrom(getWrapped(), URIUtil.encodePath(getIncludedPathInContext(request, includedServletPath, false)));
else
_uri = Request.newHttpURIFrom(getWrapped(), URIUtil.addPaths(_servletRequest.getServletPath(), _servletRequest.getPathInfo()));
_uri = Request.newHttpURIFrom(getWrapped(), URIUtil.encodePath(URIUtil.addPaths(_servletRequest.getServletPath(), _servletRequest.getPathInfo())));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
import org.eclipse.jetty.ee10.servlet.util.ServletOutputStreamWrapper;
import org.eclipse.jetty.http.HttpURI;
import org.eclipse.jetty.http.pathmap.MatchedResource;
import org.eclipse.jetty.server.Request;
import org.eclipse.jetty.util.Fields;
import org.eclipse.jetty.util.MultiMap;
import org.eclipse.jetty.util.StringUtil;
Expand Down Expand Up @@ -64,30 +63,30 @@ public class Dispatcher implements RequestDispatcher

private final ServletContextHandler _contextHandler;
private final HttpURI _uri;
private final String _pathInContext;
private final String _decodedPathInContext;
private final String _named;
private final ServletHandler.MappedServlet _mappedServlet;
private final ServletHandler _servletHandler;
private final ServletPathMapping _servletPathMapping;

public Dispatcher(ServletContextHandler contextHandler, HttpURI uri, String pathInContext)
public Dispatcher(ServletContextHandler contextHandler, HttpURI uri, String decodedPathInContext)
{
_contextHandler = contextHandler;
_uri = uri.asImmutable();
_pathInContext = pathInContext;
_decodedPathInContext = decodedPathInContext;
_named = null;

_servletHandler = _contextHandler.getServletHandler();
MatchedResource<ServletHandler.MappedServlet> matchedServlet = _servletHandler.getMatchedServlet(pathInContext);
MatchedResource<ServletHandler.MappedServlet> matchedServlet = _servletHandler.getMatchedServlet(decodedPathInContext);
_mappedServlet = matchedServlet.getResource();
_servletPathMapping = _mappedServlet.getServletPathMapping(_pathInContext, matchedServlet.getMatchedPath());
_servletPathMapping = _mappedServlet.getServletPathMapping(_decodedPathInContext, matchedServlet.getMatchedPath());
}

public Dispatcher(ServletContextHandler contextHandler, String name) throws IllegalStateException
{
_contextHandler = contextHandler;
_uri = null;
_pathInContext = null;
_decodedPathInContext = null;
_named = name;

_servletHandler = _contextHandler.getServletHandler();
Expand All @@ -100,7 +99,7 @@ public void error(ServletRequest request, ServletResponse response) throws Servl
HttpServletRequest httpRequest = (request instanceof HttpServletRequest) ? (HttpServletRequest)request : new ServletRequestHttpWrapper(request);
HttpServletResponse httpResponse = (response instanceof HttpServletResponse) ? (HttpServletResponse)response : new ServletResponseHttpWrapper(response);

_mappedServlet.handle(_servletHandler, _pathInContext, new ErrorRequest(httpRequest), httpResponse);
_mappedServlet.handle(_servletHandler, _decodedPathInContext, new ErrorRequest(httpRequest), httpResponse);
}

@Override
Expand All @@ -111,7 +110,7 @@ public void forward(ServletRequest request, ServletResponse response) throws Ser

ServletContextRequest servletContextRequest = ServletContextRequest.getServletContextRequest(request);
servletContextRequest.getResponse().resetForForward();
_mappedServlet.handle(_servletHandler, _pathInContext, new ForwardRequest(httpRequest), httpResponse);
_mappedServlet.handle(_servletHandler, _decodedPathInContext, new ForwardRequest(httpRequest), httpResponse);

// If we are not async and not closed already, then close via the possibly wrapped response.
if (!servletContextRequest.getState().isAsync() && !servletContextRequest.getHttpOutput().isClosed())
Expand All @@ -136,7 +135,7 @@ public void include(ServletRequest request, ServletResponse response) throws Ser

try
{
_mappedServlet.handle(_servletHandler, _pathInContext, new IncludeRequest(httpRequest), new IncludeResponse(httpResponse));
_mappedServlet.handle(_servletHandler, _decodedPathInContext, new IncludeRequest(httpRequest), new IncludeResponse(httpResponse));
}
finally
{
Expand All @@ -149,7 +148,7 @@ public void async(ServletRequest request, ServletResponse response) throws Servl
HttpServletRequest httpRequest = (request instanceof HttpServletRequest) ? (HttpServletRequest)request : new ServletRequestHttpWrapper(request);
HttpServletResponse httpResponse = (response instanceof HttpServletResponse) ? (HttpServletResponse)response : new ServletResponseHttpWrapper(response);

_mappedServlet.handle(_servletHandler, _pathInContext, new AsyncRequest(httpRequest), httpResponse);
_mappedServlet.handle(_servletHandler, _decodedPathInContext, new AsyncRequest(httpRequest), httpResponse);
}

public class ParameterRequestWrapper extends HttpServletRequestWrapper
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1135,7 +1135,7 @@ public RequestDispatcher getRequestDispatcher(String path)
// handle relative path
if (!path.startsWith("/"))
{
String relTo = _request.getPathInContext();
String relTo = _request.getDecodedPathInContext();
int slash = relTo.lastIndexOf("/");
if (slash > 1)
relTo = relTo.substring(0, slash + 1);
Expand Down Expand Up @@ -1213,7 +1213,7 @@ public AsyncContext startAsync(ServletRequest servletRequest, ServletResponse se
@Override
public HttpServletMapping getHttpServletMapping()
{
return _request._mappedServlet.getServletPathMapping(_request.getPathInContext());
return _request._mappedServlet.getServletPathMapping(_request.getDecodedPathInContext());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -460,9 +460,9 @@ public boolean handle()
}
}
// We first worked with the core pathInContext above, but now need to convert to servlet style
pathInContext = URIUtil.decodePath(pathInContext);
String decodedPathInContext = URIUtil.decodePath(pathInContext);

Dispatcher dispatcher = new Dispatcher(getContextHandler(), uri, pathInContext);
Dispatcher dispatcher = new Dispatcher(getContextHandler(), uri, decodedPathInContext);
dispatcher.async(asyncContextEvent.getSuppliedRequest(), asyncContextEvent.getSuppliedResponse());
});
break;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1150,20 +1150,20 @@ protected void doStop() throws Exception
protected ServletContextRequest newServletContextRequest(ServletChannel servletChannel,
Request request,
Response response,
String pathInContext,
String decodedPathInContext,
MatchedResource<ServletHandler.MappedServlet> matchedResource)
{
return new ServletContextRequest(_servletContext, servletChannel, request, response, pathInContext, matchedResource, getSessionHandler());
return new ServletContextRequest(_servletContext, servletChannel, request, response, decodedPathInContext, matchedResource, getSessionHandler());
}

@Override
protected ServletContextRequest wrapRequest(Request request, Response response)
{
// Need to ask directly to the Context for the pathInContext, rather than using
// Request.getPathInContext(), as the request is not yet wrapped in this Context.
String pathInContext = URIUtil.decodePath(getContext().getPathInContext(request.getHttpURI().getCanonicalPath()));
String decodedPathInContext = URIUtil.decodePath(getContext().getPathInContext(request.getHttpURI().getCanonicalPath()));

MatchedResource<ServletHandler.MappedServlet> matchedResource = _servletHandler.getMatchedServlet(pathInContext);
MatchedResource<ServletHandler.MappedServlet> matchedResource = _servletHandler.getMatchedServlet(decodedPathInContext);
if (matchedResource == null)
return null;
ServletHandler.MappedServlet mappedServlet = matchedResource.getResource();
Expand All @@ -1179,7 +1179,7 @@ protected ServletContextRequest wrapRequest(Request request, Response response)
cache.setAttribute(ServletChannel.class.getName(), servletChannel);
}

ServletContextRequest servletContextRequest = newServletContextRequest(servletChannel, request, response, pathInContext, matchedResource);
ServletContextRequest servletContextRequest = newServletContextRequest(servletChannel, request, response, decodedPathInContext, matchedResource);
servletChannel.associate(servletContextRequest);
return servletContextRequest;
}
Expand All @@ -1197,7 +1197,7 @@ protected boolean handleByContextHandler(String pathInContext, ContextRequest re
{
ServletContextRequest scopedRequest = Request.as(request, ServletContextRequest.class);
DispatcherType dispatch = scopedRequest.getHttpServletRequest().getDispatcherType();
if (dispatch == DispatcherType.REQUEST && isProtectedTarget(scopedRequest.getPathInContext()))
if (dispatch == DispatcherType.REQUEST && isProtectedTarget(scopedRequest.getDecodedPathInContext()))
{
Response.writeError(request, response, callback, HttpServletResponse.SC_NOT_FOUND, null);
return true;
Expand Down Expand Up @@ -2824,16 +2824,16 @@ public RequestDispatcher getRequestDispatcher(String uriInContext)
String contextPath = getContextPath();
// uriInContext is canonicalized by HttpURI.
HttpURI.Mutable uri = HttpURI.build(uriInContext);
String pathInfo = uri.getCanonicalPath();
if (StringUtil.isEmpty(pathInfo))
String encodedPathInContext = uri.getCanonicalPath();
if (StringUtil.isEmpty(encodedPathInContext))
return null;

if (!StringUtil.isEmpty(contextPath))
{
uri.path(URIUtil.addPaths(contextPath, uri.getPath()));
pathInfo = uri.getCanonicalPath().substring(contextPath.length());
encodedPathInContext = uri.getCanonicalPath().substring(contextPath.length());
}
return new Dispatcher(ServletContextHandler.this, uri, pathInfo);
return new Dispatcher(ServletContextHandler.this, uri, URIUtil.decodePath(encodedPathInContext));
}
catch (Exception e)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ public static ServletContextRequest getServletContextRequest(ServletRequest requ
private final ServletContextResponse _response;
final ServletHandler.MappedServlet _mappedServlet;
private final HttpInput _httpInput;
private final String _pathInContext;
private final String _decodedPathInContext;
private final ServletChannel _servletChannel;
private final PathSpec _pathSpec;
private final SessionManager _sessionManager;
Expand All @@ -93,7 +93,7 @@ protected ServletContextRequest(
ServletChannel servletChannel,
Request request,
Response response,
String pathInContext,
String decodedPathInContext,
MatchedResource<ServletHandler.MappedServlet> matchedResource,
SessionManager sessionManager)
{
Expand All @@ -102,7 +102,7 @@ protected ServletContextRequest(
_httpServletRequest = newServletApiRequest();
_mappedServlet = matchedResource.getResource();
_httpInput = _servletChannel.getHttpInput();
_pathInContext = pathInContext;
_decodedPathInContext = decodedPathInContext;
_pathSpec = matchedResource.getPathSpec();
_matchedPath = matchedResource.getMatchedPath();
_response = newServletContextResponse(response);
Expand All @@ -119,9 +119,9 @@ protected ServletContextResponse newServletContextResponse(Response response)
return new ServletContextResponse(_servletChannel, this, response);
}

public String getPathInContext()
public String getDecodedPathInContext()
{
return _pathInContext;
return _decodedPathInContext;
}

public PathSpec getPathSpec()
Expand Down Expand Up @@ -209,8 +209,8 @@ public Object getAttribute(String name)
{
case "o.e.j.s.s.ServletScopedRequest.request" -> _httpServletRequest;
case "o.e.j.s.s.ServletScopedRequest.response" -> _response.getHttpServletResponse();
case "o.e.j.s.s.ServletScopedRequest.servlet" -> _mappedServlet.getServletPathMapping(getPathInContext()).getServletName();
case "o.e.j.s.s.ServletScopedRequest.url-pattern" -> _mappedServlet.getServletPathMapping(getPathInContext()).getPattern();
case "o.e.j.s.s.ServletScopedRequest.servlet" -> _mappedServlet.getServletPathMapping(getDecodedPathInContext()).getServletName();
case "o.e.j.s.s.ServletScopedRequest.url-pattern" -> _mappedServlet.getServletPathMapping(getDecodedPathInContext()).getPattern();
default -> super.getAttribute(name);
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -471,10 +471,10 @@ public boolean handle(Request request, Response response, Callback callback) thr
if (authenticator != null)
authenticator.prepareRequest(request);

RoleInfo roleInfo = prepareConstraintInfo(servletContextRequest.getPathInContext(), servletApiRequest);
RoleInfo roleInfo = prepareConstraintInfo(servletContextRequest.getDecodedPathInContext(), servletApiRequest);

// Check data constraints
if (!checkUserDataPermissions(servletContextRequest.getPathInContext(), servletContextRequest, response, callback, roleInfo))
if (!checkUserDataPermissions(servletContextRequest.getDecodedPathInContext(), servletContextRequest, response, callback, roleInfo))
return true;

// is Auth mandatory?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ public Authentication validateRequest(Request req, Response res, Callback callba
ServletContextRequest servletContextRequest = Request.as(req, ServletContextRequest.class);
ServletApiRequest servletApiRequest = servletContextRequest.getServletApiRequest();

String pathInContext = servletContextRequest.getPathInContext();
String pathInContext = servletContextRequest.getDecodedPathInContext();
boolean jSecurityCheck = isJSecurityCheck(pathInContext);
mandatory |= jSecurityCheck;
if (!mandatory)
Expand Down
Loading

0 comments on commit e73f747

Please sign in to comment.