From 290d5f24904c7a6f7e693318271406baa84d2b73 Mon Sep 17 00:00:00 2001
From: Marc Worrell <marc@worrell.nl>
Date: Tue, 4 Jan 2022 16:21:05 +0100
Subject: [PATCH] Return 400 instead of a crash with certain illegal query
 strings. (#29)

Fixes #28
---
 src/cowmachine.erl             |  3 +++
 src/cowmachine_util.erl        | 36 +++++++++++++++++++---------------
 test/cowmachine_util_tests.erl | 33 +++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 16 deletions(-)
 create mode 100644 test/cowmachine_util_tests.erl

diff --git a/src/cowmachine.erl b/src/cowmachine.erl
index dece76e..0075731 100644
--- a/src/cowmachine.erl
+++ b/src/cowmachine.erl
@@ -102,6 +102,9 @@ request_1(Controller, Req, Env, Options, Context) ->
         throw:invalid_percent_encoding ->
             log(#{ at => ?AT, level => error, code => 400, text => "Illegal percent encoding" }, Req),
             {stop, cowboy_req:reply(400, Req)};
+        throw:invalid_qs_name ->
+            log(#{ at => ?AT, level => error, code => 400, text => "Illegal query argument name" }, Req),
+            {stop, cowboy_req:reply(400, Req)};
         throw:Reason:Stacktrace ->
             log(#{ at => ?AT, level => error, code => 500, text => "Unexpected throw",
                    class => throw, reason => Reason,
diff --git a/src/cowmachine_util.erl b/src/cowmachine_util.erl
index 85279f6..cf687c0 100644
--- a/src/cowmachine_util.erl
+++ b/src/cowmachine_util.erl
@@ -281,35 +281,39 @@ parse_qs(<<>>) ->
 parse_qs(Qs) ->
     parse_qs_name(Qs, [], <<>>).
 
-parse_qs_name(<< $%, H, L, Rest/bits >>, Acc, Name) ->
+parse_qs_name(<< $%, H, L, Rest/binary >>, Acc, Name) ->
     C = (unhex(H) bsl 4 bor unhex(L)),
-    parse_qs_name(Rest, Acc, << Name/bits, C >>);
-parse_qs_name(<< $+, Rest/bits >>, Acc, Name) ->
-    parse_qs_name(Rest, Acc, << Name/bits, " " >>);
-parse_qs_name(<< $=, Rest/bits >>, Acc, Name) when Name =/= <<>> ->
+    parse_qs_name(Rest, Acc, << Name/binary, C >>);
+parse_qs_name(<< $+, Rest/binary >>, Acc, Name) ->
+    parse_qs_name(Rest, Acc, << Name/binary, " " >>);
+parse_qs_name(<< $=, _/binary >>, _Acc, <<>>) ->
+    throw(invalid_qs_name);
+parse_qs_name(<< $=, Rest/binary >>, Acc, Name) ->
     parse_qs_value(Rest, Acc, Name, <<>>);
-parse_qs_name(<< $&, Rest/bits >>, Acc, Name) ->
+parse_qs_name(<< $&, Rest/binary >>, Acc, Name) ->
     case Name of
         <<>> -> parse_qs_name(Rest, Acc, <<>>);
         _ -> parse_qs_name(Rest, [{Name, <<>>}|Acc], <<>>)
     end;
-parse_qs_name(<< C, Rest/bits >>, Acc, Name) when C =/= $%, C =/= $= ->
-    parse_qs_name(Rest, Acc, << Name/bits, C >>);
+parse_qs_name(<< C, Rest/binary >>, Acc, Name) when C =/= $% ->
+    parse_qs_name(Rest, Acc, << Name/binary, C >>);
 parse_qs_name(<<>>, Acc, Name) ->
     case Name of
         <<>> -> lists:reverse(Acc);
         _ -> lists:reverse([{Name, <<>>}|Acc])
-    end.
+    end;
+parse_qs_name(_Rest, _Acc, _Name) ->
+    throw(invalid_percent_encoding).
 
-parse_qs_value(<< $%, H, L, Rest/bits >>, Acc, Name, Value) ->
+parse_qs_value(<< $%, H, L, Rest/binary >>, Acc, Name, Value) ->
     C = (unhex(H) bsl 4 bor unhex(L)),
-    parse_qs_value(Rest, Acc, Name, << Value/bits, C >>);
-parse_qs_value(<< $+, Rest/bits >>, Acc, Name, Value) ->
-    parse_qs_value(Rest, Acc, Name, << Value/bits, " " >>);
-parse_qs_value(<< $&, Rest/bits >>, Acc, Name, Value) ->
+    parse_qs_value(Rest, Acc, Name, << Value/binary, C >>);
+parse_qs_value(<< $+, Rest/binary >>, Acc, Name, Value) ->
+    parse_qs_value(Rest, Acc, Name, << Value/binary, " " >>);
+parse_qs_value(<< $&, Rest/binary >>, Acc, Name, Value) ->
     parse_qs_name(Rest, [{Name, Value}|Acc], <<>>);
-parse_qs_value(<< C, Rest/bits >>, Acc, Name, Value) when C =/= $% ->
-    parse_qs_value(Rest, Acc, Name, << Value/bits, C >>);
+parse_qs_value(<< C, Rest/binary >>, Acc, Name, Value) when C =/= $% ->
+    parse_qs_value(Rest, Acc, Name, << Value/binary, C >>);
 parse_qs_value(<<>>, Acc, Name, Value) ->
     lists:reverse([{Name, Value}|Acc]);
 parse_qs_value(_Rest, _Acc, _Name, _Value) ->
diff --git a/test/cowmachine_util_tests.erl b/test/cowmachine_util_tests.erl
new file mode 100644
index 0000000..1b440fe
--- /dev/null
+++ b/test/cowmachine_util_tests.erl
@@ -0,0 +1,33 @@
+%% @author Marc Worrell <marc@worrell.nl>
+%% @hidden
+
+-module(cowmachine_util_tests).
+
+-include_lib("eunit/include/eunit.hrl").
+
+
+cowmachine_parse_test() ->
+    [ {<<"a">>, <<"b">>} ] = cowmachine_util:parse_qs(<<"a=b">>),
+    [ {<<"a">>, <<>>}, {<<"b">>, <<>>} ] = cowmachine_util:parse_qs(<<"a&b">>),
+    [ {<<"a">>, <<"b">>}, {<<"ab">>, <<"cd">>} ] = cowmachine_util:parse_qs(<<"a=b&ab=cd">>),
+    [ {<<"a b">>, <<"b c">>} ] = cowmachine_util:parse_qs(<<"a+b=b+c">>),
+    [ {<<"a b">>, <<"b c">>} ] = cowmachine_util:parse_qs(<<"a%20b=b%20c">>),
+    ok.
+
+cowmachine_parse_error_test() ->
+    error = try
+        cowmachine_util:parse_qs(<<"=b">>)
+    catch
+        throw:invalid_qs_name -> error
+    end,
+    error = try
+        cowmachine_util:parse_qs(<<"b%2">>)
+    catch
+        throw:invalid_percent_encoding -> error
+    end,
+    error = try
+        cowmachine_util:parse_qs(<<"a%2=b">>)
+    catch
+        throw:invalid_percent_encoding -> error
+    end,
+    ok.
\ No newline at end of file