Closed Bug 1367626 Opened 3 years ago Closed 5 months ago

browser.downloads.download() should allow setting Referer (and others)

Categories

(WebExtensions :: General, enhancement, P5)

54 Branch
enhancement

Tracking

(firefox70 fixed)

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: none11given, Assigned: evilpie)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [design-decision-approved][triaged])

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36

Steps to reproduce:

I understand that Referer is defined as a forbidden header and all. It's fine to prevent altering it in page scripts using XMLHttpRequest.

But WebExtensions (specifically background scripts) are privileged code. There's no good reason for the downloads API of WebExtensions to be restricted from setting a Referer header, when WebExtensions can do much more obviously dangerous stuff. In fact, hooking into onBeforeSendHeaders in the webRequest API allows me to modify any header I want, including Referer!

We need one of two things: allow setting Referer in the headers property of the options object we pass to download(), or have the request made by download() be subject to webRequest hooks.

I would be overjoyed to submit the code changes to enable the first option.
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit
Whiteboard: [design-decision-needed][triaged]
This has been added to the agenda for the May 30 WebExtensions Triage meeting at 10:30am PT.

Call in info: https://wiki.mozilla.org/Add-ons/Contribute/Triage#Details_.26_How_to_Join
Agenda: https://docs.google.com/document/d/1hKKRpGFIaAaI3G_HfPX2Nk8pCchyhUIKJB9y5sIrVV4/edit
Flags: needinfo?(mixedpuppy)
none11given: You're asking for a feature without any use case.  Why do you need this?
Flags: needinfo?(none11given)
The use case is the same as for why an onBeforeSendHeaders hook lets my set the referrer. I'm the one making the request and I ought to be able to configure it how I want.

If you want a specific one for me, I use it in my personal "single click image download" extension in order to download from websites that check your referrer as an anti-leech measure.
Flags: needinfo?(none11given)
Shane, circling back to you about this -- from the meeting notes, it looks like this was approved, P5. Does that still sound right?
Severity: normal → enhancement
Priority: -- → P5
Duplicate of this bug: 1403785
Flags: needinfo?(mixedpuppy)
Whiteboard: [design-decision-needed][triaged] → [design-decision-approved][triaged]
Duplicate of this bug: 1418636
This is not a small problem. At least on this extension ( https://github.com/harytfw/GlitterDrag ), downloads.download()'s lack of referer settings cause big problems.
I think at least the referer should be setted the same as current page url in downloads.download() rather than a blank value.
I think the referer must be setted, because the referer's importance is as same as cookies to recognize the user's on many websites.
Product: Toolkit → WebExtensions
Bulk move of bugs per https://bugzilla.mozilla.org/show_bug.cgi?id=1483958
Component: Untriaged → General

This is something that would be useful to DownThemAll! (https://github.com/downthemall/downthemall/blob/master/TODO.md) and should be really simple to implement.

Assignee: nobody → evilpies
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Echoing comment 8, if we allow Referer, we should also consider allowing Cookie, both are useful for downloading from pages that require login.
I see we copied the headers restrictions from Chrome, do we have an idea of the rational here? I would imagine if we allow Cookie you could trigger remote endpoints with the users' credentials, which might be unexpected for an extension that only has the download permission, but no hosts permission.

Oh, the users' cookie is automatically included anyway. In that case I see no problem with also allowing Referer. Even setting Cookie might be okay, because if we already provide the user's cookie, what could really be worse?

Keywords: dev-doc-needed

I see we copied the headers restrictions from Chrome, do we have an idea of the rational here?

It might be from Chrome, but the list actually references the Fetch spec. It probably made sense to copy Chrome if we're going for compat, and It was a safe choice by default, without going into details of each header.

https://fetch.spec.whatwg.org/#forbidden-header-name

About Referer specifically, I can see two subtle issues:

  • If your extension has a.com host permission, it can modify the download headers and insert b.com as Referer,
  • but If it has b.com permission, there are ways to trigger a load from a.com with b.com as a Referer (ie embedding an image).

I'm not sure which one is more surprising, but since either workaround doesn't already require both permissions, I think we can allow the header for downloads without any host permissions.

I would imagine if we allow Cookie you could trigger remote endpoints with the users' credentials, which might be unexpected for an extension that only has the download permission, but no hosts permission.

Cookies already require host permissions, so we wouldn't want to relax that. Checking permissions would be possible, but not sure it's worth it without a good use case that isn't covered by just using the cookies api.

Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f5b29024aa67
browser.downloads.download() should allow setting Referer. r=zombie
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

Hello,

Will this fix require manual validation? If yes, please provide some steps to reproduce in order to correctly test it and also, please set the "qe-verify+" flag. Otherwise, could the "qe-verify-" flag be added? Thanks!

Flags: needinfo?(evilpies)

No thank you. I am sure the automated test is enough.

Flags: needinfo?(evilpies)

Thank you. Setting the "qe-verify-" flag as per the above comment.

Flags: qe-verify-

Hi Tom, looking for some advice on the documentation changes needed. Is this as simple as adding:

referrer | optional
A string representing the URL of the document requesting the download. Use this parameter where the site you are downloading from expects requests from specific pages.

to downloads.download?

With the browser compatibility details updated to show support from Firefox 70?

Thanks!

Flags: needinfo?(evilpies)

No, Referer can be used inside with the headers field now. So the sentence

restricted to those allowed by XMLHttpRequest.

needs to be adjusted like

restricted to those allowed by XMLHttpRequest except for Referer.

Flags: needinfo?(evilpies)

Not "except" really... That, to me, implies Referer is not allowed.

So maybe something like this, to avoid ambiguity.

An array of objects representing extra HTTP headers to send with the request if the URL uses the HTTP[s] protocol. Each header is represented as a dictionary object containing the keys name and either value or binaryValue. Allowed are the same headers that are allowed by XMLHttpRequest. Firefox 70 and later additionally allows the Referer header. Any attempt to use any header that is not allowed will throw an error.

(Changes in italics)

And for the compat table, it should be noted that Referer is an allowed header since Firefox 70, but not allowed in other browsers (yet).

hmm... The XMLHttpRequest link should better be replaced by a link to https://developer.mozilla.org/en-US/docs/Glossary/Forbidden_header_name

And that is a list of blacklisted headers not a list of whitelisted ones.

So maybe

An array of objects representing extra HTTP headers to send with the request if the URL uses the HTTP[s] protocol. Each header is represented as a dictionary object containing the keys name and either value or binaryValue. Forbidden are the same headers that are forbidden by XMLHttpRequest and fetch, except Firefox 70 and later also allows the use of the Referer header. Any attempt to use any forbidden header will throw an error.

Many thanks for the feedback and suggestions. I've made the changes to downloads.download() as well as the browser compatibility table.

You need to log in before you can comment on or make changes to this bug.