Closed Bug 789732 Opened 12 years ago Closed 10 years ago

Send HTTP header to differentiate addon updates from normal file downloads

Categories

(Toolkit :: Add-ons Manager, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: nONoNonO, Assigned: nONoNonO)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 3 obsolete files)

Some websites, like sourceforge.net, always show a download page when you select a file for download. This download page is only bypassed if you don't use a known browser, e.g. if you download the file using wget.
If the add-on updater would use a different user-agent string than the browser/mail program, it would be possible for the website to distinguish the download from a normal client request, so the download page can be skipped.
TBH, I'd consider that a bug in sites like sourceforge. 

The user agent is currently useful for easily gathering stats on addon updates, and occasionally to do the opposite of how sourceforge is redirecting - to redirect non-Gecko user agents to a page. Changing the user agent string to something that wouldn't be detected as a known browser by the likes of sourceforge would break that existing functionality (thus breaking updates for some addons that currently update fine).
Status: UNCONFIRMED → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
Well, maybe you can reconsider. You don't have to ditch the User-Agent string altogether, but maybe you can add something to it, like ' Updater' or ' Backend' or whatever to distinguish it from a normal page request. That way you can still detect OS and App from the User-Agent header and see the difference between a normal download (e.g. to download the extension from amo to install it in thunderbird) and an extension update.
If we wanted to allow sites to distinguish between different reasons for the request in some way other than by URL then I think the more appropriate way to do so would be with a custom http header in the request. Regardless of why it is making the request it is still Firefox making it so I don't think altering the UA makes sense.
SourceForge developer here. We actually look at the referer first. If no referer is set, then we check the UA to see if it's any of the known browsers. If it is, we serve up the download landing page; if not then we assume a CLI tool and send the download directly. I'm open to ideas for better ways to handle the problem, including setting a custom header.
IMO what SF does is bogus mystery meat navigation design.

