1344465 - Can't submit form to an http(s) URL using POST method from WebExtensions

Status: NEW

(WebExtensions :: General, defect, P3)

Description

[afnankhan]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170303030202

Steps to reproduce:

I am trying to submit form form webextension page with dynamic data using post method like describe here:
http://stackoverflow.com/a/23687543
I want to show response page so i can't use fetch or XMLHttpRequest.
Also it would be helpful if there is a webextension api to open url using post method.

Comment 1

[Kris Maglione [:kmag]]

It should work. Can you attach a testcase add-on?

Comment 2

[afnankhan]

Attachment: test-addon.zip

There is form in options page which submits to http://posttestserver.com/post.php. In chrome the site say "Successfully dumped 1 post variables" but in firefox it says 0.

Comment 3

[YF (Yang)]

Also occur for opened this html file as file://, since bug 1147911.

Comment 4

[Bob Owen (:bobowen)]

I have a fix for the file:// URI case, I expect it will fix the web extension one as well, just testing now.

Comment 5

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=74d1d5265a708803fe9885f557ac42f3f10325c6

and again with test and fix for large allocation test failure:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6a75942b9b528e3391799f15721c5012c5cfac1

Comment 6

[Bob Owen (:bobowen)]

Attachment: Part 1: Pass through post data when redirecting load to a different process

Comment 7

[Bob Owen (:bobowen)]

Attachment: Part 2: Check that we can form post from a different process

Comment 8

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

Comment on attachment 8847638 [details] [diff] [review]
Part 1: Pass through post data when redirecting load to a different process

Since mystor is doing something related in a different bug, he should also take a look.

Did you test how this works when uploading lots of data? I'm a bit worried about memory usage.

Comment 9

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

Comment on attachment 8847639 [details] [diff] [review]
Part 2: Check that we can form post from a different process

We need better tests. Something testing multiple values, and I'm mostly worried about file uploads. Better to ensure serializing to string and back to binary data doesn't lose any information.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8847638 [details] [diff] [review]
Part 1: Pass through post data when redirecting load to a different process

Review of attachment 8847638 [details] [diff] [review]:
-----------------------------------------------------------------

This implementation looks good to me for the most part, except for the problems which I mention below. You should be able to revert bug 1347983 part 1 before landing this and ideally the test in part 2 should still pass once this has landed.

