Closed Bug 661582 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla6
Tracking Status
firefox6 + fixed
firefox7 --- affected

People

(Reporter: khuey, Assigned: emk)

References

Details

Attachments

(4 files, 5 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
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.
Attached patch patch v2 (obsolete) — Splinter Review
Added a constructor to nsDOMFile and nsDOMMemoryFile.
Attachment #537160 - Attachment is obsolete: true
Attachment #537160 - Flags: review?(jonas)
Attachment #537300 - Flags: review?(jonas)
Attached patch patch v2 (obsolete) — Splinter Review
Sorry, wrong file.
Attachment #537300 - Attachment is obsolete: true
Attachment #537300 - Flags: review?(jonas)
Attachment #537302 - Flags: review?(jonas)
Attached patch patch v2 (obsolete) — Splinter Review
Sorry for the spam...
Attachment #537302 - Attachment is obsolete: true
Attachment #537302 - Flags: review?(jonas)
Attachment #537304 - Flags: review?(jonas)
Attached patch patch v2.1 (obsolete) — Splinter Review
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.
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?
Attached patch patch v3Splinter Review
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)
Attached patch Bandaid patchSplinter Review
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?
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
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
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
Resolution: FIXED → ---
http://hg.mozilla.org/mozilla-central/rev/84bbc200e89a
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
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.
Attached file 64K dummy file
Attached file testcase
This test will fail with latest Nightly/Aurora on first load.
It will succeed on reload.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
> smaller files (< 8K) will not be written in the cache
smaller files (< 8K) will not be written as separate cache files
Depends on: 669049
I've filed a new bug because the cache issue not directly related to this bug (xhr.response should return a Blob).
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.