avro_idl: confine IDL import paths

[zmstone]

No description provided.

[seriyps]

Thanks for the fix. I used AI for some research, but the ideas below are my personal:

One concern about the .. half of the change:

The fix uses filelib:safe_relative_path(Path, Cwd), Cwd is the importing file's own directory. That means any .. in an import path is rejected outright, even when the resolved path stays inside the tree of the top-level .avdl file.

Layouts like this become impossible to load:

schemas/
  common.avdl
  orders/order.avdl       %% imports "../common.avdl" <- this fails
  events.avdl             %% imports "orders/order.avdl" <- this is ok

avro_schema_store:new([], ["schemas/events.avdl"]) sets Cwd to schemas, when we go down to schemas/orders/order.avdl, Cwd changes to schemas/orders then ../common.avdl fails safe_relative_path — even though it lands at the same level as initial events.avdl.

The reference Java parser (IdlReader.findImport) supports this implicitly by using URI.resolve with no path restrictions. 😱 .

There are lots of pblic auro schemas which do .. imports: https://github.com/search?q=import+idl+%22..%2F%22+path%3A*.avdl&type=code

Cross-directory layouts are also an explicit user request in the Avro ecosystem (e.g. sbt-avrohugger#68) — splitting schemas by sub-domain via sibling directories is the normal way to organize a non-trivial schema set.

This PR also rewrites the existing subdir/submodule.avdl -> ../foo.avdl test fixture to a sibling import. That fixture was the in-repo example of exactly this pattern so the new default breaks the case.

I suggest PR is updated to a well-established pattern for this kind of confinement is the web-server "document root" / chroot-style boundary: a fixed root directory is set at the top level, and traversal is permitted freely below it but never above. Apache's DocumentRoot, nginx's root, every static file server works this way because legitimate content layouts use .. to reach sibling directories within the tree. Conflating the boundary with the current file's directory (which is what this PR does) is stricter than any of those.

Concretely: track a rootdir (the initial caller's Cwd) that doesn't shift on nested imports, and check safe_relative_path against rootdir instead of the per-file Cwd. Imports stay confined to the tree the caller loaded from, but .. within that tree works. Optionally expose {rootdir, Dir} via new_context/2 for callers who want a wider/narrower boundary. It can be tracked either as extra field in #st{} or as a fun closure.

I understand that such result can be achieved with custom read_fun but IMO we can do better by-default.

Minor: README ## Avro IDL section doesn't mention the new behavior — worth a paragraph there that explains import directory restrictions alongside the changelog entry.

[seriyps]

Smth like this seem to work:

diff --git a/src/avro_idl.erl b/src/avro_idl.erl
index 47db97eb3b6..68735bd1173 100644
--- a/src/avro_idl.erl
+++ b/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, []).
@@ -42,23 +44,41 @@ decode_schema(SchemaStr, Cwd, Opts) ->
 
 %% @doc Default `read_fun' used when one is not supplied via options.
 %% Import paths are resolved relative to the importing file's directory
-%% (`Cwd'). Absolute paths and relative paths that escape `Cwd' through
-%% `..' are refused with `{error, {import_outside_root, Path}}'. Callers
-%% that need different resolution semantics can supply their own
-%% function via the `read_fun' option.
--spec default_read_fun() -> read_fun().
-default_read_fun() ->
+%% (`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) ->
+    RootParts = filename:split(RootDir),
     fun(Cwd, Path) ->
         case filename:pathtype(Path) of
-            absolute ->
-                {error, {import_outside_root, Path}};
+            relative ->
+                CwdParts = filename:split(Cwd),
+                case lists:prefix(RootParts, CwdParts) of
+                    true ->
+                        RelCwd =
+                            case lists:nthtail(length(RootParts), CwdParts) of
+                                []   -> ".";
+                                Rest -> filename:join(Rest)
+                            end,
+                        Candidate = filename:join(RelCwd, Path),
+                        case filelib:safe_relative_path(Candidate, RootDir) of
+                            unsafe ->
+                                {error, {import_outside_root, Path}};
+                            SafePath ->
+                                file:read_file(
+                                  filename:join(RootDir, SafePath))
+                        end;
+                    false ->
+                        {error, {import_outside_root, Path}}
+                end;
             _ ->
-                case filelib:safe_relative_path(Path, Cwd) of
-                    unsafe ->
-                        {error, {import_outside_root, Path}};
-                    SafePath ->
-                        file:read_file(filename:join(Cwd, SafePath))
-                end
+                {error, {import_outside_root, Path}}
         end
     end.
 
@@ -66,8 +86,9 @@ 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, []).
@@ -119,11 +140,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) ->
diff --git a/src/avro_schema_store.erl b/src/avro_schema_store.erl
index 08be9fca68d..c664e2f092c 100644
--- a/src/avro_schema_store.erl
+++ b/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).
 

[seriyps]

Thanks!