Merge pull request #133 from zmstone/confine-idl-imports-to-cwd

avro_idl: confine IDL import paths

Author: mikpe

README.md

@@ -307,6 +307,35 @@ protocol Orders {
 Store = avro_schema_store:new([], ["schemas/orders.avdl"]),
 ```
 
+### Import directory confinement
+
+The default reader confines imports to a "root directory" — by default the
+directory of the top-level `.avdl` file passed to the schema store.
+Absolute paths and relative paths that escape the root through `..` are
+refused with `{error, {import_outside_root, Path}}`. `..` is allowed as
+long as the resolved path stays inside the root, so a layout that splits
+schemas across sibling directories still works:
+
+```text
+schemas/
+  common/types.avdl
+  orders/order.avdl    %% can import "../common/types.avdl"
+```
+
+To use that layout, point `rootdir` at the top of the tree so imports
+above the loaded file's own directory can still resolve:
+
+```erlang
+Store = avro_schema_store:new(
+          [], ["schemas/orders/order.avdl"],
+          [{rootdir, "schemas"}]).
+```
+
+Callers that need different resolution semantics (loading from arbitrary
+directories or from an in-memory store) can supply their own function
+via the `{read_fun, Fun}` option — that bypasses the path checks
+entirely. See the next section for an example.
+
 ## In-memory schema loading (no filesystem)
 
 For testing or embedded schemas, supply a custom `read_fun` that resolves

changelog.md

@@ -1,3 +1,18 @@
+* 2.11.1
+   - avro_idl: the default IDL `read_fun` now confines imports to a root
+     directory. Absolute paths and relative paths that escape the root
+     through `..` are refused with `{error, {import_outside_root, Path}}`,
+     but `..` is allowed as long as the resolved path stays under the
+     root. The root defaults to the top-level caller's `Cwd` and can be
+     overridden via the new `{rootdir, Dir}` option, supported by
+     `avro_idl:decode_schema/3`, `avro_idl:new_context/2`,
+     `avro_idl:str_to_avpr/3`, and the new
+     `avro_schema_store:new/3` / `import_file/3` / `import_files/3`
+     entry points. Callers that need different resolution semantics
+     (loading from arbitrary directories or from an in-memory store)
+     can still supply their own function via the existing
+     `{read_fun, Fun}` option, which bypasses path checks.
+
 * 2.11.0
    - Deleted jsone as dependency.
      When OTP release is 27 or later, the default JSON provider module is `json`, otherwise `jsone`.

src/avro_idl.erl

@@ -18,7 +18,9 @@
 -type read_fun() :: fun((Cwd :: string(), Path :: string()) ->
                             {ok, binary()} | {error, term()}).
 
--record(st, {cwd, read_fun :: read_fun()}).
+-record(st, {cwd :: string(),
+             rootdir :: string(),
+             read_fun :: read_fun()}).
 
 decode_schema(SchemaStr, Cwd) ->
     decode_schema(SchemaStr, Cwd, []).
@@ -40,16 +42,62 @@ decode_schema(SchemaStr, Cwd, Opts) ->
         end, Types1),
     avro:decode_schema(Types, [{ignore_bad_default_values, true}]).
 
--spec default_read_fun() -> read_fun().
-default_read_fun() ->
-    fun(Cwd, Path) -> file:read_file(filename:join(Cwd, Path)) end.
+%% @doc Default `read_fun' used when one is not supplied via options.
+%% Import paths are resolved relative to the importing file's directory
+%% (`Cwd'), but the resolved location must stay under `RootDir' — the
+%% top-level caller's `Cwd' unless overridden via the `rootdir' option.
+%% Absolute paths and relative paths that escape `RootDir' through `..'
+%% are refused with `{error, {import_outside_root, Path}}'. `..' within
+%% `RootDir' is allowed, so layouts like `orders/order.avdl' importing
+%% `../common/types.avdl' work as long as `common/' is also under
+%% `RootDir'. Callers that need different resolution semantics can
+%% supply their own function via the `read_fun' option.
+-spec default_read_fun(RootDir :: string()) -> read_fun().
+default_read_fun(RootDir) ->
+    {ok, RootParts} = normalize(filename:split(RootDir)),
+    fun(Cwd, Path) ->
+        case check_path(Cwd, Path, RootParts) of
+            {ok, Parts} -> file:read_file(filename:join(Parts));
+            unsafe      -> {error, {import_outside_root, Path}}
+        end
+    end.
+
+%% @private Resolve `Path' against `Cwd', collapse `.'/`..' segments,
+%% and check the result still lies under the root directory identified
+%% by `RootParts'. Returns the normalized path parts on success, or
+%% `unsafe' if the resolved path escapes the root via `..' or otherwise
+%% leaves the rooted tree. Absolute `Path' is accepted only when the
+%% root is also absolute and is a prefix of `Path' — otherwise
+%% `filename:join/2' substitutes `Path' for `Cwd' and the prefix check
+%% rejects it.
+check_path(Cwd, Path, RootParts) ->
+    case normalize(filename:split(filename:join(Cwd, Path))) of
+        {ok, Parts} ->
+            case lists:prefix(RootParts, Parts) of
+                true  -> {ok, Parts};
+                false -> unsafe
+            end;
+        escape ->
+            unsafe
+    end.
+
+%% @private Collapse `.' and `..' segments. Returns `escape' if `..'
+%% would pop past the start of the path, otherwise `{ok, Parts}'.
+normalize(Parts) -> normalize(Parts, []).
+
+normalize([], Acc)                -> {ok, lists:reverse(Acc)};
+normalize(["." | T], Acc)         -> normalize(T, Acc);
+normalize([".." | T], [_ | AccT]) -> normalize(T, AccT);
+normalize([".." | _], [])         -> escape;
+normalize([P | T], Acc)           -> normalize(T, [P | Acc]).
 
 new_context(Cwd) ->
     new_context(Cwd, []).
 
 new_context(Cwd, Opts) ->
-    ReadFun = proplists:get_value(read_fun, Opts, default_read_fun()),
-    #st{cwd = Cwd, read_fun = ReadFun}.
+    RootDir = proplists:get_value(rootdir, Opts, Cwd),
+    ReadFun = proplists:get_value(read_fun, Opts, default_read_fun(RootDir)),
+    #st{cwd = Cwd, rootdir = RootDir, read_fun = ReadFun}.
 
 str_to_avpr(String, Cwd) ->
     str_to_avpr(String, Cwd, []).
@@ -101,11 +149,11 @@ process_imports(Defs, St) ->
               [Def]
       end, Defs).
 
-load_idl_import(Path, #st{cwd = Cwd, read_fun = ReadFun} = St) ->
+load_idl_import(Path, #st{cwd = Cwd, rootdir = Root, read_fun = ReadFun} = St) ->
     {ok, Bin} = ReadFun(Cwd, Path),
     ImportedCwd = filename:dirname(filename:join(Cwd, Path)),
     Avpr = str_to_avpr(binary_to_list(Bin), ImportedCwd,
-                       [{read_fun, ReadFun}]),
+                       [{read_fun, ReadFun}, {rootdir, Root}]),
     types_from_avpr(Avpr, St).
 
 load_avpr_import(Path, #st{cwd = Cwd, read_fun = ReadFun} = St) ->

src/avro_schema_store.erl

@@ -40,13 +40,16 @@
 -export([ new/0
         , new/1
         , new/2
+        , new/3
         , close/1
         , is_store/1
         ]).
 
 %% Import
 -export([ import_file/2
+        , import_file/3
         , import_files/2
+        , import_files/3
         , import_schema_json/2
         , import_schema_json/3
         ]).
@@ -97,11 +100,19 @@ new(Options) ->
           end
   end.
 
-%% @doc Create a new schema store and improt the given schema JSON files.
+%% @doc Create a new schema store and import the given schema JSON files.
 -spec new([proplists:property()], [filename()]) -> store().
 new(Options, Files) ->
+  new(Options, Files, []).
+
+%% @doc Same as {@link new/2} but forwards `ImportOpts' to
+%% {@link import_files/3} — e.g. `[{rootdir, "schemas"}]' when loading
+%% `.avdl' files that import from sibling directories via `..'.
+-spec new([proplists:property()], [filename()], [proplists:property()]) ->
+        store().
+new(Options, Files, ImportOpts) ->
   Store = new(Options),
-  import_files(Files, Store).
+  import_files(Files, Store, ImportOpts).
 
 %% @doc Return true if the given arg is a schema store.
 -spec is_store(term()) -> boolean().
@@ -120,7 +131,13 @@ to_lookup_fun(Store) ->
 %% @doc Import avro JSON files into schema store.
 -spec import_files([filename()], store()) -> store().
 import_files(Files, Store) ->
-  lists:foldl(fun(File, S) -> import_file(File, S) end, Store, Files).
+  import_files(Files, Store, []).
+
+%% @doc Import avro JSON/IDL files into schema store with options.
+%% Options are forwarded to {@link import_file/3}.
+-spec import_files([filename()], store(), [proplists:property()]) -> store().
+import_files(Files, Store, Opts) ->
+  lists:foldl(fun(File, S) -> import_file(File, S, Opts) end, Store, Files).
 
 %% @doc Import avro JSON or IDL file into schema store.
 %% For ".avdl" files, the IDL is compiled and all types are imported.
@@ -134,9 +151,22 @@ import_files(Files, Store) ->
 %% @end
 -spec import_file(filename(), store()) -> store().
 import_file(File, Store) ->
+  import_file(File, Store, []).
+
+%% @doc Same as {@link import_file/2} but accepts options.
+%% Supported options for `.avdl' inputs:
+%% <ul>
+%%   <li>`{rootdir, Dir}' — directory that bounds default import
+%%       resolution. Imports may use `..' freely within `Dir' but
+%%       cannot escape it. Defaults to the directory of `File'.</li>
+%%   <li>`{read_fun, Fun}' — custom reader; bypasses all path checks.</li>
+%% </ul>
+%% Options are ignored for non-IDL inputs.
+-spec import_file(filename(), store(), [proplists:property()]) -> store().
+import_file(File, Store, Opts) ->
   case filename:extension(File) of
     Ext when Ext =:= ".avdl"; Ext =:= <<".avdl">> ->
-      import_idl_file(File, Store);
+      import_idl_file(File, Store, Opts);
     _ ->
       case file:read_file(File) of
         {ok, Json} ->
@@ -254,11 +284,11 @@ import_schema_json(AssignedName, Json, Store) ->
 
 %% @private Import an Avro IDL (.avdl) file into the schema store.
 %% All named types defined in (or imported by) the protocol are added.
--spec import_idl_file(filename(), store()) -> store().
-import_idl_file(File, Store) ->
+-spec import_idl_file(filename(), store(), [proplists:property()]) -> store().
+import_idl_file(File, Store, Opts) ->
   Cwd = filename:dirname(File),
   {ok, Bin} = file:read_file(File),
-  Schema = avro_idl:decode_schema(binary_to_list(Bin), Cwd),
+  Schema = avro_idl:decode_schema(binary_to_list(Bin), Cwd, Opts),
   {_Root, FlatTypes} = avro:flatten_type(Schema),
   lists:foldl(fun do_add_type/2, Store, FlatTypes).
 

test/avro_idl_tests.erl

@@ -170,9 +170,12 @@ import_schema_test() ->
        Types).
 
 import_nested_idl_test() ->
-    %% subdir/submodule.avdl imports ../foo.avdl (relative to its own dir).
-    %% Verifies that import paths are resolved relative to the importing file,
-    %% not relative to the top-level caller's cwd.
+    %% subdir/submodule.avdl imports ../foo.avdl. The top-level caller's
+    %% cwd is "test/data" which is also the default rootdir, so the ".."
+    %% from subdir/ stays inside the rooted tree and the import resolves
+    %% to test/data/foo.avdl. Also verifies that import paths are resolved
+    %% relative to the importing file, not the top-level cwd (foo.avdl is
+    %% not at "test/data/subdir/../foo.avdl" unless we recurse correctly).
     Proto = avro_idl:str_to_avpr(
               "protocol P { import idl \"subdir/submodule.avdl\"; }",
               "test/data"),
@@ -183,6 +186,36 @@ import_nested_idl_test() ->
         #{<<"name">> := <<"SubRecord">>, <<"type">> := ?AVRO_RECORD}],
        Types).
 
+import_with_rootdir_allows_dotdot_within_tree_test() ->
+    %% Loading subdir/submodule.avdl directly: its cwd becomes subdir/, so
+    %% "../foo.avdl" would escape if rootdir defaulted to that. Passing the
+    %% wider rootdir explicitly lets the import resolve while keeping the
+    %% confinement boundary.
+    SubmoduleFile = test_data("subdir/submodule.avdl"),
+    {ok, Bin} = file:read_file(SubmoduleFile),
+    RootDir = test_data(""),
+    Proto = avro_idl:str_to_avpr(
+              binary_to_list(Bin),
+              filename:dirname(SubmoduleFile),
+              [{rootdir, RootDir}]),
+    #{<<"types">> := Types} = Proto,
+    ?assertMatch(
+       [#{<<"name">> := <<"FooRecord">>, <<"type">> := ?AVRO_RECORD},
+        #{<<"name">> := <<"FooEnum">>, <<"type">> := ?AVRO_ENUM},
+        #{<<"name">> := <<"SubRecord">>, <<"type">> := ?AVRO_RECORD}],
+       Types).
+
+import_with_rootdir_blocks_escape_test() ->
+    %% rootdir set to subdir/ — ../foo.avdl escapes the rooted tree and
+    %% must be refused even though the file exists.
+    SubmoduleFile = test_data("subdir/submodule.avdl"),
+    {ok, Bin} = file:read_file(SubmoduleFile),
+    Cwd = filename:dirname(SubmoduleFile),
+    ?assertError(
+       {badmatch, {error, {import_outside_root, "../foo.avdl"}}},
+       avro_idl:str_to_avpr(
+         binary_to_list(Bin), Cwd, [{rootdir, Cwd}])).
+
 import_with_read_fun_test() ->
     %% All schemas kept in memory; no filesystem access.
     %% dep.avdl transitively imports transitive.avdl, verifying read_fun
@@ -212,6 +245,88 @@ import_with_read_fun_test() ->
         #{<<"name">> := <<"DepRecord">>,   <<"type">> := ?AVRO_RECORD}],
        Types).
 
+import_outside_root_rejected_test_() ->
+    %% The default read_fun resolves imports relative to the importing
+    %% file's directory and refuses paths that point outside it, both
+    %% absolute paths and relative paths that escape through "..".
+    AttackPaths = ["/etc/passwd", "../../etc/passwd"],
+    Variants = ["idl", "protocol", "schema"],
+    [?_assertError(
+        {badmatch, {error, {import_outside_root, _}}},
+        avro_idl:str_to_avpr(
+          "protocol P { import " ++ Kind ++ " \"" ++ AP ++ "\"; }",
+          "test/data"))
+     || Kind <- Variants, AP <- AttackPaths].
+
+import_absolute_path_under_absolute_root_test() ->
+    %% An absolute import path is accepted when rootdir is also absolute
+    %% and the path lies under it.
+    AbsRoot = test_data(""),
+    AbsFoo = test_data("foo.avdl"),
+    Proto = avro_idl:str_to_avpr(
+              "protocol P { import idl \"" ++ AbsFoo ++ "\"; }",
+              AbsRoot,
+              [{rootdir, AbsRoot}]),
+    #{<<"types">> := Types} = Proto,
+    ?assertMatch(
+       [#{<<"name">> := <<"FooRecord">>, <<"type">> := ?AVRO_RECORD},
+        #{<<"name">> := <<"FooEnum">>, <<"type">> := ?AVRO_ENUM}],
+       Types).
+
+import_absolute_path_outside_absolute_root_test() ->
+    %% Absolute path with absolute rootdir is rejected when the path is
+    %% not under the root. Exercises the prefix check in the
+    %% absolute-vs-absolute case (the existing /etc/passwd test uses a
+    %% relative root, so the abs-vs-rel mismatch alone makes it fail).
+    AbsRoot = test_data("subdir"),
+    AbsFoo = test_data("foo.avdl"),
+    ?assertError(
+       {badmatch, {error, {import_outside_root, _}}},
+       avro_idl:str_to_avpr(
+         "protocol P { import idl \"" ++ AbsFoo ++ "\"; }",
+         AbsRoot,
+         [{rootdir, AbsRoot}])).
+
+import_rootdir_with_dotdot_segments_test() ->
+    %% rootdir with embedded `..' segments is normalized once at
+    %% setup, so the prefix check still works against canonical paths.
+    DenormRoot = test_data("subdir/.."),
+    Proto = avro_idl:str_to_avpr(
+              "protocol P { import idl \"subdir/submodule.avdl\"; }",
+              DenormRoot,
+              [{rootdir, DenormRoot}]),
+    #{<<"types">> := Types} = Proto,
+    ?assertMatch(
+       [#{<<"name">> := <<"FooRecord">>, <<"type">> := ?AVRO_RECORD},
+        #{<<"name">> := <<"FooEnum">>, <<"type">> := ?AVRO_ENUM},
+        #{<<"name">> := <<"SubRecord">>, <<"type">> := ?AVRO_RECORD}],
+       Types).
+
+import_dot_segments_collapsed_test() ->
+    %% `.' segments in the import path are collapsed by normalize/1.
+    Proto = avro_idl:str_to_avpr(
+              "protocol P { import idl \"./foo.avdl\"; }",
+              "test/data"),
+    #{<<"types">> := Types} = Proto,
+    ?assertMatch(
+       [#{<<"name">> := <<"FooRecord">>, <<"type">> := ?AVRO_RECORD},
+        #{<<"name">> := <<"FooEnum">>, <<"type">> := ?AVRO_ENUM}],
+       Types).
+
+import_outside_root_with_read_fun_override_test() ->
+    %% The strict default can be bypassed by supplying a custom read_fun;
+    %% verify the option still takes effect and the default is not applied
+    %% on top of it.
+    ReadFun = fun(_Cwd, _Path) ->
+                {ok, <<"protocol Bypassed { record R { int n; } }">>}
+              end,
+    Proto = avro_idl:str_to_avpr(
+              "protocol P { import idl \"/anywhere/on/disk.avdl\"; }",
+              "test/data",
+              [{read_fun, ReadFun}]),
+    #{<<"types">> := Types} = Proto,
+    ?assertMatch([#{<<"name">> := <<"R">>}], Types).
+
 duplicate_annotation_avpr_test() ->
     ?assertError(
        {duplicate_annotation, "my_decorator", _, _},
@@ -241,10 +356,12 @@ nested_complex_types_avr_test() ->
       ).
 
 encode_decode_test() ->
-    %% subdir/submodule.avdl imports ../foo.avdl (idl); verify types from both
-    %% files can be encoded/decoded after loading via schema store
+    %% subdir/submodule.avdl imports ../foo.avdl (idl); verify types from
+    %% both files can be encoded/decoded after loading via schema store.
+    %% rootdir is the test/data tree so the "../" import resolves inside it.
     SubmoduleFile = test_data("subdir/submodule.avdl"),
-    Store1 = avro_schema_store:new([], [SubmoduleFile]),
+    Store1 = avro_schema_store:new(
+               [], [SubmoduleFile], [{rootdir, test_data("")}]),
     LookupFun1 = avro_schema_store:to_lookup_fun(Store1),
     Encoder1 = avro:make_encoder(LookupFun1, []),
     Decoder1 = avro:make_decoder(LookupFun1, []),