Repository: netty/netty
Issue: AbstractHttpData is too restrict in name
Labels: defect

When a html form input name includes space, an IllegalArgumentException will be thrown. However, in the specification, name is actually defined as CDATA. See http://www.w3.org/TR/xhtml1/dtds.html#dtdentry_xhtml1-transitional.dtd_input

```
protected AbstractHttpData(String name, Charset charset, long size) {
    if (name == null) {
        throw new NullPointerException("name");
    }
    name = name.trim();
    if (name.isEmpty()) {
        throw new IllegalArgumentException("empty name");
    }

    for (int i = 0; i < name.length(); i ++) {
        char c = name.charAt(i);
        if (c > 127) {
            throw new IllegalArgumentException(
                    "name contains non-ascii character: " + name);
        }

        // Check prohibited characters.
        switch (c) {
        case '=':
        case ',':
        case ';':
        case ' ':
        case '\t':
        case '\r':
        case '\n':
        case '\f':
        case 0x0b: // Vertical tab
            throw new IllegalArgumentException(
                    "name contains one of the following prohibited characters: " +
                    "=,; \\t\\r\\n\\v\\f: " + name);
        }
    }
    this.name = name;
    if (charset != null) {
        setCharset(charset);
    }
    definedSize = size;
}
```


Issue comments:
Could anyone take a look a this one? It causes lots of web site to fail

Sorry I'm quite busy… maybe you could issue a pull-req with a fix ?

Am 08.04.2013 um 18:45 schrieb xhh11 notifications@github.com:

> Could anyone take a look a this one? It causes lots of web site to fail
> 
> —
> Reply to this email directly or view it on GitHub.

Sorry but you point to XHTML, which is not HTML...
See http://www.w3.org/TR/REC-html40/types.html where name is expressed as:

```
ID and NAME tokens must begin with a letter ([A-Za-z]) and may be followed by any number of letters, digits ([0-9]), hyphens ("-"), underscores ("_"), colons (":"), and periods (".").
```

And in http://www.w3.org/TR/xhtml1/diffs.html#diffs
There is the following indication:

```
Due to the fact that XHTML is an XML application, certain practices that were perfectly legal in SGML-based HTML 4 [HTML4] must be changed.
```

And for name and id:

```
4.2. Element and attribute names must be in lower case
XHTML documents must use lower case for all HTML element and attribute names. This difference is necessary because XML is case-sensitive e.g. <li> and <LI> are different tags.
```

and

```
4.10. The elements with 'id' and 'name' attributes

HTML 4 defined the name attribute for the elements a, applet, form, frame, iframe, img, and map. HTML 4 also introduced the id attribute. Both of these attributes are designed to be used as fragment identifiers.

In XML, fragment identifiers are of type ID, and there can only be a single attribute of type ID per element. Therefore, in XHTML 1.0 the id attribute is defined to be of type ID. In order to ensure that XHTML 1.0 documents are well-structured XML documents, XHTML 1.0 documents MUST use the id attribute when defining fragment identifiers on the elements listed above. See the HTML Compatibility Guidelines for information on ensuring such anchors are backward compatible when serving XHTML documents as media type text/html.

Note that in XHTML 1.0, the name attribute of these elements is formally deprecated, and will be removed in a subsequent version of XHTML.
```

So, if I am reading correctly, no "space" are allowed in name attribute.

Am I wrong?

