Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
794803002:
Refactor: cleanup declarative_api.cc. (Closed)
DescriptionRefactor: cleanup declarative_api.cc and get rid of
"webViewIntanal" prefix when passing declarative webRequest
Event from <webview>.
When <webview> call an API, the event
that passes to the browser will keep its original name.
For example, "declarativeWebRequest.onMessage".
With this patch, we don't need to add the "webViewIntanal"
prefix when creating an event, and remove the prefix when
browser receive it.
However, this would cause a problem: you cannot tell
whether this event comes from <webview> or regular tabs
simply from its name. So the event will be treated as a
regular API event rather than a <webview> event, and fails
when doing feature permission checks if <webview> calls
declarativeWebRequest API from a chrome app.
In this CL, web_view_instance_id is passed along during
event registry to bypass the feature permission checks.
BUG=437566
Committed: https://crrev.com/b684c7ada5434b1954cced5e13a9d91ba88de6cd
Cr-Commit-Position: refs/heads/master@{#309017}
Patch Set 1 #Patch Set 2 : Pass webViewInstanceId when attachEvent (in event.js) to avoid permission check. #
Total comments: 4
Patch Set 3 : nits #Patch Set 4 : #
Total comments: 10
Patch Set 5 : nits. #Patch Set 6 : Clean up. #
Total comments: 12
Patch Set 7 : Update based on kalman@'s comments. #Patch Set 8 : #
Total comments: 2
Patch Set 9 : Add "webViewInternal" back to event name. #
Total comments: 6
Patch Set 10 : kalman's comments #
Messages
Total messages: 40 (15 generated)
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
hanxi@chromium.org changed reviewers: + fsamuel@chromium.org
lgtm + nits. Please add kalman@ to this CL to get his thoughts. https://codereview.chromium.org/794803002/diff/60001/extensions/renderer/even... File extensions/renderer/event_bindings.cc (right): https://codereview.chromium.org/794803002/diff/60001/extensions/renderer/even... extensions/renderer/event_bindings.cc:173: int web_view_instance_id = args[1]->Int32Value(); Let's just call this the guest_view_instance_id https://codereview.chromium.org/794803002/diff/60001/extensions/renderer/even... extensions/renderer/event_bindings.cc:175: if (web_view_instance_id == 0 && I think !web_view_instance_id is the preferred style.
hanxi@chromium.org changed reviewers: + kalman@chromium.org
kalman@chromium.org: Please review all changes, thanks! https://codereview.chromium.org/794803002/diff/60001/extensions/renderer/even... File extensions/renderer/event_bindings.cc (right): https://codereview.chromium.org/794803002/diff/60001/extensions/renderer/even... extensions/renderer/event_bindings.cc:173: int web_view_instance_id = args[1]->Int32Value(); On 2014/12/11 15:39:09, Fady Samuel wrote: > Let's just call this the guest_view_instance_id Done. https://codereview.chromium.org/794803002/diff/60001/extensions/renderer/even... extensions/renderer/event_bindings.cc:175: if (web_view_instance_id == 0 && On 2014/12/11 15:39:09, Fady Samuel wrote: > I think !web_view_instance_id is the preferred style. Done.
Patchset #4 (id:100001) has been deleted
https://codereview.chromium.org/794803002/diff/120001/extensions/browser/api/... File extensions/browser/api/declarative/declarative_api.cc (right): https://codereview.chromium.org/794803002/diff/120001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:104: int web_view_instance_id = 0; I think this is a good change. An even better change would be to hook this into the ExtensionFunction mechanism - add a method to ExtensionFunction as to whether the request comes from webview (you could expose this via some kind of webview_instance_id method), rather than tacking it onto event registration. You should be able to do this in a mechanism similar to this: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser... https://codereview.chromium.org/794803002/diff/120001/extensions/renderer/eve... File extensions/renderer/event_bindings.cc (right): https://codereview.chromium.org/794803002/diff/120001/extensions/renderer/eve... extensions/renderer/event_bindings.cc:175: if (!guest_view_instance_id && If it's an int, prefer to use guest_view_instance_id == 0 rather than a boolean expression. https://codereview.chromium.org/794803002/diff/120001/extensions/renderer/eve... extensions/renderer/event_bindings.cc:176: !dispatcher_->CheckContextAccessToExtensionAPI(event_name, context())) It's nice to surround this in {} when the condition is complex (i.e. multi-line). https://codereview.chromium.org/794803002/diff/120001/extensions/renderer/eve... extensions/renderer/event_bindings.cc:178: All the above said, it's pretty weird to see security checks being skipped here just because it's from a webview. Could you modify the event functions to be callable from web contexts, then you don't need this change?
https://codereview.chromium.org/794803002/diff/120001/extensions/browser/api/... File extensions/browser/api/declarative/declarative_api.cc (right): https://codereview.chromium.org/794803002/diff/120001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:104: int web_view_instance_id = 0; On 2014/12/11 22:10:48, kalman wrote: > I think this is a good change. > > An even better change would be to hook this into the ExtensionFunction mechanism > - add a method to ExtensionFunction as to whether the request comes from webview > (you could expose this via some kind of webview_instance_id method), rather than > tacking it onto event registration. > > You should be able to do this in a mechanism similar to this: > > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser... This is a good suggestion, but I found when this function is called, the guest isn't attached to its WebContents yet. It ends up with instance_id = 0. https://codereview.chromium.org/794803002/diff/120001/extensions/renderer/eve... File extensions/renderer/event_bindings.cc (right): https://codereview.chromium.org/794803002/diff/120001/extensions/renderer/eve... extensions/renderer/event_bindings.cc:175: if (!guest_view_instance_id && On 2014/12/11 22:10:48, kalman wrote: > If it's an int, prefer to use guest_view_instance_id == 0 rather than a boolean > expression. Done. https://codereview.chromium.org/794803002/diff/120001/extensions/renderer/eve... extensions/renderer/event_bindings.cc:176: !dispatcher_->CheckContextAccessToExtensionAPI(event_name, context())) On 2014/12/11 22:10:48, kalman wrote: > It's nice to surround this in {} when the condition is complex (i.e. > multi-line). Done. https://codereview.chromium.org/794803002/diff/120001/extensions/renderer/eve... extensions/renderer/event_bindings.cc:178: On 2014/12/11 22:10:48, kalman wrote: > All the above said, it's pretty weird to see security checks being skipped here > just because it's from a webview. Could you modify the event functions to be > callable from web contexts, then you don't need this change? As discussed offline, the problem here is: if passing in the event with its original name like "declarativeWebRequest.onMessage", this event will be treated as a regular API event, go through the permission check, and won't pass the feature permission check here. So we do need to give <webview> event some power to skip the check.
Again, I had an old comment sitting around, so sending that out. The more important part: try patching in https://codereview.chromium.org/802653003 and see if this still works. https://codereview.chromium.org/794803002/diff/120001/extensions/browser/api/... File extensions/browser/api/declarative/declarative_api.cc (right): https://codereview.chromium.org/794803002/diff/120001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:111: Feature::Availability availability = Can you change the rest of this to be "return ExtensionFunction::HasPermission()"
On 2014/12/12 22:28:30, hanxi wrote: > https://codereview.chromium.org/794803002/diff/120001/extensions/browser/api/... > File extensions/browser/api/declarative/declarative_api.cc (right): > > https://codereview.chromium.org/794803002/diff/120001/extensions/browser/api/... > extensions/browser/api/declarative/declarative_api.cc:104: int > web_view_instance_id = 0; > On 2014/12/11 22:10:48, kalman wrote: > > I think this is a good change. > > > > An even better change would be to hook this into the ExtensionFunction > mechanism > > - add a method to ExtensionFunction as to whether the request comes from > webview > > (you could expose this via some kind of webview_instance_id method), rather > than > > tacking it onto event registration. > > > > You should be able to do this in a mechanism similar to this: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser... > > > This is a good suggestion, but I found when this function is called, the guest > isn't attached to its WebContents yet. It ends up with instance_id = 0. > > https://codereview.chromium.org/794803002/diff/120001/extensions/renderer/eve... > File extensions/renderer/event_bindings.cc (right): > > https://codereview.chromium.org/794803002/diff/120001/extensions/renderer/eve... > extensions/renderer/event_bindings.cc:175: if (!guest_view_instance_id && > On 2014/12/11 22:10:48, kalman wrote: > > If it's an int, prefer to use guest_view_instance_id == 0 rather than a > boolean > > expression. > > Done. > > https://codereview.chromium.org/794803002/diff/120001/extensions/renderer/eve... > extensions/renderer/event_bindings.cc:176: > !dispatcher_->CheckContextAccessToExtensionAPI(event_name, context())) > On 2014/12/11 22:10:48, kalman wrote: > > It's nice to surround this in {} when the condition is complex (i.e. > > multi-line). > > Done. > > https://codereview.chromium.org/794803002/diff/120001/extensions/renderer/eve... > extensions/renderer/event_bindings.cc:178: > On 2014/12/11 22:10:48, kalman wrote: > > All the above said, it's pretty weird to see security checks being skipped > here > > just because it's from a webview. Could you modify the event functions to be > > callable from web contexts, then you don't need this change? > > As discussed offline, the problem here is: if passing in the event with its > original name like "declarativeWebRequest.onMessage", this event will be treated > as a regular API event, go through the permission check, and won't pass the > feature permission check here. So we do need to give <webview> event some power > to skip the check. I think we'll need to give the Feature system some knowledge of webview. I don't know what that would involve, though. First thing that comes to mind would be to add another context type WEBVIEW, which would work ok, though I'm a little worried that it'll be treated differently to WEB_PAGE, when it shouldn't really. Remind me again why web pages can call the declarativeWebRequest API?
kalman@: I applied your patch (https://codereview.chromium.org/802653003) and this still works. The web pages (html files) create a <webview> and the <webview> calls the declarativeWebRequest API (https://github.com/GoogleChrome/chrome-app-samples/tree/master/samples/webvie...). A host permission like "declarativeWebRequest", "*://*/*" is needed in manifest.json file. Let feature system know webview is a good idea, but it definitely require significant work and refactor. Can we postpone it to another CL and talk about when you come back from vacation? https://codereview.chromium.org/794803002/diff/120001/extensions/browser/api/... File extensions/browser/api/declarative/declarative_api.cc (right): https://codereview.chromium.org/794803002/diff/120001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:111: Feature::Availability availability = On 2014/12/15 04:24:30, kalman wrote: > Can you change the rest of this to be "return > ExtensionFunction::HasPermission()" Done.
hanxi@chromium.org changed reviewers: + rockot@chromium.org
Hi Ken: kalman is on vacation, could you please help me to take a look at this CL? Thanks!
lgtm
The CQ bit was checked by hanxi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/794803002/160001
I'm not on vacation, please wait On Dec 16, 2014 6:04 AM, <commit-bot@chromium.org> wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/794803002/160001 > > > https://codereview.chromium.org/794803002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by hanxi@chromium.org
On 2014/12/15 17:17:57, hanxi wrote: > kalman@: I applied your patch (https://codereview.chromium.org/802653003) and > this still works. > > The web pages (html files) create a <webview> and the <webview> calls the > declarativeWebRequest API > (https://github.com/GoogleChrome/chrome-app-samples/tree/master/samples/webvie...). > A host permission like "declarativeWebRequest", > "*://*/*" is needed in manifest.json file. *Within* a webview? Does that mean that arbitrary web content can call the declarativeWebRequest API? As in, code that is hosted on the web?
https://codereview.chromium.org/794803002/diff/160001/extensions/browser/api/... File extensions/browser/api/declarative/declarative_api.cc (right): https://codereview.chromium.org/794803002/diff/160001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:97: source_context_type_ = Feature::BLESSED_EXTENSION_CONTEXT; This looks very wrong, it's supposed to be manually determined by calling code. This could really screw up permissions checking. What is going wrong there? https://codereview.chromium.org/794803002/diff/160001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:104: EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &event_name)); Pulling out |event_name| is no longer necessary. https://codereview.chromium.org/794803002/diff/160001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:108: if (web_view_instance_id != 0 && Could you explain (in a comment) why you need to do this? https://codereview.chromium.org/794803002/diff/160001/extensions/renderer/eve... File extensions/renderer/event_bindings.cc (right): https://codereview.chromium.org/794803002/diff/160001/extensions/renderer/eve... extensions/renderer/event_bindings.cc:175: if (guest_view_instance_id == 0 && Explain this in a comment. https://codereview.chromium.org/794803002/diff/160001/extensions/renderer/res... File extensions/renderer/resources/event.js (right): https://codereview.chromium.org/794803002/diff/160001/extensions/renderer/res... extensions/renderer/resources/event.js:76: eventNatives.AttachEvent( Braces around multi-line function bodies.... especially in JS. https://codereview.chromium.org/794803002/diff/160001/extensions/renderer/res... File extensions/renderer/resources/guest_view/web_view_events.js (right): https://codereview.chromium.org/794803002/diff/160001/extensions/renderer/res... extensions/renderer/resources/guest_view/web_view_events.js:230: DeclarativeWebRequestEvent : EventBindings.Event; The only thing this function (createDeclarativeWebRequestEvent) appears to do, over just creating Events like normal, is this check here. Do you still need it?
Patchset #8 (id:200001) has been deleted
Patchset #8 (id:220001) has been deleted
Patchset #8 (id:240001) has been deleted
Patchset #8 (id:260001) has been deleted
Hi Ben: If the network request can match all the criteria (e.g., url pattern) and <webview> navigates to the allowed URLs (defined in manifest.json), the web content within the <webview> can call the declarative WebRequest API. https://codereview.chromium.org/794803002/diff/160001/extensions/browser/api/... File extensions/browser/api/declarative/declarative_api.cc (right): https://codereview.chromium.org/794803002/diff/160001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:97: source_context_type_ = Feature::BLESSED_EXTENSION_CONTEXT; On 2014/12/15 23:49:57, kalman wrote: > This looks very wrong, it's supposed to be manually determined by calling code. > This could really screw up permissions checking. What is going wrong there? Removed. I realized it wasn't the right thing to do even if I want to use Extension::HasPermission() in line 113. The implementation of Extension::HasPermission() passes source_context_type_ to call IsAvailable(...), but the block of code that I removed in RulesFunction::HasPermission() passes Feature::BLESSED_EXTENSION_CONTEXT. Now I moved this block of code back. It is good to reuse code, but I don't think it is necessary to introduce a new virtual function "Extension::HasPermission(Feature::Context)" just for this one time usage. https://codereview.chromium.org/794803002/diff/160001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:104: EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &event_name)); On 2014/12/15 23:49:57, kalman wrote: > Pulling out |event_name| is no longer necessary. Good catch. Since I moved the block of code back, the |event_name| will be needed again. https://codereview.chromium.org/794803002/diff/160001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:108: if (web_view_instance_id != 0 && On 2014/12/15 23:49:57, kalman wrote: > Could you explain (in a comment) why you need to do this? Added. https://codereview.chromium.org/794803002/diff/160001/extensions/renderer/eve... File extensions/renderer/event_bindings.cc (right): https://codereview.chromium.org/794803002/diff/160001/extensions/renderer/eve... extensions/renderer/event_bindings.cc:175: if (guest_view_instance_id == 0 && On 2014/12/15 23:49:57, kalman wrote: > Explain this in a comment. Done. https://codereview.chromium.org/794803002/diff/160001/extensions/renderer/res... File extensions/renderer/resources/event.js (right): https://codereview.chromium.org/794803002/diff/160001/extensions/renderer/res... extensions/renderer/resources/event.js:76: eventNatives.AttachEvent( On 2014/12/15 23:49:57, kalman wrote: > Braces around multi-line function bodies.... especially in JS. Great catch, thanks! I really think the style "don't add {} for a single line" is dangerous, I do prefer adding {} for all the cases using "if". https://codereview.chromium.org/794803002/diff/160001/extensions/renderer/res... File extensions/renderer/resources/guest_view/web_view_events.js (right): https://codereview.chromium.org/794803002/diff/160001/extensions/renderer/res... extensions/renderer/resources/guest_view/web_view_events.js:230: DeclarativeWebRequestEvent : EventBindings.Event; On 2014/12/15 23:49:57, kalman wrote: > The only thing this function (createDeclarativeWebRequestEvent) appears to do, > over just creating Events like normal, is this check here. Do you still need it? Hmmm, I think yes. The "onMessage" event is handled differently with other event (i.e., "onRequest"). It needs to add a Listener to receive message from the browser. I suggest to leave it as it is now.
https://codereview.chromium.org/794803002/diff/280001/extensions/browser/api/... File extensions/browser/api/declarative/declarative_api.cc (right): https://codereview.chromium.org/794803002/diff/280001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:117: Feature::BLESSED_EXTENSION_CONTEXT, But why does this only check blessed contexts?
On 2014/12/16 19:05:27, hanxi wrote: > Hi Ben: > If the network request can match all the criteria (e.g., url pattern) and > <webview> navigates to the allowed URLs (defined in manifest.json), the web > content within the <webview> can call the declarative WebRequest API. I'm wondering how this is enforced. declarativeWebRequest is one of the most powerful APIs we have, and this seems (?) to open it up to web pages in some cases - such that the browser can no longer do a full permission check. I.e. if a web page convinces the renderer to pass through a non-zero instance ID, then it can potentially call declarativeWebRequest. I'm having trouble interpreting the sample you linked to, but it seems like the filters are set up within the webview host? So it's actually the host that is calling these APIs? Am I misreading?
On 2014/12/17 04:13:29, kalman wrote: > On 2014/12/16 19:05:27, hanxi wrote: > > Hi Ben: > > If the network request can match all the criteria (e.g., url pattern) and > > <webview> navigates to the allowed URLs (defined in manifest.json), the web > > content within the <webview> can call the declarative WebRequest API. > > I'm wondering how this is enforced. declarativeWebRequest is one of the most > powerful APIs we have, and this seems (?) to open it up to web pages in some > cases - such that the browser can no longer do a full permission check. I.e. if > a web page convinces the renderer to pass through a non-zero instance ID, then > it can potentially call declarativeWebRequest. I think your concern is right. > I'm having trouble interpreting the sample you linked to, but it seems like the > filters are set up within the webview host? So it's actually the host that is > calling these APIs? Am I misreading? We can interpret like the host calls the APIs, but the actions is eventually applied on the WebContents of the <webview>. So it cannot do anything dangerous, right?
https://codereview.chromium.org/794803002/diff/280001/extensions/browser/api/... File extensions/browser/api/declarative/declarative_api.cc (right): https://codereview.chromium.org/794803002/diff/280001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:117: Feature::BLESSED_EXTENSION_CONTEXT, On 2014/12/17 04:06:42, kalman wrote: > But why does this only check blessed contexts? Hmmm, it is legacy code and it was introduced like this at the very beginning.
Please take a look this simple solution first.Thanks!
lgtm, let's get this committed. with comments. https://codereview.chromium.org/794803002/diff/300001/extensions/browser/api/... File extensions/browser/api/declarative/declarative_api.cc (right): https://codereview.chromium.org/794803002/diff/300001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:110: // permission, this function will return true to allow the permission request. A better comment would be: // <webview> embedders use the declarativeWebRequest API via <webview>.onRequest. https://codereview.chromium.org/794803002/diff/300001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:115: Feature::Availability availability = was there anything stopping us from changing this to ExtensionFunction::HasPermission()? (if it works now, you might as well change it. if not, let's do it in a follow up. I don't trust this code) https://codereview.chromium.org/794803002/diff/300001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:136: event_name = event_name.substr(event_name.find(kDeclarativeEventPrefix)); explain this? you should also EXTENSION_FUNCTION_VALIDATE that kDeclarativeEventPrefix is actually found.
Patchset #10 (id:320001) has been deleted
Patchset #10 (id:320001) has been deleted
https://codereview.chromium.org/794803002/diff/300001/extensions/browser/api/... File extensions/browser/api/declarative/declarative_api.cc (right): https://codereview.chromium.org/794803002/diff/300001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:110: // permission, this function will return true to allow the permission request. On 2014/12/18 06:24:27, kalman wrote: > A better comment would be: > > // <webview> embedders use the declarativeWebRequest API via > <webview>.onRequest. Done. https://codereview.chromium.org/794803002/diff/300001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:115: Feature::Availability availability = On 2014/12/18 06:24:27, kalman wrote: > was there anything stopping us from changing this to > ExtensionFunction::HasPermission()? > > (if it works now, you might as well change it. if not, let's do it in a follow > up. I don't trust this code) It seems everything is good, updated now:) https://codereview.chromium.org/794803002/diff/300001/extensions/browser/api/... extensions/browser/api/declarative/declarative_api.cc:136: event_name = event_name.substr(event_name.find(kDeclarativeEventPrefix)); On 2014/12/18 06:24:27, kalman wrote: > explain this? you should also EXTENSION_FUNCTION_VALIDATE that > kDeclarativeEventPrefix is actually found. Done.
The CQ bit was checked by hanxi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/794803002/340001
Message was sent while issue was closed.
Committed patchset #10 (id:340001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/b684c7ada5434b1954cced5e13a9d91ba88de6cd Cr-Commit-Position: refs/heads/master@{#309017} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
