Skip to content

Commit

Permalink
Strictly validate annotations
Browse files Browse the repository at this point in the history
  • Loading branch information
ansd committed Sep 18, 2024
1 parent cd600be commit 2d246db
Show file tree
Hide file tree
Showing 6 changed files with 34 additions and 22 deletions.
8 changes: 4 additions & 4 deletions deps/amqp10_client/test/system_SUITE.erl
Original file line number Diff line number Diff line change
Expand Up @@ -348,8 +348,8 @@ roundtrip(OpenConf, Body) ->
Msg0 = amqp10_msg:new(<<"my-tag">>, Body, true),
Msg1 = amqp10_msg:set_application_properties(#{"a_key" => "a_value"}, Msg0),
Msg2 = amqp10_msg:set_properties(Props, Msg1),
Msg = amqp10_msg:set_message_annotations(#{<<"x-key">> => "x-value",
<<"x_key">> => "x_value"}, Msg2),
Msg = amqp10_msg:set_message_annotations(#{<<"x-key 1">> => "value 1",
<<"x-key 2">> => "value 2"}, Msg2),
ok = amqp10_client:send_msg(Sender, Msg),
ok = amqp10_client:detach_link(Sender),
await_link(Sender, {detached, normal}, link_detach_timeout),
Expand All @@ -364,8 +364,8 @@ roundtrip(OpenConf, Body) ->
% ct:pal(?LOW_IMPORTANCE, "roundtrip message Out: ~tp~nIn: ~tp~n", [OutMsg, Msg]),
?assertMatch(Props, amqp10_msg:properties(OutMsg)),
?assertEqual(#{<<"a_key">> => <<"a_value">>}, amqp10_msg:application_properties(OutMsg)),
?assertMatch(#{<<"x-key">> := <<"x-value">>,
<<"x_key">> := <<"x_value">>}, amqp10_msg:message_annotations(OutMsg)),
?assertMatch(#{<<"x-key 1">> := <<"value 1">>,
<<"x-key 2">> := <<"value 2">>}, amqp10_msg:message_annotations(OutMsg)),
?assertEqual([Body], amqp10_msg:body(OutMsg)),
ok.

Expand Down
18 changes: 14 additions & 4 deletions deps/amqp10_common/src/amqp10_framing.erl
Original file line number Diff line number Diff line change
Expand Up @@ -122,11 +122,11 @@ decode({described, Descriptor, {map, Fields} = Type}) ->
#'v1_0.application_properties'{} ->
#'v1_0.application_properties'{content = decode_map(Fields)};
#'v1_0.delivery_annotations'{} ->
#'v1_0.delivery_annotations'{content = decode_map(Fields)};
#'v1_0.delivery_annotations'{content = decode_annotations(Fields)};
#'v1_0.message_annotations'{} ->
#'v1_0.message_annotations'{content = decode_map(Fields)};
#'v1_0.message_annotations'{content = decode_annotations(Fields)};
#'v1_0.footer'{} ->
#'v1_0.footer'{content = decode_map(Fields)};
#'v1_0.footer'{content = decode_annotations(Fields)};
#'v1_0.amqp_value'{} ->
#'v1_0.amqp_value'{content = Type};
Else ->
Expand All @@ -149,6 +149,16 @@ decode(Other) ->
decode_map(Fields) ->
[{decode(K), decode(V)} || {K, V} <- Fields].

%% "The annotations type is a map where the keys are restricted to be of type symbol
%% or of type ulong. All ulong keys, and all symbolic keys except those beginning
%% with "x-" are reserved." [3.2.10]
%% Since we already parse annotations here and neither the client nor server uses
%% reserved keys, we perform strict validation and crash if any reserved keys are used.
decode_annotations(Fields) ->
lists:map(fun({{symbol, <<"x-", _/binary>>} = K, V}) ->
{K, decode(V)}
end, Fields).

-spec encode_described(list | map | binary | annotations | '*',
non_neg_integer(),
amqp10_frame()) ->
Expand Down Expand Up @@ -216,7 +226,7 @@ pprint(Other) -> Other.
-include_lib("eunit/include/eunit.hrl").

