[chromium] Fix ClipboardChromium::validateFilename to actually operate on extensions
authordcheng@chromium.org <dcheng@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 26 Jan 2012 05:02:12 +0000 (05:02 +0000)
committerdcheng@chromium.org <dcheng@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 26 Jan 2012 05:02:12 +0000 (05:02 +0000)
https://bugs.webkit.org/show_bug.cgi?id=76996

Source/WebCore:

As it turns out, we were always calling validateFilename on a data object with an empty
extension. Now we call it on an actual extension so that it's sanitized.

Reviewed by Tony Chang.

Unit test: webkit_unit_tests --gtest_filter=ClipboardChromium.*

* WebCore.gypi:
* platform/chromium/ClipboardChromium.cpp:
(WebCore::writeImageToDataObject):
* platform/chromium/ClipboardChromium.h:
(ClipboardChromium):
* platform/chromium/ClipboardChromiumLinux.cpp: Removed.
* platform/chromium/ClipboardChromiumPosix.cpp: Renamed from Source/WebCore/platform/chromium/ClipboardChromiumMac.cpp.
(WebCore):
(WebCore::isInvalidFileCharacter):
(WebCore::ClipboardChromium::validateFilename):
* platform/chromium/ClipboardChromiumWin.cpp:
(WebCore):
(WebCore::ClipboardChromium::validateFilename):

Source/WebKit/chromium:

Reviewed by Tony Chang.

* WebKit.gypi:
* tests/ClipboardChromiumTest.cpp: Added.
(WebCore):
(WebCore::TEST):

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

Source/WebCore/ChangeLog
Source/WebCore/WebCore.gypi
Source/WebCore/platform/chromium/ClipboardChromium.cpp
Source/WebCore/platform/chromium/ClipboardChromium.h
Source/WebCore/platform/chromium/ClipboardChromiumLinux.cpp [deleted file]
Source/WebCore/platform/chromium/ClipboardChromiumPosix.cpp [moved from Source/WebCore/platform/chromium/ClipboardChromiumMac.cpp with 58% similarity]
Source/WebCore/platform/chromium/ClipboardChromiumWin.cpp
Source/WebKit/chromium/ChangeLog
Source/WebKit/chromium/WebKit.gypi
Source/WebKit/chromium/tests/ClipboardChromiumTest.cpp [new file with mode: 0644]

index b4d3d6b..f267670 100644 (file)
@@ -1,3 +1,29 @@
+2012-01-25  Daniel Cheng  <dcheng@chromium.org>
+
+        [chromium] Fix ClipboardChromium::validateFilename to actually operate on extensions
+        https://bugs.webkit.org/show_bug.cgi?id=76996
+
+        As it turns out, we were always calling validateFilename on a data object with an empty
+        extension. Now we call it on an actual extension so that it's sanitized.
+
+        Reviewed by Tony Chang.
+
+        Unit test: webkit_unit_tests --gtest_filter=ClipboardChromium.*
+
+        * WebCore.gypi:
+        * platform/chromium/ClipboardChromium.cpp:
+        (WebCore::writeImageToDataObject):
+        * platform/chromium/ClipboardChromium.h:
+        (ClipboardChromium):
+        * platform/chromium/ClipboardChromiumLinux.cpp: Removed.
+        * platform/chromium/ClipboardChromiumPosix.cpp: Renamed from Source/WebCore/platform/chromium/ClipboardChromiumMac.cpp.
+        (WebCore):
+        (WebCore::isInvalidFileCharacter):
+        (WebCore::ClipboardChromium::validateFilename):
+        * platform/chromium/ClipboardChromiumWin.cpp:
+        (WebCore):
+        (WebCore::ClipboardChromium::validateFilename):
+
 2012-01-25  James Robinson  <jamesr@chromium.org>
 
         [chromium] Rollout r100751, this mechanism does not work and is very slow
