NoScript's XSS popup breaks circuit display in some cases

Go to https://getharvest.com and sign in. After entering your credentials and sending them you'll get a NoScript XSS popup which is a false positive in this case. More importantly for this bug, though: if you go back to your window with the Harvest URL in the URL bar, clicking on the identity box does not show your circuit anymore. The whole panel is gone. This does not change either if I select the "block your request" option. The result is, one is stuck and can't change/look at the circuit even.

Found by arma.

added TorBrowserTeam201904R component::applications/tor browser owner::tbb-team priority::medium resolution::fixed severity::normal status::closed tbb-circuit-display tbb-torbutton type::defect labels

Trac:
Cc: N/A to arma

The problem might be related to NoScript creating a new window for that. However, there are a ton of other sites where this does not result in this bug.

patch: https://github.com/acatarineu/torbutton/commit/30115

The problem seems to be common to #27749 (moved) and #25145 (moved), these should also be fixed with the patch. Currently we keep a mapping of gBrowser.selectedBrowser -> socks credentials for the circuit display, populated when there is a request. This fails for cases when the browser "moves" to a different location but there is no request, which I think is the case here and in the other bugs above. In this case, upon successful login there are several HTTP redirects, one of which triggers the XSS popup and is blocked.

The suggested fix keeps a mapping of domain -> socks credentials instead. I have seen #16936 (moved), which I think aims for a different approach, but not sure why.

Trac:
Keywords: TorBrowserTeam201904 deleted, TorBrowserTeam201904R added
Status: new to needs_review

The code changes look mostly good.

let urlOrigin = new URL(origin); can throw, though, which we don't catch anymore (given let origin = browser.contentPrincipal.origin || '';). I think we should catch that nevertheless, even if we omit log output now.

That said, consider the following scenario: in tab 1 you open https://torproject.org and in tab 2 you open https://blog.torproject.org. Checking the circuit display on both tabs should show you the same circuit (assume there are no circuit errors like timeouts etc. for the whole example). Now, select tab 2 and change the circuit for the blog by requesting a new one for that site. What should the circuit display show for each tab? With your patch it shows for both tabs the new circuit used for https://blog.torproject.org. Without your patch only tab 2 shows the new circuit while tab 1 still shows the one that got originally used to fetch its contents. I think #16936 (moved) was concerned with corner cases like that where you have a circuit in one tab and a different one for the same domain in a new tab. And, yes, I think the current behavior we have (not the one coming with your patch) regarding my example above is the one we want. Thus, this patch needs revision.

Trac:
Status: needs_review to needs_revision
Keywords: TorBrowserTeam201904R deleted, TorBrowserTeam201904 added

I think https://github.com/acatarineu/torbutton/commit/30115+1 should preserve current behaviour and still fix this and mentioned bugs (also addressed other review comments).

So, the intended behaviour for circuit display in a given tab and firstPartyDomain is to show the circuit of the last request proxied for that domain in that tab, if I understand it correctly.

Trac:
Status: needs_revision to needs_review
Keywords: TorBrowserTeam201904 deleted, TorBrowserTeam201904R added

Replying to acat:

I think https://github.com/acatarineu/torbutton/commit/30115+1 should preserve current behaviour and still fix this and mentioned bugs (also addressed other review comments).

Looks good to me. Merged to master (commit e44be49b35060f6ae17230bbc63bd617126843ee).

So, the intended behaviour for circuit display in a given tab and firstPartyDomain is to show the circuit of the last request proxied for that domain in that tab, if I understand it correctly.

Yes, that's correct.

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

closed

moved to tpo/applications/tor-browser#30115 (closed)