Patch for packet_parser.c to Include HTTP Methods Defined in RFC 2518 / WebDAV

[Jan Lehnardt-3]

Dear OTP Team,

the attached patch adds additional HTTP methods to packet_parser.c.
The methods are defined in RFC 2518 / WebDAV. While not part of the
HTTP/1.1 RFC 2616, these methods are useful when implementing
a WebDAV (or similar) server in Erlang.

I work on CouchDB. We borrowed the COPY and MOVE methods
(but not the others) from RFC 2518 to allow additional operations on
resources on the server. Until now, a parsed packet will include a
list /  string as the HTTP method if the method is not defined in
packet_parser.c and as an atom otherwise.

CouchDB uses this code to mitigate the issue:

   % Non standard HTTP verbs aren't atoms (COPY, MOVE etc) so convert  
when
   % possible (if any module references the atom, then it's existing).
   Meth -> couch_util:to_existing_atom(Meth)

couch_util:to_existing_atom/1 handles the case:

   % works like list_to_existing_atom, except can be list or binary  
and it
   % gives you the original value instead of an error if no existing  
atom.
   to_existing_atom(V) when is_list(V)->
       try list_to_existing_atom(V) catch _ -> V end;
   to_existing_atom(V) when is_binary(V)->
       try list_to_existing_atom(?b2l(V)) catch _ -> V end;
   to_existing_atom(V) when is_atom(V)->
       V.

CouchDB (and other implementors of HTTP extending standards) would
benefit from the inclusion of the methods in RFC 2518. While CouchDB
only needs COPY and MOVE (for now), adding the complete suite of
additional HTTP methods seems sensible. Please consider the patch
for inclusion into the next OTP release.

The patch is made against the preview release R13A but it equally
applies to R12B-5.

Cheers
Jan
--


--- a/erts/emulator/beam/packet_parser.c 2009-02-09 00:14:24.000000000  
+0100
+++ b/erts/emulator/beam/packet_parser.c 2009-02-09 00:12:56.000000000  
+0100
@@ -139,6 +139,13 @@
      "PUT",
      "DELETE",
      "TRACE",
+    "PROPFIND",
+    "PROPPATCH",
+    "MKCOL",
+    "COPY",
+    "MOVE",
+    "LOCK",
+    "UNLOCK",
      NULL
  };

_______________________________________________
erlang-patches mailing list
erlang-patches@...
http://www.erlang.org/mailman/listinfo/erlang-patches

[Sverker Eriksson]

Jan Lehnardt wrote:

> [...]
> CouchDB (and other implementors of HTTP extending standards) would
> benefit from the inclusion of the methods in RFC 2518. While CouchDB
> only needs COPY and MOVE (for now), adding the complete suite of
> additional HTTP methods seems sensible. Please consider the patch
> for inclusion into the next OTP release.
>  
> --- a/erts/emulator/beam/packet_parser.c 2009-02-09 00:14:24.000000000  
> +0100
> +++ b/erts/emulator/beam/packet_parser.c 2009-02-09 00:12:56.000000000  
> +0100
> @@ -139,6 +139,13 @@
>      "PUT",
>      "DELETE",
>      "TRACE",
> +    "PROPFIND",
> +    "PROPPATCH",
> +    "MKCOL",
> +    "COPY",
> +    "MOVE",
> +    "LOCK",
> +    "UNLOCK",
>       NULL

What do you think about the risk of applications that may break
expecting any of these methods as strings and not as atoms?

/Sverker, Erlang/OTP

_______________________________________________
erlang-patches mailing list
erlang-patches@...
http://www.erlang.org/mailman/listinfo/erlang-patches

[Jan Lehnardt-3]

On 13 Feb 2009, at 20:08, Sverker Eriksson wrote:

> Jan Lehnardt wrote:
>> [...]
>> CouchDB (and other implementors of HTTP extending standards) would
>> benefit from the inclusion of the methods in RFC 2518. While CouchDB
>> only needs COPY and MOVE (for now), adding the complete suite of
>> additional HTTP methods seems sensible. Please consider the patch
>> for inclusion into the next OTP release.
>>  --- a/erts/emulator/beam/packet_parser.c 2009-02-09  
>> 00:14:24.000000000  +0100
>> +++ b/erts/emulator/beam/packet_parser.c 2009-02-09  
>> 00:12:56.000000000  +0100
>> @@ -139,6 +139,13 @@
>>     "PUT",
>>     "DELETE",
>>     "TRACE",
>> +    "PROPFIND",
>> +    "PROPPATCH",
>> +    "MKCOL",
>> +    "COPY",
>> +    "MOVE",
>> +    "LOCK",
>> +    "UNLOCK",
>>      NULL
>
>
> What do you think about the risk of applications that may break  
> expecting any of these methods as strings and not as atoms?

