Skip to content

test: glob_files_rec with a relative path#8265

Merged
rgrinberg merged 1 commit intomainfrom
ps/rr/test__glob_files_rec_with_a_relative_path
Aug 4, 2023
Merged

test: glob_files_rec with a relative path#8265
rgrinberg merged 1 commit intomainfrom
ps/rr/test__glob_files_rec_with_a_relative_path

Conversation

@rgrinberg
Copy link
Copy Markdown
Member

@rgrinberg rgrinberg commented Jul 24, 2023

We demonstrate that using relative paths in [glob_files_rec] has
unintended consequences. In this test, we install some artifacts outside
the package directory just by globbing from a parent dir.

Signed-off-by: Rudi Grinberg me@rgrinberg.com

@rgrinberg rgrinberg requested review from anmonteiro and gridbugs July 24, 2023 20:16
gridbugs
gridbugs reviewed Jul 25, 2023
View reviewed changes
@rgrinberg
test: glob_files_rec with a relative path
9fe0c3f 
We demonstrate that using relative paths in [glob_files_rec] has
unintended consequences. In this test, we install some artifacts outside
the package directory just by globbing from a parent dir.

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

<!-- ps-id: 0f38b0df-b5b3-4797-a555-8687aed27290 -->
@rgrinberg rgrinberg force-pushed the ps/rr/test__glob_files_rec_with_a_relative_path branch from dee4a3d to 9fe0c3f Compare July 25, 2023 08:30
@rgrinberg
Copy link
Copy Markdown
Member Author

rgrinberg commented Jul 25, 2023

Improved the test to describe the issue better.

How should we go about the fix? I've copied @anmonteiro because he dealt with a similar issue with runtime_deps.

One way would be to allow users to explicitly write out the prefix. E.g. (glob_files_rec ../foo/*.txt as foo/) similar to how we have as for normal installed files.

@anmonteiro
Copy link
Copy Markdown
Collaborator

anmonteiro commented Aug 3, 2023

For the Melange rules we added a check that the runtime assets must be present under the source tree where the library is defined:

dune/src/dune_rules/install_rules.ml

Lines 46 to 63 in dfb6330

let check_runtime_deps_relative_path local_path ~loc ~lib_info =
let lib_src_dir = Lib_info.src_dir lib_info in
match
Path.Local.descendant local_path ~of_:(Path.Build.local lib_src_dir)
with
| None ->
User_error.raise ~loc
[ Pp.textf
"Public library `%s' depends on assets outside its source tree. This \
is not allowed."
(lib_info |> Lib_info.name |> Lib_name.to_string)
]
~hints:
[ Pp.textf
"Move the dependency to a descendant of the folder where the \
library is defined"
]
| Some _ -> ()

@rgrinberg rgrinberg merged commit 7b211ab into main Aug 4, 2023
@rgrinberg rgrinberg deleted the ps/rr/test__glob_files_rec_with_a_relative_path branch August 4, 2023 12:15
@gridbugs
Copy link
Copy Markdown
Collaborator

gridbugs commented Aug 7, 2023

I'm looking into this now. I noticed that the non-recursive glob_files is also affected. Also it's currently possible to get this behaviour without any globs by writing (files (../stuff/foo.txt as ../stuff/foo.txt)). Dune will happily let you install files outside the current package if the as keyword specifies a relative path as the destination.

I'm going to do two things to fix this:

  1. add a check that ensures the destination of an install file entry is a descendant of the package's directory.
  2. as (1) will prevent an install file entry from using globs beginning with .. I'll add some syntax for changing the prefix of files matched by a glob as @rgrinberg suggested.

@gridbugs gridbugs mentioned this pull request Aug 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gridbugs gridbugs gridbugs left review comments

@anmonteiro anmonteiro Awaiting requested review from anmonteiro

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rgrinberg @anmonteiro @gridbugs