Closed
Bug 1329822
Opened 8 years ago
Closed 8 years ago
file:// documents can't use <a download=foo.txt> to set a download name/force a download
Categories
(Core :: Security: Process Sandboxing, defect)
Tracking
()
VERIFIED
FIXED
mozilla54
| Tracking | Status | |
|---|---|---|
| firefox50 | --- | unaffected |
| firefox51 | --- | unaffected |
| firefox52 | --- | unaffected |
| firefox53 | --- | disabled |
| firefox54 | --- | verified |
| firefox55 | --- | verified |
People
(Reporter: jgilbert, Assigned: haik)
References
Details
(Keywords: regression, Whiteboard: sbmc2 sbwc2 sblc3)
Attachments
(2 files)
This means scripts which generate downloads in response to user commands need to be run through a server, and do not function correctly as file:// urls.
May be related to bug 1321020.
|
||
Comment 1•8 years ago
|
||
This doesn't reproduce if you turn off the separate process for file: using the pref, so it seems fairly clear it's a regression from that change. Bob, I'm not sure the cause of this one is the same as the view-source: and/or missing file ones that are already on file, so I'm flagging this up in case it piques your interest.
|
Assignee | |
Updated•8 years ago
|
Assignee: nobody → haftandilian
|
||
Updated•8 years ago
|
status-firefox50:
--- → unaffected
status-firefox51:
--- → unaffected
status-firefox52:
--- → unaffected
status-firefox53:
--- → affected
Version: unspecified → 53 Branch
|
|
Assignee | |
Updated•8 years ago
|
Whiteboard: sbmc2 sbwc2 sblc3
|
|
Assignee | |
Comment 2•8 years ago
|
||
Still debugging, but here's my progress so far.
When clicking on the click-me link in the test case, we switch from a file content process to a web content process and display 1 2 3 in the tab.
1 <body>
2 <a onclick="DownloadText('foo.txt', ['1','2','3']);">click-me</a>
3 <script>
4 function DownloadText(filename, textArr, mimetype='text/plain') {
5 var blob = new Blob(textArr, {type: mimetype});
6 var url = URL.createObjectURL(blob);
7
8 var link = document.createElement('a');
9 link.href = url;
10 link.download = filename;
11
12 document.body.appendChild(link);
13 link.click();
14 document.body.removeChild(link);
15 }
16 </script>
17 </body>
After line 6, "url" holds
"blob:null/af8d5526-243d-8a46-95c3-669f6e41217c"
When the link is clicked we enter this code to decide if it should be loaded in the current process.
JS validatedWebRemoteType() at E10SUtils.jsm:38
JS getRemoteTypeForURI("blob:null/...", true, "file") at E10SUtils.jsm:144
JS shouldLoadURIInThisProcess() at E10SUtils.jsm: 149
JS shouldLoadURI() at E10SUtils.jsm:158
JS shouldLoadURI() at tab-content.js:708
nsDocShell::InternalLoad() at nsDocShell.cpp:10690
nsDocShell::OnLinkClickSync() at nsDocShell.cpp:14054
OnLinkClickEvent::Run() at nsDocShell.cpp:13820
In getRemoteTypeForURI(), we have tests for these URI protocol prefixes, but not "blob:".
if (aURL.startsWith("javascript:")) { ...
if (aURL.startsWith("data:")) { ...
if (aURL.startsWith("file:")) { ...
if (aURL.startsWith("about:")) { ...
if (aURL.startsWith("chrome:")) { ...
if (aURL.startsWith("moz-extension:")) { ...
These each return a remoteType which must be "file" in order to load the URI in the file content process. The existing window remote type is passed in and that is returned for data: and javascript: URI's.
For blob: URI's, we fall through to validatedWebRemoteType()
function validatedWebRemoteType(aPreferredRemoteType) {
return aPreferredRemoteType && aPreferredRemoteType.startsWith(WEB_REMOTE_TYPE)
? aPreferredRemoteType : WEB_REMOTE_TYPE;
}
which only returns WEB_REMOTE_TYPE or types that start with WEB_REMOTE_TYPE ("web").
I suspect we should treat "blob:" URI's the same way we treat "data:" URI's by opening them in the existing process type, but I'm wondering if we should instead change validatedWebRemoteType() to be catchall for any other protocol.
The "null" in the blob URL comes from this stack because the URI has no host.
frame #0 XUL`mozilla::net::nsStandardURL::Host() at nsStandardURL.h:387
frame #1 XUL`mozilla::net::nsStandardURL::GetAsciiHost() at nsStandardURL.cpp:1436
frame #2 XUL`nsContentUtils::GetASCIIOrigin() at nsContentUtils.cpp:6023
frame #3 XUL`nsContentUtils::GetASCIIOrigin() at nsContentUtils.cpp:5986
frame #4 XUL`nsHostObjectProtocolHandler::GenerateURIString() at nsHostObjectProtocolHandler.cpp:671
frame #5 XUL`nsHostObjectProtocolHandler::GenerateURIStringForBlobURL() at nsHostObjectProtocolHandler.cpp:690
frame #6 XUL`nsHostObjectProtocolHandler::AddDataEntry() at nsHostObjectProtocolHandler.cpp:517
frame #7 XUL` namespace)::CreateObjectURLInternal<moz::CreateObjectURLInternal<mozilla::dom::BlobImpl*>() at URL.cpp:52
frame #8 XUL`mozilla::dom::(anonymous namespace)::URLMainThread::CreateObjectURL() at URL.cpp:87
frame #9 XUL`mozilla::dom::URL::CreateObjectURL() at URL.cpp:1727
...
Flags: needinfo?(bobowencode)
|
||
Comment 3•8 years ago
|
||
(In reply to Haik Aftandilian [:haik] from comment #2)
Thanks for looking into this.
> I suspect we should treat "blob:" URI's the same way we treat "data:" URI's
> by opening them in the existing process type, but I'm wondering if we should
> instead change validatedWebRemoteType() to be catchall for any other
> protocol.
(Chrome also returns blob:null/... for file URIs by the way. The spec seems to say it's up to the user agent for non-http(s) URIs.)
Yes we might have to special case blob, but in that case we should possibly only allow in non-web remote type if they are blob:null.
Hmm, but what if a nested http(s) page did this, then presumably we would get the normal blob URI and we would still need this to work.
We should get someone who knows the blob code to review or possibly provide advice up front, as to how this all works.
Also, I guess we would only allow them in remote browsers (i.e. not in the parent), the same as data ones.
We shouldn't change validatedWebRemoteType, that is specifically for testing if it is a special type of web remote type.
There is only the large allocation one at the moment.
|
|
||
Comment 4•8 years ago
|
||
(In reply to Bob Owen (:bobowen) from comment #3)
> (In reply to Haik Aftandilian [:haik] from comment #2)
>
> Thanks for looking into this.
>
> > I suspect we should treat "blob:" URI's the same way we treat "data:" URI's
> > by opening them in the existing process type, but I'm wondering if we should
> > instead change validatedWebRemoteType() to be catchall for any other
> > protocol.
>
> (Chrome also returns blob:null/... for file URIs by the way. The spec seems
> to say it's up to the user agent for non-http(s) URIs.)
>
> Yes we might have to special case blob, but in that case we should possibly
> only allow in non-web remote type if they are blob:null.
> Hmm, but what if a nested http(s) page did this, then presumably we would
> get the normal blob URI and we would still need this to work.
Can you explain what you mean by "nested http page"? Out of interest, what happens right now if you have a file page with an iframe pointing to http:// ? Where do we load the page, and does it work?
It feels like blob: URIs when passed to this method should just always use the process they're currently using, like data: and javascript: .
> We should get someone who knows the blob code to review or possibly provide
> advice up front, as to how this all works.
:baku might be a good person to do this.
>
> Also, I guess we would only allow them in remote browsers (i.e. not in the
> parent), the same as data ones.
It's possible for chrome-privileged parent process things to create and point to blob: things.
|
|
||
Comment 5•8 years ago
|
||
(In reply to :Gijs from comment #4)
> Can you explain what you mean by "nested http page"? Out of interest, what
> happens right now if you have a file page with an iframe pointing to http://
> ? Where do we load the page, and does it work?
By nested http page I mean something like the example that you give.
This does work, but we have to load these in the file content process at the moment.
> > Also, I guess we would only allow them in remote browsers (i.e. not in the
> > parent), the same as data ones.
>
> It's possible for chrome-privileged parent process things to create and
> point to blob: things.
Then I would have thought this might break in a similar way (and would have done before my changes).
|
||
Comment 7•8 years ago
|
||
Blob URL are 'owned' by a global and they are broadcasted to any process when they are created and when they are unregistered. This means that, if we try to load a blob URL, after the click (on because of the click) from another process (the parent one, for instance), that blob URL doesn't exist anymore.
What you need to know, is that, those blob URLs, are still 'valid' for the current process (where the global belongs to) for 1 additional second. This extra timing is meant to be used exactly for what you want to do: if there is some <a>, or some other object, who has a reference to one of those blob URLs, everything works fine for it.
So, yes, the blob URL _must_ be loaded by the content process.
Flags: needinfo?(amarchesini)
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 9•8 years ago
|
||
Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7826d0cfb4114ce7a59bfbbac7e59d314c35f0a8
Try push with browser.tabs.remote.separateFileUriProcess=false:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=36c5f0a3baacdcf570f5b1e84a49805c74dfa066
|
||
Comment 10•8 years ago
|
||
(In reply to Haik Aftandilian [:haik] from comment #9)
> Try push:
>
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=7826d0cfb4114ce7a59bfbbac7e59d314c35f0a8
>
> Try push with browser.tabs.remote.separateFileUriProcess=false:
>
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=36c5f0a3baacdcf570f5b1e84a49805c74dfa066
Why don't you add both configurations to one test suite?
|
|
Assignee | |
Comment 11•8 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #10)
> (In reply to Haik Aftandilian [:haik] from comment #9)
> > Try push:
> >
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=7826d0cfb4114ce7a59bfbbac7e59d314c35f0a8
> >
> > Try push with browser.tabs.remote.separateFileUriProcess=false:
> >
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=36c5f0a3baacdcf570f5b1e84a49805c74dfa066
>
> Why don't you add both configurations to one test suite?
That's a great idea. I'll look into doing that. I don't have much experience writing tests so any pointers would be appreciated. I do see some blob URI tests in the tree and I'll look at those.
|
|
||
Comment 12•8 years ago
|
||
| mozreview-review | ||
Comment on attachment 8829747 [details]
Bug 1329822 - file:// documents can't use <a download=foo.txt> to set a download name/force a download;
https://reviewboard.mozilla.org/r/106710/#review107810
::: browser/modules/E10SUtils.jsm:83
(Diff revision 1)
> : aPreferredRemoteType;
> }
>
> + // Let blob URI's load in the current process
> + if (aURL.startsWith("blob:")) {
> + return aPreferredRemoteType;
You should really get a Firefox peer to review this (maybe gijs as he reviewed the file content process stuff).
I think you are changing the behaviour here for the parent/non-remote process.
We should probably do the same as for data: I think.
Given that before the file content process changes, I think we would have always loaded in the child.
(This might actually make no difference if the URI isn't valid in that process, but still best to be safe.)
Attachment #8829747 -
Flags: review?(bobowencode)
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 14•8 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8829747 [details]
Bug 1329822 - file:// documents can't use <a download=foo.txt> to set a download name/force a download;
https://reviewboard.mozilla.org/r/106710/#review107810
> You should really get a Firefox peer to review this (maybe gijs as he reviewed the file content process stuff).
>
> I think you are changing the behaviour here for the parent/non-remote process.
>
> We should probably do the same as for data: I think.
> Given that before the file content process changes, I think we would have always loaded in the child.
> (This might actually make no difference if the URI isn't valid in that process, but still best to be safe.)
Thanks. Updated the fix to treat blob: URI's like data: URI's, but I realize that this changes the behavior when the remote type is "extension". Before file content process, that would be loaded in "web". Will update the code to fix this.
| Comment hidden (mozreview-request) |
|
|
||
Comment 16•8 years ago
|
||
(In reply to Haik Aftandilian [:haik] from comment #14)
> Thanks. Updated the fix to treat blob: URI's like data: URI's, but I realize
> that this changes the behavior when the remote type is "extension". Before
> file content process, that would be loaded in "web". Will update the code to
> fix this.
I didn't realize we added a remote type for add-ons. I would have expected using "extension" to be correct. Why do blobs created with extension principals need to be loaded in the web process?
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(haftandilian)
|
||
Comment 17•8 years ago
|
||
They don't. They need to be loaded in the extension content process.
Flags: needinfo?(kmaglione+bmo)
|
|
||
Comment 18•8 years ago
|
||
| mozreview-review | ||
Comment on attachment 8829747 [details]
Bug 1329822 - file:// documents can't use <a download=foo.txt> to set a download name/force a download;
https://reviewboard.mozilla.org/r/106710/#review108054
r- as this is wrong for add-on generated blob. I think we should just always return aPreferredRemoteType for `blob:`. Is there some reason that's a bad idea that I'm missing?
Attachment #8829747 -
Flags: review?(gijskruitbosch+bugs) → review-
|
|
Assignee | |
Comment 19•8 years ago
|
||
(In reply to :Gijs from comment #18)
> Comment on attachment 8829747 [details]
> Bug 1329822 - file:// documents can't use <a download=foo.txt> to set a
> download name/force a download;
>
> https://reviewboard.mozilla.org/r/106710/#review108054
>
> r- as this is wrong for add-on generated blob. I think we should just always
> return aPreferredRemoteType for `blob:`. Is there some reason that's a bad
> idea that I'm missing?
No, that makes more sense. I was trying to preserve the original behavior. Before this fix, blob: URI's coming from EXTENSION_REMOTE_TYPE would fall through to validatedWebRemoteType() which would return WEB_REMOTE_TYPE. It sounds like that's a bug and I'll change the code to do the same thing as we do for data: URI's which will load them in the originating process as long as it isn't NOT_REMOTE.
Flags: needinfo?(haftandilian)
|
|
||
Comment 20•8 years ago
|
||
Sorry Haik, I perhaps should have made it clear when I said it should be treated the same as data:, that I meant that it was indeed broken for other remote types at the moment as well, so we wanted to change the behaviour for those.
(In reply to :Gijs from comment #18)
> r- as this is wrong for add-on generated blob. I think we should just always
> return aPreferredRemoteType for `blob:`. Is there some reason that's a bad
> idea that I'm missing?
Are you sure we don't want to return the same as for data:, i.e.:
return aPreferredRemoteType == NOT_REMOTE ? DEFAULT_REMOTE_TYPE : aPreferredRemoteType;
Isn't this what we would have done before any of the file content process changes?
Of course this could also have been a bug, if we need to load blobs at the top level in the parent process for some reason.
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
|
|
||
Comment 21•8 years ago
|
||
(In reply to Bob Owen (:bobowen) from comment #20)
> (In reply to :Gijs from comment #18)
>
> > r- as this is wrong for add-on generated blob. I think we should just always
> > return aPreferredRemoteType for `blob:`. Is there some reason that's a bad
> > idea that I'm missing?
>
> Are you sure we don't want to return the same as for data:, i.e.:
>
> return aPreferredRemoteType == NOT_REMOTE ? DEFAULT_REMOTE_TYPE :
> aPreferredRemoteType;
>
> Isn't this what we would have done before any of the file content process
> changes?
> Of course this could also have been a bug, if we need to load blobs at the
> top level in the parent process for some reason.
I'm not really very opinionated either way, though I'm not sure how often that would happen and if it would realistically break something. I imagine you could try with something like the following steps:
1. open about:newtab
2. open the web console
3. eval:
x = new Blob(["abc"], {type: "text/plain"});
a = document.createElement('a');
a.textContent = "Look at me, I'm a blob link";
a.href = URL.createObjectURL(x);
document.body.appendChild(a);
4. open the link you just appended in a new tab with the context menu / ctrl-click
I don't know if it's necessary to open those in the parent or in a non-parent explicitly, but I guess it makes sense to use a remote browser if we can just for security reasons, assuming this works (which per comment #7 sounds like it should be OK?). I'd trust :baku's judgment over mine, though.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(amarchesini)
|
|
||
Comment 22•8 years ago
|
||
Opening a blobURL in the parent process and/or in the content process works. The only issue is that, if you change process, the original document must stay alive, otherwise the blobURL is revoked everywhere. If you stay on the same process, the revoke, just for that particular process, happens with a delay.
This delay has been introduced exactly to run the code written by Gijs in comment 21.
In general, for a blobURL point of view, it's better to stay on the same process.
Flags: needinfo?(amarchesini)
|
|
||
Comment 23•8 years ago
|
||
So, this must have been broken before the file content process changes then.
Anyway, as long as we don't think that we might be introducing any new security bugs, then we should return aPreferredRemoteType for blob URIs.
|
|
Assignee | |
Comment 24•8 years ago
|
||
Given that we have no known reason to _require_ blob URI's to load in the parent when coming from the parent, I'd rather err on the side of security and load them in a sandboxed process with fewer privileges.
| Comment hidden (mozreview-request) |
|
|
||
Updated•8 years ago
|
Attachment #8829747 -
Flags: review?(amarchesini)
|
|
||
Comment 26•8 years ago
|
||
| mozreview-review | ||
Comment on attachment 8829747 [details]
Bug 1329822 - file:// documents can't use <a download=foo.txt> to set a download name/force a download;
https://reviewboard.mozilla.org/r/106710/#review109570
Well, r=me if we're sure this works. r?baku to make sure that we're sure - I still don't feel that I fully understand if there are edgecases we would break this way.
Attachment #8829747 -
Flags: review?(gijskruitbosch+bugs) → review+
|
|
||
Comment 27•8 years ago
|
||
(In reply to :Gijs from comment #26)
> Comment on attachment 8829747 [details]
> Bug 1329822 - file:// documents can't use <a download=foo.txt> to set a
> download name/force a download;
>
> https://reviewboard.mozilla.org/r/106710/#review109570
>
> Well, r=me if we're sure this works. r?baku to make sure that we're sure - I
> still don't feel that I fully understand if there are edgecases we would
> break this way.
This is what happens now I believe, so I think we should be OK (assuming it's not broken now on release).
If you:
* load about:robots
* run this in the Console:
var blob = new Blob(['2','3','5'], {type: 'text/plain'}); URL.createObjectURL(blob);
* load the resulting URL in a new tab
It loads in the content process (if e10s is enabled).
|
|
||
Comment 28•8 years ago
|
||
| mozreview-review | ||
Comment on attachment 8829747 [details]
Bug 1329822 - file:// documents can't use <a download=foo.txt> to set a download name/force a download;
https://reviewboard.mozilla.org/r/106710/#review109580
Attachment #8829747 -
Flags: review?(amarchesini) → review+
|
|
Assignee | |
Comment 29•8 years ago
|
||
(In reply to Bob Owen (:bobowen) from comment #27)
> (In reply to :Gijs from comment #26)
> > Comment on attachment 8829747 [details]
> > Bug 1329822 - file:// documents can't use <a download=foo.txt> to set a
> > download name/force a download;
> >
> > https://reviewboard.mozilla.org/r/106710/#review109570
> >
> > Well, r=me if we're sure this works. r?baku to make sure that we're sure - I
> > still don't feel that I fully understand if there are edgecases we would
> > break this way.
>
> This is what happens now I believe, so I think we should be OK (assuming
> it's not broken now on release).
>
> If you:
> * load about:robots
> * run this in the Console:
> var blob = new Blob(['2','3','5'], {type: 'text/plain'});
> URL.createObjectURL(blob);
> * load the resulting URL in a new tab
>
> It loads in the content process (if e10s is enabled).
Thanks and, to reiterate, the current fix preserves this behavior.
Keywords: checkin-needed
|
||
Comment 30•8 years ago
|
||
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9cecb0eb3c5b
file:// documents can't use <a download=foo.txt> to set a download name/force a download; r=baku,Gijs
Keywords: checkin-needed
|
||
Comment 31•8 years ago
|
||
| bugherder | ||
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
|
|
Assignee | |
Comment 33•8 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #32)
> Does this need to be uplifted to Aurora?
No, it doesn't.
The fix is only applicable when the file-content process is enabled or the extensions process is enabled. The file-content process is pref'd on for Nightly only at this time. That's browser.tabs.remote.separateFileUriProcess set in modules/libpref/init/all.js. The extensions process is new for build 54 and disabled by default.
When neither are enabled, the fix doesn't change behavior for blob: loads from chrome or content.
Flags: needinfo?(haftandilian)
|
||
Updated•8 years ago
|
|
||
Comment 34•8 years ago
|
||
Reproduced using Nightly 54.0a1 (Build ID: 20170109030209) on Windows 10 x64.
Verified as fixed using the latest Nightly 55.0a1 (Build ID: 20170330030213) and Aurora 54.0a2 (Build ID: 20170330004004, having the preference "browser.tabs.remote.separateFileUriProcess" set to true) on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12.
You need to log in
before you can comment on or make changes to this bug.
Description
•