Repository: netty/netty
Issue: Display which header failed validation

### Expected behavior

Currently if a HeaderValue fails validation we get an IllegalArgumentException which has the message `a header value contains prohibited character {some_invalid_character} at index {index}`. Seeing this in production makes it hard to debug which header is having this problem.

### Actual behavior
We should instead have the message, `Header value for {header_name} contains prohibited character {some_invalid_character} at index {index}

To fix this I am suggesting to change the interface of ValueValidator to also take in the key which will be purely used for adding debug information.

Issue comments:
When can this be released ?
I think in the next 2-3 weeks. 

Pull request review comments:
API Breakage
API Breakage
API Breakage
API Breakage
API Breakage
API Breakage
API Breakage
I know there is API breakage, but I was wondering if we can still afford it given that this interface is pretty recent.
No, we can't break applications.
Hmm, then I need to figure out another way to add this debug information. BTW the original change of adding the validation itself is a breaking change. 
No, it isn't. The public API was preserved.
The public API is preserved but the actual runtime behavior was changed. Prior to the validation change the header values were leniently accepted but now there is a stricter validation, which IMO is a good thing but it broke one of our applications which was adding an empty space in a header.

Now an error is thrown but it isn't clear which header is the one which has a whitespace.
I agree the runtime behaviour changed but I still think it was an "acceptable" change due the security impact of not doing so
I didn't look to closely but maybe what could be done is to "catch and rethrow" when this happens. The caller would then add the extra details 
@normanmaurer That was what I initially thought of doing, but it seemed hacky. I have another approach of creating another interface which is also API compatible. I will update the PR, if you like it that's fine. Otherwise will fallback to catch, change message and rethrow.
 ~~@normanmaurer This is what I am thinking of doing,~~

~~1. Create a new interface `HeaderValueValidator<K, V>`, the benefit of this interface would be if we want to do a contextual validations of headers on the basis of what the header represents it would be possible via this interface. I feel validation of header values without having context of their name works now but seems inflexible to me.~~
~~2. Mark `ValueValidator` as deprecated but still keep it around because we can't break public APIs. During actual validation we check which of `HeaderValueValidator` or  `ValueValidator` has been provided and based on that call the appropriate validator. If a user is currently extending `DefaultHeaders` their implementations would still pass  `ValueValidator` and continue to function as is, new users would pass a `HeaderValueValidator`.~~

~~I could have made `HeaderValueValidator` extend `ValueValidator` but that pollutes the new interface with an unnecessary `validate(V value)`.~~

~~WDYT ?~~

I ended up catching and throwing, because we do it many places in the codebase.
Throwing a stack trace is nothing less than making applications vulnerable to Denial-of-Service (DoS). This can be abused easily.

Consider making it stackless.
@normanmaurer WDYT?
I am not inclined to make this exception stackless, at the very most I can maybe remove the cause, but making it stackless severely hampers debugging ability and is not even the standard norm in the Netty codebase.

> Throwing a stack trace is nothing less than making applications vulnerable to Denial-of-Service (DoS)

If someone is trying to DoS an application by sending invalid headers, I don't think generating an exception stacktrace would be the bottleneck. Logging the stack trace might be but there are other mitigations for that in the logger configurations which applications need to use anyways.
No need to concat the message. It will be part of the wrapped cause anyway. 
```suggestion
            throw new IllegalArgumentException("Validation failed for header '" + name + "'", e);
```
I agree with @adwsingh here... I think its ok to just wrap.
I will do profiling in the future for this and see how things behave in stack trace vs stackless.

Patch that resolved the issue:
diff --git a/codec/src/main/java/io/netty/handler/codec/DefaultHeaders.java b/codec/src/main/java/io/netty/handler/codec/DefaultHeaders.java
index 6abe71be0e2..6ff05fd6acb 100644
--- a/codec/src/main/java/io/netty/handler/codec/DefaultHeaders.java
+++ b/codec/src/main/java/io/netty/handler/codec/DefaultHeaders.java
@@ -1013,7 +1013,11 @@ protected void validateName(NameValidator<K> validator, boolean forAdd, K name)
     }
 
     protected void validateValue(ValueValidator<V> validator, K name, V value) {
-        validator.validate(value);
+        try {
+            validator.validate(value);
+        } catch (IllegalArgumentException e) {
+            throw new IllegalArgumentException("Validation failed for header '" + name + "'", e);
+        }
     }
 
     protected HeaderEntry<K, V> newHeaderEntry(int h, K name, V value, HeaderEntry<K, V> next) {


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/DefaultHttpHeadersTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/DefaultHttpHeadersTest.java
index 4cd1beedfe3..77e6fd37629 100644
--- a/codec-http/src/test/java/io/netty/handler/codec/http/DefaultHttpHeadersTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/DefaultHttpHeadersTest.java
@@ -238,23 +238,25 @@ public void setObjectIterable() {
     @Test
     public void setCharSequenceValidatesValue() {
         final DefaultHttpHeaders headers = newDefaultDefaultHttpHeaders();
-        assertThrows(IllegalArgumentException.class, new Executable() {
+        IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, new Executable() {
             @Override
             public void execute() throws Throwable {
                 headers.set(HEADER_NAME, ILLEGAL_VALUE);
             }
         });
+        assertTrue(exception.getMessage().contains(HEADER_NAME));
     }
 
     @Test
     public void setIterableValidatesValue() {
         final DefaultHttpHeaders headers = newDefaultDefaultHttpHeaders();
-        assertThrows(IllegalArgumentException.class, new Executable() {
+        IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, new Executable() {
             @Override
             public void execute() throws Throwable {
                 headers.set(HEADER_NAME, Collections.singleton(ILLEGAL_VALUE));
             }
         });
+        assertTrue(exception.getMessage().contains(HEADER_NAME));
     }
 
     @Test