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

Wrong cookie merging #996

Closed
na-- opened this issue Apr 9, 2019 · 3 comments
Closed

Wrong cookie merging #996

na-- opened this issue Apr 9, 2019 · 3 comments
Labels
bug docs new-http issues that would require (or benefit from) a new HTTP API ux

Comments

@na--
Copy link
Member

na-- commented Apr 9, 2019

This script (copied from here):

import http from "k6/http";

export let options = {
  iterations: 2,
};

export default function () {
  // Set cookies
  if (__ITER === 0) {
    let resp1 = http.get("https://httpbingo.org/cookies/set?key=value1");
    console.log(resp1.body);
  }

  // Check cookies
  let resp2 = http.get("https://httpbingo.org/cookies");
  console.log(resp2.body);

  // Check custom cookies...
  let resp3 = http.get("https://httpbingo.org/cookies", {
    headers: { "Cookie": "key=value2;anotherkey=anothervalue" }
  });
  console.log(resp3.body);
}

will produce

{"key":"value1"}
{"key":"value1"}
{"anotherkey":"anothervalue","key":"value1"}
{}
{"anotherkey":"anothervalue","key":"value2"}

And I haven't tested how passing cookies or jar in the http.Params object plays into the whole mess... Since we already have way too many ways to specify cookies, we should at least fix them so merging the cookies works like how most users would expect it to work, and we should also explicitly document the precedence order and verify it with tests...

@na-- na-- added this to the v0.26.0 milestone Aug 27, 2019
@na-- na-- modified the milestones: v0.26.0, v0.27.0 Oct 10, 2019
@na-- na-- modified the milestones: v0.27.0, v0.28.0 May 21, 2020
@na-- na-- modified the milestones: v0.28.0, v0.30.0 Sep 9, 2020
@na-- na-- removed this from the v0.30.0 milestone Jan 13, 2021
@mstoykov
Copy link
Contributor

mstoykov commented Mar 23, 2021

For the record if you dump the request headers with --http-debug you can see that actually the value that is sent for the third request is

Cookie: key=value2;anotherkey=anothervalue; key=value1

So it is ... technically merged ;).

edit: I seem to have run some experiment that "proved" the below but later investigation suggest it is just not true and I probably got confused due to the example being more complex than necessary 🤦

Additional problems are

  let resp2 = http.get("https://httpbingo.org/cookies", {cookies: {key2:value}});

Would actually set key2 to value in the VU's jar

I actually came around this while looking at the code working on #1650 and #1226, and I think we need to actually agree on what the behaviour needs to be so I can do it right for the was codebase and then backport it to the HTTP one ...

@na-- na-- added the new-http issues that would require (or benefit from) a new HTTP API label Oct 18, 2021
@imiric
Copy link
Contributor

imiric commented Mar 14, 2023

I recently looked into this while trying to answer a support question with the httpx JS lib, and the behavior is indeed confusing and inconsistent when mixing the Cookie header with the cookies request option property.

In order to simplify things for the new HTTP API, I think we should abandon any smart merging when specifying the Cookie header, and simply send the header as specified, overriding any previous cookies, jars, etc. If the user wants to set the header directly, then we should assume that they want to explicitly override it.

Otherwise, if they want to do any merging, then they should use the cookies property instead, which allows specifying if a cookie should be replaced or not. This also gives us a structured object to work with, that would make merging easier, instead of having to parse stringy header values.

@mstoykov
Copy link
Contributor

It seems that while confusing for us - no users have come around to say something in the 4 years of this issue.

The new http design already specifies that this will just overwrite cookies as @imiric proposes above.

So I will close this

@mstoykov mstoykov closed this as not planned Won't fix, can't repro, duplicate, stale Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug docs new-http issues that would require (or benefit from) a new HTTP API ux
Projects
None yet
Development

No branches or pull requests

3 participants