Last Comment Bug 666604 - Allow untrusted events to trigger a link
: Allow untrusted events to trigger a link
Status: VERIFIED FIXED
[landed m-c 7/06] [semi-widespread We...
: regression
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 4 votes (vote)
: mozilla7
Assigned To: Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
:
Mentors:
Depends on: 812202 684208
Blocks: 265176 395917 583514 666674 666769 666955 666985 667632 668223 670528
  Show dependency treegraph
 
Reported: 2011-06-23 09:06 PDT by Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
Modified: 2013-12-27 14:32 PST (History)
22 users (show)
dveditz: sec‑review+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed


Attachments
testcase (508 bytes, text/html)
2011-06-23 09:06 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details
patch (4.50 KB, patch)
2011-06-29 04:48 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
patch (4.49 KB, patch)
2011-06-29 06:50 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
patch (14.82 KB, patch)
2011-06-29 11:38 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
v4 (18.17 KB, patch)
2011-06-29 12:09 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
bzbarsky: review+
Details | Diff | Splinter Review
For fx5 (19.20 KB, patch)
2011-06-29 12:58 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
bzbarsky: review+
asa: approval‑mozilla‑beta+
Details | Diff | Splinter Review
param rename (18.17 KB, patch)
2011-06-30 04:59 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
+additional test (1.16 KB, patch)
2011-07-06 04:10 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
+additional test (2.74 KB, patch)
2011-07-06 05:38 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
mounir: review+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-23 09:06:55 PDT
Created attachment 541396 [details]
testcase

Webkit and Opera do this. Haven't tested on IE.
Comment 1 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-23 10:05:19 PDT
The trustness check was added in Bug 265176.
Comment 2 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-23 10:10:34 PDT
Which was fixed before Bug 289940, which would have fixed it too, I think.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2011-06-27 20:40:01 PDT
With bug 583514 fixed, I think we need to do this for web compat.  Lots of sites seem to expect that click() on an <a>, if present, will trigger the link, and it doesn't right now due to this bug...

I don't think there's really anything you can do with click events on anchors that you can't do by setting location.href or calling window.open, from a security perspective.  We'd need to check that you can't bypass the popup blocker with it, I guess.
Comment 4 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-28 10:02:38 PDT
David, if you're busy with other things, assign this to me.
Comment 5 Steffan Corley 2011-06-29 02:05:25 PDT
Looks like this is causing bug 666955 / bug 666674, which breaks some key functionality in a commonly used Ajax / web control library for ASP.NET (http://www.telerik.com/products/aspnet-ajax/ajax.aspx).

For my company, this means we will likely have to recommend our customers not to use Firefox 5 with our multi-million user educational intranet application.  I suspect the RadAjax controls controls are fairly common on corporate intranet applications.  I'd therefore strongly vote for a 5.01 fix, but I know that Mozilla doesn't care about corporates :-).
Comment 6 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-29 04:10:22 PDT
(I don't know why you think Mozilla doesn't care about corporates.
 If some mozillian says something like that, it doesn't necessarily mean
 everyone in the project agrees with that.)


Anyway, patch coming, and I certainly think this must be fixed for Fx6, which
will be released in August.
Comment 7 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-29 04:48:30 PDT
Created attachment 542772 [details] [diff] [review]
patch

Uploaded to tryserver. We may have tests which check that 
untrusted events *don't* active links.
Comment 8 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-29 06:50:44 PDT
Created attachment 542801 [details] [diff] [review]
patch
Comment 9 Jesse Ruderman 2011-06-29 07:40:02 PDT
> We'd need to check that you can't bypass the popup blocker with it, I guess.

The test should probably cover both synthetic ctrl+click and synthetic normal clicks on links with target=_blank.

Similarly, I don't want sites to be able to synthesize alt+click and cause Firefox to download something it normally wouldn't download.
Comment 10 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-29 07:46:08 PDT
The patch allows synthetic clicks to trigger link only when
!ctrl && !alt && !meta && !shift.
And the test does cover the ctrl case (alt, meta and shift are handled the same way)

I'm not sure how we should handle target="_blank" .
Comment 11 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-29 07:52:49 PDT
The patch gives the same _blank handling what Chrome has.
Comment 12 Jesse Ruderman 2011-06-29 07:54:53 PDT
What is Chrome's behavior?
Comment 13 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-29 07:58:17 PDT
A new tab is opened, even if click is dispatched using a timer.
Comment 14 Jesse Ruderman 2011-06-29 08:01:38 PDT
Let's not do that.
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2011-06-29 08:11:15 PDT
> I'm not sure how we should handle target="_blank" .

The same way as window.open(), I would think....  Why is the popup blocker not kicking in?
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2011-06-29 08:13:03 PDT
In particular, OnLinkClickEvent::OnLinkClickEvent saves the popup control state, and then pushes it before doing OnLinkClickSync.  So I'd expect the popup blocker to work here...
Comment 17 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-29 08:20:38 PDT
Ah, Chrome's behavior depends on whether the test is local file or not.
If not, _blank pages aren't opened.
Comment 18 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-29 08:23:21 PDT
Better patch coming.
Comment 19 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-29 08:33:56 PDT
Hmm, no, the problem wasn't what I was thinking about.
Comment 20 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-29 11:33:04 PDT
A testcase with some timers http://mozilla.pettay.fi/moztests/a_click.html
Comment 21 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-29 11:38:04 PDT
Created attachment 542881 [details] [diff] [review]
patch

Push the cx to stack if untrusted event triggers the link.
With that popup blocker works the same way as with window.open() when
target="_blank"
Comment 22 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-29 11:38:32 PDT
Pushed to try.
Comment 23 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-29 11:47:35 PDT
(In reply to comment #17)
> Ah, Chrome's behavior depends on whether the test is local file or not.
> If not, _blank pages aren't opened.
Hmm, I'm not sure about this anymore. _blank is opened sometimes, but
not always.
Comment 24 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-29 11:48:37 PDT
Just to clarify, comment 23 was about Chrome.
Comment 25 Boris Zbarsky [:bz] (still a bit busy) 2011-06-29 11:49:40 PDT
I would really prefer a required argument to TriggerLink and for that matter on OnLinkClick.  For the latter, there are only two callers, and one of them already passes all the args anyway and you're changing the other one.

Also, would it make more sense to make the booleand "aIsTrusted"?  There would be fewer negatives in the code that way, I think....
Comment 26 Boris Zbarsky [:bz] (still a bit busy) 2011-06-29 11:49:55 PDT
Oh, to be clear in general this looks fine.
Comment 27 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-29 12:09:50 PDT
Created attachment 542904 [details] [diff] [review]
v4
Comment 28 Boris Zbarsky [:bz] (still a bit busy) 2011-06-29 12:14:11 PDT
Comment on attachment 542904 [details] [diff] [review]
v4

r=me
Comment 29 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-29 12:27:44 PDT
Compiling now a patch for Fx5 (with nsILinkHandler_5_0 so that there is no
need for an interface change)
Comment 30 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-29 12:58:23 PDT
Created attachment 542911 [details] [diff] [review]
For fx5
Comment 31 Boris Zbarsky [:bz] (still a bit busy) 2011-06-29 14:09:19 PDT
Comment on attachment 542911 [details] [diff] [review]
For fx5

r=me
Comment 32 Mounir Lamouri (:mounir) 2011-06-29 16:08:48 PDT
Are we really taking patches for Firefox 5? I though Firefox 6 will be the update.
Comment 33 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-06-29 16:34:04 PDT
There's discussion that we might do a Firefox 5 respin *for this bug*.
Comment 34 Mounir Lamouri (:mounir) 2011-06-29 16:44:55 PDT
(In reply to comment #33)
> There's discussion that we might do a Firefox 5 respin *for this bug*.

Oh sorry, I didn't know.
Comment 35 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-30 04:48:44 PDT
Technically this is not a regression, or it is a very old regression.
Some web pages just happen to rely click() to do one thing (dispatch click event 
as we do), and untrusted click to activate link (which we haven't done since Bug 
265176).
Comment 36 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-30 04:59:51 PDT
Created attachment 543104 [details] [diff] [review]
param rename

The previous patch for trunk missed a parameter rename.
Comment 37 Christopher Blizzard (:blizzard) 2011-06-30 10:33:57 PDT
(In reply to comment #35)
> Technically this is not a regression, or it is a very old regression.

I see dates like 2004.  Is this a regression from 4-5?  If it's that old I doubt we need to spin a 5.0.1 for this.
Comment 38 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-30 10:36:26 PDT
The issue is that we added .click to <a>, and that click() dispatches
untrusted click, like it should, but does not trigger the link.

And for unknown reasons web pages have code like
if (a.click) {
  a.click();
} else {
  window.location = a.href;
}
Comment 39 Boris Zbarsky [:bz] (still a bit busy) 2011-06-30 10:44:19 PDT
The behavior in this bug is not new.

Sites exercising this codepath is new, and is a regression from 4 to 5.
Comment 40 Christopher Blizzard (:blizzard) 2011-06-30 10:45:35 PDT
OK, thanks.
Comment 41 Daniel Veditz [:dveditz] 2011-06-30 10:53:11 PDT
(In reply to comment #32)
> Are we really taking patches for Firefox 5? I though Firefox 6 will be the
> update.

Firefox 6 is the "scheduled" update. There's always the possibility of an interim release ("chemspill") if we broke the web. It's expensive (in time and resources) so we have to balance how broken vs. how soon a planned fix can come.
Comment 42 Christopher Blizzard (:blizzard) 2011-06-30 10:59:08 PDT
I put this on the agenda for today's Aurora meeting since that's the right audience.  First time someone has wanted a chemspill for compatibility.
Comment 43 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-30 11:27:27 PDT
http://hg.mozilla.org/mozilla-central/rev/7deef688bbf5
Comment 44 Jesse Ruderman 2011-06-30 12:22:43 PDT
This seems like the kind of patch that could have unintended compatibility effects, too.  Could we back out bug 583514 for Firefox 5.0.x instead?
Comment 45 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-06-30 13:41:29 PDT
No. It changed interfaces.
Comment 46 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-06-30 15:06:07 PDT
tracking-firefox6+ per triage meeting

Does comment 43 mean this is no longer checkin-needed and should be RESOLVED-FIXED?  If not, what is checkin-needed?

Please make an approval-aurora request for the appropriate patch so this can get into Aurora soon.
Comment 47 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-07-01 06:44:20 PDT
Comment on attachment 542911 [details] [diff] [review]
For fx5

We could take this patch to aurora.
Comment 48 Daniel Cater 2011-07-03 09:08:03 PDT
If a modifier key is physically pressed and then the link is triggered by an untrusted event, does the link act as if the key was pressed or not?
Comment 49 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-07-03 12:04:21 PDT
It acts as if the key was not pressed.
Comment 50 Asa Dotzler [:asa] 2011-07-05 14:50:41 PDT
Comment on attachment 542911 [details] [diff] [review]
For fx5

Please re-request approval on the patch after all reviews are completed. (Security review still pending.)
Comment 51 Jesse Ruderman 2011-07-05 15:19:47 PDT
test7() seems backwards. It should be testing that the link does *not* open (at least not in a new window).  Why is it passing?
Comment 52 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-07-06 03:20:54 PDT
test7 relies on dom.disable_open_during_load to be false,
and that is set false for mochitests.
(we have way too many prefs for popup handling, and the naming of them isn't
quite right).

I'll add a test for dom.disable_open_during_load=true
Comment 53 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-07-06 04:10:11 PDT
Created attachment 544189 [details] [diff] [review]
+additional test
Comment 54 Mounir Lamouri (:mounir) 2011-07-06 04:28:06 PDT
Comment on attachment 544189 [details] [diff] [review]
+additional test

Review of attachment 544189 [details] [diff] [review]:
-----------------------------------------------------------------

Could you explicitly set dom.disable_open_during_load to false in test7?
In addition, I do not really like the setTimeout(test9, 1000). Can't you use the click event and go X time to the event loop to assume nothing happened?
Comment 55 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-07-06 04:30:37 PDT
(In reply to comment #54)
> Could you explicitly set dom.disable_open_during_load to false in test7?
Why?


> In addition, I do not really like the setTimeout(test9, 1000). Can't you use
> the click event and go X time to the event loop to assume nothing happened?
I don't understand what you mean. X time to the event loop?
Comment 56 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-07-06 05:38:31 PDT
Created attachment 544209 [details] [diff] [review]
+additional test
Comment 57 Mounir Lamouri (:mounir) 2011-07-06 05:56:27 PDT
Comment on attachment 544209 [details] [diff] [review]
+additional test

Review of attachment 544209 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/test/test_bug666604.html
@@ +137,5 @@
> +      ok(false, "Click() should not activate a link");
> +      setTimeout(test9, 0);
> +    }
> +  testlink.click();  
> +  hitEventLoop(10, test9);

I would have make gActivated being a variable, testlink.href setting it to "true" and hitEventLoop calling:
function() {
  ok(!gActivated, "...");
  gActivated = false;
  test9();
}

Though, what you did should work too.
Comment 58 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-07-06 07:27:30 PDT
http://hg.mozilla.org/mozilla-central/rev/eb4d059af819
Comment 59 herriedm 2011-07-08 17:04:22 PDT
Does not work in firefox 6 beta,  confirmed to work in the Aurora version.
Comment 60 Boris Zbarsky [:bz] (still a bit busy) 2011-07-08 20:54:15 PDT
That would be a neat trick, since this fix didn't land in aurora yet.

Olli, is there a security review scheduled for this at least?
Comment 61 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-07-08 20:58:42 PDT
(In reply to comment #60)
> That would be a neat trick, since this fix didn't land in aurora yet.

It landed before the merge, no?
Comment 62 Boris Zbarsky [:bz] (still a bit busy) 2011-07-08 21:12:23 PDT
Oh, the testcase is what landed after the merge.  Nevermind me....
Comment 63 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-07-09 02:44:20 PDT
I did ask for a sec review. No idea when it could happen.
Comment 64 Daniel Veditz [:dveditz] 2011-07-12 10:47:31 PDT
sec-review-complete, fix is limited in scope to links (as opposed to changing events themselves) and doesn't regress old related security bugs.

Didn't intend for "sec-review-needed" to block landing (we do many reviews post-landing), would have put an explicit review request on the patch if I did.
Comment 65 Johnathan Nightingale [:johnath] 2011-07-13 11:39:09 PDT
Discussed this request in approval triage today. Late-landing this in the beta cycle, which is supposed to be exclusively for backouts of bustage caused in this branch feels like a bad idea, but blizzard thinks that we need to be willing to make exceptions for major web compatibility fixes.

We in the room couldn't get a good sense of how widespread this bustage was, though - it feels like a weird pattern to synthesize click events on links, but we know one framework seems to use it. Smaug, bz - any intuitions here on it being more widespread than we think? Otherwise it feels like letting people get this fix in 7 is the right choice, since we don't take changes on aurora/beta that aren't current-version-bustage-fixes.
Comment 66 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-07-13 12:18:08 PDT
calling <a>.click() doesn't cause us to follow the link.
In Fx4 we didn't have <a>.click();

For some mysterious reason code mentioned in comment 38 is used quite often.

Also, see all the bugs this bug fixed.
Comment 67 Boris Zbarsky [:bz] (still a bit busy) 2011-07-13 12:35:08 PDT
Johnathan, we have 6 independent reports of this issue, from sites using at least 3 different frameworks that all have essentially the same code, structured like comment 38.  It seems to be a common workaround for a bug in some old browser (old IE version, I'm guessing based on other code involved) that doesn't send a referrer on location.href sets.  As a result, sites that want to track the referrer do this hack with .click if it's present and setting location if it's not present (this last presumably to work with gecko-based browsers).

So yes, it's a weird pattern, but it's being done for very logical reasons, and the usage seems to be somewhat widespread.
Comment 68 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-07-19 10:22:39 PDT
http://hg.mozilla.org/releases/mozilla-beta/rev/084847ea02b4
http://hg.mozilla.org/releases/mozilla-beta/rev/daef8d0f83b0
Comment 69 Ruy Asan 2011-07-21 10:39:02 PDT
In terms of working around the issue in 5.x, what would be the best way to detect if a given event object will actually be allowed to open a new link without resorting to browser + version detection?

Previously I relied on event.isTrusted === false but it seems this new fix retains synthetic events as untrusted but simply changes their policy, so now what? :/
Comment 70 Boris Zbarsky [:bz] (still a bit busy) 2011-07-21 10:45:10 PDT
> what would be the best way to detect if a given event object will actually be
> allowed to open a new link

Condition it off event.isTrusted after testing via object-detection (e.g. by using an untrusted click event in a hidden iframe) whether untrusted events are allowed to trigger links?

There's really no good way to object-detect security policy without trying to do something that the policy might disallow and seeing whether it's allowed, in general.
Comment 71 Ruy Asan 2011-07-21 11:03:01 PDT
Version detection may be the lesser evil here then - it seems to be a fairly moz-specific peculiarity at least
Comment 72 Simona B [:simonab ] 2011-08-11 06:31:28 PDT
Mozilla/5.0 (Windows NT 5.1; rv:8.0a1) Gecko/20110810 Firefox/8.0a1
Mozilla/5.0 (Windows NT 5.1; rv:6.0) Gecko/20100101 Firefox/6.0

Verified issue on Win XP, Win 7, Mac OS X 10.7 and Ubuntu 11.04 using the test case attached in the description. 

Setting resolution to VERIFIED FIXED.

Note You need to log in before you can comment on or make changes to this bug.