fix: validate header name and value in webRequest.onBeforeSendHeaders#51340
Conversation
|
For more information on signing commits, see GitHub's documentation on Telling Git about your signing key. |
789819d to
284a77b
Compare
Chromium's net::HttpRequestHeaders::SetHeader() uses CHECK() to enforce valid header names and values, which causes a fatal crash if the caller passes invalid strings. When users modify requestHeaders in the onBeforeSendHeaders callback with invalid header names (e.g. containing spaces) or invalid header values (e.g. containing CRLF), the gin::Converter<net::HttpRequestHeaders>::FromV8() calls SetHeader() directly, triggering the CHECK and crashing the process. This change adds pre-validation using net::HttpUtil::IsValidHeaderName() and net::HttpUtil::IsValidHeaderValue() before calling SetHeader(), silently skipping invalid headers instead of crashing.
284a77b to
d0fb3d5
Compare
|
@loufultoncz-coder please add release note in the PR description. |
Done! I've added the release note to the PR description. |
|
I checked this path locally and the change looks correct to me. @loufultoncz-coder could you also complete the checklist? Thanks. |
ckerr
left a comment
There was a problem hiding this comment.
This PR is a good idea & should be backported to the maintenance branches.
I had a style nit for the C++ code and made a suggestion to fix a potential timing issue in the new test, but overall this is good. Thanks for the PR!
Co-authored-by: Charles Kerr <charles@charleskerr.com>
Co-authored-by: Charles Kerr <charles@charleskerr.com>
Agreed. I'll make the requested changes. Thank you for the review! |
Thanks for verifying it locally! I have just completed the PR checklist. Let me know if there's anything else needed. |
codebytere
left a comment
There was a problem hiding this comment.
Thanks! @loufultoncz-coder please note that @xakep8 is in no way officially associated with the project so you can consider his feedback if you so choose but it is not relevant to whether the PR is ultimately approved.
55eb4a2 to
1d642c2
Compare
Thanks for the clarification and the approval! Really appreciate your time reviewing this. |
|
Release Notes Persisted
|
|
Congrats on merging your first pull request! 🎉🎉🎉 |
|
I have automatically backported this PR to "40-x-y", please check out #51364 |
|
I have automatically backported this PR to "41-x-y", please check out #51365 |
|
I have automatically backported this PR to "42-x-y", please check out #51366 |
Description of Change
Chromium's net::HttpRequestHeaders::SetHeader() uses CHECK() to enforce valid header names and values, which causes a fatal crash if the caller passes invalid strings. When users modify requestHeaders in the onBeforeSendHeaders callback with invalid header names (e.g. containing spaces) or invalid header values (e.g. containing CRLF), the gin::Converternet::HttpRequestHeaders::FromV8() calls SetHeader() directly, triggering the CHECK and crashing the process.
This change adds pre-validation using net::HttpUtil::IsValidHeaderName() and net::HttpUtil::IsValidHeaderValue() before calling SetHeader(), silently skipping invalid headers instead of crashing.
Checklist
npm testpassesRelease Notes
Notes: Fixed a crash when providing invalid HTTP header names or values in the
webRequest.onBeforeSendHeaders()callback