Skip to content

Commit

Permalink
The context path is no longer used when determining the path applicat…
Browse files Browse the repository at this point in the history 
…ion path

Servlet-Path + Path-Info is used instead

NOTE: some servlet containers will decode the context-path (Tomcat) and others will not (Jetty).  Per the Servlet spect the servlet-path and path-info are always decoded.
Shiro only makes decisions based on the application path, I.e. the request URI with the context-path removed, so we can just avoid dealing with it all together.

`WebUtils.getRequestURI()` has been deprecated to reflect this, and the previous behavior restored (as there was an issue with double decoding the URL in SHIRO-753)

Fixes: SHIRO-753
  • Loading branch information
@bdemers
bdemers committed Apr 7, 2020
1 parent 4cf242d commit b90f918
Show file tree
Hide file tree
Showing 6 changed files with 91 additions and 80 deletions.
3 changes: 1 addition & 2 deletions support/guice/src/test/java/org/apache/shiro/guice/web/FilterConfigTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,7 @@ public void testWithConfig() throws Exception {
private HttpServletRequest createMockRequest(String path) {
HttpServletRequest request = createNiceMock(HttpServletRequest.class);

expect(request.getAttribute(WebUtils.INCLUDE_CONTEXT_PATH_ATTRIBUTE)).andReturn(null).anyTimes();
expect(request.getContextPath()).andReturn("");
expect(request.getServletPath()).andReturn("");
expect(request.getPathInfo()).andReturn(path);
replay(request);
return request;
Expand Down
12 changes: 7 additions & 5 deletions support/guice/src/test/java/org/apache/shiro/guice/web/ShiroWebModuleTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,11 +177,13 @@ public void testAddFilterChainGuice3and4() {
servletContext.setAttribute(eq(EnvironmentLoader.ENVIRONMENT_ATTRIBUTE_KEY), EasyMock.anyObject());
expect(request.getAttribute("javax.servlet.include.context_path")).andReturn("").anyTimes();
expect(request.getCharacterEncoding()).andReturn("UTF-8").anyTimes();
expect(request.getAttribute("javax.servlet.include.request_uri")).andReturn("/test_authc");
expect(request.getAttribute("javax.servlet.include.request_uri")).andReturn("/test_custom_filter");
expect(request.getAttribute("javax.servlet.include.request_uri")).andReturn("/test_authc_basic");
expect(request.getAttribute("javax.servlet.include.request_uri")).andReturn("/test_perms");
expect(request.getAttribute("javax.servlet.include.request_uri")).andReturn("/multiple_configs");
expect(request.getAttribute("javax.servlet.include.path_info")).andReturn(null).anyTimes();
expect(request.getPathInfo()).andReturn(null).anyTimes();
expect(request.getAttribute("javax.servlet.include.servlet_path")).andReturn("/test_authc");
expect(request.getAttribute("javax.servlet.include.servlet_path")).andReturn("/test_custom_filter");
expect(request.getAttribute("javax.servlet.include.servlet_path")).andReturn("/test_authc_basic");
expect(request.getAttribute("javax.servlet.include.servlet_path")).andReturn("/test_perms");
expect(request.getAttribute("javax.servlet.include.servlet_path")).andReturn("/multiple_configs");
replay(servletContext, request);

Injector injector = Guice.createInjector(new ShiroWebModule(servletContext) {
Expand Down
8 changes: 4 additions & 4 deletions support/guice/src/test/java/org/apache/shiro/guice/web/SimpleFilterChainResolverTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ public void testGetChain() throws Exception {
ServletResponse response = ctrl.createMock(HttpServletResponse.class);
FilterChain originalChain = ctrl.createMock(FilterChain.class);

expect(request.getAttribute(WebUtils.INCLUDE_CONTEXT_PATH_ATTRIBUTE)).andReturn("/context");
expect(request.getAttribute(WebUtils.INCLUDE_REQUEST_URI_ATTRIBUTE)).andReturn("/mychain");
expect(request.getAttribute(WebUtils.INCLUDE_SERVLET_PATH_ATTRIBUTE)).andReturn("/mychain");
expect(request.getAttribute(WebUtils.INCLUDE_PATH_INFO_ATTRIBUTE)).andReturn("");

expect(request.getCharacterEncoding()).andStubReturn(null);

Expand Down Expand Up @@ -108,8 +108,8 @@ public void testGetChain() throws Exception {

ctrl.reset();

expect(request.getAttribute(WebUtils.INCLUDE_CONTEXT_PATH_ATTRIBUTE)).andReturn("/context");
expect(request.getAttribute(WebUtils.INCLUDE_REQUEST_URI_ATTRIBUTE)).andReturn("/nochain");
expect(request.getAttribute(WebUtils.INCLUDE_SERVLET_PATH_ATTRIBUTE)).andReturn("/nochain");
expect(request.getAttribute(WebUtils.INCLUDE_PATH_INFO_ATTRIBUTE)).andReturn("");

expect(request.getCharacterEncoding()).andStubReturn(null);

Expand Down
31 changes: 18 additions & 13 deletions web/src/main/java/org/apache/shiro/web/util/WebUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,16 +109,7 @@ public class WebUtils {
* @return the path within the web application
*/
public static String getPathWithinApplication(HttpServletRequest request) {
String contextPath = getContextPath(request);
String requestUri = getRequestUri(request);
if (StringUtils.startsWithIgnoreCase(requestUri, contextPath)) {
// Normal case: URI contains context path.
String path = requestUri.substring(contextPath.length());
return (StringUtils.hasText(path) ? path : "/");
} else {
// Special case: rather unusual.
return requestUri;
}
return normalize(removeSemicolon(getServletPath(request) + getPathInfo(request)));
}

/**
Expand All @@ -132,17 +123,27 @@ public static String getPathWithinApplication(HttpServletRequest request) {
*
* @param request current HTTP request
* @return the request URI
* @deprecated use getPathWithinApplication() to get the path minus the context path, or call HttpServletRequest.getRequestURI() directly from your code.
*/
@Deprecated
public static String getRequestUri(HttpServletRequest request) {
String uri = (String) request.getAttribute(INCLUDE_REQUEST_URI_ATTRIBUTE);
if (uri == null) {
uri = valueOrEmpty(request.getContextPath()) + "/" +
valueOrEmpty(request.getServletPath()) +
valueOrEmpty(request.getPathInfo());
uri = request.getRequestURI();
}
return normalize(decodeAndCleanUriString(request, uri));
}

private static String getServletPath(HttpServletRequest request) {
String servletPath = (String) request.getAttribute(INCLUDE_SERVLET_PATH_ATTRIBUTE);
return servletPath != null ? servletPath : valueOrEmpty(request.getServletPath());
}

private static String getPathInfo(HttpServletRequest request) {
String pathInfo = (String) request.getAttribute(INCLUDE_PATH_INFO_ATTRIBUTE);
return pathInfo != null ? pathInfo : valueOrEmpty(request.getPathInfo());
}

private static String valueOrEmpty(String input) {
if (input == null) {
return "";
Expand Down Expand Up @@ -240,6 +241,10 @@ private static String normalize(String path, boolean replaceBackSlash) {
*/
private static String decodeAndCleanUriString(HttpServletRequest request, String uri) {
uri = decodeRequestString(request, uri);
return removeSemicolon(uri);
}

private static String removeSemicolon(String uri) {
int semicolonIndex = uri.indexOf(';');
return (semicolonIndex != -1 ? uri.substring(0, semicolonIndex) : uri);
}
Expand Down
100 changes: 61 additions & 39 deletions web/src/test/groovy/org/apache/shiro/web/util/WebUtilsTest.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,63 +143,85 @@ public class WebUtilsTest {
@Test
void testGetRequestUriWithServlet() {

dotTestGetPathWithinApplicationFromRequest("/", "/servlet", "/foobar", "/servlet/foobar")
dotTestGetPathWithinApplicationFromRequest("", "/servlet", "/foobar", "/servlet/foobar")
dotTestGetPathWithinApplicationFromRequest("", "servlet", "/foobar", "/servlet/foobar")
dotTestGetPathWithinApplicationFromRequest("/", "servlet", "/foobar", "/servlet/foobar")
dotTestGetPathWithinApplicationFromRequest("//", "servlet", "/foobar", "/servlet/foobar")
dotTestGetPathWithinApplicationFromRequest("//", "//servlet", "//foobar", "/servlet/foobar")
dotTestGetPathWithinApplicationFromRequest("/context-path", "/servlet", "/foobar", "/servlet/foobar")
dotTestGetPathWithinApplicationFromRequest("//context-path", "//servlet", "//foobar", "/servlet/foobar")
dotTestGetPathWithinApplicationFromRequest("//context-path", "/servlet", "/../servlet/other", "/servlet/other")
dotTestGetPathWithinApplicationFromRequest("//context-path", "/asdf", "/../servlet/other", "/servlet/other")
dotTestGetPathWithinApplicationFromRequest("//context-path", "/asdf", ";/../servlet/other", "/asdf")
dotTestGetPathWithinApplicationFromRequest("/context%2525path", "/servlet", "/foobar", "/servlet/foobar")
dotTestGetPathWithinApplicationFromRequest("/c%6Fntext%20path", "/servlet", "/foobar", "/servlet/foobar")
dotTestGetPathWithinApplicationFromRequest("/context path", "/servlet", "/foobar", "/servlet/foobar")
dotTestGetPathWithinApplicationFromRequest("", null, null, "/")
dotTestGetPathWithinApplicationFromRequest("", "index.jsp", null, "/index.jsp")
dotTestGetPathWithinApplicationFromRequest("/servlet", "/foobar", "/servlet/foobar")
dotTestGetPathWithinApplicationFromRequest("/servlet", "/foobar", "/servlet/foobar")
dotTestGetPathWithinApplicationFromRequest("servlet", "/foobar", "/servlet/foobar")
dotTestGetPathWithinApplicationFromRequest("servlet", "/foobar", "/servlet/foobar")
dotTestGetPathWithinApplicationFromRequest("servlet", "/foobar", "/servlet/foobar")
dotTestGetPathWithinApplicationFromRequest("//servlet", "//foobar", "/servlet/foobar")
dotTestGetPathWithinApplicationFromRequest("/servlet", "/foobar", "/servlet/foobar")
dotTestGetPathWithinApplicationFromRequest("//servlet", "//foobar", "/servlet/foobar")
dotTestGetPathWithinApplicationFromRequest("/servlet", "/../servlet/other", "/servlet/other")
dotTestGetPathWithinApplicationFromRequest("/asdf", "/../servlet/other", "/servlet/other")
dotTestGetPathWithinApplicationFromRequest("/asdf/foo", ";/../servlet/other", "/asdf/foo")
dotTestGetPathWithinApplicationFromRequest("/servlet", "/foobar", "/servlet/foobar")
dotTestGetPathWithinApplicationFromRequest("/servlet", "/foobar", "/servlet/foobar")
dotTestGetPathWithinApplicationFromRequest("/servlet", "/foobar", "/servlet/foobar")
dotTestGetPathWithinApplicationFromRequest(null, null, "/")
dotTestGetPathWithinApplicationFromRequest("index.jsp", null, "/index.jsp")
}

@Test
void testGetPathWithinApplication() {

doTestGetPathWithinApplication("/", "/foobar", "/foobar");
doTestGetPathWithinApplication("", "/foobar", "/foobar");
doTestGetPathWithinApplication("", "foobar", "/foobar");
doTestGetPathWithinApplication("/", "foobar", "/foobar");
doTestGetPathWithinApplication("//", "foobar", "/foobar");
doTestGetPathWithinApplication("//", "//foobar", "/foobar");
doTestGetPathWithinApplication("/context-path", "/context-path/foobar", "/foobar");
doTestGetPathWithinApplication("/context-path", "/context-path/foobar/", "/foobar/");
doTestGetPathWithinApplication("//context-path", "//context-path/foobar", "/foobar");
doTestGetPathWithinApplication("//context-path", "//context-path//foobar", "/foobar");
doTestGetPathWithinApplication("//context-path", "//context-path/remove-one/remove-two/../../././/foobar", "/foobar");
doTestGetPathWithinApplication("//context-path", "//context-path//../../././/foobar", null);
doTestGetPathWithinApplication("/context%2525path", "/context%2525path/foobar", "/foobar");
doTestGetPathWithinApplication("/c%6Fntext%20path", "/c%6Fntext%20path/foobar", "/foobar");
doTestGetPathWithinApplication("/context path", "/context path/foobar", "/foobar");
doTestGetPathWithinApplication("/foobar", null, "/foobar");
doTestGetPathWithinApplication("/foobar", "", "/foobar");
doTestGetPathWithinApplication("", "/", "/");
doTestGetPathWithinApplication("", null, "/");
doTestGetPathWithinApplication("/foobar", "//", "/foobar/");
doTestGetPathWithinApplication("/foobar", "//extra", "/foobar/extra");
doTestGetPathWithinApplication("/foobar", "//extra///", "/foobar/extra/");
doTestGetPathWithinApplication("/foo bar", "/path info" ,"/foo bar/path info");
}

@Test
void testGetRequestURI() {
doTestGetRequestURI("/foobar", "/foobar")
doTestGetRequestURI( "/foobar/", "/foobar/")
doTestGetRequestURI("", "/");
doTestGetRequestURI("foobar", "/foobar");
doTestGetRequestURI("//foobar", "/foobar");
doTestGetRequestURI("//foobar///", "/foobar/");
doTestGetRequestURI("/context-path/foobar", "/context-path/foobar");
doTestGetRequestURI("/context-path/foobar/", "/context-path/foobar/");
doTestGetRequestURI("//context-path/foobar", "/context-path/foobar");
doTestGetRequestURI("//context-path//foobar", "/context-path/foobar");
doTestGetRequestURI("//context-path/remove-one/remove-two/../../././/foobar", "/context-path/foobar");
doTestGetRequestURI("//context-path//../../././/foobar", null);
doTestGetRequestURI("/context%2525path/foobar", "/context%25path/foobar");
doTestGetRequestURI("/c%6Fntext%20path/foobar", "/context path/foobar");
doTestGetRequestURI("/context path/foobar", "/context path/foobar");
}

void doTestGetPathWithinApplication(String contextPath, String requestUri, String expectedValue) {
void doTestGetPathWithinApplication(String servletPath, String pathInfo, String expectedValue) {

def request = createMock(HttpServletRequest)
expect(request.getAttribute(WebUtils.INCLUDE_CONTEXT_PATH_ATTRIBUTE)).andReturn(contextPath)
expect(request.getAttribute(WebUtils.INCLUDE_REQUEST_URI_ATTRIBUTE)).andReturn(requestUri)
expect(request.getCharacterEncoding()).andReturn("UTF-8").times(2)
expect(request.getAttribute(WebUtils.INCLUDE_SERVLET_PATH_ATTRIBUTE)).andReturn(servletPath)
expect(request.getAttribute(WebUtils.INCLUDE_PATH_INFO_ATTRIBUTE)).andReturn(pathInfo)
if (pathInfo == null) {
expect(request.getPathInfo()).andReturn(null) // path info can be null
}
replay request
assertEquals expectedValue, WebUtils.getPathWithinApplication(request)
verify request
}

void dotTestGetPathWithinApplicationFromRequest(String contextPath, String servletPath, String pathInfo, String expectedValue) {
void doTestGetRequestURI(String rawRequestUri, String expectedValue) {

def request = createMock(HttpServletRequest)
expect(request.getAttribute(WebUtils.INCLUDE_REQUEST_URI_ATTRIBUTE)).andReturn(rawRequestUri)
expect(request.getCharacterEncoding()).andReturn("UTF-8").times(1)
replay request
assertEquals expectedValue, WebUtils.getRequestUri(request)
verify request
}

void dotTestGetPathWithinApplicationFromRequest(String servletPath, String pathInfo, String expectedValue) {

HttpServletRequest request = createMock(HttpServletRequest)
expect(request.getAttribute(WebUtils.INCLUDE_CONTEXT_PATH_ATTRIBUTE)).andReturn(null)
expect(request.getAttribute(WebUtils.INCLUDE_REQUEST_URI_ATTRIBUTE)).andReturn(null)
expect(request.getAttribute(WebUtils.INCLUDE_SERVLET_PATH_ATTRIBUTE)).andReturn(null)
expect(request.getAttribute(WebUtils.INCLUDE_PATH_INFO_ATTRIBUTE)).andReturn(null)
expect(request.getServletPath()).andReturn(servletPath)
expect(request.getContextPath()).andReturn(contextPath).times(2)
expect(request.getPathInfo()).andReturn(pathInfo)
expect(request.getCharacterEncoding()).andReturn("UTF-8").anyTimes()
replay request
Expand Down
17 changes: 0 additions & 17 deletions web/src/test/java/org/apache/shiro/web/filter/mgt/PathMatchingFilterChainResolverTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,6 @@ public void testGetChainsWithMatch() {
//ensure at least one chain is defined:
resolver.getFilterChainManager().addToChain("/index.html", "authcBasic");

expect(request.getAttribute(WebUtils.INCLUDE_CONTEXT_PATH_ATTRIBUTE)).andReturn(null).anyTimes();
expect(request.getContextPath()).andReturn("");
expect(request.getServletPath()).andReturn("");
expect(request.getPathInfo()).andReturn("/index.html");
replay(request);
Expand All @@ -117,8 +115,6 @@ public void testPathTraversalWithDot() {
//ensure at least one chain is defined:
resolver.getFilterChainManager().addToChain("/index.html", "authcBasic");

expect(request.getAttribute(WebUtils.INCLUDE_CONTEXT_PATH_ATTRIBUTE)).andReturn(null).anyTimes();
expect(request.getContextPath()).andReturn("");
expect(request.getServletPath()).andReturn("/");
expect(request.getPathInfo()).andReturn("./index.html");
replay(request);
Expand All @@ -136,9 +132,6 @@ public void testPathTraversalWithDotDot() {

//ensure at least one chain is defined:
resolver.getFilterChainManager().addToChain("/index.html", "authcBasic");

expect(request.getAttribute(WebUtils.INCLUDE_CONTEXT_PATH_ATTRIBUTE)).andReturn(null).anyTimes();
expect(request.getContextPath()).andReturn("");
expect(request.getServletPath()).andReturn("/public/");
expect(request.getPathInfo()).andReturn("../index.html");
replay(request);
Expand All @@ -157,8 +150,6 @@ public void testGetChainsWithoutMatch() {
//ensure at least one chain is defined:
resolver.getFilterChainManager().addToChain("/index.html", "authcBasic");

expect(request.getAttribute(WebUtils.INCLUDE_CONTEXT_PATH_ATTRIBUTE)).andReturn(null).anyTimes();
expect(request.getContextPath()).andReturn("");
expect(request.getServletPath()).andReturn("/");
expect(request.getPathInfo()).andReturn(null);
replay(request);
Expand All @@ -180,8 +171,6 @@ public void testGetChain() {
//ensure at least one chain is defined:
resolver.getFilterChainManager().addToChain("/resource/book", "authcBasic");

expect(request.getAttribute(WebUtils.INCLUDE_CONTEXT_PATH_ATTRIBUTE)).andReturn(null).anyTimes();
expect(request.getContextPath()).andReturn("");
expect(request.getServletPath()).andReturn("");
expect(request.getPathInfo()).andReturn("/resource/book");
replay(request);
Expand All @@ -203,8 +192,6 @@ public void testGetChainEqualUrlSeparator() {
//ensure at least one chain is defined:
resolver.getFilterChainManager().addToChain("/", "authcBasic");

expect(request.getAttribute(WebUtils.INCLUDE_CONTEXT_PATH_ATTRIBUTE)).andReturn(null).anyTimes();
expect(request.getContextPath()).andReturn("");
expect(request.getServletPath()).andReturn("/");
expect(request.getPathInfo()).andReturn(null);
replay(request);
Expand All @@ -226,8 +213,6 @@ public void testGetChainEndWithUrlSeparator() {
//ensure at least one chain is defined:
resolver.getFilterChainManager().addToChain("/resource/book", "authcBasic");

expect(request.getAttribute(WebUtils.INCLUDE_CONTEXT_PATH_ATTRIBUTE)).andReturn(null).anyTimes();
expect(request.getContextPath()).andReturn("");
expect(request.getServletPath()).andReturn("");
expect(request.getPathInfo()).andReturn("/resource/book");
replay(request);
Expand All @@ -249,8 +234,6 @@ public void testGetChainEndWithMultiUrlSeparator() {
//ensure at least one chain is defined:
resolver.getFilterChainManager().addToChain("/resource/book", "authcBasic");

expect(request.getAttribute(WebUtils.INCLUDE_CONTEXT_PATH_ATTRIBUTE)).andReturn(null).anyTimes();
expect(request.getContextPath()).andReturn("");
expect(request.getServletPath()).andReturn("");
expect(request.getPathInfo()).andReturn("/resource/book//");
replay(request);
Expand Down

0 comments on commit b90f918

Please sign in to comment.