Added handling of httpOnly and secure flags of cookies when redirecting

[unoexperto]

Certain calls work incorrectly due to overridden cookies.

For example GET request of
http://www.thelancet.com/journals/lancet/article/PIIS1474-4422(17)30433-7
consists of eight redirects and all of them are HTTPS calls.

Response from 3rd redirect to id.elsevier.com returns Set-Cookie for JSESSION with httpOnly flag. But JSESSION is already set. This response overrides it which breaks subsequent call to secure.jbs.elsevierhealth.com.

As result client is being redirected to front page of the website.

[slandelle]

Hi,

Thanks for trying to contribute. Sadly, your fix is wrong. The wrong JSESSIONID cookie is being sent because AHC doesn't currently have a cookie jar implementing RFC6265. httpOnly attribute has nothing to do with this (makes a cookie not reachable with Javascript).

CookieJar feature request was logged as #1235. I recently closed it due to lack of activity.
Feel free to give it a try!

[unoexperto]

thank you for quick response, @slandelle ! Could you please clarify what would be different in handling my example in case of implementation of CookieJar ? Do you expect httpOnly version of cookie to survive redirects to HTTPS urls in case it ever ends up on HTTP resource ? But CookieKey in your implementation doesn't have httpOnly flag. Even if CookieJar used here it will override correct JSESSION. What do you think ?

[slandelle]

@unoexperto Please take the time to read RFC6265, in particular the rules about domain and path matching. Also please properly read specification of Secure and HttpOnly attributes. "HttpOnly" blocks non-HTTP ways, typically Javascript.

[unoexperto]

@slandelle Perhaps I misinterpret the RFC. Consider following statement: The HttpOnly attribute limits the scope of the cookie to HTTP requests. In particular, the attribute instructs the user agent to omit the cookie when providing access to cookies via "non-HTTP" APIs (such as a web browser API that exposes cookies to scripts).

In case of redirection handler AHC is the agent. Thus it should not use httpOnly=true cookie when it opens non-HTTP url which is exactly what I do in my PR. How would your CookieJar handle this situation if your CookieKey cannot distinguish between cookie with httpOnly flag and without ?

I can work on implementing cookie jar for AHC but do you agree that httpOnly should be part of CookieKey ?

[slandelle]

Perhaps I misinterpret the RFC

Indeed

In case of redirection handler AHC is the agent. Thus it should not use httpOnly=true cookie when it opens non-HTTP url

You get it wrong. A redirect is an HTTP request. A "non-HTTP" API is for example Javascript's document.cookie.

[unoexperto]

A redirect is an HTTP request. A "non-HTTP" API is for example Javascript's document.cookie.

It doesn't matter that it's HTTP request. From server's prospective it's an agent whether it's your code or browser with JS interpreter. It wants certain cookie to be present which is curently lost.

I'm ready to spend my personal time and implement cookie jar for this project but tell me exactly step-by-step what behavior for downloading http://www.thelancet.com/journals/lancet/article/PIIS1474-4422(17)30433-7 you consider correct handling.

Here is dump of correct and incorrect communication
https://transfer.sh/ema6C/lancet_valid_and_invalid_calls.chls
Session file can be opened with www.charlesproxy.com

Feel free to answer in French if it's more convenient for you.

[slandelle]

Please, try reading the spec. HttpOnly is meant so to block cookie stealing with malicious Javascript.

AHC doesn't support fetching your page because you set 2 different cookies, both named JSESSIONID, on 2 different domains. As AHC doesn't currently have a CookieJar that implements domain matching, it gets it wrong.

If you want to contribute, there's everything you need in the Gatling implementation. You can also check the test.

[unoexperto]

@slandelle Actually now I see that you're right. I'll ping you when I'm done with integrating CookieJar.

[slandelle]

Cool!

As I said, the main difference is an implementation for AHC would need to be mutable and threadsafe, typically thanks to a ConcurrentHashMap, on contrary to the Gatling's one whose thread safety is guaranteed by Gatling's model.

[unoexperto]

@slandelle Got it. Could you create https://gitter.im room for any of your projects for easier communication in future ?

[slandelle]

I don't plan to have chat rooms for my projects: AHC is my pet project, and I don't have time to provide free support, and Gatling is the means of making a living for my company, so we provide support for our customers.

[unoexperto]

@slandelle Following is not final implementation but demonstration of approach I'd like to take. I looked at the AHC code and I think the least intrusive change would be to remove cookie manipulations from Redirect30xInterceptor and instead make sure RequestFilter-s are called before executing redirected requests.

Here is the change
https://github.com/unoexperto/async-http-client/commit/94e19b0e870fff88f17c8f03bc32f1fc899f13db

