Bug 128593 – Tighten XMLHttpRequest setRequestHeader value check

• Component: XML
• Status: RESOLVED FIXED

[youenn fablet]

XMLHttpRequest::setRequestHeader currently validates header names and values.
This validation could be tighten for header values as illustrated by http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/setrequestheader-bogus-value.htm
According RFC2616, header values should be encoded as ISO-8859-1 and should not contain any control character except whitespaces (CR, LF, SPACE and TAB)

[youenn fablet]

Created attachment 223839 [details]
Patch

[Darin Adler]

Comment on attachment 223839 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=223839&action=review

> Source/WebCore/platform/network/HTTPParsers.cpp:105
> +bool isValidHTTPHeaderValue(const String& characters)

I don’t think “characters” is a good name for a string. Maybe “string” or “value”. Maybe make this take a StringView instead of a const String&.

> Source/WebCore/platform/network/HTTPParsers.cpp:116
> +    // FIXME: Check all constraints in one loop
> +    if (!characters.containsOnlyLatin1() || characters.contains('\r') || characters.contains('\n'))
> +        return false;
> +    // FIXME: Add validation of encoding rules defined by RFC 2047.
> +    for (unsigned i = 0; i < characters.length(); ++i) {
> +        UChar c = characters[i];
> +        // Check that c is not a control character except if whitespace (reduced to 0x09 in that context)
> +        if (c <= 0x1F && c != 0x09)
> +            return false;
> +    }

This seems like a strange way to write this. Why have four different loops here? I would write:

    unsigned length = value.length();
    for (unsigned i = 0; i < length; ++i) {
        UChar character = value[i];
        if (character != '\t' && (character < 0x20 || character > 0x7F))
            return false;
    }
    return true;

If you took a string view it could be:

    for (UChar character : value.codeunits()) {
        if (character != '\t' && (character < 0x20 || character > 0x7F))
            return false;
    }

No need for containsOnlyLatin1, or a special check for \r or \n.

[youenn fablet]

(In reply to comment #2)
> (From update of attachment 223839 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223839&action=review
> 
> > Source/WebCore/platform/network/HTTPParsers.cpp:105
> > +bool isValidHTTPHeaderValue(const String& characters)
> 
> I don’t think “characters” is a good name for a string. Maybe “string” or “value”. Maybe make this take a StringView instead of a const String&.
> 
> > Source/WebCore/platform/network/HTTPParsers.cpp:116
> > +    // FIXME: Check all constraints in one loop
> > +    if (!characters.containsOnlyLatin1() || characters.contains('\r') || characters.contains('\n'))
> > +        return false;
> > +    // FIXME: Add validation of encoding rules defined by RFC 2047.
> > +    for (unsigned i = 0; i < characters.length(); ++i) {
> > +        UChar c = characters[i];
> > +        // Check that c is not a control character except if whitespace (reduced to 0x09 in that context)
> > +        if (c <= 0x1F && c != 0x09)
> > +            return false;
> > +    }
> 
> This seems like a strange way to write this. Why have four different loops here? I would write:
> 
>     unsigned length = value.length();
>     for (unsigned i = 0; i < length; ++i) {
>         UChar character = value[i];
>         if (character != '\t' && (character < 0x20 || character > 0x7F))
>             return false;
>     }
>     return true;
> 
> If you took a string view it could be:
> 
>     for (UChar character : value.codeunits()) {
>         if (character != '\t' && (character < 0x20 || character > 0x7F))
>             return false;
>     }
> 
> No need for containsOnlyLatin1, or a special check for \r or \n.

Thanks for the review.
IIRC, I wrote it this way to easily align with the spec rules.
Reading it again, I agree with your suggestion.
Also I just saw RFC2616 was recently obsolete by RFC7230, which may have an impact on this patch.

[youenn fablet]

Created attachment 239942 [details]
Updated according comments and RFC7230

[Darin Adler]

Comment on attachment 239942 [details]
Updated according comments and RFC7230

View in context: https://bugs.webkit.org/attachment.cgi?id=239942&action=review

Looks OK, but there is a bug here and the test case has insufficient coverage.

> Source/WebCore/platform/network/HTTPParsers.cpp:111
> +    c = value[value.length()];

This will always return 0, since it’s trying to fetch a character past the end of the string.

> LayoutTests/http/tests/xmlhttprequest/set-bad-headervalue.html:24
> +      try_value("t\rt", true)

Coverage of this test is insufficient. I don’t see any cases checking the rules about the first and last characters not being spaces or tabs. If we had cases checking for that we would have noticed the bug I mentioned above.

[youenn fablet]

Created attachment 240150 [details]
fixing bug and improving test coverage

[WebKit Commit Bot]

Comment on attachment 240150 [details]
fixing bug and improving test coverage

Clearing flags on attachment: 240150

Committed r174920: <http://trac.webkit.org/changeset/174920>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Mitchell]

I can see that the changes for this have tightened the header value validation more than what RFC 7230 suggests. If you set the value to a single SPACE character it is reported as an invalid header value. RFC 7320 does not suggest this check should be done. You can check this by adding try_value(" ") to the test harness. You will see that this test passes (i.e. it throws an exception) but accordinging to RFC 7320 it shouldn't since a single SPACE character is a valid value.

I am reporting this since we utilites the ablility to set the "Content-Type" to a space character and these changes have broken our site.

Should I raise a new bug?

[youenn fablet]

(In reply to comment #9)
> I can see that the changes for this have tightened the header value
> validation more than what RFC 7230 suggests. If you set the value to a
> single SPACE character it is reported as an invalid header value. RFC 7320
> does not suggest this check should be done. You can check this by adding
> try_value(" ") to the test harness. You will see that this test passes (i.e.
> it throws an exception) but accordinging to RFC 7320 it shouldn't since a
> single SPACE character is a valid value.
> 
> I am reporting this since we utilites the ablility to set the "Content-Type"
> to a space character and these changes have broken our site.
> 
> Should I raise a new bug?

Hi.
Yes, please file a new bug.
Can you CC me, as well as ap and darin?

Out of curiosity, can you explain why setting Content-Type to " "?

[Chris Mitchell]

Hi,

We are uploading files to a service which requires the content-type to be set. Any value will work. We are not 100% of their MimeType so we set it to a space character and the system we are uploading to doesn't seem to care about the validity but does allow the upload to happen.

I will file the bug and CC those you mentioned.

Thanks

[youenn fablet]

(In reply to comment #11)
> Hi,
> 
> We are uploading files to a service which requires the content-type to be
> set. Any value will work. We are not 100% of their MimeType so we set it to
> a space character and the system we are uploading to doesn't seem to care
> about the validity but does allow the upload to happen.
> 
> I will file the bug and CC those you mentioned.
> 
> Thanks

Thanks for filing bug 147445