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

max-age and httponly cookie attributes not appearing in cookies responseCookies #2165

Closed
yevon opened this issue Nov 1, 2022 · 8 comments
Closed
Assignees

Comments

@yevon
Copy link

yevon commented Nov 1, 2022

Hi! Just recently started using this amazing library, but I think I've found an issue with it. I realized that I'm not receiving the max-age attribute and http-only cookie attributes in the "responseCookies", but I can see them clearly when using my app through chrome dev mode:

Using latest version 1.20:

Response printed by karate:
image

Chrome dev:
image

@ptrthomas ptrthomas added the bug label Nov 1, 2022
@ptrthomas
Copy link
Member

@yevon thanks, I think this is related to this comment: #1708 (comment)

so there's a bit of history - but I hope we can at least get some of these "missing" attributes working again.

it would be really great if you can find a public end-point that can simulate this. or any mock (ideally karate) server that helps you replicate this, perhaps you can hard code some header responses etc.

@yevon
Copy link
Author

yevon commented Nov 1, 2022

Yes, I think that it is the same. The responseCookies should be able to capture all cookie atrributes, even custom ones. I cannot provide a public IP but I think that this could be reproduced just making a GET request to google: https://www.google.es/. There are some cookies with those parameters set.

@yevon
Copy link
Author

yevon commented Nov 1, 2022

Find attached this example:

Feature: google cookies

  Background:
    * url 'https://www.google.com'

  Scenario: test google cookies

    Given path '/'
    When method get
    Then status 200

    * def first = response
    * print 'google cookies are: ', responseCookies

There are some parameters that doesn't match:

Karate:
image

Chrome:

Sin título

@elafontaine
Copy link

elafontaine commented Nov 1, 2022

Yes, that is linked to the cookie management as I identified in the other issue. This is a side effect of trying to manage the cookies and transferring between format (expiryDate vs max-age).

@ptrthomas I was doing some thinking on it, and I was wondering why we don't use the standard cookies system from the apache library (without trying to retain them ourselves in a separate cookieRegistry). If instead we exposed the cookies from the registry, then the management of them would be left to the librairies implementer while we took care of exposing the cookies in the registry. It would also simplify the logic I believe. Anyway, I'm unable to "work" on this at the moment... I would have to do it outside of work. I will look at it in my weekends.

I'm planning on looking if I could just remove the part below ( but keep the "headers = toHeaders..."). That way we don't clear the cookie store...

            // removing is probably not needed since apache cookie handling is enabled, but anyway
            httpResponse.removeHeaders(HttpConstants.HDR_SET_COOKIE);
            headers = toHeaders(httpResponse);
            headers.put(HttpConstants.HDR_SET_COOKIE, cookieValues);
            cookieStore.clear();

I'm still digging on the system and how this all interact everywhere.

@ptrthomas ptrthomas self-assigned this Nov 2, 2022
@ptrthomas ptrthomas added the fixed label Nov 2, 2022
@ptrthomas ptrthomas added this to the 1.3.0 milestone Nov 2, 2022
@ptrthomas
Copy link
Member

all: I think I managed a reasonable fix for this. so we use the proper "raw" headers as far as possible. in the edge case where we need to "fall back" to the apache cookie store, we merge them in and don't overwrite a fresh header. I think this solves all the reported issues and will last a while

ptrthomas added a commit that referenced this issue Nov 2, 2022
@yevon
Copy link
Author

yevon commented Nov 2, 2022

Nice! Thanks all

@ptrthomas
Copy link
Member

1.3.0 released

@yevon
Copy link
Author

yevon commented Nov 2, 2022

Thanks! Confirmed working nicely in 1.3.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants