Part 1.1: Is policy list subsumed under subsuming policy?

third_party/WebKit/Source/core/frame/csp/CSPSource.cpp

Index: third_party/WebKit/Source/core/frame/csp/CSPSource.cpp
diff --git a/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp b/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp
index 7b6be14df07012db9a466cba5bf8d1f89d9e452f..403bb5b6ac1ce96d44b1e477c9da7d8ed477af1f 100644
--- a/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp
+++ b/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp
@@ -105,10 +105,78 @@ bool CSPSource::portMatches(int port, const String& protocol) const {
   return false;
 }
 
+bool CSPSource::isSimilar(CSPSource* other) {
+  bool schemesMatch =
+      schemeMatches(other->m_scheme) || other->schemeMatches(m_scheme);
+  if (!schemesMatch || isSchemeOnly() || other->isSchemeOnly())
+    return schemesMatch;
+  bool hostsMatch = (m_host == other->m_host) || hostMatches(other->m_host) ||
+                    other->hostMatches(m_host);
+  bool portsMatch = (other->m_portWildcard == HasWildcard) ||
+                    portMatches(other->m_port, other->m_scheme);

[jochen (gone - plz use gerrit), 2016/11/02 11:11:32]

why not also other->portMatches(m_port, m_scheme)?

[amalika, 2016/11/02 12:31:41]

Two parts A and B match if either:
1. one or both ports is/are 0 -> we need to determine default ports for the
given protocols.
2. Both are non-zero, in which case they must be just equal.

since ports are integers.

In the first case portMatches does assume that supplied port (in this case,
other->m_port) might be empty so it would call defaultPortForProtocol.
Otherwise, if they are equal, there is no need to call portMatches second time.
In particular, we can either call portMatches(other->m_port, other->m_scheme) or
other->portMatches(m_port, m_scheme)


Situation with, for example, paths is a bit more complex because for similarity
purposes we want to get the same result whether we call other->isSimilar(this)
or this->isSimilar(other). But pathMatches would only match "/" with
"/path1.html" but not "/path1.html" with "/". So we have to call pathMatches
twice. 

+  bool pathsMatch = pathMatches(other->m_path) || other->pathMatches(m_path);
+  if (hostsMatch && portsMatch && pathsMatch)
+    return true;
+
+  return false;
+}
+
+bool CSPSource::isSubsumedBy(CSPSource* other) {
+  if (!isSimilar(other) || !isSchemeSubsumedBy(other) ||
+      !isWildcardsSubsumedBy(other) || !isPortSubsumedBy(other) ||
+      !isPathSubsumedBy(other))

[jochen (gone - plz use gerrit), 2016/11/02 11:11:33]

add { } around if body

+    return false;
+
+  return true;
+}
+
+bool CSPSource::isWildcardsSubsumedBy(CSPSource* other) {
+  if ((m_hostWildcard == HasWildcard && other->m_hostWildcard == NoWildcard) ||
+      (m_portWildcard == HasWildcard && other->m_portWildcard == NoWildcard)) {
+    return false;
+  }
+  return true;

[jochen (gone - plz use gerrit), 2016/11/02 11:11:33]

isn't that the same as

return m_hostWildcard == other->m_hostWildcard && m_portWildcard ==
other->m_portWildcard?

[amalika, 2016/11/02 12:31:41]

It would not hold for example, when m_hostWildcard == NoWildcard but
other->m_hostWildcard == HasWildcard.
However, in this case we want to return `true` because `this` is more
restrictive than `other`.

+}
+
+bool CSPSource::isSchemeSubsumedBy(CSPSource* other) {
+  if (other->isSchemeOnly()) {
+    if (other->m_scheme.length() == m_scheme.length())
+      return true;
+    return m_scheme.length() == 3 || m_scheme.length() == 5 ? true : false;

[jochen (gone - plz use gerrit), 2016/11/02 11:11:32]

you really want something like isSchemeSecure(m_scheme) here, no?

[amalika, 2016/11/02 12:31:41]

Yes! 
But I could not find it in the codebase or did you mean for me to create a new
such method?

It is just I wanted to save up on string comparisons as those are more
expensive, especially since we call to isSimilar prior to this.

So I wanted to assume that if the schemes match then it must be one of two
cases:

1. Schemes are completely equal -> their lengths are thus identical.
2. There is one (and only one) more secure scheme ( for example, "https" and
"http"). In this case, we know lengths are not equal

isSimilar already checks if schemes match. So I was thinking of using lengths
for efficiency since we know that if the lengths do not match, it must be the
second case. 

Would it be better to be more explicit and use string comparisons? 

+  }
+  if (isSchemeOnly())
+    return false;
+
+  if (m_scheme.length() == other->m_scheme.length())
+    return true;
+
+  // If the schemes match but their lengths are not equal, that means one of the
+  // schemes is 'https' or 'wss' and the other one is 'http' or 'ws'.
+  return m_scheme.length() > 3 ? (m_scheme == "https") : (m_scheme == "wss");
+}
+
+bool CSPSource::isPortSubsumedBy(CSPSource* other) {
+  bool otherIsMoreRestrictive =
+      (other->m_portWildcard == NoWildcard) && (!m_port && other->m_port);
+  return !otherIsMoreRestrictive;
+}
+
+bool CSPSource::isPathSubsumedBy(CSPSource* other) {
+  bool otherIsMoreRestrictive =
+      (isPathEmptyOrSlashOnly() && !other->isPathEmptyOrSlashOnly()) ||
+      (!isPathEmptyOrSlashOnly() && m_path.endsWith("/") &&
+       !other->m_path.endsWith("/"));
+  return !otherIsMoreRestrictive;
+}
+
 bool CSPSource::isSchemeOnly() const {
   return m_host.isEmpty();
 }
 
+bool CSPSource::isPathEmptyOrSlashOnly() const {
+  return m_path.isEmpty() || m_path == "/";
+}
+
 DEFINE_TRACE(CSPSource) {
   visitor->trace(m_policy);
 }