OpenLiberty / open-liberty

Open Liberty is a highly composable, fast to start, dynamic application server runtime environment
https://openliberty.io
Eclipse Public License 2.0
1.15k stars 590 forks source link

Investigate the new Set-Cookie header that generate for an arbitrary attribute. #22324

Closed pmd1nh closed 4 months ago

pmd1nh commented 2 years ago

When use servlet response.addHeader or setHeader to Set-Cookie, any random attribute is created in a separate Set-Cookie header.

For example:

Cookie wcCookieAtt = new Cookie("CookieSetAttributeServlet", "TestAddSetHeader"); wcCookieAtt.setAttribute("SetAttributeName", "SetAttributeValue"); response.addCookie(wcCookieAtt);

response.setHeader("Set-Cookie", "Cookie_viaSetHeader=CookieValue_viaSetHeader; Secure; SameSite=None; randomAttributeA=myAttValueA");

response.addHeader("Set-Cookie", "Cookie_viaAddHeader=CookieValue_viaAddHeader; Secure; SameSite=None; randomAttributeB=myAttValueB");

Will generate 2 separate Set-Cookie headers:

Set-Cookie: randomAttributeA=myAttValueA Set-Cookie: randomAttributeB=myAttValueB

pmd1nh commented 1 year ago

closed as problem has addressed in one of the cookie works for serlvet 6.0

====
[10/17/22, 9:56:13:576 EDT] 0000003f BNFHeadersImp >  marshallHeaders Entry 
[10/17/22, 9:56:13:576 EDT] 0000003f BNFHeadersImp 1   Marshalling: Key: Content-Language Ordinal: 16 undefined: false [en-US]
[10/17/22, 9:56:13:576 EDT] 0000003f BNFHeadersImp 1   Marshalling: Key: Content-Length Ordinal: 11 undefined: false [90]
[10/17/22, 9:56:13:576 EDT] 0000003f BNFHeadersImp 1   Marshalling: Key: Set-Cookie Ordinal: 48 undefined: false [CookieSetAttributeServlet=TestCookieBasic; Path=BasicPath; Domain=basicdomain; Secure; HttpOnly; basicsetattribute1=BasicAttributeValue1; basicsetattribute2=BasicAttributeValue2]
[10/17/22, 9:56:13:576 EDT] 0000003f BNFHeadersImp 1   Marshalling: Key: Date Ordinal: 22 undefined: false [Mon, 17 Oct 2022 13:56:13 GMT]
[10/17/22, 9:56:13:576 EDT] 0000003f BNFHeadersImp 1   Marshalling: Key: Expires Ordinal: 25 undefined: false [Thu, 01 Dec 1994 16:00:00 GMT]
[10/17/22, 9:56:13:576 EDT] 0000003f BNFHeadersImp 1   Marshalling: Key: Cache-Control Ordinal: 17 undefined: false [no-cache="set-cookie, set-cookie2"]
[10/17/22, 9:56:13:576 EDT] 0000003f BNFHeadersImp <  marshallHeaders Exit 
=====
pmd1nh commented 8 months ago

The result from above

[10/17/22, 9:56:13:576 EDT] 0000003f BNFHeadersImp 1 Marshalling: Key: Set-Cookie Ordinal: 48 undefined: false [CookieSetAttributeServlet=TestCookieBasic; Path=BasicPath; Domain=basicdomain; Secure; HttpOnly; basicsetattribute1=BasicAttributeValue1; basicsetattribute2=BasicAttributeValue2]

is actually from a different test (testBasic) from the CookieSetAttributeServlet.

Problem still exists. Reopen and address in Servlet 6.1 if possible

pmd1nh commented 8 months ago

Target servlet

/*
     * Generates headers:
     * Set-Cookie: Cookie_viaSetHeader=CookieValue_viaSetHeader; Secure; SameSite=None
     * Set-Cookie: randomAttributeA=myAttValueA; SameSite=Lax
     * Set-Cookie: Cookie_viaAddHeader=CookieValue_viaAddHeader; Secure; SameSite=None
     * Set-Cookie: randomAttributeB=myAttValueB; SameSite=Lax
     */
    private void testAddSetHeader(HttpServletResponse response) {
        LOG.info(addDivider());
        LOG.info(" testAddSetHeader");

        Cookie wcCookieAtt = new Cookie("CookieSetAttributeServlet", "TestAddSetHeader");
        wcCookieAtt.setAttribute("SetAttributeName", "SetAttributeValue");
        response.addCookie(wcCookieAtt);

        response.setHeader("Set-Cookie", "Cookie_viaSetHeader=CookieValue_viaSetHeader; Secure; SameSite=None; randomAttributeA=myAttValueA");
        response.addHeader("Set-Cookie", "Cookie_viaAddHeader=CookieValue_viaAddHeader; Secure; SameSite=None; randomAttributeB=myAttValueB");

        LOG.info(" testAddSetHeader END.");
        LOG.info(addDivider());
    }
pmd1nh commented 8 months ago

When that is fixed, this one test needs to be modified or disabled for EE11+

https://github.com/OpenLiberty/open-liberty/blob/integration/dev/io.openliberty.webcontainer.servlet.6.0.internal_fat/fat/src/io/openliberty/webcontainer/servlet60/fat/tests/Servlet60CookieSetAttributeTest.java#L145

volosied commented 7 months ago

Split Cookie Behavior During Parsing

Adding my notes for future reference:

The current tWAS and Liberty implementations can split “Set-Cookie” Headers in certain scenarios. For example, the following java code looks to produce only two cookie headers: Cookie_AddHeader and AddCookie_Cookie. However, in practice, it will create three. Code: response.addHeader("Set-Cookie", "Cookie_AddHeader=CookieValue_AddHeader; Secure; customValue");

Cookie cookie = new Cookie("AddCookie_Cookie", "AddCookie_CookieValue_2");
response.addCookie(cookie);
Actual Headers:
Set-Cookie: Cookie_AddHeader=CookieValue_AddHeader; Secure
Set-Cookie: customValue=""
Set-Cookie: AddCookie_Cookie=AddCookie_CookieValue_2

The split cookie behavior only occurs when the following conditions are met:

This matters because if someone adds a new attribute (i.e “Partitioned”) via setHeader with Liberty/tWAS’s current implementation, it would be seen as a new cookie and not as an attribute.

Note: Servlet 6.1 added a Cookie#setAttribute field, but these cookies not split if a custom attribute is added. The attribute is part the class (not added via Condition 1) , and, therefore, no parsing occurs.

pmd1nh commented 4 months ago

This change also addresses the setAttribute for empty and null attribute value. For empty value, there will be no = sign after the attribute name.