incorrect validation regexes in serialize

[bewinsnw]

The code here: https://github.com/jshttp/cookie/blob/master/index.js#L119-L125

... tests cookie names and values against the field-content regex from RFC7230. Unfortunately, that's the wrong pattern; that's the regex for the whole header value, not the individual parts. The cookie name and value per https://datatracker.ietf.org/doc/html/rfc6265#section-4.1.1 use token from rfc7230 for the name, and a specific pattern in rfc6265 for the value. While the default encode function will keep values within the required pattern, it can be overridden, so the validation can fail on user-supplied encoders; the name validation is always wrong.

It's pretty easy to demonstrate:

cookie = require('cookie');

function identity(s) {
  return s;
}

tests = {
  goodname: 'goodvalue',
  'bad name': 'bad value',
  'bad=name': 'good=value',
  'bad;name': 'bad;value',
  'bad☹name': 'bad;value',
  name1: 'bad☹value',
  name2: 'bad"value',
  name3: '"goodvalue"',
};
// https://datatracker.ietf.org/doc/html/rfc6265#section-4.1.1 refers
// to https://datatracker.ietf.org/doc/html/rfc2616#section-2.2
// which is obsoleted and replaced by the token definition in
// https://www.rfc-editor.org/rfc/rfc7230#appendix-B
tokenRegex = /^[!#$%&'*+.^_`|~0-9A-Za-z-]+$/;
// https://datatracker.ietf.org/doc/html/rfc6265#section-4.1.1
cookieValueRegex =
  /^("?)[\u0021\u0023-\u002B\u002D-\u003A\u003C-\u005B\u005D-\u007E]*\1$/;

for (const [name, value] of Object.entries(tests)) {
  var validName = tokenRegex.test(name);
  var validValue = cookieValueRegex.test(value);
  var serialized;
  try {
    serialized = cookie.serialize(name, value, { encode: identity });
  } catch (error) {
    serialized = error;
  }
  console.log(
    `name: ${name} (${validName}) value: ${value} (${validValue}) serialized: ${serialized}`
  );
}

(demonstrating both good/bad values where the true regexes and cookie agree, and a handful of the many values where cookie would incorrectly say it's valid)

Discovered looking for whatever was leaking whitespace into our cookies; I'm not certain cookie did it, but seeing that the validation allowed whitespace jumped out at me.

[wesleytodd]

Hey, thanks for the report! I have not had a chance to dig deeply into this report, but with my limited time I wonder if you were willing to submit a PR for this? It feels like it would be easier to make sure we are all on the same page of what the report means for changes in the behavior of this relatively stable lib. If not I can loop back around sometime next week and read the relevant spec sections to make sure I follow the issue.

[bewinsnw]

Sure. Coincidentally we later had a production incident that very definitely did involve this bug so I'm happy to contribute

[wesleytodd]

Cool, well I will look for that PR and we can discuss there. When you open that please close this so we don't have the same conversation going on in two places.

[bewinsnw]

Closing per discussion