Title: [100691] trunk/Source/WebCore
Revision
100691
Author
aba...@webkit.org
Date
2011-11-17 15:33:25 -0800 (Thu, 17 Nov 2011)

Log Message

Refactor SecurityOrigin::create to be easier to understand
https://bugs.webkit.org/show_bug.cgi?id=72342

Reviewed by Eric Seidel.

Over time, the SecurityOrigin constructor has grown a bit out of
control.  This patch attempts to separate the different concerns into
free functions.  The general approach is to put more logic in the
"create" function and introduce a simple constructor for unique
origins.

This patch shouldn't change any behavior.

* page/SecurityOrigin.cpp:
(WebCore::schemeRequiresAuthority):
(WebCore::shouldUseInnerURL):
(WebCore::extractInnerURL):
(WebCore::isDirectory):
(WebCore::shouldTreatAsUniqueOrigin):
(WebCore::SecurityOrigin::SecurityOrigin):
(WebCore::SecurityOrigin::create):
(WebCore::SecurityOrigin::createUnique):
(WebCore::SecurityOrigin::databaseIdentifier):
* page/SecurityOrigin.h:

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (100690 => 100691)


--- trunk/Source/WebCore/ChangeLog	2011-11-17 23:27:57 UTC (rev 100690)
+++ trunk/Source/WebCore/ChangeLog	2011-11-17 23:33:25 UTC (rev 100691)
@@ -1,3 +1,30 @@
+2011-11-17  Adam Barth  <aba...@webkit.org>
+
+        Refactor SecurityOrigin::create to be easier to understand
+        https://bugs.webkit.org/show_bug.cgi?id=72342
+
+        Reviewed by Eric Seidel.
+
+        Over time, the SecurityOrigin constructor has grown a bit out of
+        control.  This patch attempts to separate the different concerns into
+        free functions.  The general approach is to put more logic in the
+        "create" function and introduce a simple constructor for unique
+        origins.
+
+        This patch shouldn't change any behavior.
+
+        * page/SecurityOrigin.cpp:
+        (WebCore::schemeRequiresAuthority):
+        (WebCore::shouldUseInnerURL):
+        (WebCore::extractInnerURL):
+        (WebCore::isDirectory):
+        (WebCore::shouldTreatAsUniqueOrigin):
+        (WebCore::SecurityOrigin::SecurityOrigin):
+        (WebCore::SecurityOrigin::create):
+        (WebCore::SecurityOrigin::createUnique):
+        (WebCore::SecurityOrigin::databaseIdentifier):
+        * page/SecurityOrigin.h:
+
 2011-11-17  Shawn Singh  <shawnsi...@chromium.org>
 
         [chromium] Fix minor style nit in CCLayerImpl

Modified: trunk/Source/WebCore/page/SecurityOrigin.cpp (100690 => 100691)


--- trunk/Source/WebCore/page/SecurityOrigin.cpp	2011-11-17 23:27:57 UTC (rev 100690)
+++ trunk/Source/WebCore/page/SecurityOrigin.cpp	2011-11-17 23:33:25 UTC (rev 100691)
@@ -44,90 +44,128 @@
 const int InvalidPort = 0;
 const int MaxAllowedPort = 65535;
 
-static bool schemeRequiresAuthority(const String& scheme)
+static bool schemeRequiresAuthority(const KURL& url)
 {
-    DEFINE_STATIC_LOCAL(URLSchemesMap, schemes, ());
+    // We expect URLs with these schemes to have authority components. If the
+    // URL lacks an authority component, we get concerned and mark the origin
+    // as unique.
+    return url.protocolIsInHTTPFamily() || url.protocolIs("ftp");
+}
 
