Last Comment Bug 471962 - When saving an inner frame as file only, the POST data of the outer page is sent to the address of the inner page
: When saving an inner frame as file only, the POST data of the outer page is s...
Status: RESOLVED FIXED
[sg:low] [1.8.1 fix in 484621]
: fixed1.8.1.22, verified1.9.0.9, verified1.9.1
Product: Toolkit
Classification: Components
Component: Download Manager (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9.2a1
Assigned To: :Gavin Sharp [email: gavin@gavinsharp.com]
:
Mentors:
Depends on: 483959 484621
Blocks: 300032
  Show dependency treegraph
 
Reported: 2009-01-03 06:12 PST by :Paolo Amadini
Modified: 2009-12-19 19:24 PST (History)
9 users (show)
mbeltzner: blocking1.9.1-
dveditz: wanted1.9.0.x+
dveditz: blocking1.8.1.next-
dveditz: wanted1.8.1.x+
gavin.sharp: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test page for POW (1 of 2) (300 bytes, text/plain)
2009-01-03 06:13 PST, :Paolo Amadini
no flags Details
Test page for POW (2 of 2) (236 bytes, text/plain)
2009-01-03 06:14 PST, :Paolo Amadini
no flags Details
Unzip files into $OBJDIR/_tests/testing/mochitest, load http://localhost:8888/test-post.sjs and follow STR to verify (1.12 KB, application/zip)
2009-01-04 14:28 PST, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details
potential patch (1.88 KB, patch)
2009-01-08 18:33 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
bzbarsky: review+
mbeltzner: approval1.9.1+
dveditz: approval1.9.0.9+
Details | Diff | Splinter Review
Work in progress - Chrome test case (6.71 KB, patch)
2009-01-31 08:54 PST, :Paolo Amadini
no flags Details | Diff | Splinter Review
Work in progress - Browser chrome test case (6.94 KB, patch)
2009-02-08 03:49 PST, :Paolo Amadini
no flags Details | Diff | Splinter Review
Browser chrome test case (11.52 KB, patch)
2009-02-08 11:15 PST, :Paolo Amadini
sdwilsh: review+
Details | Diff | Splinter Review
Browser chrome test case, using internalSave directly (18.43 KB, patch)
2009-03-11 11:29 PDT, :Paolo Amadini
no flags Details | Diff | Splinter Review
Browser chrome test case, using internalSave directly (19.04 KB, patch)
2009-03-12 11:57 PDT, :Paolo Amadini
no flags Details | Diff | Splinter Review
Browser chrome test case, using internalSave directly (19.22 KB, patch)
2009-03-15 08:22 PDT, :Paolo Amadini
gavin.sharp: review+
mbeltzner: approval1.9.1+
Details | Diff | Splinter Review
for 1.8 branches (1.96 KB, patch)
2009-04-28 04:28 PDT, Alexander Sack
bzbarsky: review+
dveditz: approval1.8.1.next-
asac: approval1.8.0.next+
Details | Diff | Splinter Review

Description :Paolo Amadini 2009-01-03 06:12:29 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5 (.NET CLR 3.5.30729)

When saving an inner frame as file only (not as a complete web page), the POST data of the outer page is incorrectly sent to the address of the inner frame.

I've not tested the case explicitly, but in "toolkit/conten/contentAreaUtils.js"
it seems that the POST data is attached even if the page displayed in the frame
is from a different domain. This could cause the POST data to be available to
the inner site even if not originally intended by the author of the outer page.
Because of this concern I've marked this bug with the security flag, even if
this kind of data leak is quite unlikely in a real situation.

Reproducible: Always

Steps to Reproduce:
I tested this with the POW extension and the attached files.
1. Install POW (http://davidkellogg.com/wiki/Main_Page)
2. Copy the attached files to web server's root directory
3. Navigate to http://127.0.0.1:6670/test-post.sjs
4. Type "Outer" in the upper textbox and click "Submit Query" nearby.
5. Type "Inner" in the lower textbox and click "Submit Query" nearby.
6. Right click inside the inner frame, select "This Frame", "Save Frame As...".
7. Select "Web Page, HTML only" and save the page.
8. Open the saved page.
Actual Results:  
You will see "Inner input: Outer".

Expected Results:  
You should have seen "Inner input: Inner".
Comment 1 :Paolo Amadini 2009-01-03 06:13:46 PST
Created attachment 355213 [details]
Test page for POW (1 of 2)
Comment 2 :Paolo Amadini 2009-01-03 06:14:52 PST
Created attachment 355214 [details]
Test page for POW (2 of 2)
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2009-01-04 14:28:13 PST
Created attachment 355327 [details]
Unzip files into $OBJDIR/_tests/testing/mochitest, load http://localhost:8888/test-post.sjs and follow STR to verify
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2009-01-04 14:34:03 PST
I can verify this problem is present using the files in attachment 355327 [details] (with the slight difference that the displayed input for each frame is "inputdata=Outer" or "inputdata=Inner", due to my being lazy in converting the provided attachments to httpd.js format).

Given that you have explicit user interaction inside an iframe inside a page which is the result of a form submit including sensitive data, I think this is probably sg:low.
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-01-08 18:33:42 PST
Created attachment 356119 [details] [diff] [review]
potential patch

I don't really have time to test this or drive it in at the moment, but I think this would likely fix it.
Comment 6 Stephen Donner [:stephend] 2009-01-09 11:19:39 PST
(In reply to comment #5)
> Created an attachment (id=356119) [details]
> potential patch
> 
> I don't really have time to test this or drive it in at the moment, but I think
> this would likely fix it.

Maybe waldo can drive this in?
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2009-01-09 11:44:04 PST
Not really, I'm busy with other things at the moment.
Comment 8 :Paolo Amadini 2009-01-09 12:31:36 PST
I tested the change on Firefox 3.0.5 on Windows using an overlay, and it solves
the problem I reported in that the POST data of the inner frame is not resent
at all (the sessionHistory variable in getPostData appears to be null).

Passing aDocument to getPostData is correct in any case, and has the side effect
of allowing saveInternal to be called on a page different from the one displayed
in the current tab, which is never done in the browser - but now I can do it
safely in the Mozilla Archive Format extension I'm currently maintaining,
thank you :-)

I also tested the case where the outer page is saved, and the page in the
current tab is changed while the save file picker is displayed (since the save
dialog is modal, this can only be done from another window, like "Library").
In this case no POST data is sent, since the newly displayed page has no POST
data associated with it, and no potential data leak occurs.

Said that, I think this patch allows this bug to be closed.

If this helps, since I'm already working on this portion of the code (see bug
471875), I may be interested in providing a more complete solution over time,
allowing the right POST data to be resent or the cached version of the page to
be saved. I'm not sure this is possible on the current code, however the
information in bug 84106 comment 19 seems to hint in that direction. I'd only
need some help with validating and testing my patches, since I currently test
them outside of the normal build process.
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-01-10 16:05:40 PST
(In reply to comment #8)
> If this helps, since I'm already working on this portion of the code (see bug
> 471875), I may be interested in providing a more complete solution over time,
> allowing the right POST data to be resent or the cached version of the page to
> be saved.

Is that not the case with this patch?

Any work you'd be willing to do in this area would be more than welcome!
Comment 10 :Paolo Amadini 2009-01-11 11:54:37 PST
(In reply to comment #9)
> (In reply to comment #8)
> > I may be interested in providing a more complete solution over time,
> > allowing the right POST data to be resent or the cached version of the
> > page to be saved.
>
> Is that not the case with this patch?

What I mean is that, if you follow the steps to reproduce above, the current
patch causes the saved page to contain "Inner input: undefined". That's fine
for resolving the current bug, however what I'd have expected would be to
see "Inner input: Inner", either because the request containing the POST
data was repeated or because the page was fetched from the cache.

> Any work you'd be willing to do in this area would be more than welcome!

Thank you! I have some questions open in bug 471875, maybe you can have a
look at them when you find some time. It seems someone also requested a
review from you for the patch I attached there.

I'll proceed from there, however consider that I'll not always be able to
respond in a timely manner, since I'm working on this in my spare time.

Thank you again!
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-01-11 20:24:13 PST
(In reply to comment #10)
> What I mean is that, if you follow the steps to reproduce above, the current
> patch causes the saved page to contain "Inner input: undefined".

Ah, that's strange. I would expect it to work correctly...
Comment 12 :Paolo Amadini 2009-01-13 10:21:04 PST
(In reply to comment #11)
> (In reply to comment #10)
> > What I mean is that, if you follow the steps to reproduce above, the current
> > patch causes the saved page to contain "Inner input: undefined".
> 
> Ah, that's strange. I would expect it to work correctly...

I did some research and found this:

[nsDocShell::AddChildSHEntry]
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#2713

It seems that, for each tab, only one session history object is actually present, and is associated with the root docshell. This session history
object contains a list of session history entries. Each entry can be the root
of a tree of other entries, whose structure probably resembles that of the
root document.

When a new document is loaded in a child frame, a new root history entry is
created. The new entry is a deep copy of the previous one, with a difference
only for the child element associated with the modified frame.

I think this is why the sessionHistory variable in getPostData is null when
the document was loaded in a child docshell.
Comment 13 :Paolo Amadini 2009-01-25 14:13:13 PST
What needs to be done to get Gavin's patch committed? May I help?

If a proper test case needs to be written, what do you think is the
most appropriate testing framework? Writing an automated "browser" test
would be tricky, since at present we have no way of knowing when the
asynchronous internalSave call completes (unless we monitor the presence
and accessibility of the output file). Maybe a manual test procedure like
the one already available is enough for now?
Comment 14 Shawn Wilsher :sdwilsh 2009-01-25 14:14:40 PST
(In reply to comment #13)
> If a proper test case needs to be written, what do you think is the
> most appropriate testing framework? Writing an automated "browser" test
> would be tricky, since at present we have no way of knowing when the
> asynchronous internalSave call completes (unless we monitor the presence
> and accessibility of the output file). Maybe a manual test procedure like
> the one already available is enough for now?
Doesn't the save end up going through the download manager?  If so, you can listen for completion from the download manager...
Comment 15 :Paolo Amadini 2009-01-25 14:45:08 PST
(In reply to comment #14)
> (In reply to comment #13)
> > If a proper test case needs to be written, what do you think is the
> > most appropriate testing framework? Writing an automated "browser" test
> > would be tricky, since at present we have no way of knowing when the
> > asynchronous internalSave call completes (unless we monitor the presence
> > and accessibility of the output file). Maybe a manual test procedure like
> > the one already available is enough for now?
> Doesn't the save end up going through the download manager?  If so, you can
> listen for completion from the download manager...

I still have to study this part, however yes, I think so, assuming that the
download is the only one that is started. Or, maybe, by narrowing the search
using download properties like the source URL and target file.

Besides, what I actually did in the latest Mozilla Archive Format extension
was to overlay internalSave and modify it to allow chaining another download
listener exactly for the save being started. In that case there was also the
option not to show the save process in the download manager at all.

Do you think writing a browser test case in the former way, rather than a
preliminary manual test, at this stage is necessary and worth the effort
anyway? Consider that I'll probably work more thoroughly on this area of the
code in the next few months (only bug 471875 is blocking me right now), and
at that point we might have a more powerful internalSave anyway.

If the answer is yes, are there other tests that do a similar job that you
know of, to start from?
Comment 16 Shawn Wilsher :sdwilsh 2009-01-25 14:52:54 PST
An automated test is always better.  Machine time is cheap, and human time isn't.  Also, automated tests can run after just about every check-in, and cause a patch to get backed out when they fail, so it prevents regressions.

There are a slew of automated chrome tests here that do much of what you need to already do:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/
Comment 17 :Paolo Amadini 2009-01-25 15:03:14 PST
Thank you very much, I'll have a look at the pointers you provided.

I'll be back in a week or two, with a test I hope!
Comment 18 :Paolo Amadini 2009-01-31 08:54:49 PST
Created attachment 359923 [details] [diff] [review]
Work in progress - Chrome test case

I attached a skeleton chrome test case that loads the test page in a
browser and submits the two forms. The frame save operation and the
actual test for POST data is still missing.

I sent my work in advance in order to have some feedback as to whether
this is the correct approach, so that I may fix possible problems early.
Comment 19 Shawn Wilsher :sdwilsh 2009-02-03 15:50:07 PST
Comment on attachment 359923 [details] [diff] [review]
Work in progress - Chrome test case

I think in general this looks right
Comment 20 :Paolo Amadini 2009-02-07 02:04:23 PST
Thanks for reviewing. Now I have a little problem: I can't figure out how to
access the global functions defined in the chrome window of the "browser"
element I have in my external test window. I need that, I think, to call the
internalSave function defined in contentAreaUtils.js and start the automatic
save operation on the inner frame while specifying the save file name.

All the solutions I found use overlays or are browser chrome tests, but maybe
you know a simple way to do it from a normal chrome test using a few lines of
code!
Comment 21 Shawn Wilsher :sdwilsh 2009-02-07 10:46:12 PST
You probably want a browser-chrome test then
Comment 22 :Paolo Amadini 2009-02-08 03:49:46 PST
Created attachment 361137 [details] [diff] [review]
Work in progress - Browser chrome test case

I copied some infrastructure from toolkit/components/downloads/test/browser,
and turned the chrome test into a browser chrome test. Now I reached what
seems to be a dead end: I cannot call internalSave and specify the file name
that will be used to save the document (parameter aChosenData) while at the
same time specifying to save the page as HTML only (saveAsType different from
kSaveAsType_Complete). If I don't specify aChosenData, the native file picker
dialog is shown, and I think that the user choices in that widget cannot be
automated reliably.

Do you think it is ok to modify internalSave specifically to allow this
automated test to be executed? If so, since there could be different
approaches to achieve the same result, can you tell me how you would do it?
Comment 23 Shawn Wilsher :sdwilsh 2009-02-08 08:13:10 PST
What you'll probably have to do is register your own mock implementation of nsIFilePicker.  I don't think there are any samples of this, but there are some samples of how to do it with nsIPromptService:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/test_bug_412360.xul#69

Note - you'll want to unregister your factor after you are done testing.
Comment 24 :Paolo Amadini 2009-02-08 11:15:28 PST
Created attachment 361163 [details] [diff] [review]
Browser chrome test case

(In reply to comment #23)
> What you'll probably have to do is register your own mock implementation of
> nsIFilePicker.

Excellent! That worked. You may find attached the finished test case for review.
Comment 25 Shawn Wilsher :sdwilsh 2009-02-10 10:30:21 PST
Comment on attachment 361163 [details] [diff] [review]
Browser chrome test case

>+# The Initial Developer of the Original Code is
>+# Mozilla Corporation.
You here

>+# Contributor(s):
>+#	  Shawn Wilsher <me@shawnwilsher.com> (Original Author)
not me

>+++ b/toolkit/content/tests/browser/browser_bug471962.js
license block in this file please

>+    // Call the internal save function defined in contentAreaUtils.js, while
>+    //  replacing the file picker component with a mock implementation that
>+    //  returns the path of the temporary file 
nit: weird spacing, and trailing whitespace

>diff --git a/toolkit/content/tests/browser/bug471962_testpage_inner.sjs b/toolkit/content/tests/browser/bug471962_testpage_inner.sjs
license block please

>diff --git a/toolkit/content/tests/browser/bug471962_testpage_outer.sjs b/toolkit/content/tests/browser/bug471962_testpage_outer.sjs
ditto

This looks good overall, but I'd feel better about this if gavin took a look as well.
Comment 26 :Paolo Amadini 2009-02-11 02:46:48 PST
Properly filling in the license boilerplate is harder than it seems :-) I
omitted the boilerplate from some files on the basis that many other test
cases I saw didn't include an explicit license block to begin with, however
I agree that for new files it is far better to include it. This opens up
another round of observations:

* The only file in my patch that included the boilerplate is a modification
   of mine of "toolkit/components/downloads/test/browser/Makefile.in". Shawn
   Wilsher is listed as a contributor in that file. I didn't remove his name,
   however I agree with him that his name should be removed on the basis that
   the makefile was probably a copy of another one, and I probably removed
   all his original contributions as part of further modifying the file.
* Since I am not the original developer of the two makefiles I included in
   my patch, probably my name shouldn't appear in place of the original
   developer. I also believe that I can choose not to list my name as a
   contributor in the makefiles, since in this case I consider my
   modifications to be trivial enough and to be the unavoidable consequence
   of the addition of other files (that will include my name) in other
   portions of the source tree nearby. That is consistent with many other
   makefiles I saw, that don't include the name of any contributor even if
   they have plenty of revisions by different people in the source control
   log files.
* The main browser chrome test file is obviously my original work.
* The test pages were created by me with no explicit license, then modified
   by Jeff Walden still with no explicit license, then modified again by me.
   I guess I am the original developer and Jeff Walden shoud be listed as a
   contributor.

As for the actual contributor name line in the boilerplate, I guess the
bugmail address is the generally accepted form of identity in the Mozilla
project. In my own source files, I usually place my homepage URL (that might
also become an OpenID in the future) beside my name. I guess the latter way
of identifying me here is precluded at present, right?

Oh, and I'll fix the whitespace, of course :-)
Comment 27 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-03-10 16:58:21 PDT
(In reply to comment #26)
> I guess the latter way of identifying me here is precluded at present, right?

There are no formal rules on what is acceptable, but it's probably best to avoid using more than one line, and the goal of the notice was originally to ensure that you can be contacted should there be a desire to relicense the file at some point in the future, so using an email address would probably be best.

The rest of your points seem fine, though I wouldn't worry too much about the license boilerplate in general. I think it's probably fine to just omit them as in the current patch.
Comment 28 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-03-10 18:13:38 PDT
So I've applied that test patch and noticed a couple of things:

-it doesn't seem to fail on a current trunk build, even without my patch. Is that expected? Was it actually fixed by bug 480318?

-it causes the download manager UI to appear, and stay open. Tests should generally clean up after themselves to avoid interfering with other tests. The easiest way to fix that is probably to set the browser.download.manager.showWhenStarting pref to false during the test and then reset it, I suppose.

Otherwise it looks fine. Need to sort out the first issue, though.
Comment 29 Boris Zbarsky [:bz] (TPAC) 2009-03-10 19:52:50 PDT
On trunk, bug 84106 being fixed means that if the page is cached we'll just read it from cache and not resend any kind of POST data at all.  Can we change the test to return a "Cache-Control: no-store" header?  That should help with that little issue, I think...

I'm a little confused by this bug (and patch) in general, though.  When doing a save as "HTML only", isDocument is false:

  var isDocument = aDocument != null && saveMode != SAVEMODE_FILEONLY;

So right now we're always passing in null for the postdata, I would think (which saves us from the full effects of bug 479296).  See bug 300032 comment 1.  That also has some comments relevant to this bug.

Given the above, I admit to being a little confused about how people are seeing the wrong post data submitted.  That, and very worried, since I'm clearly missing something in the code flow here.  Any ideas what?
Comment 30 Boris Zbarsky [:bz] (TPAC) 2009-03-10 20:07:38 PDT
Oh, I see what I'm missing.  The saveMode at this point is based on the content-type only, not on what the user selects.  So for HTML it won't be FILEONLY.  Given that, we do want to keep bug 300032 separate, and fix it separately, I think.
Comment 31 :Paolo Amadini 2009-03-11 11:29:17 PDT
Created attachment 366856 [details] [diff] [review]
Browser chrome test case, using internalSave directly

The attached patch should work again with trunk. The problem was indeed caused
by the fact that the fix for bug 84106 caused the page to be retrieved from the
history cache. I solved the problem by bypassing saveDocument and calling
internalSave directly. I also tried using the "Cache-Control: no-store"
header, but it didn't work, and that is expected as for RFC 2616:

http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9.2

 "Even when this directive is associated with a response, users might
  explicitly store such a response outside of the caching system (e.g.,
  with a "Save As" dialog). History buffers MAY store such responses as
  part of their normal operation."

The license boilerplate text is now fixed, have a look at what I mean with
using the website address. If the goal is to be able to contact me easily
after two years or so, it's probably better than the bugmail, which is more
subject to change.

I also bypassed the download manager UI completely, using the same mock object
trick that is present for the file picker. It's safer and faster, I think.
Comment 32 Boris Zbarsky [:bz] (TPAC) 2009-03-11 11:39:41 PDT
Hmm.  Yeah, indeed; no-store would not affect an ONLY_FROM_CACHE load.

I'd really rather gavin reviewed this; he's much more familiar with the download manager stuff than I am.
Comment 33 :Paolo Amadini 2009-03-11 11:44:53 PDT
Comment on attachment 366856 [details] [diff] [review]
Browser chrome test case, using internalSave directly

> I'd really rather gavin reviewed this; he's much more familiar with the
> download manager stuff than I am.

I assigned the review to him.
Comment 34 :Paolo Amadini 2009-03-11 11:46:37 PDT
(In reply to comment #30)
> Oh, I see what I'm missing.  The saveMode at this point is based on the
> content-type only, not on what the user selects.  So for HTML it won't be
> FILEONLY.  Given that, we do want to keep bug 300032 separate, and fix it
> separately, I think.

Exactly, saveMode is a bitmask while saveAsType is an integer that can be set
to one of the kSaveAsType_* constants and depends on the user's choice. The
save code is not really easy to read and understand, and depends heavily on
assumptions on the supplied parameters, and that's what bug 471875 is all
about.
Comment 35 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-03-11 13:50:07 PDT
The test still seems to leave the test page loaded after the test completes. You  should probably close that tab and add a new blank one after the test is complete (or just start the load in a new tab to begin with, then close it).

Replacing the transfer implementation seems a bit more fragile than just setting the relevant pref (to avoid the showing) and using a normal download manager listener (to detect the download finishing), as in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/test/unit/test_bug_420230.js#91 .

Otherwise this looks good. Want to fix those two issues and repost the patch?
Comment 36 :Paolo Amadini 2009-03-12 11:57:26 PDT
Created attachment 367087 [details] [diff] [review]
Browser chrome test case, using internalSave directly

(In reply to comment #35)
> The test still seems to leave the test page loaded after the test completes.
> You  should probably close that tab and add a new blank one after the test is
> complete (or just start the load in a new tab to begin with, then close it).

Done: I went for the first alternative.

> Replacing the transfer implementation seems a bit more fragile than just
> setting the relevant pref (to avoid the showing) and using a normal download
> manager listener (to detect the download finishing), as in
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/test/unit/test_bug_420230.js#91
> .

I'm not sure about the merits of ether alternative, however I reverted the
code to the original implementation that used the download listener. In
addition to setting the preference to avoid opening the UI, I had to set
another one to avoid the Download Manager retaining the download in the list.

The only thing that I noticed the test leaves behind now is the default save
directory, that becomes the current user's temporary directory. I don't think
this can cause problems to any other test, however.

Another important fix is that the original XPCOM factory for the file picker
needs to be restored manually. Probably also this test should be fixed:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/test_bug_412360.xul

At present I haven't time to run the entire browser test suite to cross-check
if the patch now plays well with other tests; you'd do me a favor if you can
do the final test, thanks.
Comment 37 Boris Zbarsky [:bz] (TPAC) 2009-03-12 12:02:22 PDT
I pushed that patch to the try server; let's see how that goes.
Comment 38 Boris Zbarsky [:bz] (TPAC) 2009-03-12 14:48:25 PDT
Try server output at:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1236884552.1236892236.26327.gz

In particular, this test failed...
Comment 39 Boris Zbarsky [:bz] (TPAC) 2009-03-12 14:49:21 PDT
Ah, which makes sense since I only pushed the test and not the fix for the bug.  ;)  In any case, everything else passed, looks like.  I'll try pushing both fix and test to make sure.
Comment 41 :Paolo Amadini 2009-03-13 09:47:26 PDT
(In reply to comment #40)
> OK, new log is
> http://tinderbox.mozilla.org/showlog.cgi?tree=MozillaTry&errorparser=unittest&logfile=1236896907.1236905288.21120.gz&buildtime=1236896907&buildname=Try%20server%20linux%20unittest%20builder&fulltext=1
> and the tests all passed.  So looks good!

Thank you very much for testing this! Now only the code review remains.
Comment 42 Boris Zbarsky [:bz] (TPAC) 2009-03-13 10:35:43 PDT
Gavin, want to take the bug, since it's your patch?
Comment 43 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-03-13 10:47:01 PDT
Comment on attachment 356119 [details] [diff] [review]
potential patch

Sure, if you want to stamp it :)
Comment 44 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-03-13 12:45:31 PDT
Comment on attachment 367087 [details] [diff] [review]
Browser chrome test case, using internalSave directly

>diff --git a/toolkit/content/tests/browser/browser_bug471962.js b/toolkit/content/tests/browser/browser_bug471962.js

>+    var mockFilePicker = {

>+      show: function() {
>+        // Assume we overwrite the file if it exists
>+        return (destFile.exists() ?
>+                Ci.nsIFilePicker.returnOK :
>+                Ci.nsIFilePicker.returnReplace);

Looks like this check is reversed?

>+    onDownloadStateChange: function(aState, aDownload) {
>+      switch (aDownload.state) {
>+        case Ci.nsIDownloadManager.DOWNLOAD_FINISHED:
>+        case Ci.nsIDownloadManager.DOWNLOAD_FAILED:
>+        case Ci.nsIDownloadManager.DOWNLOAD_CANCELED:
>+        case Ci.nsIDownloadManager.DOWNLOAD_DIRTY:
>+          // The download is finished, continue the test assuming success
>+          onDownloadFinished();

The test will fail if we don't get FINISHED, right? Seems like we should handle the other cases separately and explicitly fail.

>+  function tweakDownloadManagerPrefs() {

>+  function restoreDownloadManagerPrefs() {

These can avoid having to record the previous value by just using clearUserPref and assuming the default value (a fair assumption for unit test profiles, I would hope).

>+  function onOuterSubmitted() {
>+    gBrowser.removeEventListener("DOMContentLoaded", onOuterSubmitted, false);
>+
>+    // Wait for the inner frame in the outer page to be loaded
>+    gBrowser.addEventListener("DOMContentLoaded", onInnerReady, false);
>+  }

Hmm, this relies on the DOMContentLoaded for the container frame firing first, which I suppose is probably safe, but I'm a bit leery any time we rely on event ordering this way in tests (it's often caused "intermittent" problems on the slower and overburdened automated test VMs). We'll just have to keep an eye out for sporadic failures when it lands, I guess.

>+  function onInnerReady() {
>+    gBrowser.removeEventListener("DOMContentLoaded", onInnerReady, false);
>+
>+    // Save a reference to the inner frame for later
>+    innerFrame = gBrowser.contentDocument.getElementById("innerFrame");

Add an ok(innerFrame) check, for better diagnostics should this fail for whatever reason? In fact, this could apply to a few of the other getElementById calls as well.

Looks good otherwise, I will r+ with those changes. Sorry for making you go through so many review iterations!
Comment 45 Boris Zbarsky [:bz] (TPAC) 2009-03-13 13:08:39 PDT
> Hmm, this relies on the DOMContentLoaded for the container frame firing first,

You can't rely on that.  Can this code look for load events and check the target, perhaps?
Comment 46 Boris Zbarsky [:bz] (TPAC) 2009-03-13 13:13:52 PDT
Comment on attachment 356119 [details] [diff] [review]
potential patch

This is ok as a first cut to close up the security hole, but the only reason it fixes this bug is that sessionHistory for the subframe ends up null so we return null from getPostData.

Please file a followup bug on actually getting the right postdata for the given document?
Comment 47 :Paolo Amadini 2009-03-14 12:23:28 PDT
(In reply to comment #44)
> (From update of attachment 367087 [details] [diff] [review])
> >diff --git a/toolkit/content/tests/browser/browser_bug471962.js b/toolkit/content/tests/browser/browser_bug471962.js
> 
> >+        return (destFile.exists() ?
> >+                Ci.nsIFilePicker.returnOK :
> >+                Ci.nsIFilePicker.returnReplace);
> 
> Looks like this check is reversed?

Of course. Fixed, thanks.

> >+      switch (aDownload.state) {
> 
> The test will fail if we don't get FINISHED, right? Seems like we should handle
> the other cases separately and explicitly fail.

Yes, that's probably better, to avoid the test succeeding because for example
a truncated file was generated.

> >+  function restoreDownloadManagerPrefs() {
> 
> These can avoid having to record the previous value by just using clearUserPref
> and assuming the default value (a fair assumption for unit test profiles, I
> would hope).

Good idea, this simplifies the code.

> >+  function onOuterSubmitted() {
> >+    gBrowser.removeEventListener("DOMContentLoaded", onOuterSubmitted, false);
> >+
> >+    // Wait for the inner frame in the outer page to be loaded
> >+    gBrowser.addEventListener("DOMContentLoaded", onInnerReady, false);
> >+  }
> 
> Hmm, this relies on the DOMContentLoaded for the container frame firing first

Actually it doesn't. It relies, however, on the fact that the DOMContentLoaded
event is fired two times, one for the outer document and one for the iframe.

Do you think this can be a fair assumption? If so, I'll just update the
comments to make this clear. The alternative would be to rewrite the event
handling code in a completely different way, because to explicitly check
the targets the safest way is probably to register the event listeners once,
and then keep track of the expected load state as the events are fired.

> >+  function onInnerReady() {
> >+    gBrowser.removeEventListener("DOMContentLoaded", onInnerReady, false);
> >+
> >+    // Save a reference to the inner frame for later
> >+    innerFrame = gBrowser.contentDocument.getElementById("innerFrame");
> 
> Add an ok(innerFrame) check, for better diagnostics should this fail for
> whatever reason? In fact, this could apply to a few of the other getElementById
> calls as well.

Calling the "ok" function, even without a message, would cause a line to
appear in the test output log, and the count of total and passed tests to
be incremented. Since the point of this test case is not to verify that the
DOM elements are loaded correctly, neither to meta-test that the test itself
is written correctly, probably it's better to let an exception occur if
something goes wrong. I could also wrap an ok(false, "message") in an if
block that checks for null pointers, but probably the message would say
exactly what reading the line causing the exception would have said in
any case.

> Looks good otherwise, I will r+ with those changes. Sorry for making you go
> through so many review iterations!

No problem for this! I appreciate that all of you take the time to check my
code thoroughly; it's an opportunity to learn Mozilla programming, and the
next piece of code I'll write will be of better quality.

I'll be able to provide an updated patch tomorrow.
Comment 48 :Paolo Amadini 2009-03-15 08:22:51 PDT
Created attachment 367481 [details] [diff] [review]
 Browser chrome test case, using internalSave directly

Here is the updated patch, addressing some of the concerns of comment #44.

(In reply to comment #45)
> > Hmm, this relies on the DOMContentLoaded for the container frame firing first,
> 
> You can't rely on that.  Can this code look for load events and check the
> target, perhaps?

I rewrote the code a little to make it explicit that I check that
DOMContentLoaded is fired two times. If you have a better idea, please
be a little more specific, as this would save me a lot of time figuring
out what the exact document / window / browser / frame / load / pageshow /
DOMContentLoaded / bubbling / capturing / target / originalTarget
interactions are, and which of them can be relied upon :-)
Comment 49 Boris Zbarsky [:bz] (TPAC) 2009-03-15 09:52:15 PDT
> It relies, however, on the fact that the DOMContentLoaded event is fired two
> times

That's perfectly reasonable.  With the modifications in the last patch, it looks just fine to me.
Comment 50 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-03-16 11:42:23 PDT
Comment on attachment 367481 [details] [diff] [review]
 Browser chrome test case, using internalSave directly

r=me, passes on my mac machine as well. Thanks for the test, and for having the patience to see this through! I'll get this checked in now.
Comment 51 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-03-16 11:54:53 PDT
Pushed the test and the patch:
https://hg.mozilla.org/mozilla-central/rev/ec5e307ca38d
https://hg.mozilla.org/mozilla-central/rev/637f4515b88d
Comment 52 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-03-16 12:19:15 PDT
Hmm, I guess I forgot that this bug was marked security-sensitive. The checkin comments aren't super specific, but the test reveals the general issue fairly clearly to anyone who's watching.

On the other hand, maybe there isn't much point in keeping this bug hidden...
Comment 53 Boris Zbarsky [:bz] (TPAC) 2009-03-16 12:26:13 PDT
Imo we should fix this on 1.9.1 too.
Comment 54 Mike Beltzner [:beltzner, not reading bugmail] 2009-03-17 12:12:01 PDT
Comment on attachment 356119 [details] [diff] [review]
potential patch

Not blocking, but a=beltzner
Comment 55 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-03-17 12:57:28 PDT
Comment on attachment 356119 [details] [diff] [review]
potential patch

The test doesn't work on the 1.9.0 branch because bug 462864 (test harness http server feature) hasn't landed there. 

I've run it manually using a newer test harness, though, and verified that it fails without the patch and passes with it.
Comment 56 Daniel Veditz [:dveditz] 2009-03-17 13:14:09 PDT
Comment on attachment 356119 [details] [diff] [review]
potential patch

Approved for 1.9.0.8, a=dveditz
Comment 57 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-03-17 13:21:34 PDT
mozilla/toolkit/content/contentAreaUtils.js 	1.103
Comment 59 :Paolo Amadini 2009-03-18 07:14:37 PDT
(In reply to comment #52)
> On the other hand, maybe there isn't much point in keeping this bug hidden...

I agree, and the more now that the problem is fixed. As I said in comment #1,
in any case, the data leak caused by this bug is quite unlikely in a real
situation, since it's the POST data of the outer frame that is sent to the
inner frame and not the other way around.
Comment 60 :Paolo Amadini 2009-03-18 10:45:31 PDT
"comm-central/suite/common/contentAreaUtils.js" should be patched too
(see also bug 483959, about the test case timing out in SeaMonkey).
Comment 61 Boris Zbarsky [:bz] (TPAC) 2009-03-18 10:51:01 PDT
Probably worth a separate bug on that...  I'm somewhat sad that that's still forked.  :(
Comment 62 Justin Wood (:Callek) 2009-03-18 12:27:23 PDT
(In reply to comment #61)
> Probably worth a separate bug on that...  I'm somewhat sad that that's still
> forked.  :(

I'll say skip the second bug, I'll just port it into my work with: Bug 381157. I agree though that contentAreaUtils.js still being forked is painful though.
Comment 63 :Paolo Amadini 2009-03-18 13:01:59 PDT
> (In reply to comment #61)
> > Probably worth a separate bug on that...  I'm somewhat sad that that's still
> > forked.  :(
> 
> I'll say skip the second bug, I'll just port it into my work with: Bug 381157.
> I agree though that contentAreaUtils.js still being forked is painful though.

Actually, Serge and I were thinking that, to make things simple, Gavin's
five-liner patch could be applied to the other file and attached to bug
483959, beside the fix for the test case.

There's nothing wrong in including the fix also in bug 381157, but I suppose
the change would then be difficult to backport to the release branches?

On the long term, maybe there's a way to share contentAreaUtils.js code again?
Comment 64 Justin Wood (:Callek) 2009-03-18 13:35:16 PDT
(In reply to comment #59)
> (In reply to comment #52)
> > On the other hand, maybe there isn't much point in keeping this bug hidden...
> 
> I agree, and the more now that the problem is fixed. As I said in comment #1,
> in any case, the data leak caused by this bug is quite unlikely in a real
> situation, since it's the POST data of the outer frame that is sent to the
> inner frame and not the other way around.

(In reply to comment #63)
> > (In reply to comment #61)
> > > Probably worth a separate bug on that...  I'm somewhat sad that that's still
 > 
> > I'll say skip the second bug, I'll just port it into my work with: Bug 381157.
> 
> Actually, Serge and I were thinking that, to make things simple, Gavin's
> five-liner patch could be applied to the other file and attached to bug
> 483959, beside the fix for the test case.

The right patch for the other bug is the ifdef, as discussed. I'll fix this issue in my DLMGR port. No need to bitrot the large file for such a trivial issue (that will be fixed by the next BETA of seamonkey).  If serge however wants to draft a patch for SM-stable, I'll be glad to accept it (or I can create one later), note SeaMonkey stable is on 1.8.1.
Comment 65 Boris Zbarsky [:bz] (TPAC) 2009-03-18 13:54:02 PDT
Fixing both seamonkey issues in the other bug is fine by me.
Comment 66 :Paolo Amadini 2009-03-18 14:29:52 PDT
(In reply to comment #64)
> The right patch for the other bug is the ifdef, as discussed. I'll fix this
> issue in my DLMGR port. No need to bitrot the large file for such a trivial
> issue (that will be fixed by the next BETA of seamonkey).  If serge however
> wants to draft a patch for SM-stable, I'll be glad to accept it (or I can
> create one later), note SeaMonkey stable is on 1.8.1.

This is ok for me. It seems that Gavin's patch would apply cleanly to
SeaMonkey stable, if I looked at the right branch in MXR :-)

The fix for the test on bug 483959 is also ready to be committed on trunk,
however it's likely that the test can't be backported to SeaMonkey stable,
as comment #55 suggests.

Thus, if we're interested in fixing the issue in SM-stable, we may just
attach a small patch to the other existing bug, or file a dedicated bug.
I don't have a comm-central checkout however, so I cannot do this at present.
Comment 67 Robert Kaiser 2009-03-18 16:14:24 PDT
(In reply to comment #66)
> The fix for the test on bug 483959 is also ready to be committed on trunk,
> however it's likely that the test can't be backported to SeaMonkey stable,
> as comment #55 suggests.

As said, SeaMonkey stable is 1.8.1 (comm-central is SeaMonkey trunk though it's using Mozilla 1.9.1 right now, we left out 1.9.0 and will only start having dev versions with 1.9.2 once we branch comm-central) and we have no support for running tests on stable/1.8.1, i.e. the contentAreaUtils.js fix is enough there.
Comment 68 Serge Gautherie (:sgautherie) 2009-03-21 18:09:53 PDT
I added a patch for SeaMonkey 2.0 to bug 483959.
I'm not going to port this bug fix to SeaMonkey 1.1.x.
Comment 69 Serge Gautherie (:sgautherie) 2009-03-21 18:39:34 PDT
(In reply to comment #61)
> I'm somewhat sad that that's still forked.  :(

Fwiw, I filed bug 484616.
Comment 70 Al Billings [:abillings] 2009-03-23 18:07:01 PDT
Is there any way to get this automated test checked into 1.9.0? I don't see a manual way to test this. The original case in comment 0 gives the proper behavior when run in 1.9.0.7.
Comment 71 Jeff Walden [:Waldo] (remove +bmo to email) 2009-03-23 18:26:05 PDT
That requires bug 483854 to be fixed; doable, dunno how much work it'd be.  It probably isn't that much effort, but I'm not sure, and I don't have time to try to do it.
Comment 72 :Paolo Amadini 2009-03-25 09:55:43 PDT
(In reply to comment #46)
> (From update of attachment 356119 [details] [diff] [review])
> This is ok as a first cut to close up the security hole, but the only reason it
> fixes this bug is that sessionHistory for the subframe ends up null so we
> return null from getPostData.
> 
> Please file a followup bug on actually getting the right postdata for the given
> document?

I did one more round of tests, and since this bug and bug 84106 were fixed
the only remaining problem, for subframes, is that sometimes pages generated
by POST may be retried as GET when saved locally. This now happens only when
the page is not in the cache anymore. This is now filed as bug 485196.

The situation is worse if the saved page is the top-level document, but that
is already filed as bug 479296.
Comment 73 :Paolo Amadini 2009-03-25 10:29:41 PDT
(In reply to comment #36)
> Another important fix is that the original XPCOM factory for the file picker
> needs to be restored manually. Probably also this test should be fixed:
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/test_bug_412360.xul

I filed bug 485205 to possibly fix the test using a new library function.

I looked over this bug a last time, and it seems this is the last followup
bug that needed to be filed, at present, if we don't want to file a separate
bug for getting the automated test checked into 1.9.0 (comment #70).
Comment 74 Boris Zbarsky [:bz] (TPAC) 2009-03-25 11:58:54 PDT
Paolo, are you willing to give bug 300032 a stab too?
Comment 75 :Paolo Amadini 2009-03-26 08:00:43 PDT
(In reply to comment #74)
> Paolo, are you willing to give bug 300032 a stab too?

I commented there with what I think is the very simple fix for that, however
I'd be grateful if you can also have a look at the more comprehensive patch
included in bug 471875.
Comment 76 Al Billings [:abillings] 2009-03-31 17:45:19 PDT
Any further updates on this for 1.9.0?
Comment 77 Boris Zbarsky [:bz] (TPAC) 2009-03-31 19:00:54 PDT
Al, the original testcase should work to reproduce the bug if the cache is cleared after loading the page.
Comment 78 Justin Wood (:Callek) 2009-04-01 19:33:27 PDT
dveditz, you marked wanted1.8.1+. I have patches on Bug 484621 to fix this on 1.8.1 as well, just waiting for a+
Comment 79 Al Billings [:abillings] 2009-04-02 13:53:56 PDT
(In reply to comment #77)
> Al, the original testcase should work to reproduce the bug if the cache is
> cleared after loading the page.

When I clear the cache, 1.9.0.8 (pre-patch) reproduces the bug (the saved page says "Inner input: Outer").

When I run it in the current nightly (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.9pre) Gecko/2009040204 GranParadiso/3.0.9pre), I get "Inner input: undefined" on the saved page (following the instructions and clearing the cache after I initially load the test page).

This doesn't look fixed in 1.9.0.9.
Comment 80 Boris Zbarsky [:bz] (TPAC) 2009-04-02 13:59:16 PDT
The bug is fixed if the POST data for the outer page is not sent to the inner page's URI.  With a cleared cache you're not sending the inner frame's data, because of bug 485196.  But sounds like _this_ bug is fixed.
Comment 81 Al Billings [:abillings] 2009-04-02 14:05:18 PDT
Yes, then that is correct. Marking it as verified then. The inner frame is definitely not getting the POST data.
Comment 82 Al Billings [:abillings] 2009-04-02 14:09:26 PDT
Verified for 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090402 Shiretoko/3.5b4pre as well. Same behavior.
Comment 83 Jeff Walden [:Waldo] (remove +bmo to email) 2009-04-21 14:22:16 PDT
The browser-chrome test can be checked in now, right?
Comment 84 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-04-21 14:30:53 PDT
It's already checked in...
http://mxr.mozilla.org/mozilla1.9.1/source/toolkit/content/tests/browser/browser_bug471962.js
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/browser/browser_bug471962.js

Not in on the 1.9.0 branch, but I think that depends on bug 483854.
Comment 85 Alexander Sack 2009-04-28 04:28:11 PDT
Created attachment 374886 [details] [diff] [review]
for 1.8 branches

Asking bz to sign-off this as the context of this patch is slightly different. Thanks!
Comment 86 Alexander Sack 2009-05-04 05:35:05 PDT
Comment on attachment 374886 [details] [diff] [review]
for 1.8 branches

a=asac for 1.8.0
Comment 87 Daniel Veditz [:dveditz] 2009-05-20 16:00:46 PDT
Comment on attachment 374886 [details] [diff] [review]
for 1.8 branches

This was already checked in to the 1.8.1 branch from bug 484621, right?
Comment 88 Alexander Sack 2009-06-03 04:06:15 PDT
(In reply to comment #87)
> (From update of attachment 374886 [details] [diff] [review])
> This was already checked in to the 1.8.1 branch from bug 484621, right?

yes, marking as fixed1.8.1.22 accordingly

Note You need to log in before you can comment on or make changes to this bug.