Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
352523003:
Have the Debugger extension api check that it has access to the tab (Closed)
DescriptionHave the Debugger extension api check that it has access to the tab
Check PermissionsData::CanAccessTab() prior to attaching the debugger.
BUG=367567
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=280354
Patch Set 1 #
Total comments: 10
Patch Set 2 : #
Total comments: 26
Patch Set 3 : Mustafa's #Patch Set 4 : Add PermissionsData::UrlIsRestricted() #
Total comments: 13
Patch Set 5 : #Patch Set 6 : Test fixes #
Total comments: 4
Patch Set 7 : #Messages
Total messages: 30 (0 generated)
Minimalist patch to address the main issue. https://codereview.chromium.org/352523003/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://codereview.chromium.org/352523003/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/debugger/debugger_api.cc:627: Ben, I think you were mentioning that we should also do a check here for where access to chrome urls could count for chrome-extension urls. Do you still want that?
Would be great to have some tests. Also, are you planning to do something with the command line flag? The same flag can be used for enabling the debugging of chrome-extension:// pages for backward compatibility. https://codereview.chromium.org/352523003/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://codereview.chromium.org/352523003/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/debugger/debugger_api.cc:623: -1, // igore process id. igore -> ignore https://codereview.chromium.org/352523003/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/debugger/debugger_api.cc:625: return false; You can print an error message here too.
https://codereview.chromium.org/352523003/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://codereview.chromium.org/352523003/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/debugger/debugger_api.cc:601: agent_host_->GetRenderViewHost(), extension->name()); what is the lifetime/behaviour of constructing the infobar here and then later (with your permission check) not using it? for safety's sake we might as well do the permission check you're adding before attempting this. https://codereview.chromium.org/352523003/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/debugger/debugger_api.cc:619: extension, tracing this down, it looks like this is backwards incompatible; extensions didn't previously need to declare host permissions, now they do. could you add a special-case for the debugger permission to PermissionData::CanRunOnPage? we *should* be able to back that change out in manifest v3 since I can't see anything that allows the API to break site isolation (e.g. webRequest can break site isolation) but cross that bridge. https://codereview.chromium.org/352523003/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/debugger/debugger_api.cc:627: On 2014/06/23 19:17:47, Devlin wrote: > Ben, I think you were mentioning that we should also do a check here for where > access to chrome urls could count for chrome-extension urls. Do you still want > that? I think we should just do that as part of the low-level code which checks the effect of that flag, but it's a mostly unrelated change (just happens to be relevant after this change).
Update: It looks like we already _did_ block against chrome:// scheme (in fact, all web_ui schemes) - just not against extension pages. I've put the guard for that in, and made sure that if the switch for chrome urls is present, we allow it. Also added a test. https://codereview.chromium.org/352523003/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://codereview.chromium.org/352523003/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/debugger/debugger_api.cc:601: agent_host_->GetRenderViewHost(), extension->name()); On 2014/06/23 19:34:19, kalman wrote: > what is the lifetime/behaviour of constructing the infobar here and then later > (with your permission check) not using it? > > for safety's sake we might as well do the permission check you're adding before > attempting this. Done. https://codereview.chromium.org/352523003/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/debugger/debugger_api.cc:619: extension, On 2014/06/23 19:34:19, kalman wrote: > tracing this down, it looks like this is backwards incompatible; extensions > didn't previously need to declare host permissions, now they do. > > could you add a special-case for the debugger permission to > PermissionData::CanRunOnPage? we *should* be able to back that change out in > manifest v3 since I can't see anything that allows the API to break site > isolation (e.g. webRequest can break site isolation) but cross that bridge. I don't really like the idea of giving any extension with the debugger permission access to all pages, even if we already warned the user about it. With the new patch, though, we don't need to. https://codereview.chromium.org/352523003/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/debugger/debugger_api.cc:623: -1, // igore process id. On 2014/06/23 19:29:07, Mustafa Emre Acer wrote: > igore -> ignore Done. https://codereview.chromium.org/352523003/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/debugger/debugger_api.cc:625: return false; On 2014/06/23 19:29:07, Mustafa Emre Acer wrote: > You can print an error message here too. Moot now (but |error_| would have been populated via CanAccessPage()).
https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_api.cc:308: bool CanRunOnOtherExtensionPages(const Extension* extension) { nit: CanDebugExtensionPages sounds slightly more descriptive. https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_api.cc:309: return extension->permissions_data()->CanExecuteScriptEverywhere( Static method, you can use PermissionsData::CanExecuteScriptEverywhere instead. https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_api.cc:312: switches::kExtensionsOnChromeURLs); Is kExtensionsOnChromeURLs the right switch? Looks like kSilentDebuggerExtensionAPI was actually introduced for this purpose (debugging without infobars). https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/debugger/debugger_apitest.cc (right): https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_apitest.cc:41: base::CommandLine* command_line() { return command_line_; } command_line() const {..}? https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_apitest.cc:70: const std::string& url, const std::string& expected_error) { nit: Could pass |url| as a GURL. https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_apitest.cc:73: browser()->tab_strip_model()->GetActiveWebContents(); Two more spaces https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_apitest.cc:109: nit: remove empty line https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_apitest.cc:131: EXPECT_TRUE(RunAttachFunction( nit: It's somewhat unintuitive to get EXPECT_TRUE from all test cases when they different opposite things. Maybe rename to CanAttachFunction and test for true and false?
https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_api.cc:308: bool CanRunOnOtherExtensionPages(const Extension* extension) { On 2014/06/24 18:15:54, Mustafa Emre Acer wrote: > nit: CanDebugExtensionPages sounds slightly more descriptive. +1 though I'm still worried that a blacklist is just going to regress. I don't like that this API uses a different permissions mechanism than the rest of chrome. can we at least put a static method in PermissionsData like IsRestricted which takes an extension + GURL, which checks this flag + things like whether it's a component extension? https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_api.cc:312: switches::kExtensionsOnChromeURLs); On 2014/06/24 18:15:54, Mustafa Emre Acer wrote: > Is kExtensionsOnChromeURLs the right switch? Looks like > kSilentDebuggerExtensionAPI was actually introduced for this purpose (debugging > without infobars). kSilentDebuggerExtensionAPI is subtly different, and the whole reason this bug exists is because only that flag was being checked.
https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_api.cc:308: bool CanRunOnOtherExtensionPages(const Extension* extension) { On 2014/06/24 18:15:54, Mustafa Emre Acer wrote: > nit: CanDebugExtensionPages sounds slightly more descriptive. Done. https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_api.cc:309: return extension->permissions_data()->CanExecuteScriptEverywhere( On 2014/06/24 18:15:54, Mustafa Emre Acer wrote: > Static method, you can use PermissionsData::CanExecuteScriptEverywhere instead. Whoops! Done. https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_api.cc:312: switches::kExtensionsOnChromeURLs); On 2014/06/24 18:15:54, Mustafa Emre Acer wrote: > Is kExtensionsOnChromeURLs the right switch? Looks like > kSilentDebuggerExtensionAPI was actually introduced for this purpose (debugging > without infobars). I'm not sure - the chrome urls switch was taken from Ben's comment in the bug. But I'm not sure the silent debugging is right, either - can't we attach an infobar to an extension's page? Otherwise, we actually don't need to do anything with this bug (since we already check the infobar requirement). https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/debugger/debugger_apitest.cc (right): https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_apitest.cc:41: base::CommandLine* command_line() { return command_line_; } On 2014/06/24 18:15:54, Mustafa Emre Acer wrote: > command_line() const {..}? Sure. Didn't originally because it _does_ modify the command line, but the test object is untouched, I suppose. https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_apitest.cc:70: const std::string& url, const std::string& expected_error) { On 2014/06/24 18:15:54, Mustafa Emre Acer wrote: > nit: Could pass |url| as a GURL. Yeah, just made the test code look more cluttered to me. But I don't feel strongly. Done. https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_apitest.cc:73: browser()->tab_strip_model()->GetActiveWebContents(); On 2014/06/24 18:15:54, Mustafa Emre Acer wrote: > Two more spaces Done. https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_apitest.cc:109: On 2014/06/24 18:15:54, Mustafa Emre Acer wrote: > nit: remove empty line Done. https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_apitest.cc:131: EXPECT_TRUE(RunAttachFunction( On 2014/06/24 18:15:54, Mustafa Emre Acer wrote: > nit: It's somewhat unintuitive to get EXPECT_TRUE from all test cases when they > different opposite things. Maybe rename to CanAttachFunction and test for true > and false? It makes the code messier. If we just test for true and false, we can't necessarily also test for the error, unless we, e.g., populate an out parameter and compare it there. Then we also have to pass in a bool for if we expect an error. Even then, it gets somewhat tricky - what if we can attach it but fail to detach it? Do we return true (because we could attach) or false (because we failed)? If it would make it more clear, I could move all the logic into the main test body, or make two functions (one for run successfully, one for fail and expect error) but I personally find the former a bit harder to read and the latter results in more code. Preferences?
https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_api.cc:308: bool CanRunOnOtherExtensionPages(const Extension* extension) { On 2014/06/24 18:35:08, kalman wrote: > On 2014/06/24 18:15:54, Mustafa Emre Acer wrote: > > nit: CanDebugExtensionPages sounds slightly more descriptive. > > +1 > > though I'm still worried that a blacklist is just going to regress. I don't like > that this API uses a different permissions mechanism than the rest of chrome. > > can we at least put a static method in PermissionsData like IsRestricted which > takes an extension + GURL, which checks this flag + things like whether it's a > component extension? I'm not entirely sure I follow - won't it _still_ use a different permission mechanism, and just have the mechanism be in a different class? I'd be worried that if we do that, it increases the chance that other callers will accidentally use that one. If it's only used in this file, is there a compelling reason to move it? (I'd like to avoid putting yet-more one-call-site-only functions into PermissionsData...)
https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_api.cc:529: GURL url = web_contents->GetVisibleURL(); Better to use GetLastCommittedURL instead. https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_api.cc:618: HasSwitch(::switches::kSilentDebuggerExtensionAPI)) { Is this flag going to be redundant with this change? !kExtensionsOnChromeURLs && !kSilentDebuggerExtensionAPI = can only debug web pages kExtensionsOnChromeURLs && !kSilentDebuggerExtensionAPI = can debug all pages !kExtensionsOnChromeURLs && kSilentDebuggerExtensionAPI = can debug bg but not chrome-extensions kExtensionsOnChromeURLs && kSilentDebuggerExtensionAPI = can debug all pages Given that the only difference between a bg page and chrome-extension page is showing the infobar, kSilentDebuggerExtensionAPI doesn't have any value any more?
https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/debugger/debugger_apitest.cc (right): https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_apitest.cc:131: EXPECT_TRUE(RunAttachFunction( On 2014/06/24 18:45:40, Devlin wrote: > On 2014/06/24 18:15:54, Mustafa Emre Acer wrote: > > nit: It's somewhat unintuitive to get EXPECT_TRUE from all test cases when > they > > different opposite things. Maybe rename to CanAttachFunction and test for true > > and false? > > It makes the code messier. If we just test for true and false, we can't > necessarily also test for the error, unless we, e.g., populate an out parameter > and compare it there. Then we also have to pass in a bool for if we expect an > error. Even then, it gets somewhat tricky - what if we can attach it but fail > to detach it? Do we return true (because we could attach) or false (because we > failed)? If it would make it more clear, I could move all the logic into the > main test body, or make two functions (one for run successfully, one for fail > and expect error) but I personally find the former a bit harder to read and the > latter results in more code. > > Preferences? Thanks, makes sense. Sounds good as it is.
https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_api.cc:308: bool CanRunOnOtherExtensionPages(const Extension* extension) { On 2014/06/24 18:49:08, Devlin wrote: > On 2014/06/24 18:35:08, kalman wrote: > > On 2014/06/24 18:15:54, Mustafa Emre Acer wrote: > > > nit: CanDebugExtensionPages sounds slightly more descriptive. > > > > +1 > > > > though I'm still worried that a blacklist is just going to regress. I don't > like > > that this API uses a different permissions mechanism than the rest of chrome. > > > > can we at least put a static method in PermissionsData like IsRestricted which > > takes an extension + GURL, which checks this flag + things like whether it's a > > component extension? > > I'm not entirely sure I follow - won't it _still_ use a different permission > mechanism, and just have the mechanism be in a different class? I'd be worried > that if we do that, it increases the chance that other callers will accidentally > use that one. If it's only used in this file, is there a compelling reason to > move it? (I'd like to avoid putting yet-more one-call-site-only functions into > PermissionsData...) IsRestricted would be checked from CanAccessPage, so it'd be called in 2 places. I imagine there are other places in the codebase where we should be using that function as well.
https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_api.cc:308: bool CanRunOnOtherExtensionPages(const Extension* extension) { On 2014/06/24 19:07:51, kalman wrote: > On 2014/06/24 18:49:08, Devlin wrote: > > On 2014/06/24 18:35:08, kalman wrote: > > > On 2014/06/24 18:15:54, Mustafa Emre Acer wrote: > > > > nit: CanDebugExtensionPages sounds slightly more descriptive. > > > > > > +1 > > > > > > though I'm still worried that a blacklist is just going to regress. I don't > > like > > > that this API uses a different permissions mechanism than the rest of > chrome. > > > > > > can we at least put a static method in PermissionsData like IsRestricted > which > > > takes an extension + GURL, which checks this flag + things like whether it's > a > > > component extension? > > > > I'm not entirely sure I follow - won't it _still_ use a different permission > > mechanism, and just have the mechanism be in a different class? I'd be > worried > > that if we do that, it increases the chance that other callers will > accidentally > > use that one. If it's only used in this file, is there a compelling reason to > > move it? (I'd like to avoid putting yet-more one-call-site-only functions > into > > PermissionsData...) > > IsRestricted would be checked from CanAccessPage, so it'd be called in 2 places. > I imagine there are other places in the codebase where we should be using that > function as well. Done... https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_api.cc:529: GURL url = web_contents->GetVisibleURL(); On 2014/06/24 18:54:27, Mustafa Emre Acer wrote: > Better to use GetLastCommittedURL instead. Agreed. And tried that first. Unfortunately, this broke one of the tests (haven't investigated enough as to why). Since the deprecated GetURL() is just a forward of GetVisibleURL(), I figured it was okay to leave it functionally as it has been for now. https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_api.cc:618: HasSwitch(::switches::kSilentDebuggerExtensionAPI)) { On 2014/06/24 18:54:27, Mustafa Emre Acer wrote: > Is this flag going to be redundant with this change? > > !kExtensionsOnChromeURLs && !kSilentDebuggerExtensionAPI = can only debug web > pages > kExtensionsOnChromeURLs && !kSilentDebuggerExtensionAPI = can debug all pages > !kExtensionsOnChromeURLs && kSilentDebuggerExtensionAPI = can debug bg but not > chrome-extensions > kExtensionsOnChromeURLs && kSilentDebuggerExtensionAPI = can debug all pages > > Given that the only difference between a bg page and chrome-extension page is > showing the infobar, kSilentDebuggerExtensionAPI doesn't have any value any > more? Hmm... good question. The thing is, we don't even create the InfoBar if we don't have the switch on, so it could also be a nice ui feature for people who don't wanna see it. WDYT?
On 2014/06/24 19:54:55, Devlin wrote: > https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... > File chrome/browser/extensions/api/debugger/debugger_api.cc (right): > > https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... > chrome/browser/extensions/api/debugger/debugger_api.cc:308: bool > CanRunOnOtherExtensionPages(const Extension* extension) { > On 2014/06/24 19:07:51, kalman wrote: > > On 2014/06/24 18:49:08, Devlin wrote: > > > On 2014/06/24 18:35:08, kalman wrote: > > > > On 2014/06/24 18:15:54, Mustafa Emre Acer wrote: > > > > > nit: CanDebugExtensionPages sounds slightly more descriptive. > > > > > > > > +1 > > > > > > > > though I'm still worried that a blacklist is just going to regress. I > don't > > > like > > > > that this API uses a different permissions mechanism than the rest of > > chrome. > > > > > > > > can we at least put a static method in PermissionsData like IsRestricted > > which > > > > takes an extension + GURL, which checks this flag + things like whether > it's > > a > > > > component extension? > > > > > > I'm not entirely sure I follow - won't it _still_ use a different permission > > > mechanism, and just have the mechanism be in a different class? I'd be > > worried > > > that if we do that, it increases the chance that other callers will > > accidentally > > > use that one. If it's only used in this file, is there a compelling reason > to > > > move it? (I'd like to avoid putting yet-more one-call-site-only functions > > into > > > PermissionsData...) > > > > IsRestricted would be checked from CanAccessPage, so it'd be called in 2 > places. > > I imagine there are other places in the codebase where we should be using that > > function as well. > > Done... > > https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... > chrome/browser/extensions/api/debugger/debugger_api.cc:529: GURL url = > web_contents->GetVisibleURL(); > On 2014/06/24 18:54:27, Mustafa Emre Acer wrote: > > Better to use GetLastCommittedURL instead. > > Agreed. And tried that first. Unfortunately, this broke one of the tests > (haven't investigated enough as to why). Since the deprecated GetURL() is just > a forward of GetVisibleURL(), I figured it was okay to leave it functionally as > it has been for now. > > https://codereview.chromium.org/352523003/diff/20001/chrome/browser/extension... > chrome/browser/extensions/api/debugger/debugger_api.cc:618: > HasSwitch(::switches::kSilentDebuggerExtensionAPI)) { > On 2014/06/24 18:54:27, Mustafa Emre Acer wrote: > > Is this flag going to be redundant with this change? > > > > !kExtensionsOnChromeURLs && !kSilentDebuggerExtensionAPI = can only debug web > > pages > > kExtensionsOnChromeURLs && !kSilentDebuggerExtensionAPI = can debug all pages > > !kExtensionsOnChromeURLs && kSilentDebuggerExtensionAPI = can debug bg but not > > > chrome-extensions > > kExtensionsOnChromeURLs && kSilentDebuggerExtensionAPI = can debug all pages > > > > Given that the only difference between a bg page and chrome-extension page is > > showing the infobar, kSilentDebuggerExtensionAPI doesn't have any value any > > more? > > Hmm... good question. > > The thing is, we don't even create the InfoBar if we don't have the switch on, > so it could also be a nice ui feature for people who don't wanna see it. WDYT? Sure, I also think showing the infobar is useful. But kSilentDebuggerExtensionAPI now only controls debugging background pages (since there is no infobar), which kExtensionsOnChromeURLs already covers, right? So kSilentDebuggerExtensionAPI doesn't seem to be worth keeping around anymore, unless I'm missing anything.
On 2014/06/24 20:50:28, Mustafa Emre Acer wrote: > Sure, I also think showing the infobar is useful. But > kSilentDebuggerExtensionAPI now only controls debugging background pages (since > there is no infobar), which kExtensionsOnChromeURLs already covers, right? So > kSilentDebuggerExtensionAPI doesn't seem to be worth keeping around anymore, > unless I'm missing anything. Sorry, I must have been unclear. If we _don't_ have the silent debugger switch on, we show an infobar (regardless of page). If we _do_ have the silent debugger switch on, we don't show an infobar (regardless of page). Thus, the switch is still useful if someone does not want to see an infobar while debugging. In either case, we should probably take out the error message (since it shouldn't be reached), but we might want to keep the switch in.
On 2014/06/24 20:53:52, Devlin wrote: > On 2014/06/24 20:50:28, Mustafa Emre Acer wrote: > > Sure, I also think showing the infobar is useful. But > > kSilentDebuggerExtensionAPI now only controls debugging background pages > (since > > there is no infobar), which kExtensionsOnChromeURLs already covers, right? So > > kSilentDebuggerExtensionAPI doesn't seem to be worth keeping around anymore, > > unless I'm missing anything. > > Sorry, I must have been unclear. > > If we _don't_ have the silent debugger switch on, we show an infobar (regardless > of page). > If we _do_ have the silent debugger switch on, we don't show an infobar > (regardless of page). > Thus, the switch is still useful if someone does not want to see an infobar > while debugging. > > In either case, we should probably take out the error message (since it > shouldn't be reached), but we might want to keep the switch in. Oh, I see. The flag essentially just controls showing/hiding the infobar rather than giving any more privileges to extensions. Sorry for the confusion.
lgtm but some actionable things https://codereview.chromium.org/352523003/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://codereview.chromium.org/352523003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_api.cc:521: GURL url = web_contents->GetVisibleURL(); yeah this really should be GetLastCommittedURL. could you add a TODO or something. https://codereview.chromium.org/352523003/diff/60001/extensions/common/permis... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/352523003/diff/60001/extensions/common/permis... extensions/common/permissions/permissions_data.cc:78: bool can_execute_everywhere = CanExecuteScriptEverywhere(extension); this is a bit silly, because every condition checks !can_execute_script_everywhere. so we can save a bunch of lines of code by just doing if (CanExecuteScriptEverywhere(extension)) { return false; } and not worry about |can_execute_script_everywhere| https://codereview.chromium.org/352523003/diff/60001/extensions/common/permis... extensions/common/permissions/permissions_data.cc:84: bool has_switch = base::CommandLine::ForCurrentProcess()->HasSwitch( please call this allow_chrome_urls or something. https://codereview.chromium.org/352523003/diff/60001/extensions/common/permis... File extensions/common/permissions/permissions_data.h (right): https://codereview.chromium.org/352523003/diff/60001/extensions/common/permis... extensions/common/permissions/permissions_data.h:74: static bool UrlIsRestricted(const GURL& document_url, please call this IsRestrictedURL, or IsRestrictedUrl if you prefer (I typically prefer Url but URL is more common in chrome). https://codereview.chromium.org/352523003/diff/60001/extensions/common/permis... extensions/common/permissions/permissions_data.h:77: std::string* error); it would also be nice to test this function (complete with the command line flag logic). shouldn't take too long.
https://codereview.chromium.org/352523003/diff/60001/extensions/common/permis... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/352523003/diff/60001/extensions/common/permis... extensions/common/permissions/permissions_data.cc:74: bool PermissionsData::UrlIsRestricted(const GURL& document_url, Would these schemes/urls be covered by this method? - about:settings, about:version etc. - view-source: - data: - chrome-devtools: (can you debug the debugger?) - filesystem: - mailto: or any external protocol (not sure what happens if you try to debug them)
https://codereview.chromium.org/352523003/diff/60001/extensions/common/permis... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/352523003/diff/60001/extensions/common/permis... extensions/common/permissions/permissions_data.cc:74: bool PermissionsData::UrlIsRestricted(const GURL& document_url, On 2014/06/25 21:24:25, Mustafa Emre Acer wrote: > Would these schemes/urls be covered by this method? > > - about:settings, about:version etc. > - view-source: > - data: > - chrome-devtools: (can you debug the debugger?) > - filesystem: > - mailto: or any external protocol (not sure what happens if you try to debug > them) oh, good point. we should also check the whitelisted set of origins, which is checked on extension installation, but we should check here tool.
https://codereview.chromium.org/352523003/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://codereview.chromium.org/352523003/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/debugger/debugger_api.cc:521: GURL url = web_contents->GetVisibleURL(); On 2014/06/25 20:42:16, kalman wrote: > yeah this really should be GetLastCommittedURL. could you add a TODO or > something. Done. https://codereview.chromium.org/352523003/diff/60001/extensions/common/permis... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/352523003/diff/60001/extensions/common/permis... extensions/common/permissions/permissions_data.cc:74: bool PermissionsData::UrlIsRestricted(const GURL& document_url, On 2014/06/25 21:26:27, kalman wrote: > On 2014/06/25 21:24:25, Mustafa Emre Acer wrote: > > Would these schemes/urls be covered by this method? > > > > - about:settings, about:version etc. > > - view-source: > > - data: > > - chrome-devtools: (can you debug the debugger?) > > - filesystem: > > - mailto: or any external protocol (not sure what happens if you try to debug > > them) > > oh, good point. we should also check the whitelisted set of origins, which is > checked on extension installation, but we should check here tool. Done. https://codereview.chromium.org/352523003/diff/60001/extensions/common/permis... extensions/common/permissions/permissions_data.cc:78: bool can_execute_everywhere = CanExecuteScriptEverywhere(extension); On 2014/06/25 20:42:17, kalman wrote: > this is a bit silly, because every condition checks > !can_execute_script_everywhere. so we can save a bunch of lines of code by just > doing > > if (CanExecuteScriptEverywhere(extension)) { > return false; > } > > and not worry about |can_execute_script_everywhere| Done. https://codereview.chromium.org/352523003/diff/60001/extensions/common/permis... extensions/common/permissions/permissions_data.cc:84: bool has_switch = base::CommandLine::ForCurrentProcess()->HasSwitch( On 2014/06/25 20:42:17, kalman wrote: > please call this allow_chrome_urls or something. Done. https://codereview.chromium.org/352523003/diff/60001/extensions/common/permis... File extensions/common/permissions/permissions_data.h (right): https://codereview.chromium.org/352523003/diff/60001/extensions/common/permis... extensions/common/permissions/permissions_data.h:74: static bool UrlIsRestricted(const GURL& document_url, On 2014/06/25 20:42:17, kalman wrote: > please call this IsRestrictedURL, or IsRestrictedUrl if you prefer (I typically > prefer Url but URL is more common in chrome). Done. https://codereview.chromium.org/352523003/diff/60001/extensions/common/permis... extensions/common/permissions/permissions_data.h:77: std::string* error); On 2014/06/25 20:42:17, kalman wrote: > it would also be nice to test this function (complete with the command line flag > logic). shouldn't take too long. Done.
Lgtm. Thanks Devlin.
lgtm
Please take a quick look at the delta between patches 5 and 6.
https://codereview.chromium.org/352523003/diff/100001/extensions/common/permi... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/352523003/diff/100001/extensions/common/permi... extensions/common/permissions/permissions_data.cc:84: // Hmm.... > about urls are valid Is this because of the somewhat recent change that allowed content scripts on about: pages? AFAIR that CL only modified "matches" and didn't add a host permission for about:, but still. That change also checked if the parent of the about: page was owned by a page which the extension can access, so I'm not sure if document_url.SchemeIs(url::kAboutScheme) will be too broad of a check?
https://codereview.chromium.org/352523003/diff/100001/extensions/common/permi... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/352523003/diff/100001/extensions/common/permi... extensions/common/permissions/permissions_data.cc:84: // Hmm.... On 2014/06/26 17:32:08, Mustafa Emre Acer wrote: > > about urls are valid > > Is this because of the somewhat recent change that allowed content scripts on > about: pages? AFAIR that CL only modified "matches" and didn't add a host > permission for about:, but still. > > That change also checked if the parent of the about: page was owned by a page > which the extension can access, so I'm not sure if > document_url.SchemeIs(url::kAboutScheme) will be too broad of a check? Mostly, this is going off the PermissionsData tests, in which we don't consider about:flags a restricted url (even though we can't script on it, we can capture it). Is this the right behavior? Most likely not. But I'm also not sure that no extensions rely on capturing about: urls. Note that this is doesn't allow anything that wasn't allowed before (unlike the chrome-extensions:// change).
https://codereview.chromium.org/352523003/diff/100001/extensions/common/permi... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/352523003/diff/100001/extensions/common/permi... extensions/common/permissions/permissions_data.cc:84: // Hmm.... On 2014/06/26 17:37:13, Devlin wrote: > On 2014/06/26 17:32:08, Mustafa Emre Acer wrote: > > > about urls are valid > > > > Is this because of the somewhat recent change that allowed content scripts on > > about: pages? AFAIR that CL only modified "matches" and didn't add a host > > permission for about:, but still. > > > > That change also checked if the parent of the about: page was owned by a page > > which the extension can access, so I'm not sure if > > document_url.SchemeIs(url::kAboutScheme) will be too broad of a check? > > Mostly, this is going off the PermissionsData tests, in which we don't consider > about:flags a restricted url (even though we can't script on it, we can capture > it). Is this the right behavior? Most likely not. But I'm also not sure that > no extensions rely on capturing about: urls. Note that this is doesn't allow > anything that wasn't allowed before (unlike the chrome-extensions:// change). If this is just so that tabCapture can work, can't you put this extra check into CanCaptureTab or whatever it's called? If not, a more specific comment would be better.
https://codereview.chromium.org/352523003/diff/100001/extensions/common/permi... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/352523003/diff/100001/extensions/common/permi... extensions/common/permissions/permissions_data.cc:84: // Hmm.... On 2014/06/26 18:11:28, kalman wrote: > On 2014/06/26 17:37:13, Devlin wrote: > > On 2014/06/26 17:32:08, Mustafa Emre Acer wrote: > > > > about urls are valid > > > > > > Is this because of the somewhat recent change that allowed content scripts > on > > > about: pages? AFAIR that CL only modified "matches" and didn't add a host > > > permission for about:, but still. > > > > > > That change also checked if the parent of the about: page was owned by a > page > > > which the extension can access, so I'm not sure if > > > document_url.SchemeIs(url::kAboutScheme) will be too broad of a check? > > > > Mostly, this is going off the PermissionsData tests, in which we don't > consider > > about:flags a restricted url (even though we can't script on it, we can > capture > > it). Is this the right behavior? Most likely not. But I'm also not sure > that > > no extensions rely on capturing about: urls. Note that this is doesn't allow > > anything that wasn't allowed before (unlike the chrome-extensions:// change). > > If this is just so that tabCapture can work, can't you put this extra check into > CanCaptureTab or whatever it's called? If not, a more specific comment would be > better. I meant this bug, not specific to tab capture: http://crbug.com/76429
Patchset #7 still lgtm.
The CQ bit was checked by rdevlin.cronin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/352523003/...
Message was sent while issue was closed.
Change committed as 280354 |
