Skip to content

Accept optional whitespace around commas when parsing If-None-Match - #5454

Open
iprithv wants to merge 1 commit into
apache:mainfrom
iprithv:fix/if-none-match-rfc-compliance
Open

Accept optional whitespace around commas when parsing If-None-Match#5454
iprithv wants to merge 1 commit into
apache:mainfrom
iprithv:fix/if-none-match-rfc-compliance

Conversation

@iprithv

@iprithv iprithv commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

The If-None-Match parser hard-codes the list separator as a comma followed by exactly one space (", "). Per RFC 9110, Section 5.6.1 (the #rule list extension, formerly RFC 7230, Section 7), list elements are separated by a comma with optional surrounding whitespace (OWS = *( SP / HTAB )). Valid headers such as W/"a",W/"b" or W/"a" , W/"b" are therefore rejected with 400.

This relaxes the separator to allow zero or more spaces or tabs on either side of the comma. ETag matching semantics are unchanged; only list parsing is affected. See RFC 9110, Section 13.1.2 for If-None-Match.

@vigneshio vigneshio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, I agree that returning a 400 for those headers was unnecessary 💯. Thanks for working on this @iprithv..

@github-project-automation github-project-automation Bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Sep 7, 2026
@flyingImer flyingImer closed this Sep 8, 2026
@github-project-automation github-project-automation Bot moved this from Ready to merge to Done in Basic Kanban Board Sep 8, 2026
@flyingImer flyingImer reopened this Sep 8, 2026
@github-project-automation github-project-automation Bot moved this from Done to PRs In Progress in Basic Kanban Board Sep 8, 2026

@flyingImer flyingImer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, no blocking concerns with this change. I just closed and re-opened the pr to see if the CI will pass

@github-project-automation github-project-automation Bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Sep 8, 2026

@dimas-b dimas-b left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good catch, @iprithv !

Code changes LGTM 👍 with a minor test coverage concern.

String etagValue3 = "W/\"etag3\"";

IfNoneMatch ifNoneMatch =
IfNoneMatch.fromHeader(etagValue1 + " , " + etagValue2 + "\t,\t" + etagValue3);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Technically we're missing the cases of multiple / mixed / before-only / after-only spaces/tabs.

Should this be a parameterized test with some more extensive input permutations?

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.

5 participants