Note: There are a few cases of duplicates in user autocompletion which are being worked on.

xhr.response should return a Blob, not a File, when appropriate

RESOLVED FIXED in Firefox 6

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: khuey, Assigned: emk)

Tracking

unspecified
mozilla6
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox6+ fixed, firefox7 affected)

Details

Attachments

(4 attachments, 5 obsolete attachments)

xhr.responseType = "blob";
xhr.response instanceof Blob; // Should be true, is true
xhr.response instanceof File; // Should be false, is true

We need to set the mIsFullFile flag on nsDOMFile to false.  This might require a slightly different constructor.
This also means we leak the filename from the cache to the web page.  I don't think that's a security/privacy problem, but it's not ideal.
This is a *potential* security/privacy leak. I can't think of any attacks either, but it's making me a bit nervous and should be easy to fix.

Masatoshi: Do you think you'd have time to fix this soonish? If not I should be able to.
tracking-firefox6: --- → ?
tracking-firefox6: ? → +
(Assignee)

Comment 3

6 years ago
Created attachment 537160 [details] [diff] [review]
patch
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #537160 - Flags: review?(jonas)
Comment on attachment 537160 [details] [diff] [review]
patch

It seems to me that a better solution would be to introduce a new constructor which sets the mIsFile flag to false. Both to nsDOMMemoryFile and nsDOMFile.
(Assignee)

Comment 5

6 years ago
Created attachment 537300 [details] [diff] [review]
patch v2

Added a constructor to nsDOMFile and nsDOMMemoryFile.
Attachment #537160 - Attachment is obsolete: true
Attachment #537160 - Flags: review?(jonas)
Attachment #537300 - Flags: review?(jonas)
(Assignee)

Comment 6

6 years ago
Created attachment 537302 [details] [diff] [review]
patch v2

Sorry, wrong file.
Attachment #537300 - Attachment is obsolete: true
Attachment #537300 - Flags: review?(jonas)
Attachment #537302 - Flags: review?(jonas)
(Assignee)

Comment 7

6 years ago
Created attachment 537304 [details] [diff] [review]
patch v2

Sorry for the spam...
Attachment #537302 - Attachment is obsolete: true
Attachment #537302 - Flags: review?(jonas)
Attachment #537304 - Flags: review?(jonas)
(Assignee)

Comment 8

6 years ago
Created attachment 537312 [details] [diff] [review]
patch v2.1

Unreferencing mFile after bResponseBlob has been created.
Attachment #537304 - Attachment is obsolete: true
Attachment #537304 - Flags: review?(jonas)
Attachment #537312 - Flags: review?(jonas)
Comment on attachment 537312 [details] [diff] [review]
patch v2.1

>diff --git a/content/base/public/nsDOMFile.h b/content/base/public/nsDOMFile.h
>--- a/content/base/public/nsDOMFile.h
>+++ b/content/base/public/nsDOMFile.h
>@@ -79,16 +79,29 @@ public:
>       mIsFullFile(true)
>   {}
> 
>   nsDOMFile(nsIFile *aFile)
>     : mFile(aFile),
>       mIsFullFile(true)
>   {}
> 
>+  nsDOMFile(nsIFile *aFile, PRUint64 aStart, PRUint64 aLength,
>+            const nsAString& aContentType)
>+    : mFile(aFile),
>+      mStart(aStart),
>+      mLength(aLength),
>+      mContentType(aContentType),
>+      mIsFullFile(false)
>+  {

I think you can do this even simpler. Give the current

nsDOMFile(nsIFile *aFile, const nsAString& aContentType)

constructor another bool argument for which defaults to true and set mIsFullFile to the value of that argument.
(Assignee)

Comment 10

6 years ago
I had to give a length explicitly for non-full file blobs, otherwise nsDOMFile::GetSize will not work correctly.
I don't think it's a good idea calling nsIFile::GetFileSize inside the constructor. Is it better to rewrite nsDOMFile::GetSize to support constructing non-full file blobs without a length?
(Assignee)

Comment 11

6 years ago
Created attachment 540335 [details] [diff] [review]
patch v3

Added a boolean flag to indicate whether the filename sould be hidden or not.
Attachment #537312 - Attachment is obsolete: true
Attachment #537312 - Flags: review?(jonas)
Attachment #540335 - Flags: review?(jonas)
Created attachment 543234 [details] [diff] [review]
Bandaid patch

This is hacky, but we need a bandaid for 6.
Attachment #543234 - Flags: review?(jonas)
Comment on attachment 543234 [details] [diff] [review]
Bandaid patch

Drivers, this patch fixes a correctness issue and a potential security/privacy leak (of a cache filename, N.B. comments 1 and 2) with a new feature introduced in Firefox 6.  This patch is a bandaid because the "correct" fix here requires significant refactoring that is out of scope for Aurora.  I believe the risk in this patch is low (and in well-tested code).
Attachment #543234 - Flags: approval-mozilla-aurora?
Attachment #543234 - Flags: review?(jonas) → review+

Comment 14

6 years ago
Comment on attachment 543234 [details] [diff] [review]
Bandaid patch

Approved for releases/mozilla-aurora. Please land asap before 2011-07-05 @ 9:00 am PDT.
Attachment #543234 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
http://hg.mozilla.org/mozilla-central/rev/42f2aca1fde9
http://hg.mozilla.org/releases/mozilla-aurora/rev/4afee8a13b96

I think the real fix here is to refactor nsDOMFile to be cleaner, which Jonas has plans to do, so I'm going to close this out.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
status-firefox6: --- → fixed
Target Milestone: --- → mozilla6
(Assignee)

Comment 16

6 years ago
Comment on attachment 540335 [details] [diff] [review]
patch v3

Canceling review per comment #15.
Attachment #540335 - Flags: review?(jonas)
Backed out from mozilla-central during investigation of Android browser-chrome test failures:
http://hg.mozilla.org/mozilla-central/rev/00bb08972e46
Status: RESOLVED → REOPENED
status-firefox7: --- → affected
Resolution: FIXED → ---
http://hg.mozilla.org/mozilla-central/rev/84bbc200e89a
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 19

6 years ago
Comment on attachment 543234 [details] [diff] [review]
Bandaid patch

This patch doesn't work for larger remote file because the cache file size is 0 bytes at the time of CreateResponseBlob unless the file is already stored in the cache. And the blob size will be "frozen" by mozSlice call.
It's the reason I moved |new nsDOMFile| call from CreateResponseBlob in my v2 patch.
Mochitest couldn't catch this because smaller files (< 8K) will not be written in the cache even if cacheAsFile flag is set.
Sorry, I should have added a test to catch this case.
(Assignee)

Comment 20

6 years ago
Created attachment 543658 [details]
64K dummy file
(Assignee)

Comment 21

6 years ago
Created attachment 543659 [details]
testcase

This test will fail with latest Nightly/Aurora on first load.
It will succeed on reload.
(Assignee)

Updated

6 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 22

6 years ago
> smaller files (< 8K) will not be written in the cache
smaller files (< 8K) will not be written as separate cache files
(Assignee)

Updated

6 years ago
Depends on: 669049
(Assignee)

Comment 23

6 years ago
I've filed a new bug because the cache issue not directly related to this bug (xhr.response should return a Blob).
(Assignee)

Updated

6 years ago
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.