Last Comment Bug 583288 - Authorize PUT and DELETE methods for form submission
: Authorize PUT and DELETE methods for form submission
Status: RESOLVED WONTFIX
not-ready
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
: 583289 (view as bug list)
Depends on: 582412
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-30 10:13 PDT by Mounir Lamouri (:mounir)
Modified: 2011-03-23 03:30 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part1 - Let forms use PUT and DELETE HTTP method when submitting. (30.04 KB, patch)
2010-08-26 12:20 PDT, Mounir Lamouri (:mounir)
jst: review+
jonas: review-
Details | Diff | Splinter Review
Part 2 - Fixing assert in nsHTMLDocument.cpp when using PUT and DELETE HTTP methods for a form submission. (1.18 KB, patch)
2010-08-26 13:41 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
unknown_request_v1 (4.70 KB, patch)
2010-08-26 22:04 PDT, Byron Milligan [:byronm]
no flags Details | Diff | Splinter Review
Part 1 - ForceOpenCacheEntry (5.04 KB, patch)
2010-08-27 01:22 PDT, Mounir Lamouri (:mounir)
jonas: review-
Details | Diff | Splinter Review
Part 2 - PuUT and DELETE methods form form submission (35.55 KB, patch)
2010-08-27 01:25 PDT, Mounir Lamouri (:mounir)
jonas: superreview-
Details | Diff | Splinter Review
Enables client to force caching no matter their request type. (2.09 KB, patch)
2010-08-27 15:18 PDT, Byron Milligan [:byronm]
jonas: review+
mbeltzner: approval2.0-
Details | Diff | Splinter Review
Part 2 v2 - PUT and DELETE methods (42.39 KB, patch)
2010-08-27 21:50 PDT, Mounir Lamouri (:mounir)
jonas: feedback-
Details | Diff | Splinter Review
Part 2 - v3 - PUT and DELETE methods (49.23 KB, patch)
2010-09-01 23:54 PDT, Mounir Lamouri (:mounir)
jonas: review+
mbeltzner: approval2.0-
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2010-07-30 10:13:47 PDT
Using 'PUT' in @method or @formmethod should use the HTTP PUT method.
Comment 1 Mounir Lamouri (:mounir) 2010-08-26 12:20:16 PDT
Created attachment 469560 [details] [diff] [review]
Part1 - Let forms use PUT and DELETE HTTP method when submitting.
Comment 2 Mounir Lamouri (:mounir) 2010-08-26 13:41:28 PDT
Created attachment 469593 [details] [diff] [review]
Part 2 - Fixing assert in nsHTMLDocument.cpp when using PUT and DELETE HTTP methods for a form submission.

With the first part, an assert in nsHTMLDocument is thrown when submitting a form with PUT or DELETE HTTP methods. When using GET and POST methods, this code is called but not asserting.
I tried to find where this was set or reset but I didn't found anything. According to Jonas, changing to an assert may not be harmful so...

Let me know if that sound incorrect or if I should look at that more deeply.
Comment 3 Mounir Lamouri (:mounir) 2010-08-26 13:47:39 PDT
The last part of this patch would be to do a same origin check for PUT and DELETE methods. One possibility would be to do that in nsDocShell: if aHttpMethod is set to PUT or DELETE, the same origin check is added as a notification callback to the channel.
However, I'm not sure if that would be the best way to do this check and I have no idea how that could be tested...
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2010-08-26 16:06:21 PDT
https://developer.mozilla.org/En/Mochitest#How_do_I_test_issues_which_only_show_up_when_tests_are_run_across_domains.3f

Use that together with sjs files.
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2010-08-26 17:45:12 PDT
Comment on attachment 469560 [details] [diff] [review]
Part1 - Let forms use PUT and DELETE HTTP method when submitting.

This patch changes docshell/base/nsIDocShell.idl and webshell/public/nsILinkHandler.h but doesn't update their IIDs. r=jst with that fixed.
Comment 6 Jonas Sicking (:sicking) PTO Until July 5th 2010-08-26 19:22:33 PDT
Comment on attachment 469560 [details] [diff] [review]
Part1 - Let forms use PUT and DELETE HTTP method when submitting.

You need same-origin checks if the method is something other than GET or POST.

Also, change the "No specific HTTP method" to "Use default HTTP method" as those callsites definitely care about what method is used.
Comment 7 Byron Milligan [:byronm] 2010-08-26 22:04:28 PDT
Created attachment 469768 [details] [diff] [review]
unknown_request_v1

Enables the client of the httpChannel to specify that they would like the response to their request to be cached, even though the request is not of type GET, POST, or HEAD.

One thing I worry about doing this is that things may not behave properly since the cache code is not expecting this type of request. I have seen conditionals in the cache code based on what type of request was made... we can give this a try though and see what happens.
Comment 8 Mounir Lamouri (:mounir) 2010-08-26 22:10:09 PDT
Comment on attachment 469768 [details] [diff] [review]
unknown_request_v1

This replace the my part 2 patch. I'm going to rewrite based on this one.
Comment 9 Mounir Lamouri (:mounir) 2010-08-27 01:22:17 PDT
Created attachment 469808 [details] [diff] [review]
Part 1 - ForceOpenCacheEntry

We had to change the patch because the flag was in nsHttpChannel but we have access to nsIHttpChannel. But that might be a bad idea to add such a function into nsIHttpChannel...?
Comment 10 Mounir Lamouri (:mounir) 2010-08-27 01:22:37 PDT
Actually, at least, this is fixing the assert ;)
Comment 11 Mounir Lamouri (:mounir) 2010-08-27 01:25:11 PDT
Created attachment 469809 [details] [diff] [review]
Part 2 - PuUT and DELETE methods form form submission

jst, I ask you another review because there is a few lines more (cross-origin check).
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2010-08-27 10:43:34 PDT
Comment on attachment 469808 [details] [diff] [review]
Part 1 - ForceOpenCacheEntry

While we've been unfreezing some interfaces, I suspect nsIHttpChannel is used enough that we can't modify it.

A better solution is to add a load-flags-value to nsICachingChannel which can then be set using nsIRequest.loadFlags.

This seems like a better solution anyway, since it allows that flag to be implemented by other channels too.
Comment 13 Jonas Sicking (:sicking) PTO Until July 5th 2010-08-27 13:14:47 PDT
Comment on attachment 469809 [details] [diff] [review]
Part 2 - PuUT and DELETE methods form form submission

>@@ -8695,16 +8713,24 @@ nsDocShell::DoURILoad(nsIURI * aURI,
>         if (secMan &&
>             NS_SUCCEEDED(secMan->IsSystemPrincipal(ownerPrincipal,
>                                                    &isSystem)) &&
>             !isSystem) {
>             channel->SetOwner(aOwner);
>         }
>     }
> 
>+    // If a specific HTTP channel has been set and it is "PUT" or "DELETE", we
>+    // should prevent cross-origin requets.
>+    if (aHttpMethod && ownerPrincipal &&
>+        (!strcmp(aHttpMethod, "PUT") || !strcmp(aHttpMethod, "DELETE")) &&
>+        NS_FAILED(ownerPrincipal->CheckMayLoad(aURI, PR_FALSE))) {
>+      return NS_OK;
>+    }

Instead doing this when the method is PUT or DELETE, you should do it whenever it isn't GET, HEAD or POST.

You also need to add code to deal with redirects. Looking into how you best can do that now...
Comment 14 Byron Milligan [:byronm] 2010-08-27 15:18:31 PDT
Created attachment 470047 [details] [diff] [review]
Enables client to force caching no matter their request type.

Accounts for Jonas' feedback.
Comment 15 Mounir Lamouri (:mounir) 2010-08-27 21:50:31 PDT
Created attachment 470148 [details] [diff] [review]
Part 2 v2 - PUT and DELETE methods

So, this is now preventing cross-domain submission for DELETE and PUT even if that's done with a redirection.
However, the assert about the notification is shown so that means there is already a notification when we try to add ours for the redirection...

I will probably do some investigations but if one of you (Johnny or Jonas) have an idea on top of your head...
Comment 16 Mounir Lamouri (:mounir) 2010-08-27 21:52:51 PDT
Comment on attachment 470047 [details] [diff] [review]
Enables client to force caching no matter their request type.

I guess we can just go with Jonas review for this first part.
Comment 17 Jonas Sicking (:sicking) PTO Until July 5th 2010-08-28 00:31:54 PDT
Doh! It appears we are installing the docshell as the notification callback on the channel. See the 5th argument in the call to NS_NewChannel.

The easiest solution is probably to implement nsIChannelEventSink, modify the GetInterface implementation to hand support that interface, and then make OnChannelRedirect look at the new channels request method and if it is something other than get/post/head do a same-origin check.

Also, you should make the get/post/head checks case insensitive.
Comment 18 Jonas Sicking (:sicking) PTO Until July 5th 2010-08-30 14:41:24 PDT
Comment on attachment 470148 [details] [diff] [review]
Part 2 v2 - PUT and DELETE methods

minusing based on previous comment.
Comment 19 Mounir Lamouri (:mounir) 2010-09-01 18:04:45 PDT
(In reply to comment #17)
> Doh! It appears we are installing the docshell as the notification callback on
> the channel. See the 5th argument in the call to NS_NewChannel.
> 
> The easiest solution is probably to implement nsIChannelEventSink, modify the
> GetInterface implementation to hand support that interface, and then make
> OnChannelRedirect look at the new channels request method and if it is
> something other than get/post/head do a same-origin check.

Why not using OnRedirectStateChange?
Comment 20 Mounir Lamouri (:mounir) 2010-09-01 23:54:26 PDT
Created attachment 471424 [details] [diff] [review]
Part 2 - v3 - PUT and DELETE methods
Comment 21 Mounir Lamouri (:mounir) 2010-09-08 11:43:28 PDT
That would be very nice to have this feature in Firefox 4.
Comment 22 Jonas Sicking (:sicking) PTO Until July 5th 2010-09-09 16:22:48 PDT
Comment on attachment 471424 [details] [diff] [review]
Part 2 - v3 - PUT and DELETE methods

>diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
> nsDocShell::OnRedirectStateChange(nsIChannel* aOldChannel,
>                                   nsIChannel* aNewChannel,
>                                   PRUint32 aRedirectFlags,
>                                   PRUint32 aStateFlags)
> {

>+    // DELETE and PUT HTTP channel should not be redirected to a cross-domain.
>+    if (ChannelIsDeleteOrPut(aNewChannel)) {
>+        // This code is very similar to the code of nsSameOriginChecker in
>+        // nsContentUtils but we can't use nsSameOriginChecker because it
>+        // implies to use a channel callback (which way be already used).

"needs to use a channel callback (which we already use)"


Also add a comment to nsSameOriginChecker saying that if someone changes that code, this code should be changed too.


>+        nsCOMPtr<nsIPrincipal> oldPrincipal;
>+        nsresult rv;
>+
>+        nsCOMPtr<nsIScriptSecurityManager> secMan =
>+            do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
>+
>+        rv = secMan->GetChannelPrincipal(aOldChannel,
>+                                         getter_AddRefs(oldPrincipal));
>+        NS_ENSURE_SUCCESS(rv, NS_OK);
>+
>+        NS_ASSERTION(oldPrincipal, "oldPrincipal should not be null!");
>+
>+        nsCOMPtr<nsIURI> newOriginalURI;
>+        aNewChannel->GetOriginalURI(getter_AddRefs(newOriginalURI));
>+
>+        rv = oldPrincipal->CheckMayLoad(newURI, PR_FALSE);
>+        if (NS_SUCCEEDED(rv) && newOriginalURI != newURI) {
>+            rv = oldPrincipal->CheckMayLoad(newOriginalURI, PR_FALSE);
>+        }
>+
>+        // The requested tried to be redirected, we have to cancel it.
>+        if (NS_FAILED(rv)) {
>+            return rv;
>+        }

NS_ENSURE_SUCCESS(rv, rv);

>@@ -8695,16 +8749,27 @@ nsDocShell::DoURILoad(nsIURI * aURI,
>         if (secMan &&
>             NS_SUCCEEDED(secMan->IsSystemPrincipal(ownerPrincipal,
>                                                    &isSystem)) &&
>             !isSystem) {
>             channel->SetOwner(aOwner);
>         }
>     }
> 
>+    // If a specific HTTP channel has been set and it is not GET, POST or
>+    // HEAD, we should prevent cross-origin requests.
>+    if (aHttpMethod && ownerPrincipal &&
>+        nsCRT::strcasecmp(aHttpMethod, "GET") &&
>+        nsCRT::strcasecmp(aHttpMethod, "POST") &&
>+        nsCRT::strcasecmp(aHttpMethod, "HEAD")) {

Can you use ChannelIsDeleteOrPut here? Yes, it'll cost a couple of extra cycles, but it's nice to reuse code.

>+nsDocShell::ChannelIsDeleteOrPut(nsIChannel* aChannel)
>+{
>+    nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(aChannel));
>+    if (!httpChannel) {
>+        return false;
>+    }
>+
>+    nsCAutoString method;
>+    httpChannel->GetRequestMethod(method);
>+    return method.Equals("DELETE") || method.Equals("PUT");
>+}

You should make this function check that the method isn't get/post/head. And rename the function ChannelIsUnsafeMethod or some such.


>diff --git a/uriloader/base/nsDocLoader.cpp b/uriloader/base/nsDocLoader.cpp
>--- a/uriloader/base/nsDocLoader.cpp
>+++ b/uriloader/base/nsDocLoader.cpp
>@@ -1604,17 +1604,21 @@ NS_IMETHODIMP nsDocLoader::AsyncOnChanne
>       stateFlags |= nsIWebProgressListener::STATE_IS_DOCUMENT;
> 
> #if defined(DEBUG)
>       nsCOMPtr<nsIRequest> request(do_QueryInterface(aOldChannel));
>       NS_ASSERTION(request == mDocumentRequest, "Wrong Document Channel");
> #endif /* DEBUG */
>     }
> 
>-    OnRedirectStateChange(aOldChannel, aNewChannel, aFlags, stateFlags);
>+    nsresult rv = OnRedirectStateChange(aOldChannel, aNewChannel, aFlags,
>+                                        stateFlags);
>+    if (NS_FAILED(rv)) {
>+      return rv;
>+    }

NS_ENSURE_SUCCESS(rv, rv)

sr=me with that (or r=me if you prefer)
Comment 23 Mounir Lamouri (:mounir) 2010-09-09 16:28:35 PDT
(In reply to comment #22)
> sr=me with that (or r=me if you prefer)

I thought a sr might be needed for this patch. Let me know if it isn't, I will cancel jst's review.
Comment 24 Jonas Sicking (:sicking) PTO Until July 5th 2010-09-09 17:28:43 PDT
I don't really think it is. It's a pretty simple change.
Comment 26 Mounir Lamouri (:mounir) 2010-09-10 02:19:16 PDT
*** Bug 583289 has been marked as a duplicate of this bug. ***
Comment 27 Jean-Yves Perrier [:teoli] 2010-09-10 03:08:59 PDT
I've updated https://developer.mozilla.org/en/HTML/Element/input to mark PUT and DELETE as implemented (they were marked as unimplemented)

I've also added @formmethod and its value to https://developer.mozilla.org/en/HTML/HTML5/Forms_in_HTML5

dev-doc-needed -> dev-doc-complete ?

Or is there somewhere else we should document this?
Comment 28 Jonas Sicking (:sicking) PTO Until July 5th 2010-09-10 09:31:26 PDT
I think we also need to document that PUT and DELETE is same-origin only, and we also need to update
https://developer.mozilla.org/en/HTML/Element/form
Comment 30 Mounir Lamouri (:mounir) 2010-09-10 11:20:13 PDT
Eric, can you confirm that the doc has been done? (see comment 29)
Comment 31 Eric Shepherd [:sheppy] 2010-09-10 11:29:41 PDT
Yep, this looks good.
Comment 32 Julian Reschke 2010-09-20 06:30:58 PDT
Could you please verify that this change doesn't add even more breakage to the 301/302 handling?

That 301/302-redirected POSTs are rewritten to GET is a bug as per RFC 2616. Please do not extend it to additional methods.
Comment 33 Julian Reschke 2010-09-20 07:01:05 PDT
Initial tests show that the browser doesn't follow redirects anymore (my test just uses a folder name with missing trailing slash on Apache httpd/moddav, which causes a 301 redirect).

Also, it appears that the URIs constructed for PUT and DELETE aren't correct; they use the format used for GET.
Comment 34 Jonas Sicking (:sicking) PTO Until July 5th 2010-09-20 09:25:25 PDT
Julian, please file separate bugs and mark them dependent on this one. And do include exact descriptions of expected behavior and actual behavior, ideally in the form of test cases.
Comment 35 Julian Reschke 2010-09-20 09:50:15 PDT
(In reply to comment #34)
> Julian, please file separate bugs and mark them dependent on this one. And do
> include exact descriptions of expected behavior and actual behavior, ideally in
> the form of test cases.

Jonas, I'll try, but this may take some time. Optimally, the feature would have been added *including* test coverage right?

Can I ask to put this on hold so it doesn't get into FF4 without more review?
Comment 36 Jonas Sicking (:sicking) PTO Until July 5th 2010-09-20 09:53:27 PDT
Given that I don't understand what behavior you want or is seeing, I'm not really inclined to back anything out. New bugs would help with this understanding.

However given that we're just reusing existing code for PUT/DELETE handling I doubt any new breakage has been introduced.
Comment 37 Jonas Sicking (:sicking) PTO Until July 5th 2010-09-20 09:55:16 PDT
Oh, and there was certainly test coverage included in the patch. See attachment 471424 [details] [diff] [review] as well as the links in comment 25
Comment 38 Julian Reschke 2010-09-20 10:11:24 PDT
The thing that I wanted to confirm -- as expressed in https://bugzilla.mozilla.org/show_bug.cgi?id=583288#c32 -- that the browser does NOT reuse the known-to-be-broken redirect handling (that is, that is redirected PUT or DELETE is not rewritten to GET).

I tried, and found that at least on my machine, POST redirects currently are not followed at all. I fully realize that if it was totally broken more people would already have complained; so this needs a bit more investigation that I'll try to do tomorrow.

I'll have a look at the actual test cases tomorrow, thanks for the pointer.
Comment 39 Jonas Sicking (:sicking) PTO Until July 5th 2010-09-20 10:36:04 PDT
Also, do file new bugs. We're not planning to take any further actions in this one.
Comment 40 Julian Reschke 2010-09-21 06:47:11 PDT
I have confirmed that form submission with PUT/DELETE does the incorrect redirect handling (rewriting the method to GET). As this appears to be a general problem, for now I have only reported this as problem with XHR: see https://bugzilla.mozilla.org/show_bug.cgi?id=598304.
Comment 41 Julian Reschke 2010-09-30 01:21:28 PDT
Note that with http://www.w3.org/Bugs/Public/show_bug.cgi?id=10671, support for PUT and DELETE in forms has been removed from HTML5.
Comment 44 j.j. 2010-09-30 19:34:52 PDT
Shouldn't this be a WONTFIX to avoid confusion?
Can be reopend if "someday" arrives.
Comment 45 Mounir Lamouri (:mounir) 2010-09-30 20:55:19 PDT
(In reply to comment #44)
> Shouldn't this be a WONTFIX to avoid confusion?
> Can be reopend if "someday" arrives.

I have no strong opinions. I just know that if someone want to implement that in the future, they might look at open bugs before filing one but very unlikely going to look at closed bugs... I just want to prevent someone else wasting some time on that...
Comment 47 Mike Beltzner [:beltzner, not reading bugmail] 2011-02-23 10:43:00 PST
Comment on attachment 470047 [details] [diff] [review]
Enables client to force caching no matter their request type.

(bookkeeping)
Comment 48 :Ehsan Akhgari 2011-03-22 13:41:58 PDT
Is this WONTFIX now?
Comment 49 Mounir Lamouri (:mounir) 2011-03-23 03:30:57 PDT
Yes.

Note You need to log in before you can comment on or make changes to this bug.