pathExists: Return false on "/nix/store" in pure mode #10505
Conversation
github-actions
bot
added
the
with-tests
|
What about |
AllowListInputAccessor has the invariant that if a path is accessible, its parent directories are also considered accessible (though reading them only yields the allowed subdirectories). As a result `builtins.pathExists "/nix/store"` returns true. However this wasn't the behaviour of previous path access control, where `builtins.pathExists "/nix/store"` returns false even if a subdirectory of the store is accessible. Fixes NixOS#9672.
This seems like a good invariant in normal use when composing accessors and such.
Maybe the system accessor should be a composition of Regardless of how the accessor is named and structured, this is better handled in the accessor abstraction than in the language frontend, as highlighted by John's observation; this behavior needs to be reflected in all operations. Then the builtins can just use that and not have to duplicate a "hack". |
'--restrict-eval true' was probably not what was intended (i.e. parsing a file named 'true').
E.g. `pathExists` on such parents will return `false`, and `readDir` will fail.
edolstra
force-pushed
the
pathExists-regression
branch
from
480449e
to
873be03
Compare
github-actions
bot
added
the
fetching
|
I removed the special case handling in |
| std::unordered_set<CanonPath> files{CanonPath::root}; | ||
| for (auto path : wd.files) { | ||
| while (!path.isRoot()) { | ||
| if (!files.insert(path).second) break; | ||
| path.pop(); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is a generic solution for getting the closure of path parents. Would be nice to factor out.
| @@ -111,7 +111,7 @@ struct SourceAccessor | |||
| std::optional<uint64_t> narOffset; | |||
| }; | |||
|
|
|||
| Stat lstat(const CanonPath & path); | |||
| virtual Stat lstat(const CanonPath & path); | |||
There was a problem hiding this comment.
IIRC if allowed paths is closed under taking parent dirs, then we don't need this, because this is just done for sake of resolving symlinks?
There was a problem hiding this comment.
This is necessary to get the correct error message for inaccessible paths, i.e. forbidden in restricted mode rather than does not exist.
There was a problem hiding this comment.
Makes sense,
I was thinking more the other thing, having lstatMaybe not allow /nix and /nix/store again. And the interaction with the other PR more broadly.
|
We talked about this a while today in the Nix meting. I unfortunately basically changed my mind: I am not comfortable with This means I am actually back to preferring the original version of the PR, which did not change the deep abstraction of |
roberth
added
regression
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2024-09-25-nix-team-meeting-minutes-181-180/52712/1 |
Motivation
AllowListInputAccessorhas the invariant that if a path is accessible, its parent directories are also considered accessible (though reading them only yields the allowed subdirectories). As a resultbuiltins.pathExists "/nix/store"returns true.However this wasn't the behaviour of previous path access control, where
builtins.pathExists "/nix/store"returns false even if a subdirectory of the store is accessible.Fixes #9672.
Context
Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.