Files set by mozSetFileArray inaccessible to page

RESOLVED FIXED in Firefox 38

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: darktrojan, Assigned: jld)

Tracking

unspecified
mozilla39
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox37 unaffected, firefox38 fixed, firefox39 fixed, firefox-esr38 affected)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Created attachment 8581279 [details]
test-frame.js

To get around bug 1143934, I switched to using mozSetFileArray, which works as intended, except that when I look at the File using the devtools, it's just "Object", with no accessible properties.


STR:
Load a form with a file input control.
Load the attached frame script.
Call messageManager.sendAsyncMessage("setfiles", new File("/path/to/a/file")). The value of the control should change. Check the control's files property in developer tools.
> Call messageManager.sendAsyncMessage("setfiles", new File("/path/to/a/file"))

Call where?  This is creating a File in a system-principal context, I assume?

If so, then after that it's placed in a page <input>, where the C++ code can touch it (though arguably we should fix that, because it would provide much more actionable feedback than what happens now) but the JS object visible to the web page is a COW with, as you note, no accessible properties.

The right way to use mozSetFileArray with Files you are creating is to create the File objects using |new content.File|, if you're going to put them into some element residing in content.document.

If that's not possible for some reason, we _could_ consider having mozSetFileArray clone the File instances as needed, but we'd have to audit existing consumers to make sure that doesn't break them...
Component: HTML: Form Submission → DOM
(Reporter)

Comment 2

3 years ago
(In reply to Not doing reviews right now from comment #1)
> The right way to use mozSetFileArray with Files you are creating is to
> create the File objects using |new content.File|, if you're going to put
> them into some element residing in content.document.

I can't create a File from the filesystem, as it isn't allowed on the content side.
Passing a File/Blob/TypedArray via the messageManager and creating a File from it doesn't work (I assume it's for a security reason).


Why does this test work? Is it only because the input element is wrapped?
https://mxr.mozilla.org/mozilla-central/source/dom/base/test/test_bug403852.html?force=1
(Assignee)

Comment 3

3 years ago
(In reply to Not doing reviews right now from comment #1)
> > Call messageManager.sendAsyncMessage("setfiles", new File("/path/to/a/file"))
> 
> Call where?  This is creating a File in a system-principal context, I assume?

Call that in the system-principal context of a chrome script; the message is received (and the File is reconstituted) in the system-principal context of a frame script.  So, "yes".

The frame script would need to use Cu.cloneInto on the received file before passing it to mozSetFileArray, I think.

> If that's not possible for some reason, we _could_ consider having
> mozSetFileArray clone the File instances as needed, but we'd have to audit
> existing consumers to make sure that doesn't break them...

mozSetFileArray is relatively new — it was added in bug 1068838 for use by mochitests, which see the File objects through permissive COWs and therefore didn't need to care about its wrapperness.  (mozSetFileArray is also not really documented, yet.)

Also, for what it's worth: SessionStore is affected by bug 1143934 and is going to run into the same problems as add-ons here.
> I can't create a File from the filesystem, as it isn't allowed on the content side.

Ah.  So the issue is that you're creating the File in the chrome process, then passing it to the content process (structured cloning in the process)?

You _should_ be able to do this in the content process:

  var contentExposedFile = new content.File(myChromeFile, { name: myChromeFile.name });

but I agree that this is a bit of a pain.

> and creating a File from it doesn't work (I assume it's for a security reason)

Creating how?  The File constructor does only allow doing new File(someOtherFile) for chrome code, but I'd think your frame script is chrome code, right?

> Why does this test work?

Because, as Jed mentioned, SpecialPowers cheats.  From specialpowersAPI.js:

31 // Allow stuff from this scope to be accessed from non-privileged scopes. This
32 // would crash if used outside of automation.
33 Cu.forcePermissiveCOWs();

so we get transparent wrappers instead of opaque ones.

> The frame script would need to use Cu.cloneInto on the received file before passing it
> to mozSetFileArray

Yes, that would work.  That's cleaner than using the File constructors, for sure.

That said, it sounds like mozSetFileNameArray might be something we want to restrict to test automation....
(Reporter)

Comment 5

3 years ago
(In reply to Not doing reviews right now from comment #4)
> You _should_ be able to do this in the content process:
> 
>   var contentExposedFile = new content.File(myChromeFile, { name:
> myChromeFile.name });
> 
> but I agree that this is a bit of a pain.

That's what I tried and it throws NS_ERROR_FAILURE.

> > and creating a File from it doesn't work (I assume it's for a security reason)
> 
> Creating how?  The File constructor does only allow doing new
> File(someOtherFile) for chrome code, but I'd think your frame script is
> chrome code, right?
> That's what I tried and it throws NS_ERROR_FAILURE.