Then in my client code I I added one RequestFilter and ResponseFilter that catches and sets cookies like this (code is not thread-safe and just for purpose of demonstration)

object Test {
  var store = CookieJar.Empty
}

object SetRequestCookiesFilter extends RequestFilter {
  override def filter[T](ctx: FilterContext[T]): FilterContext[T] = {
    println(s"ReqFilter: ${ctx.getRequest.getUrl} [cookies: ${ctx.getRequest.getCookies}]")
    val items: List[Cookie] = Test.store.get(ctx.getRequest.getUri)
    if (items.nonEmpty) {
      val newRequest = new RequestBuilder(ctx.getRequest).setCookies(items.asJava).build()
      new FilterContextBuilder[T](ctx).request(newRequest).build()
    } else
      ctx
  }
}


object CatchResponseCookiesFilter extends ResponseFilter {
  override def filter[T](ctx: FilterContext[T]): FilterContext[T] = {
    println(s"RespFilter: ${ctx.getRequest.getUrl} [status: ${ctx.getResponseStatus}; headers: ${ctx.getResponseHeaders.entries().size()}]")

    val cookies = ctx.getResponseHeaders.getAll(HttpHeaderNames.SET_COOKIE).asScala.map(ClientCookieDecoder.STRICT.decode)
    if (cookies.nonEmpty)
      Test.store = Test.store.add(ctx.getRequest.getUri, cookies.toList)

    ctx
  }
}

of course I'd need to add expiration of cookies here too. Other things I may do:

1. Merge RequestFilter and ResponseFilter into single interface because they look alike;
2. For simplicity define CookieJarLike interface instead of implementing two above which would manage cookies

trait CookieJarLike {
  def storeCookies(requestUri: Uri, cookies: List[Cookie])
  def getCookies(requestUri: Uri): List[Cookie]
}

What do you think ?

[slandelle]

make sure RequestFilter-s are called before executing redirected requests.

There's a chance those Filters (Request, Response, IOExceptionFilter) will go away or will be redesigned in a future major release:

• I never use them personally (they were introduced way before I joined here)
• I never get contributions to help fixing bugs there (see Realm updated in ResponseFilter is not honored #1094 and IOExceptionFilters not Invoked on ConnectException #426)
• They introduce allocation overhead

Atm, I'd rather not have implementation rely on something that might go away.

Merge RequestFilter and ResponseFilter into single interface because they look alike;

Sorry, but that's not possible to break API now as I'm about to release 2.1.0 stable.

For simplicity define CookieJarLike interface instead of implementing two above which would manage cookies

Yep, just please code in Java here, not in Scala, so other people might be more inclined to join the discussion.

[unoexperto]

@slandelle I see. So would you prefer more then if I instead add CookieJarLike getCookieStorage() property to AsyncHttpClientConfig and call its methods in Redirect30xInterceptor.exitAfterHandlingRedirect, DefaultAsyncHttpClient.executeRequest and ResponseFiltersInterceptor.exitAfterProcessingFilters ? Basically it will be similar to CookieStore in Apache Http Client.

[slandelle]

Yep

[unoexperto]

@slandelle All right. Here we go :)

https://github.com/unoexperto/async-http-client/commit/746ff0d15cb092fa82486260d78f343e75709cda

I decided not to invent the wheel so I took java.net.InMemoryCookieStore and adopted it to use org.asynchttpclient.uri.Uri and io.netty.handler.codec.http.cookie.Cookie.

I also think I should implement Comparable<Uri> in your Uri.

Let me know what you think.

[slandelle]

I took java.net.InMemoryCookieStore and adopted it to use org.asynchttpclient.uri.Uri and io.netty.handler.codec.http.cookie.Cookie

I'm sorry, but I don't feel comfortable with this.
java.net.InMemoryCookieStore has an Oracle copyright and is under GPLv2 license, so that's a big no.
I think translating Gatling's implementation to Java and use a ConcurrentHashMap should be pretty straightforward.

[slandelle]

FYI, I'll be releasing AHC 2.1.0 stable once this lands.

[unoexperto]

@slandelle No worries. I can rewrite it tomorrow. How do you feel about rest of changes and CookieStore ?

[slandelle]

Regarding CookieStore, I'd rather keep the interface minimal first, ie only add and get, with the methods that we really needs internally, and then maybe extends when/if we get feedback.

Then, there's a problem with adding the cookies from the CookieStore into the request in executeRequest: it's performed after the SignatureCalculator with is performed when builder the request. But sadly that's something that can't be fixed atm. Will do when redesigning in next major release.

So, except for trimming CookieStore interface down, LGTM