(In reply to Olli Pettay [:smaug] from comment #9)
> Comment on attachment 8847639 [details] [diff] [review]
> Part 2: Check that we can form post from a different process
> 
> We need better tests. Something testing multiple values, and I'm mostly
> worried about file uploads. Better to ensure serializing to string and back
> to binary data doesn't lose any information.

We should also make sure that trying to upload a large file (say 1GB) shouldn't crash the browser, IIRC there is a cap of ~500mb for how large we allow IPC messages to be, so if we're sending it over IPC as a string that could crash the browser.

That seems unacceptable to me. I think we need some way to redirect the stream without serializing it into a string. r- from me until we can ensure that this handles large file uploads and large post messages without discarding the data or crashing the browser.

Comment 11

[Bob Owen (:bobowen)]

Attachment: Part 2: Check that we can form post from a different process

I tried for a while to get the other plain form submission tests to work from a file:// URL, but after battling for a bit I hit cross-origin issues in the test runner.
So, I went back to just enhancing this test a bit.

I've added in a checkbox to test something other that a simple text field and also two files, one text one binary.
I moved the test to where the other form submission stuff is as I'm using the form_submit_server.sjs there.

I have also locally tested with a 1GB binary file, confirming that the file is read in the parent.
Firefox didn't crash, although http.js did. :-)

Comment 12

[Bob Owen (:bobowen)]

Comment on attachment 8847638 [details] [diff] [review]
Part 1: Pass through post data when redirecting load to a different process

mystor - are you happier that this solution seems OK now?

baku - smaug also suggested that you should look over this fix as you've been looking at this sort of streaming between child and parent recently.

Comment 13

[Andrea Marchesini [:baku]]

Comment on attachment 8847638 [details] [diff] [review]
Part 1: Pass through post data when redirecting load to a different process

Review of attachment 8847638 [details] [diff] [review]:
-----------------------------------------------------------------

Utils.serializeInputStream(stream) calls NetUtil.readInputStreamToString(aStream, aStream.available()).
This means that if we are posting 20mb of postData, we are going to convert it into a string. Then all is send via IPC.
This can easily reach the limit of IPC message size, and then we crash.

We need to find a better solution here. Something we can do is to support the nsIInputStream in structured clone as we support Blob and other types.

Comment 14

[Andrea Marchesini [:baku]]

I filed bug 1350386 because it seems that what you are doing here, it happens in other places in our codebase.

Comment 15

[Bob Owen (:bobowen)]

Comment on attachment 8847638 [details] [diff] [review]
Part 1: Pass through post data when redirecting load to a different process

(In reply to Andrea Marchesini [:baku] from comment #14)
> I filed bug 1350386 because it seems that what you are doing here, it
> happens in other places in our codebase.

Thanks for the feedback.
Yes, I'd copied this from elsewhere.

I think given all of this I might need to break out the file:// URL issue and instead enable the pref I added in bug 1343184.
That way the http(s) URL that we are posting to will be loaded in the same process.

It might be safer from a web compat point of view anyway, until we have better support.

Comment 16

[Bob Owen (:bobowen)]

Comment on attachment 8851031 [details] [diff] [review]
Part 2: Check that we can form post from a different process

Cancelling review for now.

Comment 17

[Nika Layzell [:nika] (ni? for response)]

(In reply to Bob Owen (:bobowen) from comment #15)
> Comment on attachment 8847638 [details] [diff] [review]
> Part 1: Pass through post data when redirecting load to a different process
> 
> (In reply to Andrea Marchesini [:baku] from comment #14)
> > I filed bug 1350386 because it seems that what you are doing here, it
> > happens in other places in our codebase.
> 
> Thanks for the feedback.
> Yes, I'd copied this from elsewhere.
> 
> I think given all of this I might need to break out the file:// URL issue
> and instead enable the pref I added in bug 1343184.
> That way the http(s) URL that we are posting to will be loaded in the same
> process.
> 
> It might be safer from a web compat point of view anyway, until we have
> better support.

I think we would still want to have this feature of redirecting post requests across process, I just think we need the feature which baku is talking about in bug 1350386 to implement it. It would be a real shame if we couldn't enforce a rule like toplevel HTML content never loads in the file:// process.

Comment 19

[Bob Owen (:bobowen)]

Broken the file case out to bug 1351358.

Comment 20

[Bob Owen (:bobowen)]

Comment on attachment 8851031 [details] [diff] [review]
Part 2: Check that we can form post from a different process

I'll move this patch to bug 1351358.

Comment 21

[devunion]

Hi guys, could you release fix for this bug a bit faster? Or does it possible to release only partial fix for the basic cases only?
4 months is too much :-(.

Thanks!

Comment 24

[CoolCmd]

Is there a workaround for this bug? The form sends GET instead of POST. 9 months, still no fix. :(

Comment 25

[Martin S.]

Hey, beetles, I've been using top-level data URI (containing POST form submitted onload) as a workaround for this.
Since page data URIs are gotten disabled ( https://bugzilla.mozilla.org/show_bug.cgi?id=1401895 ), this bug gets serious.

Comment 26

[Andrew L]

Andy,

We're running into a problem I hope you can help with.

In an effort to block external parties from injecting malicious code on our New Tab page, we decided to load the page within an iframe. When performing a form POST which targets a new tab, we noticed the POST data is discarded by the WebExtension (as previously stated here). 
A similar setup outside of the WebExtension works as expected, the POST request is successful and results are displayed on the new tab. Please note the expected behavior is currently supported by Chrome Extension. In order to properly secure our extensions we need to be able to POST results from an iframe and load expected results on a new tab.

Is there an update on a resolution for this bug or a suggested work around for us to move forward? Thanks for your help

Comment 27

[Firefox Bug Husbandry Bot]

Bulk move of bugs per https://bugzilla.mozilla.org/show_bug.cgi?id=1483958

Comment 28

[Anna]

Instead of extensions having to rely on ugly workarounds like described in http://stackoverflow.com/a/23687543 it would be much better if you could add POST support for browser.tabs.create. IDK what the best way is to do this but something like adding postData to createProperties and then either have the API decide based on the postData whether it needs to use application/x-www-form-urlencoded or multipart/form-data or text/plain OR add another option where extension devs can specify the enctype themselves.
The cross-process problem with POST data wouldn't be much of an issue if extensions had an easy way to make POST requests.

For example the extension Temporary Containers is currently broken in "automatic mode" (which opens a new temporary container for every new domain) when a FORM sends data to another domain or when using search keywords. It listens to onBeforeRequest and can see the POST data but it can't pass it on to tabs.create.

Comment 29

[David Durst [:ddurst]]

Unassigning kmaglione. Baku -- Kris suggested that you might be a good person to look at this bug. Could you?

Comment 30

[Nika Layzell [:nika] (ni? for response)]

This should hopefully be fixed once I fix bug 1467223.

Clearing baku's review request as I'm dealing with the situation.

Comment 28 mentions container switching losing POST data etc, which unfortunately won't be fixed by that bug, but will probably be fixed by other changes coming down the pipe. It may be possible for addons to do a hacky workaround once this patch is fixed, however.

Comment 31

[Mike Conca [:mconca]]

Seems like this might be fixed now, based on Nika's comment 30. Can someone confirm?

Comment 32

[David Durst [:ddurst]]

That's a fine question. Anna, do you want to test this (since you cited a case in #c28)? Some solid STR would be great.

Comment 33

[Anna]

I tested it with an updated version of the attached test extension by using the source code from http://httpbin.org/forms/post for the options page (and using the full url in the form action) because the original url used in the extension doesn't seem to exist anymore.
Result after opening the options page and submitting the form is that http://httpbin.org/post fails with 405: Method Not Allowed and looking at the request logs in the browser console shows that the request was made with GET instead of POST.
Tested with Firefox 66b14

The case I cited in #28 won't work either unless the addon devs update Temporary Containers with a hacky workaround, because as Nika mentioned in #30 container switching losing POST data etc, won't be fixed by that bug anyway.

Comment 34

[Claudio Guarnieri]

I apologize for reviving this issue, but I just stumble into this problem which seems to be unique to Firefox. Is there any intention to resolve this? It is a significant breaker for me, and I imagine for others too.

Comment 35

[devunion]

(In reply to Claudio Guarnieri from comment #34)

I apologize for reviving this issue, but I just stumble into this problem which seems to be unique to Firefox. Is there any intention to resolve this? It is a significant breaker for me, and I imagine for others too.

Hi, not sure if the problem is fixed at the moment. I also faced this issue a couple of years ago. My decision was to use custom events for communication with the target site instead of sending POST data. Of course, this way works only if you've an access to the necessary site. May be my solution will be helpful for you.

Comment 36

[ibrahim.alexandru]

Any news on this? It seems that is still happening on latest version of Firefox.