Opened 4 years ago

Closed 4 years ago

#16427 closed enhancement (fixed)

Use internal update URL for Torbutton

Reported by: gk Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: tbb-torbutton, TorBrowserTeam201506R
Cc: brade, mcs Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by gk)

In #10682 we disabled update pings by setting the update URL to https://127.0.0.1 which caused #13129. We can do something more clever without sending pings AND causing #13129 as shown in comment:13:ticket:16200.

Child Tickets

Change History (9)

comment:1 Changed 4 years ago by gk

Description: modified (diff)

comment:2 Changed 4 years ago by gk

Summary: User internal update URL for TorbuttonUse internal update URL for Torbutton

comment:3 Changed 4 years ago by mcs

Cc: brade mcs added

comment:4 Changed 4 years ago by gk

Keywords: TorBrowserTeam201506R added; TorBrowserTeam201506 removed
Status: newneeds_review

bug_16427 (https://gitweb.torproject.org/user/gk/tor-launcher.git/commit/?h=bug_16428&id=06f135535fac7e79be9051229030d195a44c84a7) in my public TorLauncher repo has the fix. I see complaints in the console now about the update manifest not being valid XML but I think this is okay.

comment:5 in reply to:  4 Changed 4 years ago by gk

Replying to gk:

bug_16427 (https://gitweb.torproject.org/user/gk/tor-launcher.git/commit/?h=bug_16428&id=06f135535fac7e79be9051229030d195a44c84a7) in my public TorLauncher repo has the fix. I see complaints in the console now about the update manifest not being valid XML but I think this is okay.

Err, I meant https://gitweb.torproject.org/user/gk/torbutton.git/commit/?h=bug_16427 in my public Torbutton repo.

comment:6 Changed 4 years ago by mcs

r=mcs

comment:7 Changed 4 years ago by mikeperry

Is there a reason why we need the comma after the data url? Is that related to the XML warnings?

comment:8 in reply to:  7 Changed 4 years ago by mcs

Replying to mikeperry:

Is there a reason why we need the comma after the data url? Is that related to the XML warnings?

The comma is required to separate the content type from the data URL payload (which in our case is an empty string). See section 3 of https://tools.ietf.org/html/rfc2397. I think the XML warning occurs because the document (text) is empty, which means that request.responseXML is null or undefined. But we do not want a non-empty XML document, because then the code will probably get as far as trying to check for a signature and try to verify it using the updateKey... which we do not have.

So... I think this is the best we can do without patching the code inside toolkit/mozapps/extensions/internal/AddonUpdateChecker.jsm. If you want to follow the execution patch, start at the checkForUpdates() function within that file. If I was going to patch something, I would modify UpdateParser.onLoad() to accept (and ignore) a zero-length document (request.responseText == ""). I am not sure if Mozilla would like that idea or not.

comment:9 Changed 4 years ago by mikeperry

Resolution: fixed
Status: needs_reviewclosed

Ok, I went ahead and merged this. I've also noticed a lot of noise accumulating in our error console, so perhaps we should just file a new ticket to work on that.

Note: See TracTickets for help on using tickets.