Good point. I'm not aware of any Erlang WebDAV server
but I do not constantly monitor the application space and
I'm relatively new to Erlang (2 years now). I certainly have
no idea about internal applications that use the WebDAV
protocol or use any other non-standard HTTP extensions.

I found http://code.google.com/p/webdav4erlang/ which
doesn't have any code.

I'd argue that if nobody has stepped up to "fix" this problem,
as it is a relatively simple patch, this hasn't gotten a lot of
attention at all.

An inquiry on erlang-questions@ might be a good idea.

If really needed, it might be worth the time to make the
old or the new code optional. With say,

     inet:setopts(Socket, [{packet, http}]), % http + webdav, this patch
     inet:setopts(Socket, [{packet, http_strict}]), % RFC 2616 only,  
old behaviour

or

     inet:setopts(Socket, [{packet, http}]), % RFC 2616 only, old  
behaviour
     inet:setopts(Socket, [{packet, webdav}]), % RFC 2518, this patch

If at all, I'd go for the first option (the naming of the PacketType
atom is just a suggestion). Legacy  applications (the number
I believe to be in the single digits) can be fixed by either removing
their special handling of the non-standard methods or
using `http_strict`.

The second option is more defensive for legacy applications
and would require us to patch MochiWeb (and hope for acceptance).
I don't know how feasible or not this is.



Cheers
Jan
--

_______________________________________________
erlang-patches mailing list
erlang-patches@...
http://www.erlang.org/mailman/listinfo/erlang-patches

[Jan Lehnardt-3]

On 14 Feb 2009, at 14:17, Jan Lehnardt wrote:
>
> Good point. I'm not aware of any Erlang WebDAV server
> but I do not constantly monitor the application space and
> I'm relatively new to Erlang (2 years now). I certainly have
> no idea about internal applications that use the WebDAV
> protocol or use any other non-standard HTTP extensions.

A little more digging.

YAWS has a WevDAV extension and it uses this code to
solve the issue:

call_method(Method, CliSock, Req, H) ->
     case Method of
         F when atom(F) ->
             ?MODULE:F(CliSock, Req, H);
         L when list(L) ->
             handle_extension_method(L, CliSock, Req, H)
     end.

(YAWS 1.79 source src/yaws_server.erl line 1155 ff.)

YAWS hence, wouldn't break. Maybe because Klacke
was smart enough to foresee that this is getting fixed
eventually :)

--

Erlware includes a WebDAV module and as far as I
understand*, it does its own http parsing and always
uses lists / strings, not atoms for methods.

* http://git.erlware.org/web?p=crary_dav.git;a=blob_plain;f=crary_dav/src/crary_dav.erl;hb=HEAD 
  and
http://git.erlware.org/web?p=crary.git;a=blob_plain;f=src/crary_sock.erl;hb=HEAD

Cheers,
Jan
--

_______________________________________________
erlang-patches mailing list
erlang-patches@...
http://www.erlang.org/mailman/listinfo/erlang-patches

[Oscar Hellström-2]

Hi,

Firstly, I fail to understand why this is an issue that need to be
"fixed"? Except the increased size and a few more CPU cycles when
matching (which should quite small any way) why is it it a problem
having the methods as strings? Personally I wouldn't mind if *all*
methods would have been strings since mixing types makes programming a
bit bloated, but this obviously can't be changed.

Anyway, for the legacy discussion. Looking at the yaws code, your
assumption seems wrong. In handle_extension_method/4 the code assumes
that the extension method is a string, at least in the rather old
version that I looked at. It would certainly be possible to add clause
before the last one such as:
handle_extension_method(Method, CliSock, Req, Head) when is_atom(Methed) ->
    handle_extension_method(atom_to_list(Method), CliSock, Req, Head);
but without this, it looks to me as if yaws would fail.

Jan Lehnardt wrote:

