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

feat(cookies): filter expired cookies when injected in httpClient #1165

Closed
davinkevin opened this issue Jun 9, 2020 · 11 comments
Closed

feat(cookies): filter expired cookies when injected in httpClient #1165

davinkevin opened this issue Jun 9, 2020 · 11 comments

Comments

@davinkevin
Copy link

I've read a part of the karate code and, if I'm right, Karate doesn't support expiration time for cookies.

The problem is when Karate is sending a cookie, it doesn't verify if it is expired or not and sends all cookies previously set (especially those set by a previous request with an EXPIRES parameter).

In the following logs, the log time is in GMT+2 (🇫🇷).

16:43:03.220 [ForkJoinPool-1-worker-3] DEBUG com.intuit.karate - response time in milliseconds: 66,84
1 < 200
1 < Access-Control-Allow-Credentials: true
1 < Access-Control-Allow-Headers: Accept, Accept-Version, Content-Length, Content-MD5, Content-Type, Date
1 < Access-Control-Allow-Methods: GET,POST,PUT,DELETE,PATCH,OPTIONS
1 < Access-Control-Allow-Origin: karate
1 < Alt-Svc: clear
1 < Content-Type: application/json
1 < Date: Tue, 09 Jun 2020 14:43:03 GMT
1 < Server: IIS
1 < Set-Cookie: value-of-cookie-removed-for-privacy; path=/; Expires=Tue, 09 Jun 2020 14:43:04 GMT; Secure; HttpOnly; SameSite=None;
1 < Strict-Transport-Security: max-age=31536000; includeSubdomains;
1 < Transfer-Encoding: chunked
1 < Vary: Origin
1 < Via: 1.1 google
1 < X-Content-Type-Options: nosniff
1 < X-XSS-Protection: 1; mode=block
{ "message": "cookie set, thank you", "lifetime": 1 }


16:43:09.243 [ForkJoinPool-1-worker-3] DEBUG com.intuit.karate - request:
2 > POST https://our-httpbin/post
2 > Accept-Encoding: gzip,deflate
2 > Connection: Keep-Alive
2 > Content-Length: 0
2 > Content-Type: text/plain
2 > Cookie: value-of-cookie-removed-for-privacy
2 > Host: our-httpbin
2 > User-Agent: Apache-HttpClient/4.5.11 (Java/13.0.1)
2 > origin: karate

16:43:09.314 [ForkJoinPool-1-worker-3] DEBUG com.intuit.karate - response time in milliseconds: 64,65
2 < 200
2 < Access-Control-Allow-Credentials: true
2 < Access-Control-Allow-Headers: Accept, Accept-Version, Content-Length, Content-MD5, Content-Type, Date
2 < Access-Control-Allow-Methods: GET,POST,PUT,DELETE,PATCH,OPTIONS
2 < Access-Control-Allow-Origin: karate
2 < Alt-Svc: clear
2 < Content-Length: 2034
2 < Content-Type: application/json
2 < Date: Tue, 09 Jun 2020 14:43:09 GMT
2 < Server: IIS
2 < Strict-Transport-Security: max-age=31536000; includeSubdomains;
2 < Vary: Origin
2 < Via: 1.1 google
2 < X-Content-Type-Options: nosniff
2 < X-XSS-Protection: 1; mode=block
{
  "args": {}, 
  "data": "", 
  "files": {}, 
  "form": {}, 
  "headers": {
    "Accept-Encoding": "gzip,deflate", 
    "Connection": "close", 
    "Content-Length": "0", 
    "Content-Type": "text/plain", 
    "Cookie": "value-of-cookie-removed-for-privacy", 
    "Host": "our-httpbin", 
    "Origin": "karate", 
    "User-Agent": "Apache-HttpClient/4.5.11 (Java/13.0.1)", 
  }, 
  "json": null, 
  "origin": "...", 
  "url": "..."
}

You can see a "wait" in the middle of this execution, and the only cookie set is already expired but still send in the last request.

I've read all I can do about cookies in karate documentation and code (especially here and here and I don't see any references to Expires filer inside this usage of cookies.

BTW, the key com.intuit.karate.http.Cookie.EXPIRES when cookies are fetched from a response and nowhere else.

I didn't provide a code example because I think this is a feature request and not a bug. If you need I will be able to provide one, but it's time-consuming to have a demo backend to demonstrate the use case.

