Opened 15 months ago

Closed 15 months ago

Last modified 15 months ago

#18168 closed defect (fixed)

Iframe based AJAX call blocked by popup-blocker or opening in new tab

Reported by: sky_kohai Owned by: mcs
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Major Keywords: tbb-usability-website, TorBrowserTeam201602, TorBrowserTeam201602R tbb-5.5-regression
Cc: mcs, brade, arthuredelstein Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Iframe based AJAX call blocked by popup-blocker. After allowing popups, form submits to new tab instead of hidden iframe.

Issue appeared in TBB 5.5. TBB 5.0.7 (previous stable) works with same code normally.

To reproduce go to http://dmirrgetyojz735v.onion/d/#set-lang:en and try to make post with JS support enabled.
Code of submit and AJAX handlers can be found in http://dmirrgetyojz735v.onion/js/tinaib1.js , seek doPostForm func.

Trace is like

WARNING: content window passed to PrivateBrowsingUtils.isWindowPrivate. Use isContentWindowPrivate instead (but only for frame scripts).
pbu_isWindowPrivate@resource://gre/modules/PrivateBrowsingUtils.jsm:25:14
nsBrowserAccess.prototype.openURI@chrome://browser/content/browser.js:15194:21

Depending of the exact code, call stack may contain func, which actually submitted form.

Tested on Ubuntu 14.04 x64.

Child Tickets

Change History (15)

comment:1 Changed 15 months ago by gk

  • Cc mcs brade arthuredelstein added
  • Priority changed from Medium to High
  • Severity changed from Normal to Major

I can reproduce this and it seems we have more users with similar problems (see e.g.: https://blog.torproject.org/blog/tor-browser-55-released#comment-153779) I am wondering what is causing this. One quick revert of Torbutton to 1.9.3.7 did not show a difference.

comment:2 Changed 15 months ago by gk

5.5a4 is the first alpha that breaks. Looking at the intersection of bugs that both got fixed in this alpha and in 5.5 stable this leaves us with #16620, #17351 and the font bugs closed in the alpha.

comment:3 Changed 15 months ago by gk

Reverting Torbutton to 1.9.3.7 should take #17351 out of the loop and the Torbutton portion of #16620. Seems the tor-browser patch for #16620 and the font bugs are left.

comment:4 Changed 15 months ago by gk

  • Owner changed from tbb-team to mcs
  • Status changed from new to assigned

Okay, 95a62d13e045b950ed2b2f242da7413d33386a47 seems to be the culprit. Backing that one out from tor-browser-38.3.0esr-5.5-2 leads to a working Tor Browser while leaving the commit in leads to the issue on this bug. mcs, brade: could you take a look at it?

comment:5 follow-up: Changed 15 months ago by mcs

Kathy and I did not try a post to ​http://dmirrgetyojz735v.onion/d/#set-lang:en because we are not sure where to post without annoying real users of that site. But we created our own test case based on the JS code and were able to reproduce the problem.

The root cause is that the hidden iframe initially loads about:blank, and since there is no referrer at that point the #16620 code clears the iframe's window.name. We are working on a fix, which most likely will involve never clearing window.name when about:blank is being loaded (the old Torbutton JS implementation included an exception for about:blank, but we thought it was only needed to handle new windows; apparently we were wrong).

comment:6 in reply to: ↑ 5 ; follow-up: Changed 15 months ago by sky_kohai

Replying to mcs:

Kathy and I did not try a post to ​http://dmirrgetyojz735v.onion/d/#set-lang:en because we are not sure where to post without annoying real users of that site.

You should not worry about it. /d/ board is OK for testing. I also created special thread for tests. Feel free to use them if needed.
http://dmirrgetyojz735v.onion/d/res/133826.html#set-lang:en

comment:7 in reply to: ↑ 6 Changed 15 months ago by mcs

  • Keywords TorBrowserTeam201601R added
  • Status changed from assigned to needs_review

Replying to sky_kohai:

You should not worry about it. /d/ board is OK for testing. I also created special thread for tests. Feel free to use them if needed.
http://dmirrgetyojz735v.onion/d/res/133826.html#set-lang:en

Thanks! Kathy and I prepared a fix which includes a regression test:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug18168-01&id=179c9bfd56d3146c7d8f5ab6b6389010e1a570de

Tor Browser developers: please review.

comment:8 Changed 15 months ago by mcs

  • Status changed from needs_review to needs_revision

I just saw https://blog.torproject.org/blog/tor-browser-55-released#comment-153806 and tested this page:

http://www.tagindex.net/html/frame/example_t04.html

with:

network.http.sendRefererHeader = 1
network.http.sendSecureXSiteReferrer = false

Unfortunately, the patch I posted a few minutes ago does not fix that regression. Kathy and I will work on a revised patch (I assume the root cause is related but I have not debugged this new scenario yet).

comment:10 Changed 15 months ago by mcs

  • Status changed from needs_revision to needs_review

Here is a new patch:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug18168-02&id=4a83eab098f0064fa8e1ce55b729dc42e0dd4058

Please review. We now avoid clearing window.name for all frames/iframes, which more closely matches the behavior of the original JavaScript implementation from Torbutton. Kathy and I tested this with all of the sites mentioned in this ticket and we added more automated tests, but more testing is welcome.

comment:11 Changed 15 months ago by gk

  • Keywords TorBrowserTeam201602 added

Putting stuff on the radar for February.

comment:12 Changed 15 months ago by gk

  • Keywords TorBrowserTeam201602R added; TorBrowserTeam201601R removed

Carrying over review tickets.

comment:13 Changed 15 months ago by arthuredelstein

This patch looks good to me. I inspected the code, ran the unit tests and tried out the examples.

comment:14 Changed 15 months ago by gk

  • Resolution set to fixed
  • Status changed from needs_review to closed

This landed on tor-browser-38.6.0esr-5.5-1 (89226c03e3aed9c1ff3c740e0dcdcb0ac7e8e5ff) and tor-browser-38.6.0esr-6.0-1 (bc8af4dcdafd3de656bbff245eb70a0025829b90), thanks.

Last edited 15 months ago by gk (previous) (diff)

comment:15 Changed 15 months ago by gk

  • Keywords tbb-5.5-regression added
Note: See TracTickets for help on using tickets.