Skip to content

fix: keep vary values that have whitespace before the comma - #426

Open
eknathdj wants to merge 1 commit into
fastify:mainfrom
eknathdj:fix/vary-whitespace-before-comma
Open

eknathdj wants to merge 1 commit into
fastify:mainfrom
eknathdj:fix/vary-whitespace-before-comma

Conversation

@eknathdj

Copy link
Copy Markdown

Fixes #422

parse() drops any Vary token that has optional whitespace before its comma:

const { parse } = require('@fastify/cors/vary')
parse('Origin , Accept-Encoding')
// ['accept-encoding']  — 'origin' is dropped

The tokenizer advanced pos past whitespace as it scanned, so when the comma arrived
pos === i and the value before it was never pushed. The same happened to a leading *:
parse('* , foo') returned ['foo'], so createAddFieldnameToVary never saw the wildcard —
which is how the same header also ended up as 'Origin , Accept-Encoding, Origin'.

RFC 9110 §5.6.1 allows OWS around the commas of a list, so that whitespace is not part of the
value.

Change

Drop the whitespace branch and trim each value as it is sliced, skipping the ones that come out
empty:

if (char === ',') {
  const token = header.slice(pos, i).trim()
  if (token !== '') {
    result.push(token)
  }
  pos = i + 1
}

Empty segments behave exactly as before — 'a, , b,c', 'a,,b,c', ',a,b,c' and 'a,b,c,' all
still parse to ['a', 'b', 'c'].

Tests

Added to test/vary.test.js:

  • 'a ,b', 'a , b', 'a , , b' and '* , a' parse to the expected values
  • a createAddFieldnameToVary case showing 'Origin , Accept-Encoding' is left untouched when
    Origin is added (before this it became 'Origin , Accept-Encoding, Origin')

npm run test:unit passes with 100% coverage.

parse() advanced its position past whitespace as it scanned, so a value
followed by whitespace and then a comma was never pushed: 'Origin ,
Accept-Encoding' parsed to ['accept-encoding'], and a leading '*' was
dropped the same way, which left createAddFieldnameToVary unable to see
the wildcard.

Trim each value as it is sliced and ignore the ones that come out empty,
which keeps the existing behaviour for empty segments.

Fixes fastify#422
@SameRainbows

Copy link
Copy Markdown

Thanks for opening this! Note that #423 was opened earlier for the same issue (#422) with a zero-allocation charCodeAt scan that also covers \t (HTAB) per RFC 9110 OWS. Maintainers may prefer consolidating behind #423.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: header tokens with whitespace preceding commas are dropped in vary parser

2 participants