You are right on HTML 4 spec. HTML 5 actually specifies: Attribute names must consist of one or more characters other than the space characters, U+0000 NULL, """, "'", ">", "/", "=", the control characters, and any characters that are not defined by Unicode.

But the reality is that the specs are different and people are not strictly following specs. When Netty lib parses a form submission,  the current strict implementation cannot handle some web sites that are not following the strict implementation. IMO, the Netty lib should leave the restriction to the apps that using Netty instead of making handling of those "mis-formed" web pages impossible.

Hi Norman,

The fix is easy, just remove the name check code. I just want to make sure you guys agree on this.

I'm not so happy to remove all those checks.
But anyway, maybe removing some could be useful as you said.
Among all items, are they some that really need to be out (taking into account that it will allow some bad things and could have some side effects on other code, not in Netty, but in application side since non conforment to HTML rules).

I believe you asked at least for ' ' to be removed. Is there any other ?
I would feel strange for instance if we removed the following ones (using (*) to note them):

```
switch (c) {
    case '=': (*)
    case ',': (*)
    case ';': (*)
    case ' ':
    case '\t':
    case '\r': (*)
    case '\n': (*)
    case '\f': (*)
    case 0x0b: // Vertical tab
        throw new IllegalArgumentException(
                "name contains one of the following prohibited characters: " +
                "=,; \\t\\r\\n\\v\\f: " + name);
```

Another way would be to add a property to the configuration of the Http driver, in order to say: ignore bad characters in attribute name. WDYT ?
However the difficulty would be to find where could be the best place for that configuration ?
Putting it in HttpDataFactory could be the right solution but will force to create new constructors for all items with an extra argument to say: ignore those name checking.

My 2 cents ;-)

As far as the application can decide what to do with the atypical names, it would be fine.

The options can be put into constructor of HttpPostRequestDecoder. An application either handle those strange names itself or just fail if the names don't follow specs. I don't think an application would need both.

I've made a proposal to fix that issue.
But note however that I did not check it intensively (and I had to do this one through web since I've got some serious issues on my PC configuration). So please, before accepting it, someone has to check it.
The very same could be done for the other stream (3 ? ).

I leave one comment that we feel from "first at a glance" fix proposal (not final one):
- do you confirm that if disabled, all checking must be ignored (whatever ',', '=', ';', '\n', ...) ?

Yes. I see no reason why a web app just wants to get an Exception instead of the name/value pair that is inside the content. Maybe the checking functionality can be pull into a helper class/method. 

I don't see what you mean by helper class/method?
=> Could you explain that point?

I don't want to reopen the discussion, but for the reason for a web app to get an Exception, I think that in normal situation, it is preferable to get an exception than accepting wrong HTTP request/answer, in order to enforce the security, since most of the time, non conforming client/server should be considered as potential attacks.

What we can propose, is all OR nothing, in a property to set within the DefaultHttpDataFactory. If disabled (not by default), then all checking MUST be done by the application, while it should have been done in the HTTP codec.

Take for instance that someone implements badly SSL, would you accept that the codec will allow you to continue with the SSL connection, while it is not conform to the rules? I think not.

However, if one have real need for that (and that the non conform client/server are out of control), then the application will have to handle all cases itself.

Maybe we could also put the check code in a separate method in order to allow someone to check manually the correctness (while still done by default), but then useful for the application that disable this automatic checking.
=> Is that the helper class/method you were talking about?

Yes, I meant that user can get the name/value pair and then call something like isValidHTML4Name(String name) to verify if name is valid.

The situation I saw was that the existing browser and web server can all live with the "bad" names. As a network lib, we definitely want user to have the flexibility to do the same thing. 

Anyway, what you did is perfect for all needs. Thanks

@trustin I think we can move this to CR3 ?

I had a chance to read the specification today, and I guess you guys are confusing the name of types from the 'NAME' attribute in INPUT.  The type of INPUT.NAME attribute is CDATA, which means the validation logic should be:

> CDATA is a sequence of characters from the document character set and may include character entities. User agents should interpret attribute values as follows:
> - Replace character entities with characters,
> - Ignore line feeds,
> - Replace each carriage return or tab with a single space.
> 
> User agents may ignore leading and trailing white space in CDATA attribute values (e.g., "   myval   " may be interpreted as "myval"). Authors should not declare attribute values with leading or trailing white space.
> 
> For some HTML 4 attributes with CDATA attribute values, the specification imposes further constraints on the set of legal values for the attribute that may not be expressed by the DTD.
> 
> Although the STYLE and SCRIPT elements use CDATA for their data model, for these elements, CDATA must be handled differently by user agents. Markup and entities must be treated as raw text and passed to the application as is. The first occurrence of the character sequence "</" (end-tag open delimiter) is treated as terminating the end of the element's content. In valid documents, this would be the end tag for the element.

Fixed both in 3 and master.

Sorry Trustin but:

In chapter [6.2 SGML basic types] of http://www.w3.org/TR/REC-html40/types.html, the CDATA is restricted for ID and NAME as follow:

> ID and NAME tokens must begin with a letter ([A-Za-z]) and may be followed by any number of letters, digits 
> ([0-9]), hyphens ("-"), underscores ("_"), colons (":"), and periods (".").

So it might be not so simple... my 2 cents.
Cheers, 

No.  The `ID` and `NAME` tokens here refers to the DTD types, not attributes.  Also, I don't see anything that says `CDATA` must conform to `ID`/`NAME` rules.

Yes, you're right! My misunderstanding... Thank you Trustin!


Pull request review comments:
Do we have to disable the check code completely?  Shouldn't the characters like `\t`, `\r`, `\n`, `\f`, `0x0b` be prohibited yet?

Please keep the old signature and add a new one.  We should probably set the default less strict.

Please fill the Javadoc correctly or just leave it empty.

The description of this method is not very informative.  Please revise.

Same applies to the subtypes.

Why pass `false` rather than `this.checkBadName`?

There are many similar issues in this pull request.  Please fix them, too.

On all others I am totally agree. I was doing it just to show what can be done, not really a final step (I should say it).
On this point, (and in other part of the code wher I put false also):
- the checkBadName is only visible in DefaultHttpDataFactory, and I fell like unuseful to push this variable into each and every Data created.
- since those "copy" methods will be called after the constructor was already called, we have the 2 following cases:
  a) checkBadName was true then checking it again on the very same name (getName() is called) is not useful
  b) checkBadName was false, then we must not check it (since it could break the checking rules)

So I believe that put false in "copy" methods is correct.

On this one, I really would like that the user that asked this change specifies what to remove or not.
As I said in the ticket, I don't feel confortable with not checkink '=', ',', ';' and maybe '\n', '\r'.
So without answer, maybe we could make this by unchecking all, leaving the application with the responsability to check itself.
By default, all checking should be done, since it is the official way, even in HTML5.

Could we ever make this pluggable so that a user can choose whatever naming rule he wants to enforce?  Perhaps a user could pass a matcher when creating `HttpPostRequestDecoder`?  The default could be the strictly RFC-conforming one.

Yes it is still possible, but note that both HttpPostRequestDecoder and HttpPostRequestEncoder have the HattpDataFactory as argument, which should be setup to use (or not use) this RFC conforming rule, since this is where (HttpDataFactory) this rule has to be setup.

When I wrote this helper method, I feel like a specific class should be setup (a new class helper) such that it can easily be reused, maybe in the DefaultHttpDataFactory class, or a new one (which name ?).
Note that I'm not so happy that someone could implement a "bad" conforming service (contrary to any good practices in IT), so the reason to not make it "so" easy, and in particular not the default one. So putting this as an argument to the Encoder/Decoder as explicit argument is somehow not particularly intended.

Moreovern put it in Encoder/Decoder will break the underlying implementation where the factory is responsible of the constructor (factory), while the encoder/decoder is not. It could lead to an heavy change (I did not take time however to see if it is particularly difficult or not).

So let me know what is your preference...
Cheers

I prefer to:
1. Add a matcher interface
2. Add the matcher as a constructor parameter of `DefaultHttpDataFactory`
3. Keep the old constructors of `DefaultHttpDataFactory` which conforms to the RFC strictly.

OK, I agree since it is more "adaptable" then than just all or nothing.
Just to ensure I have correctly understood (I'm so busy that I need to someone recheck, sorry ;-(
You want to have:
1. the user can define his own Pattern p = Pattern.compile("xxx");
2. Then giving this Pattern p as argument to DefaultHttpDataFactory new constructor (keeping the one without extra argument, but removing the extra set methods and in particular this isValidHtml4Name method)
3. By default, if no argument is given, the following Pattern will be compiled
   
   Pattern.compile("\p{ASCII}&&[^=,;\s]");

Is that correct?
Also, if you could check the pattern, since 4 eyes are better than one when playing with regex...
(note that there is double '\' but git show only one)
If I'm correct: 
- \p{ASCII} => [\x00-\x7F] = c <= 127
- &&[^=,;\s] => substract '=', ',', ';', any "space character"

Cheers

One more comment, while I tried to implement this option, I found out that I need to be able to pass this Pattern in every cases, and in particular in copy mode.
While I don't think it is a good idea to "copy" the pattern in each and every item (data), due to memory overhead that it could imply, the only solution I figure out right now is to put this Pattern in a static variable.

But as it must be accessed by all and every part of the code, the change should then be as the following:
1. Create an abstract class of the interface HttpDataFactory that will host this static Pattern variable, and nothing more.
2. Change all access or definition to HttpDataFactory to the new AbstractHttpDataFactory, in order that this variable is visible, and moreover that someone implementing a new Factory will have to start from this abstract.

Is that correct? Or do you have a better idea?
I don't really like the "static" way, but I don't find a clear way to do that.
Once again, this is the "copy" function that brings the issue...

Instead of a Pattern argument can't you just define an interface that has a function to match an individual character? something like:

boolean isValidCharacter(char c)

that can be called by the loop and the exception thrown if it return false.

I add one answer and one comment:
1. isValidCharacter(char c) will be very very expensive in term of computation. In this codec various items are made to try to be as much as possible optimized (in particular, we allow mixed types of variable, in memory or on disk according to a size, and we try when possible to check arrays directly without getting bytes one by one). So putting this will lead to obviously a slow down in the overall process. So I feel like this is not a good idea. The pattern, even if costly too, should be faster than one by one check.
2. On my "pre" test to make this Abstract class with static property Pattern, I must admit that, even if it is not as clean as I would like, it is really helpful, since:
3. no more constructor with or without argument on each "data" to specify if we want to check and how the name W3C rule
4. only the factory has those constructors (and those without a specific pattern will cause the pattern to be the standard one)
5. In more than 90%, I feel like once someone decides to use a specific pattern (or the default one), he/she probably stick with this one for every implementation in one VM.

So while answering 90% of the case (one way used to verify the W3C conformity), there is still some cases where perhaps some people could want to have several web services in the same VM using different patterns. In this case, what could be done is to specify the "less" restrictive pattern for all, and then explicitely call an helper class (put on Data classes) that could check the name with a specific (maybe one shot) pattern given as parameter, thus allowing to check various levels of conformance.

WDYT?

Slightly confused as to the expense of a character at a time check given the for loop in the code below at line 46 which is already doing a character at a time check. Or are you suggesting that the function call itself is expensive?

Ah, ok, I understand the confusion.
Currently yes, the code is one by one character, but however in the same "method" which tends to be optimized, compared to a method called each and every time (compare getByte() to getBytes(byte[]) then looping on array).

However the remarks on going are more on the request from Trustin about using a Pattern instead of a method as currently implemented. Therefore, the code will look like:

```
if (! pattern.matcher(name).matches())
```

instead of

```
if (! isValidHtml4Name(name)) 
```

so replacing the method isValidHtml4Name by a call to a "supposed to be" more efficient way (pattern.matcher) and moreover more flexible for the user (he/she can implement the pattern he wants instead of the current logic which is all or nothing).

Sorry for the confusion...

Which method is "faster" is hard to say as the JIT of course is very magical :)

But I'm not too concerned about that -- I'm more curious about your comment above:

"Change all access or definition to HttpDataFactory to the new AbstractHttpDataFactory, in order that this variable is visible, and moreover that someone implementing a new Factory will have to start from this abstract."

since wouldn't this break the existing API?

To the breaking of API, the answer is yes and no:
- yes: it will introduce a new Abstract class that is the first level of the Interface HttpDataFactory as the factory will have to include a "static" member : the pattern
- no : except that, no other change (even less than in the current example which is fully compatible with the prior API already)

So from a strict point of view, yes, there is potential break. The only way I see to not introduce this change is to replace the interface HttpDataFactory by the Abstract class directly (keeping the very same name), but then lead to a change of "implements" to "extends".

So far, I'm not so happy about this change (as you understand, I try to find the best way, taking all advices).
So to resume;
1. The current proposition does not change the API, but lack of "control" for the programmer, since the method is then "all or nothing".
2. The next proposition (abstract class) allows more flexibility but could lead to an API change (minor but still).
3. The next-next proposition would be to "duplicate" the pattern in each and every "data", by constructor addition, but in fact will lead to the very same kind of API break, since each "data" will have to extend their own constructor to accept such a pattern, and if none specified (default current constructor), they will have to rely on the "static default pattern" (especially the "copy" operation), which could lead to 2 optional issues:
- either the default pattern is different to the default one specified by the user in the factory, and then could lead to misbehaviour
- either we introduce again the abstract class (proposition 2) to host the current pattern (whatever the one from the user if specified, or the default one) and then we came back to the API issue of (2) (and moreover no interest of solution 3).

I must admit that it is like the chicken and egg issue...


Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractDiskHttpData.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractDiskHttpData.java
index c854a719d23..6199b676f32 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractDiskHttpData.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractDiskHttpData.java
@@ -38,8 +38,12 @@ public abstract class AbstractDiskHttpData extends AbstractHttpData {
     private boolean isRenamed;
     private FileChannel fileChannel;
 
+    protected AbstractDiskHttpData(String name, Charset charset, long size, boolean checkBadName) {
+        super(name, charset, size, checkBadName);
+    }
+
     protected AbstractDiskHttpData(String name, Charset charset, long size) {
-        super(name, charset, size);
+        this(name, charset, size, false);
     }
 
     /**
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractHttpData.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractHttpData.java
index d80b57f735f..0066de9d743 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractHttpData.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractHttpData.java
@@ -35,19 +35,20 @@ public abstract class AbstractHttpData extends AbstractReferenceCounted implemen
     protected boolean completed;
 
     protected AbstractHttpData(String name, Charset charset, long size) {
-        if (name == null) {
-            throw new NullPointerException("name");
-        }
-        name = name.trim();
-        if (name.isEmpty()) {
-            throw new IllegalArgumentException("empty name");
-        }
+        this(name, charset, size, false);
+    }
 
+    /**
+     * Convenient method to check if an attribute name respects the W3C rules
+     * (character are ASCII (less than 127) and no characters from "=,; \t\r\n\v\f:")
+     * @param name the attribute name to check
+     * @return True if the name is conform with the W3C rules
+     */
+    public static boolean isValidHtml4Name(String name) {
         for (int i = 0; i < name.length(); i ++) {
             char c = name.charAt(i);
             if (c > 127) {
-                throw new IllegalArgumentException(
-                        "name contains non-ascii character: " + name);
+                return false;
             }
 
             // Check prohibited characters.
@@ -61,11 +62,25 @@ protected AbstractHttpData(String name, Charset charset, long size) {
             case '\n':
             case '\f':
             case 0x0b: // Vertical tab
-                throw new IllegalArgumentException(
-                        "name contains one of the following prohibited characters: " +
-                        "=,; \\t\\r\\n\\v\\f: " + name);
+                 return false;
             }
         }
+        return true;
+    }
+
+    protected AbstractHttpData(String name, Charset charset, long size, boolean checkBadName) {
+        if (name == null) {
+            throw new NullPointerException("name");
+        }
+        name = name.trim();
+        if (name.isEmpty()) {
+            throw new IllegalArgumentException("empty name");
+        }
+        if (checkBadName && ! isValidHtml4Name(name)) {
+            throw new IllegalArgumentException(
+                    "name contains non-ascii character or one of the following prohibited characters: " +
+                            "=,; \\t\\r\\n\\v\\f: " + name);
+        }
         this.name = name;
         if (charset != null) {
             setCharset(charset);
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractMemoryHttpData.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractMemoryHttpData.java
index f3461d8f883..1c12a2269f5 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractMemoryHttpData.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractMemoryHttpData.java
@@ -39,7 +39,11 @@ public abstract class AbstractMemoryHttpData extends AbstractHttpData {
     protected boolean isRenamed;
 
     protected AbstractMemoryHttpData(String name, Charset charset, long size) {
-        super(name, charset, size);
+        this(name, charset, size, false);
+    }
+
+    protected AbstractMemoryHttpData(String name, Charset charset, long size, boolean checkBadName) {
+        super(name, charset, size, checkBadName);
     }
 
     @Override
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DefaultHttpDataFactory.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DefaultHttpDataFactory.java
index 8f6f68e18b5..0ad8b01b6af 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DefaultHttpDataFactory.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DefaultHttpDataFactory.java
@@ -44,6 +44,11 @@ public class DefaultHttpDataFactory implements HttpDataFactory {
 
     private long minSize;
 
+    /**
+     * Default should check the correctness with W3c rules on attribute names
+     */
+    private boolean checkBadName = true;
+
     /**
      * Keep all HttpDatas until cleanAllHttpDatas() is called.
      */
@@ -78,6 +83,26 @@ public DefaultHttpDataFactory(long minSize) {
         this.minSize = minSize;
     }
 
+    /**
+     * @return True if the current Factory is checking the correctness of the attribute names regarding W3c rules
+     * which say character are ASCII (less than 127) and no characters from "=,; \t\r\n\v\f:"
+     */
+    public boolean isCheckBadName() {
+        return checkBadName;
+    }
+
+    /**
+     * Note that it is still possible to check later on manually using
+     * {@link AbstractHttpData}.isValidHtml4Name(String name) method
+     *
+     * @param checkBadName True to check if names conform to HTML definition ASCII (less than 127)
+     * and no characters from "=,; \t\r\n\v\f:", and False to ignore this check.
+     *
+     */
+    public void setCheckBadName(boolean checkBadName) {
+        this.checkBadName = checkBadName;
+    }
+
     /**
      * @return the associated list of Files for the request
      */
@@ -93,18 +118,18 @@ private List<HttpData> getList(HttpRequest request) {
     @Override
     public Attribute createAttribute(HttpRequest request, String name) {
         if (useDisk) {
-            Attribute attribute = new DiskAttribute(name);
+            Attribute attribute = new DiskAttribute(name, checkBadName);
             List<HttpData> fileToDelete = getList(request);
             fileToDelete.add(attribute);
             return attribute;
         }
         if (checkSize) {
-            Attribute attribute = new MixedAttribute(name, minSize);
+            Attribute attribute = new MixedAttribute(name, minSize, checkBadName);
             List<HttpData> fileToDelete = getList(request);
             fileToDelete.add(attribute);
             return attribute;
         }
-        return new MemoryAttribute(name);
+        return new MemoryAttribute(name, checkBadName);
     }
 
     @Override
@@ -112,23 +137,23 @@ public Attribute createAttribute(HttpRequest request, String name, String value)
         if (useDisk) {
             Attribute attribute;
             try {
-                attribute = new DiskAttribute(name, value);
+                attribute = new DiskAttribute(name, value, checkBadName);
             } catch (IOException e) {
                 // revert to Mixed mode
-                attribute = new MixedAttribute(name, value, minSize);
+                attribute = new MixedAttribute(name, value, minSize, checkBadName);
             }
             List<HttpData> fileToDelete = getList(request);
             fileToDelete.add(attribute);
             return attribute;
         }
         if (checkSize) {
-            Attribute attribute = new MixedAttribute(name, value, minSize);
+            Attribute attribute = new MixedAttribute(name, value, minSize, checkBadName);
             List<HttpData> fileToDelete = getList(request);
             fileToDelete.add(attribute);
             return attribute;
         }
         try {
-            return new MemoryAttribute(name, value);
+            return new MemoryAttribute(name, value, checkBadName);
         } catch (IOException e) {
             throw new IllegalArgumentException(e);
         }
@@ -140,20 +165,20 @@ public FileUpload createFileUpload(HttpRequest request, String name, String file
             long size) {
         if (useDisk) {
             FileUpload fileUpload = new DiskFileUpload(name, filename, contentType,
-                    contentTransferEncoding, charset, size);
+                    contentTransferEncoding, charset, size, checkBadName);
             List<HttpData> fileToDelete = getList(request);
             fileToDelete.add(fileUpload);
             return fileUpload;
         }
         if (checkSize) {
             FileUpload fileUpload = new MixedFileUpload(name, filename, contentType,
-                    contentTransferEncoding, charset, size, minSize);
+                    contentTransferEncoding, charset, size, minSize, checkBadName);
             List<HttpData> fileToDelete = getList(request);
             fileToDelete.add(fileUpload);
             return fileUpload;
         }
         return new MemoryFileUpload(name, filename, contentType,
-                contentTransferEncoding, charset, size);
+                contentTransferEncoding, charset, size, checkBadName);
     }
 
     @Override
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DiskAttribute.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DiskAttribute.java
index ccb59bdfa3b..8b37e6b5d58 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DiskAttribute.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DiskAttribute.java
@@ -39,11 +39,19 @@ public class DiskAttribute extends AbstractDiskHttpData implements Attribute {
      * Constructor used for huge Attribute
      */
     public DiskAttribute(String name) {
-        super(name, HttpConstants.DEFAULT_CHARSET, 0);
+        this(name, false);
+    }
+
+    public DiskAttribute(String name, boolean checkBadName) {
+        super(name, HttpConstants.DEFAULT_CHARSET, 0, checkBadName); // Attribute have no default size
     }
 
     public DiskAttribute(String name, String value) throws IOException {
-        super(name, HttpConstants.DEFAULT_CHARSET, 0); // Attribute have no default size
+        this(name, value, false);
+    }
+
+    public DiskAttribute(String name, String value, boolean checkBadName) throws IOException {
+        this(name, checkBadName);
         setValue(value);
     }
 
@@ -142,7 +150,7 @@ protected String getPrefix() {
 
     @Override
     public Attribute copy() {
-        DiskAttribute attr = new DiskAttribute(getName());
+        DiskAttribute attr = new DiskAttribute(getName(), false);
         attr.setCharset(getCharset());
         ByteBuf content = data();
         if (content != null) {
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DiskFileUpload.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DiskFileUpload.java
index 988bf955ede..df9c2d74255 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DiskFileUpload.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DiskFileUpload.java
@@ -43,7 +43,12 @@ public class DiskFileUpload extends AbstractDiskHttpData implements FileUpload {
 
     public DiskFileUpload(String name, String filename, String contentType,
             String contentTransferEncoding, Charset charset, long size) {
-        super(name, charset, size);
+        this(name, filename, contentType, contentTransferEncoding, charset, size, false);
+    }
+
+    public DiskFileUpload(String name, String filename, String contentType,
+            String contentTransferEncoding, Charset charset, long size, boolean checkBadName) {
+        super(name, charset, size, checkBadName);
         setFilename(filename);
         setContentType(contentType);
         setContentTransferEncoding(contentTransferEncoding);
@@ -166,7 +171,7 @@ protected String getPrefix() {
     @Override
     public FileUpload copy() {
         DiskFileUpload upload = new DiskFileUpload(getName(),
-                getFilename(), getContentType(), getContentTransferEncoding(), getCharset(), size);
+                getFilename(), getContentType(), getContentTransferEncoding(), getCharset(), size, false);
         ByteBuf buf = data();
         if (buf != null) {
             try {
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/MemoryAttribute.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/MemoryAttribute.java
index c22c26e5681..ed32a5ee713 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/MemoryAttribute.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/MemoryAttribute.java
@@ -29,11 +29,19 @@
 public class MemoryAttribute extends AbstractMemoryHttpData implements Attribute {
 
     public MemoryAttribute(String name) {
-        super(name, HttpConstants.DEFAULT_CHARSET, 0);
+        this(name, false);
+    }
+
+    public MemoryAttribute(String name, boolean checkBadName) {
+        super(name, HttpConstants.DEFAULT_CHARSET, 0, checkBadName); // Attribute have no default size
     }
 
     public MemoryAttribute(String name, String value) throws IOException {
-        super(name, HttpConstants.DEFAULT_CHARSET, 0); // Attribute have no default size
+        this(name, value, false);
+    }
+
+    public MemoryAttribute(String name, String value, boolean checkBadName) throws IOException {
+        this(name, checkBadName);
         setValue(value);
     }
 
@@ -103,7 +111,7 @@ public String toString() {
 
     @Override
     public Attribute copy() {
-        MemoryAttribute attr = new MemoryAttribute(getName());
+        MemoryAttribute attr = new MemoryAttribute(getName(), false);
         attr.setCharset(getCharset());
         ByteBuf content = data();
         if (content != null) {
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/MemoryFileUpload.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/MemoryFileUpload.java
index d31d4e88ce1..e639f49e61f 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/MemoryFileUpload.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/MemoryFileUpload.java
@@ -37,7 +37,12 @@ public class MemoryFileUpload extends AbstractMemoryHttpData implements FileUplo
 
     public MemoryFileUpload(String name, String filename, String contentType,
             String contentTransferEncoding, Charset charset, long size) {
-        super(name, charset, size);
+        this(name, filename, contentType, contentTransferEncoding, charset, size, false);
+    }
+
+    public MemoryFileUpload(String name, String filename, String contentType,
+            String contentTransferEncoding, Charset charset, long size, boolean checkBadName) {
+        super(name, charset, size, checkBadName);
         setFilename(filename);
         setContentType(contentType);
         setContentTransferEncoding(contentTransferEncoding);
@@ -132,7 +137,7 @@ public String toString() {
     @Override
     public FileUpload copy() {
         MemoryFileUpload upload = new MemoryFileUpload(getName(), getFilename(), getContentType(),
-                getContentTransferEncoding(), getCharset(), size);
+                getContentTransferEncoding(), getCharset(), size, false);
         ByteBuf buf = data();
         if (buf != null) {
             try {
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/MixedAttribute.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/MixedAttribute.java
index f15a1ba4448..b173508fbf1 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/MixedAttribute.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/MixedAttribute.java
@@ -31,26 +31,34 @@ public class MixedAttribute implements Attribute {
     private final long limitSize;
 
     public MixedAttribute(String name, long limitSize) {
+        this(name, limitSize, false);
+    }
+
+    public MixedAttribute(String name, long limitSize, boolean checkBadName) {
         this.limitSize = limitSize;
-        attribute = new MemoryAttribute(name);
+        attribute = new MemoryAttribute(name, checkBadName);
     }
 
     public MixedAttribute(String name, String value, long limitSize) {
+        this(name, value, limitSize, false);
+    }
+
+    public MixedAttribute(String name, String value, long limitSize, boolean checkBadName) {
         this.limitSize = limitSize;
         if (value.length() > this.limitSize) {
             try {
-                attribute = new DiskAttribute(name, value);
+                attribute = new DiskAttribute(name, value, checkBadName);
             } catch (IOException e) {
                 // revert to Memory mode
                 try {
-                    attribute = new MemoryAttribute(name, value);
+                    attribute = new MemoryAttribute(name, value, checkBadName);
                 } catch (IOException e1) {
                     throw new IllegalArgumentException(e);
                 }
             }
         } else {
             try {
-                attribute = new MemoryAttribute(name, value);
+                attribute = new MemoryAttribute(name, value, checkBadName);
             } catch (IOException e) {
                 throw new IllegalArgumentException(e);
             }
@@ -62,7 +70,7 @@ public void addContent(ByteBuf buffer, boolean last) throws IOException {
         if (attribute instanceof MemoryAttribute) {
             if (attribute.length() + buffer.readableBytes() > limitSize) {
                 DiskAttribute diskAttribute = new DiskAttribute(attribute
-                        .getName());
+                        .getName(), false);
                 if (((MemoryAttribute) attribute).getByteBuf() != null) {
                     diskAttribute.addContent(((MemoryAttribute) attribute)
                         .getByteBuf(), false);
@@ -133,7 +141,7 @@ public void setContent(ByteBuf buffer) throws IOException {
         if (buffer.readableBytes() > limitSize) {
             if (attribute instanceof MemoryAttribute) {
                 // change to Disk
-                attribute = new DiskAttribute(attribute.getName());
+                attribute = new DiskAttribute(attribute.getName(), false);
             }
         }
         attribute.setContent(buffer);
@@ -144,7 +152,7 @@ public void setContent(File file) throws IOException {
         if (file.length() > limitSize) {
             if (attribute instanceof MemoryAttribute) {
                 // change to Disk
-                attribute = new DiskAttribute(attribute.getName());
+                attribute = new DiskAttribute(attribute.getName(), false);
             }
         }
         attribute.setContent(file);
@@ -154,7 +162,7 @@ public void setContent(File file) throws IOException {
     public void setContent(InputStream inputStream) throws IOException {
         if (attribute instanceof MemoryAttribute) {
             // change to Disk even if we don't know the size
-            attribute = new DiskAttribute(attribute.getName());
+            attribute = new DiskAttribute(attribute.getName(), false);
         }
         attribute.setContent(inputStream);
     }
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/MixedFileUpload.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/MixedFileUpload.java
index ee554df5b6e..bbaf58bff2b 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/MixedFileUpload.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/MixedFileUpload.java
@@ -36,13 +36,20 @@ public class MixedFileUpload implements FileUpload {
     public MixedFileUpload(String name, String filename, String contentType,
             String contentTransferEncoding, Charset charset, long size,
             long limitSize) {
+        this(name, filename, contentType, contentTransferEncoding, charset, size,
+           limitSize, false);
+    }
+
+    public MixedFileUpload(String name, String filename, String contentType,
+            String contentTransferEncoding, Charset charset, long size,
+            long limitSize, boolean checkBadName) {
         this.limitSize = limitSize;
         if (size > this.limitSize) {
             fileUpload = new DiskFileUpload(name, filename, contentType,
-                    contentTransferEncoding, charset, size);
+                    contentTransferEncoding, charset, size, checkBadName);
         } else {
             fileUpload = new MemoryFileUpload(name, filename, contentType,
-                    contentTransferEncoding, charset, size);
+                    contentTransferEncoding, charset, size, checkBadName);
         }
         definedSize = size;
     }
@@ -56,7 +63,7 @@ public void addContent(ByteBuf buffer, boolean last)
                         .getName(), fileUpload.getFilename(), fileUpload
                         .getContentType(), fileUpload
                         .getContentTransferEncoding(), fileUpload.getCharset(),
-                        definedSize);
+                        definedSize, false);
                 if (((MemoryFileUpload) fileUpload).getByteBuf() != null) {
                     diskFileUpload.addContent(((MemoryFileUpload) fileUpload)
                         .getByteBuf(), false);
@@ -146,7 +153,7 @@ public void setContent(ByteBuf buffer) throws IOException {
                         .getName(), fileUpload.getFilename(), fileUpload
                         .getContentType(), fileUpload
                         .getContentTransferEncoding(), fileUpload.getCharset(),
-                        definedSize);
+                        definedSize, false);
             }
         }
         fileUpload.setContent(buffer);
@@ -161,7 +168,7 @@ public void setContent(File file) throws IOException {
                         .getName(), fileUpload.getFilename(), fileUpload
                         .getContentType(), fileUpload
                         .getContentTransferEncoding(), fileUpload.getCharset(),
-                        definedSize);
+                        definedSize, false);
             }
         }
         fileUpload.setContent(file);
@@ -175,7 +182,7 @@ public void setContent(InputStream inputStream) throws IOException {
                     .getName(), fileUpload.getFilename(), fileUpload
                     .getContentType(), fileUpload
                     .getContentTransferEncoding(), fileUpload.getCharset(),
-                    definedSize);
+                    definedSize, false);
         }
         fileUpload.setContent(inputStream);
     }


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/multipart/AbstractMemoryHttpDataTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/multipart/AbstractMemoryHttpDataTest.java
index 0334ba612df..82c14bf7c9c 100644
--- a/codec-http/src/test/java/io/netty/handler/codec/http/multipart/AbstractMemoryHttpDataTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/multipart/AbstractMemoryHttpDataTest.java
@@ -69,7 +69,7 @@ private static final class TestHttpData extends AbstractMemoryHttpData {
          * @param size    Expected data block size.
          */
         protected TestHttpData(String name, Charset charset, long size) {
-            super(name, charset, size);
+            super(name, charset, size, true);
         }
 
         @Override