From f077c711a8d455922e099a3e69de07054e47c242 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lo=C3=AFc=20Hoguin?= <essen@ninenines.eu>
Date: Mon, 17 Dec 2012 12:32:17 +0100
Subject: Fix cowboy_http:cookie_to_iodata/3

No more trying to quote, this is still completely broken everywhere.
---
 src/cowboy_http.erl | 63 ++++++++++++++++++++++++++++++++++-------------------
 src/cowboy_req.erl  |  6 +++++
 2 files changed, 47 insertions(+), 22 deletions(-)

(limited to 'src')

diff --git a/src/cowboy_http.erl b/src/cowboy_http.erl
index 44984e0..4754649 100644
--- a/src/cowboy_http.erl
+++ b/src/cowboy_http.erl
@@ -815,9 +815,20 @@ ce_identity(Data) ->
 -spec cookie_to_iodata(iodata(), iodata(), cowboy_req:cookie_opts())
 	-> iodata().
 cookie_to_iodata(Name, Value, Opts) ->
+	case binary:match(Name, [<<$=>>, <<$,>>, <<$;>>,
+			<<$\s>>, <<$\t>>, <<$\r>>, <<$\n>>, <<$\013>>, <<$\014>>]) of
+		nomatch -> ok
+	end,
+	case binary:match(Value, [<<$,>>, <<$;>>,
+			<<$\s>>, <<$\t>>, <<$\r>>, <<$\n>>, <<$\013>>, <<$\014>>]) of
+		nomatch -> ok
+	end,
 	MaxAgeBin = case lists:keyfind(max_age, 1, Opts) of
 		false -> <<>>;
-		{_, MaxAge} when is_integer(MaxAge), MaxAge >= 0 ->
+		{_, 0} ->
+			%% MSIE requires an Expires date in the past to delete a cookie.
+			<<"; Expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0">>;
+		{_, MaxAge} when is_integer(MaxAge), MaxAge > 0 ->
 			UTC = calendar:universal_time(),
 			Secs = calendar:datetime_to_gregorian_seconds(UTC),
 			Expires = calendar:gregorian_seconds_to_datetime(Secs + MaxAge),
@@ -826,11 +837,11 @@ cookie_to_iodata(Name, Value, Opts) ->
 	end,
 	DomainBin = case lists:keyfind(domain, 1, Opts) of
 		false -> <<>>;
-		{_, Domain} -> [<<"; Domain=">>, quote(Domain)]
+		{_, Domain} -> [<<"; Domain=">>, Domain]
 	end,
 	PathBin = case lists:keyfind(path, 1, Opts) of
 		false -> <<>>;
-		{_, Path} -> [<<"; Path=">>, quote(Path)]
+		{_, Path} -> [<<"; Path=">>, Path]
 	end,
 	SecureBin = case lists:keyfind(secure, 1, Opts) of
 		false -> <<>>;
@@ -840,21 +851,9 @@ cookie_to_iodata(Name, Value, Opts) ->
 		false -> <<>>;
 		{_, true} -> <<"; HttpOnly">>
 	end,
