Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1227793006:
Navigations are renderer-initiated if no WebUI bindings present. (Closed)
|
Created:
5 years, 5 months ago by wjmaclean Modified:
5 years, 5 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, creis+watch_chromium.org, darin-cc_chromium.org, kmadhusu Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNavigations are renderer-initiated if no WebUI bindings present.
This CL is designed to allow extensions to have their
navigations to be considered "renderer-initiated". It also
allows extensions to have referrers, and removes some
apparently dead code.
BUG=488053
Patch Set 1 #Patch Set 2 : Apply WebUI bindings criterion to entire block. #
Total comments: 8
Depends on Patchset: Messages
Total messages: 22 (7 generated)
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1227793006/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1227793006/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
wjmaclean@chromium.org changed reviewers: + creis@chromium.org
creis@ - Since there are no branch points imminent, shall we try to land this, and watch the canary builds to see if anything unexpected happens? We can always revert if need be ...
creis@chromium.org changed reviewers: + kalman@chromium.org, kmadhusu@chromium.org
[+kmadhusu,kalman] It looks like this change might not be safe after all. I'm including people who know more about the NTP and extensions to verify. The background is that we want link clicks in extensions to have is_renderer_initiated set to true, but this code is overriding it. That seems like it's probably wrong and only necessary for Web UI pages with bindings, and we weren't sure whether the other code nearby (transition type and referrer) was wrong as well. https://codereview.chromium.org/1227793006/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (left): https://codereview.chromium.org/1227793006/diff/20001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:567: // link clicks (e.g., so the new tab page can specify AUTO_BOOKMARK for Looking deeper, this change doesn't look safe. NewTabUI still calls SetLinkTransitionType with AUTO_BOOKMARK, so the NTP may still rely on this. @kmadhusu: Can you confirm? https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... I also see it used by the bookmark manager extension in ExtensionWebUI (which doesn't have bindings): https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... https://codereview.chromium.org/1227793006/diff/20001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:580: params.referrer = Referrer(); @kalman: Should extension processes be sending a null referrer (as they do today thanks to this code), or a real referrer? https://codereview.chromium.org/1227793006/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1227793006/diff/20001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:565: // Note: extensions don't have bindings, so will not be affected by the Extensions aren't a concept in content/, so I don't think we need this comment. https://codereview.chromium.org/1227793006/diff/20001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:570: // Note that we hide the referrer for Web UI pages. We don't really nit: for Web UI pages with bindings. https://codereview.chromium.org/1227793006/diff/20001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:576: // Navigations in Web UI pages (with WebUI bindings) count as nit: Web UI
I'd rather tackle this problem from why extensions need to be treated as webui, rather than whatever this change does (seems very low level and I don't understand it). https://codereview.chromium.org/1227793006/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (left): https://codereview.chromium.org/1227793006/diff/20001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:580: params.referrer = Referrer(); On 2015/07/15 20:38:00, Charlie Reis wrote: > @kalman: Should extension processes be sending a null referrer (as they do today > thanks to this code), or a real referrer? We should maintain the old, existing behavior.
On 2015/07/15 20:42:03, kalman wrote: > I'd rather tackle this problem from why extensions need to be treated as webui, > rather than whatever this change does (seems very low level and I don't > understand it). > > https://codereview.chromium.org/1227793006/diff/20001/content/browser/frame_h... > File content/browser/frame_host/navigator_impl.cc (left): > > https://codereview.chromium.org/1227793006/diff/20001/content/browser/frame_h... > content/browser/frame_host/navigator_impl.cc:580: params.referrer = Referrer(); > On 2015/07/15 20:38:00, Charlie Reis wrote: > > @kalman: Should extension processes be sending a null referrer (as they do > today > > thanks to this code), or a real referrer? > > We should maintain the old, existing behavior. kalman@ - is there any reason why extension navigations cannot be considered renderer_initiated? The original patch on this issue only changed that one aspect.
> > kalman@ - is there any reason why extension navigations cannot be considered > renderer_initiated? The original patch on this issue only changed that one > aspect. Only that I don't know what renderer initiated means, and what it changes.
On 2015/07/15 21:00:15, kalman wrote:
> >
> > kalman@ - is there any reason why extension navigations cannot be considered
> > renderer_initiated? The original patch on this issue only changed that one
> > aspect.
>
> Only that I don't know what renderer initiated means, and what it changes.
Here's what I know about is_renderer_initiated:
- it generally defaults to true when nav/load parameters are constructed
- the single *biggest* direct effects I can find are:
- is it safe to allow the navigation to use POST (only allowed when
!is_renderer_initiated)
- should we show a pending entry in history/GetVisibleEntry()
- can it be used in pre-renderered search (I don't know what this entails)
- the single biggest *indirect* effect I found
- an AppWindow will kill in-tab navigations of a WebView it contains
(presumably it thinks it is protecting
itself from being navigated away from, though in the case of a guest
navigation this seems wrong)
My $0.02CDN: The biggest risk here is accidentally disabling something that (1)
uses WebUI and (2) wants to use POST. I would *hope* that such a case would be
caught by testing, but if it isn't, we should find out. Also, since using POST
is potentially a security issue, perhaps it would be best if it has its own flag
in the nav parameters? Something like can_use_post, which defaults to 'false'
and must be explicitly set? (That's another CL however.)
Below I've included a list of all the non-trivial references (including
initializations) I could find. I did this using "git gs" to avoid getting caught
by code-search being preferential to Unix builds (as always, code-search links
are a moving target, so get'em while they're fresh!):
**** Declarations
is_renderer_initiated
- part of OpenUrlParams
OpenUrlParams.user_gesture seems to always be initialized to the opposite of
is_renderer_initiated
- a parameter with the same name exists in chrome::NavigateParams,
NavigateParams::NavigateParams
**** Places Where Set
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
***
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
- navigate on thumbnail click (in new tab page? or bookmark manager? both?)
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se...
- service worker OpenWindowOnUI()
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...
- WebContentsImpl::CreateNewWindow()
https://code.google.com/p/chromium/codesearch#chromium/src/ios/web/navigation...
- iOS: one of the few places where 'false' is the default initialization
https://code.google.com/p/chromium/codesearch#chromium/src/ios/web/navigation...
- ditto above
**** Places Referenced (non-test only)
** Disclaimer - I can't guarantee that all the places listed below are
"reachable" from Line 595 in NavigatorImpl
** Note - I haven't included references below that are just
initialization/copying to another parameter with the same name in a different
structure
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
- sets up entry, redirect chain; start of main-frame navigation
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
- causes WebViewGuest to pass off navigation to a WebContentsDelegate
(in this case AppWindow; this drops all same-tab navigations)
- this is curious since PDF viewer isn't an app (though it's being viewed within
one)
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
- part of determining if its safe to show a panding navigation in the history?
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
- similar to above
-
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
- pre-renderered pages vs. renderer-initiated search requests
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
- only allow browser-initiated navigations to use POST
** This would seem to be an argument *against* letting extensions navigate as
browser_initiated, but I could be wrong (perhaps we trust extensions more?)
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/and...
- ditto above
https://code.google.com/p/chromium/codesearch#chromium/src/components/web_con...
- ditto above
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dow...
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dow...
- downloading "group" / "item" notifications
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
- in CloneAndReplace() - interesting comment ("reset for commit")
- seems to refer to
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
https://code.google.com/p/chromium/codesearch#chromium/src/ios/web/navigation...
- GetVisibleEntry()
A follow-on question to the is_renderer_initiated discussion above: "What are all the ways an extension can initiate a navigation?" - via an embedded link? (presumably this would be renderer-initiated?) - via javascript? (less clear if this is renderer-initiated, though I would tend to think 'yes') - other methods?
> is it safe to allow the navigation to use POST I don't see why not, or at least, it doesn't make any difference currently. However, this is an interesting question when considering that we're going to have service workers for extensions. I still don't see why not. > should we show a pending entry in history/GetVisibleEntry() What does this mean? That navigating to a chrome-extension://... page can then go back to the previous content (maybe web)? Or navigating away from chrome-extension:// can then go back to that chrome-extension:// page? This doesn't seem like a problem to me from a clickjacking/spoofing/whatever perspective - so long as we don't set a referrer - but there may be a site isolation issue. We may as well also be paranoid. > can it be used in pre-renderered search (I don't know what this entails) I don't see why not, though it's a bit weird for serving local content. > an AppWindow will kill in-tab navigations of a WebView it contains > (presumably it thinks it is protecting > itself from being navigated away from, though in the case of a guest > navigation this seems wrong) I agree this sounds wrong.
On 2015/07/16 17:26:06, kalman wrote: > > > an AppWindow will kill in-tab navigations of a WebView it contains > > (presumably it thinks it is protecting > > itself from being navigated away from, though in the case of a guest > > navigation this seems wrong) > > I agree this sounds wrong. It seems like this may be the safest point in the code to deal with this particular bug. So far there's no obvious way to demarcate extensions navigations from generic !is_renderer_initiated ones. This might be possible with a more expressive set of params, but it may be the extra overhead isn't worth it.
Message was sent while issue was closed.
kmadhusu@chromium.org changed reviewers: + fserb@chromium.org, mathp@chromium.org
Message was sent while issue was closed.
creis@: Adding current owners of NTP to the reviewers list to address your comment. Thanks. https://codereview.chromium.org/1227793006/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (left): https://codereview.chromium.org/1227793006/diff/20001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:567: // link clicks (e.g., so the new tab page can specify AUTO_BOOKMARK for On 2015/07/15 20:38:00, Charlie Reis wrote: > Looking deeper, this change doesn't look safe. > > NewTabUI still calls SetLinkTransitionType with AUTO_BOOKMARK, so the NTP may > still rely on this. @kmadhusu: Can you confirm? > mathp@/fserb@: Please address this comment. > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > I also see it used by the bookmark manager extension in ExtensionWebUI (which > doesn't have bindings): > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
Message was sent while issue was closed.
https://codereview.chromium.org/1227793006/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (left): https://codereview.chromium.org/1227793006/diff/20001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:567: // link clicks (e.g., so the new tab page can specify AUTO_BOOKMARK for On 2015/07/17 04:06:47, kmadhusu wrote: > On 2015/07/15 20:38:00, Charlie Reis wrote: > > Looking deeper, this change doesn't look safe. > > > > NewTabUI still calls SetLinkTransitionType with AUTO_BOOKMARK, so the NTP may > > still rely on this. @kmadhusu: Can you confirm? > > > > mathp@/fserb@: Please address this comment. > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > I also see it used by the bookmark manager extension in ExtensionWebUI (which > > doesn't have bindings): > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > Yep. We still rely on this when we analyse history. This should not be removed. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
