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.
Activity
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.
The code changes look mostly good.
let urlOrigin = new URL(origin);can throw, though, which we don't catch anymore (givenlet 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 addedI 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.
-
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