Fix HttpHeaders.Remove to handle well-known header names

[Copilot]

Instead of throwing for header names that don't belong in this collection, return false.
This lets the caller fall back to the other header collection without knowing upfront which collection it should have picked.
To workaround this, YARP currently has a hardcoded list of our internal implementation detail of which headers are considered "content": https://github.com/dotnet/yarp/blob/3a6fd9f54b2ebc5de82a3e55bda2c6ada7bab4f6/src/ReverseProxy/Forwarder/RequestUtilities.cs#L96-L111

Closes #61968

[Copilot]

Pull request overview

This PR adjusts System.Net.Http.Headers.HttpHeaders behavior so that Contains(string) and Remove(string) return false (instead of throwing) when given invalid header names or well-known header names that are disallowed for a particular header collection (e.g., content headers on request/response headers).

Changes:

• Update HttpHeaders.Contains(string) and HttpHeaders.Remove(string) to rely on TryGetHeaderDescriptor and return false on invalid/disallowed names.
• Update existing unit tests that previously expected exceptions for invalid names to now expect false.
• Add new unit tests ensuring Contains/Remove return false for disallowed well-known headers across request/response/content header collections.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs	Changes Contains(string)/Remove(string) to use TryGetHeaderDescriptor, avoiding exceptions for invalid/disallowed names.
src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs	Updates exception-based tests to validate false return values for null/empty/invalid names; renames a mismatched test.
src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpRequestHeadersTest.cs	Adds coverage for disallowed content headers on request headers returning false from Contains/Remove.
src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpResponseHeadersTest.cs	Adds coverage for disallowed content headers on response headers returning false from Contains/Remove.
src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpContentHeadersTest.cs	Adds coverage for disallowed request headers on content headers returning false from Contains/Remove.

[github-actions]

🤖 Copilot Code Review — PR #127165

Note

This review was generated by Copilot and reflects analysis from multiple model families (Claude, GPT, System.Net domain reviewer).

Holistic Assessment

Motivation: The change aligns Contains(string) and Remove(string) with the non-throwing pattern already established by TryAddWithoutValidation and TryGetValues. The direction is sensible — if a header name can't map to a valid descriptor in this collection's domain, it is definitionally "not contained" and "not removable," so returning false is semantically correct for the FormatException/InvalidOperationException cases.

Approach: The implementation is clean — switching from GetHeaderDescriptor (throwing) to TryGetHeaderDescriptor (returning false) is minimal and consistent with how the two Try* methods already work. However, the change sweeps together four distinct exception cases (null, empty, invalid format, disallowed type) under a single false return, when they have different semantic gravity.

Summary: ⚠️ Needs Changes. The core direction is sound for invalid/disallowed header names, but null argument handling needs to be preserved (.NET convention), a breaking change issue must be filed, and the missing trailing newline in HttpResponseHeadersTest.cs should be fixed. Multiple reviewers (3/3) flagged the null handling and breaking change concerns independently.

---

Detailed Findings

❌ Null argument must still throw ArgumentNullException — HttpHeaders.cs:312-314, 483-485

Remove(null) and Contains(null) previously threw ArgumentNullException. After this PR they silently return false. This violates a bedrock .NET convention — every BCL collection type throws ArgumentNullException for null keys on ContainsKey/Remove:

• Dictionary<TKey,TValue>.ContainsKey(null) → ArgumentNullException
• Dictionary<TKey,TValue>.Remove(null) → ArgumentNullException
• HttpHeaders.Add(null, ...) → still throws ArgumentNullException (unchanged in this PR)

Silently absorbing null masks bugs in calling code. This was flagged by all three reviewers independently.

Fix: Add a null guard at the top of both methods:

public bool Contains(string name)
{
    ArgumentNullException.ThrowIfNull(name);
    return TryGetHeaderDescriptor(name, out HeaderDescriptor descriptor) &&
        Contains(descriptor);
}

public bool Remove(string name)
{
    ArgumentNullException.ThrowIfNull(name);
    return TryGetHeaderDescriptor(name, out HeaderDescriptor descriptor) &&
        Remove(descriptor);
}

Update the test Remove_UseNullOrEmptyHeaderName_ReturnsFalse / Contains_UseNullOrEmptyHeaderName_ReturnsFalse to split null back to a throw test and keep empty as returns-false.

⚠️ Breaking change documentation required — PR-level

Even if the team approves the behavioral change for FormatException/InvalidOperationException/ArgumentException("") cases, this is a breaking change that removes previously-thrown exceptions. Per the dotnet breaking change process, this requires a breaking change issue before merging, documenting:

• Old behavior: Contains/Remove threw FormatException for invalid names, InvalidOperationException for disallowed header types, ArgumentException for empty strings.
• New behavior: Returns false in all those cases.
• Justification: Aligns with TryAddWithoutValidation/TryGetValues patterns.
• Workaround: Callers that need validation should use HttpHeaders.Add or validate header names separately.

⚠️ Missing trailing newline — HttpResponseHeadersTest.cs

The original file ended with }\n (proper trailing newline). The PR removed it — the file now ends with } without a trailing newline (\ No newline at end of file in the diff). This violates the .editorconfig rule insert_final_newline = true. The commit message says "fix trailing newline" but the result is the opposite.

💡 Consider empty string handling — HttpHeaders.cs:312, 483

After this PR, Add("") throws ArgumentException while Contains("") returns false. Whether empty string should still throw is debatable, but worth an explicit decision. If the team accepts false for empty, the asymmetry with Add should be documented.

💡 Test coverage could be broader

The new tests cover content headers disallowed on request/response headers and request headers disallowed on content headers. Missing coverage for:

• Response-only headers (Age, Server) disallowed on request headers
• Request-only headers (Accept, Host) disallowed on response headers

These are exactly the InvalidOperationException → false transitions this PR enables.

---

Method Consistency After This PR

Method	Null/empty	Invalid name	Disallowed type
Add(string, ...)	throws	throws	throws
GetValues(string)	throws	throws	throws
TryAddWithoutValidation(string, ...)	false	false	false
TryGetValues(string, ...)	false	false	false
Contains(string) 🔄	false	false	false
Remove(string) 🔄	false	false	false

The bool-returning methods are now uniformly non-throwing (except Add/GetValues), which is a coherent split. The null concern is the main sticking point.

Models contributing: Claude Opus 4.6 (primary), GPT-5.3-Codex, System.Net domain reviewer (Claude Haiku 4.5)

Generated by Code Review for issue #127165 · ◷