The way to handle the problem is to provide and actual download link that can be followed in any HTTP compliant UA and that has to be fixed at SF. Firefox is not at fault at all.
I agree that Firefox is not at fault here. If Mozilla chooses to keep this bug closed, that's fine by me. With that said, as a site developer, it would be nice for us if Firefox had some way of identifying/distinguishing its non-interactive downloader from its browser. Think of this as an enhancement request, not a bug report.
kadams, if there's no referrer, does sf.net skip the download page? If so couldn't we just fix this by setting the Referer header blank?
(In reply to Luke Crouch [:groovecoder] from comment #7)
> kadams, if there's no referrer, does sf.net skip the download page? If so
> couldn't we just fix this by setting the Referer header blank?

There should already be no Referer header sent. According to comment 4, when that's the case it then checks the User-Agent.


(In reply to Dave Townsend (:Mossop) from comment #3)
> I think the more appropriate way
> to do so would be with a custom http header in the request

That seems reasonable. Strawman proposal:
  Moz-XPI-Update: 1

(Note: Not using an X- prefix, as per RFC6648.)

An alternative (and probably technically more appropriate) would be to set the Accept header to "application/x-xpinstall". But I don't know what that would break :\


There's also the question of whether to make the differentiation between new installs and updates (I don't think there's any privacy implication there, as its already doable via using a different URL). Not all new installs would be affected, just those initiated via InstallTrigger, which should pass through without a landing page anyway - so I'm leaning towards not making the distinction.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WONTFIX → ---
Summary: Add-on Update check should use different user-agent → Send HTTP header to differentiate addon updates from normal file downloads
I'd be thrilled with either the Moz-XPI-Update or an Accept header.
Maybe someone from amo has thoughts about differentiating normal downloads from updates and an extra http header...
When I look at the download statistics for my extension, I see that addons.mozilla.org differentiates between downloads from the add-ons manager and downloads from the website:
https://addons.mozilla.org/en-US/firefox/addon/forward/statistics/downloads/sources/?last=30

Maybe someone can explain how amo recognizes one from another and sourceforge can do it the same way, without having to wait for an extra Mox-XPI-Update header being built into core.
(In reply to Onno Ekker from comment #11)
> Maybe someone can explain how amo recognizes one from another and
> sourceforge can do it the same way, without having to wait for an extra
> Mox-XPI-Update header being built into core.

The sources there are literally src=_______ in the URLs.  Go to the homepage and hover over one of the links and you'll see the src parameter.
(In reply to Wil Clouser [:clouserw] from comment #12)
> The sources there are literally src=_______ in the URLs.  Go to the homepage
> and hover over one of the links and you'll see the src parameter.

There are a couple interesting things on AMO: the src param in the querystring and the data-hash attribute. That being said, neither seem likely culprits for how AMO is able to distinguish updater from regular browser in download stats. Good catch Onno; if someone can explain how this magic happens, we can do the same on SF.
(In reply to kadams from comment #13)
> (In reply to Wil Clouser [:clouserw] from comment #12)
> > The sources there are literally src=_______ in the URLs.  Go to the homepage
> > and hover over one of the links and you'll see the src parameter.
> 
> There are a couple interesting things on AMO: the src param in the
> querystring and the data-hash attribute. That being said, neither seem
> likely culprits for how AMO is able to distinguish updater from regular
> browser in download stats. Good catch Onno; if someone can explain how this
> magic happens, we can do the same on SF.

It uses the src variable.  The "Addons Manager" header in the statistics page comes from src=api which is added to the URL when you search for something from the interface at Tools->Add-ons in Firefox.  I'm the AMO Engineering Lead.
Attached patch AddonUpdateChecker.diff (obsolete) — Splinter Review
This patch adds a Moz-XPI-Update: 1 header to extensions update check.
Attachment #806484 - Attachment is obsolete: true
Attachment #806484 - Flags: feedback?(bmcbride)
Attachment #806580 - Flags: review?(bmcbride)
Comment on attachment 806580 [details] [diff] [review]
2nd try at patch, now hopefully according to guidelines

Review of attachment 806580 [details] [diff] [review]:
-----------------------------------------------------------------

This handles it for the request for the update manifest, but not the request for the actual XPI file. You'll need to add the same to AddonInstall.openChannel() in XPIProvider.
Attachment #806580 - Flags: review?(bmcbride) → review-
I can't figure out how to add a request header to a nsIHttpChannelInternal channel, which seems to be the type of the channel. As far as I can tell I can only add a request header to a nsIHttpChannel channel.
Calling QueryInterface(Ci.nsIHttpChannel) adds the setRequestHeader method to the channel...
Attachment #806580 - Attachment is obsolete: true
Attachment #8475918 - Flags: review?(bmcbride)
Comment on attachment 8475918 [details] [diff] [review]
Add Moz-XPI-Update header also to download request for the XPI itself

Review of attachment 8475918 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +5246,5 @@
>        this.channel.notificationCallbacks = this;
>        if (this.channel instanceof Ci.nsIHttpChannelInternal)
>          this.channel.forceAllowThirdPartyCookie = true;
> +      this.channel = this.channel.QueryInterface(Ci.nsIHttpChannel); // Bug 789732
> +      this.channel.setRequestHeader("Moz-XPI-Update", "1", true);    // " "

As the above check hints, this isn't guaranteed to be a HTTP channel - it could be FTP, for instance.

Rather than explicitly using QueryInterface, you should just be able to use an instanceof check like above, which will (IIRC) implicitly give you that interface.

Also, no need for comments mentioning the bug this was added by - the VCS history handles blame for you.
Attachment #8475918 - Flags: review?(bmcbride) → review-
Thanks for your review and comments. I hope I got it right this time... It's my first patch
Attachment #8475918 - Attachment is obsolete: true
Attachment #8478106 - Flags: review?(bmcbride)
Comment on attachment 8478106 [details] [diff] [review]
Use instanceof instead of QueryInterface

Review of attachment 8478106 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good - thanks for this!
Attachment #8478106 - Flags: review?(bmcbride) → review+
I think documentation should be added at https://developer.mozilla.org/en-US/Add-ons/Install_Manifests#updateURL and 
https://developer.mozilla.org/en-US/docs/Extension_Versioning,_Update_and_Compatibility#Update_RDF_Format (updateLink). But that should probably wait until the patch is checked in, so it is clear in which Gecko/Firefox version the functionality will be added.
Can we please run this through Try to avoid any unexpected test failures?
Keywords: checkin-needed
Try run pushed:

remote: You can view the progress of your build at the following URL:
remote:   https://tbpl.mozilla.org/?tree=Try&rev=3e770e83af31
remote: Alternatively, view them on Treeherder (experimental):
remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3e770e83af31
Try run is all green.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/0783f33c0270

Thanks for the Try push, Irving, and thanks for the patch, Onno!
Assignee: nobody → o.e.ekker
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0783f33c0270
Status: REOPENED → RESOLVED
Closed: 12 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.