@ptrthomas
Copy link
Member

@davinkevin yes we probably don't handle cookie expiration correctly, but no one else has raised this either. see if you can use this as a reference, someone had created a great example for fixing a similar issue in the past: #552

would be great if you can submit a PR, but if you submit an example, that would increase the likelihood of this getting fixed sooner !

@davinkevin
Copy link
Author

Thank for the answer, I will try to see if I can provide a PR for example project and/or a PR for this.

@ptrthomas
Copy link
Member

@chaudharydeepak
Copy link
Contributor

Hi @ptrthomas - if its ok can I pls pick this? I tried to request this from project board proposed column - couldnt find anything to request? Let me know, Thank you

@ptrthomas
Copy link
Member

@chaudharydeepak yes go ahead

@ptrthomas ptrthomas reopened this Oct 6, 2020
@chaudharydeepak
Copy link
Contributor

chaudharydeepak commented Oct 6, 2020

Thank you @ptrthomas - I quickly glanced through code base
https://github.com/intuit/karate/blob/2b45168df0d8ade7d609b83acf46a942a560fe1f/karate-apache/src/main/java/com/intuit/karate/http/apache/ApacheHttpClient.java#L262

Actually we simply need to set the expiration date - and internal implementation will take care of rest.

Secondly, we can make this configurable - just to ensure if there any possible use case where users might want to push through an expired cookie - can't think of any feasible usecase - but just to keep it flexible.

Please let me know your inputs and will go make changes accordingly. Thank you.

@ptrthomas
Copy link
Member

lets not do configuration for now, user can always manually send a cookie
I'm not sure if setting the expiration date is enough - but the best way is to write a test and add behavior to the karate-demo server if needed, we will need tests. also I'm okay if we focus on apache only, but would be ideal if we look at jersey also. it may be a good idea to see what the jersey behavior is - in case it is doing the right thing

@chaudharydeepak
Copy link
Contributor

@ptrthomas - I verified that as long as expiration date is set on the cookie - apache CookieStore takes care of removing any expired cookies before making a request - so I think we should be good there - i wrote some simple tests for ApacheHttpClient to verify. Will validate for Jersey Client and testing with demo server too.

@chaudharydeepak
Copy link
Contributor

@ptrthomas - just to keep you posted -
Below tests run fine with Apachehttpclient but fail with Jersey

`Scenario: expired cookie is not in response
Given path 'search', 'cookies'
And cookie foo = {value:'bar', expires:'Tue, 09 Jun 2020 14:43:04 GMT'}
And param domain = '.abcdfdf.com'
#And param expires = 'Tue, 09 Jun 2020 14:43:04 GMT'
When method get
Then status 200
And match response == []

Scenario: non-expired cookie is in response
Given path 'search', 'cookies'
And cookie foo = {value:'bar', expires:'Wed, 07 Oct 2020 14:43:04 GMT', path:'/search'}
And param domain = '.abc.com'
#And param expires = 'Wed, 07 Oct 2020 14:43:04 GMT'
When method get
Then status 200
And match response[0] contains { name: 'foo', value: 'bar', domain: '.abc.com' }`

Additionally I had trouble setting cookie expiration as #And param expires = 'Wed, 07 Oct 2020 14:43:04 GMT', so I added it to cookie json request - I will do some more testing - let me know if you have any feedback. Thank you.

@ptrthomas
Copy link
Member

ptrthomas commented Oct 6, 2020

@chaudharydeepak thanks. you can try for jersey - but worst case you can use a tag @apache so that the scenario is not run on the jersey side - most teams use apache, so it is ok (but not ideal) to have some gaps like this

@ptrthomas ptrthomas added this to the 1.0.0 milestone Oct 10, 2020
ptrthomas added a commit that referenced this issue Oct 10, 2020
#1165 - filter expired cookies when injected in httpClient
@chaudharydeepak chaudharydeepak mentioned this issue Oct 28, 2020
1 task
@ptrthomas
Copy link
Member

I'm closing this issue because the code is completely rewritten. @chaudharydeepak did a great job implementing expiry checks but after some back and forth, I've decided that as a testing framework, users may want to send invalid cookies to the server to see how it behaves. the next version has no cookie checks at all, users can send whatever they want

@ptrthomas ptrthomas removed this from the 1.0.0 milestone Feb 9, 2021
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