Skip to content

fix: do not split If-None-Match entity-tags on commas inside quotes - #80

Open
official-burak wants to merge 1 commit into
jshttp:masterfrom
official-burak:fix/quoted-etag-commas
Open

official-burak wants to merge 1 commit into
jshttp:masterfrom
official-burak:fix/quoted-etag-commas

Conversation

@official-burak

@official-burak official-burak commented Aug 30, 2026

Copy link
Copy Markdown

parseTokenList treated every comma as a list separator. RFC 9110 opaque-tag allows commas inside the quoted entity-tag, so a validator sending W/"foo,bar" never matched that same ETag and fresh() returned stale.

Repro (2.0.0)

fresh(
  { 'if-none-match': 'W/"foo,bar"' },
  { etag: 'W/"foo,bar"' }
)
// => false  (expected true)

The list was parsed as W/"foo and bar", neither of which equals W/"foo,bar".

Change

Track double quotes while scanning so commas inside an opaque-tag stay in that token. Commas outside quotes still separate the list. Existing spacing and weak/strong matching tests are unchanged.

Tests

  • exact match on W/"foo,bar"
  • still splits W/"foo,bar", "baz"
  • a fragment that the old splitter would have produced (W/"foo) does not match

RFC 9110 opaque-tags may contain commas. parseTokenList treated every
comma as a list separator, so W/"foo,bar" never matched itself.

@kilisamemarisaaa kilisamemarisaaa left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified at fbbde9c on Windows with Node 24.12.0: npm test passes all 27 tests and npm run lint is clean. I also exercised seven additional cases covering strong and weak validators, multiple comma-bearing tags, an empty/comma-only opaque tag, fragment non-matches, and commas outside quotes; all produced the expected freshness result. The quote-state scan matches RFC 9110 opaque-tag grammar, where DQUOTE is excluded from etagc, so commas inside a valid quoted tag remain data while list commas still split. I found no correctness issues in this change.

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.

2 participants