Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1974413003:
Whitelist hangouts.google.com further to allow video effects plugin access. (Closed)
DescriptionUrgent: whitelist hangouts.google.com further (should've been done in issue 1552383002 from Dec 2015, but...) to allow video effects plugin access; tag on meet.google.com for a similar purpose; tax: refactor whitelist checks into separate thing.
R=noahric@chromium.org,sky@chromium.org,jschuh@chromium.org,tommi@chromium.org,tsepez@chromium.org
BUG=614062
Committed: https://crrev.com/81dcbd6123178d58c5edc53d1453e03f9afa31ca
Cr-Commit-Position: refs/heads/master@{#395172}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Moving to single domain check #
Total comments: 4
Patch Set 3 : Inlining url.host() #Patch Set 4 : Define string constant only when it'll be used somewhere #Patch Set 5 : Fix 'unreachable code' error #
Total comments: 4
Patch Set 6 : Move whitelisting logic and most of the tests out into a separate thing #Patch Set 7 : Include the added files #Patch Set 8 : Swapping comments #
Total comments: 17
Patch Set 9 : Addressing misc review comments #
Total comments: 6
Patch Set 10 : Use static methods instead of namespace #
Messages
Total messages: 86 (31 generated)
The CQ bit was checked by qaz@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1974413003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1974413003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
+jschuh All of these checks should be structured in its own file so their can be a set of security reviewers.
On 2016/05/13 23:13:26, sky wrote: > +jschuh > All of these checks should be structured in its own file so their can be a set > of security reviewers. I don't think we need these checks at all at this time. But for now, I'd like to get this in to fix a more urgent issue, and someone from Chrome could refactor this later... Thanks. -alex
On 2016/05/14 00:08:34, AlexZ wrote: > On 2016/05/13 23:13:26, sky wrote: > > +jschuh > > All of these checks should be structured in its own file so their can be a set > > of security reviewers. > > I don't think we need these checks at all at this time. But for now, I'd like to > get this in to fix a more urgent issue, and someone from Chrome could refactor > this later... > > Thanks. > -alex Oh forgot to mention the context: this was originally intended to be fixed by https://codereview.chromium.org/1552383002 which turned out to be ineffective.
tommi@chromium.org changed reviewers: + jschuh@chromium.org, tommi@chromium.org
drive-by and adding jschuh to the reviewer list. https://codereview.chromium.org/1974413003/diff/1/chrome/renderer/chrome_cont... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/1974413003/diff/1/chrome/renderer/chrome_cont... chrome/renderer/chrome_content_renderer_client.cc:931: (base::EndsWith(app_url_host, "hangouts.google.com", It looks like this change doesn't make a difference - is it just moving this check up or am I missing what the change is? https://codereview.chromium.org/1974413003/diff/1/chrome/renderer/chrome_cont... chrome/renderer/chrome_content_renderer_client.cc:1238: (base::EndsWith(url_host, "hangouts.google.com", I guess this is the real change. Since this looks like the same list as above and we missed updating it before, can you extract the list out to where other whitelists are at the top of the file, e.g.: const char* const kPredefinedAllowedPepperMediaStreamOrigins[] = { "hangouts.google.com", "talkgadget.google.com", "plus.google.com", "plus.sandbox.google.com", }; and then update this part and the one above to something like: // Allow access for tests. if (base::CommandLine::ForCurrentProcess()->HasSwitch( switches::kEnablePepperTesting)) { return true; } if (!url.SchemeIs("https")) <- should we be using url::kHttpsScheme? return false; if (!base::StartsWith(url.path(), "/hangouts/", base::CompareCase::INSENSITIVE_ASCII)) { return false; } std::string url_host = url.host(); for (const char* entry : kPredefinedAllowedPepperMediaStreamOrigins) { if (base::EndsWith(url_host, entry, base::CompareCase::INSENSITIVE_ASCII) return true; } return false;
Why do we need a list anyways? Why can't we just test against ".google.com"? What's the security concern?
On 2016/05/17 15:56:23, AlexZ wrote: > Why do we need a list anyways? Why can't we just test against ".google.com"? > What's the security concern? That would work too.
ptal https://codereview.chromium.org/1974413003/diff/1/chrome/renderer/chrome_cont... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/1974413003/diff/1/chrome/renderer/chrome_cont... chrome/renderer/chrome_content_renderer_client.cc:931: (base::EndsWith(app_url_host, "hangouts.google.com", On 2016/05/16 19:51:56, tommi-chrömium wrote: > It looks like this change doesn't make a difference - is it just moving this > check up or am I missing what the change is? Was just making the two lists the same. Moving to single domain check. https://codereview.chromium.org/1974413003/diff/1/chrome/renderer/chrome_cont... chrome/renderer/chrome_content_renderer_client.cc:1238: (base::EndsWith(url_host, "hangouts.google.com", Moving to single domain check.
Sorry I'm having problems committing / uploading my change... Wrestling with git.
Ok now it's ready for review. Thanks.
The CQ bit was checked by qaz@google.com to run a CQ dry run
On 2016/05/17 16:55:01, AlexZ wrote: > Sorry I'm having problems committing / uploading my change... Wrestling with > git. While you're at it, can you update the description? Otherwise, I think it makes sense to limit to google.com like the extension.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1974413003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1974413003/20001
Description was changed from ========== Whitelist hangouts.google.com further to allow video effects plugin access. R=noahric@chromium.org,sky@chromium.org BUG= ========== to ========== Allow video effects plugin access for all of google.com. R=noahric@chromium.org,sky@chromium.org,jschuh@chromium.org,tommi@chromium.org BUG= ==========
Updated description. Thanks.
lgtm https://codereview.chromium.org/1974413003/diff/20001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/1974413003/diff/20001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:1233: std::string url_host = url.host(); I don't think you need this copy now.
https://codereview.chromium.org/1974413003/diff/20001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/1974413003/diff/20001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:1233: std::string url_host = url.host(); On 2016/05/17 17:10:44, tommi-chrömium wrote: > I don't think you need this copy now. It's still in use at line 1235 below.
https://codereview.chromium.org/1974413003/diff/20001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/1974413003/diff/20001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:1233: std::string url_host = url.host(); On 2016/05/17 17:20:24, AlexZ wrote: > On 2016/05/17 17:10:44, tommi-chrömium wrote: > > I don't think you need this copy now. > > It's still in use at line 1235 below. I mean, you could call url.host() inline there and avoid an extra string copy.
Thanks. https://codereview.chromium.org/1974413003/diff/20001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/1974413003/diff/20001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:1233: std::string url_host = url.host(); On 2016/05/17 17:25:38, tommi-chrömium wrote: > On 2016/05/17 17:20:24, AlexZ wrote: > > On 2016/05/17 17:10:44, tommi-chrömium wrote: > > > I don't think you need this copy now. > > > > It's still in use at line 1235 below. > > I mean, you could call url.host() inline there and avoid an extra string copy. Done.
The CQ bit was checked by qaz@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1974413003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1974413003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Updated: Define string constant only when it'll be used somewhere
The CQ bit was checked by qaz@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1974413003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1974413003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by qaz@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1974413003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1974413003/80001
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 qaz@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org Link to the patchset: https://codereview.chromium.org/1974413003/#ps80001 (title: "Fix 'unreachable code' error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1974413003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1974413003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Need owner's approval. Thanks.
On 2016/05/14 00:08:34, AlexZ wrote: > On 2016/05/13 23:13:26, sky wrote: > > +jschuh > > All of these checks should be structured in its own file so their can be a set > > of security reviewers. > > I don't think we need these checks at all at this time. But for now, I'd like to > get this in to fix a more urgent issue, and someone from Chrome could refactor > this later... > > Thanks. > -alex If you don't want to move the checks as I suggested, then at least get someone from security to review. If jschuh isn't available perhaps someone else from security could take a look.
On 2016/05/17 22:47:00, sky wrote: > On 2016/05/14 00:08:34, AlexZ wrote: > > On 2016/05/13 23:13:26, sky wrote: > > > +jschuh > > > All of these checks should be structured in its own file so their can be a > set > > > of security reviewers. > > > > I don't think we need these checks at all at this time. But for now, I'd like > to > > get this in to fix a more urgent issue, and someone from Chrome could refactor > > this later... > > > > Thanks. > > -alex > > If you don't want to move the checks as I suggested, then at least get someone > from security to review. If jschuh isn't available perhaps someone else from > security could take a look. Sorry I should've mentioned that I'm not at all familiar with the Chromium codebase: learning how to set up a Chromium development environment has been an interesting experience... If this issue hadn't been urgent, then I would've left the bug in the hands of someone actually know what they are doing to do whatever refactoring that's appropriate. I don't understand why this change specifically needs additional security review, since the basic logic has been here for a long time and I'm just relaxing the host restriction, but still within the scope of the google.com domain. If it really needs additional security review, shouldn't someone familiar with the codebase do it separately? Thanks.
sky@chromium.org changed reviewers: + tsepez@chromium.org
I hope you can appreciate that changes to 'relax' security checks requires extra scrutiny. I'm only asking that you find someone from the security team to look this over and make sure they are ok with it. jschuh is such a person, but he appears to be slow to respond. +tsepez to do the review.
https://codereview.chromium.org/1974413003/diff/80001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (left): https://codereview.chromium.org/1974413003/diff/80001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:915: (base::EndsWith(app_url_host, "plus.google.com", Pre-existing: EndsWith isn't ideal for these kinds of comparisions unless there's a leading "." in the literal. Otherwise, things like fooplus.google.com would match. https://codereview.chromium.org/1974413003/diff/80001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/1974413003/diff/80001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:189: const char* const kGoogleComDomainSuffix = ".google.com"; No, we really don't want to whitelist all of google.com, you'll add another host to the comparision at line 919
(In case it isn't obvious, the reason we don't want to whitelist all of google is that an XSS anywhere on google is now good enough to get access to the feature, whereas in the past, you'd have to find an XSS on a specific google subdomain, which is likely to be much harder).
And now I'm going require you to move the checks to a new file that can be owned by security folks such as Tom. -Scott On Tue, May 17, 2016 at 4:23 PM, <tsepez@chromium.org> wrote: > > https://codereview.chromium.org/1974413003/diff/80001/chrome/renderer/chrome_... > File chrome/renderer/chrome_content_renderer_client.cc (left): > > https://codereview.chromium.org/1974413003/diff/80001/chrome/renderer/chrome_... > chrome/renderer/chrome_content_renderer_client.cc:915: > (base::EndsWith(app_url_host, "plus.google.com", > Pre-existing: EndsWith isn't ideal for these kinds of comparisions > unless there's a leading "." in the literal. Otherwise, things like > fooplus.google.com would match. > > https://codereview.chromium.org/1974413003/diff/80001/chrome/renderer/chrome_... > File chrome/renderer/chrome_content_renderer_client.cc (right): > > https://codereview.chromium.org/1974413003/diff/80001/chrome/renderer/chrome_... > chrome/renderer/chrome_content_renderer_client.cc:189: const char* const > kGoogleComDomainSuffix = ".google.com"; > No, we really don't want to whitelist all of google.com, you'll add > another host to the comparision at line 919 > > https://codereview.chromium.org/1974413003/ -- 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.
On 2016/05/17 23:14:21, sky wrote: > I hope you can appreciate that changes to 'relax' security checks requires extra > scrutiny. I'm only asking that you find someone from the security team to look > this over and make sure they are ok with it. jschuh is such a person, but he > appears to be slow to respond. > > +tsepez to do the review. :) I did understand one should not randomly relax security checks. Thank you for finding a security person since I had no idea who they could be.
What would be a good place? Do you have an example? Thanks. On Tue, May 17, 2016 at 4:35 PM, Scott Violet <sky@chromium.org> wrote: > And now I'm going require you to move the checks to a new file that > can be owned by security folks such as Tom. > > -Scott > > On Tue, May 17, 2016 at 4:23 PM, <tsepez@chromium.org> wrote: > > > > > https://codereview.chromium.org/1974413003/diff/80001/chrome/renderer/chrome_... > > File chrome/renderer/chrome_content_renderer_client.cc (left): > > > > > https://codereview.chromium.org/1974413003/diff/80001/chrome/renderer/chrome_... > > chrome/renderer/chrome_content_renderer_client.cc:915: > > (base::EndsWith(app_url_host, "plus.google.com", > > Pre-existing: EndsWith isn't ideal for these kinds of comparisions > > unless there's a leading "." in the literal. Otherwise, things like > > fooplus.google.com would match. > > > > > https://codereview.chromium.org/1974413003/diff/80001/chrome/renderer/chrome_... > > File chrome/renderer/chrome_content_renderer_client.cc (right): > > > > > https://codereview.chromium.org/1974413003/diff/80001/chrome/renderer/chrome_... > > chrome/renderer/chrome_content_renderer_client.cc:189: const char* const > > kGoogleComDomainSuffix = ".google.com"; > > No, we really don't want to whitelist all of google.com, you'll add > > another host to the comparision at line 919 > > > > https://codereview.chromium.org/1974413003/ > -- Q. Alex Zhao | Video Hangouts Frontend SWE | qaz@google.com | (206) 925-3176 -- 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.
How about naming the file app_categorizer in the same directory with the functions, IsPhotosApp(const GURL&) and IsHangoutsApp(const GURL&). On Tue, May 17, 2016 at 5:26 PM, Q. Alex Zhao <qaz@google.com> wrote: > What would be a good place? Do you have an example? > > Thanks. > > On Tue, May 17, 2016 at 4:35 PM, Scott Violet <sky@chromium.org> wrote: >> >> And now I'm going require you to move the checks to a new file that >> can be owned by security folks such as Tom. >> >> -Scott >> >> On Tue, May 17, 2016 at 4:23 PM, <tsepez@chromium.org> wrote: >> > >> > >> > https://codereview.chromium.org/1974413003/diff/80001/chrome/renderer/chrome_... >> > File chrome/renderer/chrome_content_renderer_client.cc (left): >> > >> > >> > https://codereview.chromium.org/1974413003/diff/80001/chrome/renderer/chrome_... >> > chrome/renderer/chrome_content_renderer_client.cc:915: >> > (base::EndsWith(app_url_host, "plus.google.com", >> > Pre-existing: EndsWith isn't ideal for these kinds of comparisions >> > unless there's a leading "." in the literal. Otherwise, things like >> > fooplus.google.com would match. >> > >> > >> > https://codereview.chromium.org/1974413003/diff/80001/chrome/renderer/chrome_... >> > File chrome/renderer/chrome_content_renderer_client.cc (right): >> > >> > >> > https://codereview.chromium.org/1974413003/diff/80001/chrome/renderer/chrome_... >> > chrome/renderer/chrome_content_renderer_client.cc:189: const char* const >> > kGoogleComDomainSuffix = ".google.com"; >> > No, we really don't want to whitelist all of google.com, you'll add >> > another host to the comparision at line 919 >> > >> > https://codereview.chromium.org/1974413003/ > > > > > -- > Q. Alex Zhao | Video Hangouts Frontend SWE | qaz@google.com | (206) 925-3176 -- 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.
Description was changed from ========== Allow video effects plugin access for all of google.com. R=noahric@chromium.org,sky@chromium.org,jschuh@chromium.org,tommi@chromium.org BUG= ========== to ========== Urgent: whitelist hangouts.google.com further (should've been done in https://codereview.chromium.org/1552383002, but...) to allow video effects plugin access; tag on meet.google.com for a similar purpose; refactor whitelist checks into separate class. R=noahric@chromium.org,sky@chromium.org,jschuh@chromium.org,tommi@chromium.or... BUG= ==========
Description was changed from ========== Urgent: whitelist hangouts.google.com further (should've been done in https://codereview.chromium.org/1552383002, but...) to allow video effects plugin access; tag on meet.google.com for a similar purpose; refactor whitelist checks into separate class. R=noahric@chromium.org,sky@chromium.org,jschuh@chromium.org,tommi@chromium.or... BUG= ========== to ========== Urgent: whitelist hangouts.google.com further (should've been done in https://codereview.chromium.org/1552383002, but...) to allow video effects plugin access; tag on meet.google.com for a similar purpose; tax: refactor whitelist checks into separate class. R=noahric@chromium.org,sky@chromium.org,jschuh@chromium.org,tommi@chromium.or... BUG= ==========
Description was changed from ========== Urgent: whitelist hangouts.google.com further (should've been done in https://codereview.chromium.org/1552383002, but...) to allow video effects plugin access; tag on meet.google.com for a similar purpose; tax: refactor whitelist checks into separate class. R=noahric@chromium.org,sky@chromium.org,jschuh@chromium.org,tommi@chromium.or... BUG= ========== to ========== Urgent: whitelist hangouts.google.com further (should've been done in issue 1552383002 from Dec 2015, but...) to allow video effects plugin access; tag on meet.google.com for a similar purpose; tax: refactor whitelist checks into separate class. R=noahric@chromium.org,sky@chromium.org,jschuh@chromium.org,tommi@chromium.or... BUG= ==========
PTAL. Thanks. https://codereview.chromium.org/1974413003/diff/80001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (left): https://codereview.chromium.org/1974413003/diff/80001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:915: (base::EndsWith(app_url_host, "plus.google.com", This should actually be an equality test. https://codereview.chromium.org/1974413003/diff/80001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/1974413003/diff/80001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:189: const char* const kGoogleComDomainSuffix = ".google.com"; On 2016/05/17 23:23:17, Tom Sepez wrote: > No, we really don't want to whitelist all of http://google.com, you'll add another host > to the comparision at line 919 Done.
Description was changed from ========== Urgent: whitelist hangouts.google.com further (should've been done in issue 1552383002 from Dec 2015, but...) to allow video effects plugin access; tag on meet.google.com for a similar purpose; tax: refactor whitelist checks into separate class. R=noahric@chromium.org,sky@chromium.org,jschuh@chromium.org,tommi@chromium.or... BUG= ========== to ========== Urgent: whitelist hangouts.google.com further (should've been done in issue 1552383002 from Dec 2015, but...) to allow video effects plugin access; tag on meet.google.com for a similar purpose; tax: refactor whitelist checks into separate thing. R=noahric@chromium.org,sky@chromium.org,jschuh@chromium.org,tommi@chromium.or... BUG= ==========
The CQ bit was checked by qaz@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1974413003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1974413003/140001
https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/OWNERS File chrome/renderer/OWNERS (right): https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/OWNERS... chrome/renderer/OWNERS:6: per-file app_categorizer*=tsepez@chromium.org add per-file app_categorizer*=set noparent per-file app_categorizer*=jschuh@chromium.org per-file app_categorizer*=wfh@chromium.org https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/app_ca... File chrome/renderer/app_categorizer.cc (right): https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/app_ca... chrome/renderer/app_categorizer.cc:43: bool IsHangoutsUrl(const GURL& url) { I think we need to lowercase the domain in both of these before making the comparisons. https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/app_ca... chrome/renderer/app_categorizer.cc:45: base::StartsWith(url.path(), "/hangouts/", This doesn't add much security as it turns out, since if I'm running JS under /non-hangouts/foo/path, I can open an iframe to /hangouts/ and then doc.write() into it via same origin policy. You can leave it or remove it as you like. https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/app_ca... chrome/renderer/app_categorizer.cc:61: (manifest_url_path.find("s2/oz/nacl/") == 1 || put in a leading "/" in your literal and check for == 0.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/app_ca... File chrome/renderer/app_categorizer.h (right): https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/app_ca... chrome/renderer/app_categorizer.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/app_ca... chrome/renderer/app_categorizer.h:8: #include "url/gurl.h" forward declare GURL. https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/app_ca... File chrome/renderer/app_categorizer_unittest.cc (right): https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/app_ca... chrome/renderer/app_categorizer_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2016 (and no (c)). https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/app_ca... chrome/renderer/app_categorizer_unittest.cc:63: typedef testing::Test AppCategorizerTest; Remove this and use TEST rather than TEST_F
The CQ bit was checked by qaz@google.com to run a CQ dry run
PTAL. Thanks. https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/OWNERS File chrome/renderer/OWNERS (right): https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/OWNERS... chrome/renderer/OWNERS:6: per-file app_categorizer*=tsepez@chromium.org On 2016/05/18 23:42:19, Tom Sepez wrote: > add > > per-file app_categorizer*=set noparent > per-file mailto:app_categorizer*=jschuh@chromium.org > per-file mailto:app_categorizer*=wfh@chromium.org Done. https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/app_ca... File chrome/renderer/app_categorizer.cc (right): https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/app_ca... chrome/renderer/app_categorizer.cc:43: bool IsHangoutsUrl(const GURL& url) { On 2016/05/18 23:42:19, Tom Sepez wrote: > I think we need to lowercase the domain in both of these before making the > comparisons. No, GURL handles this. Added a test for this and it passed. https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/app_ca... chrome/renderer/app_categorizer.cc:45: base::StartsWith(url.path(), "/hangouts/", On 2016/05/18 23:42:19, Tom Sepez wrote: > This doesn't add much security as it turns out, since if I'm running JS under > /non-hangouts/foo/path, I can open an iframe to /hangouts/ and then doc.write() > into it via same origin policy. You can leave it or remove it as you like. Pre-existing. Leaving it as-is. https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/app_ca... chrome/renderer/app_categorizer.cc:61: (manifest_url_path.find("s2/oz/nacl/") == 1 || On 2016/05/18 23:42:19, Tom Sepez wrote: > put in a leading "/" in your literal and check for == 0. No idea why this was written this way but base::StartsWith makes more sense. https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/app_ca... File chrome/renderer/app_categorizer.h (right): https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/app_ca... chrome/renderer/app_categorizer.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/05/19 15:43:10, sky wrote: > no (c) Done. https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/app_ca... chrome/renderer/app_categorizer.h:8: #include "url/gurl.h" On 2016/05/19 15:43:10, sky wrote: > forward declare GURL. Done. https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/app_ca... File chrome/renderer/app_categorizer_unittest.cc (right): https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/app_ca... chrome/renderer/app_categorizer_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2016/05/19 15:43:10, sky wrote: > 2016 (and no (c)). Done. https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/app_ca... chrome/renderer/app_categorizer_unittest.cc:63: typedef testing::Test AppCategorizerTest; On 2016/05/19 15:43:10, sky wrote: > Remove this and use TEST rather than TEST_F Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1974413003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1974413003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/app_ca... File chrome/renderer/app_categorizer.cc (right): https://codereview.chromium.org/1974413003/diff/140001/chrome/renderer/app_ca... chrome/renderer/app_categorizer.cc:43: bool IsHangoutsUrl(const GURL& url) { On 2016/05/19 16:18:51, AlexZ wrote: > On 2016/05/18 23:42:19, Tom Sepez wrote: > > I think we need to lowercase the domain in both of these before making the > > comparisons. > > No, GURL handles this. Added a test for this and it passed. Acknowledged.
The CQ bit was checked by qaz@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org Link to the patchset: https://codereview.chromium.org/1974413003/#ps160001 (title: "Addressing misc review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1974413003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1974413003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Still need owner's approval. Thanks.
LGTM with the following changes https://codereview.chromium.org/1974413003/diff/160001/chrome/renderer/app_ca... File chrome/renderer/app_categorizer.cc (right): https://codereview.chromium.org/1974413003/diff/160001/chrome/renderer/app_ca... chrome/renderer/app_categorizer.cc:28: bool isInWhitelistedDomain( is->Is https://codereview.chromium.org/1974413003/diff/160001/chrome/renderer/app_ca... chrome/renderer/app_categorizer.cc:29: const GURL& url, const char* const domains[], size_t numberOfDomains) { number_of_domains https://codereview.chromium.org/1974413003/diff/160001/chrome/renderer/app_ca... File chrome/renderer/app_categorizer.h (right): https://codereview.chromium.org/1974413003/diff/160001/chrome/renderer/app_ca... chrome/renderer/app_categorizer.h:10: namespace app_categorizer { no namespace here (see https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/namesp... for details on chromes use of namespaces).
https://codereview.chromium.org/1974413003/diff/160001/chrome/renderer/app_ca... File chrome/renderer/app_categorizer.cc (right): https://codereview.chromium.org/1974413003/diff/160001/chrome/renderer/app_ca... chrome/renderer/app_categorizer.cc:28: bool isInWhitelistedDomain( On 2016/05/20 19:21:42, sky wrote: > is->Is Done. https://codereview.chromium.org/1974413003/diff/160001/chrome/renderer/app_ca... chrome/renderer/app_categorizer.cc:29: const GURL& url, const char* const domains[], size_t numberOfDomains) { On 2016/05/20 19:21:42, sky wrote: > number_of_domains Done. https://codereview.chromium.org/1974413003/diff/160001/chrome/renderer/app_ca... File chrome/renderer/app_categorizer.h (right): https://codereview.chromium.org/1974413003/diff/160001/chrome/renderer/app_ca... chrome/renderer/app_categorizer.h:10: namespace app_categorizer { On 2016/05/20 19:21:42, sky wrote: > no namespace here (see > https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/namesp... > for details on chromes use of namespaces). Done.
The CQ bit was checked by qaz@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, tommi@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1974413003/#ps180001 (title: "Use static methods instead of namespace")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1974413003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1974413003/180001
Message was sent while issue was closed.
Description was changed from ========== Urgent: whitelist hangouts.google.com further (should've been done in issue 1552383002 from Dec 2015, but...) to allow video effects plugin access; tag on meet.google.com for a similar purpose; tax: refactor whitelist checks into separate thing. R=noahric@chromium.org,sky@chromium.org,jschuh@chromium.org,tommi@chromium.or... BUG= ========== to ========== Urgent: whitelist hangouts.google.com further (should've been done in issue 1552383002 from Dec 2015, but...) to allow video effects plugin access; tag on meet.google.com for a similar purpose; tax: refactor whitelist checks into separate thing. R=noahric@chromium.org,sky@chromium.org,jschuh@chromium.org,tommi@chromium.or... BUG= ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Urgent: whitelist hangouts.google.com further (should've been done in issue 1552383002 from Dec 2015, but...) to allow video effects plugin access; tag on meet.google.com for a similar purpose; tax: refactor whitelist checks into separate thing. R=noahric@chromium.org,sky@chromium.org,jschuh@chromium.org,tommi@chromium.or... BUG= ========== to ========== Urgent: whitelist hangouts.google.com further (should've been done in issue 1552383002 from Dec 2015, but...) to allow video effects plugin access; tag on meet.google.com for a similar purpose; tax: refactor whitelist checks into separate thing. R=noahric@chromium.org,sky@chromium.org,jschuh@chromium.org,tommi@chromium.or... BUG= Committed: https://crrev.com/81dcbd6123178d58c5edc53d1453e03f9afa31ca Cr-Commit-Position: refs/heads/master@{#395172} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/81dcbd6123178d58c5edc53d1453e03f9afa31ca Cr-Commit-Position: refs/heads/master@{#395172}
Message was sent while issue was closed.
Description was changed from ========== Urgent: whitelist hangouts.google.com further (should've been done in issue 1552383002 from Dec 2015, but...) to allow video effects plugin access; tag on meet.google.com for a similar purpose; tax: refactor whitelist checks into separate thing. R=noahric@chromium.org,sky@chromium.org,jschuh@chromium.org,tommi@chromium.or... BUG= Committed: https://crrev.com/81dcbd6123178d58c5edc53d1453e03f9afa31ca Cr-Commit-Position: refs/heads/master@{#395172} ========== to ========== Urgent: whitelist hangouts.google.com further (should've been done in issue 1552383002 from Dec 2015, but...) to allow video effects plugin access; tag on meet.google.com for a similar purpose; tax: refactor whitelist checks into separate thing. R=noahric@chromium.org,sky@chromium.org,jschuh@chromium.org,tommi@chromium.or... BUG=614062 Committed: https://crrev.com/81dcbd6123178d58c5edc53d1453e03f9afa31ca Cr-Commit-Position: refs/heads/master@{#395172} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
