Skip to content

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Sep 25, 2025

Motivation

No AllowListSourceAccessor for pure eval --- not needed anymore!

Context

Depends on #14080


Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@github-actions github-actions bot added new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority fetching Networking with the outside (non-Nix) world, input locking labels Sep 25, 2025
@Ericson2314
Copy link
Member Author

The last commit (the one not in a previous PR) needs some debugging.

Copy link

dpulls bot commented Sep 25, 2025

🎉 All dependencies have been resolved !

mkdir -p "$traverseDir"
goUp="..$(echo "$traverseDir" | sed -e 's,[^/]\+,..,g')"
output="$(nix eval --raw --restrict-eval -I "$traverseDir" \
output="$(nix eval --raw --impure --restrict-eval -I "$traverseDir" \
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just a mistake I think --- it didn't mean to test pure and restricted evaluation together.

@Ericson2314 Ericson2314 force-pushed the pure-eval-mount-not-filter branch from 0264b6a to 4917e48 Compare September 25, 2025 18:00
@Ericson2314
Copy link
Member Author

@edolstra in the remaining test failures, I am getting an empty memory source accessor from the mounted source accessor, rather than my failing one with the right error message, and I am not sure why.

@edolstra
Copy link
Member

At first glance this appears to make the code more complicated, so I'm not sure if it's an improvement?

@Ericson2314
Copy link
Member Author

No I don't think this makes it more complicated. The only thing that is complicated right now is my crude attempts to get the error message right.

If we ignore the error messages part, it is extremely simple:

  • restricted eval: Use filtering as before
  • pure eval: simply mount the things we're supposed to have in storeFS, no filtering is needed.

Comment on lines 229 to 253
/* This is just an overkill way to make sure other store
paths get this error, and not the "doesn't exist" error
that the mounted source accessor would do on its own. */
accessor->mount(
CanonPath::root,
AllowListSourceAccessor::create(
getFSSourceAccessor(), {}, {CanonPath::root, CanonPath(store->storeDir)}, [&](const CanonPath & path) -> RestrictedPathError {
throw RestrictedPathError(
"access to absolute path '%1%' is forbidden in pure evaluation mode (use '--impure' to override)",
CanonPath(store->storeDir) / path);
}));
/* We don't want to list store paths */
accessor->mount(CanonPath(store->storeDir), makeEmptySourceAccessor());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is me trying to arrange the error reporting. It is not yet working. Another approach is fine.

Comment on lines +269 to 285
if (settings.restrictEval)
return AllowListSourceAccessor::create(
makeImpureAccessor(), {}, {}, [](const CanonPath & path) -> RestrictedPathError {
throw RestrictedPathError("access to absolute path '%1%' is forbidden in restricted mode", path);
});
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is simpler than before, because it is just for restricted eval now.

@Ericson2314 Ericson2314 force-pushed the pure-eval-mount-not-filter branch from e540a35 to c4bf20c Compare September 28, 2025 00:39
No `AllowListSourceAccessor` for pure eval --- not needed anymore!
@Ericson2314 Ericson2314 force-pushed the pure-eval-mount-not-filter branch from c4bf20c to 80edaab Compare September 28, 2025 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fetching Networking with the outside (non-Nix) world, input locking new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants