Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2978953002:
[Extensions] Don't allow content scripts on the New Tab Page (Closed)
|
Created:
3 years, 5 months ago by Devlin Modified:
3 years, 3 months ago Reviewers:
karandeepb CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, lazyboy Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Extensions] Don't allow content scripts on the New Tab Page
Extensions wishing to modify the New Tab Page should do so through the
use of the chrome url overrides [1] API, not through content scripts.
Disallow content script injection on the New Tab Page.
[1] https://developer.chrome.com/extensions/override
BUG=662610
Review-Url: https://codereview.chromium.org/2978953002
Cr-Commit-Position: refs/heads/master@{#487664}
Committed: https://chromium.googlesource.com/chromium/src/+/993682e9e2b181a14f9fd9e6a7f848e401a551e7
Patch Set 1 : . #
Total comments: 20
Patch Set 2 : Karan's #
Total comments: 2
Patch Set 3 : Remove logging! #
Messages
Total messages: 34 (23 generated)
The CQ bit was checked by rdevlin.cronin@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...
Description was changed from ========== [Extensions] Don't allow content scripts on the New Tab Page Extensions wishing to modify the New Tab Page should do so through the use of the chrome url overrides [1] API, not through content scripts. Disallow content script injection on the New Tab Page. BUG=662610 ========== to ========== [Extensions] Don't allow content scripts on the New Tab Page Extensions wishing to modify the New Tab Page should do so through the use of the chrome url overrides [1] API, not through content scripts. Disallow content script injection on the New Tab Page. [1] https://developer.chrome.com/extensions/override BUG=662610 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rdevlin.cronin@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rdevlin.cronin@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
rdevlin.cronin@chromium.org changed reviewers: + karandeepb@chromium.org
Karan, mind taking a look?
https://codereview.chromium.org/2978953002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2978953002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1825: Do we also need to update extension docs for this change? https://codereview.chromium.org/2978953002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1832: bool ExecuteCodeInTabFunction::CanExecuteScriptOnPage() { So we don't do any checks in the browser process? https://codereview.chromium.org/2978953002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1852: LOG(WARNING) << "No frame"; Remove logging. https://codereview.chromium.org/2978953002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/content_script_apitest.cc (right): https://codereview.chromium.org/2978953002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/content_script_apitest.cc:646: EXPECT_FALSE( nit: Also verify that the active web contents url is the new tabs page. https://codereview.chromium.org/2978953002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/content_script_apitest.cc:649: // Next, check content script injection. Also check injection on a normal page first, to verify our setup. https://codereview.chromium.org/2978953002/diff/40001/chrome/renderer/extensi... File chrome/renderer/extensions/renderer_permissions_policy_delegate.cc (right): https://codereview.chromium.org/2978953002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/renderer_permissions_policy_delegate.cc:43: *error = errors::kCannotScriptNtp; Do we also need to hook into ChromeExtensionsClient::IsScriptableURL? We do for kCannotScriptGallery. https://codereview.chromium.org/2978953002/diff/40001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/content_scripts/ntp/background.js (right): https://codereview.chromium.org/2978953002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/ntp/background.js:13: chrome.test.assertTrue(!!chrome.runtime.lastError); I am not up to speed with how different JS types behave with !. Can this check be made more specific? (Meaning assuming the default value for lastError when no error is set is null, can we check lastError !== null)
https://codereview.chromium.org/2978953002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2978953002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1825: On 2017/07/18 19:26:59, karandeepb wrote: > Do we also need to update extension docs for this change? I don't think so. We state in numerous places that the only sanctioned way to modify chrome pages is through the chrome urls overrides. https://codereview.chromium.org/2978953002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1832: bool ExecuteCodeInTabFunction::CanExecuteScriptOnPage() { On 2017/07/18 19:26:59, karandeepb wrote: > So we don't do any checks in the browser process? Funny thing - the URL in the browser side is actually chrome://newtab, and so extensions aren't allowed to inject into it since it's a chrome://-scheme URL. Only the renderer knows that chrome://newtab is kinda-like google.com/newtab. So the only specific place we need to check for this is in the renderer. https://codereview.chromium.org/2978953002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1852: LOG(WARNING) << "No frame"; On 2017/07/18 19:26:59, karandeepb wrote: > Remove logging. Whoops, done. https://codereview.chromium.org/2978953002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/content_script_apitest.cc (right): https://codereview.chromium.org/2978953002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/content_script_apitest.cc:646: EXPECT_FALSE( On 2017/07/18 19:26:59, karandeepb wrote: > nit: Also verify that the active web contents url is the new tabs page. Done. https://codereview.chromium.org/2978953002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/content_script_apitest.cc:649: // Next, check content script injection. On 2017/07/18 19:27:00, karandeepb wrote: > Also check injection on a normal page first, to verify our setup. Done. Good suggestion; this helped flush out a bug. https://codereview.chromium.org/2978953002/diff/40001/chrome/renderer/extensi... File chrome/renderer/extensions/renderer_permissions_policy_delegate.cc (right): https://codereview.chromium.org/2978953002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/renderer_permissions_policy_delegate.cc:43: *error = errors::kCannotScriptNtp; On 2017/07/18 19:27:00, karandeepb wrote: > Do we also need to hook into ChromeExtensionsClient::IsScriptableURL? We do for > kCannotScriptGallery. I don't think we should need to - the existing browser-side checks are sufficient, and that isn't used for renderer-side checks. https://codereview.chromium.org/2978953002/diff/40001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/content_scripts/ntp/background.js (right): https://codereview.chromium.org/2978953002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/ntp/background.js:13: chrome.test.assertTrue(!!chrome.runtime.lastError); On 2017/07/18 19:27:00, karandeepb wrote: > I am not up to speed with how different JS types behave with !. Can this check > be made more specific? (Meaning assuming the default value for lastError when no > error is set is null, can we check lastError !== null) !!x checks that x is not "false-y". This has tradeoffs vs !== null. In particular, it discounts any falsey value, including undefined null 0 '' etc In some cases, that's bad, but in this case, that's actually a good thing. In particular, the test of chrome.test.assertTrue(chrome.runtime.lastError !== null) would pass, even if chrome.runtime.lastError === undefined. if there is a last error, then lastError should be something truthy, and this check is targeting that.
LGTM. But yeah, remove logging. https://codereview.chromium.org/2978953002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/2978953002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1825: On 2017/07/18 20:53:46, Devlin wrote: > On 2017/07/18 19:26:59, karandeepb wrote: > > Do we also need to update extension docs for this change? > > I don't think so. We state in numerous places that the only sanctioned way to > modify chrome pages is through the chrome urls overrides. Acknowledged. https://codereview.chromium.org/2978953002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_api.cc:1832: bool ExecuteCodeInTabFunction::CanExecuteScriptOnPage() { On 2017/07/18 20:53:46, Devlin wrote: > On 2017/07/18 19:26:59, karandeepb wrote: > > So we don't do any checks in the browser process? > > Funny thing - the URL in the browser side is actually chrome://newtab, and so > extensions aren't allowed to inject into it since it's a chrome://-scheme URL. > Only the renderer knows that chrome://newtab is kinda-like google.com/newtab. > So the only specific place we need to check for this is in the renderer. Acknowledged. https://codereview.chromium.org/2978953002/diff/40001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/content_scripts/ntp/background.js (right): https://codereview.chromium.org/2978953002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/ntp/background.js:13: chrome.test.assertTrue(!!chrome.runtime.lastError); On 2017/07/18 20:53:46, Devlin wrote: > On 2017/07/18 19:27:00, karandeepb wrote: > > I am not up to speed with how different JS types behave with !. Can this check > > be made more specific? (Meaning assuming the default value for lastError when > no > > error is set is null, can we check lastError !== null) > > !!x checks that x is not "false-y". This has tradeoffs vs !== null. In > particular, it discounts any falsey value, including > undefined > null > 0 > '' > etc > > In some cases, that's bad, but in this case, that's actually a good thing. In > particular, the test of > chrome.test.assertTrue(chrome.runtime.lastError !== null) > would pass, even if chrome.runtime.lastError === undefined. > > if there is a last error, then lastError should be something truthy, and this > check is targeting that. Acknowledged. https://codereview.chromium.org/2978953002/diff/40001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/content_scripts/ntp/manifest.json (right): https://codereview.chromium.org/2978953002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/ntp/manifest.json:9: "run_at": "document_start" For my own knowledge, why didn't document_start/window.didInject work? https://codereview.chromium.org/2978953002/diff/60001/extensions/browser/scri... File extensions/browser/script_executor.cc (right): https://codereview.chromium.org/2978953002/diff/60001/extensions/browser/scri... extensions/browser/script_executor.cc:138: LOG(WARNING) << "Done: " << error << ", " << on_url; Remove logging in this file as well.
The CQ bit was checked by rdevlin.cronin@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...
https://codereview.chromium.org/2978953002/diff/40001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/content_scripts/ntp/manifest.json (right): https://codereview.chromium.org/2978953002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/ntp/manifest.json:9: "run_at": "document_start" On 2017/07/18 21:06:25, karandeepb wrote: > For my own knowledge, why didn't document_start/window.didInject work? window.didInject didn't work because the content script (or script added by tabs.executeScript) executes in an isolated world, so the variables aren't exposed to the main world - which is where content::ExecuteScriptAndExtract* are executed. As a workaround, we can modify the DOM, so that the changes are visible to the main world script context, even though they were performed in an isolated world. So that leads us to using document.title instead of window.didInject. From there, document_start is just too early in the rendering process for document.title to be assigned, so we need to wait 'til at least document_end. https://codereview.chromium.org/2978953002/diff/60001/extensions/browser/scri... File extensions/browser/script_executor.cc (right): https://codereview.chromium.org/2978953002/diff/60001/extensions/browser/scri... extensions/browser/script_executor.cc:138: LOG(WARNING) << "Done: " << error << ", " << on_url; On 2017/07/18 21:06:26, karandeepb wrote: > Remove logging in this file as well. D'oh. Don't know how I missed so many on this CL. Thanks for your diligence, and sorry for needing repeated comments!
https://codereview.chromium.org/2978953002/diff/40001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/content_scripts/ntp/manifest.json (right): https://codereview.chromium.org/2978953002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/ntp/manifest.json:9: "run_at": "document_start" On 2017/07/18 21:12:49, Devlin wrote: > On 2017/07/18 21:06:25, karandeepb wrote: > > For my own knowledge, why didn't document_start/window.didInject work? > > window.didInject didn't work because the content script (or script added by > tabs.executeScript) executes in an isolated world, so the variables aren't > exposed to the main world - which is where content::ExecuteScriptAndExtract* are > executed. As a workaround, we can modify the DOM, so that the changes are > visible to the main world script context, even though they were performed in an > isolated world. So that leads us to using document.title instead of > window.didInject. From there, document_start is just too early in the rendering > process for document.title to be assigned, so we need to wait 'til at least > document_end. Makes sense! Thanks.
The CQ bit was unchecked by rdevlin.cronin@chromium.org
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from karandeepb@chromium.org Link to the patchset: https://codereview.chromium.org/2978953002/#ps80001 (title: "Remove logging!")
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": 80001, "attempt_start_ts": 1500418925597690,
"parent_rev": "239b085ef005c805d2b85daeadd600e9fec61e1a", "commit_rev":
"993682e9e2b181a14f9fd9e6a7f848e401a551e7"}
Message was sent while issue was closed.
Description was changed from ========== [Extensions] Don't allow content scripts on the New Tab Page Extensions wishing to modify the New Tab Page should do so through the use of the chrome url overrides [1] API, not through content scripts. Disallow content script injection on the New Tab Page. [1] https://developer.chrome.com/extensions/override BUG=662610 ========== to ========== [Extensions] Don't allow content scripts on the New Tab Page Extensions wishing to modify the New Tab Page should do so through the use of the chrome url overrides [1] API, not through content scripts. Disallow content script injection on the New Tab Page. [1] https://developer.chrome.com/extensions/override BUG=662610 Review-Url: https://codereview.chromium.org/2978953002 Cr-Commit-Position: refs/heads/master@{#487664} Committed: https://chromium.googlesource.com/chromium/src/+/993682e9e2b181a14f9fd9e6a7f8... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/chromium/src/+/993682e9e2b181a14f9fd9e6a7f8...
Message was sent while issue was closed.
Why the sudden change after many years of allowing content scripts in the iframe of NTP? Wouldn't users of the extensions that allowed customizing/restyling the standard NTP feel confounded and annoyed? It'd be nice if you post a publicly accessible issue or an announcement explaining the change so that the extension developers and users know it's not a bug.
Message was sent while issue was closed.
On 2017/07/22 14:27:54, wOxxOm wrote: > Why the sudden change after many years of allowing content scripts in the iframe > of NTP? Wouldn't users of the extensions that allowed customizing/restyling the > standard NTP feel confounded and annoyed? It'd be nice if you post a publicly > accessible issue or an announcement explaining the change so that the extension > developers and users know it's not a bug. Hey wOxxOm! There's a few reasons for this change. One is to enforce policy, the other is for consistency. We've had a public policy [1] for awhile now that states that modification of the NTP through anything other than Chrome URL overrides isn't allowed (though we didn't begin enforcing this policy in many cases until July 1st). This is merely bringing chrome code more inline with that same policy to help prevent surprise if an extension is modifying the NTP and is taken down for policy violations. This is also for consistency, since we've actually treated scripts on the NTP differently for years now, due to certain NTP magic. For example, the URL seen by the browser on the NTP is chrome://newtab, but the url in the renderer is https://www.google.com/_/chrome/newtab. Since chrome.tabs.executeScript checks the URL in the browser, the script would be denied, even though content scripts (checked in the renderer) would be allowed. In theory, these permissions should *not* be different. Similarly odd, if the user is using the local ntp (chrome-search://local-ntp/local-ntp.html), injection would already be disallowed in both the renderer and the browser. And, if we go waaaaay back, the NTP used to be pure WebUI with an URL of chrome://newtab, where injections were again disallowed. Rather than have inconsistent behavior depending on the type of script injection the extension uses, we want to have consistency throughout the system. [1] https://developer.chrome.com/webstore/program_policies#extensions I'll see if I can't post an announcement in chromium-extensions@ with this announcement (even though the policy's been published for awhile now, a little extra exposure couldn't hurt).
Message was sent while issue was closed.
> I'll see if I can't post an announcement in chromium-extensions@ with this > announcement (even though the policy's been published for awhile now, a little > extra exposure couldn't hurt). Please do post this — I just spent a bunch of time debugging why our extension was no longer adding a link to the new tab page, and I'd have seen it if you posted to the list :). (As it was I had to find this commit in the git log, because https://bugs.chromium.org/p/chromium/issues/detail?id=662610 cannot be viewed). Conrad |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
