HTTP Cookies merge fix proposal

[mstoykov]

Current state

Cookies in k6 can be sourced from 3 places:

1. the cookie jar used. Which is either the default VU jar or provided with the params.jar

    {
        let defaultJar = http.cookieJar();
        defaultJar.set("https://httpbin.test.k6.io/cookies", "key", "default value");
        let resp = http.get("https://httpbin.test.k6.io/cookies");
        console.log(resp.body);
    }

    {
        let jar = new http.CookieJar();
        jar.set("https://httpbin.test.k6.io/cookies", "key", "not default value");
        let resp = http.get("https://httpbin.test.k6.io/cookies", {jar: jar}); // Binks
        console.log(resp.body);
    }

Will produce

INFO[0001] {
  "cookies": {
    "key": "default value"
  }
}  source=console
INFO[0001] {
  "cookies": {
    "key": "not default value"
  }
}  source=console

2. using params.cookies:

        let resp = http.get("https://httpbin.test.k6.io/cookies", {cookies: {val: "key"}});
        console.log(resp.body);

produces

INFO[0001] {
  "cookies": {
    "val": "key"
  }
}       source=console

3. Just setting the Cookie header manually:

        let resp = http.get("https://httpbin.test.k6.io/cookies", {headers: {Cookie: "key=headervalue"}});
        console.log(resp.body);

produces

INFO[0001] {
  "cookies": {
    "key": "headervalue"
  }
}  source=console

The current behaviour is as follows:

1. get the active jar (either from the VU or from params.jar).
2. add all cookies from params.cookies to the Cookie header through using Request.AddCookie, which effectively appends it to the header Cookie.
3. For each cookie for the domain in the jar use Request.AddCookie as well. This is skipped for cookies with key that was added by params.cookies that had replace:true.

So if you have something like:

    let jar = new http.CookieJar();
    jar.set("https://httpbin.test.k6.io/cookies", "key", "jar.value");
    let resp = http.get("https://httpbin.test.k6.io/cookies", {
        jar: jar, // Binks
        cookies: {"key": {value:"params.cookie.value", replace: false}}, // the replace is just illustrative, false is the default value
        headers: {Cookie: "key=header.value"}
    });
    console.log(resp.body);

you get:

INFO[0001] {
  "cookies": {
    "key": "jar.value"
  }
}  source=console

and the value that is actual header is:

Cookie: key=header.value; key=params.cookie.value; key=jar.value

This, effectively, is not well defined as per 4.2.2 of rfc6264:

Although cookies are serialized linearly in the Cookie header, servers SHOULD NOT rely upon the serialization order. In particular, if the Cookie header contains two cookies with the same name (e.g., that were set with different Path or Domain attributes), servers SHOULD NOT rely upon the order in which these cookies appear in the header.

And from 5.4 of the same

2. The user agent SHOULD sort the cookie-list in the following order:

   • Cookies with longer paths are listed before cookies with shorter paths.

   • Among cookies that have equal-length path fields, cookies with earlier creation-times are listed before cookies with later creation-times.

   NOTE: Not all user agents sort the cookie-list in this order, but this order reflects common practice when this document was written, and, historically, there have been servers that (erroneously) depended on this order.

I would argue that in general, the first value is the correct cookie value(as it has the longer path).

I have no idea if this is different for different server implementations, and what is more common.

Correction params.cookies is being set in the active jar mentioned in https://github.com/loadimpact/k6/issues/996#issuecomment-805056658

Some earlier experiment of mine seemed to prove that using params.cookies will set the cookies in the jar, but now looking at the code and doing an actual example shows that no the params.cookies don't get added to the "active jar", so at least that works correctly.

I probably ran some script that seemed like it does it because of reusage of key/value pairs or jars. Also this is how it is implemented in the #1650, although there it doesn't use VU jar so :shrug: , I guess I didn't have enough :coffee: at the time

Note on Set-Cookie:

Set-cookie basically sets on the active jar ... which seems completely right, so nothing to see here.}

Proposed change:

1. if there is a header Cookie this is what is actually send. Here it becomes tricky with redirects, do we continue to use exactly what was add as a cookie header or just decide that the Cookie header is only for the first request? Currently the latter is true and it seems correct to me.
2. Currently both params.jar and params.cookies are used for each redirect. Which seems correct to me.
3. Merging params.jar/VU's jar and params.cookies should produce a Cookie header value that has 1 key/value pair for each unique key with the jar having preference unless replace is true. Here we can debate if replace should not be true by default(edit: it is currently false by default) ... and I think that that makes sense but is one more breaking change so :shrug: .
4. Document all of this after we implement and test it thoroughly.
5. Implement the same thing for websockets

Additional redirect example with the current implementation:

import http from "k6/http";

