Form re-submission makes GET request instead of POST when reload hits a network error (e.g. net::ERR_CACHE_MISS)
Reported by
alex.ken...@replacements.com,
Jul 6
|
||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/67.0.3396.79 Safari/537.36 Steps to reproduce the problem: 1. Open a new tab or browser instance 2. Navigate to a page via an HTTP POST request(the POST request must directly return the given page without redirects so that the final request that results in the loading of the page is a POST) 3. Close the tab 4. Re-open the tab using the "CTRL + Shift + t" keyboard shortcut 5. You will see a "Confirm Form Re-submission" prompt(see attached screenshot). Follow the instructions there and click the reload button(or hit F5) to reload the page. 6. The page will be requested using an HTTP GET request rather than a POST and all request parameters will be lost. What is the expected behavior? As the displayed message suggests, the request should be made as a POST just as the original request with all form data preserved. What went wrong? Attempting to reload the page triggered an HTTP GET request to be made instead of a POST and all parameters(form data) from the original request. The request parameters were lost and the form could not be reloaded(re-submitted) correctly. Did this work before? N/A Chrome version: 67.0.3396.79 Channel: stable OS Version: 6.1 (Windows 7, Windows Server 2008 R2) Flash Version:
,
Jul 9
,
Jul 10
Thanks for filing the issue! @Reporter: Could you please provide sample test URL/file which helps us to triage it further in a better way. Any further inputs from your may be helpful.
,
Jul 12
@vamshi.kommuri@chromium.org - I cannot share the original URL where I encountered the problem. I am working on setting up an example that I can share that allows this issue to be recreated and will update once that is available. Also, I will add another detail - this will not happen if the response/page is cache-able and cached. The response where I encountered this issue includes a Cache-Control header which Chromium respects and does not cache the page. If the page is cached, the page will be loaded from cache and no HTTP request will be made, hence the issue can only be reproduced with an un-cached page.
,
Jul 12
Thank you for providing more feedback. Adding the requester to the cc list. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 13
As per the comment#4 by reporter, awaiting the sample URL to be provided on which reporter is working on...adding Needs-Feedback label till sample test URL is provided. Thanks!
,
Jul 18
@vamshi.kommuri@chromium.org - I have set up a test site to reproduce this issue. The URL is: http://ec2-54-71-23-13.us-west-2.compute.amazonaws.com/ That URL is for a form. Submit that form by clicking the "Do POST" button. Close the tab. Restore the tab by hitting "CTRL + Shift + t." This will show the "confirm form re-submission" message. Reload the page to re-submit the form. The resulting request will be a GET rather than a POST. I have confirmed from the network log in the developer tools in addition to the response from my server. The attached screenshots demonstrate using the example form. Let me know if there are any questions.
,
Jul 18
Thank you for providing more feedback. Adding the requester to the cc list. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 27
Thank you very much for the report! I think I can try to take a look and at least try to create a content_browsertest that repros the problem. So far, I've verified that the following single-tab steps also repro the bug in content_shell: 1. Visit http://ec2-54-71-23-13.us-west-2.compute.amazonaws.com/ 2. Press "Do POST" 3. Go back 4. Go forward - Observe the following error: Could not load the requested resource. Error code: -400 (net::ERR_CACHE_MISS) 5. Reload - Observe that GET rather than POST request has been issued
,
Jul 27
Interestingly, we already have some test coverage of that scenario (e.g. SessionHistoryTest.GoBackToCrossSitePostWithRedirect and NavigationControllerBrowserTest.PostViaOpenUrlMsg) - AFAICT the difference in this bug is the presence of "Cache-Control: no-cache, no-store, must-revalidate" response headers. A WIP CL that contains a (currently still failing) regression test: https://crrev.com/c/1153564
,
Jul 27
It seems that (at least in content_browsertests) the problem happens during the first history navigation to the form-target - this navigation expectedly fails with net::ERR_CACHE_MISS but this clobbers FNE::method_ from POST to GET. This navigation sees the following sequence of events:
- RenderFrameImpl::CommitFailedNavigation gets called (still with the correct common_params.method set to POST)
- RenderFrameImpl::CommitFailedNavigation constructs a |failed_request| which has the correct HttpMethod() set to POST
- When RenderFrameImpl::CommitFailedNavigation exits, frame_->GetDocumentLoader()->GetRequest().HttpMethod() is set to GET. I don't yet understand where this value comes from.
- Some time after, the failed navigation commits. This involves propagating frame_->GetDocumentLoader()->GetRequest().HttpMethod() value into CommonNavigationParams::method:
#1 0x7f3cb5bdae70 content::RenderFrameImpl::MakeDidCommitProvisionalLoadParams()
#2 0x7f3cb5bd1f42 content::RenderFrameImpl::DidCommitNavigationInternal()
#3 0x7f3cb5bd199e content::RenderFrameImpl::DidCommitProvisionalLoad()
#4 0x7f3cb1c81772 blink::LocalFrameClientImpl::DispatchDidCommitLoad()
#5 0x7f3cb231a66b blink::DocumentLoader::DidCommitNavigation()
#6 0x7f3cb2319335 blink::DocumentLoader::InstallNewDocument()
#7 0x7f3cb2318f36 blink::DocumentLoader::CommitNavigation()
#8 0x7f3cb2317916 blink::DocumentLoader::CommitData()
#9 0x7f3cb23198d1 blink::DocumentLoader::ProcessData()
#10 0x7f3cb231984d blink::DocumentLoader::DataReceived()
#11 0x7f3cb05a267c blink::Resource::DidAddClient()
#12 0x7f3cb0599cb7 blink::RawResource::DidAddClient()
#13 0x7f3cb05a3176 blink::Resource::FinishPendingClients()
- When the navigation commits in the browser process, the HTTP method reported by the renderer will override the old value in the FNE:
#1 0x7f2f8f006665 content::FrameNavigationEntry::UpdateEntry()
#2 0x7f2f8f01fd27 content::NavigationEntryImpl::AddOrUpdateFrameEntry()
#3 0x7f2f8f016ee6 content::NavigationControllerImpl::RendererDidNavigateToExistingPage()
#4 0x7f2f8f014e64 content::NavigationControllerImpl::RendererDidNavigate()
#5 0x7f2f8f0346cb content::NavigatorImpl::DidNavigate()
#6 0x7f2f8f048e5f content::RenderFrameHostImpl::DidCommitNavigationInternal()
#7 0x7f2f8f048958 content::RenderFrameHostImpl::DidCommitProvisionalLoad()
,
Jul 27
Adding some navigation/loading experts to CC... QUESTION: Should we update FNE if the navigation errored out? (the FNE is updated in the last step in #c11 despite the fact that we did CommitFailedNavigation with net::ERR_CACHE_MISS earlier) Also - it feels weird that Blink thinks that we are committing a data: URI (with contents of the error page), but the IPC to the browser says that the original URI has committed. OTOH, I am not sure how I feel about it - I think I am still a bit confused with how all of this should work...
,
Jul 27
FWIW, the original repro steps no longer reproduce this bug in https://crrev.com/c/1153564/3 which avoids updating FNE if the navigation failed. I am not sure if similar changes need to be also made in RendererDidNavigateToNewPage (which AFAICT can also be called in case of a failed navigation), but it seems to be a separate, orthogonal problem (since in this case there is no correct FNE and no PageState-with-PostData at all).
,
Jul 30
Comments 12-13: Hmm, I'm actually a little nervous skipping the AddOrUpdateFrameEntry call entirely. Maybe it's ok in the case of an error page when we want to preserve what was there before for a later successful reload, but we'll need to review that carefully to make sure no stale data gets left around (which is similar to security bugs we've seen in the past). The main risk is that that the NavEntry or FNE gets partially updated somewhere else and then things get mixed up. I'm guessing you're headed in the right direction, though, and we'll just need to be careful about it.
,
Jul 30
,
Jul 30
FWIW, I opened another bug for NCI::RendererDidNavigateToNewPage (separate from NCI::RendererDidNavigateToExistingPage which is dealt with in the current bug): issue 869117 .
,
Jul 31
Could we trace why HTTP method gets changed in Blink? If we could make sure it doesn't get changed, we wouldn't have to modify the update of FTN entries.
,
Jul 31
+dcheng who probably wants to know about discussions related to provisional documents RE: #c17 from clamy@: why HTTP method gets changed in Blink RenderFrameImpl::LoadNavigationErrorPageInternal creates the error page in Blink via |frame_->CommitDataNavigation|. Interestingly, there is some code in WebLocalFrameImpl::CommitDataNavigation that tries to preserve properties of the original request, but here (I think because this is a browser-initiated navigation) we have |provisional_document_loader == nullptr| and therefore the copying of |provisional_document_loader->OriginalRequest()| doesn't take place. When later RenderFrameImpl::MakeDidCommitProvisionalLoadParams creates FrameHostMsg_DidCommitProvisionalLoad_Params it trusts the values provided by Blink (including the HTTP method). Do you have suggestions on how to conjure the right request in Blink? - On one hand, this is tricky, because on one hand we are committing an error page that has different properties from the original request (e.g. the origin is different than what it would have been without an error - with an error we use an opaque/unique origin) but maybe some properties of the original request need to be preserved (e.g. the HTTP method + POST data). - On the other hand, having the right request in Blink would also address issue 869117 and therefore might be a more correct fix WDYT?
,
Jul 31
After talking with nasko@ and dcheng@ I tried to use |failed_request| from RFI::LoadNavigationErrorPage in place of / in addition to |provisional_document_loader->OriginalRequest()| in WebLocalFrameImpl::CommitDataNavigation. This seems promising, but: 1. It seems that WebLocalFrameImpl::CommitDataNavigation isn't always called with a |failed_request| - this means that we can't yank out our reliance on |provisional_document_loader->OriginalRequest()|. 2. WebLocalFrameImpl::CommitDataNavigation only does the switcheroo if |replace| is true, but this wasn't the case in the repro. I don't know why we wouldn't want to always |replace| in case of RFI::Commit*Failed*Navigation... 3. Additional work is needed to preserve POST data by putting it into PageState of the error page. I am not sure at this point if constructing a new WebHistoryItem from CommonNavigationParams is desirable or not (it is not something that seems possible today).
,
Aug 1
Turns out that the POST body was getting lost in CreateURLRequestForNavigation which (unlike RenderFrameImpl::CreateURLRequestForCommit) didn't propagate |common_params.post_data| and/or |request_params.post_content_type| into the returned WebURLRequest. The fix I am attempting is to move processing of these inputs into CreateURLRequestForNavigation (which is called by CreateURLRequestForCommit).
FWIW, the call tree looks as follows:
- CreateURLRequestForNavigation
- RenderFrameImpl::CommitFailedNavigation
- RenderFrameImpl::CreateURLRequestForCommit
- RenderFrameImpl::CommitNavigation
I don't have a strong opinion on other |request| properties that are set by CreateURLRequestForCommit but not by CreateURLRequestForNavigation - I think just moving |common_params.post_data| and |request_params.post_content_type| should be fine for now.
,
Aug 23
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/98ccf1c6b1a024e078b6c96cebb8ed52e63bff1b commit 98ccf1c6b1a024e078b6c96cebb8ed52e63bff1b Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Thu Aug 23 18:14:31 2018 Preserve request properties of failed navigations (e.g. POST data). A failed navigation commits a fake data: page (see RenderFrameImpl::LoadNavigationErrorPageInternal). This commit should preserve most aspects of the original request which failed. This includes the http method and POST data. This CL makes sure that the |failed_request| in RenderFrameImpl::CommitFailedNavigation includes POST data (this is fixed by making sure that CreateURLRequestForNavigation preserves the POST data. This CL also passes |failed_request| from RenderFrameImpl::CommitFailedNavigation into WebLocalFrameImpl::CommitDataNavigation - this ensures that the |failed_request|'s properties are properly preserved. This CL also makes sure that |replace| value calculated in RenderFrameImpl::CommitFailedNavigation is correct not only for reloads, but also for back/forward navigations. Bug: 860807 , 869117 Change-Id: Id4e781379defddb26f4e684632d173c46a6f660f Reviewed-on: https://chromium-review.googlesource.com/1153564 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Ryan Sleevi <rsleevi@chromium.org> Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Cr-Commit-Position: refs/heads/master@{#585547} [modify] https://crrev.com/98ccf1c6b1a024e078b6c96cebb8ed52e63bff1b/content/browser/frame_host/navigation_controller_impl_browsertest.cc [modify] https://crrev.com/98ccf1c6b1a024e078b6c96cebb8ed52e63bff1b/content/browser/frame_host/render_frame_host_manager_browsertest.cc [modify] https://crrev.com/98ccf1c6b1a024e078b6c96cebb8ed52e63bff1b/content/public/test/url_loader_interceptor.cc [modify] https://crrev.com/98ccf1c6b1a024e078b6c96cebb8ed52e63bff1b/content/public/test/url_loader_interceptor.h [modify] https://crrev.com/98ccf1c6b1a024e078b6c96cebb8ed52e63bff1b/content/renderer/dom_serializer_browsertest.cc [modify] https://crrev.com/98ccf1c6b1a024e078b6c96cebb8ed52e63bff1b/content/renderer/render_frame_impl.cc [modify] https://crrev.com/98ccf1c6b1a024e078b6c96cebb8ed52e63bff1b/content/renderer/render_frame_impl.h [add] https://crrev.com/98ccf1c6b1a024e078b6c96cebb8ed52e63bff1b/content/test/data/form_that_posts_to_echoall_nocache.html [modify] https://crrev.com/98ccf1c6b1a024e078b6c96cebb8ed52e63bff1b/net/test/embedded_test_server/default_handlers.cc [modify] https://crrev.com/98ccf1c6b1a024e078b6c96cebb8ed52e63bff1b/third_party/blink/public/web/web_local_frame.h [modify] https://crrev.com/98ccf1c6b1a024e078b6c96cebb8ed52e63bff1b/third_party/blink/renderer/core/frame/web_local_frame_impl.cc [modify] https://crrev.com/98ccf1c6b1a024e078b6c96cebb8ed52e63bff1b/third_party/blink/renderer/core/frame/web_local_frame_impl.h
,
Aug 29
Commit 98ccf1c6... initially landed in 70.0.3531.0. I've verified using repro steps from #c9 that the bug seems fixed on Mac Canary. alex.kenion@ - could you please verify the fix? Feel free to tear down the repro page - it shouldn't be needed going forward. |
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by susan.boorgula@chromium.org
, Jul 9