RESOLVED FIXED 32326
Refactor some security code out of V8 bindings
https://bugs.webkit.org/show_bug.cgi?id=32326
Summary Refactor some security code out of V8 bindings
Charles Reis
Reported 2009-12-09 09:50:00 PST
As a first step toward sharing code between the V8 and JSC bindings, we should try to refactor some of V8's code for enforcing the Same Origin Policy. I suggest moving canAccessFrame and checkNodeSecurity from V8Proxy to a new BindingsSecurity class that can be used by multiple bindings. Since JSC passes around execution state explicitly and V8 does not, the BindingsSecurity interface should be able to take in execution state as an argument. There has been some discussion about starting to share bindings code in this thread: http://lists.macosforge.org/pipermail/webkit-dev/2009-May/007935.html
Attachments
Moves V8's canAccessFrame and checkNodeSecurity to a new BindingsSecurity class (40.21 KB, patch)
2009-12-09 10:01 PST, Charles Reis
no flags
Fixes issues from the style-queue bot (40.62 KB, patch)
2009-12-09 10:34 PST, Charles Reis
abarth: review-
Update BindingSecurity class to use templates (43.32 KB, patch)
2009-12-11 10:51 PST, Charles Reis
abarth: review+
abarth: commit-queue-
Adds newlines and a BindingSecurityBase class (45.48 KB, patch)
2009-12-11 14:56 PST, Charles Reis
no flags
Charles Reis
Comment 1 2009-12-09 10:01:05 PST
Created attachment 44542 [details] Moves V8's canAccessFrame and checkNodeSecurity to a new BindingsSecurity class
WebKit Review Bot
Comment 2 2009-12-09 10:14:23 PST
Attachment 44542 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/bindings/v8/custom/V8LocationCustom.cpp:42: Alphabetical sorting problem. [build/include_order] [4] WebCore/bindings/v8/custom/V8LocationCustom.cpp:222: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] WebCore/bindings/v8/custom/V8LocationCustom.cpp:240: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] WebCore/bindings/v8/custom/V8LocationCustom.cpp:259: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 4
Charles Reis
Comment 3 2009-12-09 10:34:39 PST
Created attachment 44544 [details] Fixes issues from the style-queue bot
WebKit Review Bot
Comment 4 2009-12-09 10:35:20 PST
style-queue ran check-webkit-style on attachment 44544 [details] without any errors.
Adam Barth
Comment 5 2009-12-09 10:40:34 PST
Comment on attachment 44544 [details] Fixes issues from the style-queue bot Thanks for fixing the style nits. At a high level, this looks great. I have a few questions: + virtual DOMWindow* getActiveWindow() = 0; Does this need to be virtual, or can we do link-time dispatch here? + // Implement BindingsSecurity methods that depend on V8-specific code. Don't we want to implement this in a V8BindingsSecurity.cpp file instead of V8Proxy? V8Proxy is an all-sing, all-dance dumping ground. + class V8State : public BindingsState { I'd call this V8BindingsState to mirror the DOMWindow / V8DOMWindow name convention. In general, it's slightly unclear to me whether we actually want a V8BindingsState object that derives from BindingsState or whether we want to have a single class where some of the methods are implemented by the different bindings, like ScriptController. The advantage of the latter is that we'll get compile-time dispatch instead of runtime dispatch. Another option is to have a shared BindingsStateBase and V8 implement a subclass BindingsState that the shared code refers to. That way, the bindings can override whatever methods they need without incurring a virtual method call. A final option is to use template namespaces. These shared classes would be template<classname Bindings> class ... and then refer to Bindings::State. Then we can have V8Binding::State derive from GenericBinding::State. That's more code, but it might make it easier to support things like re-using this code for ObjCBindings::State, etc. Thoughts?
Adam Barth
Comment 6 2009-12-09 10:48:11 PST
Anding some more folks in case they'd like to have input at this early stage. We're starting small to make sure we get started in the right direction.
Charles Reis
Comment 7 2009-12-09 13:43:28 PST
(In reply to comment #5) > (From update of attachment 44544 [details]) > Thanks for fixing the style nits. At a high level, this looks great. I have a > few questions: > > + virtual DOMWindow* getActiveWindow() = 0; > > Does this need to be virtual, or can we do link-time dispatch here? > (See below) > + // Implement BindingsSecurity methods that depend on V8-specific code. > > Don't we want to implement this in a V8BindingsSecurity.cpp file instead of > V8Proxy? V8Proxy is an all-sing, all-dance dumping ground. > Well, reportUnsafeAccessTo (and the enum it relies on) looks like it's only definied within V8Proxy.cpp, but I could move it to be a static member of V8Proxy that we can call outside V8Proxy.cpp. I could also move the immediatelyReportUnsafeAccessTo method to the BindingsState class, since that already has bindings-specific code. That'll clean up BindingsSecurity so that it's all shared code. > + class V8State : public BindingsState { > > I'd call this V8BindingsState to mirror the DOMWindow / V8DOMWindow name > convention. > > In general, it's slightly unclear to me whether we actually want a > V8BindingsState object that derives from BindingsState or whether we want to > have a single class where some of the methods are implemented by the different > bindings, like ScriptController. The advantage of the latter is that we'll get > compile-time dispatch instead of runtime dispatch. > > Another option is to have a shared BindingsStateBase and V8 implement a > subclass BindingsState that the shared code refers to. That way, the bindings > can override whatever methods they need without incurring a virtual method > call. > > A final option is to use template namespaces. These shared classes would be > > template<classname Bindings> > class ... > > and then refer to Bindings::State. Then we can have V8Binding::State derive > from GenericBinding::State. That's more code, but it might make it easier to > support things like re-using this code for ObjCBindings::State, etc. > > Thoughts? I think the template approach is probably the most flexible (while still giving us compile-time dispatch). I'll look into it.
Charles Reis
Comment 8 2009-12-11 10:51:08 PST
Created attachment 44696 [details] Update BindingSecurity class to use templates This updated patch makes BindingSecurity a template class so that each binding will be able to implement things it depends on, like execution state, without needing virtual methods. I threw in a few typedefs to make the code easier to use at the call sites. I also made the error reporting function visible outside V8Proxy.cpp so that we can call it. Thoughts?
WebKit Review Bot
Comment 9 2009-12-11 10:53:44 PST
style-queue ran check-webkit-style on attachment 44696 [details] without any errors.
Adam Barth
Comment 10 2009-12-11 13:08:59 PST
Comment on attachment 44696 [details] Update BindingSecurity class to use templates This looks like a great first step. I've noted a few formatting issues below. There are a couple of things that I'm not sure how they'll scale up when we convert more of the bindings, but they're fine for now. We can revisit them if the code isn't growing the right direction. + template <class Binding> class BindingSecurity { I think we'd prefer + template <class Binding> + class BindingSecurity { (meaning a line break) + class BindingSecurityHelper I'm not entirely sold on this class. We might need to see more examples to see what the best general pattern is for this kind of thing. + typedef BindingSecurity<V8Binding> V8BindingSecurity; I'm also not sure how this will scale. It's fine here though. + template <> class State<V8Binding> : public State<GenericBinding> { We'd also like a line break here.
Charles Reis
Comment 11 2009-12-11 13:27:47 PST
(In reply to comment #10) > (From update of attachment 44696 [details]) > This looks like a great first step. I've noted a few formatting issues below. > There are a couple of things that I'm not sure how they'll scale up when we > convert more of the bindings, but they're fine for now. We can revisit them if > the code isn't growing the right direction. > > + template <class Binding> class BindingSecurity { > > I think we'd prefer > > + template <class Binding> > + class BindingSecurity { > > (meaning a line break) > Sure, I'll upload a patch in just a minute with linebreaks on all the templates. > + class BindingSecurityHelper > > I'm not entirely sold on this class. We might need to see more examples to see > what the best general pattern is for this kind of thing. > I wasn't thrilled about this, but it seemed the best approach. That is, I don't think files like DOMWindow.h are supposed to be included in header files like BindingSecurity.h. For example, it was causing stack corruption problems for me in V8 code. On the flip side, templates won't link unless you forward declare all your instantiations in the cpp file (which is undesirable), or unless you include the method implementations in the header file (which appears to be common in WebKit). This helper class at least solves both problems-- it lets us call methods on the WebCore types while keeping them opaque in BindingSecurity.h. > + typedef BindingSecurity<V8Binding> V8BindingSecurity; > > I'm also not sure how this will scale. It's fine here though. > I was just using it to make the call sites a little shorter-- I'm not too attached to it in the long run. > + template <> class State<V8Binding> : public State<GenericBinding> { > > We'd also like a line break here. Sure.
Adam Barth
Comment 12 2009-12-11 13:32:20 PST
> I wasn't thrilled about this, but it seemed the best approach. Another option to consider is a non-template base class. That can implement its methods in its own cpp file. I'm not sure if that scales either. I'm inclined to try this. As we do move of this work, the right answer will likely reveal itself. Another option, by the way, is to move this code into WebCore proper. If it doesn't need to call back into the bindings, the dependencies allow it escape from the bindings directory.
Adam Barth
Comment 13 2009-12-11 13:34:29 PST
There's also the ScriptState abstraction, but I think this code is different. Basically ScriptState is part of WebCore's view of the bindings. This code is internal to the bindings themselves.
Charles Reis
Comment 14 2009-12-11 14:56:27 PST
Created attachment 44709 [details] Adds newlines and a BindingSecurityBase class I took Adam's suggestion to make BindingSecurityHelper into BindingSecurityBase, and I added the requested newlines. I'm inclined to think that this code should stay in the bindings directory, but if WebCore proper has a better place for it, that's fine.
WebKit Review Bot
Comment 15 2009-12-11 14:59:48 PST
style-queue ran check-webkit-style on attachment 44709 [details] without any errors.
Adam Barth
Comment 16 2009-12-11 15:13:00 PST
Comment on attachment 44709 [details] Adds newlines and a BindingSecurityBase class This looks great. I'm going to wait a bit before landing this in case someone else wants to give their opinion. + DEFINE_STATIC_LOCAL(State, globalV8BindingState, ()); I'm surprised that compiles without the template paramater, but if it does that's fine. :)
Adam Barth
Comment 17 2009-12-13 21:01:13 PST
Comment on attachment 44709 [details] Adds newlines and a BindingSecurityBase class Clearing flags on attachment: 44709 Committed r52080: <http://trac.webkit.org/changeset/52080>
Adam Barth
Comment 18 2009-12-13 21:01:21 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.