1629441 - Cross-process POST requests degrade to GET requests when they open new windows

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[Eduardo Philippi]

Attachment: Post_in_firefox_(j_spring_security_check).png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:75.0) Gecko/20100101 Firefox/75.0

Steps to reproduce:

Authenticate user using a POST request with "j_spring_security_check", https://help.pentaho.com/Documentation/8.2/Developer_Center/REST_API

Actual results:

Authenticate using a POST request with "j_spring_security_check" fails in Firefox, before the last update of Firefox was working

Expected results:

Authenticate the user

Comment 1

[Tom S [:evilpie]]

Thank you for your detailed bug report.

The problem seems to be that trying to POST a form in an HTML file loaded via file:// to a server using an http:// scheme. Instead of using POST it uses GET. I created a simple testcase to reproduce this locally and arrived at bug 1603006.

If possible you could also host log_in.html on a server and it should work.

Comment 3

[Tom S [:evilpie]]

Attachment: Testcase

Test needs to be loaded via file://. Tries to submit the form to http://httpbin.org/post, which will fail for GET requests.

Comment 4

[Kris Maglione [:kmag]]

Paul, it looks like the problem is that, while we always use DocumentChannel for cross-process loads in existing docshells, because of this:

https://searchfox.org/mozilla-central/rev/2cd2d511c0d94a34fb7fa3b746f54170ee759e35/toolkit/modules/E10SUtils.jsm#893-903

We don't have a similar check for loads that are retargeted to open new windows, which hit this path:

https://searchfox.org/mozilla-central/rev/2cd2d511c0d94a34fb7fa3b746f54170ee759e35/toolkit/modules/E10SUtils.jsm#842-853
https://searchfox.org/mozilla-central/rev/2cd2d511c0d94a34fb7fa3b746f54170ee759e35/dom/ipc/ContentChild.cpp#901-918

And then throw away the LoadInfo with the post data when they trigger the load:

https://searchfox.org/mozilla-central/rev/2cd2d511c0d94a34fb7fa3b746f54170ee759e35/dom/ipc/ContentChild.cpp#944-947

Comment 5

[Paul Bone [:pbone]]

Thanks. Looking/testing now.

Comment 6

[Paul Bone [:pbone]]

Hi kmag/evilpie

I cannot easilly see the screen shot (would have to enhance it in an image editor etc). Is there anything important I need to understand in the screenshot but not captured elsewhere?

Thanks.

Comment 7

[Tom S [:evilpie]]

No, I don't think you are missing any important information.

Comment 8

[Paul Bone [:pbone]]

Thanks.

Comment 10

[Tom S [:evilpie]]

Sorry! I do wonder if it would be better to revert bug 1603006 for now.

Comment 12

[Paul Bone [:pbone]]

(In reply to Tom Schuster [:evilpie] from comment #10)

Sorry! I do wonder if it would be better to revert bug 1603006 for now.

I have a fix now. I'll post it for review & try shortly. What I'm unsure of is whether we uplift it. and what that looks like for release. I'm unfamiliar with our process for that.

Comment 13

[Paul Bone [:pbone]]

Attachment: Bug 1629441 - Defer to DocumentChannel in shouldLoadURLInThisProcess r=kmag

Comment 14

[Paul Bone [:pbone]]

Hi Julien,

I heard you were the person I need to speak to regarding advice about uplifting my change (once it's reviewed and gets through try/landing!).

This is a regression that made it onto our release channel before we noticed. How is it decided what to do WRT uplifting to release and who decides?

I have a second regression on release Bug 1630757, which was has the same regression cause. Maybe we backout that change on release while uplifting on beta?

Because Evilpie was talking about backout I'm going to NI him also, in case he knows something about these procedures.

Thanks.

Comment 15

[Paul Bone [:pbone]]

[Tracking Requested - why for this release]: I don't know if this should uplift onto release. I have a feeling it should for beta.

Comment 16

[Firefox Bug Husbandry Bot]

Resetting severity to default of --.

Comment 17

[Tom S [:evilpie]]

I doubt that we will uplift this to release, but I will defer to other people.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ebea9991db0e
Defer to DocumentChannel in shouldLoadURLInThisProcess r=kmag

Comment 19

[Alexandru Michis [:malexandru]]

Backed out changeset ebea9991db0e for causing lint failures in browser_form_post_from_file_to_http.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/e001cc8bb74f0fc4ffa5c40f72fee6d02f4f8c88

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=298341490&repo=autoland&lineNumber=318

[task 2020-04-20T05:10:38.272Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/dom/html/test/browser_form_post_from_file_to_http.js:114:62 | 'binContentType' is already declared in the upper scope. (no-shadow)

Comment 20

[Paul Bone [:pbone]]

Thanks fixing.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/711080298194
Defer to DocumentChannel in shouldLoadURLInThisProcess r=kmag

Comment 22

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/711080298194

Comment 23

[Paul Bone [:pbone]]

Comment on attachment 9141266 [details]
Bug 1629441 - Defer to DocumentChannel in shouldLoadURLInThisProcess r=kmag

Beta/Release Uplift Approval Request

• User impact if declined: Form posts from a file:// URL to a http:// URL with target="_blank" do not work, and maybe other file:// -> http:// situations where HTTP POST is replaced with HTTP GET.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The diff is small and the condition it adds is already used on a different code path.
• String changes made/needed: There are no string changes

Comment 24

[Benoit Plessis]

Form posts from a file:// URL to a http:// URL with target="_blank" do not work, and maybe other file:// -> http:// situations where HTTP POST is replaced with HTTP GET.

It also impact http:// to http:// POST if under a checkLoadUri "allAccess" policy

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9141266 [details]
Bug 1629441 - Defer to DocumentChannel in shouldLoadURLInThisProcess r=kmag

Fixes a commonly-reported regression in Fx75 and includes new automated tests. Approved for 76.0b8.

Comment 26

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3199c2610b2a

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Should the fix uplifted to release too?

Comment 29

[Ryan VanderMeulen [:RyanVM]]

76 is in RC and ships next week.