guice servlet: HttpServletRequest.getPathInfo not decoded

[gissuebot]

From matt@hillsdon.net on April 11, 2013 06:52:21

getPathInfo in the anonymous HttpServletRequestWrapper in ServletDefinition.doService does not perform URL decoding, but it is required to do so by the servlet spec.

The inputs, getRequestURI and getContextPath, are provided by the container and are (correctly) provided without decoding.

The HttpServletRequest docs for getPathInfo say "a String, decoded by the web container".  Tomcat decodes it, and provides container-level configuration to control the character encoding used.  See http://wiki.apache.org/tomcat/FAQ/CharacterEncoding "How do I change how GET parameters are interpreted?"

I switched an existing application to use Guice to create my servlets and tests for character encoding failed as the raw string was returned from getPathInfo.

Original issue: http://code.google.com/p/google-guice/issues/detail?id=745

[gissuebot]

From t.cservenak on February 13, 2014 12:53:27

Looks like a problem at this place, as it wraps original Request but calculates the pathInfo (should be decoded) using requestUrl (is not decoded): https://code.google.com/p/google-guice/source/browse/extensions/servlet/src/com/google/inject/servlet/ServletDefinition.java?r=389cb718a70cd11fdf9c336209246ebcfe944755#205

[gissuebot]

From t.cservenak on February 14, 2014 03:40:34

Proposed patch sonatype/sisu-guice#8

[gissuebot]

From ukjung.kim on February 19, 2014 20:01:47

I have same issue with it.
Gitiles gerrit plugin is not working well because of this issue.

Please refer following discussion. https://groups.google.com/forum/#!topic/repo-discuss/px6X8_HOMPQ

[oberlies]

BTW in the HttpServletRequest instances provided by Guice, getPathInfo() consists of an URI-encoded path and the query string. The spec requires that getPathInfo() only returns a de-coded path.

[sameb]

@oberlies , would you be able to supply a pull request with the fix+test? Thanks!

[mcculls]

@sameb FYI there's a working patch+tests here: https://github.com/sonatype/sisu-guice/blob/master/PATCHES/GUICE_745_getpathinfo_not_decoded.patch which we've used successfully in production (this fix was originally suggested by Tamas a few comments above in #745 (comment))

[oberlies]

Sorry, no time for this now, but I can share the code & test how to get from the current getPathInfo() value to the correct one. It is probably a lot easier to apply the same approach to the Guice code than it would be for me.

public class HttpServletRequestWithDecodedPathInfo extends HttpServletRequestWrapper {

    private final String correctedPathInfo;

    /**
     * Creates a valid {@link HttpServletRequest} from the instance that Guice passes to servlets
     * (see Guice issue 745 - https://github.com/google/guice/issues/745)
     *
     * @param originalRequest
     *            The {@link HttpServletRequest} passed to
     *            {@link HttpServlet#service(ServletRequest, javax.servlet.ServletResponse)} by
     *            Guice.
     * @return An {@link HttpServletRequest} which returns only returns the decoded path from
     *         {@link HttpServletRequest#getPathInfo()}, as required by the spec.
     */
    public static HttpServletRequest createRepairedRequest(final HttpServletRequest originalRequest) {
        String originalPathInfo = originalRequest.getPathInfo();
        if (originalPathInfo == null) {
            return originalRequest;
        }
        if (!originalPathInfo.startsWith("/")) {
            // never observed
            throw new IllegalArgumentException("Invalid pathInfo: " + originalPathInfo);
        }

        return requestWithDecodedPathInfo(originalRequest, originalPathInfo);
    }

    private static HttpServletRequest requestWithDecodedPathInfo(final HttpServletRequest originalRequest,
            final String originalPathInfo) {
        String decodedPathInfo = extractAndDecodePath(originalPathInfo);

        if (decodedPathInfo.equals(originalPathInfo)) {
            return originalRequest;
        } else {
            return new HttpServletRequestWithDecodedPathInfo(originalRequest, decodedPathInfo);
        }
    }

    private static String extractAndDecodePath(final String uriEncodedPathAndQuery) {
        try {
            return new URI(uriEncodedPathAndQuery).getPath();
        } catch (URISyntaxException e) {
            // TODO use an exception type which produces a "bad request" responses
            throw new IllegalArgumentException(e);
        }
    }

    private HttpServletRequestWithDecodedPathInfo(final HttpServletRequest originalRequest,
            final String correctedPathInfo) {
        super(originalRequest);
        this.correctedPathInfo = correctedPathInfo;
    }

    @Override
    public String getPathInfo() {
        return correctedPathInfo;
    }
}

Test:

import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.CoreMatchers.nullValue;
import static org.junit.Assert.assertThat;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import javax.servlet.http.HttpServletRequest;
import org.junit.Test;

public class HttpServletRequestWithDecodedPathInfoTest {

    private HttpServletRequest subject;

    @Test
    public void testEmptyPathInfo() throws Exception {
        subject = HttpServletRequestWithDecodedPathInfo.createRepairedRequest(requestWithPathInfo(null));
        assertThat(subject.getPathInfo(), is(nullValue()));
    }

    @Test
    public void testSimplePathInfo() throws Exception {
        subject = HttpServletRequestWithDecodedPathInfo.createRepairedRequest(requestWithPathInfo("/simple/path"));
        assertThat(subject.getPathInfo(), is("/simple/path"));
    }

    @Test
    public void testEncodedPathInfo() throws Exception {
        subject =
            HttpServletRequestWithDecodedPathInfo.createRepairedRequest(requestWithPathInfo("/uri+encoded%20path/with/percent-literal:%2520/"));
        assertThat(subject.getPathInfo(), is("/uri+encoded path/with/percent-literal:%20/"));
    }

    @Test(expected = IllegalArgumentException.class)
    public void testIncorrectlyEncodedPathInfo() throws Exception {
        subject =
            HttpServletRequestWithDecodedPathInfo.createRepairedRequest(requestWithPathInfo("/pipe|is-illegal-in-URLs"));
    }

    @Test
    public void testDecodingKeepsPathIntact() throws Exception {
        subject =
            HttpServletRequestWithDecodedPathInfo.createRepairedRequest(requestWithPathInfo("/http://may%20be%20part%20of%20the%20path"));
        assertThat(subject.getPathInfo(), is("/http://may be part of the path"));
    }

    @Test
    public void testPathInfoDoesntIncludeQuery() throws Exception {
        subject =
            HttpServletRequestWithDecodedPathInfo.createRepairedRequest(requestWithPathInfo("/path/with/%3Fquestionmark?querykey=val"));
        assertThat(subject.getPathInfo(), is("/path/with/?questionmark"));
    }

    private HttpServletRequest requestWithPathInfo(final String pathInfo) {
        HttpServletRequest request = mock(HttpServletRequest.class);
        when(request.getPathInfo()).thenReturn(pathInfo);
        return request;
    }
}

[sameb]

@cstamas -- could you sign the Google Contributor CLA @ https://developers.google.com/open-source/cla/individual (see the 'Sign Electronically' at the bottom) so I can incorporate your patch from #745 (comment) (also contributed to sisu @ sonatype/sisu-guice#8)? If you'd like to open a new pull request to Guice w/ the patch after signing the CLA, I can merge it directly from the PR, or I can manually patch your patch... but both need your CLA. Thanks!

[cstamas]

CLA signed, PR created:
#860

[sameb]

PR merged.