Opened 7 weeks ago

Closed 5 weeks ago

Last modified 5 weeks ago

#31562 closed defect (fixed)

The circuit display is not visible on error pages in Tor Browser based on ESR68

Reported by: gk Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ff68-esr, TorBrowserTeam201909R, tbb-9.0-must-alpha
Cc: Actual Points: 0.5
Parent ID: Points: 1
Reviewer: Sponsor:

Description

https://expired.badssl.com/ shows a circuit display in 9.0a4 but not in recent nightlies.

Child Tickets

Change History (11)

comment:1 Changed 6 weeks ago by gk

Moving must-alpha tickets to September.

comment:2 Changed 6 weeks ago by pili

Points: 1

comment:3 Changed 6 weeks ago by acat

Status: newneeds_review

Fix in https://github.com/acatarineu/torbutton/commit/31562. contentPrincipal.origin does not contain the original URI causing the error anymore, so switched to contentPrincipal.URI.spec.

comment:4 Changed 6 weeks ago by acat

Keywords: TorBrowserTeam201909R added; TorBrowserTeam201909 removed

comment:5 Changed 6 weeks ago by gk

Keywords: TorBrowserTeam201909 added; TorBrowserTeam201909R removed
Status: needs_reviewneeds_revision

Are we sure browser.contentPrincipal.URI can't be null? It seems the Firefox folks think otherwise (see: https://searchfox.org/mozilla-esr68/source/browser/base/content/browser.js#1092 and the comment at
https://searchfox.org/mozilla-esr60/source/browser/base/content/browser.js#800). We should adapt our code I think.

Are we sure origin was just syntactic sugar for URI.spec? We should be certain that we don't introduce a subtle bug here. I wonder what the new siteOrigin (https://bugzilla.mozilla.org/show_bug.cgi?id=1485177) would give us here. Maybe that's what we actually want?

Last edited 5 weeks ago by gk (previous) (diff)

comment:6 Changed 6 weeks ago by acat

Keywords: TorBrowserTeam201909R added; TorBrowserTeam201909 removed
Status: needs_revisionneeds_review

Thanks for the review.

It seems the Firefox folks think otherwise (see: ​https://searchfox.org/mozilla-esr68/source/browser/base/content/browser.js#1092 and the comment at

https://searchfox.org/mozilla-esr60/source/browser/base/content/browser.js#800).

I'm not sure if the first check

if (firstPartyDomain === k_tb_about_uri_first_party_domain) {

would have ruled out the possibility of null URIs, but you're right, we should check that in general.

That code in the first link pointed me to a way to get the original URI that caused the error in (I think) a slightly less-hacky way: https://searchfox.org/mozilla-esr68/source/browser/base/content/browser-safebrowsing.js#10. It seems that when there is an error gBrowser.currentURI keeps the original URL, and gBrowser.selectedBrowser.documentURI (or the contentPrincipal) has the about:* one (gBrowser.currentURI is an alias of gBrowser.selectedBrowser.currentURI). This would have worked in esr60 too.

So here is a patch with this different approach: https://github.com/acatarineu/torbutton/commit/31562+1. I also removed the check with the hardcoded k_tb_about_uri_first_party_domain.

comment:7 Changed 5 weeks ago by gk

Keywords: TorBrowserTeam201909 added; TorBrowserTeam201909R removed
Status: needs_reviewneeds_revision

Looks almost good now, just a nit: please remove the trailing whitespace after

+  // Bug 31562: For neterror or certerror, get the original URL from

comment:9 Changed 5 weeks ago by acat

Keywords: TorBrowserTeam201909R added; TorBrowserTeam201909 removed
Status: needs_revisionneeds_review

comment:10 Changed 5 weeks ago by gk

Resolution: fixed
Status: needs_reviewclosed

Looks good. Cherry-picked to master (commit bb975009a65ab9039641d03b11023fe6c52f5b7a)

comment:11 Changed 5 weeks ago by acat

Actual Points: 0.5
Note: See TracTickets for help on using tickets.