index 8789fe9..c42ceb2 100644 (file)
             'platform/chromium/ChromiumDataObject.h',
             'platform/chromium/ClipboardChromium.cpp',
             'platform/chromium/ClipboardChromium.h',
-            'platform/chromium/ClipboardChromiumLinux.cpp',
-            'platform/chromium/ClipboardChromiumMac.cpp',
+            'platform/chromium/ClipboardChromiumPosix.cpp',
             'platform/chromium/ClipboardChromiumWin.cpp',
             'platform/chromium/ClipboardMimeTypes.cpp',
             'platform/chromium/ClipboardMimeTypes.h',
index e65bfd3..75f8f63 100644 (file)
@@ -403,13 +403,15 @@ static void writeImageToDataObject(ChromiumDataObject* dataObject, Element* elem
         if (extensionIndex != -1)
             filename.truncate(extensionIndex);
     }
-    filename = ClipboardChromium::validateFileName(filename, dataObject);
 
     String extension = MIMETypeRegistry::getPreferredExtensionForMIMEType(
         cachedImage->response().mimeType());
-    dataObject->setFileExtension(extension.isEmpty() ? emptyString() : "." + extension);
+    extension = extension.isEmpty() ? emptyString() : "." + extension;
 
-    dataObject->setFileContentFilename(filename + dataObject->fileExtension());
+    ClipboardChromium::validateFilename(filename, extension);
+
+    dataObject->setFileContentFilename(filename + extension);
+    dataObject->setFileExtension(extension);
 }
 
 void ClipboardChromium::declareAndWriteDragImage(Element* element, const KURL& url, const String& title, Frame* frame)
index 30e8739..924226d 100644 (file)
@@ -51,11 +51,10 @@ namespace WebCore {
         static PassRefPtr<ClipboardChromium> create(
             ClipboardType, PassRefPtr<ChromiumDataObject>, ClipboardAccessPolicy, Frame*);
 
-        // Returns the file name (not including the extension). This removes any
-        // invalid file system characters as well as making sure the
-        // path + extension is not bigger than allowed by the file system.
-        // This may change the file extension in dataObject.
-        static String validateFileName(const String& title, ChromiumDataObject* dataObject);
+        // Validates a filename (without the extension) and the extension. This removes any invalid
+        // file system characters as well as making sure the path + extension is not bigger than
+        // allowed by the file system.
+        static void validateFilename(String& name, String& extension);
 
         virtual void clearData(const String& type);
         void clearAllData();
diff --git a/Source/WebCore/platform/chromium/ClipboardChromiumLinux.cpp b/Source/WebCore/platform/chromium/ClipboardChromiumLinux.cpp
deleted file mode 100644 (file)
index 2c89f6e..0000000
+++ /dev/null
@@ -1,41 +0,0 @@
-/*
- * Copyright (C) 2009 Apple Inc.  All rights reserved.
- * Copyright (C) 2009 Google Inc.
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
- * 1. Redistributions of source code must retain the above copyright
- *    notice, this list of conditions and the following disclaimer.
- * 2. Redistributions in binary form must reproduce the above copyright
- *    notice, this list of conditions and the following disclaimer in the
- *    documentation and/or other materials provided with the distribution.
- *
- * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY
- * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
- * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
- * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE COMPUTER, INC. OR
- * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
- * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
- * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
- * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
- * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
- * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
- * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- */
-
-#include "config.h"
-#include "ClipboardChromium.h"
-
-#include "ChromiumDataObject.h"
-#include "NotImplemented.h"
-
-namespace WebCore {
-
-String ClipboardChromium::validateFileName(const String& title, ChromiumDataObject* dataObject)
-{
-    notImplemented();
-    return title;
-}
-
-}  // namespace WebCore
 #include "config.h"
 #include "ClipboardChromium.h"
 
-#include "ChromiumDataObject.h"
-
 namespace WebCore {
-    
-
-// Filename length and character-set limits vary by filesystem, and there's no
-// real way to tell what filesystem is in use. Since HFS+ is by far the most
-// common, we'll abide by its limits, which are 255 Unicode characters (slightly
-// simplified; really it uses Normalization Form D, but the differences aren't
-// really worth dealing with here.)
-static const unsigned MaxHFSFilenameLength = 255;
 
+// On POSIX systems, the typical filename length limit is 255 character units. HFS+'s limit is
+// actually 255 Unicode characters using Apple's modification of Normzliation Form D, but the
+// differences aren't really worth dealing with here.
+static const unsigned maxFilenameLength = 255;
 
 static bool isInvalidFileCharacter(UChar c)
 {
-    // HFS+ basically allows anything but '/'. For sanity's sake we'll disallow
-    // control characters.
+    // HFS+ disallows '/' and Linux systems also disallow null. For sanity's sake we'll also
+    // disallow control characters.
     return c < ' ' || c == 0x7F || c == '/';
 }
 
-String ClipboardChromium::validateFileName(const String& title, ChromiumDataObject* dataObject)
+void ClipboardChromium::validateFilename(String& name, String& extension)
 {
     // Remove any invalid file system characters, especially "/".
-    String result = title.removeCharacters(isInvalidFileCharacter);
-    String extension = dataObject->fileExtension().removeCharacters(&isInvalidFileCharacter);
+    name = name.removeCharacters(&isInvalidFileCharacter);
+    extension = extension.removeCharacters(&isInvalidFileCharacter);
 
     // Remove a ridiculously-long extension.
-    if (extension.length() >= MaxHFSFilenameLength)
-        extension = "";
-    
-    // Truncate an overly-long filename.
-    int overflow = result.length() + extension.length() - MaxHFSFilenameLength;
-    if (overflow > 0) 
-        result.remove(result.length() - overflow, overflow);
-    
-    dataObject->setFileExtension(extension);
-    return result;
+    if (extension.length() >= maxFilenameLength)
+        extension = String();
+
+    // Truncate an overly-long filename, reserving one character for a dot.
+    name.truncate(maxFilenameLength - extension.length() - 1);
 }
 
-}  // namespace WebCore
+} // namespace WebCore
index d9bbeb5..39b5806 100644 (file)
 #include "config.h"
 #include "ClipboardChromium.h"
 
