Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JdkConnectorProvider cannot parse Set-cookie header value when expires attribute is present #4678

Closed
cykl opened this issue Jan 11, 2021 · 2 comments
Labels
bug Something isn't working connectors
Milestone

Comments

@cykl
Copy link

cykl commented Jan 11, 2021

JdkConnectorProvider is unable to parse Set-cookie header value with an expires attributes. A date header contains a comma after day-name but jdk-connector assumes this comma mean concatened header values. Original header value is then split in two values, the second being invalid (ie. value should start by <cookie-name>=<cookie-value>).

When a web server returns following header Set-cookie: foo=bar; expires=Wed, 10-Feb-2021 16:16:26 GMT; HttpOnly; Path=/; SameSite=Lax" ), jdk-connector parses it as Set-cookie: foo=bar; expires=Wed\nSet-cookie: 10-Feb-2021 16:16:26 GMT; HttpOnly; Path=/; SameSite=Lax.

Following test case reproduce the issue:

    @Test
    void fixme( ) throws IOException {
        var clientConfig = new ClientConfig( ).connectorProvider( new JdkConnectorProvider() );
        var client = ClientBuilder.newClient( clientConfig );

        try (var server = new MockWebServer()) {
            server.enqueue( new MockResponse( )
                    .setResponseCode( 200 )
                    .setHeader( "Set-cookie", "foo=bar; expires=Wed, 10-Feb-2021 16:16:26 GMT; HttpOnly; Path=/; SameSite=Lax" ) );

            var target = client.target( server.url( "" ).toString( ) );
            var response = target.request( ).get( );

            var cookie = response.getCookies().get( "foo" );
            assertThat( cookie.getValue() ).isEqualTo( "bar" );
        }
    }
Jan 11, 2021 5:53:56 PM java.net.CookieManager put
SEVERE: Invalid cookie for http://localhost:44111/: 10-Feb-2021 16:16:26 GMT; HttpOnly; Path=/; SameSite=Lax

java.lang.ArrayIndexOutOfBoundsException: Index 2 out of bounds for length 2

	at org.glassfish.jersey.message.internal.CookiesParser.parseNewCookie(CookiesParser.java:168)
	at org.glassfish.jersey.message.internal.HttpHeaderReader.readNewCookie(HttpHeaderReader.java:335)
	at org.glassfish.jersey.message.internal.InboundMessageContext.getResponseCookies(InboundMessageContext.java:592)
	at org.glassfish.jersey.client.InboundJaxrsResponse.getCookies(InboundJaxrsResponse.java:178)
	at com.greencomnetworks.xxx.HttpClientsTest.fixme(HttpClientsTest.java:91)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:686)
	at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
	[...]

Root cause is HttpParser#parseHeaderValue

    private int parseHeaderValue(ByteBuffer input, boolean parsingTrailerHeaders) throws ParseException {

        final int limit = Math.min(((Buffer) input).limit(), headerParsingState.packetLimit);

        int offset = headerParsingState.offset;

        final boolean hasShift = (offset != headerParsingState.checkpoint);

        while (offset < limit) {
            final byte b = input.get(offset);
            /* This if is not in Grizzly, it is used for parsing comma separated values.
             Grizzly separates the header in Header class. */
            if (b == HttpParserUtils.COMMA && !isInseparableHeader()) { <-- HERE
                headerParsingState.offset = offset + 1;
                String value = parseString(input,
                        headerParsingState.start, headerParsingState.checkpoint2);
                httpResponse.addHeader(headerParsingState.headerName, value);
                headerParsingState.start = headerParsingState.checkpoint2;
                return -2;
            }

WWW-Authenticate and Proxy-Authenticate are protected from this splitting but Set-cookie isn't. It's unclear to me if Set-cookie should be added to isInseparableHeader or if something else is wrong.

HttpUrlConnectorProvider parses the same header value without any problem.

@jansupol
Copy link
Contributor

RFC 7230 Says:

Note: In practice, the "Set-Cookie" header field ([RFC6265]) often
appears multiple times in a response message and does not use the
list syntax, violating the above requirements on multiple header
fields with the same name. Since it cannot be combined into a
single field-value, recipients ought to handle "Set-Cookie" as a
special case while processing header fields.

Hence, I agree that Set-Cookie should be taken into an account by isInseparableHeader.

@jansupol jansupol added this to the 2.34 milestone Jan 13, 2021
@jansupol
Copy link
Contributor

Fixed by #4681

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working connectors
Projects
None yet
Development

No branches or pull requests

2 participants