-    if (schemes.isEmpty()) {
-        schemes.add("http");
-        schemes.add("https");
-        schemes.add("ftp");
+// Some URL schemes use nested URLs for their security context. For example,
+// filesystem URLs look like the following:
+//
+//   filesystem:http://example.com/temporary/path/to/file.png
+//
+// We're supposed to use "http://example.com" as the origin.
+//
+// Generally, we add URL schemes to this list when WebKit support them. For
+// example, we don't include the "jar" scheme, even though Firefox understands
+// that jar uses an inner URL for it's security origin.
+//
+static bool shouldUseInnerURL(const KURL& url)
+{
+#if ENABLE(BLOB)
+    if (url.protocolIs("blob"))
+        return true;
+#endif
+#if ENABLE(FILE_SYSTEM)
+    if (url.protocolIs("filesystem"))
+        return true;
+#endif
+    return false;
+}
+
+// In general, extracting the inner URL varies by scheme. It just so happens
+// that all the URL schemes we currently support that use inner URLs for their
+// security origin can be parsed using this algorithm.
+static KURL extractInnerURL(const KURL& url)
+{
+    // FIXME: Update this callsite to use the innerURL member function when
+    // we finish implementing it.
+    return KURL(ParsedURLString, decodeURLEscapeSequences(url.path()));
+}
+
+static bool isDirectory(const String& path)
+{
+    return path.endsWith("/");
+}
+
+static bool shouldTreatAsUniqueOrigin(const KURL& url)
+{
+    if (!url.isValid())
+        return true;
+
+    // FIXME: Do we need to unwrap the URL further?
+    KURL innerURL = shouldUseInnerURL(url) ? extractInnerURL(url) : url;
+
+    // FIXME: Check whether innerURL is valid.
+
+    // For edge case URLs that were probably misparsed, make sure that the origin is unique.
+    // FIXME: Do we really need to do this? This looks to be a hack around a
+    // security bug in CFNetwork that might have been fixed.
+    if (schemeRequiresAuthority(innerURL) && innerURL.host().isEmpty())
+        return true;
+
+    // SchemeRegistry needs a lower case protocol because it uses HashMaps
+    // that assume the scheme has already been canonicalized.
+    String protocol = innerURL.protocol().lower();
+
+    if (SchemeRegistry::shouldTreatURLSchemeAsNoAccess(protocol))
+        return true;
+
+    // We use unique origins for directory listings to make it harder to crawl
+    // a local filesystem. Notice that we apply this protection only when we
+    // use the outer URL for the security context because schemes that wrap
+    // other URLs don't have directory listings.
+    if (SchemeRegistry::shouldTreatURLSchemeAsLocal(protocol) && !shouldUseInnerURL(url)) {
+        if (!innerURL.hasPath() || isDirectory(innerURL.path()))
+            return true;
     }
 
-    return schemes.contains(scheme);
+    // This is the common case.
+    return false;
 }
 
-SecurityOrigin::SecurityOrigin(const KURL& url, bool forceUnique)
+SecurityOrigin::SecurityOrigin(const KURL& url)
     : m_protocol(url.protocol().isNull() ? "" : url.protocol().lower())
     , m_host(url.host().isNull() ? "" : url.host().lower())
     , m_port(url.port())
-    , m_isUnique(forceUnique || SchemeRegistry::shouldTreatURLSchemeAsNoAccess(m_protocol))
+    , m_isUnique(false)
     , m_universalAccess(false)
     , m_domainWasSetInDOM(false)
     , m_enforceFilePathSeparation(false)
-    , m_needsStorageIdentifierQuirkForFiles(false)
+    , m_needsDatabaseIdentifierQuirkForFiles(false)
 {
-#if ENABLE(BLOB) || ENABLE(FILE_SYSTEM)
-    bool isBlobOrFileSystemProtocol = false;
-#if ENABLE(BLOB)
-    if (m_protocol == BlobURL::blobProtocol())
-        isBlobOrFileSystemProtocol = true;
-#endif
-#if ENABLE(FILE_SYSTEM)
-    if (m_protocol == "filesystem")
-        isBlobOrFileSystemProtocol = true;
-#endif
-    if (isBlobOrFileSystemProtocol) {
-        KURL originURL(ParsedURLString, decodeURLEscapeSequences(url.path()));
-        if (originURL.isValid()) {
-            m_protocol = originURL.protocol().lower();
-            m_host = originURL.host().lower();
-            m_port = originURL.port();
-        } else
-            m_isUnique = true;
-    }
-#endif
+    ASSERT(url.isValid());
 
-    // For edge case URLs that were probably misparsed, make sure that the origin is unique.
-    if (schemeRequiresAuthority(m_protocol) && m_host.isEmpty())
-        m_isUnique = true;
-
-    if (m_protocol.isEmpty())
-        m_isUnique = true;
-
     // document.domain starts as m_host, but can be set by the DOM.
     m_domain = m_host;
 
-    // By default, only local SecurityOrigins can load local resources.
-    m_canLoadLocalResources = isLocal();
-    if (m_canLoadLocalResources) {
-        // Directories should never be readable.
-        // Note that we do not do this check for blob or filesystem url because its origin is file:/// when it is created from local file urls.
-#if ENABLE(BLOB) || ENABLE(FILE_SYSTEM)
-        bool doDirectoryCheck = !isBlobOrFileSystemProtocol;
-#else
-        bool doDirectoryCheck = true;
-#endif
-        if (doDirectoryCheck && (!url.hasPath() || url.path().endsWith("/")))
-            m_isUnique = true;
-        // Store the path in case we are doing per-file origin checking.
-        m_filePath = url.path();
-    }
-
     if (isDefaultPortForProtocol(m_port, m_protocol))
         m_port = InvalidPort;
 
-    if (m_protocol == "file")
-        m_needsStorageIdentifierQuirkForFiles = true;
+    // By default, only local SecurityOrigins can load local resources.
+    m_canLoadLocalResources = isLocal();
 
-    // Don't leak details from URLs into unique origins.
-    if (m_isUnique) {
-        m_protocol = "";
-        m_host = "";
-        m_port = InvalidPort;
-    }
+    if (m_canLoadLocalResources)
+        m_filePath = url.path(); // In case enforceFilePathSeparation() is called.
 }
 
+SecurityOrigin::SecurityOrigin()
+    : m_protocol("")
+    , m_host("")
+    , m_domain("")
+    , m_port(InvalidPort)
+    , m_isUnique(true)
+    , m_universalAccess(false)
+    , m_domainWasSetInDOM(false)
+    , m_canLoadLocalResources(false)
+    , m_enforceFilePathSeparation(false)
+    , m_needsDatabaseIdentifierQuirkForFiles(false)
+{
+}
+
 SecurityOrigin::SecurityOrigin(const SecurityOrigin* other)
     : m_protocol(other->m_protocol.isolatedCopy())
     , m_host(other->m_host.isolatedCopy())
@@ -140,20 +178,35 @@
     , m_domainWasSetInDOM(other->m_domainWasSetInDOM)
     , m_canLoadLocalResources(other->m_canLoadLocalResources)
     , m_enforceFilePathSeparation(other->m_enforceFilePathSeparation)
-    , m_needsStorageIdentifierQuirkForFiles(other->m_needsStorageIdentifierQuirkForFiles)
+    , m_needsDatabaseIdentifierQuirkForFiles(other->m_needsDatabaseIdentifierQuirkForFiles)
 {
 }
 
 PassRefPtr<SecurityOrigin> SecurityOrigin::create(const KURL& url, bool forceUnique)
 {
-    if (!url.isValid())
-        return adoptRef(new SecurityOrigin(blankURL(), forceUnique));
-    return adoptRef(new SecurityOrigin(url, forceUnique));
+    if (forceUnique || shouldTreatAsUniqueOrigin(url)) {
+        RefPtr<SecurityOrigin> origin = adoptRef(new SecurityOrigin());
+
+        if (url.protocolIs("file")) {
+            // Unfortunately, we can't represent all unique origins exactly
+            // the same way because we need to produce a quirky database
+            // identifier for file URLs due to persistent storage in some
+            // embedders of WebKit.
+            origin->m_needsDatabaseIdentifierQuirkForFiles = true;
+        }
+
+        return origin.release();
+    }
+
+    if (shouldUseInnerURL(url))
+        return adoptRef(new SecurityOrigin(extractInnerURL(url)));
+
+    return adoptRef(new SecurityOrigin(url));
 }
 
 PassRefPtr<SecurityOrigin> SecurityOrigin::createUnique()
 {
-    RefPtr<SecurityOrigin> origin = create(KURL());
+    RefPtr<SecurityOrigin> origin = adoptRef(new SecurityOrigin());
     ASSERT(origin->isUnique());
     return origin.release();
 }
@@ -424,7 +477,7 @@
     // string because of a bug in how we handled the scheme for file URLs.
     // Now that we've fixed that bug, we still need to produce this string
     // to avoid breaking existing persistent state.
-    if (m_needsStorageIdentifierQuirkForFiles)
+    if (m_needsDatabaseIdentifierQuirkForFiles)
         return "file__0";
 
     String separatorString(&SeparatorCharacter, 1);

Modified: trunk/Source/WebCore/page/SecurityOrigin.h (100690 => 100691)


--- trunk/Source/WebCore/page/SecurityOrigin.h	2011-11-17 23:27:57 UTC (rev 100690)
+++ trunk/Source/WebCore/page/SecurityOrigin.h	2011-11-17 23:33:25 UTC (rev 100691)
@@ -165,7 +165,8 @@
     bool isSameSchemeHostPort(const SecurityOrigin*) const;
 
 private:
-    explicit SecurityOrigin(const KURL&, bool forceUnique);
+    SecurityOrigin();
+    explicit SecurityOrigin(const KURL&);
     explicit SecurityOrigin(const SecurityOrigin*);
 
     // FIXME: Rename this function to something more semantic.
@@ -182,7 +183,7 @@
     bool m_domainWasSetInDOM;
     bool m_canLoadLocalResources;
     bool m_enforceFilePathSeparation;
-    bool m_needsStorageIdentifierQuirkForFiles;
+    bool m_needsDatabaseIdentifierQuirkForFiles;
 };
 
 } // namespace WebCore
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-changes

Reply via email to