-#include "ChromiumDataObject.h"
-
 #include <shlwapi.h>
 
 namespace WebCore {
 
+// FAT32 and NTFS both limit filenames to a maximum of 255 characters.
+static const unsigned maxFilenameLength = 255;
+
 // Returns true if the specified character is not valid in a file name. This
 // is intended for use with removeCharacters.
 static bool isInvalidFileCharacter(UChar c)
@@ -40,17 +41,17 @@ static bool isInvalidFileCharacter(UChar c)
     return (PathGetCharType(c) & (GCT_LFNCHAR | GCT_SHORTCHAR)) == 0;
 }
 
-String ClipboardChromium::validateFileName(const String& title, ChromiumDataObject* dataObject)
+void ClipboardChromium::validateFilename(String& name, String& extension)
 {
     // Remove any invalid file system characters.
-    String result = title.removeCharacters(&isInvalidFileCharacter);
-    if (result.length() + dataObject->fileExtension().length() + 1 >= MAX_PATH) {
-        if (dataObject->fileExtension().length() + 1 >= MAX_PATH)
-            dataObject->setFileExtension("");
-        if (result.length() + dataObject->fileExtension().length() + 1 >= MAX_PATH)
-            result = result.substring(0, MAX_PATH - dataObject->fileExtension().length() - 1);
-    }
-    return result;
+    name = name.removeCharacters(&isInvalidFileCharacter);
+    extension = extension.removeCharacters(&isInvalidFileCharacter);
+
+    if (extension.length() >= maxFilenameLength)
+        extension = String();
+
+    // Truncate overly-long filenames, reserving one character for a dot.
+    name.truncate(maxFilenameLength - extension.length() - 1);
 }
 
 }  // namespace WebCore