> On 14 Feb 2009, at 14:17, Jan Lehnardt wrote:
>  
>> Good point. I'm not aware of any Erlang WebDAV server
>> but I do not constantly monitor the application space and
>> I'm relatively new to Erlang (2 years now). I certainly have
>> no idea about internal applications that use the WebDAV
>> protocol or use any other non-standard HTTP extensions.
>>    
>
> A little more digging.
>
> YAWS has a WevDAV extension and it uses this code to
> solve the issue:
>
> call_method(Method, CliSock, Req, H) ->
>      case Method of
>          F when atom(F) ->
>              ?MODULE:F(CliSock, Req, H);
>          L when list(L) ->
>              handle_extension_method(L, CliSock, Req, H)
>      end.
>
> (YAWS 1.79 source src/yaws_server.erl line 1155 ff.)
>
> YAWS hence, wouldn't break. Maybe because Klacke
> was smart enough to foresee that this is getting fixed
> eventually :)
>
> --
>
> Erlware includes a WebDAV module and as far as I
> understand*, it does its own http parsing and always
> uses lists / strings, not atoms for methods.
>
> * http://git.erlware.org/web?p=crary_dav.git;a=blob_plain;f=crary_dav/src/crary_dav.erl;hb=HEAD 
>   and
> http://git.erlware.org/web?p=crary.git;a=blob_plain;f=src/crary_sock.erl;hb=HEAD
>
> Cheers,
> Jan
> --
>
> _______________________________________________
> erlang-patches mailing list
> erlang-patches@...
> http://www.erlang.org/mailman/listinfo/erlang-patches
>  

--
Oscar Hellström, oscar@...
Office: [phone]
Mobile: [phone]
Erlang Training and Consulting
http://www.erlang-consulting.com/

_______________________________________________
erlang-patches mailing list
erlang-patches@...
http://www.erlang.org/mailman/listinfo/erlang-patches

[Jan Lehnardt-3]

Hi Oscar,

thanks for your comments.

On 16 Feb 2009, at 12:04, Oscar Hellström wrote:
> Firstly, I fail to understand why this is an issue that need to be
> "fixed"? Except the increased size and a few more CPU cycles when
> matching (which should quite small any way) why is it it a problem
> having the methods as strings? Personally I wouldn't mind if *all*
> methods would have been strings since mixing types makes programming a
> bit bloated, but this obviously can't be changed.

I wouldn't mind atoms or strings either, what's weird is that the API
returns both and clients have to deal with whether they use RFC 2616-
defined methods or not. Either asking explicitly for WebDAV methods
using `inet:setopts(Socket, [{packet, webdav}])` or just handling all
known HTTP extension methods as atoms (for backwards compatibility),
"fixes" that.

The reasoning behind the patch is less code (less code is good), not
speed.


> Anyway, for the legacy discussion. Looking at the yaws code, your
> assumption seems wrong. In handle_extension_method/4 the code assumes
> that the extension method is a string, at least in the rather old
> version that I looked at. It would certainly be possible to add clause
> before the last one such as:
> handle_extension_method(Method, CliSock, Req, Head) when  
> is_atom(Methed) ->
>    handle_extension_method(atom_to_list(Method), CliSock, Req, Head);
> but without this, it looks to me as if yaws would fail.

My version of `handle_extension_method()` reads:

handle_extension_method("PROPFIND", CliSock, Req, Head) ->
     'PROPFIND'(CliSock, Req, Head);
handle_extension_method("MKCOL", CliSock, Req, Head) ->
     'MKCOL'(CliSock, Req, Head);
handle_extension_method("MOVE", CliSock, Req, Head) ->
     'MOVE'(CliSock, Req, Head);
handle_extension_method("COPY", CliSock, Req, Head) ->
     'COPY'(CliSock, Req, Head);
handle_extension_method(_Method, CliSock, Req, Head) ->
     not_implemented(CliSock, Req, Head).

It gets called from:

call_method(Method, CliSock, Req, H) ->
     case Method of
         F when atom(F) ->
             ?MODULE:F(CliSock, Req, H);
         L when list(L) ->
             handle_extension_method(L, CliSock, Req, H)
     end.

I.e. "METHOD" is routed through `handle_extension_method()`
to call `'METHOD'()` in `call_method()` while 'METHOD' directly
calls `'METHOD'()`.

Again, Yaws 1.79.

So earlier versions of YAWS break.


Cheers
Jan
--

