Webkit and Opera do this. Haven't tested on IE.

The trustness check was added in Bug 265176.

Which was fixed before Bug 289940, which would have fixed it too, I think.

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.

David, if you're busy with other things, assign this to me.

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 :-).

(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.

Uploaded to tryserver. We may have tests which check that untrusted events *don't* active links.

> 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.

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" .

The patch gives the same _blank handling what Chrome has.

What is Chrome's behavior?

A new tab is opened, even if click is dispatched using a timer.

Let's not do that.

> 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?

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...

Ah, Chrome's behavior depends on whether the test is local file or not. If not, _blank pages aren't opened.

Better patch coming.

Hmm, no, the problem wasn't what I was thinking about.

A testcase with some timers http://mozilla.pettay.fi/moztests/a_click.html

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"

Pushed to try.

(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.

Just to clarify, comment 23 was about Chrome.

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....

Oh, to be clear in general this looks fine.

Comment on attachment 542904 [details] [diff] [review] v4 r=me

Compiling now a patch for Fx5 (with nsILinkHandler_5_0 so that there is no need for an interface change)

Comment on attachment 542911 [details] [diff] [review] For fx5 r=me

Are we really taking patches for Firefox 5? I though Firefox 6 will be the update.

There's discussion that we might do a Firefox 5 respin *for this bug*.

(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.

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).

The previous patch for trunk missed a parameter rename.

(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.

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; }

The behavior in this bug is not new. Sites exercising this codepath is new, and is a regression from 4 to 5.

OK, thanks.

(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.

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.

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?

No. It changed interfaces.

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 on attachment 542911 [details] [diff] [review] For fx5 We could take this patch to aurora.

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?

It acts as if the key was not pressed.

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.)

test7() seems backwards. It should be testing that the link does *not* open (at least not in a new window). Why is it passing?

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 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?

(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 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.

Does not work in firefox 6 beta, confirmed to work in the Aurora version.

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?

(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?

Oh, the testcase is what landed after the merge. Nevermind me....

I did ask for a sec review. No idea when it could happen.

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.

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.

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.

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.

http://hg.mozilla.org/releases/mozilla-beta/rev/084847ea02b4 http://hg.mozilla.org/releases/mozilla-beta/rev/daef8d0f83b0

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? :/

> 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.

Version detection may be the lesser evil here then - it seems to be a fairly moz-specific peculiarity at least

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.