Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2495353003:
chrome.webRequest support for ExtensionSettings (Closed)
Descriptionchrome.webRequest support for ExtensionSettings
-If URL or Origin matches an ExtensionSettings protected host, the event will not be dispatched to that extension / app.
BUG=624649
Review-Url: https://codereview.chromium.org/2495353003
Cr-Commit-Position: refs/heads/master@{#478018}
Committed: https://chromium.googlesource.com/chromium/src/+/ea0116aad7867695cb2e438d3546f148ea271858
Patch Set 1 #Patch Set 2 : Log which webpages the embedded test server has served & query them. #
Total comments: 4
Patch Set 3 : Return ACCESS_DENIED, Remove excess check or URL #
Total comments: 4
Patch Set 4 : Clean up and update tests to work with ACCESS_DENIED #Patch Set 5 : runtime_blocked_hosts, runtime_allowed_hosts no longer accept paths, checks url::Origin rather than… #Patch Set 6 : Rebase #Patch Set 7 : Policy template translation doesn't like '&', switching to 'and'. Small fix to browser test. #
Total comments: 35
Patch Set 8 : Clean up tests & comments #
Total comments: 3
Patch Set 9 : Move policy changes to seperate CL #
Total comments: 22
Patch Set 10 : Moved policy check to WebRequestPermissions, code cleanup, rebase #
Total comments: 5
Patch Set 11 : Comment cleanup #Messages
Total messages: 82 (56 generated)
Description was changed from ========== chrome.webRequest support for ExtensionSettings -If URL or Origin matches an ExtensionSettings protected host, the event will not be dispatched to that extension / app. BUG=624649 ========== to ========== chrome.webRequest support for ExtensionSettings -If URL or Origin matches an ExtensionSettings protected host, the event will not be dispatched to that extension / app. BUG=624649 ==========
nrpeter@chromium.org changed reviewers: + asargent@chromium.org
nrpeter@chromium.org changed required reviewers: + asargent@chromium.org
nrpeter@chromium.org changed reviewers: + nrpeter@chromium.org
nrpeter@google.com changed reviewers: + rdevlin.cronin@chromium.org - asargent@chromium.org
nrpeter@google.com changed required reviewers: + rdevlin.cronin@chromium.org - asargent@chromium.org
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by nrpeter@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...
nrpeter@google.com changed reviewers: + dcheng@chromium.org
Rebased, ran a CQ dry-run which passed. Looks like we're ready for review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
(let me know if there's anything in particular you'd like me to review here; I don't think I'm an OWNER for any of the changed files)
https://codereview.chromium.org/2495353003/diff/40001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api.cc (right): https://codereview.chromium.org/2495353003/diff/40001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1507: if (extension->permissions_data()->IsRuntimeBlockedHost(url)) Why do we need this? WebRequestPermissions::CanExtensionAccessURL() checks PermissionsData::CanRunOnPage(), which is where we put the check for IsRuntimeBlockedHost(). https://codereview.chromium.org/2495353003/diff/40001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1508: access = PermissionsData::ACCESS_WITHHELD; We would want DENIED, right? WITHHELD will surface this to the user.
https://codereview.chromium.org/2495353003/diff/40001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api.cc (right): https://codereview.chromium.org/2495353003/diff/40001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1507: if (extension->permissions_data()->IsRuntimeBlockedHost(url)) On 2017/05/15 20:54:51, Devlin (catching up) wrote: > Why do we need this? WebRequestPermissions::CanExtensionAccessURL() checks > PermissionsData::CanRunOnPage(), which is where we put the check for > IsRuntimeBlockedHost(). IIUC, if you want to redirect a web request, the extension needs permission for the host being redirected, but not for the origin. I only want to verify that the extension has permissions for the origin when using my policy. If we call WebRequestPermissions::CanExtensionAccessURL() on the origin, then we're changing the behavior of WebRequests API regardless of whether my policy is being applied. I don't think we'd want to do that. As you mentioned since IsRuntimeBlockedHost is called as part of WebRequestPermissions::CanExtensionAccessURL() I just need to check the origin. This means I can remove the check for IsRuntimeBlockedHost(url). https://codereview.chromium.org/2495353003/diff/40001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1508: access = PermissionsData::ACCESS_WITHHELD; On 2017/05/15 20:54:51, Devlin (catching up) wrote: > We would want DENIED, right? WITHHELD will surface this to the user. Done.
nrpeter@google.com changed reviewers: - dcheng@chromium.org
https://codereview.chromium.org/2495353003/diff/60001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api.cc (right): https://codereview.chromium.org/2495353003/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1509: request->first_party_for_cookies())) I don't think this is doing what you want. This isn't necessarily related to the redirectUrl returned from an extension webRequest handler; it's related to server redirects. What's more, the comments above this method are quite explicit: // WARNING: This URL must only be used for the third-party cookie blocking // policy. It MUST NEVER be used for any kind of SECURITY check. What exactly are you trying to detect/prevent? Can you point to a test case or describe a scenario?
https://codereview.chromium.org/2495353003/diff/60001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api.cc (right): https://codereview.chromium.org/2495353003/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1509: request->first_party_for_cookies())) On 2017/05/17 20:03:40, Devlin (catching up) wrote: > I don't think this is doing what you want. This isn't necessarily related to > the redirectUrl returned from an extension webRequest handler; it's related to > server redirects. What's more, the comments above this method are quite > explicit: > // WARNING: This URL must only be used for the third-party cookie blocking > // policy. It MUST NEVER be used for any kind of SECURITY check. > > What exactly are you trying to detect/prevent? Can you point to a test case or > describe a scenario? (For reference, comment is at https://chromium.googlesource.com/chromium/src/+/c29cc4ea75913bb687853ef0722c...)
https://codereview.chromium.org/2495353003/diff/60001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api.cc (right): https://codereview.chromium.org/2495353003/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1509: request->first_party_for_cookies())) On 2017/05/17 20:03:40, Devlin (catching up) wrote: > I don't think this is doing what you want. This isn't necessarily related to > the redirectUrl returned from an extension webRequest handler; it's related to > server redirects. What's more, the comments above this method are quite > explicit: > // WARNING: This URL must only be used for the third-party cookie blocking > // policy. It MUST NEVER be used for any kind of SECURITY check. > > What exactly are you trying to detect/prevent? Can you point to a test case or > describe a scenario? This section of code deals with determining which extensions should be notified in the case of a web request. If we have a web request to example.com, the call to WebRequestPermissions::CanExtensionAccessURL verifies whether each extension declared example.com in its permissions. However, there is a situation I didn't see covered. Lets say an extension declares three permissions "webRequest", "webRequestBlocking", "*://*.example.com/*". This means it can alter any request to example.com. What if we have a website https://secret.google.com that happens to use a script hosted on example.com. This would allow the extension to redirect a script request initiated by secret.google.com leading to execution on secret.google.com. Now what if *://*.google.com/* was on my runtime_blocked_hosts for the extension above. We wouldn't want that extension being able to view or alter any request from secret.google.com. This means we need to check the URL that initiated the request, and not just the URL being loaded. This is why I'm checking first_party_for_cookies since it seemed to be the only location where the full URL of the initiator was stored. I saw URLRequest::Initiator, but that only contains the scheme, host, and port. However, we need to be able to make decisions based on the path of the URL. Is there anywhere that the path of the initiator is saved so I can recreate a GURL to check against my policy? I wrote three tests for this this which demonstrate what I'm trying to do. chrome/browser/extensions/api/web_request/web_request_apitest.cc
https://codereview.chromium.org/2495353003/diff/60001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api.cc (right): https://codereview.chromium.org/2495353003/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1509: request->first_party_for_cookies())) On 2017/05/18 21:10:46, nrpeter wrote: > On 2017/05/17 20:03:40, Devlin (catching up) wrote: > > I don't think this is doing what you want. This isn't necessarily related to > > the redirectUrl returned from an extension webRequest handler; it's related to > > server redirects. What's more, the comments above this method are quite > > explicit: > > // WARNING: This URL must only be used for the third-party cookie blocking > > // policy. It MUST NEVER be used for any kind of SECURITY check. > > > > What exactly are you trying to detect/prevent? Can you point to a test case > or > > describe a scenario? > > This section of code deals with determining which extensions should be notified > in the case of a web request. If we have a web request to http://example.com, the call > to WebRequestPermissions::CanExtensionAccessURL verifies whether each extension > declared http://example.com in its permissions. > > However, there is a situation I didn't see covered. Lets say an extension > declares three permissions "webRequest", "webRequestBlocking", > "*://*.example.com/*". This means it can alter any request to http://example.com. What > if we have a website https://secret.google.com that happens to use a script > hosted on http://example.com. This would allow the extension to redirect a script > request initiated by http://secret.google.com leading to execution on > http://secret.google.com. > > Now what if *://*.google.com/* was on my runtime_blocked_hosts for the extension > above. We wouldn't want that extension being able to view or alter any request > from http://secret.google.com. This means we need to check the URL that initiated the > request, and not just the URL being loaded. This is why I'm checking > first_party_for_cookies since it seemed to be the only location where the full > URL of the initiator was stored. > > I saw URLRequest::Initiator, but that only contains the scheme, host, and port. > However, we need to be able to make decisions based on the path of the URL. Is > there anywhere that the path of the initiator is saved so I can recreate a GURL > to check against my policy? > > I wrote three tests for this this which demonstrate what I'm trying to do. > chrome/browser/extensions/api/web_request/web_request_apitest.cc Yes, this is a known issue in the webRequest API. It's a bit weird to have a significantly different permissions model for policy cases, but I can appreciate why it might be important. But using first_party_url_for_cookies is the wrong thing to do. Instead, we should check for the parent frame's url.
Patchset #5 (id:100001) has been deleted
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
The CQ bit was checked by nrpeter@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nrpeter@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...
It looks like the URLRequest::Initiator is the only reliable method to determine where a request came from. The initiator is however a url::Origin which doesn’t include the path of the initial requester. I talked to the team and we decided that being able to reliably match the initiator was more important than matching the path. This lead to a change in the parsing to disallow user provided paths. I’ve updated the policy handler to accommodate and added tests. The code now checks the initiator of the request rather than first_party_url_for_cookies. This patchset also includes small changes to the policy template where I forgot to previously document runtime_blocked_hosts and runtime_allowed_hosts with examples.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Patchset #7 (id:200001) has been deleted
The CQ bit was checked by nrpeter@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Patchset #7 (id:220001) has been deleted
The CQ bit was checked by nrpeter@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: This issue passed the CQ dry run.
rdevlin.cronin@chromium.org changed reviewers: + mkwst@chromium.org
On 2017/05/24 16:50:02, nrpeter wrote: > It looks like the URLRequest::Initiator is the only reliable method to determine > where a request came from. The initiator is however a url::Origin which doesn’t > include the path of the initial requester. I talked to the team and we decided > that being able to reliably match the initiator was more important than matching > the path. > > This lead to a change in the parsing to disallow user provided paths. I’ve > updated the policy handler to accommodate and added tests. The code now checks > the initiator of the request rather than first_party_url_for_cookies. > > This patchset also includes small changes to the policy template where I forgot > to previously document runtime_blocked_hosts and runtime_allowed_hosts with > examples. If this is the route we want to go, we should separate this into two different CLs - one to change the policy behavior to only accept origins, and one to enable the check in webRequest. Those are pretty distinct behaviors, and shouldn't necessarily be bundled in a single commit. Also +mkwst, since I he's the expert here (having written it all). Mike, will using URLRequest::initiator() work as Nick hopes here (to prevent extensions from intercepting requests to a.com from b.com if b.com is a disallowed host)?
On 2017/05/25 19:01:31, Devlin (catching up) wrote: > On 2017/05/24 16:50:02, nrpeter wrote: > > It looks like the URLRequest::Initiator is the only reliable method to > determine > > where a request came from. The initiator is however a url::Origin which > doesn’t > > include the path of the initial requester. I talked to the team and we > decided > > that being able to reliably match the initiator was more important than > matching > > the path. > > > > This lead to a change in the parsing to disallow user provided paths. I’ve > > updated the policy handler to accommodate and added tests. The code now checks > > the initiator of the request rather than first_party_url_for_cookies. > > > > This patchset also includes small changes to the policy template where I > forgot > > to previously document runtime_blocked_hosts and runtime_allowed_hosts with > > examples. > > If this is the route we want to go, we should separate this into two different > CLs - one to change the policy behavior to only accept origins, and one to > enable the check in webRequest. Those are pretty distinct behaviors, and > shouldn't necessarily be bundled in a single commit. > > Also +mkwst, since I he's the expert here (having written it all). Mike, will > using URLRequest::initiator() work as Nick hopes here (to prevent extensions > from intercepting requests to a.com from b.com if b.com is a disallowed host)? Thanks, I'll move the policy changes to a separate CL.
On 2017/05/25 19:37:08, nrpeter wrote: > On 2017/05/25 19:01:31, Devlin (catching up) wrote: > > On 2017/05/24 16:50:02, nrpeter wrote: > > > It looks like the URLRequest::Initiator is the only reliable method to > > determine > > > where a request came from. The initiator is however a url::Origin which > > doesn’t > > > include the path of the initial requester. I talked to the team and we > > decided > > > that being able to reliably match the initiator was more important than > > matching > > > the path. > > > > > > This lead to a change in the parsing to disallow user provided paths. I’ve > > > updated the policy handler to accommodate and added tests. The code now > checks > > > the initiator of the request rather than first_party_url_for_cookies. > > > > > > This patchset also includes small changes to the policy template where I > > forgot > > > to previously document runtime_blocked_hosts and runtime_allowed_hosts with > > > examples. > > > > If this is the route we want to go, we should separate this into two different > > CLs - one to change the policy behavior to only accept origins, and one to > > enable the check in webRequest. Those are pretty distinct behaviors, and > > shouldn't necessarily be bundled in a single commit. > > > > Also +mkwst, since I he's the expert here (having written it all). Mike, will > > using URLRequest::initiator() work as Nick hopes here (to prevent extensions > > from intercepting requests to a.com from b.com if b.com is a disallowed host)? > > Thanks, I'll move the policy changes to a separate CL. Thanks! I'll see if I can't take a look at the webRequest portion of this one in the meantime. :)
first round https://codereview.chromium.org/2495353003/diff/240001/chrome/browser/extensi... File chrome/browser/extensions/api/web_request/web_request_apitest.cc (right): https://codereview.chromium.org/2495353003/diff/240001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:192: class ExtensionWebRequestApiTest : public ExtensionApiTestWithManagementPolicy { Let's not have all web request API tests inherit from this, but rather only the ones that use it. https://codereview.chromium.org/2495353003/diff/240001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:959: // Test that web requests made by a webpage protected by the test comments should be descriptive, not imperative. https://codereview.chromium.org/2495353003/diff/240001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:960: // runtime_blocked_hosts of the ExtensionsSettings policy are not modifyable. typo: modifiable https://codereview.chromium.org/2495353003/diff/240001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:961: IN_PROC_BROWSER_TEST_F(ExtensionWebRequestApiTest, policyBlockByOrigin) { Tests should be TitleCase https://codereview.chromium.org/2495353003/diff/240001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:987: pref.AddRuntimeBlockedHost("*", "*://not_example.com"); Underscores aren't allowed in domain names by the spec, so let's keep this valid. https://codereview.chromium.org/2495353003/diff/240001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:990: PermissionsRequestFunction::SetAutoConfirmForTests(true); Why do we need this? https://codereview.chromium.org/2495353003/diff/240001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:1003: embedded_test_server()->GetURL(example_com, extension_test_url), instead of const'ing example.com and the file, we should cache a GURL and use that. https://codereview.chromium.org/2495353003/diff/240001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:1019: // Now lets set the policy to hide webRequests to example.com or any resource In comments like this, omit words like "let's", "we'll", etc. So here, just: // Set the policy to hide requests to example.com... https://codereview.chromium.org/2495353003/diff/240001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/webrequest/policy_block_by_origin.html (right): https://codereview.chromium.org/2495353003/diff/240001/chrome/test/data/exten... chrome/test/data/extensions/api_test/webrequest/policy_block_by_origin.html:2: * Copyright (c) 2016 The Chromium Authors. All rights reserved. Use of this no (c) Not 2016 (applies to all new files) https://codereview.chromium.org/2495353003/diff/240001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/webrequest/policy_blocked/background.js (right): https://codereview.chromium.org/2495353003/diff/240001/chrome/test/data/exten... chrome/test/data/extensions/api_test/webrequest/policy_blocked/background.js:7: if(details.url.indexOf("example2") != -1) { be sure to check JS style. Space required between 'if' and '('. https://codereview.chromium.org/2495353003/diff/240001/chrome/test/data/exten... chrome/test/data/extensions/api_test/webrequest/policy_blocked/background.js:8: chrome.test.sendMessage("protected_origin"); single quotes in js https://codereview.chromium.org/2495353003/diff/240001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/webrequest/policy_blocked/manifest.json (right): https://codereview.chromium.org/2495353003/diff/240001/chrome/test/data/exten... chrome/test/data/extensions/api_test/webrequest/policy_blocked/manifest.json:7: "scripts": ["background.js"] prefer 2-space indentation https://codereview.chromium.org/2495353003/diff/240001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/webrequest/policy_blocked/ref_remote_js.js (right): https://codereview.chromium.org/2495353003/diff/240001/chrome/test/data/exten... chrome/test/data/extensions/api_test/webrequest/policy_blocked/ref_remote_js.js:17: document.firstElementChild.appendChild(differentOriginScript); Why do we need this instead of just var script = document.createElement('script'); script.src = <url>; document.body.appendChild(script); ? https://codereview.chromium.org/2495353003/diff/240001/extensions/browser/api... File extensions/browser/api/web_request/web_request_api.cc (right): https://codereview.chromium.org/2495353003/diff/240001/extensions/browser/api... extensions/browser/api/web_request/web_request_api.cc:1507: extension_info_map->extensions().GetByID(listener->id.extension_id); Can we just put this in WebRequestPermissions::CanExtensionAccessURL? https://codereview.chromium.org/2495353003/diff/240001/extensions/common/perm... File extensions/common/permissions/permissions_data.h (right): https://codereview.chromium.org/2495353003/diff/240001/extensions/common/perm... extensions/common/permissions/permissions_data.h:268: bool IsRuntimeBlockedHost(const GURL& url) const { We should put this above for-testing methods. https://codereview.chromium.org/2495353003/diff/240001/url/origin.h File url/origin.h (right): https://codereview.chromium.org/2495353003/diff/240001/url/origin.h#newcode163 url/origin.h:163: GURL GetURLWithoutSuborigin() const; Mike would know better, but this seems to be a little against the spirit of Origin.
Patchset #8 (id:260001) has been deleted
Patchset #9 (id:300001) has been deleted
The CQ bit was checked by nrpeter@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...
These changes address Devlin's comments related to the webRequest section of this CL. I'll now focus on splitting the policy changes out into a separate CL. https://codereview.chromium.org/2495353003/diff/240001/chrome/browser/extensi... File chrome/browser/extensions/api/web_request/web_request_apitest.cc (right): https://codereview.chromium.org/2495353003/diff/240001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:192: class ExtensionWebRequestApiTest : public ExtensionApiTestWithManagementPolicy { On 2017/05/25 20:38:32, Devlin (catching up) wrote: > Let's not have all web request API tests inherit from this, but rather only the > ones that use it. Done. https://codereview.chromium.org/2495353003/diff/240001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:959: // Test that web requests made by a webpage protected by the On 2017/05/25 20:38:32, Devlin (catching up) wrote: > test comments should be descriptive, not imperative. Done. https://codereview.chromium.org/2495353003/diff/240001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:960: // runtime_blocked_hosts of the ExtensionsSettings policy are not modifyable. On 2017/05/25 20:38:32, Devlin (catching up) wrote: > typo: modifiable Done. https://codereview.chromium.org/2495353003/diff/240001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:961: IN_PROC_BROWSER_TEST_F(ExtensionWebRequestApiTest, policyBlockByOrigin) { On 2017/05/25 20:38:32, Devlin (catching up) wrote: > Tests should be TitleCase Done. https://codereview.chromium.org/2495353003/diff/240001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:987: pref.AddRuntimeBlockedHost("*", "*://not_example.com"); On 2017/05/25 20:38:32, Devlin (catching up) wrote: > Underscores aren't allowed in domain names by the spec, so let's keep this > valid. Done. https://codereview.chromium.org/2495353003/diff/240001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:990: PermissionsRequestFunction::SetAutoConfirmForTests(true); On 2017/05/25 20:38:32, Devlin (catching up) wrote: > Why do we need this? Done. https://codereview.chromium.org/2495353003/diff/240001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:1003: embedded_test_server()->GetURL(example_com, extension_test_url), On 2017/05/25 20:38:32, Devlin (catching up) wrote: > instead of const'ing http://example.com and the file, we should cache a GURL and use > that. Made a const GURL as suggested. However I kept example.com const'd since it is referenced more than once. Also went through other tests and const'd strings that were repeated. https://codereview.chromium.org/2495353003/diff/240001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:1019: // Now lets set the policy to hide webRequests to example.com or any resource On 2017/05/25 20:38:32, Devlin (catching up) wrote: > In comments like this, omit words like "let's", "we'll", etc. So here, just: > // Set the policy to hide requests to example.com... Think I got all of these. If you notice any I missed I'm happy to fix. https://codereview.chromium.org/2495353003/diff/240001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/webrequest/policy_block_by_origin.html (right): https://codereview.chromium.org/2495353003/diff/240001/chrome/test/data/exten... chrome/test/data/extensions/api_test/webrequest/policy_block_by_origin.html:2: * Copyright (c) 2016 The Chromium Authors. All rights reserved. Use of this On 2017/05/25 20:38:32, Devlin (catching up) wrote: > no (c) > > Not 2016 > > (applies to all new files) Done. https://codereview.chromium.org/2495353003/diff/240001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/webrequest/policy_blocked/background.js (right): https://codereview.chromium.org/2495353003/diff/240001/chrome/test/data/exten... chrome/test/data/extensions/api_test/webrequest/policy_blocked/background.js:7: if(details.url.indexOf("example2") != -1) { On 2017/05/25 20:38:32, Devlin (catching up) wrote: > be sure to check JS style. Space required between 'if' and '('. Done. https://codereview.chromium.org/2495353003/diff/240001/chrome/test/data/exten... chrome/test/data/extensions/api_test/webrequest/policy_blocked/background.js:8: chrome.test.sendMessage("protected_origin"); On 2017/05/25 20:38:32, Devlin (catching up) wrote: > single quotes in js Thanks, I just ran gjslint on all the JS files and fixed notices. https://codereview.chromium.org/2495353003/diff/240001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/webrequest/policy_blocked/manifest.json (right): https://codereview.chromium.org/2495353003/diff/240001/chrome/test/data/exten... chrome/test/data/extensions/api_test/webrequest/policy_blocked/manifest.json:7: "scripts": ["background.js"] On 2017/05/25 20:38:32, Devlin (catching up) wrote: > prefer 2-space indentation Done. https://codereview.chromium.org/2495353003/diff/240001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/webrequest/policy_blocked/ref_remote_js.js (right): https://codereview.chromium.org/2495353003/diff/240001/chrome/test/data/exten... chrome/test/data/extensions/api_test/webrequest/policy_blocked/ref_remote_js.js:17: document.firstElementChild.appendChild(differentOriginScript); On 2017/05/25 20:38:32, Devlin (catching up) wrote: > Why do we need this instead of just > var script = document.createElement('script'); > script.src = <url>; > document.body.appendChild(script); > ? Each run, the embedded test server uses a different port. This code takes a url like http://example.com:1234/extensions/api_test/webrequest/policy_blocked/ref_rem... and turns it into http://example2.com:1234/extensions/api_test/webrequest/policy_blocked/remote.... A script tag is then generated and set to the example2.com url which initiates a web request with example.com as the initiator. The main issues this approach solves is 1. Getting the port and 2. Not hardcoding a path to the test file. This could alternatively be done by using chrome.runtime to get the embedded test server port from the content script. If you'd prefer me to use the chrome.runtime approach please let me know and I'll rewrite it. https://codereview.chromium.org/2495353003/diff/240001/extensions/browser/api... File extensions/browser/api/web_request/web_request_api.cc (right): https://codereview.chromium.org/2495353003/diff/240001/extensions/browser/api... extensions/browser/api/web_request/web_request_api.cc:1507: extension_info_map->extensions().GetByID(listener->id.extension_id); On 2017/05/25 20:38:32, Devlin (catching up) wrote: > Can we just put this in WebRequestPermissions::CanExtensionAccessURL? I would like to but doing so would change the behavior of existing extensions regardless of whether they are using the ExtensionSettings policy. It may be helpful to re-read #17 & #21 https://codereview.chromium.org/2495353003/diff/240001/extensions/common/perm... File extensions/common/permissions/permissions_data.h (right): https://codereview.chromium.org/2495353003/diff/240001/extensions/common/perm... extensions/common/permissions/permissions_data.h:268: bool IsRuntimeBlockedHost(const GURL& url) const { On 2017/05/25 20:38:33, Devlin (catching up) wrote: > We should put this above for-testing methods. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mkwst@chromium.org changed reviewers: + jochen@chromium.org
https://codereview.chromium.org/2495353003/diff/280001/url/origin.cc File url/origin.cc (right): https://codereview.chromium.org/2495353003/diff/280001/url/origin.cc#newcode172 url/origin.cc:172: } +Jochen, who's responsible for suborigins these days. What should we do here? It does seem like extensions' permission grants shouldn't care about suborigins, but adding this kind of accessor just to get around the serialization differences feels strange. We have a `ToPhysicalOriginString()` on `blink::SecurityOrigin`, which we use for `postMessage()`. Should we replicate that here? I'd prefer not to if we can get away without it.
https://codereview.chromium.org/2495353003/diff/280001/url/origin.cc File url/origin.cc (right): https://codereview.chromium.org/2495353003/diff/280001/url/origin.cc#newcode172 url/origin.cc:172: } why not use origin->GetPhysicalOrigin().GetURL() ?
https://codereview.chromium.org/2495353003/diff/280001/url/origin.cc File url/origin.cc (right): https://codereview.chromium.org/2495353003/diff/280001/url/origin.cc#newcode172 url/origin.cc:172: } On 2017/05/26 11:51:53, jochen wrote: > why not use origin->GetPhysicalOrigin().GetURL() ? Good point, I'll switch to GetPhysicalOrigin().GetURL() as that should be equivalent.
Patchset #9 (id:320001) has been deleted
Patchset #9 (id:340001) has been deleted
Moved policy changes to a CL on gerrit. https://chromium-review.googlesource.com/c/517228/ I'll make sure to submit the Gerrit CL first.
Hey Nick, friendly reminder that often comments can apply to a whole patch set (though reviewers often point it out only once to avoid adding near-duplicate comments to a review). :) https://codereview.chromium.org/2495353003/diff/240001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/webrequest/policy_blocked/ref_remote_js.js (right): https://codereview.chromium.org/2495353003/diff/240001/chrome/test/data/exten... chrome/test/data/extensions/api_test/webrequest/policy_blocked/ref_remote_js.js:17: document.firstElementChild.appendChild(differentOriginScript); On 2017/05/26 02:46:52, nrpeter wrote: > On 2017/05/25 20:38:32, Devlin (catching up) wrote: > > Why do we need this instead of just > > var script = document.createElement('script'); > > script.src = <url>; > > document.body.appendChild(script); > > ? > > > > Each run, the embedded test server uses a different port. This code takes a url > like > http://example.com:1234/extensions/api_test/webrequest/policy_blocked/ref_rem... > and turns it into > http://example2.com:1234/extensions/api_test/webrequest/policy_blocked/remote.... > > A script tag is then generated and set to the http://example2.com url which initiates a > web request with http://example.com as the initiator. > > The main issues this approach solves is 1. Getting the port and 2. Not > hardcoding a path to the test file. > This could alternatively be done by using chrome.runtime to get the embedded > test server port from the content script. If you'd prefer me to use the > chrome.runtime approach please let me know and I'll rewrite it. var script = document.createElement('script'); script.src = 'http://example2.com:' + location.port + '/extensions/api_test/webrequest/policy_blocked/remote.js'; document.body.appendChild(script); Would that work? https://codereview.chromium.org/2495353003/diff/240001/extensions/browser/api... File extensions/browser/api/web_request/web_request_api.cc (right): https://codereview.chromium.org/2495353003/diff/240001/extensions/browser/api... extensions/browser/api/web_request/web_request_api.cc:1507: extension_info_map->extensions().GetByID(listener->id.extension_id); On 2017/05/26 02:46:52, nrpeter wrote: > On 2017/05/25 20:38:32, Devlin (catching up) wrote: > > Can we just put this in WebRequestPermissions::CanExtensionAccessURL? > > I would like to but doing so would change the behavior of existing extensions > regardless of whether they are using the ExtensionSettings policy. > > It may be helpful to re-read #17 & #21 I'm suggesting putting the entire block, including the IsRuntimeBlockedHost() check, in WebRequestPermissions. How would that change behavior? https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi... File chrome/browser/extensions/api/web_request/web_request_apitest.cc (right): https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:970: const std::string listener_name = "protected_origin"; this isn't a name, it's a message. https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:979: PermissionsRequestFunction::SetAutoConfirmForTests(true); As mentioned in https://codereview.chromium.org/2495353003/diff/240001/chrome/browser/extensi..., why do we need this? https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:1011: // Set the policy to hide webRequests to example.com or any resource s/webRequests/requests (webRequest is the API) https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:1042: const std::string unprotected_domain = "not_blocked_example.com"; As mentioned at https://codereview.chromium.org/2495353003/diff/240001/chrome/browser/extensi..., please keep URLs valid according to the spec. https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:1045: const std::string file_url = variables should be declared near their first use. https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:1143: runner->set_default_bubble_close_action_for_testing( Trying to run when the extension doesn't want to is a bit odd. Why not just grant tab permissions directly? https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:1149: // The runner will have refreshed the page... Will it have, if there was no blocked action? https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:1154: EXPECT_EQ(xhr_count, 0); EXPECT_EQ(expected, actual) https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:1158: EXPECT_EQ(xhr_count, prefer 0, since that's what we expect https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi... File chrome/browser/extensions/extension_with_management_policy_apitest.h (right): https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.h:11: #include "chrome/browser/extensions/api/permissions/permissions_api.h" needed? https://codereview.chromium.org/2495353003/diff/360001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/webrequest/policy_block_by_origin.html (right): https://codereview.chromium.org/2495353003/diff/360001/chrome/test/data/exten... chrome/test/data/extensions/api_test/webrequest/policy_block_by_origin.html:1: <!-- Is this file used?
Patchset #10 (id:380001) has been deleted
Patchset #11 (id:420001) has been deleted
Patchset #10 (id:400001) has been deleted
Patchset #10 (id:440001) has been deleted
Thanks for your patience, I was focusing on getting the policy changes this CL relies on into another CL and committed. https://codereview.chromium.org/2495353003/diff/240001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/webrequest/policy_blocked/ref_remote_js.js (right): https://codereview.chromium.org/2495353003/diff/240001/chrome/test/data/exten... chrome/test/data/extensions/api_test/webrequest/policy_blocked/ref_remote_js.js:17: document.firstElementChild.appendChild(differentOriginScript); On 2017/06/01 15:21:32, Devlin wrote: > On 2017/05/26 02:46:52, nrpeter wrote: > > On 2017/05/25 20:38:32, Devlin (catching up) wrote: > > > Why do we need this instead of just > > > var script = document.createElement('script'); > > > script.src = <url>; > > > document.body.appendChild(script); > > > ? > > > > > > > > Each run, the embedded test server uses a different port. This code takes a > url > > like > > > http://example.com:1234/extensions/api_test/webrequest/policy_blocked/ref_rem... > > and turns it into > > > http://example2.com:1234/extensions/api_test/webrequest/policy_blocked/remote.... > > > > A script tag is then generated and set to the http://example2.com url which > initiates a > > web request with http://example.com as the initiator. > > > > The main issues this approach solves is 1. Getting the port and 2. Not > > hardcoding a path to the test file. > > This could alternatively be done by using chrome.runtime to get the embedded > > test server port from the content script. If you'd prefer me to use the > > chrome.runtime approach please let me know and I'll rewrite it. > > var script = document.createElement('script'); > script.src = 'http://example2.com:' + location.port + > '/extensions/api_test/webrequest/policy_blocked/remote.js'; > document.body.appendChild(script); > > Would that work? Switched to using your suggested implementation. Note: This required changes to ref_remote_js.html since it didn't previously declare a body. Without a declared body before the script execution document.body will be null and we can't appendChild. https://codereview.chromium.org/2495353003/diff/240001/extensions/browser/api... File extensions/browser/api/web_request/web_request_api.cc (right): https://codereview.chromium.org/2495353003/diff/240001/extensions/browser/api... extensions/browser/api/web_request/web_request_api.cc:1507: extension_info_map->extensions().GetByID(listener->id.extension_id); On 2017/06/01 15:21:32, Devlin wrote: > On 2017/05/26 02:46:52, nrpeter wrote: > > On 2017/05/25 20:38:32, Devlin (catching up) wrote: > > > Can we just put this in WebRequestPermissions::CanExtensionAccessURL? > > > > I would like to but doing so would change the behavior of existing extensions > > regardless of whether they are using the ExtensionSettings policy. > > > > It may be helpful to re-read #17 & #21 > > I'm suggesting putting the entire block, including the IsRuntimeBlockedHost() > check, in WebRequestPermissions. How would that change behavior? Done. https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi... File chrome/browser/extensions/api/web_request/web_request_apitest.cc (right): https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:970: const std::string listener_name = "protected_origin"; On 2017/06/01 15:21:33, Devlin wrote: > this isn't a name, it's a message. Done. https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:979: PermissionsRequestFunction::SetAutoConfirmForTests(true); On 2017/06/01 15:21:33, Devlin wrote: > As mentioned in > https://codereview.chromium.org/2495353003/diff/240001/chrome/browser/extensi..., > why do we need this? Done. https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:1011: // Set the policy to hide webRequests to example.com or any resource On 2017/06/01 15:21:32, Devlin wrote: > s/webRequests/requests (webRequest is the API) Done. https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:1042: const std::string unprotected_domain = "not_blocked_example.com"; On 2017/06/01 15:21:32, Devlin wrote: > As mentioned at > https://codereview.chromium.org/2495353003/diff/240001/chrome/browser/extensi..., > please keep URLs valid according to the spec. Done. https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:1045: const std::string file_url = On 2017/06/01 15:21:33, Devlin wrote: > variables should be declared near their first use. Done. https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:1143: runner->set_default_bubble_close_action_for_testing( On 2017/06/01 15:21:32, Devlin (slow June 6) wrote: > Trying to run when the extension doesn't want to is a bit odd. Why not just > grant tab permissions directly? I could, but wanted to make sure that modification of permissions (e.g. activeTab) would still be blocked. https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:1149: // The runner will have refreshed the page... On 2017/06/01 15:21:33, Devlin (slow June 6) wrote: > Will it have, if there was no blocked action? Done. Thanks, that was an old comment. https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:1154: EXPECT_EQ(xhr_count, 0); On 2017/06/01 15:21:33, Devlin wrote: > EXPECT_EQ(expected, actual) Done. https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:1158: EXPECT_EQ(xhr_count, On 2017/06/01 15:21:33, Devlin wrote: > prefer 0, since that's what we expect Done. https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi... File chrome/browser/extensions/extension_with_management_policy_apitest.h (right): https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.h:11: #include "chrome/browser/extensions/api/permissions/permissions_api.h" On 2017/06/01 15:21:33, Devlin wrote: > needed? Done. https://codereview.chromium.org/2495353003/diff/360001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/webrequest/policy_block_by_origin.html (right): https://codereview.chromium.org/2495353003/diff/360001/chrome/test/data/exten... chrome/test/data/extensions/api_test/webrequest/policy_block_by_origin.html:1: <!-- On 2017/06/01 15:21:33, Devlin wrote: > Is this file used? Done. https://codereview.chromium.org/2495353003/diff/450001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/webrequest/policy_blocked/ref_remote_js.html (right): https://codereview.chromium.org/2495353003/diff/450001/chrome/test/data/exten... chrome/test/data/extensions/api_test/webrequest/policy_blocked/ref_remote_js.html:6: <html> Needed since the new JS code modifies body. Previously body didn't exist and document.body would be null.
The CQ bit was checked by nrpeter@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: This issue passed the CQ dry run.
lgtm; thanks for bearing with us through the review! https://codereview.chromium.org/2495353003/diff/450001/chrome/browser/extensi... File chrome/browser/extensions/api/web_request/web_request_apitest.cc (right): https://codereview.chromium.org/2495353003/diff/450001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:963: // We expect that no webRequest will be hidden or modification blocked. This As mentioned at https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi..., prefer "request" or "network request" for this context. https://codereview.chromium.org/2495353003/diff/450001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:975: // URL within the text extension that initiates cross domain requests when two nits: - I assume s/text/test? - this actually isn't within the test extension; it's navigated to as an URL from example.com. I'd just say "URL of a page that initiates..."
https://codereview.chromium.org/2495353003/diff/450001/chrome/browser/extensi... File chrome/browser/extensions/api/web_request/web_request_apitest.cc (right): https://codereview.chromium.org/2495353003/diff/450001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:963: // We expect that no webRequest will be hidden or modification blocked. This On 2017/06/08 13:58:46, Devlin (slow through June 8) wrote: > As mentioned at > https://codereview.chromium.org/2495353003/diff/360001/chrome/browser/extensi..., > prefer "request" or "network request" for this context. Thanks must have missed this one. https://codereview.chromium.org/2495353003/diff/450001/chrome/browser/extensi... chrome/browser/extensions/api/web_request/web_request_apitest.cc:975: // URL within the text extension that initiates cross domain requests when On 2017/06/08 13:58:46, Devlin (slow through June 8) wrote: > two nits: > - I assume s/text/test? > - this actually isn't within the test extension; it's navigated to as an URL > from http://example.com. I'd just say "URL of a page that initiates..." Done.
The CQ bit was checked by nrpeter@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2495353003/#ps470001 (title: "Comment cleanup")
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": 470001, "attempt_start_ts": 1496936875852560,
"parent_rev": "24309d877571d159bc14f871671ff0630d6f8d32", "commit_rev":
"ea0116aad7867695cb2e438d3546f148ea271858"}
Message was sent while issue was closed.
Description was changed from ========== chrome.webRequest support for ExtensionSettings -If URL or Origin matches an ExtensionSettings protected host, the event will not be dispatched to that extension / app. BUG=624649 ========== to ========== chrome.webRequest support for ExtensionSettings -If URL or Origin matches an ExtensionSettings protected host, the event will not be dispatched to that extension / app. BUG=624649 Review-Url: https://codereview.chromium.org/2495353003 Cr-Commit-Position: refs/heads/master@{#478018} Committed: https://chromium.googlesource.com/chromium/src/+/ea0116aad7867695cb2e438d3546... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:470001) as https://chromium.googlesource.com/chromium/src/+/ea0116aad7867695cb2e438d3546...
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:470001) has been created in https://codereview.chromium.org/2930983002/ by dbeam@chromium.org. The reason for reverting is: Breaking ExtensionApiTestWithManagementPolicy.InitiatorProtectedByPolicy browser_test on multiple testers https://build.chromium.org/p/chromium.memory/builders/Linux%20ChromiumOS%20MS... https://build.chromium.org/p/chromium.memory/builders/Linux%20MSan%20Tests/bu... https://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests.... |
