[Extensions] Restrict browser.devtools to devtools page context [chromium/src : main]
Justin Lulejian (Gerrit)
Justin Lulejian voted and added 1 comment![]( "Open in Gerrit")
Votes added by Justin Lulejian
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
Hi Devlin! A follow-up fix to a fix for the browser namespace when you have a moment. Thanks!
Related details
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin added 2 comments![]( "Open in Gerrit")
Thanks, Justin! One question on this one
chrome_manifest_urls::GetDevToolsPage(context->extension())) {Hmm... what's the logic we use to determine if we expose chrome.devtools today?
I'm a bit worried about doing an exact URL check, since it would fail if there's ever, say, fragments or query params. I'm not sure whether that's a thing that could easily happen in a devtools page like this, but it seems like it _might_ be able to (it could through pushState, but that's a bit strange, since the page is already loaded at that point)
Related details
- Justin Lulejian
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Lulejian (Gerrit)
Justin Lulejian voted and added 2 comments![]( "Open in Gerrit")
Votes added by Justin Lulejian
| Auto-Submit | +1 |
| Commit-Queue | +1 |
2 comments
Hi Devlin! This should be ready to review prior to trybots finishing. Trybots just passed on the previous patchset and I've locally verified the relevant tests pass again after the changes I've made.
chrome_manifest_urls::GetDevToolsPage(context->extension())) {Hmm... what's the logic we use to determine if we expose chrome.devtools today?
I'm a bit worried about doing an exact URL check, since it would fail if there's ever, say, fragments or query params. I'm not sure whether that's a thing that could easily happen in a devtools page like this, but it seems like it _might_ be able to (it could through pushState, but that's a bit strange, since the page is already loaded at that point)
Hmm... what's the logic we use to determine if we expose chrome.devtools today?
It probably comes as not a surprise that it's complicated 😄
Code references for the current flow:
Browser-side: [devtools_ui_bindings.cc](https://crsrc.org/c/chrome/browser/devtools/devtools_ui_bindings.cc;l=2597-2605;drc=bf712ec1a13783224debb691ba88ad5c15b93194) — Checks if an extension has a devtools page and attempts to addExtension it.
Renderer-side (JS): [ExtensionServer.ts](https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/front_end/panels/common/ExtensionServer.ts;l=1341;drc=062cb72b54bf5df18d84536250f862e9c6e9c06f) — Performs numerous checks on whether a specific extension can be loaded for the given URL (e.g., blocking the Web Store).
Timing:
- devtools_ui_bindings.cc code runs to addExtension in ExtensionServer.ts
- ExtensionServer.ts addExtensionFrame() loads the page
- our bindings code runs (UpdateBindingsForContext)
- the devtools extension API [exposes](https://www.google.com/search?q=https://crsrc.org/c/third_party/devtools-frontend/src/front_end/models/extensions/ExtensionAPI.ts%3Bl%3D1538%3Bbpv%3D1%3Bbpt%3D0) chrome.devtools.
Because of this order, we can't simply check if chrome.devtools is already defined which would've been the easiest/simplest check.
I'm a bit worried about doing an exact URL check...
I agree this is fragile, and has flaws anyways so I came up with logic that I think covers it better. These must be true:
1. the context's must have an ancestor frame scheme that is devtools://
1. the context's URL is the extension's devtools page or is within an extension devtools page
The dual checks are both important because:
- checking the ancestor frame ensures that if a devtools page is loaded outside of the devtools frontend (e.g., via manual navigation or an iframe in an arbitrary page), browser.devtools won't be defined (devtools frontend won't be around to define chrome.devtools so we shouldn't)
- the devtools frontend could (if not now then in the future) request other extension contexts where chrome.devtools shouldn't be defined so we check the devtools_page key to avoid that
- if we just check only the literal extension devtools page, any iframes within it wouldn't get browser.devtools because their .html files wouldn't match the literal devtools_page .html file.
Related details
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin added 3 comments![]( "Open in Gerrit")
Thank you, Justin! And thanks for expanding the testing!
async function waitForDevTools() {I'm actually surprised this is necessary, since this means that it's very developer-perceptible that chrome.devtools might not be defined at a given point. I'm assuming this was added in response to flakiness you observed?
I'm curious, would waiting for onload work and be a bit more deterministic? If not, this is fine.
is_devtools_page_tree = true;optional: I think, technically, checking for the devtools page tree part of this is unnecessary -- if an extension frame is hosted within a devtools panel, it must (hopefully!) be part of the extension devtools page, and we check this with the combination of the origin check for the extension and the scheme check for devtools on the parents.
Related details
- Justin Lulejian
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Justin Lulejian
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Lulejian (Gerrit)
Justin Lulejian voted and added 3 comments![]( "Open in Gerrit")
Votes added by Justin Lulejian
| Auto-Submit | +1 |
| Commit-Queue | +2 |
3 comments
I'm actually surprised this is necessary, since this means that it's very developer-perceptible that chrome.devtools might not be defined at a given point. I'm assuming this was added in response to flakiness you observed?
I'm curious, would waiting for onload work and be a bit more deterministic? If not, this is fine.
I added it because it was 100% failing locally and I had a suspicion that it was a timing issue between how quickly the page loaded and how quickly the devtools frontend could loop through the frames to inject the devtools API on the chrome.devtools. window.onload is much cleaner and seemed to work the same, thanks!
optional: I think, technically, checking for the devtools page tree part of this is unnecessary -- if an extension frame is hosted within a devtools panel, it must (hopefully!) be part of the extension devtools page, and we check this with the combination of the origin check for the extension and the scheme check for devtools on the parents.
Done
chrome_manifest_urls::GetDevToolsPage(context->extension())) {Justin LulejianHmm... what's the logic we use to determine if we expose chrome.devtools today?
I'm a bit worried about doing an exact URL check, since it would fail if there's ever, say, fragments or query params. I'm not sure whether that's a thing that could easily happen in a devtools page like this, but it seems like it _might_ be able to (it could through pushState, but that's a bit strange, since the page is already loaded at that point)
Hmm... what's the logic we use to determine if we expose chrome.devtools today?
It probably comes as not a surprise that it's complicated 😄
Code references for the current flow:
Browser-side: [devtools_ui_bindings.cc](https://crsrc.org/c/chrome/browser/devtools/devtools_ui_bindings.cc;l=2597-2605;drc=bf712ec1a13783224debb691ba88ad5c15b93194) — Checks if an extension has a devtools page and attempts to addExtension it.
Renderer-side (JS): [ExtensionServer.ts](https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/front_end/panels/common/ExtensionServer.ts;l=1341;drc=062cb72b54bf5df18d84536250f862e9c6e9c06f) — Performs numerous checks on whether a specific extension can be loaded for the given URL (e.g., blocking the Web Store).
Timing:
- devtools_ui_bindings.cc code runs to addExtension in ExtensionServer.ts
- ExtensionServer.ts addExtensionFrame() loads the page
- our bindings code runs (UpdateBindingsForContext)
- the devtools extension API [exposes](https://www.google.com/search?q=https://crsrc.org/c/third_party/devtools-frontend/src/front_end/models/extensions/ExtensionAPI.ts%3Bl%3D1538%3Bbpv%3D1%3Bbpt%3D0) chrome.devtools.
Because of this order, we can't simply check if chrome.devtools is already defined which would've been the easiest/simplest check.
I'm a bit worried about doing an exact URL check...
I agree this is fragile, and has flaws anyways so I came up with logic that I think covers it better. These must be true:
1. the context's must have an ancestor frame scheme that is devtools://
1. the context's URL is the extension's devtools page or is within an extension devtools pageThe dual checks are both important because:
- checking the ancestor frame ensures that if a devtools page is loaded outside of the devtools frontend (e.g., via manual navigation or an iframe in an arbitrary page), browser.devtools won't be defined (devtools frontend won't be around to define chrome.devtools so we shouldn't)
- the devtools frontend could (if not now then in the future) request other extension contexts where chrome.devtools shouldn't be defined so we check the devtools_page key to avoid that
- if we just check only the literal extension devtools page, any iframes within it wouldn't get browser.devtools because their .html files wouldn't match the literal devtools_page .html file.
Done
Related details
- Justin Lulejian
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
14 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: extensions/renderer/native_extension_bindings_system.cc
Insertions: 2, Deletions: 23.
@@ -477,13 +477,6 @@
return false;
}
- // If the current frame is the devtools page itself, or if it is an iframe
- // nested within the devtools page.
- bool is_devtools_page_tree = false;
- if (context->url().path() == devtools_page_url.path()) {
- is_devtools_page_tree = true;
- }
-
// The devtools page is hosted in an iframe in the devtools frontend.
blink::WebLocalFrame* web_frame = context->web_frame();
if (!web_frame) {
@@ -492,31 +485,17 @@
// To prevent false positives (e.g. manually navigating to the devtools page
// in a regular tab), we must verify that the frame hierarchy is actually
- // rooted in the devtools frontend (which has the `devtools://` scheme). We
- // track this with `is_hosted_by_devtools`. If we also find the context is a
- // frame within the devtools page then we update `is_devtools_page_tree` too.
- bool is_hosted_by_devtools = false;
+ // rooted in the devtools frontend (which has the `devtools://` scheme).
blink::WebFrame* parent = web_frame->Parent();
while (parent) {
if (parent->GetSecurityOrigin().Protocol().Utf8() ==
content::kChromeDevToolsScheme) {
- is_hosted_by_devtools = true;
- }
- if (parent->IsWebLocalFrame()) {
- blink::WebLocalFrame* local_parent = parent->ToWebLocalFrame();
- if (GURL(local_parent->GetDocument().Url()).path() ==
- devtools_page_url.path()) {
- is_devtools_page_tree = true;
- }
- }
- if (is_hosted_by_devtools && is_devtools_page_tree) {
return true;
}
- // Continue going up the frame hierarchy.
parent = parent->Parent();
}
- return is_hosted_by_devtools && is_devtools_page_tree;
+ return false;
}
} // namespace
```
```
The name of the file: chrome/browser/extensions/native_bindings_apitest.cc
Insertions: 15, Deletions: 30.
@@ -870,37 +870,22 @@
test_dir.WriteFile(FILE_PATH_LITERAL("child.html"),
"<script src='child.js'></script>");
test_dir.WriteFile(FILE_PATH_LITERAL("child.js"),
- R"(chrome.test.runTests([
- // It takes a bit of time for the devtools frontend to
- // inject the API since this is a iframe. So to avoid
- // test flakiness we wait for chrome.devtools to be
- // defined before proceeding with the test.
- async function waitForDevTools() {
- const start = Date.now();
- // 2 second timeout
- while (Date.now() - start < 2000) {
- if (chrome.devtools) {
- chrome.test.succeed();
- return;
- }
- await new Promise(r => setTimeout(r, 50));
+ R"(window.onload = function() {
+ chrome.test.runTests([
+ function checkNestedFrameHasDevTools() {
+ chrome.test.assertTrue(
+ chrome.hasOwnProperty('devtools'));
+ chrome.test.assertNe(undefined, chrome.devtools);
+ chrome.test.assertTrue(browser.hasOwnProperty(
+ 'devtools'));
+ chrome.test.assertNe(undefined,
+ browser.devtools);
+ chrome.test.assertEq(chrome.devtools,
+ browser.devtools);
+ chrome.test.succeed();
}
- chrome.test.fail('Timed out waiting for ' +
- 'devtools frontend to define chrome.devtools.');
- },
- async function checkNestedFrameHasDevTools() {
- chrome.test.assertTrue(
- chrome.hasOwnProperty('devtools'));
- chrome.test.assertNe(undefined, chrome.devtools);
- chrome.test.assertTrue(browser.hasOwnProperty(
- 'devtools'));
- chrome.test.assertNe(undefined,
- browser.devtools);
- chrome.test.assertEq(chrome.devtools,
- browser.devtools);
- chrome.test.succeed();
- }
- ]);)");
+ ]);
+ };)");
ResultCatcher catcher;
const Extension* extension = LoadExtension(test_dir.UnpackedPath());
```
Change information
[Extensions] Restrict browser.devtools to devtools page context
browser.devtools is an alias of chrome.devtools when the browser
namespace feature is enabled. Since chrome.devtools is only injected by
the DevTools frontend into the extension's devtools page,
browser.devtools should also only be exposed in that context.
Before this change the browser namespace feature would always define the
devtools key on the browser object in all extensions contexts, which
caused confusion for developers who would perform tests like `'devtools
in browser'`.
This change prevents browser.devtools from being defined in other
extension contexts (like background scripts, extensions pages, injected
contents scripts, and etc.) where the devtools API would otherwise not
be set on the chrome object at all. It does this by comparing the
current context URL to the expected devtools page URL in the extension's
manifest and checking to see if the context is being hosted by the
devtools frontend.
This also condenses the devtools visibility tests into one called
"ChromeAndBrowserObjects_DevToolsVisibility" to fully confirm that the
devtools key should only be defined in a devtools page context and
virtually nowhere else. It adds two addition tests to test edge cases
involving loading the devtools page outside the devtools context and
when the context is an iframe of the extension's devtools page.
- M chrome/browser/extensions/native_bindings_apitest.cc
- M extensions/renderer/native_extension_bindings_system.cc
Code-Review: +1 by Devlin Cronin
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |