Skip to content

Fix: Process ETag header value to be RFC 9110 compliant. #367

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
mathieucarbou merged 2 commits into from
Jan 16, 2026
Merged

Fix: Process ETag header value to be RFC 9110 compliant. #367

mathieucarbou merged 2 commits into from
Jan 16, 2026
+21 −17

Conversation

@StefanOberhumer
Copy link

@StefanOberhumer StefanOberhumer commented Jan 15, 2026

Enclose ETag header value in double quotes.

The following definitions require the ETag header value to be enclosed in double quotes:

@StefanOberhumer
Fix: Process ETag header value to be RFC 9110 compliant.
f24633e 
Enclose ETag header value in double quotes.

The following definitions require the ETag header value to be enclosed in double quotes:

* RFC 9110 - https://www.rfc-editor.org/rfc/rfc9110#section-8.8.3
  ( and previous RFC 7232 - https://www.rfc-editor.org/rfc/rfc7232#section-2.3 )

* MDN docs - https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/ETag#directives
@mathieucarbou mathieucarbou requested a review from a team January 15, 2026 16:22
Copilot AI reviewed Jan 15, 2026
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

willmmiles
willmmiles approved these changes Jan 16, 2026
View reviewed changes
Copy link

@willmmiles willmmiles left a comment

Choose a reason for hiding this comment

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

I haven't run any tests, but it looks good to a visual inspection.

Outside the scope of this PR: we might want to factor a short "format-32-bits-as-ETag" function, using either snprintf like src/Webhandlers.cpp:241, OR the hexChars approach in src/AsyncWebServerRequest.cpp:82 -- I don't love that there's two different implementations of this operation in the codebase. (Personally I would favor snprintf as I'm used to valuing code size over CPU time -- that hexChars constant must already be embedded somewhere internally there -- but it's pretty minor either way.)

@mathieucarbou
Copy link
Member

mathieucarbou commented Jan 16, 2026

hexChars

Yes I saw this duplication when fixing bb0dd46#diff-646b25b11691c11dce25529e3abce843f0ba4bd07ab75ec9eee7e72b06dbf13fR446

The diff is also in the behavior: snprintf adds a null terminator where the hexChars table allows more control and can be used in pretty much any cases.

@mathieucarbou mathieucarbou merged commit 4c86b44 into ESP32Async:main Jan 16, 2026
33 checks passed
@willmmiles
Copy link

willmmiles commented Jan 16, 2026

The diff is also in the behavior: snprintf adds a null terminator where the hexChars table allows more control and can be used in pretty much any cases.

You can ask snprintf to omit the null terminator by passing a length that doesn't leave room for it. It was always an interesting design decision in the snprintf API -- the result is that the overwhelming majority of call sites need to check if it did, or add one themselves just to be sure -- but it is occasionally useful.

@StefanOberhumer StefanOberhumer deleted the ETag_RFC9110 branch January 16, 2026 20:36
@mathieucarbou
Copy link
Member

mathieucarbou commented Jan 17, 2026

@StefanOberhumer : this issue being merged, is this fix can wait for the upcoming release (not planned yet), or this can be a blocker in some cases and would then require a release now ?

Asking because I don't know how browsers may behave when receiving non compliant etag values...

@StefanOberhumer
Copy link
Author

StefanOberhumer commented Jan 17, 2026 •
edited
Loading

@mathieucarbou :

@StefanOberhumer : this issue being merged, is this fix can wait for the upcoming release (not planned yet), or this can be a blocker in some cases and would then require a release now ?

Asking because I don't know how browsers may behave when receiving non compliant etag values...

I don't think it is a blocker.
In my experience, most web browsers and tools (like curl or wget) handle ETags without quotes quite gracefully.
While some might treat it as a literal string, they typically just send it back in the next request as they received it.
The worst-case scenario is a minor performance hit if a client requests a page that could have been served from their cache.
Clients should be built to handle or ignore invalid ETags anyway.

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

Reviewers

Copilot code review Copilot Copilot left review comments

@mathieucarbou mathieucarbou mathieucarbou approved these changes

@willmmiles willmmiles willmmiles approved these changes

@me-no-dev me-no-dev Awaiting requested review from me-no-dev

@vortigont vortigont Awaiting requested review from vortigont

Assignees

@StefanOberhumer StefanOberhumer

Labels

Status: Pending Merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@StefanOberhumer @mathieucarbou @willmmiles