Closed Bug 471962 Opened 15 years ago Closed 15 years ago

When saving an inner frame as file only, the POST data of the outer page is sent to the address of the inner page

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: Paolo, Assigned: Gavin)

References

Details

(Keywords: fixed1.8.1.22, verified1.9.0.9, verified1.9.1, Whiteboard: [sg:low] [1.8.1 fix in 484621])

Attachments

(6 files, 5 obsolete files)

300 bytes, text/plain
Details
236 bytes, text/plain
Details
1.12 KB, application/zip
Details
1.88 KB, patch
bzbarsky
: review+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
19.22 KB, patch
Gavin
: review+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
1.96 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
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".
Attachment #355213 - Attachment mime type: application/octet-stream → text/html
Attachment #355213 - Attachment mime type: text/html → text/plain
Attachment #355214 - Attachment mime type: application/octet-stream → text/plain
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [sg:low]
Attached patch potential patchSplinter Review
I don't really have time to test this or drive it in at the moment, but I think this would likely fix it.
(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?
Not really, I'm busy with other things at the moment.
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.
(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!
(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!
(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...
(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.
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?
(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...
(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?
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/
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!
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.
Attachment #359923 - Flags: review?(sdwilsh)
Attachment #359923 - Flags: review?(sdwilsh)
Comment on attachment 359923 [details] [diff] [review]
Work in progress - Chrome test case

I think in general this looks right
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!
You probably want a browser-chrome test then
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?
Attachment #359923 - Attachment is obsolete: true
Attachment #361137 - Flags: review?(sdwilsh)
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.
Attached patch Browser chrome test case (obsolete) — Splinter Review
(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.
Attachment #361137 - Attachment is obsolete: true
Attachment #361163 - Flags: review?(sdwilsh)
Attachment #361137 - Flags: review?(sdwilsh)
Attachment #361163 - Flags: review?(sdwilsh)
Attachment #361163 - Flags: review?(gavin.sharp)
Attachment #361163 - Flags: review+
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.
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 :-)
(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.
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.
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?
Blocks: 300032
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.
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.
Attachment #361163 - Attachment is obsolete: true
Attachment #366856 - Flags: review?(bzbarsky)
Attachment #361163 - Flags: review?(gavin.sharp)
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 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.
Attachment #366856 - Flags: review?(bzbarsky) → review?(gavin.sharp)
(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.
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?
(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.
Attachment #366856 - Attachment is obsolete: true
Attachment #367087 - Flags: review?(gavin.sharp)
Attachment #366856 - Flags: review?(gavin.sharp)
I pushed that patch to the try server; let's see how that goes.
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.
(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.
Gavin, want to take the bug, since it's your patch?
Attachment #356119 - Flags: review?(bzbarsky)
Comment on attachment 356119 [details] [diff] [review]
potential patch

Sure, if you want to stamp it :)
Assignee: nobody → gavin.sharp
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!
Attachment #367087 - Flags: review?(gavin.sharp)
> 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 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?
Attachment #356119 - Flags: review?(bzbarsky) → review+
(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.
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 :-)
Attachment #367087 - Attachment is obsolete: true
Attachment #367481 - Flags: review?(gavin.sharp)
> 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 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.
Attachment #367481 - Flags: review?(gavin.sharp) → review+
Pushed the test and the patch:
https://hg.mozilla.org/mozilla-central/rev/ec5e307ca38d
https://hg.mozilla.org/mozilla-central/rev/637f4515b88d
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
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...
Flags: wanted1.9.0.x?
Imo we should fix this on 1.9.1 too.
Flags: blocking1.9.1?
Attachment #361163 - Flags: approval1.9.1?
Attachment #367481 - Flags: approval1.9.1?
Attachment #361163 - Flags: approval1.9.1? → approval1.9.1+
Attachment #367481 - Flags: approval1.9.1? → approval1.9.1+
Attachment #356119 - Flags: approval1.9.1?
Attachment #361163 - Flags: approval1.9.1+
Attachment #356119 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 356119 [details] [diff] [review]
potential patch

Not blocking, but a=beltzner
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.
Attachment #356119 - Flags: approval1.9.0.8?
Keywords: checkin-needed
Whiteboard: [sg:low] → [checkin-needed for 1.9.1, both attachments with a191+][sg:low]
Comment on attachment 356119 [details] [diff] [review]
potential patch

Approved for 1.9.0.8, a=dveditz
Attachment #356119 - Flags: approval1.9.0.8? → approval1.9.0.8+
mozilla/toolkit/content/contentAreaUtils.js 	1.103
Flags: wanted1.9.0.x?
Keywords: fixed1.9.0.8
Flags: blocking1.9.1? → blocking1.9.1-
(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.
Depends on: 483959
"comm-central/suite/common/contentAreaUtils.js" should be patched too
(see also bug 483959, about the test case timing out in SeaMonkey).
Probably worth a separate bug on that...  I'm somewhat sad that that's still forked.  :(
(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.
> (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?
(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.
Fixing both seamonkey issues in the other bug is fine by me.
(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.
(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.
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.
(In reply to comment #61)
> I'm somewhat sad that that's still forked.  :(

Fwiw, I filed bug 484616.
Depends on: 484621
Flags: wanted1.9.0.x+
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.
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.
(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.
(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).
Paolo, are you willing to give bug 300032 a stab too?
(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.
Any further updates on this for 1.9.0?
Al, the original testcase should work to reproduce the bug if the cache is cleared after loading the page.
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.next-
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+
(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.
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.
Yes, then that is correct. Marking it as verified then. The inner frame is definitely not getting the POST data.
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.
Group: core-security
The browser-chrome test can be checked in now, right?
Flags: in-testsuite?
Attached patch for 1.8 branchesSplinter Review
Asking bz to sign-off this as the context of this patch is slightly different. Thanks!
Attachment #374886 - Flags: review?(bzbarsky)
Attachment #374886 - Flags: approval1.8.0.next?
Attachment #374886 - Flags: review?(bzbarsky) → review+
Comment on attachment 374886 [details] [diff] [review]
for 1.8 branches

a=asac for 1.8.0
Attachment #374886 - Flags: approval1.8.1.next?
Attachment #374886 - Flags: approval1.8.0.next?
Attachment #374886 - Flags: approval1.8.0.next+
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?
Attachment #374886 - Flags: approval1.8.1.next? → approval1.8.1.next-
(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
Keywords: fixed1.8.1.22
Whiteboard: [sg:low] → [sg:low] [1.8.1 fix in 484621]
No longer blocks: 535907
You need to log in before you can comment on or make changes to this bug.