index e7639b3..e4ec053 100644 (file)
@@ -1,3 +1,15 @@
+2012-01-25  Daniel Cheng  <dcheng@chromium.org>
+
+        [chromium] Fix ClipboardChromium::validateFilename to actually operate on extensions
+        https://bugs.webkit.org/show_bug.cgi?id=76996
+
+        Reviewed by Tony Chang.
+
+        * WebKit.gypi:
+        * tests/ClipboardChromiumTest.cpp: Added.
+        (WebCore):
+        (WebCore::TEST):
+
 2012-01-25  James Robinson  <jamesr@chromium.org>
 
         [chromium] Rollout r100751, this mechanism does not work and is very slow
index 7025990..84ecfb8 100644 (file)
@@ -84,6 +84,7 @@
             'tests/CCTiledLayerImplTest.cpp',
             'tests/CCThreadTaskTest.cpp',
             'tests/CCTimerTest.cpp',
+            'tests/ClipboardChromiumTest.cpp',
             'tests/CompositorFakeGraphicsContext3D.h',
             'tests/CompositorFakeWebGraphicsContext3D.h',
             'tests/FakeCCLayerTreeHostClient.h',
diff --git a/Source/WebKit/chromium/tests/ClipboardChromiumTest.cpp b/Source/WebKit/chromium/tests/ClipboardChromiumTest.cpp
new file mode 100644 (file)
index 0000000..24c11a4
--- /dev/null
@@ -0,0 +1,89 @@
+/*
+ * Copyright (C) 2010 Google Inc. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above
+ * copyright notice, this list of conditions and the following disclaimer
+ * in the documentation and/or other materials provided with the
+ * distribution.
+ *     * Neither the name of Google Inc. nor the names of its
+ * contributors may be used to endorse or promote products derived from
+ * this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include "config.h"
+
+#include "ClipboardChromium.h"
+
+#include <gtest/gtest.h>
+
+using namespace WebCore;
+
+namespace {
+
+#if OS(WINDOWS)
+const char invalidCharacters[] = "\x00/\\:*?\"<>|";
+#else
+const char invalidCharacters[] =
+    "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f"
+    "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f"
+    "\x7f/";
+#endif
+const char longString[] =
+    "0,1,1,2,3,5,8,13,21,34,55,89,144,233,377,610,987,1597,2584,4181,6765,10946,17711,28657,46368,"
+    "75025,121393,196418,317811,514229,832040,1346269,2178309,3524578,5702887,9227465,14930352";
+
+TEST(ClipboardChromiumTest, Normal)
+{
+    String name = "name";
+    String extension = "ext";
+    ClipboardChromium::validateFilename(name, extension);
+    EXPECT_EQ("name", name);
+    EXPECT_EQ("ext", extension);
+}
+
+TEST(ClipboardChromiumTest, InvalidCharacters)
+{
+    String name = makeString("na", String(invalidCharacters, arraysize(invalidCharacters)), "me");
+    String extension = makeString("e", String(invalidCharacters, arraysize(invalidCharacters)), "xt");
+    ClipboardChromium::validateFilename(name, extension);
+    EXPECT_EQ("name", name);
+    EXPECT_EQ("ext", extension);
+}
+
+TEST(ClipboardChromiumTest, ExtensionTooLong)
+{
+    String name;
+    String extension = makeString(longString, longString);
+    ClipboardChromium::validateFilename(name, extension);
+    EXPECT_EQ(String(), extension);
+}
+
+TEST(ClipboardChromiumTest, NamePlusExtensionTooLong)
+{
+    String name = makeString(longString, longString);
+    String extension = longString;
+    ClipboardChromium::validateFilename(name, extension);
+    EXPECT_EQ("0,1,1,2,3,5,8,13,21,34,55,89,144,233,377,610,987,1597,2584,4181,6765,109", name);
+    EXPECT_EQ(longString, extension);
+    EXPECT_EQ(254u, name.length() + extension.length());
+}
+
+} // anonymous namespace