479296 - Saving POST result page silently resends POST data if page no longer in cache

Status: NEW

(Firefox :: File Handling, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:

1)  Load a POST result page
2)  Clear the cache.
3)  File > Save As
4)  Save as "HTML only"
5)  Either make an HTTP log as you do this, or have access to the server, or use
    a server that produces different output on every POST.

EXPECTED RESULTS: No silent repost

ACTUAL RESULTS: silent repost

Now that bug 84106 is fixed, we'll at least _try_ to hit the cache before reposting, but if the content has expired from cache we'll happily redo the POST.  If you're saving a purchase confirmation page, you might end up repeating the purchase as a result.

For history traversal, we make sure to never do this.  If the POST page is not cached, we put up a dialog asking the user whether to resend the POST data.

Is that the behavio we want here?  Do we want something else (like just aborting the save operation)?

Comment 1

[Mardeg]

What happens if you Ctrl A, right-click -> View Selection Source, then in the "DOM Source of Selection" window do File -> Save Page As...?
If that doesn't repost could this be the wanted behaviour?

Comment 2

[Boris Zbarsky [:bzbarsky]]

> What happens if

That calls ViewSourceSavePage(), which does a GET on the original page URL.  So it's completely broken for POST pages no matter what.  Want to file a bug on that?

Comment 3

[Nochum Sossonko [:Natch]]

(In reply to comment #2)
I think that's bug 469302, is it fixed on trunk and branch?

Comment 4

[Boris Zbarsky [:bzbarsky]]

No, it is NOT bug 469302.  That bug is about the source shown.  Comment 2 is about the source saved (which is distinctly different from what's shown).  Neither has anything to do with this bug.

Comment 5

[Jesse Ruderman]

"Please pay me $5 via PayPal and save two copies of the confirmation page -- one for your files and one to send to me."

Comment 6

[Boris Zbarsky [:bzbarsky]]

Need UI spec; see end of comment 0.

Comment 7

[:Paolo Amadini]

Some development references:

* The place where the "web browser persist" component sets the channel
   properties, to control whether an URL is loaded from the cache:

http://mxr.mozilla.org/mozilla-central/source/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp#1194

* The place where the docshell sets the HTTP channel properties, to control
   whether an URL is loaded *only* from the cache:

http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#7532

* The docshell code that handles the case where a document was not found in
   the cache, and asks the user whether to repost:

http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsWebShell.cpp#1198

* The actual repost dialog implementation:

http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8991


The open question from comment 0 is still if, when "Save As" requires a repost,
we should:
 * Put up a dialog asking the user whether to resend the POST data
 * Abort the save
 * Something else

Comment 8

[:Paolo Amadini]

Noting related bug 177329.

Comment 9

[Shad Sterling]

This should block bug 288462

Comment 10

[Alin Ilea]

@Boris, is this issue still reproducible or it can be closed since there are no new updates for a long time?
Thanks!

Comment 11

[Andrew McCreight [:mccr8]]

There are steps in comment 0 that could be used to check if this issue still applies or not.

Comment 12

[Ralu]

I tested this issue using the steps from comment 0. Can you please confirm that silent repost refers to the post that appears after Saving page as HTML? I attached a screenshot with my results after saving page as HTML and no other actions on the page.

Comment 13

[Ralu]

Attachment: 2021-07-19_16h14_17.png

Comment 14

[Boris Zbarsky [:bzbarsky]]

It's hard to say what's going on there, especially given the serviceworker. But also, as far as I know the fetches "save as" does don't show up in the devtools network panel anyway.

I just tried those steps with https://www.randomresult.com/num.php and I definitely see a new POST happening when I save, in that the saved data does not match the displayed data, if I clear cache before saving.

I believe I also see a new POST happening on "view source" (again, based on examination of the actual data in the source and whether it matches the original page), which is really broken. I tested with "view_source.tab" both true and false and see the same behavior in both cases.

Comment 15

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bzbarsky] from comment #14)

It's hard to say what's going on there, especially given the serviceworker. But also, as far as I know the fetches "save as" does don't show up in the devtools network panel anyway.

They do show up in the browser toolbox's network panel. :-)

I just tried those steps with https://www.randomresult.com/num.php and I definitely see a new POST happening when I save, in that the saved data does not match the displayed data, if I clear cache before saving.

Yeah, same. This still seems broken.

I believe I also see a new POST happening on "view source" (again, based on examination of the actual data in the source and whether it matches the original page), which is really broken. I tested with "view_source.tab" both true and false and see the same behavior in both cases.

I filed bug 1721580 for the view source case.

In terms of what to do here, I think a warning along the same lines as the history navigation case would make sense. The docshell code for the history navigation case is here:

https://searchfox.org/mozilla-central/rev/699174544b058f13f02e7586b3c8fdbf438f084b/docshell/base/nsDocShell.cpp#11747-11753

and the relevant prompt is completely encapsulated in a simple idl API call that we can easily reuse

However, it is not clear to me how the webbrowserpersist code (which sets up the channel to make the request for the "HTML only" save) would know whether or not there'd be a cache hit for the request. Nihanth, do you know?

Comment 16

[(inactive) Nihanth Subramanya [:nhnt11]]

Whoops, meant to look at this earlier and then it got lost in backlog when I went on vacation.

I think we have an opportunity to do some logic in nsWebBrowserPersist::SaveURIInternal, which seems to do the actual loads for each URI. There are some load flags being set (defaults). I wonder if we could try to do a cache-only load, and if it fails, prompt before doing a network load?

I didn't dig further than that because I couldn't reproduce - clearing all data and then saving the page didn't show an extra POST request nor did I see newly generated random numbers in the saved page. Any chance something "fixed" this already in the 2 months that I didn't respond here, I wonder? I can ask in the necko meeting tomorrow...

Comment 17

[Boris Zbarsky [:bzbarsky]]

clearing all data and then saving the page didn't show an extra POST request

Just to double-check: did you save as "Web Page, HTML only", or "Web Page, complete"? The default behavior in a clean profile is "Web Page, complete", iirc. The codepath that has the bug is "Web Page, HTML only".

Comment 18

[(inactive) Nihanth Subramanya [:nhnt11]]

You're right, I missed that somehow. I can reproduce with https://www.randomresult.com/num.php.

I think my suspicion in comment 16 that this might be actionable still holds, but my queue is pretty full right now so I'll have to let this go through triage.