Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2442513004:
Part 1.1: Is policy list subsumed under subsuming policy? (Closed)
DescriptionPart 1.1: Is policy list subsumed under subsuming policy?
This is part of an experimental feature Embedding-CSP.
In particular this is the initial effort in implementing
*3.3. Is policy list subsumed under subsuming policy?
Given two CSPSources, we try to determine whether one is
subsumed under another. If not, return false.
BUG=647588
Committed: https://crrev.com/ab18fdcd7bad1f68ec6bc0d2ebdb67fd5f200df0
Cr-Commit-Position: refs/heads/master@{#429872}
Patch Set 1 : Addressing comments #
Total comments: 6
Patch Set 2 : Separating functionalities #
Total comments: 10
Patch Set 3 : More tests, separating methods #
Total comments: 1
Patch Set 4 : CSPSource subsumption #
Total comments: 10
Patch Set 5 : Addressing commnets #Patch Set 6 : removing unnecessary checks in isSchemeSubsumedBY #Patch Set 7 : Getting rid of similarity #
Total comments: 2
Patch Set 8 : Renaming to 'subsumes' #Patch Set 9 : Addressing last comments #Patch Set 10 : Changing comment #
Dependent Patchsets: Messages
Total messages: 34 (12 generated)
amalika@google.com changed reviewers: + mkwst@chromium.org
There are quite a lot of tests... Some more corner cases were found.
Description was changed from ========== This is part of experimental feature of Embedding-CSP. In particular this is the initial effort in implementing *3.3. Is policy list subsumed under subsuming policy? Given two CSPSources, we try to match the two as much as possible based on wildcards in hosts/ports, paths, schemes and ports. If not match was found, nullptr is returned. BUG=647588 ========== to ========== This is part of an experimental feature Embedding-CSP. In particular this is the initial effort in implementing *3.3. Is policy list subsumed under subsuming policy? Given two CSPSources, we try to match the two as much as possible based on wildcards in hosts/ports, paths, schemes and ports. If not match was found, nullptr is returned. BUG=647588 ==========
https://codereview.chromium.org/2442513004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/csp/CSPSource.h (right): https://codereview.chromium.org/2442513004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/CSPSource.h:34: static Member<CSPSource> getNormalized(Member<CSPSource>, Member<CSPSource>); 1. You can probably just pass `CSPSource*` here. 2. Please add argument names here so it's clear what each of the arguments stands for. https://codereview.chromium.org/2442513004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/CSPSource.h:35: static Member<CSPSource> getPrefferredCSPSource(Member<CSPSource>, 1. s/Prefferred/Preferred/g 2. Ditto the argument names. https://codereview.chromium.org/2442513004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/CSPSource.h:58: Member<CSPSource>); 1. `CSPSource*` 2. Argument names. 3. Preferred. https://codereview.chromium.org/2442513004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): https://codereview.chromium.org/2442513004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:57: return equalIgnoringCase(protocol, m_scheme); While you're here, can you add a TODO to drop the `IgnoringCase` bit in favor of DCHECKs that we're getting normalized strings? We shouldn't need to fold cases here.
Addressing initial comments
After our conversation, it seemed like it would indeed be better to separate functionalities (namely, of normalizing and checking subsumption) separately. I separated the two and I think it is now a lot more clear what each method is trying to do. However, I did not add tests for the method getCommong() because now it just returns the most strict information but since hosts, ports, schemes are private members. So I was not sure how to compare the returned new CSPSource. Maybe after the next patch (such as higher level methods in CSPSourceList could check its correctness? Or I could remove it from this patch and address in the next one. In which case, this patch would only be about CSPSource is Subsumed.
Before I get too much further, let's chat briefly about `isSimilar()`. https://codereview.chromium.org/2442513004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPSource.h (right): https://codereview.chromium.org/2442513004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.h:34: // Find the normalized CSPSource of the two. What does this mean? :) https://codereview.chromium.org/2442513004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.h:36: // Assuming two CSPSources are matching, return whichever is more restrictive. This is incomplete: you're also synthesizing new `CSPSource` objects in some cases. https://codereview.chromium.org/2442513004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.h:52: CSPSource* getPreferredCSPSourceBasedOnEmptySchemes(CSPSource* other, Nit: Describe these methods too. I'm still not sure I understand how you plan to use them. https://codereview.chromium.org/2442513004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp (right): https://codereview.chromium.org/2442513004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp:162: const NormalizationReturn expected; Here, and elsewhere, would you mind reordering these to define `a` all together, then `b`? I find it hard to read as-is. It might be even clearer if you break it out into a distinct struct, like: ``` struct Source { const char* scheme; const int port; const char* path; }; struct TestCase { const Source a; const Source b; const int expectation; } cases[] = { { { "http", 0, "/" }, { "http", 0, "/" }, 1 }, { { "https", 0, "/" }, { "https", 0, "/" }, 1 }, { { "https", 80, "/" }, { "http", 80, "/" }, 1 }, ... }; ``` https://codereview.chromium.org/2442513004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp:206: assertNormalizationReturn(b, a, a->getPreferredCSPSource(b), test.expected); Why do you need a separate loop, rather than adding something like the following to the first loop? ``` assertNormalizationReturn(a, b, b->getPreferredCSPSource(a), test.expected); ``` https://codereview.chromium.org/2442513004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp:239: assertNormalizationReturn(a, b, a->getNormalized(b), test.expected); Might as well do the inverse here as well. https://codereview.chromium.org/2442513004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): https://codereview.chromium.org/2442513004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:130: (equalIgnoringCase(m_scheme, "https") && Nit: DCHECK_EQ(m_scheme, m_scheme.lower()), and replace `equalIgnoringCase` with `==`. https://codereview.chromium.org/2442513004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:138: (other->m_hostWildcard == HasWildcard && other->hostMatches(m_host)); Isn't the wildcard work done in `hostMatches()`? Can you just use that? https://codereview.chromium.org/2442513004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:141: portMatches(other->m_port, other->m_scheme); Isn't the wildcard work done in `portMatches()`? https://codereview.chromium.org/2442513004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:142: bool pathsMatch = other->m_path.isEmpty() || other->m_path == "/" || Nit: This matches if `schemesOnly` too, right? If so, you can return right after calculating `schemesOnly` and `schemesMatch`, and simplify the remaining comparisons. https://codereview.chromium.org/2442513004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPSource.h (right): https://codereview.chromium.org/2442513004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.h:34: bool isSubsumed(CSPSource*); Nit: Please add a comment here, and probably point off to https://w3c.github.io/webappsec-csp/embedded/#subsume-policy. In particular, does `true` mean that `this` is subsumed _by_ `b`, or that `this` _subsumes_ `b`? Perhaps changing the name to `subsumes()` or `isSubsumedBy()` could clarify? https://codereview.chromium.org/2442513004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.h:35: // Find the normalized CSPSource of the two. 1. What does "normalize" mean? 2. It would be great to document the algorithm in the spec, and point off to it as explanation. https://codereview.chromium.org/2442513004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.h:50: bool portMatches(int, const String&) const; Would you mind pulling this refactoring out into a separate patch that we can land today while we discuss the rest? https://codereview.chromium.org/2442513004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.h:52: bool isSimilar(CSPSource* other); It's probably worth adding unit tests for this piece, and describing what "similar" means.
Splitting into smaller parts + more tests. More description on the logic here: Is CSPSource subsumed by another? https://docs.google.com/document/d/1xwTxpB_sWYaTrOBJEPSXtfWKO4M2k9Xya7o0zcd6D... https://codereview.chromium.org/2442513004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): https://codereview.chromium.org/2442513004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:138: (other->m_hostWildcard == HasWildcard && other->hostMatches(m_host)); On 2016/10/26 at 11:40:29, Mike West wrote: > Isn't the wildcard work done in `hostMatches()`? Can you just use that? Unfortunately, this does not work when both hosts have wildcards. If both do, then we need to check that and still compare if the hosts are equal ( host == m_host ). We can do that because both will also contain stars so they have to perfectly match anyways. But I simplified it still adding this additional case! https://codereview.chromium.org/2442513004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:141: portMatches(other->m_port, other->m_scheme); On 2016/10/26 at 11:40:29, Mike West wrote: > Isn't the wildcard work done in `portMatches()`? It does not work for when there are wildcards in both ports considered. To handle this case, it is sufficient to just check whether one of them has a wildcard (in which case it would match anything) or if it doesnt have a wildcard, then we can just use already existing methods.
amalika@google.com changed reviewers: + jochen@chromium.org
Description was changed from ========== This is part of an experimental feature Embedding-CSP. In particular this is the initial effort in implementing *3.3. Is policy list subsumed under subsuming policy? Given two CSPSources, we try to match the two as much as possible based on wildcards in hosts/ports, paths, schemes and ports. If not match was found, nullptr is returned. BUG=647588 ========== to ========== This is part of an experimental feature Embedding-CSP. In particular this is the initial effort in implementing *3.3. Is policy list subsumed under subsuming policy? Given two CSPSources, we try to match the two as much as possible based on wildcards in hosts/ports, paths, schemes and ports. If no match was found, nullptr is returned. BUG=647588 ==========
Description was changed from ========== This is part of an experimental feature Embedding-CSP. In particular this is the initial effort in implementing *3.3. Is policy list subsumed under subsuming policy? Given two CSPSources, we try to match the two as much as possible based on wildcards in hosts/ports, paths, schemes and ports. If no match was found, nullptr is returned. BUG=647588 ========== to ========== This is part of an experimental feature Embedding-CSP. In particular this is the initial effort in implementing *3.3. Is policy list subsumed under subsuming policy? Given two CSPSources, we try to determine whether one is subsumed under another. If not, return false. BUG=647588 ==========
Adding jochen@ for an overall review. The concepts of similarity and subsumption of CSPSources are described here: https://docs.google.com/document/d/1xwTxpB_sWYaTrOBJEPSXtfWKO4M2k9Xya7o0zcd6D... We determine whether CSPSource `A` is subsumed under CSPSource `B`. It is true, if the two are similar. Secondly, if `A` matches `B`, but `B` might not match `A` (in that case `A` is more restrictive, for example, if it provides more specific path). We use existing `match*` functions. Example 1: A: http://example.com/ B: http://example.com/index.html isSubsumed should return `false`. Example 2: A: http://example.com/index.html B: http://example.com/ isSubsumed should return `true`. Example 3: A: http://non-example.com/index.html B: http://example.com/ isSubsumed should return `false` since two CSPSources are not even similar. Note: From my understanding `CSPSource` stands for `Scheme-source/ host-source expression` in the Embdedded Enforcement specs.
i guess there's some implicit assumption that m_scheme is always one out of {ws
wss http https}? why not just compare with those strings instead of relaying on
m_string.length?
https://codereview.chromium.org/2442513004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right):
https://codereview.chromium.org/2442513004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:116:
portMatches(other->m_port, other->m_scheme);
why not also other->portMatches(m_port, m_scheme)?
https://codereview.chromium.org/2442513004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:127:
!isPathSubsumedBy(other))
add { } around if body
https://codereview.chromium.org/2442513004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:138: return true;
isn't that the same as
return m_hostWildcard == other->m_hostWildcard && m_portWildcard ==
other->m_portWildcard?
https://codereview.chromium.org/2442513004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:145: return
m_scheme.length() == 3 || m_scheme.length() == 5 ? true : false;
you really want something like isSchemeSecure(m_scheme) here, no?
https://codereview.chromium.org/2442513004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/csp/CSPSource.h (right):
https://codereview.chromium.org/2442513004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/CSPSource.h:34: // Check whether this
CSPSource is subsumed under a given CSPSource for a
nit. add empty line before this one
https://codereview.chromium.org/2442513004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/CSPSource.h:53: //
https://docs.google.com/document/d/1xwTxpB_sWYaTrOBJEPSXtfWKO4M2k9Xya7o0zcd6D...
if you put links in the source, please make this a world-readable doc
https://codereview.chromium.org/2442513004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/CSPSource.h:56: bool
isWildcardsSubsumedBy(CSPSource* other);
isWildcard or areWildcards
Schemes can be something other than "http" and "ws" but there is a subsumption special case when one scheme is a more secure version of another (one is "https" and the other "http"). Example: A: https://example.com B: http://example.com A is subsumed under B because it is more restrictive. Example 2: A: http://example.com B: https://example.com A is NOT subsumed under B because it is less restrictive. When schemes are equal lengths (and are equal which is guaranteed by `isSimilar`) then A is always subsumed under B. https://codereview.chromium.org/2442513004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): https://codereview.chromium.org/2442513004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:116: portMatches(other->m_port, other->m_scheme); On 2016/11/02 at 11:11:32, jochen wrote: > why not also other->portMatches(m_port, m_scheme)? 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. https://codereview.chromium.org/2442513004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:138: return true; On 2016/11/02 at 11:11:33, jochen wrote: > isn't that the same as > > return m_hostWildcard == other->m_hostWildcard && m_portWildcard == other->m_portWildcard? 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`. https://codereview.chromium.org/2442513004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:145: return m_scheme.length() == 3 || m_scheme.length() == 5 ? true : false; On 2016/11/02 at 11:11:32, jochen wrote: > you really want something like isSchemeSecure(m_scheme) here, no? 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?
Also a side note, initially i made a few optimizations instead of calling pathMatches twice (since only special cases have to be additional checked when checking other->pathMatches(this->m_path). But Mike said it would be better to just call pathMatches twice for readability purposes and since we know for now not many people will be using this experimental feature. (He did not however check the logic extensively in these patches).
On 2016/11/02 at 13:02:05, amalika wrote:
> Also a side note, initially i made a few optimizations instead of calling
pathMatches twice (since only special cases have to be additional checked when
checking other->pathMatches(this->m_path).
>
> But Mike said it would be better to just call pathMatches twice for
readability purposes and since we know for now not many people will be using
this experimental feature. (He did not however check the logic extensively in
these patches).
yeah, I think that would be nice.
Would it be possible to remove isSimilar() and instead have the
is${THING}SubsumedBy()s do all the work?
e.g. for the path, just get the two paths (or / if the path is empty) and check
that other path is a prefix of path?
btw, you need to repeat the CL title as the first line of the CL description, as only the CL description will end up in the git commit message. Right now, the commit would be labelled "This is part of an experimental feature Embedding-CSP."
Description was changed from ========== This is part of an experimental feature Embedding-CSP. In particular this is the initial effort in implementing *3.3. Is policy list subsumed under subsuming policy? Given two CSPSources, we try to determine whether one is subsumed under another. If not, return false. BUG=647588 ========== to ========== Part 1.1: Is policy list subsumed under subsuming policy? This is part of an experimental feature Embedding-CSP. In particular this is the initial effort in implementing *3.3. Is policy list subsumed under subsuming policy? Given two CSPSources, we try to determine whether one is subsumed under another. If not, return false. BUG=647588 ==========
On 2016/11/03 at 12:27:31, jochen wrote:
> On 2016/11/02 at 13:02:05, amalika wrote:
> > Also a side note, initially i made a few optimizations instead of calling
pathMatches twice (since only special cases have to be additional checked when
checking other->pathMatches(this->m_path).
> >
> > But Mike said it would be better to just call pathMatches twice for
readability purposes and since we know for now not many people will be using
this experimental feature. (He did not however check the logic extensively in
these patches).
>
> yeah, I think that would be nice.
>
> Would it be possible to remove isSimilar() and instead have the
is${THING}SubsumedBy()s do all the work?
>
> e.g. for the path, just get the two paths (or / if the path is empty) and
check that other path is a prefix of path?
Yeah, that might be a good idea! It is just the same functionality is necessary
for normalizing CSPSources in getNormalized method (here is a patch:
https://codereview.chromium.org/2470083002) but where the order does not matter
(we have no preference between 'this' and 'other').
So I was thinking it would be more centralized and clear to define "similarity"
between CSPSources, at which point we even want to compare them because most
CSPSources would probably be 'unsimilar' when comparing. We want to normalize
two CSPSources (i.e. find their common features such as host with/without
wildcards, appropriate paths) only if they are 'similar'.
Do you think it would still be better to integrate the logic into
is${THING}SubsumedBy() and leave similarity only for normalization?
On 2016/11/03 at 12:40:50, amalika wrote:
> On 2016/11/03 at 12:27:31, jochen wrote:
> > On 2016/11/02 at 13:02:05, amalika wrote:
> > > Also a side note, initially i made a few optimizations instead of calling
pathMatches twice (since only special cases have to be additional checked when
checking other->pathMatches(this->m_path).
> > >
> > > But Mike said it would be better to just call pathMatches twice for
readability purposes and since we know for now not many people will be using
this experimental feature. (He did not however check the logic extensively in
these patches).
> >
> > yeah, I think that would be nice.
> >
> > Would it be possible to remove isSimilar() and instead have the
is${THING}SubsumedBy()s do all the work?
> >
> > e.g. for the path, just get the two paths (or / if the path is empty) and
check that other path is a prefix of path?
>
> Yeah, that might be a good idea! It is just the same functionality is
necessary for normalizing CSPSources in getNormalized method (here is a patch:
https://codereview.chromium.org/2470083002) but where the order does not matter
(we have no preference between 'this' and 'other').
>
> So I was thinking it would be more centralized and clear to define
"similarity" between CSPSources, at which point we even want to compare them
because most CSPSources would probably be 'unsimilar' when comparing. We want to
normalize two CSPSources (i.e. find their common features such as host
with/without wildcards, appropriate paths) only if they are 'similar'.
>
> Do you think it would still be better to integrate the logic into
is${THING}SubsumedBy() and leave similarity only for normalization?
yeah, I think that would make it much more readable
Greatly, simplifying the logic :) Also, changed the direction of subsumption (to check `this` is subsuming of `other`, rather than `this` is subsumed by `other`) to match it with other patches.
Patchset #9 (id:160001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
wow, so much easier to read! lgtm https://codereview.chromium.org/2442513004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/csp/CSPSource.h (right): https://codereview.chromium.org/2442513004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/CSPSource.h:35: // Check whether this CSPSource is subsuming of a given CSPSource for a not sure whether subsuming *of* is correct english, I'll defer to Mike on that
This is a lot clearer, thanks for going through all the iterations. LGTM % text nit. https://codereview.chromium.org/2442513004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPSource.h (right): https://codereview.chromium.org/2442513004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.h:55: // https://docs.google.com/document/d/1xwTxpB_sWYaTrOBJEPSXtfWKO4M2k9Xya7o0zcd6D... 1. This needs to be a publicly accessible link. `google.com` docs can't be made public. 2. I'd prefer to link to the spec; can we upstream your algorithm to that document? https://codereview.chromium.org/2442513004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/csp/CSPSource.h (right): https://codereview.chromium.org/2442513004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/CSPSource.h:35: // Check whether this CSPSource is subsuming of a given CSPSource for a On 2016/11/04 at 09:15:43, jochen wrote: > not sure whether subsuming *of* is correct english, I'll defer to Mike on that I'd say "this CSPSource _subsumes_ a given CSPSource".
On 2016/11/04 at 09:48:10, mkwst wrote: > This is a lot clearer, thanks for going through all the iterations. LGTM % text nit. > > https://codereview.chromium.org/2442513004/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/csp/CSPSource.h (right): > > https://codereview.chromium.org/2442513004/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/csp/CSPSource.h:55: // https://docs.google.com/document/d/1xwTxpB_sWYaTrOBJEPSXtfWKO4M2k9Xya7o0zcd6D... > 1. This needs to be a publicly accessible link. `google.com` docs can't be made public. > 2. I'd prefer to link to the spec; can we upstream your algorithm to that document? I think it might have been an older patch. Right now I just included a link to `https://w3c.github.io/webappsec-csp/embedded/#subsume-policy`, where we can just add all the parts. Would this be okay? > https://codereview.chromium.org/2442513004/diff/180001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/frame/csp/CSPSource.h (right): > > https://codereview.chromium.org/2442513004/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/frame/csp/CSPSource.h:35: // Check whether this CSPSource is subsuming of a given CSPSource for a > On 2016/11/04 at 09:15:43, jochen wrote: > > not sure whether subsuming *of* is correct english, I'll defer to Mike on that > > I'd say "this CSPSource _subsumes_ a given CSPSource". I updated all the functions names!
On 2016/11/04 at 10:06:49, amalika wrote: > On 2016/11/04 at 09:48:10, mkwst wrote: > > This is a lot clearer, thanks for going through all the iterations. LGTM % text nit. > > > > https://codereview.chromium.org/2442513004/diff/80001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/frame/csp/CSPSource.h (right): > > > > https://codereview.chromium.org/2442513004/diff/80001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/frame/csp/CSPSource.h:55: // https://docs.google.com/document/d/1xwTxpB_sWYaTrOBJEPSXtfWKO4M2k9Xya7o0zcd6D... > > 1. This needs to be a publicly accessible link. `google.com` docs can't be made public. > > 2. I'd prefer to link to the spec; can we upstream your algorithm to that document? > > I think it might have been an older patch. Right now I just included a link to `https://w3c.github.io/webappsec-csp/embedded/#subsume-policy`, where we can just add all the parts. Would this be okay? Yeah, sorry. Let's upstream things to the spec, and leave the spec link in as-is. > > > https://codereview.chromium.org/2442513004/diff/180001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/frame/csp/CSPSource.h (right): > > > > https://codereview.chromium.org/2442513004/diff/180001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/frame/csp/CSPSource.h:35: // Check whether this CSPSource is subsuming of a given CSPSource for a > > On 2016/11/04 at 09:15:43, jochen wrote: > > > not sure whether subsuming *of* is correct english, I'll defer to Mike on that > > > > I'd say "this CSPSource _subsumes_ a given CSPSource". > > I updated all the functions names! Please update the comment as well. :) Something like "Returns true if this CSPSource subsumes the given CSPSource, as defined by the algorithm at [link goes here]."
The CQ bit was checked by amalika@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2442513004/#ps240001 (title: "Changing comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #10 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Part 1.1: Is policy list subsumed under subsuming policy? This is part of an experimental feature Embedding-CSP. In particular this is the initial effort in implementing *3.3. Is policy list subsumed under subsuming policy? Given two CSPSources, we try to determine whether one is subsumed under another. If not, return false. BUG=647588 ========== to ========== Part 1.1: Is policy list subsumed under subsuming policy? This is part of an experimental feature Embedding-CSP. In particular this is the initial effort in implementing *3.3. Is policy list subsumed under subsuming policy? Given two CSPSources, we try to determine whether one is subsumed under another. If not, return false. BUG=647588 Committed: https://crrev.com/ab18fdcd7bad1f68ec6bc0d2ebdb67fd5f200df0 Cr-Commit-Position: refs/heads/master@{#429872} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/ab18fdcd7bad1f68ec6bc0d2ebdb67fd5f200df0 Cr-Commit-Position: refs/heads/master@{#429872} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
