WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
37008
ScriptController::processingUserGestureEvent returns true for simulated clicks
https://bugs.webkit.org/show_bug.cgi?id=37008
Summary
ScriptController::processingUserGestureEvent returns true for simulated clicks
Andy Estes
Reported
2010-04-01 21:52:55 PDT
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.
Attachments
test case
(631 bytes, text/html)
2010-04-01 21:52 PDT
,
Andy Estes
no flags
Details
patch
(25.35 KB, patch)
2010-04-01 23:42 PDT
,
Andy Estes
no flags
Details
Formatted Diff
Diff
patch (v2)
(27.94 KB, patch)
2010-04-02 00:11 PDT
,
Andy Estes
abarth
: review-
Details
Formatted Diff
Diff
patch (v3)
(25.13 KB, patch)
2010-04-02 19:54 PDT
,
Andy Estes
abarth
: review-
abarth
: commit-queue-
Details
Formatted Diff
Diff
patch (v4)
(26.08 KB, patch)
2010-04-02 23:21 PDT
,
Andy Estes
abarth
: review+
abarth
: commit-queue-
Details
Formatted Diff
Diff
Updated license headers
(6.34 KB, patch)
2010-04-06 11:05 PDT
,
Andy Estes
mitz: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Andy Estes
Comment 1
2010-04-01 21:53:29 PDT
<
rdar://problem/7783498
>
Andy Estes
Comment 2
2010-04-01 23:42:52 PDT
Created
attachment 52385
[details]
patch
Early Warning System Bot
Comment 3
2010-04-01 23:57:36 PDT
Attachment 52385
[details]
did not build on qt: Build output:
http://webkit-commit-queue.appspot.com/results/1624189
WebKit Review Bot
Comment 4
2010-04-02 00:04:33 PDT
Attachment 52385
[details]
did not build on chromium: Build output:
http://webkit-commit-queue.appspot.com/results/1609178
Andy Estes
Comment 5
2010-04-02 00:11:42 PDT
Created
attachment 52388
[details]
patch (v2) Forgot to add UserGestureState.{h, cpp} to all build systems.
Eric Seidel (no email)
Comment 6
2010-04-02 00:17:33 PDT
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.
Alexey Proskuryakov
Comment 7
2010-04-02 08:20:53 PDT
Wasn't this supposed to be fixed in
bug 21501
AKA <
rdar://problem/4599085
>?
Adam Barth
Comment 8
2010-04-02 08:31:21 PDT
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.
Adam Barth
Comment 9
2010-04-02 08:33:31 PDT
Those with Chromium security bug access can see an example here:
http://code.google.com/p/chromium/issues/detail?id=34414
Eric Carlson
Comment 10
2010-04-02 09:40:06 PDT
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)?
Geoffrey Garen
Comment 11
2010-04-02 10:12:40 PDT
+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.)
Andy Estes
Comment 12
2010-04-02 11:02:56 PDT
(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").
Andy Estes
Comment 13
2010-04-02 14:58:02 PDT
(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.
Adam Barth
Comment 14
2010-04-02 15:12:19 PDT
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. :)
Geoffrey Garen
Comment 15
2010-04-02 16:09:40 PDT
> 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?
Adam Barth
Comment 16
2010-04-02 16:12:20 PDT
> 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.
Maciej Stachowiak
Comment 17
2010-04-02 16:27:54 PDT
(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.
Andy Estes
Comment 18
2010-04-02 19:54:38 PDT
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.
WebKit Review Bot
Comment 19
2010-04-02 20:20:18 PDT
Attachment 52474
[details]
did not build on chromium: Build output:
http://webkit-commit-queue.appspot.com/results/1568187
Adam Barth
Comment 20
2010-04-02 20:43:49 PDT
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.
Andy Estes
Comment 21
2010-04-02 23:21:39 PDT
Created
attachment 52484
[details]
patch (v4)
Adam Barth
Comment 22
2010-04-02 23:26:57 PDT
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.
Andy Estes
Comment 23
2010-04-02 23:35:26 PDT
(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!
Adam Barth
Comment 24
2010-04-02 23:43:40 PDT
> 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.
Adam Barth
Comment 25
2010-04-03 00:06:17 PDT
Committed
r57045
: <
http://trac.webkit.org/changeset/57045
>
Eric Seidel (no email)
Comment 26
2010-04-04 13:13:23 PDT
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
Inferno
Comment 27
2010-04-05 10:15:50 PDT
***
Bug 34541
has been marked as a duplicate of this bug. ***
Abhishek Arya
Comment 28
2010-04-05 10:55:19 PDT
The patch does not look to fix the popup blocker bypass here -
http://www.securethoughts.com/security/popupbypass/popup.html
. any idea why.
Adam Barth
Comment 29
2010-04-05 10:59:37 PDT
(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.
Chris Evans
Comment 30
2010-04-05 15:10:31 PDT
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.
Kent Tamura
Comment 31
2010-04-06 00:36:15 PDT
UserGestureIndicator.cpp has non-ASCII characters, quotations around "AS IS". They cause a build error on Japanese Windows.
Andy Estes
Comment 32
2010-04-06 11:03:53 PDT
(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
.
Andy Estes
Comment 33
2010-04-06 11:05:19 PDT
Created
attachment 52652
[details]
Updated license headers Updated the license headers in UserGestureIndicator.{h, cpp} to remove non-ASCII characers.
mitz
Comment 34
2010-04-06 11:11:40 PDT
Comment on
attachment 52652
[details]
Updated license headers rs=me
mitz
Comment 35
2010-04-06 11:18:31 PDT
Committed
attachment 52652
[details]
as
r57159
.
Daniel Bates
Comment 36
2010-07-23 22:10:59 PDT
Comment on
attachment 52652
[details]
Updated license headers Clearing commit-queue flag to get this out of the commit-queue.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug