Open Bug 1344465 Opened 7 years ago Updated 8 months ago

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

Categories

(WebExtensions :: General, defect, P3)

defect

Tracking

(Not tracked)

People

(Reporter: afnankhan, Unassigned)

References

Details

(Keywords: testcase, Whiteboard: triaged)

Attachments

(1 file, 3 obsolete files)

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.
It should work. Can you attach a testcase add-on?
Attached file 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.
Also occur for opened this html file as file://, since bug 1147911.
Blocks: 1147911
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
Summary: Can't submit form using post method form WebExtensions page → Can't submit form using post method form WebExtensions or file:// page
Blocks: 1321430
Whiteboard: sbwc2, sbmc2, sblc3
I have a fix for the file:// URI case, I expect it will fix the web extension one as well, just testing now.
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
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.
Attachment #8847638 - Flags: review?(michael)
Attachment #8847638 - Flags: review?(bugs)
Attachment #8847638 - Flags: review+
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.
Attachment #8847639 - Flags: review?(bugs) → review-
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.
Attachment #8847638 - Flags: review?(michael) → review-
Whiteboard: sbwc2, sbmc2, sblc3 → sbwc2, sbmc2, sblc3, triaged
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. :-)
Attachment #8851031 - Flags: review?(bugs)
Attachment #8847639 - Attachment is obsolete: true
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.
Attachment #8847638 - Flags: review?(michael)
Attachment #8847638 - Flags: review-
Attachment #8847638 - Flags: feedback?(amarchesini)
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.
Attachment #8847638 - Flags: feedback?(amarchesini) → feedback-
Depends on: 1350386
I filed bug 1350386 because it seems that what you are doing here, it happens in other places in our codebase.
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.
Attachment #8847638 - Flags: review?(michael)
Comment on attachment 8851031 [details] [diff] [review]
Part 2: Check that we can form post from a different process

Cancelling review for now.
Attachment #8851031 - Flags: review?(bugs)
(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.
See Also: → 1351358
Attachment #8847638 - Attachment is obsolete: true
Broken the file case out to bug 1351358.
Assignee: bobowencode → nobody
No longer blocks: 1321430, 1147911
Status: ASSIGNED → NEW
Summary: Can't submit form using post method form WebExtensions or file:// page → Can't submit form to an http(s) URL using POST method from WebExtensions
Whiteboard: sbwc2, sbmc2, sblc3, triaged → triaged
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.
Attachment #8851031 - Attachment is obsolete: true
See Also: → 1348018
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!
Assignee: nobody → kmaglione+bmo
Priority: -- → P2
Is there a workaround for this bug? The form sends GET instead of POST. 9 months, still no fix. :(
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.
Blocks: 1457520
See Also: → 1457520
Product: Toolkit → WebExtensions
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
Bulk move of bugs per https://bugzilla.mozilla.org/show_bug.cgi?id=1483958
Component: Untriaged → General
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.
Unassigning kmaglione. Baku -- Kris suggested that you might be a good person to look at this bug. Could you?
Assignee: kmaglione+bmo → nobody
Flags: needinfo?(amarchesini)
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.
Depends on: 1467223
Flags: needinfo?(amarchesini)

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

Flags: needinfo?(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.

Flags: needinfo?(ddurst) → needinfo?(earthlng)

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.

Flags: needinfo?(earthlng)

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.

(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.

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

Severity: normal → S3
Priority: P2 → P3
You need to log in before you can comment on or make changes to this bug.