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