>
>
> Jan Lehnardt wrote:
>> On 14 Feb 2009, at 14:17, Jan Lehnardt wrote:
>>
>>> Good point. I'm not aware of any Erlang WebDAV server
>>> but I do not constantly monitor the application space and
>>> I'm relatively new to Erlang (2 years now). I certainly have
>>> no idea about internal applications that use the WebDAV
>>> protocol or use any other non-standard HTTP extensions.
>>>
>>
>> A little more digging.
>>
>> YAWS has a WevDAV extension and it uses this code to
>> solve the issue:
>>
>> call_method(Method, CliSock, Req, H) ->
>>     case Method of
>>         F when atom(F) ->
>>             ?MODULE:F(CliSock, Req, H);
>>         L when list(L) ->
>>             handle_extension_method(L, CliSock, Req, H)
>>     end.
>>
>> (YAWS 1.79 source src/yaws_server.erl line 1155 ff.)
>>
>> YAWS hence, wouldn't break. Maybe because Klacke
>> was smart enough to foresee that this is getting fixed
>> eventually :)
>>
>> --
>>
>> Erlware includes a WebDAV module and as far as I
>> understand*, it does its own http parsing and always
>> uses lists / strings, not atoms for methods.
>>
>> * http://git.erlware.org/web?p=crary_dav.git;a=blob_plain;f=crary_dav/src/crary_dav.erl;hb=HEAD
>>  and
>> http://git.erlware.org/web?p=crary.git;a=blob_plain;f=src/crary_sock.erl;hb=HEAD
>>
>> Cheers,
>> Jan
>> --
>>
>> _______________________________________________
>> erlang-patches mailing list
>> erlang-patches@...
>> http://www.erlang.org/mailman/listinfo/erlang-patches
>>
>
>
> --
> Oscar Hellström, oscar@...
> Office: [phone]
> Mobile: [phone]
> Erlang Training and Consulting
> http://www.erlang-consulting.com/
>
>

_______________________________________________
erlang-patches mailing list
erlang-patches@...
http://www.erlang.org/mailman/listinfo/erlang-patches

[Oscar Hellström-2]

Hi,

Jan Lehnardt wrote:
> Hi Oscar,
>
> thanks for your comments.
<snip>
> I wouldn't mind atoms or strings either, what's weird is that the API
> returns both and clients have to deal with whether they use RFC 2616-
> defined methods or not. Either asking explicitly for WebDAV methods
> using `inet:setopts(Socket, [{packet, webdav}])` or just handling all
> known HTTP extension methods as atoms (for backwards compatibility),
> "fixes" that.
>
> The reasoning behind the patch is less code (less code is good), not
> speed.

I agree, but since you still have to check if you got another method,
i.e. when is_atom(Method), you're just postponing the problem. Making
all methods atoms isn't possible due to DOS attacks and I'm sure that
there are a few other RFCs, more or less well known, that will add other
HTTP methods. For instance, an HTTP proxy might wan to implement the
CONNECT method... My approach is letting the receiving port of the code
transform all methods to strings before actually dealing with the request.

<snip>

> My version of `handle_extension_method()` reads:
>
> handle_extension_method("PROPFIND", CliSock, Req, Head) ->
>     'PROPFIND'(CliSock, Req, Head);
> handle_extension_method("MKCOL", CliSock, Req, Head) ->
>     'MKCOL'(CliSock, Req, Head);
> handle_extension_method("MOVE", CliSock, Req, Head) ->
>     'MOVE'(CliSock, Req, Head);
> handle_extension_method("COPY", CliSock, Req, Head) ->
>     'COPY'(CliSock, Req, Head);
> handle_extension_method(_Method, CliSock, Req, Head) ->
>     not_implemented(CliSock, Req, Head).
>
> It gets called from:
>
> call_method(Method, CliSock, Req, H) ->
>     case Method of
>         F when atom(F) ->
>             ?MODULE:F(CliSock, Req, H);
>         L when list(L) ->
>             handle_extension_method(L, CliSock, Req, H)
>     end.
>
> I.e. "METHOD" is routed through `handle_extension_method()`
> to call `'METHOD'()` in `call_method()` while 'METHOD' directly
> calls `'METHOD'()`.
>
> Again, Yaws 1.79.
>
> So earlier versions of YAWS break.

My bad, I think mine reads the same ;)

Cheers

--
Oscar Hellström, oscar@...
Office: [phone]
Mobile: [phone]
Erlang Training and Consulting
http://www.erlang-consulting.com/

_______________________________________________
erlang-patches mailing list
erlang-patches@...
http://www.erlang.org/mailman/listinfo/erlang-patches

[Jan Lehnardt-3]

On 16 Feb 2009, at 13:08, Oscar Hellström wrote:
> My approach is letting the receiving port of the code
> transform all methods to strings before actually dealing with the  
> request.

The whole idea of the patch is that there should be
no extra code to do this :) If there are more methods
that should be included, those who need it should
speak up.

Maybe `inet:setopts(Socket, [{packet, http_methods_as_lists}])`
is some middle ground?

(Note that the atom name is just to make the purpose clear :)

Cheers
Jan
--

_______________________________________________
erlang-patches mailing list
erlang-patches@...
http://www.erlang.org/mailman/listinfo/erlang-patches