From 7f653885398d6b65b86972e6fa361411132109e7 Mon Sep 17 00:00:00 2001 From: Daniel Widgren Date: Mon, 11 May 2026 20:43:07 +0200 Subject: [PATCH 1/2] fix(nova_file_controller): prevent path traversal in static file routes User-supplied path segments were joined onto the served root without canonicalisation, so a request containing .. (or %2E%2E once decoded) could escape the served directory. The controller now decodes each segment, rejects absolute paths, and resolves . and .. against a zero-depth floor; any attempt to climb above the root returns 403. Also unblocks safe URL-decoding of segments, so UTF-8 filenames served from a wildcard route work as expected. --- src/controllers/nova_file_controller.erl | 69 ++++++++++++++---- test/nova_file_controller_tests.erl | 89 ++++++++++++++++++++++++ 2 files changed, 146 insertions(+), 12 deletions(-) create mode 100644 test/nova_file_controller_tests.erl diff --git a/src/controllers/nova_file_controller.erl b/src/controllers/nova_file_controller.erl index 4b91e0c..3a247e3 100644 --- a/src/controllers/nova_file_controller.erl +++ b/src/controllers/nova_file_controller.erl @@ -49,19 +49,23 @@ get_dir(#{extra_state := #{pathinfo := Pathinfo, static := Dir, options := Optio %% This case will be invoked if a directory was set with wildcard - pathinfo will then %% contain the segments of the wildcard value Filepath = get_filepath(Dir), - Filepath0 = lists:foldl(fun(F, Acc) -> filename:join(Acc, binary_to_list(F)) end, Filepath, Pathinfo), - case filelib:is_dir(Filepath0) of - false -> - %% Check if it's a file - case filelib:is_file(Filepath0) of - true -> - %% It's a file - get_file(Req#{extra_state => #{static => {file, Filepath0}, options => Options}}); + case safe_join(Filepath, Pathinfo) of + {error, unsafe} -> + {status, 403}; + {ok, Filepath0} -> + case filelib:is_dir(Filepath0) of false -> - {status, 404} - end; - true -> - get_dir(Req#{extra_state => #{static => {dir, Filepath0}, options => Options}}) + %% Check if it's a file + case filelib:is_file(Filepath0) of + true -> + %% It's a file + get_file(Req#{extra_state => #{static => {file, Filepath0}, options => Options}}); + false -> + {status, 404} + end; + true -> + get_dir(Req#{extra_state => #{static => {dir, Filepath0}, options => Options}}) + end end; get_dir(#{path := Path, extra_state := #{static := Dir, options := Options}} = Req) -> Filepath = get_filepath(Dir), @@ -141,3 +145,44 @@ file_info(Filepath, Filename) -> _ -> undefined end. + +%% Resolve user-supplied path segments under Root and reject any result that +%% escapes Root (e.g. via "..", URL-encoded "..", or absolute segments). +safe_join(Root, []) -> + {ok, Root}; +safe_join(Root, Segments) -> + case decode_segments(Segments, []) of + {error, unsafe} -> + {error, unsafe}; + {ok, Decoded} -> + Relative = filename:join(Decoded), + case filename:pathtype(Relative) of + relative -> + case canonicalise(filename:split(Relative), []) of + unsafe -> {error, unsafe}; + [] -> {ok, Root}; + Parts -> {ok, filename:join([Root | Parts])} + end; + _ -> + {error, unsafe} + end + end. + +decode_segments([], Acc) -> + {ok, lists:reverse(Acc)}; +decode_segments([Seg | Rest], Acc) -> + case unicode:characters_to_list(uri_string:unquote(Seg)) of + Str when is_list(Str) -> decode_segments(Rest, [Str | Acc]); + _ -> {error, unsafe} + end. + +canonicalise([], Acc) -> + lists:reverse(Acc); +canonicalise([".." | _], []) -> + unsafe; +canonicalise([".." | Rest], [_ | Acc]) -> + canonicalise(Rest, Acc); +canonicalise(["." | Rest], Acc) -> + canonicalise(Rest, Acc); +canonicalise([Seg | Rest], Acc) -> + canonicalise(Rest, [Seg | Acc]). diff --git a/test/nova_file_controller_tests.erl b/test/nova_file_controller_tests.erl new file mode 100644 index 0000000..714b29b --- /dev/null +++ b/test/nova_file_controller_tests.erl @@ -0,0 +1,89 @@ +-module(nova_file_controller_tests). +-include_lib("eunit/include/eunit.hrl"). + +nova_file_controller_test_() -> + {foreach, fun setup/0, fun cleanup/1, + [ + fun serves_existing_file/1, + fun returns_404_for_missing_file/1, + fun rejects_literal_dotdot_traversal/1, + fun rejects_url_encoded_dotdot_traversal/1, + fun rejects_mixed_case_url_encoded_traversal/1, + fun rejects_url_encoded_slash_traversal/1, + fun rejects_absolute_path_segment/1, + fun serves_url_encoded_utf8_filename/1 + ]}. + +setup() -> + Dir = filename:join("/tmp", + "nova_file_controller_tests_" ++ + integer_to_list(erlang:unique_integer([positive]))), + Inside = filename:join(Dir, "inside"), + ok = filelib:ensure_dir(filename:join(Inside, "x")), + ok = file:write_file(filename:join(Inside, "ok.txt"), <<"safe">>), + UtfName = unicode:characters_to_list("café.txt"), + ok = file:write_file(filename:join(Inside, UtfName), <<"unicode">>), + %% A sibling file outside the served dir, used as the traversal target. + ok = file:write_file(filename:join(Dir, "secret.txt"), <<"do not leak">>), + #{dir => Dir, inside => Inside}. + +cleanup(#{dir := Dir}) -> + os:cmd("rm -rf " ++ Dir), + ok. + +req(Inside, Pathinfo) -> + #{path => <<"/whatever">>, + headers => #{}, + extra_state => #{static => {dir, Inside}, + pathinfo => Pathinfo, + options => #{}}}. + +serves_existing_file(#{inside := Inside}) -> + fun() -> + Result = nova_file_controller:get_dir(req(Inside, [<<"ok.txt">>])), + ?assertMatch({sendfile, 200, _, _, _}, Result) + end. + +returns_404_for_missing_file(#{inside := Inside}) -> + fun() -> + Result = nova_file_controller:get_dir(req(Inside, [<<"missing.txt">>])), + ?assertEqual({status, 404}, Result) + end. + +rejects_literal_dotdot_traversal(#{inside := Inside}) -> + fun() -> + Result = nova_file_controller:get_dir(req(Inside, [<<"..">>, <<"secret.txt">>])), + ?assertEqual({status, 403}, Result) + end. + +rejects_url_encoded_dotdot_traversal(#{inside := Inside}) -> + fun() -> + Result = nova_file_controller:get_dir(req(Inside, [<<"%2E%2E">>, <<"secret.txt">>])), + ?assertEqual({status, 403}, Result) + end. + +rejects_mixed_case_url_encoded_traversal(#{inside := Inside}) -> + fun() -> + Result = nova_file_controller:get_dir(req(Inside, [<<"%2e%2E">>, <<"secret.txt">>])), + ?assertEqual({status, 403}, Result) + end. + +rejects_url_encoded_slash_traversal(#{inside := Inside}) -> + fun() -> + Result = nova_file_controller:get_dir(req(Inside, [<<"%2E%2E%2Fsecret.txt">>])), + ?assertEqual({status, 403}, Result) + end. + +rejects_absolute_path_segment(#{inside := Inside, dir := Dir}) -> + fun() -> + Abs = unicode:characters_to_binary(filename:join(Dir, "secret.txt")), + Result = nova_file_controller:get_dir(req(Inside, [Abs])), + ?assertEqual({status, 403}, Result) + end. + +serves_url_encoded_utf8_filename(#{inside := Inside}) -> + fun() -> + Encoded = unicode:characters_to_binary(uri_string:quote("café.txt")), + Result = nova_file_controller:get_dir(req(Inside, [Encoded])), + ?assertMatch({sendfile, 200, _, _, _}, Result) + end. From 91f02164c664fae87faa4da58ed6ec707586929b Mon Sep 17 00:00:00 2001 From: Daniel Widgren Date: Mon, 11 May 2026 20:47:24 +0200 Subject: [PATCH 2/2] test(nova_file_controller): expand traversal coverage Adds 13 tests covering depth-counting edge cases (chained .., mixed subdir/.., long traversal chains, going below root after descending), common bypass attempts (double URL encoding, backslash, empty segment, '...', NUL byte), and previously-untested positive paths (subdirectory serve, '.' segments, .. that resolves back inside root). --- test/nova_file_controller_tests.erl | 175 ++++++++++++++++++++++++---- 1 file changed, 155 insertions(+), 20 deletions(-) diff --git a/test/nova_file_controller_tests.erl b/test/nova_file_controller_tests.erl index 714b29b..1e0ce7e 100644 --- a/test/nova_file_controller_tests.erl +++ b/test/nova_file_controller_tests.erl @@ -4,14 +4,34 @@ nova_file_controller_test_() -> {foreach, fun setup/0, fun cleanup/1, [ + %% Happy paths fun serves_existing_file/1, fun returns_404_for_missing_file/1, + fun serves_file_in_subdirectory/1, + fun serves_url_encoded_utf8_filename/1, + fun single_dot_segment_serves_root/1, + fun dot_segment_in_middle_serves_file/1, + fun dotdot_then_redescend_serves_file/1, + + %% Direct traversal vectors fun rejects_literal_dotdot_traversal/1, fun rejects_url_encoded_dotdot_traversal/1, fun rejects_mixed_case_url_encoded_traversal/1, fun rejects_url_encoded_slash_traversal/1, fun rejects_absolute_path_segment/1, - fun serves_url_encoded_utf8_filename/1 + + %% Depth-counting bugs the canonicaliser must not have + fun rejects_chained_dotdot/1, + fun rejects_mixed_subdir_dotdot_escape/1, + fun rejects_long_traversal_chain/1, + fun rejects_dotdot_after_descending_back_to_root/1, + + %% Unusual inputs that should not be misread as traversal + fun double_encoded_dotdot_is_literal_not_traversal/1, + fun backslash_segment_is_literal_on_unix/1, + fun empty_segment_does_not_escape/1, + fun three_dots_is_literal_filename/1, + fun null_byte_segment_does_not_escape/1 ]}. setup() -> @@ -19,11 +39,13 @@ setup() -> "nova_file_controller_tests_" ++ integer_to_list(erlang:unique_integer([positive]))), Inside = filename:join(Dir, "inside"), - ok = filelib:ensure_dir(filename:join(Inside, "x")), + Sub = filename:join(Inside, "sub"), + ok = filelib:ensure_dir(filename:join(Sub, "x")), ok = file:write_file(filename:join(Inside, "ok.txt"), <<"safe">>), + ok = file:write_file(filename:join(Sub, "child.txt"), <<"child">>), UtfName = unicode:characters_to_list("café.txt"), ok = file:write_file(filename:join(Inside, UtfName), <<"unicode">>), - %% A sibling file outside the served dir, used as the traversal target. + %% Sibling file outside the served dir; the traversal target. ok = file:write_file(filename:join(Dir, "secret.txt"), <<"do not leak">>), #{dir => Dir, inside => Inside}. @@ -38,52 +60,165 @@ req(Inside, Pathinfo) -> pathinfo => Pathinfo, options => #{}}}. +%% ---- Happy paths ---------------------------------------------------------- + serves_existing_file(#{inside := Inside}) -> fun() -> - Result = nova_file_controller:get_dir(req(Inside, [<<"ok.txt">>])), - ?assertMatch({sendfile, 200, _, _, _}, Result) + ?assertMatch({sendfile, 200, _, _, _}, + nova_file_controller:get_dir(req(Inside, [<<"ok.txt">>]))) end. returns_404_for_missing_file(#{inside := Inside}) -> fun() -> - Result = nova_file_controller:get_dir(req(Inside, [<<"missing.txt">>])), - ?assertEqual({status, 404}, Result) + ?assertEqual({status, 404}, + nova_file_controller:get_dir(req(Inside, [<<"missing.txt">>]))) + end. + +serves_file_in_subdirectory(#{inside := Inside}) -> + fun() -> + ?assertMatch({sendfile, 200, _, _, _}, + nova_file_controller:get_dir(req(Inside, [<<"sub">>, <<"child.txt">>]))) end. +serves_url_encoded_utf8_filename(#{inside := Inside}) -> + fun() -> + Encoded = unicode:characters_to_binary(uri_string:quote("café.txt")), + ?assertMatch({sendfile, 200, _, _, _}, + nova_file_controller:get_dir(req(Inside, [Encoded]))) + end. + +single_dot_segment_serves_root(#{inside := Inside}) -> + fun() -> + %% "." canonicalises to no segments; falls into the directory clause. + %% list_dir is off by default, so a directory request returns 403. + %% We only assert it is NOT 200 and not a traversal sendfile. + Result = nova_file_controller:get_dir(req(Inside, [<<".">>])), + ?assert(Result =:= {status, 403} orelse element(1, Result) =:= status), + ?assertNotMatch({sendfile, 200, _, _, _}, Result) + end. + +dot_segment_in_middle_serves_file(#{inside := Inside}) -> + fun() -> + ?assertMatch({sendfile, 200, _, _, _}, + nova_file_controller:get_dir(req(Inside, [<<".">>, <<"ok.txt">>]))) + end. + +dotdot_then_redescend_serves_file(#{inside := Inside}) -> + fun() -> + %% sub/../ok.txt resolves to ok.txt at the root of the served dir. + ?assertMatch({sendfile, 200, _, _, _}, + nova_file_controller:get_dir( + req(Inside, [<<"sub">>, <<"..">>, <<"ok.txt">>]))) + end. + +%% ---- Direct traversal vectors --------------------------------------------- + rejects_literal_dotdot_traversal(#{inside := Inside}) -> fun() -> - Result = nova_file_controller:get_dir(req(Inside, [<<"..">>, <<"secret.txt">>])), - ?assertEqual({status, 403}, Result) + ?assertEqual({status, 403}, + nova_file_controller:get_dir(req(Inside, [<<"..">>, <<"secret.txt">>]))) end. rejects_url_encoded_dotdot_traversal(#{inside := Inside}) -> fun() -> - Result = nova_file_controller:get_dir(req(Inside, [<<"%2E%2E">>, <<"secret.txt">>])), - ?assertEqual({status, 403}, Result) + ?assertEqual({status, 403}, + nova_file_controller:get_dir(req(Inside, [<<"%2E%2E">>, <<"secret.txt">>]))) end. rejects_mixed_case_url_encoded_traversal(#{inside := Inside}) -> fun() -> - Result = nova_file_controller:get_dir(req(Inside, [<<"%2e%2E">>, <<"secret.txt">>])), - ?assertEqual({status, 403}, Result) + ?assertEqual({status, 403}, + nova_file_controller:get_dir(req(Inside, [<<"%2e%2E">>, <<"secret.txt">>]))) end. rejects_url_encoded_slash_traversal(#{inside := Inside}) -> fun() -> - Result = nova_file_controller:get_dir(req(Inside, [<<"%2E%2E%2Fsecret.txt">>])), - ?assertEqual({status, 403}, Result) + ?assertEqual({status, 403}, + nova_file_controller:get_dir(req(Inside, [<<"%2E%2E%2Fsecret.txt">>]))) end. rejects_absolute_path_segment(#{inside := Inside, dir := Dir}) -> fun() -> Abs = unicode:characters_to_binary(filename:join(Dir, "secret.txt")), - Result = nova_file_controller:get_dir(req(Inside, [Abs])), + ?assertEqual({status, 403}, + nova_file_controller:get_dir(req(Inside, [Abs]))) + end. + +%% ---- Depth-counting bugs -------------------------------------------------- + +rejects_chained_dotdot(#{inside := Inside}) -> + fun() -> + ?assertEqual({status, 403}, + nova_file_controller:get_dir( + req(Inside, [<<"..">>, <<"..">>, <<"secret.txt">>]))) + end. + +rejects_mixed_subdir_dotdot_escape(#{inside := Inside}) -> + fun() -> + %% sub/../../secret.txt: enter sub, pop to root, then pop above root. + ?assertEqual({status, 403}, + nova_file_controller:get_dir( + req(Inside, [<<"sub">>, <<"..">>, <<"..">>, <<"secret.txt">>]))) + end. + +rejects_long_traversal_chain(#{inside := Inside}) -> + fun() -> + Chain = lists:duplicate(20, <<"..">>) ++ [<<"etc">>, <<"passwd">>], + ?assertEqual({status, 403}, + nova_file_controller:get_dir(req(Inside, Chain))) + end. + +rejects_dotdot_after_descending_back_to_root(#{inside := Inside}) -> + fun() -> + %% sub/../../x: depth went 0->1->0->-1, must reject. + ?assertEqual({status, 403}, + nova_file_controller:get_dir( + req(Inside, [<<"sub">>, <<"..">>, <<"..">>, <<"x">>]))) + end. + +%% ---- Unusual inputs ------------------------------------------------------- + +double_encoded_dotdot_is_literal_not_traversal(#{inside := Inside}) -> + fun() -> + %% %252E%252E decodes once to %2E%2E and stays a literal filename. + %% Must NOT be treated as ".." and must NOT serve a sibling file. + Result = nova_file_controller:get_dir( + req(Inside, [<<"%252E%252E">>, <<"secret.txt">>])), + ?assertEqual({status, 404}, Result) + end. + +backslash_segment_is_literal_on_unix(#{inside := Inside}) -> + fun() -> + %% On Unix, \ is a literal character, not a path separator, + %% so "..\\secret.txt" is a single weird filename, not traversal. + Result = nova_file_controller:get_dir( + req(Inside, [<<"..\\secret.txt">>])), + ?assertEqual({status, 404}, Result) + end. + +empty_segment_does_not_escape(#{inside := Inside}) -> + fun() -> + %% An empty segment turns the joined path absolute ("/secret.txt"), + %% which must be rejected outright. + Result = nova_file_controller:get_dir( + req(Inside, [<<>>, <<"secret.txt">>])), ?assertEqual({status, 403}, Result) end. -serves_url_encoded_utf8_filename(#{inside := Inside}) -> +three_dots_is_literal_filename(#{inside := Inside}) -> fun() -> - Encoded = unicode:characters_to_binary(uri_string:quote("café.txt")), - Result = nova_file_controller:get_dir(req(Inside, [Encoded])), - ?assertMatch({sendfile, 200, _, _, _}, Result) + %% "..." is not a traversal marker; it is a normal filename + %% that happens not to exist. Must 404, never 403 (false positive) + %% and never 200. + Result = nova_file_controller:get_dir(req(Inside, [<<"...">>])), + ?assertEqual({status, 404}, Result) + end. + +null_byte_segment_does_not_escape(#{inside := Inside}) -> + fun() -> + %% A NUL byte must not let the request escape; either 404 or 403 + %% is acceptable, but it must not serve the sibling file. + Result = nova_file_controller:get_dir( + req(Inside, [<<"ok.txt", 0, "secret.txt">>])), + ?assertNotMatch({sendfile, 200, _, _, _}, Result) end.