Incorrect management of `set-cookie` in `Headers` class

[nabetti1720]

I was trying to proceed with the implementation of Headers#getSetCookie() and noticed that the set-cookie header is not managed correctly.

At this time, llrt can only manage values as String types, and there is no way to split multiple set-cookie. Running the reproduced code in bun shows the difference.

Execution result with set-cookie:

// reproduction.js
const headers = new Headers();

headers.append("Set-Cookie", "AAA=123; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.sample.com; HttpOnly");
headers.append("Set-Cookie", "BBB=456; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.sample.com; HttpOnly");
console.log(headers);

shinya@MBA2022M2 llrt-test % bun reproduction.js
Headers {
  "set-cookie": [ "AAA=123; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.sample.com; HttpOnly",
    "BBB=456; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.sample.com; HttpOnly"
  ],
}

Execution results except for set-cookie:

// reproduction.js
const headers = new Headers();

headers.append("Except-Set-Cookie", "AAA=123; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.sample.com; HttpOnly");
headers.append("Except-Set-Cookie", "BBB=456; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.sample.com; HttpOnly");
console.log(headers);

shinya@MBA2022M2 llrt-test % bun reproduction.js
Headers {
  "except-set-cookie": "AAA=123; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.sample.com; HttpOnly, BBB=456; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.sample.com; HttpOnly",
}

We believe that the solution is to manage values in the following way.

enum HeaderValue {
    Single(String),
    Multiple(Vec<String>),
}

pub struct Headers {
    headers: BTreeMap<String, HeaderValue>,
}

If this idea is correct, please assign it to me. :)

[richarddavison]

Multiple header values are just appended to the old value if present. Are you expecting a different behavior? F.Y.I headers should be refactored to use a Vec<(String,String)> instead of BTreeMap<String, String> to preserve insertion order and improve performance (a map is more effective when dealing with +100 items and that's not likely to happen). But I see no reason to use the enum of we simply need to append the value like the current implementation:

pub fn append(&mut self, key: String, value: String) {
    let key = key.to_lowercase();

    self.headers
        .entry(key)
        .and_modify(|header| *header = format!("{}, {}", header, &value))
        .or_insert_with(|| value);
}

[nabetti1720]

Thank you for your prompt confirmation. For normal headers (other than set-cookie), we believe the current implementation is appropriate. In other words, it is to connect them with comma delimiters.

As for set-cookie, basically, commas are not allowed in the value.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#cookie-namecookie-value

A cookie-value can optionally be wrapped in double quotes and include any US-ASCII character excluding control characters (ASCII characters 0 up to 31 and ASCII character 127), Whitespace, double quotes, commas, semicolons, and backslashes.

However, the Expires=<date> option is inconsistent because it is formatted to include a comma.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#expiresdate

Indicates the maximum lifetime of the cookie as an HTTP-date timestamp. See Date for the required formatting.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Date#syntax

Date: <day-name>, <day> <month> <year> <hour>:<minute>:<second> GMT

These are the reasons why we thought that commas could not be used in managing multiple set-cookie. Also, such a value cannot be retrieved successfully with the get method, so the getSetCookie method may exist.

Additionally, there is a way to manage it with Vec<(String,String)> but I don't think it's a good idea since it would not be lexicographical sorting and would require key searching in everything.

Another method would be to internally manage the data with " | " ("|" sandwiched between white spaces), etc., and return the data with the appropriate type(Vector<String>, String separated comma, etc.) when the method is called.

[nabetti1720]

We were testing a few more other implementations.

// reproduction.js
const headers = new Headers();

headers.append("Set-Cookie", "AAA=123; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.google.com; HttpOnly");
headers.append("Set-Cookie", "BBB=456; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.google.com; HttpOnly");
headers.append("accept-encoding", "zstd");
headers.append("accept-encoding", "br");

console.log(headers.getSetCookie());
console.log("---");
console.log(headers.get("Set-Cookie"));
console.log("---");
console.log(headers);

node: getSetCookie() -> Array format, get() -> String format, String format in Headers

shinya@MBA2022M2 llrt-test % node reproduction.js 
[
  'AAA=123; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.google.com; HttpOnly',
  'BBB=456; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.google.com; HttpOnly'
]
---
AAA=123; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.google.com; HttpOnly, BBB=456; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.google.com; HttpOnly
---
Headers {
  'Set-Cookie': 'AAA=123; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.google.com; HttpOnly, BBB=456; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.google.com; HttpOnly',
  'accept-encoding': 'zstd, br'
}

bun: getSetCookie()->Array format, get() -> String format, Array format in Headers

shinya@MBA2022M2 llrt-test % bun reproduction.js
[ "AAA=123; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.google.com; HttpOnly",
  "BBB=456; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.google.com; HttpOnly"
]
---
AAA=123; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.google.com; HttpOnly, BBB=456; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.google.com; HttpOnly
---
Headers {
  "accept-encoding": "zstd, br",
  "set-cookie": [ "AAA=123; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.google.com; HttpOnly",
    "BBB=456; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.google.com; HttpOnly"
  ],
}

deno: getSetCookie()->Array format, get() -> String format, String format(And the last value) in Headers

shinya@MBA2022M2 llrt-test % deno run reproduction.js
[
  "AAA=123; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.google.com; HttpOnly",
  "BBB=456; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.google.com; HttpOnly"
]
---
AAA=123; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.google.com; HttpOnly, BBB=456; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.google.com; HttpOnly
---
Headers {
  "accept-encoding": "zstd, br",
  "set-cookie": "BBB=456; expires=Sun, 10-Nov-2024 12:29:35 GMT; path=/; domain=.google.com; HttpOnly"
}

[richarddavison]

I see, thanks for reporting. I think the enum approach would be best option forward. @nabetti1720 care to take a shot a this? :)

[nabetti1720]

It seems to be affecting more places than I thought, but I'll do my best to work on it. :)