Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2487983003:
Part 2.3: Is policy list subsumed under subsuming policy? (Closed)
DescriptionPart 2.3: Is policy list subsumed under subsuming policy?
This is part of an experimental feature Embedding-CSP.
This patch considers schemes when finding an `effective`
vector of CSPSources.
Previously, we handled scheme to host and
host to host source expressions.
This CL considers specifically scheme to scheme source
expressions.
Example 1:
A: http:
B: http://example.com
Then the resulting intersaction should be B.
Example 2:
A: https: http://example.com
B: http://example.com http:
Then the result should be `http://example.com https:`,
i.e. the former should not be upgraded to "https"
Moreover, we should also NOT stop when we find a match.
Example 3:
A: https://example.com http://example.com/foo
B: http://example.com/foo https://example.com
If we stop based on first match, we would get:
`https://example.com/foo http://example.com/foo`
However, the correct result should be:
`https://example.com http://example.com/foo`
since the order of CSPSources should not matter.
BUG=647588
Committed: https://crrev.com/cb5cc2c4d0e9f56a7b408d33d264b7ca0c864a18
Cr-Commit-Position: refs/heads/master@{#434477}
Patch Set 1 : Properly handling scheme-source to scheme-source matching #
Total comments: 6
Patch Set 2 : Separating scheme to scheme normalization #
Total comments: 20
Patch Set 3 : Addressing comments #
Total comments: 4
Patch Set 4 : Adding a comment #
Total comments: 1
Messages
Total messages: 35 (18 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Part 2.3: Subsumption Algorithm This patch considers schemes when finding an `effective` vector of CSPSources. Previously, we only were matching CSPSources on host-source principle. However, we would only want to match a scheme-source to a host-source if there are no other scheme-sources in the lists. Example 1: A: http: B: http://example.com Then the resulting intersaction should be B. Example 2: A: https: http://example.com B: http://example.com http: Then the result should be `http://example.com https:`. Note: We should also NOT stop when we find a match. Example 3: A: https://example.com http://example.com/foo B: http://example.com/foo https://example.com If we stop based on first match, we would get: `https://example.com/foo http://example.com/foo` However, the correct result should be: `https://example.com http://example.com/foo` since the order of CSPSources should not matter. BUG=647588 ========== to ========== Part 2.3: Subsumption Algorithm This is part of an experimental feature Embedding-CSP. This patch considers schemes when finding an `effective` vector of CSPSources. Previously, we only were matching CSPSources on host-source principle. However, we would only want to match a scheme-source to a host-source if there are no other scheme-sources in the lists. Example 1: A: http: B: http://example.com Then the resulting intersaction should be B. Example 2: A: https: http://example.com B: http://example.com http: Then the result should be `http://example.com https:`. Note: We should also NOT stop when we find a match. Example 3: A: https://example.com http://example.com/foo B: http://example.com/foo https://example.com If we stop based on first match, we would get: `https://example.com/foo http://example.com/foo` However, the correct result should be: `https://example.com http://example.com/foo` since the order of CSPSources should not matter. BUG=647588 ==========
amalika@google.com changed reviewers: + jochen@chromium.org, mkwst@chromium.org
This is a second part for finding `effective policy`. Previously, we handled scheme to host and host to host source expressions. This CL considers specifically scheme to scheme source expressions. https://codereview.chromium.org/2487983003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp (right): https://codereview.chromium.org/2487983003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:585: if (bCspSource->subsumes(aCspSource)) { We always want to preserve the CSPSource we are currently considering instead of just finding intersaction. This is to handle the case: Example 3: A: https://example.com http://example.com/foo B: http://example.com/foo https://example.com However, as soon as we know it is subsumed by bCSPSource, we can break out of the loop.
Description was changed from ========== Part 2.3: Subsumption Algorithm This is part of an experimental feature Embedding-CSP. This patch considers schemes when finding an `effective` vector of CSPSources. Previously, we only were matching CSPSources on host-source principle. However, we would only want to match a scheme-source to a host-source if there are no other scheme-sources in the lists. Example 1: A: http: B: http://example.com Then the resulting intersaction should be B. Example 2: A: https: http://example.com B: http://example.com http: Then the result should be `http://example.com https:`. Note: We should also NOT stop when we find a match. Example 3: A: https://example.com http://example.com/foo B: http://example.com/foo https://example.com If we stop based on first match, we would get: `https://example.com/foo http://example.com/foo` However, the correct result should be: `https://example.com http://example.com/foo` since the order of CSPSources should not matter. BUG=647588 ========== to ========== Part 2.3: Subsumption Algorithm This is part of an experimental feature Embedding-CSP. This patch considers schemes when finding an `effective` vector of CSPSources. Previously, we handled scheme to host and host to host source expressions. This CL considers specifically scheme to scheme source expressions. Example 1: A: http: B: http://example.com Then the resulting intersaction should be B. Example 2: A: https: http://example.com B: http://example.com http: Then the result should be `http://example.com https:`. Note: We should also NOT stop when we find a match. Example 3: A: https://example.com http://example.com/foo B: http://example.com/foo https://example.com If we stop based on first match, we would get: `https://example.com/foo http://example.com/foo` However, the correct result should be: `https://example.com http://example.com/foo` since the order of CSPSources should not matter. BUG=647588 ==========
Description was changed from ========== Part 2.3: Subsumption Algorithm This is part of an experimental feature Embedding-CSP. This patch considers schemes when finding an `effective` vector of CSPSources. Previously, we handled scheme to host and host to host source expressions. This CL considers specifically scheme to scheme source expressions. Example 1: A: http: B: http://example.com Then the resulting intersaction should be B. Example 2: A: https: http://example.com B: http://example.com http: Then the result should be `http://example.com https:`. Note: We should also NOT stop when we find a match. Example 3: A: https://example.com http://example.com/foo B: http://example.com/foo https://example.com If we stop based on first match, we would get: `https://example.com/foo http://example.com/foo` However, the correct result should be: `https://example.com http://example.com/foo` since the order of CSPSources should not matter. BUG=647588 ========== to ========== Part 2.3: Subsumption Algorithm This is part of an experimental feature Embedding-CSP. This patch considers schemes when finding an `effective` vector of CSPSources. Previously, we handled scheme to host and host to host source expressions. This CL considers specifically scheme to scheme source expressions. Example 1: A: http: B: http://example.com Then the resulting intersaction should be B. Example 2: A: https: http://example.com B: http://example.com http: Then the result should be `http://example.com https:`, i.e. the former should not be upgraded to "https" Moreover, we should also NOT stop when we find a match. Example 3: A: https://example.com http://example.com/foo B: http://example.com/foo https://example.com If we stop based on first match, we would get: `https://example.com/foo http://example.com/foo` However, the correct result should be: `https://example.com http://example.com/foo` since the order of CSPSources should not matter. BUG=647588 ==========
Description was changed from ========== Part 2.3: Subsumption Algorithm This is part of an experimental feature Embedding-CSP. This patch considers schemes when finding an `effective` vector of CSPSources. Previously, we handled scheme to host and host to host source expressions. This CL considers specifically scheme to scheme source expressions. Example 1: A: http: B: http://example.com Then the resulting intersaction should be B. Example 2: A: https: http://example.com B: http://example.com http: Then the result should be `http://example.com https:`, i.e. the former should not be upgraded to "https" Moreover, we should also NOT stop when we find a match. Example 3: A: https://example.com http://example.com/foo B: http://example.com/foo https://example.com If we stop based on first match, we would get: `https://example.com/foo http://example.com/foo` However, the correct result should be: `https://example.com http://example.com/foo` since the order of CSPSources should not matter. BUG=647588 ========== to ========== Part 2.3: Is policy list subsumed under subsuming policy? This is part of an experimental feature Embedding-CSP. This patch considers schemes when finding an `effective` vector of CSPSources. Previously, we handled scheme to host and host to host source expressions. This CL considers specifically scheme to scheme source expressions. Example 1: A: http: B: http://example.com Then the resulting intersaction should be B. Example 2: A: https: http://example.com B: http://example.com http: Then the result should be `http://example.com https:`, i.e. the former should not be upgraded to "https" Moreover, we should also NOT stop when we find a match. Example 3: A: https://example.com http://example.com/foo B: http://example.com/foo https://example.com If we stop based on first match, we would get: `https://example.com/foo http://example.com/foo` However, the correct result should be: `https://example.com http://example.com/foo` since the order of CSPSources should not matter. BUG=647588 ==========
Small comments, mostly good to go! https://codereview.chromium.org/2487983003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): https://codereview.chromium.org/2487983003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:148: if (isSchemeOnly() || other->isSchemeOnly()) { Unit test? https://codereview.chromium.org/2487983003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp (right): https://codereview.chromium.org/2487983003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:578: if (aCspSource->isSchemeOnly() && bCspSource->isSchemeOnly()) { Nit: I guess I reviewed this earlier, but looking at it not, |sourceA| seems a lot clearer than |aCspSource|. https://codereview.chromium.org/2487983003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp (right): https://codereview.chromium.org/2487983003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp:301: SourceListDirective sourceList("script-src", sources, csp.get()); Might as well just inline the source string here. https://codereview.chromium.org/2487983003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp:332: sourceList.getIntersectCSPSources(secondList.m_list); This should be symmetric right? Could you add checks for `secondList.getIntersectCSPSources(sourceList)`?
Patchset #2 (id:80001) has been deleted
I added and refactored some parts to better scheme to scheme normalization. In particular, when we iterate over one list that contains a scheme-source expression such as `http:`, there are two possibilities: Consider normalization of listA and listB. 1. listB might contain `http:` or `https:` in which case we would want to match those together and then when we would encounter any host source expressions with `https:`, we would skip them Example: listA: http: https://example.com listB: ws://example.com https: Normalization should be: https: 2. listB does not contain `http:` or `https:`, then we want to make sure we add all host source expressions with `http:` from listB to the normalized list. Example: listA: http: listB: ws://example.com http://one.com https://two.com Normalization should be: http://one.com https://two.com Since we do not know it what's in listB in advance, I added a hash map of scheme-only source expressions that are contained in both listA and listB. Note: we also need to make sure we correctly handle http: to https: matching. Example: listA: https: http://example.com listB: http: Normalization should be: https: http://example.com (the latter is not a secure version) i.e. even if we match `http` from listB to `https` from listA, we do not give up on `http` and continue matching with expressions with that scheme. https://codereview.chromium.org/2487983003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): https://codereview.chromium.org/2487983003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:148: if (isSchemeOnly() || other->isSchemeOnly()) { On 2016/11/17 at 10:58:31, Mike West wrote: > Unit test? Added `IntersectSchemesOnly`! (and test `Intersect` just in case) https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/csp/CSPSource.h (right): https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/CSPSource.h:43: void addToMap(HashMap<String, CSPSource*>&); Would it be better if I change it to be a lambda function inside `getIntersectSchemesOnly` instead since it is the only use case? https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp (right): https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp:315: "https: http://another-example1.com/bar/"}, The above tests the proper functionality of `getIntersectSchemesOnly` https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp:324: "http://example1.com/foo/ https://example1.com/foo/"}, The normalization of a list of CSPSources is not symmetric because we match one list to another one by one. For example, in this case if we were to normalized listB to listA the result would be "https://example1.com/foo/ http://example1.com/foo/". When we normalized listA to listB, we get repetitions because listB does not have `http`, we then try to match all `http` source expressions to make sure we add them. We could make an optimization to check whether a certain CSPSource was already added to avoid adding it again, but not sure if it is necessary?
On Sat, Nov 19, 2016 at 7:34 PM, <amalika@google.com> wrote: > I added and refactored some parts to better scheme to scheme > normalization. > In particular, when we iterate over one list that contains a scheme-source > expression such as `http:`, there are two possibilities: > > Consider normalization of listA and listB. > > 1. listB might contain `http:` or `https:` in which case > we would want to match those together and then when we would encounter any > host source expressions with `https:`, we would skip them > > Example: > listA: http: https://example.com > listB: ws://example.com https: > > Normalization should be: https: > > 2. listB does not contain `http:` or `https:`, then we want to make sure > we add all host source expressions with `http:` from listB to the > normalized > list. > > Example: > listA: http: > listB: ws://example.com http://one.com https://two.com > > Normalization should be: http://one.com https://two.com > > Since we do not know it what's in listB in advance, I added a hash map of > scheme-only source expressions that are contained in both listA and listB. > > Note: we also need to make sure we correctly handle http: to https: > matching. > > Example: > listA: https: http://example.com > listB: http: > > Normalization should be: https: http://example.com > (the latter is not a secure version) > > i.e. even if we match `http` from listB to `https` from listA, we do not > give up on `http` and continue matching with expressions with that scheme. > > > > https://codereview.chromium.org/2487983003/diff/60001/ > third_party/WebKit/Source/core/frame/csp/CSPSource.cpp > File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): > > https://codereview.chromium.org/2487983003/diff/60001/ > third_party/WebKit/Source/core/frame/csp/CSPSource.cpp#newcode148 > third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:148: if > (isSchemeOnly() || other->isSchemeOnly()) { > On 2016/11/17 at 10:58:31, Mike West wrote: > > Unit test? > > Added `IntersectSchemesOnly`! > (and test `Intersect` just in case) > > https://codereview.chromium.org/2487983003/diff/100001/ > third_party/WebKit/Source/core/frame/csp/CSPSource.h > File third_party/WebKit/Source/core/frame/csp/CSPSource.h (right): > > https://codereview.chromium.org/2487983003/diff/100001/ > third_party/WebKit/Source/core/frame/csp/CSPSource.h#newcode43 > third_party/WebKit/Source/core/frame/csp/CSPSource.h:43: void > addToMap(HashMap<String, CSPSource*>&); > Would it be better if I change it to be a lambda function inside > `getIntersectSchemesOnly` instead since it is the only use case? > > https://codereview.chromium.org/2487983003/diff/100001/ > third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp > File > third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp > (right): > > https://codereview.chromium.org/2487983003/diff/100001/ > third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp# > newcode315 > third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp:315: > "https: http://another-example1.com/bar/"}, > The above tests the proper functionality of `getIntersectSchemesOnly` > > https://codereview.chromium.org/2487983003/diff/100001/ > third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp# > newcode324 > third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp:324: > "http://example1.com/foo/ https://example1.com/foo/"}, > The normalization of a list of CSPSources is not symmetric because we > match one list to another one by one. > For example, in this case if we were to normalized listB to listA the > result would be "https://example1.com/foo/ http://example1.com/foo/". > When we normalized listA to listB, we get repetitions because listB does > not have `http`, we then try to match all `http` source expressions to > make sure we add them. > We could make an optimization to check whether a certain CSPSource was > already added to avoid adding it again, but not sure if it is necessary? > In particular, we normalize the `height` (`height` is a number of policies which we squash into one) but current implementation allows for unnormalized `width` (number of CSPSources in a source list), i.e. there might be repetitions or things like "https https://example.com" (where obviously the former alone would be sufficient). If we try to normalize `width`, it would be less efficient & would require adding some data structure to keep track of csp source that we already added, which is quite unnecessary for subsumption. > https://codereview.chromium.org/2487983003/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Sat, Nov 19, 2016 at 7:34 PM, <amalika@google.com> wrote: > I added and refactored some parts to better scheme to scheme > normalization. > In particular, when we iterate over one list that contains a scheme-source > expression such as `http:`, there are two possibilities: > > Consider normalization of listA and listB. > > 1. listB might contain `http:` or `https:` in which case > we would want to match those together and then when we would encounter any > host source expressions with `https:`, we would skip them > > Example: > listA: http: https://example.com > listB: ws://example.com https: > > Normalization should be: https: > > 2. listB does not contain `http:` or `https:`, then we want to make sure > we add all host source expressions with `http:` from listB to the > normalized > list. > > Example: > listA: http: > listB: ws://example.com http://one.com https://two.com > > Normalization should be: http://one.com https://two.com > > Since we do not know it what's in listB in advance, I added a hash map of > scheme-only source expressions that are contained in both listA and listB. > > Note: we also need to make sure we correctly handle http: to https: > matching. > > Example: > listA: https: http://example.com > listB: http: > > Normalization should be: https: http://example.com > (the latter is not a secure version) > > i.e. even if we match `http` from listB to `https` from listA, we do not > give up on `http` and continue matching with expressions with that scheme. > > > > https://codereview.chromium.org/2487983003/diff/60001/ > third_party/WebKit/Source/core/frame/csp/CSPSource.cpp > File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): > > https://codereview.chromium.org/2487983003/diff/60001/ > third_party/WebKit/Source/core/frame/csp/CSPSource.cpp#newcode148 > third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:148: if > (isSchemeOnly() || other->isSchemeOnly()) { > On 2016/11/17 at 10:58:31, Mike West wrote: > > Unit test? > > Added `IntersectSchemesOnly`! > (and test `Intersect` just in case) > > https://codereview.chromium.org/2487983003/diff/100001/ > third_party/WebKit/Source/core/frame/csp/CSPSource.h > File third_party/WebKit/Source/core/frame/csp/CSPSource.h (right): > > https://codereview.chromium.org/2487983003/diff/100001/ > third_party/WebKit/Source/core/frame/csp/CSPSource.h#newcode43 > third_party/WebKit/Source/core/frame/csp/CSPSource.h:43: void > addToMap(HashMap<String, CSPSource*>&); > Would it be better if I change it to be a lambda function inside > `getIntersectSchemesOnly` instead since it is the only use case? > > https://codereview.chromium.org/2487983003/diff/100001/ > third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp > File > third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp > (right): > > https://codereview.chromium.org/2487983003/diff/100001/ > third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp# > newcode315 > third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp:315: > "https: http://another-example1.com/bar/"}, > The above tests the proper functionality of `getIntersectSchemesOnly` > > https://codereview.chromium.org/2487983003/diff/100001/ > third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp# > newcode324 > third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp:324: > "http://example1.com/foo/ https://example1.com/foo/"}, > The normalization of a list of CSPSources is not symmetric because we > match one list to another one by one. > For example, in this case if we were to normalized listB to listA the > result would be "https://example1.com/foo/ http://example1.com/foo/". > When we normalized listA to listB, we get repetitions because listB does > not have `http`, we then try to match all `http` source expressions to > make sure we add them. > We could make an optimization to check whether a certain CSPSource was > already added to avoid adding it again, but not sure if it is necessary? > In particular, we normalize the `height` (`height` is a number of policies which we squash into one) but current implementation allows for unnormalized `width` (number of CSPSources in a source list), i.e. there might be repetitions or things like "https https://example.com" (where obviously the former alone would be sufficient). If we try to normalize `width`, it would be less efficient & would require adding some data structure to keep track of csp source that we already added, which is quite unnecessary for subsumption. > https://codereview.chromium.org/2487983003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Sorry again for the delay. I've left some comments! https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/csp/CSPSource.h (right): https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/CSPSource.h:43: void addToMap(HashMap<String, CSPSource*>&); On 2016/11/19 at 18:34:32, amalika wrote: > Would it be better if I change it to be a lambda function inside `getIntersectSchemesOnly` instead since it is the only use case? I think you can just stuff it into `SourceListDirective.cpp` as a static method in an anonymous namespace. Pass in a CSPSource, read the scheme via the new public method you just added, and add it to the map there. Adding it to CSPSource directly seems like overkill. https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp (right): https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:573: HeapVector<Member<CSPSource>> other) { Why is this a vector of `Member<CSPSource>` and not `CSPSource*`? https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:574: HashMap<String, CSPSource *> schemesA, intersect; Style nits: 1. Please don't use `,` to declare more than one variable on a line. 2. Please declare |intersect| above the next `for` loop, as it's not used until then. https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:596: HeapVector<Member<CSPSource>> other) { Why is this a vector of `Member<CSPSource>` and not `CSPSource*`? https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:601: it != schemesMap.end(); ++it) { `const auto&`. It's clear from context that it's going to be an iterator. https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:602: // We do not "https:" if "http:" is present. 1. I don't understand this comment. 2. If you're special-casing `http`, shouldn't you special-case `ws` as well? https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:608: if (schemesMap.contains(sourceA->getScheme())) Should you check |schemesMap| here, or |normalized|? https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:611: Member<CSPSource> match(nullptr), localMatch(nullptr); Style: Two lines, no comma. Also, `CSPSource*` is fine. This isn't a member variable. https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:614: // matching scheme source epxression. Nit: s/epxression/expression/ https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:615: if (schemesMap.contains(sourceB->getScheme())) Should you check |schemesMap| here, or |normalized|?
Patchset #3 (id:120001) has been deleted
https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp (right): https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:608: if (schemesMap.contains(sourceA->getScheme())) On 2016/11/23 at 09:19:18, Mike West (slow) wrote: > Should you check |schemesMap| here, or |normalized|? |schemesMap| because we want to skip scheme sources that were already added in the iteration above through schemesMap. In general, we want to add all the scheme source before iterating over here because we might never end up adding scheme source from B. https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:615: if (schemesMap.contains(sourceB->getScheme())) On 2016/11/23 at 09:19:18, Mike West (slow) wrote: > Should you check |schemesMap| here, or |normalized|? As above, I was intending on checking |schemesMap|. Should we also check if |normalized| contains the source already as well? In current implementation, we normalize the `height` (`height` is a number of policies which we squash into one) but allow for unnormalized `width` (number of CSPSources in a source list), i.e. there might be repetitions or things like "https https://example.com" (where obviously the former alone would be sufficient). If we try to normalize `width`, it would be less efficient since most CSPSource will be added only once but having repetitions is not harmful. But let me know if it is better to add checks?
Would you mind responding to comments with "done" or something similar? It's hard to figure out what your patch addresses otherwise. I think you didn't respond to the `Member<CSPSource>` question, for instance. https://codereview.chromium.org/2487983003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp (right): https://codereview.chromium.org/2487983003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp:26: const int port; Nit: Can you add a comment about the meaning of `0`? https://codereview.chromium.org/2487983003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp (right): https://codereview.chromium.org/2487983003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:624: for (auto it = schemesMap.begin(); it != schemesMap.end(); ++it) { `auto&`? Or do you need a copy? (Can this not be `const`?)
On 2016/11/24 at 14:00:54, Mike West (slow) wrote: > Would you mind responding to comments with "done" or something similar? It's hard to figure out what your patch addresses otherwise. > > I think you didn't respond to the `Member<CSPSource>` question, for instance. (That said, I _think_ this is my only outstanding question, nits below notwithstanding.) > > https://codereview.chromium.org/2487983003/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp (right): > > https://codereview.chromium.org/2487983003/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp:26: const int port; > Nit: Can you add a comment about the meaning of `0`? > > https://codereview.chromium.org/2487983003/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp (right): > > https://codereview.chromium.org/2487983003/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:624: for (auto it = schemesMap.begin(); it != schemesMap.end(); ++it) { > `auto&`? Or do you need a copy? (Can this not be `const`?)
That's very strange as I had answered with an exact error. I might have mistakenly not saved a response. I tried doing CSPSource* instead of a Member but it seems like there is no viable conversion from 'HeapVector<Member<blink::CSPSource>>' to 'HeapVector<blink::CSPSource *>'. However, in `subsume` method in SourceListDirective we retrieve that heap vector of member csp sources of the Embedding-CSP: ` HeapVector<Member<CSPSource>> normalizedA = other[0]->m_list; ` So if we change getCSPSources to return HeapVector of CPSource*, when we compare `firstSubsumesSecond` of Embeddign-CSP and returned CSP, the method would be of different types: `firstSubsumesSecond(HeapVector<Member<CSPSource>>, HeapVector<CSPSource*>)`. We could get HeapVector<CSPSource*> for Embedding-CSP by creating a new HeapVector and appending all the items from m_list, if it is preferred? https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/csp/CSPSource.h (right): https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/CSPSource.h:43: void addToMap(HashMap<String, CSPSource*>&); On 2016/11/23 at 09:19:18, Mike West (slow) wrote: > On 2016/11/19 at 18:34:32, amalika wrote: > > Would it be better if I change it to be a lambda function inside `getIntersectSchemesOnly` instead since it is the only use case? > > I think you can just stuff it into `SourceListDirective.cpp` as a static method in an anonymous namespace. Pass in a CSPSource, read the scheme via the new public method you just added, and add it to the map there. Adding it to CSPSource directly seems like overkill. perfect! addressed. https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp (right): https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:573: HeapVector<Member<CSPSource>> other) { On 2016/11/23 at 09:19:18, Mike West (slow) wrote: > Why is this a vector of `Member<CSPSource>` and not `CSPSource*`? Will be answered in overall comments. https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:574: HashMap<String, CSPSource *> schemesA, intersect; On 2016/11/23 at 09:19:18, Mike West (slow) wrote: > Style nits: > > 1. Please don't use `,` to declare more than one variable on a line. > > 2. Please declare |intersect| above the next `for` loop, as it's not used until then. Done. https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:596: HeapVector<Member<CSPSource>> other) { On 2016/11/23 at 09:19:18, Mike West (slow) wrote: > Why is this a vector of `Member<CSPSource>` and not `CSPSource*`? Similarly as above for `getIntersectCSPSourcesSchemes`. https://codereview.chromium.org/2487983003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:611: Member<CSPSource> match(nullptr), localMatch(nullptr); On 2016/11/23 at 09:19:18, Mike West (slow) wrote: > Style: Two lines, no comma. > > Also, `CSPSource*` is fine. This isn't a member variable. Addressed! https://codereview.chromium.org/2487983003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp (right): https://codereview.chromium.org/2487983003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp:26: const int port; On 2016/11/24 at 14:00:54, Mike West (slow) wrote: > Nit: Can you add a comment about the meaning of `0`? Added! https://codereview.chromium.org/2487983003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp (right): https://codereview.chromium.org/2487983003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:624: for (auto it = schemesMap.begin(); it != schemesMap.end(); ++it) { On 2016/11/24 at 14:00:54, Mike West (slow) wrote: > `auto&`? Or do you need a copy? (Can this not be `const`?) It cannot be `const` since we are incrementing the iterator on each iteration. I just tried `auto&` and got ` error: non-const lvalue reference to type 'HashTableIteratorAdapter<...>' cannot bind to a temporary of type 'HashTableIteratorAdapter<...>' `
On 2016/11/24 at 14:50:33, amalika wrote: > That's very strange as I had answered with an exact error. I might have mistakenly not saved a response. > > I tried doing CSPSource* instead of a Member but it seems like there is no viable conversion from 'HeapVector<Member<blink::CSPSource>>' to 'HeapVector<blink::CSPSource *>'. > > > However, in `subsume` method in SourceListDirective we retrieve that heap vector of member csp sources of the Embedding-CSP: > ` HeapVector<Member<CSPSource>> normalizedA = other[0]->m_list; ` > > So if we change getCSPSources to return HeapVector of CPSource*, when we compare `firstSubsumesSecond` of Embeddign-CSP and returned CSP, the method would be of different types: > `firstSubsumesSecond(HeapVector<Member<CSPSource>>, HeapVector<CSPSource*>)`. > > > We could get HeapVector<CSPSource*> for Embedding-CSP by creating a new HeapVector and appending all the items from m_list, if it is preferred? I see. I remember something that conversation now, sorry if I'm changing my mind on you. I think we're going to need to change this. Let's land this patch with `Member<>`, and poke at folks in oilpan-reviews@ for help determining the impact of the current code. I don't think it's the right way to do things, but they'll know for sure.
LGTM % the `Member<>` thing, which I think we'll need to revisit in a subsequent patch.
The CQ bit was checked by amalika@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by amalika@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1480071805952270,
"parent_rev": "c7424b7e2e1f3b1ef034141de14e560eb89b0b51", "commit_rev":
"493d8dd6958119125aff28ef4417a21c31f474d3"}
Message was sent while issue was closed.
Committed patchset #4 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Part 2.3: Is policy list subsumed under subsuming policy? This is part of an experimental feature Embedding-CSP. This patch considers schemes when finding an `effective` vector of CSPSources. Previously, we handled scheme to host and host to host source expressions. This CL considers specifically scheme to scheme source expressions. Example 1: A: http: B: http://example.com Then the resulting intersaction should be B. Example 2: A: https: http://example.com B: http://example.com http: Then the result should be `http://example.com https:`, i.e. the former should not be upgraded to "https" Moreover, we should also NOT stop when we find a match. Example 3: A: https://example.com http://example.com/foo B: http://example.com/foo https://example.com If we stop based on first match, we would get: `https://example.com/foo http://example.com/foo` However, the correct result should be: `https://example.com http://example.com/foo` since the order of CSPSources should not matter. BUG=647588 ========== to ========== Part 2.3: Is policy list subsumed under subsuming policy? This is part of an experimental feature Embedding-CSP. This patch considers schemes when finding an `effective` vector of CSPSources. Previously, we handled scheme to host and host to host source expressions. This CL considers specifically scheme to scheme source expressions. Example 1: A: http: B: http://example.com Then the resulting intersaction should be B. Example 2: A: https: http://example.com B: http://example.com http: Then the result should be `http://example.com https:`, i.e. the former should not be upgraded to "https" Moreover, we should also NOT stop when we find a match. Example 3: A: https://example.com http://example.com/foo B: http://example.com/foo https://example.com If we stop based on first match, we would get: `https://example.com/foo http://example.com/foo` However, the correct result should be: `https://example.com http://example.com/foo` since the order of CSPSources should not matter. BUG=647588 Committed: https://crrev.com/cb5cc2c4d0e9f56a7b408d33d264b7ca0c864a18 Cr-Commit-Position: refs/heads/master@{#434477} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/cb5cc2c4d0e9f56a7b408d33d264b7ca0c864a18 Cr-Commit-Position: refs/heads/master@{#434477}
Message was sent while issue was closed.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
Message was sent while issue was closed.
https://codereview.chromium.org/2487983003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/csp/SourceListDirective.h (right): https://codereview.chromium.org/2487983003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/SourceListDirective.h:91: static void addSourceToMap(HashMap<String, CSPSource*>&, CSPSource*); HeapHashMap<String, Member<CSPSource>> is what's preferably used here, as the map's pointer values are GC references.
Message was sent while issue was closed.
On 2016/12/01 at 16:15:53, sigbjornf wrote: > https://codereview.chromium.org/2487983003/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/frame/csp/SourceListDirective.h (right): > > https://codereview.chromium.org/2487983003/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/frame/csp/SourceListDirective.h:91: static void addSourceToMap(HashMap<String, CSPSource*>&, CSPSource*); > HeapHashMap<String, Member<CSPSource>> is what's preferably used here, as the map's pointer values are GC references. Thank you for looking over it! This is very helpful. On a side note, we came across quite a strange error. In this CL: https://codereview.chromium.org/2519103005, win_chroimium_rel_ng was the only one failing. With SCOPED_TRACE (patch 8), it seems like the test variable `test.originB` does not hold an expected value. For example, if the intended value is "https://other-origin.test/", the printed values varies and for example is "http://example.test/folder/" (which is only ever mentioned in initializing std::vector for `policiesB`.) We were worried that these references to heap members in `subsumes` functions could somehow causing these inconsistencies? What seemed to fix the test is switching away from WTF::String() to `const char *`, but it is not clear why this fixed the issue... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
