[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #22538 [Applications/Tor Browser]: Changing circuit for page with error switches catch-all circuit instead
#22538: Changing circuit for page with error switches catch-all circuit instead
-------------------------------------------------+-------------------------
Reporter: gk | Owner: tbb-
| team
Type: defect | Status:
| needs_revision
Priority: Medium | Milestone:
Component: Applications/Tor Browser | Version:
Severity: Normal | Resolution:
Keywords: tbb-linkability, | Actual Points:
TorBrowserTeam201904 |
Parent ID: | Points:
Reviewer: | Sponsor:
-------------------------------------------------+-------------------------
Changes (by gk):
* status: needs_review => needs_revision
* keywords: tbb-linkability, TorBrowserTeam201904R => tbb-linkability,
TorBrowserTeam201904
Comment:
Replying to [comment:14 acat]:
> torbutton uses
`gBrowser.contentPrincipal.originAttributes.firstPartyDomain` to get the
active firstPartyDomain to change circuit. This is set to `about.ef2a7dd5
-93bc-417f-a698-142c3116864f.mozilla` for all about:* pages, including
about:neterror and about:certerror which are the cases here. So, if I'm
not wrong, changing the circuit when there is a network or ssl error
currently changes the circuit for all about:* pages (not the catch-all
circuit).
Yes, this behavior is new since Tor Browser 8 (we got that with
https://bugzilla.mozilla.org/show_bug.cgi?id=1300671).
> I did not find an obvious way to get the firstPartyDomain causing the
error directly from gBrowser.*. Here is a patch that gets it from the url
parameter that about:neterror and about:certerror has, which is the
original url causing it:
https://github.com/acatarineu/torbutton/commit/22538
Nice idea and I think that works for now at least. Some review comments:
1) We have been thinking about `const`ification of code as much as
possible (see: #26184) but we are not there yet. Thus, please use `let`
for now where you don't have fixed strings. (This means everywhere besides
`ABOUT_URI_FIRST_PARTY_DOMAIN`).
2) Please put the `const` at the beginning of the file using a similar
naming schema as the other `const`s we already have.
3)
{{{
+ const knownErrors = ["about:neterror", "about:certerror"];
+ const origin = gBrowser.contentPrincipal.origin || '';
+ if (isAboutFPD && knownErrors.some(x => origin.startsWith(x))) {
+ try {
}}}
I think it's not needed to determine `origin` for every domain we are
requesting but only for those that have the about-firstpartydomain. Thus,
even if the code is slightly more complex please rewrite that to something
like
{{{
+ if (isAboutFPD) {
+ let knownErrors = ["about:neterror", "about:certerror"];
+ let origin = gBrowser.contentPrincipal.origin || '';
+ if (knownErrors.some(x => origin.startsWith(x))) {
+ try {
}}}
> Another possibility could be forcing a new circuit always when there are
network or ssl errors, although I'm not sure if that's desirable.
Hm, there is the risk of burning a bunch of circuits that way which might
not help if a website is poorly configured. So, I think I'd rather avoid
that, at least for now.
> Also not sure if this is the same as
https://trac.torproject.org/projects/tor/ticket/25670, since I have only
seen this behaviour either in neterror or certerror.
I'd assume so. What would be helpful is figuring out whether we'd fix
#22513 as well with your patch. If so, we should note this in the commit
message and close that bug as well.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/22538#comment:16>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs