contentDispositionType misparses the Content-Disposition header in some obscure corne...

https://bugs.webkit.org/show_bug.cgi?id=77577

Reviewed by Eric Seidel.

The contentDispositionType extracts the disposition-type from the
Content-Disposition header.  According to RFC 6266 (and previous RFCs),
the disposition-type must be an RFC 2616 token.  Rather than enforce
this general rule, we had special-cased some examples (including
name=foo and filename=bar).  This patch generalizes our check to
properly validate that the disposition-type is an RFC 2616 token.

In conjunction with some other work in the Chromium network stack, this
causes Chromium to pass the following tests:

  http://greenbytes.de/tech/tc2231/#inlonlyquoted
  http://greenbytes.de/tech/tc2231/#attonlyquoted

Without this patch, these test cases neither trigger a navigation nor a
download in Chromium.  This patch does not appear to cause any visible
change in Safari.  (Safari passes these tests both before and after
this patch.)

* platform/network/HTTPParsers.cpp:
(WebCore::isRFC2616Token):
(WebCore::contentDispositionType):
    - This patch also adds a comment to
      filenameFromHTTPContentDisposition, which explains some of the
      was this function incorrectly implements the requirements in
      RFC 6266.  Resolving these issues is a subject for a future
      patch.
* platform/network/HTTPParsers.h:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@106514 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 3a9d499..ee1614a 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,38 @@
+2012-02-01  Adam Barth  <abarth@webkit.org>
+
+        contentDispositionType misparses the Content-Disposition header in some obscure corner cases
+        https://bugs.webkit.org/show_bug.cgi?id=77577
+
+        Reviewed by Eric Seidel.
+
+        The contentDispositionType extracts the disposition-type from the
+        Content-Disposition header.  According to RFC 6266 (and previous RFCs),
+        the disposition-type must be an RFC 2616 token.  Rather than enforce
+        this general rule, we had special-cased some examples (including
+        name=foo and filename=bar).  This patch generalizes our check to
+        properly validate that the disposition-type is an RFC 2616 token.
+
+        In conjunction with some other work in the Chromium network stack, this
+        causes Chromium to pass the following tests:
+
+          http://greenbytes.de/tech/tc2231/#inlonlyquoted
+          http://greenbytes.de/tech/tc2231/#attonlyquoted
+
+        Without this patch, these test cases neither trigger a navigation nor a
+        download in Chromium.  This patch does not appear to cause any visible
+        change in Safari.  (Safari passes these tests both before and after
+        this patch.)
+
+        * platform/network/HTTPParsers.cpp:
+        (WebCore::isRFC2616Token):
+        (WebCore::contentDispositionType):
+            - This patch also adds a comment to
+              filenameFromHTTPContentDisposition, which explains some of the
+              was this function incorrectly implements the requirements in
+              RFC 6266.  Resolving these issues is a subject for a future
+              patch.
+        * platform/network/HTTPParsers.h:
+
 2012-02-01  Dan Bernstein  <mitz@apple.com>
 
         WebCore part of <rdar://problem/10442663> Paginated display does not respect page-break-{before,after}

diff --git a/Source/WebCore/platform/network/HTTPParsers.cpp b/Source/WebCore/platform/network/HTTPParsers.cpp
index 4ef54ef..8cbd4ff 100644 (file)
--- a/Source/WebCore/platform/network/HTTPParsers.cpp
+++ b/Source/WebCore/platform/network/HTTPParsers.cpp
@@ -73,28 +73,45 @@ static inline bool skipToken(const String& str, unsigned& pos, const char* token
     return true;
 }
 
+// See RFC 2616, Section 2.2.
+bool isRFC2616Token(const String& characters)
+{
+    if (characters.isEmpty())
+        return false;
+    for (unsigned i = 0; i < characters.length(); ++i) {
+        UChar c = characters[i];
+        if (c >= 0x80 || c <= 0x1F || c == 0x7F
+            || c == '(' || c == ')' || c == '<' || c == '>' || c == '@'
+            || c == ',' || c == ';' || c == ':' || c == '\\' || c == '"'
+            || c == '/' || c == '[' || c == ']' || c == '?' || c == '='
+            || c == '{' || c == '}' || c == ' ' || c == '\t')
+        return false;
+    }
+    return true;
+}
+
 ContentDispositionType contentDispositionType(const String& contentDisposition)
-{   
+{
     if (contentDisposition.isEmpty())
         return ContentDispositionNone;
 
-    // Some broken sites just send
-    // Content-Disposition: ; filename="file"
-    // screen those out here.
-    if (contentDisposition.startsWith(";"))
-        return ContentDispositionNone;
+    Vector<String> parameters;
+    contentDisposition.split(';', parameters);
 
-    if (contentDisposition.startsWith("inline", false))
+    String dispositionType = parameters[0];
+    dispositionType.stripWhiteSpace();
+
+    if (equalIgnoringCase(dispositionType, "inline"))
         return ContentDispositionInline;
 
-    // Some broken sites just send
-    // Content-Disposition: filename="file"
+    // Some broken sites just send bogus headers like
+    //
+    //   Content-Disposition: ; filename="file"
+    //   Content-Disposition: filename="file"
+    //   Content-Disposition: name="file"
+    //
     // without a disposition token... screen those out.
-    if (contentDisposition.startsWith("filename", false))
-        return ContentDispositionNone;
-
-    // Also in use is Content-Disposition: name="file"
-    if (contentDisposition.startsWith("name", false))
+    if (!isRFC2616Token(dispositionType))
         return ContentDispositionNone;
 
     // We have a content-disposition of "attachment" or unknown.
@@ -167,6 +184,11 @@ double parseDate(const String& value)
     return parseDateFromNullTerminatedCharacters(value.utf8().data());
 }
 
+// FIXME: This function doesn't comply with RFC 6266.
+// For example, this function doesn't handle the interaction between " and ;
+// that arises from quoted-string, nor does this function properly unquote
+// attribute values. Further this function appears to process parameter names
+// in a case-sensitive manner. (There are likely other bugs as well.)
 String filenameFromHTTPContentDisposition(const String& value)
 {
     Vector<String> keyValuePairs;

diff --git a/Source/WebCore/platform/network/HTTPParsers.h b/Source/WebCore/platform/network/HTTPParsers.h
index 55b8c7b..3c808af 100644 (file)
--- a/Source/WebCore/platform/network/HTTPParsers.h
+++ b/Source/WebCore/platform/network/HTTPParsers.h
@@ -50,6 +50,7 @@ typedef enum {
 } ContentDispositionType;
 
 ContentDispositionType contentDispositionType(const String&);
+bool isRFC2616Token(const String&);
 bool parseHTTPRefresh(const String& refresh, bool fromHttpEquivMeta, double& delay, String& url);
 double parseDate(const String&);
 String filenameFromHTTPContentDisposition(const String&);