|
|
DescriptionChange to String-based CSPSource matching functions.
Currently, CSPSource's matching functions match the relevant
component against a KURL. We need to match against Strings
in order to implement some new features, so this patch
converts the comparisons in preparation for that work.
BUG=647588
Committed: https://crrev.com/8b7fe99a65e3478fed6f8a4a6f06213b18147c93
Cr-Commit-Position: refs/heads/master@{#427997}
Patch Set 1 : Removing equalIgnoringCase #
Total comments: 3
Patch Set 2 : Removing KURL equivalents #
Total comments: 1
Patch Set 3 : Removing KURL equivalents #
Total comments: 2
Patch Set 4 : Adding DCHECKs for lower case on protocol/host #
Total comments: 2
Patch Set 5 : Adding a comment #
Total comments: 1
Patch Set 6 : Removing lower and dcheck for host #
Dependent Patchsets: Messages
Total messages: 33 (18 generated)
Patchset #1 (id:1) has been deleted
amalika@google.com changed reviewers: + mkwst@chromium.org
https://codereview.chromium.org/2456493002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): https://codereview.chromium.org/2456493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:65: bool equalHosts = equalIgnoringCase(host, m_host); Can we assume hosts should also be lower case?
The CQ bit was checked by mkwst@chromium.org 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...
On 2016/10/26 at 12:18:08, amalika wrote: > https://codereview.chromium.org/2456493002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): > > https://codereview.chromium.org/2456493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:65: bool equalHosts = equalIgnoringCase(host, m_host); > Can we assume hosts should also be lower case? Let's say yes, and see what explodes? :)
Thanks! https://codereview.chromium.org/2456493002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): https://codereview.chromium.org/2456493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:24: m_scheme(scheme), I'm actually wrong about parsing here: we don't normalize like I thought we did. So let's do that here: `m_scheme(scheme.lower())`, `m_host(host.lower())`. https://codereview.chromium.org/2456493002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPSource.h (right): https://codereview.chromium.org/2456493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.h:47: bool portMatches(int, const String&) const; I think we can actually just remove the KURL variants here; they're only called from `matches()`, and you can just update that callsite to pass in the relevant bits of the URL.
Also: please change the CL description to fit the general pattern: ``` Title of ~60 characters. Slightly longer description that wraps somewhere around 70ish characters like this did. Though I didn't count, I just eyeballed it. Was I close? Who knows. ``` For this patch, that might be: ``` Change to String-based CSPSource matching functions. Currently, CSPSource's matching functions match the relevant component against a KURL. We need to match against Strings in order to implement some new features, so this patch converts the comparisons in preparation for that work. BUG=whatever ```
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Splitting *Matches functions in CSPSource to accept strings as inputs BUG=647588 ========== to ========== Change to String-based CSPSource matching functions. Currently, CSPSource's matching functions match the relevant component against a KURL. We need to match against Strings in order to implement some new features, so this patch converts the comparisons in preparation for that work. BUG=647588 ==========
The CQ bit was checked by amalika@google.com
https://codereview.chromium.org/2456493002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): https://codereview.chromium.org/2456493002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:58: bool equalHosts = (m_host == host.lower()); it seems like when creating KURL it does not apply the lower case https://codereview.chromium.org/2456493002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): https://codereview.chromium.org/2456493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:45: bool CSPSource::schemeMatches(const String& protocol) const { Do we have to apply lower case on protocol?
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
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...
On 2016/10/26 at 13:03:33, amalika wrote: > https://codereview.chromium.org/2456493002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): > > https://codereview.chromium.org/2456493002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:58: bool equalHosts = (m_host == host.lower()); > it seems like when creating KURL it does not apply the lower case Hrm. That's surprising. In that case, please add some tests. :)
https://codereview.chromium.org/2456493002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): https://codereview.chromium.org/2456493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:45: bool CSPSource::schemeMatches(const String& protocol) const { On 2016/10/26 at 13:03:33, amalika wrote: > Do we have to apply lower case on protocol? Protocols should be lower-case, yes. DCHECK, please. :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run passed! https://codereview.chromium.org/2456493002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): https://codereview.chromium.org/2456493002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:55: DCHECK_EQ(host, host.lower()); Actually, I conducted an experiment and indeed it seems like hosts on KURLs are changed to lower case! So I added this DCHECK as for protocols above.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2456493002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPSource.h (right): https://codereview.chromium.org/2456493002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPSource.h:41: bool portMatches(int, const String&) const; Nit: Can you add a comment (and argument name) for the protocol in `portMatches()`? I had to skim through the code to understand why you needed to add it. Otherwise, LGTM!
+csharrison@, as https://codereview.chromium.org/2447293002 is relevant here. :) https://codereview.chromium.org/2456493002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): https://codereview.chromium.org/2456493002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:55: DCHECK_EQ(host, host.lower()); In https://codereview.chromium.org/2447293002, csharrison pointed out that `lower()` is incorrect for hosts (as that might actually change their meaning, because we canonicalize escape characters to uppercase). So let's drop this DCHECK, as well as the call to `.lower()` in the constructor. When his patch lands, we might want to call `SecurityOrigin::canonicalizeHost()`. I don't remember if we do that work when parsing a policy in the first place. `==` is still the right check, however. :)
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 Link to the patchset: https://codereview.chromium.org/2456493002/#ps120001 (title: "Removing lower and dcheck for host")
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 #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Change to String-based CSPSource matching functions. Currently, CSPSource's matching functions match the relevant component against a KURL. We need to match against Strings in order to implement some new features, so this patch converts the comparisons in preparation for that work. BUG=647588 ========== to ========== Change to String-based CSPSource matching functions. Currently, CSPSource's matching functions match the relevant component against a KURL. We need to match against Strings in order to implement some new features, so this patch converts the comparisons in preparation for that work. BUG=647588 Committed: https://crrev.com/8b7fe99a65e3478fed6f8a4a6f06213b18147c93 Cr-Commit-Position: refs/heads/master@{#427997} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/8b7fe99a65e3478fed6f8a4a6f06213b18147c93 Cr-Commit-Position: refs/heads/master@{#427997} |