Fix value accumulation for case-sensitive keys in NettyHeadersAdapter

[qnnn]

The current NettyHeaders supports case-sensitive storage of entry values when using the add method. This causes the NettyHeadersAdapter to accumulate the values for the corresponding key when retrieving headers(see unit test).

Add a unit test testHeadersOutput to verify the final output of the headers.

[qnnn]

addAll has the same issue, and it has also been addressed.

[simonbasle]

hey @qnnn thanks for the PR. Can you elaborate on the use case that lead you to come up with this change? I'm guessing this is a matter of enumerating the headers or representing them as a whole?

[qnnn]

hey @qnnn thanks for the PR. Can you elaborate on the use case that lead you to come up with this change? I'm guessing this is a matter of enumerating the headers or representing them as a whole?

@simonbasle I discovered this problem while using the AddRequestHeader GatewayFilter in Spring Cloud Gateway. Through network packet capture, it can be seen that the HTTP requests sent by the Gateway aggregate the values of case-sensitive keys in the HTTP headers.
The expected request header should be testheader=first, second, but the actual result is testheader=first, second, first, second (the same as the output in the unit test).

[simonbasle]

Thanks for your feedback. We're exploring what we can do to improve enumeration/iteration of headers through the framework's MultiValueMap adapters, which seems to be the root of the issue. Basically, iterating over the entrySet or dumping a string representation in logs is susceptible to display fabricated duplicates for header names that have been added with multiple casing.

While all native implementations of http headers enforce case-insensitive access (as mandated by the spec), some still store headers as name-value arrays / lists where each entry's might have differences in casing.

This leads to apparent inconsistencies for "whole collection" methods when we try to offer a MultiValueMap view over these implementations, as they do not align well with that representation. Most notably:

• size()
• entrySet() (set of Entry<String, List<String>>>) which is used by HttpHeaders#formatHeaders
• keySet()
• values()

Can you confirm whether or not the duplication of values for a given header name happens on the wire however? (eg. using Wireshark)

[qnnn]

@simonbasle Thanks for your patience in explaining this. Yes, I can confirm that the duplication of values for a given header name happens on the wire (confirmed via Wireshark).

It seems that some Spring Cloud Gateway's header filters are causing this issue when they transform the NettyHeaderAdapter into MultiValueMapAdapter<>(new LinkedCaseInsensitiveMap<>(8, Locale.ENGLISH)) (such as in org.springframework.cloud.gateway.filter.headers.RemoveHopByHopHeadersFilter#filter).

I feel that MultiValueMapAdapter<>(new LinkedCaseInsensitiveMap<>(8, Locale.ENGLISH)) is a standards-compliant header implementation, and when other header adapters are converted to it, they should produce the same result (case-insensitive).

[sdeleuze]

@qnnn Is this use case of headers filtering/reconstruction in org.springframework.cloud.gateway.filter.headers.RemoveHopByHopHeadersFilter is the only concrete bug you face or are there other ones (if yes, which ones)?

[qnnn]

@sdeleuze Yes, I have verified that there are others as well:

• org.springframework.cloud.gateway.filter.headers.XForwardedHeadersFilter
• org.springframework.cloud.gateway.filter.headers.ForwardedHeadersFilter

Any implementation that iterates through the Netty Headers Adapter's entrySet and then uses addAll may have this issue. Such as(I haven’t verified this yet; it's just a guess):

• org.springframework.cloud.gateway.filter.WebsocketRoutingFilter
• org.springframework.cloud.gateway.filter.headers.TransferEncodingNormalizationHeadersFilter
• org.springframework.cloud.gateway.filter.factory.cache.CachedResponse$Builder#headers

[sdeleuze]

Thanks, that helps. We are discussing the best strategy to fix those bugs while keeping a great performance/efficiency. We will get back to you once we have made a decision.

[qnnn]

Thanks, that helps. We are discussing the best strategy to fix those bugs while keeping a great performance/efficiency. We will get back to you once we have made a decision.

@sdeleuze It's ok, I understand.

Perhaps maintaining a caseInsensitiveKeyMap to store case-insensitive keys might help with performance—(please disregard if it’s not useful.)

When operating on NettyHeaders, the following map could be updated simultaneously:

    Map<String, String> map = new LinkedCaseInsensitiveMap<>(8, Locale.ENGLISH);
    String headerKey = map.compute("TestHeader", (caseInsensitiveKey, headerKey) -> {
        if (!StringUtils.hasText(headerKey)){
	    return caseInsensitiveKey;
	}
	return headerKey;
    });

Something similar:

[simonbasle]

Correct me if I'm wrong, but the headers case variants are populated by Netty itself, i.e. it comes from the wire. In that case you are addressing the wrong issue.

The change and tests that you proposed impact the intermediate Netty4HeadersAdapter (adapting Netty DefaultHttpHeaders to MultiValueMap<String, String>)... but if you add the test's headers to DefaultHttpHeader instance itself, then construct a Netty4HeadersAdapter out of it, tests fail again despite your changes:

	@Test
	void prepopulatedNativeNetty4() {
		DefaultHttpHeaders native = new DefaultHttpHeaders();
		native.add("TestHeader", "first");
		native.add("TestHEADER", "second");
		native.add("SecondHeader", "value");
		native.add("TestHeader", "third");

		MultiValueMap<String, String> adapter = new Netty4HeadersAdapter(native);

		//copying native headers into a new HttpHeaders
		HttpHeaders headers = new HttpHeaders();
		for (Map.Entry<String, List<String>> entry : adapter.entrySet()) {
			headers.addAll(entry.getKey(), entry.getValue());
		}

		assertThat(headers.get("TestHeader")).as("TestHeader")
				.containsExactly("first", "second", "third");
	}

[simonbasle]

@qnnn we continue evaluating the possible fixes and their impact. I will probably close this PR in favor of creating a new issue tracking this, in which case I'll ping you in there.

[qnnn]

Correct me if I'm wrong, but the headers case variants are populated by Netty itself, i.e. it comes from the wire. In that case you are addressing the wrong issue.

The change and tests that you proposed impact the intermediate Netty4HeadersAdapter (adapting Netty DefaultHttpHeaders to MultiValueMap<String, String>)... but if you add the test's headers to DefaultHttpHeader instance itself, then construct a Netty4HeadersAdapter out of it, tests fail again despite your changes:

	@Test
	void prepopulatedNativeNetty4() {
		DefaultHttpHeaders native = new DefaultHttpHeaders();
		native.add("TestHeader", "first");
		native.add("TestHEADER", "second");
		native.add("SecondHeader", "value");
		native.add("TestHeader", "third");

		MultiValueMap<String, String> adapter = new Netty4HeadersAdapter(native);

		//copying native headers into a new HttpHeaders
		HttpHeaders headers = new HttpHeaders();
		for (Map.Entry<String, List<String>> entry : adapter.entrySet()) {
			headers.addAll(entry.getKey(), entry.getValue());
		}

		assertThat(headers.get("TestHeader")).as("TestHeader")
				.containsExactly("first", "second", "third");
	}

@simonbasle Indeed, perhaps this can be reset during the Adapter initialization? In my understanding, Netty itself can function normally.

[simonbasle]

Superseded by gh-33823