Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: nanoframework/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related issues
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
5ba7ef4 to
f9204da
Compare
|
@hamiddd1980 can you please validate if this fixes the issue you've reported? |
f9204da to
bd6c067
Compare
There was a problem hiding this comment.
Pull request overview
This PR changes how System.Net.Http.Headers.HttpHeaders stores headers so that callers can add headers that were previously blocked by WebHeaderCollection’s restricted-header validation logic.
Changes:
- Initialize
HttpHeaders._headerStorewithnew WebHeaderCollection(false)instead oftrue, disabling restricted-header checks for these collections.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public abstract class HttpHeaders | ||
| { | ||
| internal WebHeaderCollection _headerStore = new WebHeaderCollection(true); | ||
| internal WebHeaderCollection _headerStore = new WebHeaderCollection(false); |
There was a problem hiding this comment.
Switching the backing store to new WebHeaderCollection(false) disables the restricted-header checks (see WebHeaderCollection.ThrowOnRestrictedHeader()), which changes behavior for known headers like Accept, Connection, Host, Content-Length, etc., not just custom X-* headers. Given HeaderInfoTable treats unknown headers as non-restricted, custom headers were already allowed before this change; please either update the PR title/description to reflect the broader behavior change, or narrow the change to only what’s needed for the reported issue.
| public abstract class HttpHeaders | ||
| { | ||
| internal WebHeaderCollection _headerStore = new WebHeaderCollection(true); | ||
| internal WebHeaderCollection _headerStore = new WebHeaderCollection(false); |
There was a problem hiding this comment.
With restricted-header validation disabled, callers can now set headers that other parts of this library treat as managed/internal. For example, HttpContentHeaders.ContentLength reads/parses the Content-Length header from _headerStore and will throw if a user sets a non-numeric value via content.Headers.Add("Content-Length", ...). Consider keeping restrictions for headers that must remain controlled (e.g., Content-Length, Host, Transfer-Encoding) or adding explicit validation/guardrails in HttpHeaders.Add to prevent inconsistent state.
| internal WebHeaderCollection _headerStore = new WebHeaderCollection(false); | |
| internal WebHeaderCollection _headerStore = new WebHeaderCollection(true); |
| public abstract class HttpHeaders | ||
| { | ||
| internal WebHeaderCollection _headerStore = new WebHeaderCollection(true); | ||
| internal WebHeaderCollection _headerStore = new WebHeaderCollection(false); |
There was a problem hiding this comment.
This line changes header-validation behavior globally for HttpRequestHeaders, HttpResponseHeaders, and HttpContentHeaders, but there are no unit tests asserting the new expected behavior. Please add tests that (1) verify previously-blocked standard headers (e.g., Accept) can now be added, and (2) define the intended behavior for managed headers like Content-Length (either still rejected or validated).
|
Copilot comment is correct. The check can't simply be turned off! This requires another approach. |
Description
Allow custom HTTP headers
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist: