nsWebBrowserPersist::SaveURIInternal creates a channel out of thin air

RESOLVED FIXED in Firefox 18

Status

()

Core
Embedding: APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jdm, Assigned: jdm)

Tracking

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

Trunk
mozilla18
x86_64
Linux
addon-compat, dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox18+ fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 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.
Assignee: nobody → josh
tracking-firefox18: --- → +
(Assignee)

Comment 1

5 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

5 years ago
Keywords: addon-compat
(Assignee)

Updated

5 years ago
Depends on: 795065
(Assignee)

Comment 2

5 years ago
Created attachment 665783 [details] [diff] [review]
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.
Attachment #665783 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Blocks: 794606
(Assignee)

Comment 3

5 years ago
Looping in Neil for the Seamonkey perspective and heads up.

Comment 4

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

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

Comment 7

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=acd948d87ca6
(Assignee)

Comment 8

5 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

5 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

5 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

5 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

5 years ago
Created attachment 667636 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.

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

Updated

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

Comment 13

5 years ago
Created attachment 667676 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.
(Assignee)

Updated

5 years ago
Attachment #667636 - Attachment is obsolete: true
(Assignee)

Comment 14

5 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+
(Assignee)

Comment 16

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=6e52a2333601
Attachment #667676 - Flags: review?(bmcbride) → review+
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

5 years ago
Notify comm-central folks about this upcoming change.
Keywords: dev-doc-needed
Attachment #667676 - Flags: review?(mark.finkle) → review+
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+
(Assignee)

Comment 21

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/e94dbc151121
(Assignee)

Comment 22

5 years ago
Backed out. Relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/ff5e9f36d02e.
(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: 5 years ago
status-firefox18: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Updated

5 years ago
Blocks: 798685
Depends on: 798978

Updated

5 years ago
Depends on: 815001
Depends on: 820522
You need to log in before you can comment on or make changes to this bug.