Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
256783006:
ChromeVox enabled for every WebDialog created by call to ShowWebDialog. (Closed)
|
Created:
6 years, 8 months ago by dzhioev (left Google) Modified:
6 years, 7 months ago CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionChromeVox enabled for every WebDialog created by call to ShowWebDialog.
While I was investigating http://crbug.com/355650 I noticed that
ChromeVox doesn't work not only in cloud print dialog but in every dialog that
was created by ShowWebDialog. This problem can be fixed by creating extension
web contents observer for contents of dialog.
BUG=355650
TEST=manually
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272333
Patch Set 1 #
Total comments: 3
Messages
Total messages: 10 (0 generated)
Hi Mike, please review.
https://codereview.chromium.org/256783006/diff/1/chrome/browser/ui/views/chro... File chrome/browser/ui/views/chrome_web_dialog_view.cc (right): https://codereview.chromium.org/256783006/diff/1/chrome/browser/ui/views/chro... chrome/browser/ui/views/chrome_web_dialog_view.cc:27: // Observer is needed for ChromeVox extension to send messages between content Are there security concerns with enabling extensions for our WebUI dialogs? Could extensions modify dialog content that's presented as trusted Chrome UI? I'm sure we have a solution for our chrome:// scheme WebUI pages, but I'm not sure the same solution is in place for web dialog content. How does ChromeVox fair on chrome://settings? You should probably ping the security team, CC some webui/extensions/content owners, and get c/b/ui/views owners review from ben or sky.
https://codereview.chromium.org/256783006/diff/1/chrome/browser/ui/views/chro... File chrome/browser/ui/views/chrome_web_dialog_view.cc (right): https://codereview.chromium.org/256783006/diff/1/chrome/browser/ui/views/chro... chrome/browser/ui/views/chrome_web_dialog_view.cc:27: // Observer is needed for ChromeVox extension to send messages between content On 2014/04/28 18:32:45, msw wrote: > Are there security concerns with enabling extensions for our WebUI dialogs? > Could extensions modify dialog content that's presented as trusted Chrome UI? > I'm sure we have a solution for our chrome:// scheme WebUI pages, but I'm not > sure the same solution is in place for web dialog content. How does ChromeVox > fair on chrome://settings? > > You should probably ping the security team, CC some webui/extensions/content > owners, and get c/b/ui/views owners review from ben or sky. Creating ChromeExtensionWebContentsObserver makes possible communication between injected code and background page. But it doesn't make possible for any extension to inject their code in the web contents. ChromeVox is an exception - it can inject its script into any web contents including "chrome://*" pages. (See here https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... ). But if ChromeExtensionWebContentsObserver was not created for that contents, injected code can't communicate with background script and spoken feedback doesn't work. Adding dmazzoni@ to confirm my words.
On 2014/04/28 19:15:53, dzhioev wrote: > https://codereview.chromium.org/256783006/diff/1/chrome/browser/ui/views/chro... > File chrome/browser/ui/views/chrome_web_dialog_view.cc (right): > > https://codereview.chromium.org/256783006/diff/1/chrome/browser/ui/views/chro... > chrome/browser/ui/views/chrome_web_dialog_view.cc:27: // Observer is needed for > ChromeVox extension to send messages between content > On 2014/04/28 18:32:45, msw wrote: > > Are there security concerns with enabling extensions for our WebUI dialogs? > > Could extensions modify dialog content that's presented as trusted Chrome UI? > > I'm sure we have a solution for our chrome:// scheme WebUI pages, but I'm not > > sure the same solution is in place for web dialog content. How does ChromeVox > > fair on chrome://settings? > > > > You should probably ping the security team, CC some webui/extensions/content > > owners, and get c/b/ui/views owners review from ben or sky. > Creating ChromeExtensionWebContentsObserver makes possible communication between > injected code > and background page. But it doesn't make possible for any extension to inject > their code in the web > contents. > > ChromeVox is an exception - it can inject its script into any web contents > including "chrome://*" pages. > (See here > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... > ). > But if ChromeExtensionWebContentsObserver was not created for that contents, > injected code can't communicate with background script > and spoken feedback doesn't work. > > Adding dmazzoni@ to confirm my words. Dominic, your confirmation is still needed.
On 2014/05/12 12:14:25, dzhioev wrote: > On 2014/04/28 19:15:53, dzhioev wrote: > > > https://codereview.chromium.org/256783006/diff/1/chrome/browser/ui/views/chro... > > File chrome/browser/ui/views/chrome_web_dialog_view.cc (right): > > > > > https://codereview.chromium.org/256783006/diff/1/chrome/browser/ui/views/chro... > > chrome/browser/ui/views/chrome_web_dialog_view.cc:27: // Observer is needed > for > > ChromeVox extension to send messages between content > > On 2014/04/28 18:32:45, msw wrote: > > > Are there security concerns with enabling extensions for our WebUI dialogs? > > > Could extensions modify dialog content that's presented as trusted Chrome > UI? > > > I'm sure we have a solution for our chrome:// scheme WebUI pages, but I'm > not > > > sure the same solution is in place for web dialog content. How does > ChromeVox > > > fair on chrome://settings? > > > > > > You should probably ping the security team, CC some webui/extensions/content > > > owners, and get c/b/ui/views owners review from ben or sky. > > Creating ChromeExtensionWebContentsObserver makes possible communication > between > > injected code > > and background page. But it doesn't make possible for any extension to inject > > their code in the web > > contents. > > > > ChromeVox is an exception - it can inject its script into any web contents > > including "chrome://*" pages. > > (See here > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... > > ). > > But if ChromeExtensionWebContentsObserver was not created for that contents, > > injected code can't communicate with background script > > and spoken feedback doesn't work. > > > > Adding dmazzoni@ to confirm my words. > > Dominic, your confirmation is still needed. Dominic, ping.
lgtm https://codereview.chromium.org/256783006/diff/1/chrome/browser/ui/views/chro... File chrome/browser/ui/views/chrome_web_dialog_view.cc (right): https://codereview.chromium.org/256783006/diff/1/chrome/browser/ui/views/chro... chrome/browser/ui/views/chrome_web_dialog_view.cc:27: // Observer is needed for ChromeVox extension to send messages between content On 2014/04/28 19:15:53, dzhioev wrote: > On 2014/04/28 18:32:45, msw wrote: > > Are there security concerns with enabling extensions for our WebUI dialogs? > > Could extensions modify dialog content that's presented as trusted Chrome UI? > > I'm sure we have a solution for our chrome:// scheme WebUI pages, but I'm not > > sure the same solution is in place for web dialog content. How does ChromeVox > > fair on chrome://settings? > > > > You should probably ping the security team, CC some webui/extensions/content > > owners, and get c/b/ui/views owners review from ben or sky. > Creating ChromeExtensionWebContentsObserver makes possible communication between > injected code > and background page. But it doesn't make possible for any extension to inject > their code in the web > contents. > > ChromeVox is an exception - it can inject its script into any web contents > including "chrome://*" pages. > (See here > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... > ). > But if ChromeExtensionWebContentsObserver was not created for that contents, > injected code can't communicate with background script > and spoken feedback doesn't work. > > Adding dmazzoni@ to confirm my words. Thanks for the ping. Confirmed - just adding a ChromeExtensionWebContentsObserver doesn't make it any easier for a random third-party extension to have access to this page, all it would do is make it easier for them to send a message to their background page if they did manage to inject their extension. Also, if it helps, we have a plan underway to remove the need for injecting the ChromeVox content script at all, but that's a couple of quarters away before we could remove this code.
cool, lgtm too then
The CQ bit was checked by dzhioev@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dzhioev@chromium.org/256783006/1
Message was sent while issue was closed.
Change committed as 272333 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
