Created attachment 52373 [details] test case FrameLoader::isProcessingUserGesture is used by the various parts of WebCore to prevent scripts from doing things without input from a user, for example open a pop-up window. This function calls ScriptController::processingUserGestureEvent which only considers the type of the current event, not whether or not it was actually generated by a user gesture. It is possible to programatically call click() on an HTML button element to open a popup window without an underlying user gesture, as generated by the attached test case.
<rdar://problem/7783498>
Created attachment 52385 [details] patch
Attachment 52385 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/1624189
Attachment 52385 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1609178
Created attachment 52388 [details] patch (v2) Forgot to add UserGestureState.{h, cpp} to all build systems.
Comment on attachment 52388 [details] patch (v2) WTFNonHeapAllocatable seems like a totally separate patch for a totally separate bug. I think this would be much easier to review if you split them.
Wasn't this supposed to be fixed in bug 21501 AKA <rdar://problem/4599085>?
Comment on attachment 52388 [details] patch (v2) This is great. I've wanted to do this for a while but hadn't gotten around to it. Some nits: + createdByDOM I think you can remove this concept now. + NonHeapAllocatable.h You've add this header to the xcodeproj but not to the other build systems. Please add it to all the build systems (except the "pro", which doesn't include headers). As for whether this is fixed, we have more popup blocker bypasses that this either will fix or give us the tools to fix.
Those with Chromium security bug access can see an example here: http://code.google.com/p/chromium/issues/detail?id=34414
25 * dom/UserGestureState.cpp: Added. This class follows the RAAI pattern 26 to manage a global flag indicating that a user gesture is being 27 processed. Don't you mean "follows the RAII pattern" (Resource Acquisition Is Initialization)?
+namespace WTFNonHeapAllocatable { You should include the same comment that Noncopyable has to explain why it gets its own namespace. +class NonHeapAllocatable { What a mouthful! :) I guess I don't have a better idea, though :(. + void* operator new(size_t) + { + ASSERT_NOT_REACHED(); + return reinterpret_cast<void*>(0xbadbeef); + } + + void* operator new[](size_t) + { + ASSERT_NOT_REACHED(); + return reinterpret_cast<void*>(0xbadbeef); + } It's better just to declare these functions, without defining them. That way, if you somehow tricked a compiler into issuing a call to them, linking would still fail. Also, it's more concise. You might as well throw in the delete operators for good measure. bool Event::fromUserGesture() { - if (createdByDOM()) + if (!UserGestureState::isProcessingUserGesture()) This will work, but it leaves the code in a bit of a confusing state. A good future cleanup would be for UserGestureState to keep a HashCountedSet of frames that are processing user gestures. Then, instead of asking an event whether it thought it was a user gesture, you could ask UserGestureState whether a given frame was processing a user gesture. That would eliminate the need for things like currentEvent, too. + static inline bool isProcessingUserGesture() { return processingUserGesture; } It's unfortunate when one thing has two names. Here, the accessor name gets the word "is," but the variable name doesn't -- I assume to avoid name conflict. One of the nice benefits of prefixing variable names is that it solves such conflicts. I would recommend renaming processingUserGesture to s_ isProcessingUserGesture. + UserGestureState gestureIndicator; This is another case of one thing having two names. (Is it a "user gesture" or a "gesture?" Is it a "state" or an "indicator?") I'd recommend picking one name and using it everywhere. I like "user gesture indicator" or "user gesture record" best, since it's more specific about both the gesture and the state. (Technically, "state" could be anything, including "nothing's happening." So creating an empty "state" doesn't say as much.)
(In reply to comment #10) > 25 * dom/UserGestureState.cpp: Added. This class follows the RAAI > pattern > 26 to manage a global flag indicating that a user gesture is being > 27 processed. > Don't you mean "follows the RAII pattern" (Resource Acquisition Is > Initialization)? Right you are (I always forget that it is "is" not "as").
(In reply to comment #6) > (From update of attachment 52388 [details]) > WTFNonHeapAllocatable seems like a totally separate patch for a totally > separate bug. I think this would be much easier to review if you split them. Good point, Eric. See https://bugs.webkit.org/show_bug.cgi?id=37046. Once that patch lands, I'll re-post a patch to this bug that addresses Adam's and Geoff's comments.
Not to nit-pick, but it would be awesome to re-spin this as a generic RAII boolean class via templates. The user gesture could then be a typedefed instantiation of the template. :)
> Not to nit-pick, but it would be awesome to re-spin this as a generic RAII > boolean class via templates. The user gesture could then be a typedefed > instantiation of the template. :) Urgh. You could do that. But what's the benefit?
> Urgh. You could do that. But what's the benefit? The benefit is that we can write the next RAII boolean in one line of code instead of 78. The point is mostly that UserGestureState.* has nothing to do with user gestures and can profitably be abstracted into WTF.
(In reply to comment #16) > > Urgh. You could do that. But what's the benefit? > > The benefit is that we can write the next RAII boolean in one line of code > instead of 78. The point is mostly that UserGestureState.* has nothing to do > with user gestures and can profitably be abstracted into WTF. Sounds like a reasonable idea, but I think the extra abstraction can be separate from the bugfix.
Created attachment 52474 [details] patch (v3) Based on discussions with Maciej, I decided that since some common heap allocation scenarios could not be prevented using NonHeapAllocatable, it wasn't worth indicating and giving a false sense of security. This patch removes NonHeapAllocatable and also addresses other comments from Adam and Geoff.
Attachment 52474 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1568187
Comment on attachment 52474 [details] patch (v3) + namespace WebCore { We usually have a blank line under the namespace declaration. + static inline bool processingUserGesture() { return s_processingUserGesture; } No need to declare inline here. + #include <new> Why do we need this here? Also, please fix the Chromium build failure. If you were a committer, I would r+ your patch and ask you to fix these things on landing, but as a non-committer, I'm going to ask you to upload a new patch so we can land it using the bot. Thanks for the patch.
Created attachment 52484 [details] patch (v4)
Comment on attachment 52484 [details] patch (v4) + public: ^^^ shouldn't be indented. +} // namespace WebCore ^^^ blank line before this line. r=me I'm happy to land this for you (with these style changes) if you're tired of re-spinning the patch. I'm leaving the r? for a bit so that all the EWS bots run on your patch.
(In reply to comment #22) > (From update of attachment 52484 [details]) > + public: > > ^^^ shouldn't be indented. > > +} // namespace WebCore > > ^^^ blank line before this line. > > r=me > > I'm happy to land this for you (with these style changes) if you're tired of > re-spinning the patch. I'm leaving the r? for a bit so that all the EWS bots > run on your patch. Sorry about the style bugs :( If all the EWS bots go green, feel free to land the patch. Thanks!
> Sorry about the style bugs :( No problem. It's too bad the style bot can't catch more of these automatically. > If all the EWS bots go green, feel free to land > the patch. Thanks! Looks like we're good to go. I'll land this now.
Committed r57045: <http://trac.webkit.org/changeset/57045>
Looks like this turn a Gtk bot red: http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release/r57045%20(1733)/fast/events/popup-allowed-from-gesture-initiated-event-pretty-diff.html
*** Bug 34541 has been marked as a duplicate of this bug. ***
The patch does not look to fix the popup blocker bypass here - http://www.securethoughts.com/security/popupbypass/popup.html. any idea why.
(In reply to comment #28) > The patch does not look to fix the popup blocker bypass here - > http://www.securethoughts.com/security/popupbypass/popup.html. any idea why. There are lots of popup blocker bypasses. This patch adds a tool we can use to fix them. You'll probably have to debug the issue. I bet we're missing a processingUserGesture check somewhere in this code path.
I'm just fixing a Chromium-specific bug whereby block preferences don't necessarily get considered properly. I'll retest all the window.open() bypasses once I have this change and my change together.
UserGestureIndicator.cpp has non-ASCII characters, quotations around "AS IS". They cause a build error on Japanese Windows.
(In reply to comment #26) > Looks like this turn a Gtk bot red: > http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release/r57045%20(1733)/fast/events/popup-allowed-from-gesture-initiated-event-pretty-diff.html Looks like this was fixed in http://trac.webkit.org/changeset/57052.
Created attachment 52652 [details] Updated license headers Updated the license headers in UserGestureIndicator.{h, cpp} to remove non-ASCII characers.
Comment on attachment 52652 [details] Updated license headers rs=me
Committed attachment 52652 [details] as r57159.
Comment on attachment 52652 [details] Updated license headers Clearing commit-queue flag to get this out of the commit-queue.