From 4620366a9010e2813c0e9f355fba865418b748b0 Mon Sep 17 00:00:00 2001 From: Chan Siu Man Date: Thu, 24 Oct 2024 20:30:53 -0700 Subject: [PATCH 1/5] Add `File.target` validation and escaping This change adds these in `files.ncl`: - `File.target` validation with `ValidTarget`, - `File.target` shell-escaping in `regenerate_one`. Before this change, the `files` field has `File` with possibly pathological `target | String`, e.g.: - Empty target, i.e., `""`, - Absolute target, e.g., `"/etc/passwd"`, - Parent traversals, e.g,. `"../../../../../../../etc/passwd"`. This change adds `ValidTarget`, such that `File.target | ValidTarget` has no such cases. Also, in `regenerate_one`, we add `shell_escape` to `file_descr.target`: ``` nix-s%"regenerate_function "%{copy_command}" "%{file_descr.file}" "%{shell_escape file_descr.target}""% ``` This correctly handles paths with characters such as `"`, `'`, `*`, `\`, `\n` (newline), `\t` (tabs). --- lib/files.ncl | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/lib/files.ncl b/lib/files.ncl index c4e8f25..98ac5fa 100644 --- a/lib/files.ncl +++ b/lib/files.ncl @@ -1,11 +1,34 @@ let nix = import "./nix-interop/nix.ncl" in +let RelativePath = + std.contract.from_validator (fun x => + if x |> std.string.characters |> std.array.at 0 == "/" then + 'Error { message = m%"Absolute Path: Expecting a relative path, got an absolute path starting with "/"."% } + else + 'Ok + ) +in +let NoTraversal = + std.contract.from_validator (fun x => + if x |> std.string.split "/" |> std.array.any (fun part => part == "..") then + 'Error { message = m%"Invalid Traversal: Path contains ".."."% } + else + 'Ok + ) +in +let +# A ValidTarget eliminates these pathological targets: +# - Empty target, i.e., "", +# - Absolute target, e.g., `"/etc/passwd"`, +# - Parent traversals, e.g,. `"../../../../../../../etc/passwd"`. +ValidTarget = std.contract.all_of [std.string.NonEmpty, RelativePath, NoTraversal] +in let File = { target | doc m%" The file to write to. If null, defaults to the attribute name of the file. "% - | String + | ValidTarget | optional, content | doc m%" @@ -72,7 +95,12 @@ let regenerate_files | Files -> nix.derivation.Derivation } file_descr.materialisation_method in - nix-s%"regenerate_function "%{copy_command}" "%{file_descr.file}" "%{file_descr.target}""% + let shell_escape = fun path => + path + |> std.string.replace "\\" "\\\\" + |> std.string.replace m%"""% m%"\""% + in + nix-s%"regenerate_function "%{copy_command}" "%{file_descr.file}" "%{shell_escape file_descr.target}""% in let regenerate_files = nix-s%" %{regenerate_function} From 1da6b635360e6110994c3be1ecc590bbc2d7d973 Mon Sep 17 00:00:00 2001 From: Chan Siu Man Date: Thu, 24 Oct 2024 21:10:23 -0700 Subject: [PATCH 2/5] Cut `std.contract.all_of` by expanding --- lib/files.ncl | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/lib/files.ncl b/lib/files.ncl index 98ac5fa..1fff48c 100644 --- a/lib/files.ncl +++ b/lib/files.ncl @@ -15,20 +15,15 @@ let NoTraversal = 'Ok ) in -let -# A ValidTarget eliminates these pathological targets: -# - Empty target, i.e., "", -# - Absolute target, e.g., `"/etc/passwd"`, -# - Parent traversals, e.g,. `"../../../../../../../etc/passwd"`. -ValidTarget = std.contract.all_of [std.string.NonEmpty, RelativePath, NoTraversal] -in let File = { target | doc m%" The file to write to. If null, defaults to the attribute name of the file. "% - | ValidTarget + | std.string.NonEmpty # avoids "" + | RelativePath # avoids "/etc/passwd" + | NoTraversal # avoids "../../../../../../../etc/passwd" | optional, content | doc m%" From 48ff98aab668c4d1a3b4ebf6ded5863e7e9abdcd Mon Sep 17 00:00:00 2001 From: Chan Siu Man Date: Thu, 24 Oct 2024 21:14:54 -0700 Subject: [PATCH 3/5] =?UTF-8?q?Use=20`std.contract.from=5F{validator=20?= =?UTF-8?q?=E2=86=92=20predicate}`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- lib/files.ncl | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/lib/files.ncl b/lib/files.ncl index 1fff48c..b99acb1 100644 --- a/lib/files.ncl +++ b/lib/files.ncl @@ -1,18 +1,9 @@ let nix = import "./nix-interop/nix.ncl" in let RelativePath = - std.contract.from_validator (fun x => - if x |> std.string.characters |> std.array.at 0 == "/" then - 'Error { message = m%"Absolute Path: Expecting a relative path, got an absolute path starting with "/"."% } - else - 'Ok - ) + std.contract.from_predicate (fun x => !(x |> std.string.characters |> std.array.at 0 == "/")) in let NoTraversal = - std.contract.from_validator (fun x => - if x |> std.string.split "/" |> std.array.any (fun part => part == "..") then - 'Error { message = m%"Invalid Traversal: Path contains ".."."% } - else - 'Ok + std.contract.from_predicate (fun x => !(x |> std.string.split "/" |> std.array.any (fun part => part == "..")) ) in let File = { From c4cfe488040f5630dfe78104f5648d506e9d334b Mon Sep 17 00:00:00 2001 From: Chan Siu Man Date: Thu, 24 Oct 2024 21:18:23 -0700 Subject: [PATCH 4/5] Format nickel --- lib/files.ncl | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/files.ncl b/lib/files.ncl index b99acb1..37ccfaa 100644 --- a/lib/files.ncl +++ b/lib/files.ncl @@ -3,8 +3,10 @@ let RelativePath = std.contract.from_predicate (fun x => !(x |> std.string.characters |> std.array.at 0 == "/")) in let NoTraversal = - std.contract.from_predicate (fun x => !(x |> std.string.split "/" |> std.array.any (fun part => part == "..")) - ) + std.contract.from_predicate + ( + fun x => !(x |> std.string.split "/" |> std.array.any (fun part => part == "..")) + ) in let File = { target From 6d80d903efb807a6a54c893f78c0487e5ef211a0 Mon Sep 17 00:00:00 2001 From: Chan Siu Man Date: Fri, 25 Oct 2024 17:54:23 -0700 Subject: [PATCH 5/5] =?UTF-8?q?Use=20`"=E2=80=A6"`=E2=86=92`$'=E2=80=A6'`?= =?UTF-8?q?=20for=20escaping,=20and=20restyle=20code?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This change addresses [the code review](https://github.com/nickel-lang/organist/pull/222/files/c4cfe488040f5630dfe78104f5648d506e9d334b): - Use `$'…'` (dollar-sign with *single*-quotes) instead of `"…"` (*double*-quotes) for escape shell argument (`file.target`) in `regenerate_one`, to handle paths with characters such as `"`, `'`, `*`, `\`, `\n` (newline), `\t` (tab), `$`. - Rename `NoTraveral` to `NoParentTraveral` for more accurate naming. - Rewrite implementations of contracts `RelativePath` and `NoParentTraveral` in idiomatic nickel. --- lib/files.ncl | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/lib/files.ncl b/lib/files.ncl index 37ccfaa..ec726dd 100644 --- a/lib/files.ncl +++ b/lib/files.ncl @@ -1,11 +1,20 @@ let nix = import "./nix-interop/nix.ncl" in let RelativePath = - std.contract.from_predicate (fun x => !(x |> std.string.characters |> std.array.at 0 == "/")) + std.contract.from_predicate + ( + fun x => + x + |> std.string.characters + |> std.array.first != "/" + ) in -let NoTraversal = +let NoParentTraversal = std.contract.from_predicate ( - fun x => !(x |> std.string.split "/" |> std.array.any (fun part => part == "..")) + fun x => + x + |> std.string.split "/" + |> std.array.all ((!=) "..") ) in let File = { @@ -16,7 +25,7 @@ let File = { "% | std.string.NonEmpty # avoids "" | RelativePath # avoids "/etc/passwd" - | NoTraversal # avoids "../../../../../../../etc/passwd" + | NoParentTraversal # avoids "../../../../../../../etc/passwd" | optional, content | doc m%" @@ -86,9 +95,9 @@ let regenerate_files | Files -> nix.derivation.Derivation let shell_escape = fun path => path |> std.string.replace "\\" "\\\\" - |> std.string.replace m%"""% m%"\""% + |> std.string.replace m%"'"% m%"\'"% in - nix-s%"regenerate_function "%{copy_command}" "%{file_descr.file}" "%{shell_escape file_descr.target}""% + nix-s%"regenerate_function '%{copy_command}' '%{file_descr.file}' $'%{shell_escape file_descr.target}'"% in let regenerate_files = nix-s%" %{regenerate_function}