-	[Name, <<"=">>, quote(Value), <<"; Version=1">>,
+	[Name, <<"=">>, Value, <<"; Version=1">>,
 		MaxAgeBin, DomainBin, PathBin, SecureBin, HttpOnlyBin].
 
--spec quote(binary()) -> binary().
-quote(Bin) ->
-	quote(Bin, << $" >>).
-
--spec quote(binary(), binary()) -> binary().
-quote(<<>>, Acc) ->
-	<< Acc/binary, $" >>;
-quote(<< $", Rest/bits >>, Acc) ->
-	quote(Rest, << Acc/binary, $\\, $" >>);
-quote(<< C, Rest/bits >>, Acc) ->
-	quote(Rest, << Acc/binary, C >>).
-
 %% @doc Convert an HTTP version tuple to its binary form.
 -spec version_to_binary(version()) -> binary().
 version_to_binary({1, 1}) -> <<"HTTP/1.1">>;
@@ -1160,14 +1159,14 @@ cookie_to_iodata_test_() ->
 	Tests = [
 		{<<"Customer">>, <<"WILE_E_COYOTE">>,
 			[{http_only, true}, {domain, <<"acme.com">>}],
-			<<"Customer=\"WILE_E_COYOTE\"; Version=1; "
-				"Domain=\"acme.com\"; HttpOnly">>},
+			<<"Customer=WILE_E_COYOTE; Version=1; "
+				"Domain=acme.com; HttpOnly">>},
 		{<<"Customer">>, <<"WILE_E_COYOTE">>,
 			[{path, <<"/acme">>}],
-			<<"Customer=\"WILE_E_COYOTE\"; Version=1; Path=\"/acme\"">>},
+			<<"Customer=WILE_E_COYOTE; Version=1; Path=/acme">>},
 		{<<"Customer">>, <<"WILE_E_COYOTE">>,
 			[{path, <<"/acme">>}, {badoption, <<"negatory">>}],
-			<<"Customer=\"WILE_E_COYOTE\"; Version=1; Path=\"/acme\"">>}
+			<<"Customer=WILE_E_COYOTE; Version=1; Path=/acme">>}
 	],
 	[{R, fun() -> R = iolist_to_binary(cookie_to_iodata(N, V, O)) end}
 		|| {N, V, O, R} <- Tests].
@@ -1177,7 +1176,7 @@ cookie_to_iodata_max_age_test() ->
 		binary:split(iolist_to_binary(
 			cookie_to_iodata(N, V, O)), <<";">>, [global])
 	end,
-	[<<"Customer=\"WILE_E_COYOTE\"">>,
+	[<<"Customer=WILE_E_COYOTE">>,
 		<<" Version=1">>,
 		<<" Expires=", _/binary>>,
 		<<" Max-Age=111">>,
@@ -1186,13 +1185,33 @@ cookie_to_iodata_max_age_test() ->
 	case catch F(<<"Customer">>, <<"WILE_E_COYOTE">>, [{max_age, -111}]) of
 		{'EXIT', {{case_clause, {max_age, -111}}, _}} -> ok
 	end,
-	[<<"Customer=\"WILE_E_COYOTE\"">>,
+	[<<"Customer=WILE_E_COYOTE">>,
 		<<" Version=1">>,
 		<<" Expires=", _/binary>>,
 		<<" Max-Age=86417">>] = F(<<"Customer">>, <<"WILE_E_COYOTE">>,
 			 [{max_age, 86417}]),
 	ok.
 
+cookie_to_iodata_failures_test_() ->
+	F = fun(N, V) ->
+		try cookie_to_iodata(N, V, []) of
+			_ ->
+				false
+		catch _:_ ->
+			true
+		end
+	end,
+	Tests = [
+		{<<"Na=me">>, <<"Value">>},
+		{<<"Name;">>, <<"Value">>},
+		{<<"\r\name">>, <<"Value">>},
+		{<<"Name">>, <<"Value;">>},
+		{<<"Name">>, <<"\value">>}
+	],
+	[{iolist_to_binary(io_lib:format("{~p, ~p} failure", [N, V])),
+		fun() -> true = F(N, V) end}
+		|| {N, V} <- Tests].
+
 x_www_form_urlencoded_test_() ->
 	%% {Qs, Result}
 	Tests = [
diff --git a/src/cowboy_req.erl b/src/cowboy_req.erl
index dc98e30..b29d694 100644
--- a/src/cowboy_req.erl
+++ b/src/cowboy_req.erl
@@ -800,6 +800,12 @@ multipart_skip(Req) ->
 %% Response API.
 
 %% @doc Add a cookie header to the response.
+%%
+%% The cookie name cannot contain any of the following characters:
+%%   =,;\s\t\r\n\013\014
+%%
+%% The cookie value cannot contain any of the following characters:
+%%   ,; \t\r\n\013\014
 -spec set_resp_cookie(iodata(), iodata(), cookie_opts(), Req)
 	-> Req when Req::req().
 set_resp_cookie(Name, Value, Opts, Req) ->
-- 
cgit v1.2.3