export default function() {
    let resp = http.get("https://httpbin.test.k6.io/cookies/set/one/two", {
        headers: {Cookie: "key=header.value"},
        cookies: {"some":"other"},
    });
    console.log(resp.body);
}

produces (with --http-debug):

INFO[0000] Request:
GET /cookies/set/one/two HTTP/1.1
Host: httpbin.test.k6.io
User-Agent: k6/0.31.1 (https://k6.io/)
Cookie: key=header.value; some=other
Accept-Encoding: gzip

  group= iter=0 request_id=12059dcf-2188-4ca7-61cc-1a9d86c4b314 scenario=default source=http-debug vu=1
INFO[0001] Response:
HTTP/2.0 302 Found
Content-Length: 223
Access-Control-Allow-Credentials: true
Access-Control-Allow-Origin: *
Content-Type: text/html; charset=utf-8
Date: Wed, 24 Mar 2021 16:42:55 GMT
Location: /cookies
Server: gunicorn/19.9.0
Set-Cookie: one=two; Path=/

  group= iter=0 request_id=12059dcf-2188-4ca7-61cc-1a9d86c4b314 scenario=default source=http-debug vu=1
INFO[0001] Request:
GET /cookies HTTP/1.1
Host: httpbin.test.k6.io
User-Agent: k6/0.31.1 (https://k6.io/)
Cookie: some=other; one=two
Referer: https://httpbin.test.k6.io/cookies/set/one/two
Accept-Encoding: gzip

  group= iter=0 request_id=986e8f66-b28a-4279-69aa-ca8eab4e9042 scenario=default source=http-debug vu=1
INFO[0001] Response:
HTTP/2.0 200 OK
Content-Length: 62
Access-Control-Allow-Credentials: true
Access-Control-Allow-Origin: *
Content-Type: application/json
Date: Wed, 24 Mar 2021 16:42:55 GMT
Server: gunicorn/19.9.0

  group= iter=0 request_id=986e8f66-b28a-4279-69aa-ca8eab4e9042 scenario=default source=http-debug vu=1
INFO[0001] {
  "cookies": {
    "one": "two",
    "some": "other"
  }
}  source=console

Possible problems:

The predominant worry for me was all the har-to-k6 tests that are probably around with Cookie being set as a header that are currently getting overwritten by the VU jar. But this seems to have been resolved in https://github.com/k6io/har-to-k6/pull/23 by just dropping the whole header value (along Content-Length) so ... I am no longer worried about this.

Also, the fact that the behaviour is undefined in the RFC suggests we should probably not use it ;).

There are probably other places where the current behaviour makes a big difference and likely at least some users are depending on this in some way, so it is a question if the breakage is worth it.

p.s. This was provoked by me trying to work on #1650 and #1226 at which point looking around suggested this probably should be discussed before I start implementing it.

[imiric]

So this is a rabbit hole... :sweat_smile: Nice job summarizing everything!

Some comments about the proposal:

1. I think we should send the Cookie header set on the first request for all redirect requests as well. At least that's the behavior of curl:

   $ curl -Lv -H 'Cookie: key=value' 'https://httpbin.test.k6.io/redirect-to?url=https%3A%2F%2Fhttpbin.test.k6.io%2Fget&status_code=302'
   ...
   > GET /get HTTP/2
   > Host: httpbin.test.k6.io
   > user-agent: curl/7.75.0
   > accept: */*
   > cookie: key=value
   >
   < HTTP/2 200
   < date: Thu, 25 Mar 2021 09:11:07 GMT
   < content-type: application/json
   < content-length: 298
   < server: gunicorn/19.9.0
   < access-control-allow-origin: *
   < access-control-allow-credentials: true
   <
   {
     "args": {},
     "headers": {
       "Accept": "*/*",
       "Cookie": "key=value",
       "Host": "httpbin.test.k6.io",
       "User-Agent": "curl/7.75.0",
   ...
   

   But this gets tricky and poses a potential security issue when the redirect is to another domain. In that case curl doesn't send the cookie, see curl -Lv -H 'Cookie: key=value' 'https://httpbin.test.k6.io/redirect-to?url=https%3A%2F%2Fhttpbin.org%2Fget&status_code=302'.

   Related to this we should also respect the SameSite attribute, see this SO answer.

2. Good, makes sense, though we should make the behavior with a plain header consistent as well.

3. :thumbsup: replace being true by default (the current behavior) actually makes sense to me though.

[mstoykov]

1. Hmm ... I would think the golang lib will do this by default :thinking: And it does ... it is just that k6 deletes it :facepalm: Which was done through https://github.com/loadimpact/k6/pull/479 namely https://github.com/loadimpact/k6/commit/2e3289ac9ed06d9655dcc4a252bca36a777e47df ....
2. I think I've confused you ... the current default behaviour is replace:false, sorry about that. I probably should've phrased it differently