encode_decode_test_() ->
Data = [{{utf8, <<"k">>}, {binary, <<"v">>}}],
Data = [{{symbol, <<"x-my key">>}, {binary, <<"my value">>}}],
Test = fun(M) -> [M] = decode_bin(iolist_to_binary(encode_bin(M))) end,
[
fun() -> Test(#'v1_0.application_properties'{content = Data}) end,
Expand Down
11 changes: 9 additions & 2 deletions deps/amqp10_common/test/prop_SUITE.erl
Original file line number Diff line number Diff line change
Expand Up @@ -412,14 +412,21 @@ footer_section() ->

annotations() ->
?LET(KvList,
list({oneof([amqp_symbol(),
amqp_ulong()]),
list({non_reserved_annotation_key(),
prefer_simple_type()}),
begin
KvList1 = lists:uniq(fun({K, _V}) -> K end, KvList),
lists:filter(fun({_K, V}) -> V =/= null end, KvList1)
end).

non_reserved_annotation_key() ->
{symbol, ?LET(L,
?SIZED(Size, resize(Size * 10, list(ascii_char()))),
begin
Bin = list_to_binary(L) ,
<<"x-", Bin/binary>>
end)}.

sequence_no() ->
amqp_uint().

Expand Down
6 changes: 3 additions & 3 deletions deps/rabbit/test/amqp_client_SUITE.erl
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,7 @@ reliable_send_receive_with_outcomes(QType, Config) ->
Outcomes = [
accepted,
modified,
{modified, true, false, #{<<"fruit">> => <<"banana">>}},
{modified, true, false, #{<<"x-fruit">> => <<"banana">>}},
{modified, false, true, #{}},
rejected,
released
Expand Down Expand Up @@ -1124,7 +1124,7 @@ amqp_amqpl(QType, Config) ->
#{"my int" => -2},
amqp10_msg:new(<<>>, Body1, true)))),
%% Send with footer
Footer = #'v1_0.footer'{content = [{{symbol, <<"my footer">>}, {ubyte, 255}}]},
Footer = #'v1_0.footer'{content = [{{symbol, <<"x-my footer">>}, {ubyte, 255}}]},
ok = amqp10_client:send_msg(
Sender,
amqp10_msg:from_amqp_records(
Expand Down Expand Up @@ -5155,7 +5155,7 @@ footer_checksum(FooterOpt, Config) ->
priority => 7,
ttl => 100_000},
amqp10_msg:set_delivery_annotations(
#{"a" => "b"},
#{"x-a" => "b"},
amqp10_msg:set_message_annotations(
#{"x-string" => "string-value",
"x-int" => 3,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -298,10 +298,10 @@ module Test =
use c = connectAnon uri
let sender, receiver = senderReceiver c "test" "/queues/message_annotations"
let ann = MessageAnnotations()
let k1 = Symbol "key1"
let k2 = Symbol "key2"
ann.[Symbol "key1"] <- "value1"
ann.[Symbol "key2"] <- "value2"
let k1 = Symbol "x-key1"
let k2 = Symbol "x-key2"
ann.[Symbol "x-key1"] <- "value1"
ann.[Symbol "x-key2"] <- "value2"
let m = new Message("testing annotations", MessageAnnotations = ann)
sender.Send m
let m' = receive receiver
Expand Down
5 changes: 0 additions & 5 deletions deps/rabbit/test/mc_unit_SUITE.erl
Original file line number Diff line number Diff line change
Expand Up @@ -524,8 +524,6 @@ amqp_amqpl(_Config) ->
durable = true},
MAC = [
{{symbol, <<"x-stream-filter">>}, {utf8, <<"apple">>}},
thead2(list, [utf8(<<"l">>)]),
thead2(map, [{utf8(<<"k">>), utf8(<<"v">>)}]),
thead2('x-list', list, [utf8(<<"l">>)]),
thead2('x-map', map, [{utf8(<<"k">>), utf8(<<"v">>)}])
],
Expand Down Expand Up @@ -591,9 +589,6 @@ amqp_amqpl(_Config) ->
?assertMatch(#'P_basic'{expiration = <<"20000">>}, Props),

?assertMatch({_, longstr, <<"apple">>}, header(<<"x-stream-filter">>, HL)),
%% these are not coverted as not x- headers
?assertEqual(undefined, header(<<"list">>, HL)),
?assertEqual(undefined, header(<<"map">>, HL)),
?assertMatch({_ ,array, [{longstr,<<"l">>}]}, header(<<"x-list">>, HL)),
?assertMatch({_, table, [{<<"k">>,longstr,<<"v">>}]}, header(<<"x-map">>, HL)),

Expand Down

0 comments on commit 2d246db

Please sign in to comment.