Issue metadata
Sign in to add a comment
|
chrome.debugger.attach fails on Google Drive/Docs when using Google Chrome
Reported by
jona.d.k...@gmail.com,
Sep 18
|
||||||||||||||||||||
Issue description
UserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.100 Safari/537.36
Steps to reproduce the problem:
1. Install the Attachment Tester extension that is attached (or any other extension that uses the chrome.debugger API)
2. Attempt to attach the debugger to any tab where Google Drive or Google Docs is signed in and loaded.
3. Attachment fails -- chrome.runtime.lastError reports that it "Cannot attach to this target."
What is the expected behavior?
Attachment succeeds, and the application can now send commands using chrome.debugger.sendCommand to that tab.
What went wrong?
Attachment fails with very little explanation: chrome.runtime.lastError contains {message: "Cannot attach to this target."}
Did this work before? Yes 68.0.3440.84
Does this work in other browsers? Yes
Chrome version: 69.0.3497.100 Channel: stable
OS Version: 10.0
Flash Version:
The attached file is a tiny extension that shows whether it can attach to the currently active tab (just click the extension icon to test) -- source at https://github.com/jdkula/chrome-attach-tester
This bug is NOT reproducible on any version of Chromium (tested on 69.0.3497.81 and 71.0.3555.0) -- only in Google Chrome. I've been able to reproduce it in both Google Chrome Stable 69.0.3497.100 and Canary 71.0.3554.0. I've only tested on Windows.
,
Sep 19
Tried checking the issue on chrome reported version# 69.0.3497.100 using Windows-10 with steps mentioned below: 1) Launched chrome reported version and installed the extension provided in comment#0 2) Signed in into Google Drive and opened Devtools>Sources, under the page tab selected one instance and applied breakpoint to it 3) Clicked on "Pause Script Execution" button, then it shows "paused in debugger" 4) Observed some warning messaged in Devtools>Console tab @Reporter: Please find the attached screencast for your reference and let us know if we missed anything in reproducing the issue, provide your feedback on it which help in further triaging it. If possible could you please provide screencast for the issue which help in better understanding. Note: Tentatively adding Platform>Extensions & Platform>DevTools components to it. Thanks!
,
Sep 20
Hello! Apologies, I wasn't very clear. I was referring to programmatically attaching to the debugger using the extension API ( See: https://developer.chrome.com/extensions/debugger#method-attach ) The extension I bundled uses this API to try and connect to the current tab. If you click the browser action icon for the extension, it will display a checkmark if it's successful and an exclamation mark if not. I've attached a slightly more helpful version of the extension below. I've also provided a video that I hope is helpful in identifying the problem: https://streamable.com/m8syf -- when applicable, the extension outputs the contents of chrome.runtime.lastError. Thank you all! Please let me know if I can help clarify anything.
,
Sep 20
Thank you for providing more feedback. Adding the requester to the cc list. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 20
,
Sep 20
Able to reproduce the issue on reported version 69.0.3497.100 and latest chrome 71.0.3556.0 using Mac 10.12.6, Ubuntu 14.04 and Windows-10, hence providing Bisect Info Bisect Info: ================ Good build: 69.0.3469.0 Bad build: 69.0.3472.0 You are probably looking for a change made after 569827 (known good), but no later than 569828 (first known bad). https://chromium.googlesource.com/chromium/src/+log/b37053ca6b95f4fbbea22eb5ebb795fb31ffe4c3..4dafc317b6e614bdd86ea3e2f5c2fc2e8518a877 Change-Id: Icb5ee89bf788643eee166eef83802d10ab825a6c Reviewed-on: https://chromium-review.googlesource.com/1104954 @Devlin Cronin: Please confirm the issue and help in re-assigning if it is not related to your change. Adding ReleaseBlock-Stable as it is seems a recent break, feel free to remove it if not applicable. Thanks!
,
Sep 24
I'm unable to reproduce this locally - the extension seems to attach to the site without issue. dgozman@, any ideas, and/or can you successfully reproduce this?
,
Sep 24
,
Sep 27
The NextAction date has arrived: 2018-09-27
,
Sep 30
For what it's worth, I'm still seeing this behavior in Chrome Canary Version 71.0.3566.0. If you're unable to reproduce it, can you please ensure you're signed in to Google Drive (not on the landing page that asks you to sign in), and that you're using Google Chrome and not Chromium? Chromium does not appear to exhibit this bug. Thank you!
,
Oct 4
I had some time to dive into this a bit more, and was able to figure it out (at least, what the cause is). The reason this is failing to attach is because the Google Drive hosted app [1] is installed. This is also why it works fine on Chromium, but not on Google Chrome (Chrome bundles Google Drive by default; Chromium doesn't). When the extension tries to attach to drive.google.com, we check ExtensionCanAttachToURL() [2] with the visible URL of the web contents [3] (note that there's a TODO to update that to GetLastCommittedURL(), but that wouldn't make a difference here). This visible URL is drive.google.com, and the extension is allowed to attach. However, in the process of actually attaching the host, we then reach RenderFrameDevToolsAgentHost::ShouldAllowSession(), which calls into DevToolsAgentHostClient::MayAttachToRenderer() [4]. This calls back into ExtensionDevToolsClientHost::MayAttachToRenderer(), which checks ExtensionCanAttachToURL() with the WebContents's SiteInstance's URL, and the SiteInstance's URL [5] is that of the Google Drive hosted app (chrome-extension://apdfllckaahabafndbhieahigkjlhalf), rather than drive.google.com. This results in ExtensionCanAttachToURL() returning false, since extensions aren't allowed to access other extensions' sites. So, now the question is, what's desirable behavior? This is certainly a behavior change (since we would previously allow this). It isn't clear to me one way or the other whether we should allow extensions to debug hosted apps. There is some precedent for making exceptions the rule of "extensions can't access other extensions" for hosted app cases, particularly with the webRequest API. dgozman@, I'm assigning this to you for now, as a devtools OWNER, in case you have thoughts. +Nasko as well, for opinions from a security perspective on whether attaching to a hosted app has any implications we need to be worried about, or if we can make an exception for the debugging API. [1] https://chrome.google.com/webstore/detail/google-drive/apdfllckaahabafndbhieahigkjlhalf [2] https://chromium.googlesource.com/chromium/src/+/8104af55cafe32fce7906a9bd337d8ac4a20e2e4/chrome/browser/extensions/api/debugger/debugger_api.cc#92 [3] https://chromium.googlesource.com/chromium/src/+/8104af55cafe32fce7906a9bd337d8ac4a20e2e4/chrome/browser/extensions/api/debugger/debugger_api.cc#423 [4] https://chromium.googlesource.com/chromium/src/+/8104af55cafe32fce7906a9bd337d8ac4a20e2e4/content/browser/devtools/render_frame_devtools_agent_host.cc#945 [5] https://chromium.googlesource.com/chromium/src/+/8104af55cafe32fce7906a9bd337d8ac4a20e2e4/chrome/browser/extensions/api/debugger/debugger_api.cc#377
,
Oct 4
alexmos@ has recently made changes to hosted apps and how they are handled/isolated, so much better at answering potential security questions about those.
,
Oct 4
I'd prefer to allow attaching to hosted apps unless we have a particular reason to restrict that. Perhaps, not calling IsRestrictedURL [1] for hosted apps would be enough? [1] https://chromium.googlesource.com/chromium/src/+/8104af55cafe32fce7906a9bd337d8ac4a20e2e4/chrome/browser/extensions/api/debugger/debugger_api.cc#102
,
Oct 4
We need to be a bit careful, because we may not want to blanket-allow hosted apps either. In particular, I think the webstore (which is installed as a hosted app) should be off-limits.
,
Oct 4
I agree with Devlin that we shouldn't allow attaching to CWS, but attaching to other hosted apps seems ok otherwise. Hosted apps shouldn't ever share a process with a regular extension (e.g., they use a different process privilege level [1]). For CWS, we already special case it in a bunch of places for stronger isolation (see uses of kWebStoreAppId in ChromeContentBrowserClientExtensionsPart, for example). I imagine we'd need a similar special case in IsRestrictedURL? [1] https://cs.chromium.org/chromium/src/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc?l=98&rcl=f892900930eeb0fe9a235330441166bbe61edc6c
,
Oct 4
> I imagine we'd need a similar special case in IsRestrictedURL? Maybe... but then we also need to introduce special logic for a "allow_on_hosted_app"-type parameter to IsRestrictedURL() (because I wouldn't want to make the change for all calls of IsRestrictedURL() without doing a thorough audit). That's doable, but a bit more invasive than I'd prefer. This would be cleaner if we just didn't pass in the hosted app's chrome-extension scheme URL to IsRestrictedURL(), which would make everything "just work" (the webstore is still restricted, and other hosted apps are allowed). We couldn't grab the last committed URL from the RFH in MayAttachToRenderer(), which would have been the natural way to do this. I don't know if there's another way to grab the non-hosted-app version of the URL (since the web extent can contain multiple hosts)... alexmos@, any idea?
,
Oct 4
alexmos@: Would SiteInstanceImpl::lock_url() be what they're looking for here? I think that's the hosted app's site without the chrome-extension part, from r587295.
,
Oct 4
Yes, SiteInstanceImpl::lock_url() might work here; it's basically the site URL without the effective URL translation that hosted apps use. It should give a chrome-extension:// URL for real extensions and the underlying web URL for hosted apps. A few questions though: - how would using that filter out the web store? Wouldn't it just show up as "https://chrome.google.com/"? - does IsRestrictedURL() care about paths at all? E.g., does it care if we pass https://hosted.app.com/path/to/app vs https://hosted.app.com/? If we care about paths, there's also SiteInstanceImpl::original_url() which preserves paths. - lock_url() would affect all effective URLs use cases, which apart from hosted apps include NTP/Instant (which gets translated to chrome-search://). Do we care about that case here? - seems the usage will be in chrome/. Charlie, are we ok exposing lock_url() from content/public?
,
Oct 5
> how would using that filter out the web store? Wouldn't it just show up as "https://chrome.google.com/"? Yes, but IsRestrictedUrl() specifically checks for the webstore URL (it calls into ExtensionsClient::IsScriptableURL(), which returns false for the webstore [1]). > does IsRestrictedURL() care about paths at all? E.g., does it care if we pass https://hosted.app.com/path/to/app vs https://hosted.app.com/? If we care about paths, there's also SiteInstanceImpl::original_url() which preserves paths. I don't believe it does. > lock_url() would affect all effective URLs use cases, which apart from hosted apps include NTP/Instant (which gets translated to chrome-search://). Do we care about that case here? Can you clarify? If the page is the NTP, is it that SiteInstance::GetSiteURL() will return a chrome-search://, and lock_url() will return https://google.com/newtab, or the other way around? Does this apply to both the local and remote NTP? I don't love the idea of extensions debugging something with potentially-higher privilege like the NTP, so it might be that we need to check both URLs :/ [1] https://chromium.googlesource.com/chromium/src/+/c5fb30be3e04239045ebe2c64dd80aa6cf046a4e/chrome/common/extensions/chrome_extensions_client.cc#181
,
Oct 5
> Can you clarify? If the page is the NTP, is it that SiteInstance::GetSiteURL() will return a chrome-search://, and lock_url() should return https://google.com/newtab, or the other way around? Does this apply to both the local and remote NTP? I don't love the idea of extensions debugging something with potentially-higher privilege like the NTP, so it might be that we need to check both URLs :/ Correct. site_url has the translated site URL that uses chrome-search://, and lock_url() will have the non-translated web URL for the NTP. (That's only for remote NTP; I think local NTP would use chrome-search://local-ntp/ for both.) We had a bug where doing something purely based on effective URLs affected not only hosted apps but also remote NTP in issue 859062 . The problem I'm realizing just now is that lock_url() is a site URL which will only have eTLD+1 rather than the full host. So I'm afraid we'll just get https://google.com/ both for NTP and for webstore. That would probably break the host-based check [1] in c#19 which needs chrome.google.com, right?
,
Oct 5
> - seems the usage will be in chrome/. Charlie, are we ok exposing lock_url() from content/public? Oh, I hadn't realized that. I'm not eager to expose that, but we can if necessary. (It's pretty complex to think about, but it's mainly due to effective URLs, which are an embedder concept anyway.) That said, you raise a good point about the eTLD+1 issue.
,
Oct 5
yeah, eTLD + 1 is an issue. If that's the case, we might not have a good alternative to plumbing through an "allow for hosted apps" bit to IsRestrictedUrl() (which will then also have to check the webstore ID)
,
Oct 5
If we did expose and use lock_url(), would it be possible to use lock_url() for most cases but also check GetSiteURL() in case it's the webstore extension ID?
,
Dec 1
I just wanted to check on the status of this issue, since I'm also waiting on a resolution on this. Is it on any roadmap? Thanks!
,
Dec 3
It looks like discussion comments #15-#23 didn't lead to a solution just yet. Let me put this back to Devlin. I can help once we have a reasonable solution. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by phanindra.mandapaka@chromium.org
, Sep 19