nsWebBrowserPersist::SaveURIInternal creates a channel out of thin air

RESOLVED FIXED in Firefox 18

Status

defect
RESOLVED FIXED
7 years ago
2 months ago

People

(Reporter: jdm, Assigned: jdm)

Tracking

({addon-compat, dev-doc-needed})

Dependency tree / graph

Firefox Tracking Flags

(firefox18+ fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

7 years ago
http://mxr.mozilla.org/mozilla-central/source/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp#1219

At minimum, this means that Save Image As in private browsing mode can end up storing the image in the disk cache, which is very bad. I don't the full extent to which nsIWebBrowserPersist is used elsewhere in the code.

Updated

7 years ago
Assignee: nobody → josh
Assignee

Comment 1

7 years ago
Boris, I'm looking at adding an nsILoadContext parameter to nsIWebBrowserPersist::saveURI (documented thoroughly, I might add). Do you have any opinions on this? The results at https://mxr.mozilla.org/addons/search?string=saveuri show that addons appear to be using this method quite a bit :(
Assignee

Updated

7 years ago
Keywords: addon-compat
Assignee

Updated

7 years ago
Depends on: 795065
Assignee

Comment 2

7 years ago
I'll throw some other reviewers at the consumer bits. I'd like you to review all the WebBrowserPersist stuff, and in particular note my XXX comment. I've walked through the SaveDocument code several times now, and I still can't figure out how StartUpload fits into things.
Attachment #665783 - Flags: review?(bzbarsky)
Assignee

Updated

7 years ago
Blocks: 794606
Assignee

Comment 3

7 years ago
Looping in Neil for the Seamonkey perspective and heads up.
(In reply to Josh Matthews from comment #3)
> Looping in Neil for the Seamonkey perspective and heads up.
SeaMonkey (indirectly) uses StartUpload to upload files to FTP. Also I think Composer/Kompozer/NVu etc. might use nsWebBrowserPersist to upload documents (including associated resources) to FTP, but I'm not familiar with that code (perhaps Kaze might be able to help you there). Either way I'm not sure that private browsing applies here, as we're not downloading anything.

As for SaveDocument, my guess is that you want to cache the privacy flag on a boolean member so that you can access it in EnumPersistURIs.
Comment on attachment 665783 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.

Comment 4 is spot on.

Also, need to change the IID here, and is there a good reason to make the new method noscript?

r=me modulo all that.
Attachment #665783 - Flags: review?(bzbarsky) → review+
Assignee

Comment 6

7 years ago
This should not require the download changes.
No longer depends on: 795065
Assignee

Comment 8

7 years ago
Comment on attachment 665783 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.

Ehsan, can you review the browser-y stuff please?
Attachment #665783 - Flags: review?(ehsan)
Assignee

Comment 9

7 years ago
Comment on attachment 665783 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.

Dave, the devtools stuff?
Attachment #665783 - Flags: review?(dcamp)
Assignee

Comment 10

7 years ago
Comment on attachment 665783 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.

Olli, the content/ stuff?
Attachment #665783 - Flags: review?(bugs)
Assignee

Comment 11

7 years ago
Comment on attachment 665783 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.

Mark, the mobile/ stuff?
Attachment #665783 - Flags: review?(mark.finkle)
Assignee

Comment 12

7 years ago
Shoot, I need to do those requests over again since I forgot the rebased version.
Assignee

Updated

7 years ago
Attachment #665783 - Attachment is obsolete: true
Attachment #665783 - Flags: review?(mark.finkle)
Attachment #665783 - Flags: review?(ehsan)
Attachment #665783 - Flags: review?(dcamp)
Attachment #665783 - Flags: review?(bugs)
Assignee

Updated

7 years ago
Attachment #667636 - Attachment is obsolete: true
Assignee

Comment 14

7 years ago
Comment on attachment 667676 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.

Ehsan, browser/, toolkit/content
Smaug, content/
Joe, devtools/
Mark, mobile/
Marco, downloads/
Blair, extensions/
Rob, widget/
Attachment #667676 - Flags: review?(roc)
Attachment #667676 - Flags: review?(mark.finkle)
Attachment #667676 - Flags: review?(mak77)
Attachment #667676 - Flags: review?(jwalker)
Attachment #667676 - Flags: review?(ehsan)
Attachment #667676 - Flags: review?(bugs)
Attachment #667676 - Flags: review?(bmcbride)
Comment on attachment 667676 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.

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

I don't know why OS/2 needs special love here, but whatever...
Attachment #667676 - Flags: review?(roc) → review+
Attachment #667676 - Flags: review?(bmcbride) → review+

Updated

7 years ago
Attachment #667676 - Flags: review?(ehsan) → review+
Attachment #667676 - Flags: review?(jwalker) → review+
Optimizer, I r+ed this already because it looks OK to me and because it's a bug that touches lots of parts and therefore hard to get agreement on. Could you take a look and comment that you're OK, or if not, we can either file a follow-up or ask for a change (if we're quick).
Comment on attachment 667676 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.


>  * Interface for persisting DOM documents and URIs to local or remote storage.
>  */
>-[scriptable, uuid(dd4e0a6a-210f-419a-ad85-40e8543b9465)]
>+[scriptable, uuid(35c1f231-582b-4315-a26c-a1227e3539b4)]
> interface nsIWebBrowserPersist : nsICancelable
> {
>   /** No special persistence behaviour. */
>   const unsigned long PERSIST_FLAGS_NONE = 0;
>   /** Use cached data if present (skipping validation), else load from network */
>   const unsigned long PERSIST_FLAGS_FROM_CACHE = 1;
>   /** Bypass the cached data. */
>   const unsigned long PERSIST_FLAGS_BYPASS_CACHE = 2;
>@@ -125,27 +126,41 @@ interface nsIWebBrowserPersist : nsICanc
>    *                   <CODE>nullptr</CODE>.
>    * @param aPostData  Post data to pass with an HTTP request or
>    *                   <CODE>nullptr</CODE>.
>    * @param aExtraHeaders Additional headers to supply with an HTTP request
>    *                   or <CODE>nullptr</CODE>.
>    * @param aFile      Target file. This may be a nsIFile object or an
>    *                   nsIURI object with a file scheme or a scheme that
>    *                   supports uploading (e.g. ftp).
>+   * @param aPrivacyContext A context from which the privacy status of this
>+   *                   save operation can be determined. Must only be null
>+   *                   in situations in which no such context is available
>+   *                   (eg. the operation has no logical association with any
>+   *                   window or document)
>    *
>    * @see nsIFile
>    * @see nsIURI
>    * @see nsIInputStream
>    *
>    * @return NS_OK Operation has been started.
>    * @return NS_ERROR_INVALID_ARG One or more arguments was invalid.
>    */
>   void saveURI(in nsIURI aURI, in nsISupports aCacheKey,
>       in nsIURI aReferrer, in nsIInputStream aPostData,
>-      in string aExtraHeaders, in nsISupports aFile);
>+      in string aExtraHeaders, in nsISupports aFile,
>+      in nsILoadContext aPrivacyContext);
>+
>+  /**
>+   * @see saveURI
>+   */
>+  void savePrivacyAwareURI(in nsIURI aURI, in nsISupports aCacheKey,
>+      in nsIURI aReferrer, in nsIInputStream aPostData,
>+      in string aExtraHeaders, in nsISupports aFile,
>+      in boolean aIsPrivate);
This needs better documentation. I have no idea what
aIsPrivate means in this context. (Had to read the method implementation to understand it.)
Attachment #667676 - Flags: review?(bugs) → review+
Assignee

Comment 19

7 years ago
Notify comm-central folks about this upcoming change.
Keywords: dev-doc-needed
Attachment #667676 - Flags: review?(mark.finkle) → review+

Comment 20

7 years ago
Comment on attachment 667676 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.

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

r=me on the rest of the toolkit parts.
Attachment #667676 - Flags: review?(mak77) → review+
(In reply to Josh Matthews [:jdm] from comment #19)
> Notify comm-central folks about this upcoming change.

Thanks for the heads up, there's only one instance for Thunderbird which I'll fix when this lands, there's SeaMonkey changes required though, so cc'ing some SM folk.
(In reply to Mark Banner (:standard8) from comment #23)
> (In reply to Josh Matthews [:jdm] from comment #19)
> > Notify comm-central folks about this upcoming change.
> 
> Thanks for the heads up, there's only one instance for Thunderbird which
> I'll fix when this lands, there's SeaMonkey changes required though, so
> cc'ing some SM folk.

SeaMonkey also does not have any notion of Private Browsing [yet] so iirc what you said to me before we'd only need |null| passed as an extra param, so it would be helpful if you patch us when you patch TB. If not "one of us" can fix soonish.
https://hg.mozilla.org/mozilla-central/rev/ff5e9f36d02e
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Updated

7 years ago
Blocks: 798685
Depends on: 798978

Updated

7 years ago
Depends on: 815001
Depends on: 820522

Updated

2 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.