794602 - nsWebBrowserPersist::SaveURIInternal creates a channel out of thin air

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Josh Matthews [:jdm]]

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.

Comment 1

[Josh Matthews [:jdm]]

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 :(

Comment 2

[Josh Matthews [:jdm]]

Attachment: Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.

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.

Comment 3

[Josh Matthews [:jdm]]

Looping in Neil for the Seamonkey perspective and heads up.

Comment 4

[[e-mail]]

(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 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Josh Matthews [:jdm]]

This should not require the download changes.

Comment 7

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=acd948d87ca6

Comment 8

[Josh Matthews [:jdm]]

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?

Comment 9

[Josh Matthews [:jdm]]

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

Dave, the devtools stuff?

Comment 10

[Josh Matthews [:jdm]]

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

Olli, the content/ stuff?

Comment 11

[Josh Matthews [:jdm]]

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

Mark, the mobile/ stuff?

Comment 12

[Josh Matthews [:jdm]]

Attachment: Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.

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

Comment 13

[Josh Matthews [:jdm]]

Attachment: Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.

Comment 14

[Josh Matthews [:jdm]]

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/

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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...

Comment 16

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=6e52a2333601

Comment 17

[Joe Walker [:jwalker]]

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 18

[Olli Pettay [:smaug][[e-mail]]]

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.)

Comment 19

[Josh Matthews [:jdm]]

Notify comm-central folks about this upcoming change.

Comment 20

[(no longer active)]

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.

Comment 21

[Josh Matthews [:jdm]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e94dbc151121

Comment 22

[Josh Matthews [:jdm]]

Backed out. Relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/ff5e9f36d02e.

Comment 23

[Mark Banner (:standard8)]

(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.

Comment 24

[Justin Wood (:Callek)]

(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.

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ff5e9f36d02e