Ah, because the security check is in the ctor impl, at which point we've already dropped privileges.  So yeah, Cu.cloneInto is what you want.
(Assignee)

Updated

3 years ago
Blocks: 1143934
(Assignee)

Comment 7

3 years ago
Before mozSetFileArray was added, if I understand correctly, there was no way for an HTMLInputElement to hold a cross-origin File — mozSetFileNameArray, or setting |value|, or using a file picker, would all create a File belonging to the input element's global.

Breaking that invariant with mozSetFileArray wasn't intentional.  Would there be objections to restoring it?
Assignee: nobody → jld
The main reason we don't automatically do that sort of thing normally is to avoid accidentally leaking things to pages.

But given that this API exists for the specific reason of exposing the files, I think it'd be fine if it cloned the files into the page global as needed.
(Assignee)

Comment 9

3 years ago
Created attachment 8583343 [details] [diff] [review]
bug1146116-setfile-clone-hg0.diff

The C++ change is small; most of the patch is adding a regression test.  I've verified that the test fails without the rest of the patch.
Attachment #8583343 - Flags: review?(jonas)
(Assignee)

Updated

3 years ago
status-firefox37: --- → unaffected
status-firefox38: --- → affected
status-firefox39: --- → affected
status-firefox-esr38: --- → affected
See Also: → bug 1068838
Comment on attachment 8583343 [details] [diff] [review]
bug1146116-setfile-clone-hg0.diff

You should probably bail out if GetScopeObject() returns null...
(Assignee)

Comment 11

3 years ago
Comment on attachment 8583343 [details] [diff] [review]
bug1146116-setfile-clone-hg0.diff

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

::: dom/html/test/test_bug1146116.html
@@ +23,5 @@
> +const xhr = new XMLHttpRequest;
> +xhr.open("GET", "/dynamic/getMyDirectory.sjs", false);
> +xhr.send();
> +const basePath = xhr.responseText;
> +const here = basePath + "/test_bug1146116.html"

…which doesn't work for mochitest-remote (B2G and Fennec), because the test files aren't local.

(Yes, I asked for review before the try run of the final version of the patch had finished.  Mea culpa.)
Attachment #8583343 - Flags: review-
(Assignee)

Comment 12

3 years ago
Created attachment 8584570 [details] [diff] [review]
Patch [v2]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=847f6ef25e01

The test is a little different now — it creates an empty temporary file and removes it afterwards.  (This will be useful for at least one other test, later, in bug 1143934.)

I'm not entirely sure if my check for GetScopeObject returning null is the right level of “can't happen”, but other uses seem to either MOZ_ASSERT or just not check at all, so it seems reasonable.
Attachment #8583343 - Attachment is obsolete: true
Attachment #8584570 - Flags: review?(jonas)
Comment on attachment 8584570 [details] [diff] [review]
Patch [v2]

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

::: dom/html/HTMLInputElement.cpp
@@ +2332,5 @@
>  void
>  HTMLInputElement::MozSetFileArray(const Sequence<OwningNonNull<File>>& aFiles)
>  {
> +  nsCOMPtr<nsIGlobalObject> global = OwnerDoc()->GetScopeObject();
> +  MOZ_ASSERT(global);

I don't actually know when GetScopeObject might return null. But it's probably safe to say that we shouldn't call mozSetFileArray when that's the case so this assertion is probably fine.
Attachment #8584570 - Flags: review?(jonas) → review+
(Assignee)

Comment 14

3 years ago
See comment #12 for try run.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8fff872add31
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Assignee)

Comment 17

3 years ago
Comment on attachment 8584570 [details] [diff] [review]
Patch [v2]

Approval Request Comment
[Feature/regressing bug #]: Bug 1068838
[User impact if declined]: Increased annoyance for add-on creators — if this is uplifted then mozSetFileArray will behave the same way in all versions where the property exists, instead of 38 needing special workarounds.  (See comment #0 for an example of what happens without this patch.)
[Describe test coverage new/current, TreeHerder]: Patch contains a test case; it's been baking on m-c for a few days.  Several other tests use mozSetFileArray to test forms.
[Risks and why]: Low; we're not using this in our code (outside of tests) yet, but it can be used by add-ons that manipulate HTML forms and want to be e10s-ready.[String/UUID change made/needed]: None.
Attachment #8584570 - Flags: approval-mozilla-beta?
(Assignee)

Updated

3 years ago
See Also: → bug 1149395
Comment on attachment 8584570 [details] [diff] [review]
Patch [v2]

should be part of 38 beta 2
Attachment #8584570 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.