Merge pull request #20 from DeterminateSystems/map-to-original-accessors-2

Source path error improvements

src/libexpr/eval.cc

@@ -15,6 +15,7 @@
 #include "print.hh"
 #include "filtering-source-accessor.hh"
 #include "memory-source-accessor.hh"
+#include "mounted-source-accessor.hh"
 #include "gc-small-vector.hh"
 #include "url.hh"
 #include "fetch-to-store.hh"
@@ -245,6 +246,12 @@ EvalState::EvalState(
     }
     , repair(NoRepair)
     , emptyBindings(0)
+    , storeFS(
+        makeMountedSourceAccessor(
+            {
+                {CanonPath::root, makeEmptySourceAccessor()},
+                {CanonPath(store->storeDir), makeFSSourceAccessor(dirOf(store->toRealPath(StorePath::dummy)))}
+            }))
     , rootFS(
         ({
             /* In pure eval mode, we provide a filesystem that only
@@ -260,19 +267,14 @@ EvalState::EvalState(
 
             auto realStoreDir = dirOf(store->toRealPath(StorePath::dummy));
             if (settings.pureEval || store->storeDir != realStoreDir) {
-                auto storeFS = makeMountedSourceAccessor(
-                    {
-                        {CanonPath::root, makeEmptySourceAccessor()},
-                        {CanonPath(store->storeDir), makeFSSourceAccessor(realStoreDir)}
-                    });
                 accessor = settings.pureEval
-                    ? storeFS
+                    ? storeFS.cast<SourceAccessor>()
                     : makeUnionSourceAccessor({accessor, storeFS});
             }
 
             /* Apply access control if needed. */
             if (settings.restrictEval || settings.pureEval)
-                accessor = AllowListSourceAccessor::create(accessor, {},
+                accessor = AllowListSourceAccessor::create(accessor, {}, {},
                     [&settings](const CanonPath & path) -> RestrictedPathError {
                         auto modeInformation = settings.pureEval
                             ? "in pure evaluation mode (use '--impure' to override)"
@@ -3071,6 +3073,11 @@ SourcePath EvalState::findFile(const LookupPath & lookupPath, const std::string_
 
         auto res = (r / CanonPath(suffix)).resolveSymlinks();
         if (res.pathExists()) return res;
+
+        // Backward compatibility hack: throw an exception if access
+        // to this path is not allowed.
+        if (auto accessor = res.accessor.dynamic_pointer_cast<FilteringSourceAccessor>())
+            accessor->checkAccess(res.path);
     }
 
     if (hasPrefix(path, "nix/"))
@@ -3141,6 +3148,11 @@ std::optional<SourcePath> EvalState::resolveLookupPathPath(const LookupPath::Pat
         if (path.resolveSymlinks().pathExists())
             return finish(std::move(path));
         else {
+            // Backward compatibility hack: throw an exception if access
+            // to this path is not allowed.
+            if (auto accessor = path.accessor.dynamic_pointer_cast<FilteringSourceAccessor>())
+                accessor->checkAccess(path.path);
+
             logWarning({
                 .msg = HintFmt("Nix search path entry '%1%' does not exist, ignoring", value)
             });

src/libexpr/eval.hh

@@ -37,6 +37,7 @@ class StorePath;
 struct SingleDerivedPath;
 enum RepairFlag : bool;
 struct MemorySourceAccessor;
+struct MountedSourceAccessor;
 namespace eval_cache {
     class EvalCache;
 }
@@ -262,6 +263,11 @@ public:
     /** `"unknown"` */
     Value vStringUnknown;
 
+    /**
+     * The accessor corresponding to `store`.
+     */
+    const ref<MountedSourceAccessor> storeFS;
+
     /**
      * The accessor for the root filesystem.
      */

src/libexpr/primops/fetchMercurial.cc

@@ -64,7 +64,7 @@ static void prim_fetchMercurial(EvalState & state, const PosIdx pos, Value * * a
     if (rev) attrs.insert_or_assign("rev", rev->gitRev());
     auto input = fetchers::Input::fromAttrs(state.fetchSettings, std::move(attrs));
 
-    auto [storePath, input2] = input.fetchToStore(state.store);
+    auto [storePath, accessor, input2] = input.fetchToStore(state.store);
 
     auto attrs2 = state.buildBindings(8);
     state.mkStorePathString(storePath, attrs2.alloc(state.sOutPath));

src/libexpr/primops/fetchTree.cc

@@ -10,6 +10,7 @@
 #include "url.hh"
 #include "value-to-json.hh"
 #include "fetch-to-store.hh"
+#include "mounted-source-accessor.hh"
 
 #include <nlohmann/json.hpp>
 
@@ -200,10 +201,12 @@ static void fetchTree(
             throw Error("input '%s' is not allowed to use the '__final' attribute", input.to_string());
     }
 
-    auto [storePath, input2] = input.fetchToStore(state.store);
+    auto [storePath, accessor, input2] = input.fetchToStore(state.store);
 
     state.allowPath(storePath);
 
+    state.storeFS->mount(CanonPath(state.store->printStorePath(storePath)), accessor);
+
     emitTreeAttrs(state, storePath, input2, v, params.emptyRevFallback, false);
 }
 

src/libfetchers/fetchers.cc

@@ -187,34 +187,30 @@ bool Input::contains(const Input & other) const
 }
 
 // FIXME: remove
-std::pair<StorePath, Input> Input::fetchToStore(ref<Store> store) const
+std::tuple<StorePath, ref<SourceAccessor>, Input> Input::fetchToStore(ref<Store> store) const
 {
     if (!scheme)
         throw Error("cannot fetch unsupported input '%s'", attrsToJSON(toAttrs()));
 
-    auto [storePath, input] = [&]() -> std::pair<StorePath, Input> {
-        try {
-            auto [accessor, result] = getAccessorUnchecked(store);
+    try {
+        auto [accessor, result] = getAccessorUnchecked(store);
 
-            auto storePath = nix::fetchToStore(*store, SourcePath(accessor), FetchMode::Copy, result.getName());
+        auto storePath = nix::fetchToStore(*store, SourcePath(accessor), FetchMode::Copy, result.getName());
 
-            auto narHash = store->queryPathInfo(storePath)->narHash;
-            result.attrs.insert_or_assign("narHash", narHash.to_string(HashFormat::SRI, true));
+        auto narHash = store->queryPathInfo(storePath)->narHash;
+        result.attrs.insert_or_assign("narHash", narHash.to_string(HashFormat::SRI, true));
 
-            result.attrs.insert_or_assign("__final", Explicit<bool>(true));
+        result.attrs.insert_or_assign("__final", Explicit<bool>(true));
 
-            assert(result.isFinal());
+        assert(result.isFinal());
 
-            checkLocks(*this, result);
+        checkLocks(*this, result);
 
-            return {storePath, result};
-        } catch (Error & e) {
-            e.addTrace({}, "while fetching the input '%s'", to_string());
-            throw;
-        }
-    }();
-
-    return {std::move(storePath), input};
+        return {std::move(storePath), accessor, result};
+    } catch (Error & e) {
+        e.addTrace({}, "while fetching the input '%s'", to_string());
+        throw;
+    }
 }
 
 void Input::checkLocks(Input specified, Input & result)
@@ -323,6 +319,8 @@ std::pair<ref<SourceAccessor>, Input> Input::getAccessorUnchecked(ref<Store> sto
 
             accessor->fingerprint = getFingerprint(store);
 
+            accessor->setPathDisplay("«" + to_string() + "»");
+
             return {accessor, *this};
         } catch (Error & e) {
             debug("substitution of input '%s' failed: %s", to_string(), e.what());

src/libfetchers/fetchers.hh

@@ -121,7 +121,7 @@ public:
      * Fetch the entire input into the Nix store, returning the
      * location in the Nix store and the locked input.
      */
-    std::pair<StorePath, Input> fetchToStore(ref<Store> store) const;
+    std::tuple<StorePath, ref<SourceAccessor>, Input> fetchToStore(ref<Store> store) const;
 
     /**
      * Check the locking attributes in `result` against

src/libfetchers/filtering-source-accessor.cc

@@ -20,9 +20,14 @@ bool FilteringSourceAccessor::pathExists(const CanonPath & path)
 }
 
 std::optional<SourceAccessor::Stat> FilteringSourceAccessor::maybeLstat(const CanonPath & path)
+{
+    return isAllowed(path) ? next->maybeLstat(prefix / path) : std::nullopt;
+}
+
+SourceAccessor::Stat FilteringSourceAccessor::lstat(const CanonPath & path)
 {
     checkAccess(path);
-    return next->maybeLstat(prefix / path);
+    return next->lstat(prefix / path);
 }
 
 SourceAccessor::DirEntries FilteringSourceAccessor::readDirectory(const CanonPath & path)
@@ -58,18 +63,23 @@ void FilteringSourceAccessor::checkAccess(const CanonPath & path)
 struct AllowListSourceAccessorImpl : AllowListSourceAccessor
 {
     std::set<CanonPath> allowedPrefixes;
+    std::unordered_set<CanonPath> allowedPaths;
 
     AllowListSourceAccessorImpl(
         ref<SourceAccessor> next,
         std::set<CanonPath> && allowedPrefixes,
+        std::unordered_set<CanonPath> && allowedPaths,
         MakeNotAllowedError && makeNotAllowedError)
         : AllowListSourceAccessor(SourcePath(next), std::move(makeNotAllowedError))
         , allowedPrefixes(std::move(allowedPrefixes))
+        , allowedPaths(std::move(allowedPaths))
     { }
 
     bool isAllowed(const CanonPath & path) override
     {
-        return path.isAllowed(allowedPrefixes);
+        return
+            allowedPaths.contains(path)
+            || path.isAllowed(allowedPrefixes);
     }
 
     void allowPrefix(CanonPath prefix) override
@@ -81,9 +91,14 @@ struct AllowListSourceAccessorImpl : AllowListSourceAccessor
 ref<AllowListSourceAccessor> AllowListSourceAccessor::create(
     ref<SourceAccessor> next,
     std::set<CanonPath> && allowedPrefixes,
+    std::unordered_set<CanonPath> && allowedPaths,
     MakeNotAllowedError && makeNotAllowedError)
 {
-    return make_ref<AllowListSourceAccessorImpl>(next, std::move(allowedPrefixes), std::move(makeNotAllowedError));
+    return make_ref<AllowListSourceAccessorImpl>(
+        next,
+        std::move(allowedPrefixes),
+        std::move(allowedPaths),
+        std::move(makeNotAllowedError));
 }
 
 bool CachingFilteringSourceAccessor::isAllowed(const CanonPath & path)

src/libfetchers/filtering-source-accessor.hh

@@ -2,6 +2,8 @@
 
 #include "source-path.hh"
 
+#include <unordered_set>
+
 namespace nix {
 
 /**
@@ -36,6 +38,8 @@ struct FilteringSourceAccessor : SourceAccessor
 
     bool pathExists(const CanonPath & path) override;
 
+    Stat lstat(const CanonPath & path) override;
+
     std::optional<Stat> maybeLstat(const CanonPath & path) override;
 
     DirEntries readDirectory(const CanonPath & path) override;
@@ -70,6 +74,7 @@ struct AllowListSourceAccessor : public FilteringSourceAccessor
     static ref<AllowListSourceAccessor> create(
         ref<SourceAccessor> next,
         std::set<CanonPath> && allowedPrefixes,
+        std::unordered_set<CanonPath> && allowedPaths,
         MakeNotAllowedError && makeNotAllowedError);
 
     using FilteringSourceAccessor::FilteringSourceAccessor;

src/libfetchers/git-utils.cc

@@ -1215,21 +1215,16 @@ ref<SourceAccessor> GitRepoImpl::getAccessor(
 ref<SourceAccessor> GitRepoImpl::getAccessor(const WorkdirInfo & wd, bool exportIgnore, MakeNotAllowedError makeNotAllowedError)
 {
     auto self = ref<GitRepoImpl>(shared_from_this());
-    /* In case of an empty workdir, return an empty in-memory tree. We
-       cannot use AllowListSourceAccessor because it would return an
-       error for the root (and we can't add the root to the allow-list
-       since that would allow access to all its children). */
     ref<SourceAccessor> fileAccessor =
-        wd.files.empty()
-        ? makeEmptySourceAccessor()
-        : AllowListSourceAccessor::create(
+        AllowListSourceAccessor::create(
             makeFSSourceAccessor(path),
-            std::set<CanonPath> { wd.files },
+            std::set<CanonPath>{ wd.files },
+            // Always allow access to the root, but not its children.
+            std::unordered_set<CanonPath>{CanonPath::root},
             std::move(makeNotAllowedError)).cast<SourceAccessor>();
     if (exportIgnore)
-        return make_ref<GitExportIgnoreSourceAccessor>(self, fileAccessor, std::nullopt);
-    else
-        return fileAccessor;
+        fileAccessor = make_ref<GitExportIgnoreSourceAccessor>(self, fileAccessor, std::nullopt);
+    return fileAccessor;
 }
 
 ref<GitFileSystemObjectSink> GitRepoImpl::getFileSystemObjectSink()

src/libfetchers/git.cc

@@ -15,6 +15,7 @@
 #include "fetch-settings.hh"
 #include "json-utils.hh"
 #include "archive.hh"
+#include "mounted-source-accessor.hh"
 
 #include <regex>
 #include <string.h>
@@ -532,14 +533,20 @@ struct GitInputScheme : InputScheme
         return *head;
     }
 
-    static MakeNotAllowedError makeNotAllowedError(std::string url)
+    static MakeNotAllowedError makeNotAllowedError(std::filesystem::path repoPath)
     {
-        return [url{std::move(url)}](const CanonPath & path) -> RestrictedPathError
-        {
-            if (nix::pathExists(path.abs()))
-                return RestrictedPathError("access to path '%s' is forbidden because it is not under Git control; maybe you should 'git add' it to the repository '%s'?", path, url);
+        return [repoPath{std::move(repoPath)}](const CanonPath & path) -> RestrictedPathError {
+            if (nix::pathExists(repoPath / path.rel()))
+                return RestrictedPathError(
+                    "Path '%1%' in the repository %2% is not tracked by Git.\n"
+                    "\n"
+                    "To make it visible to Nix, run:\n"
+                    "\n"
+                    "git -C %2% add \"%1%\"",
+                    path.rel(),
+                    repoPath);
             else
-                return RestrictedPathError("path '%s' does not exist in Git repository '%s'", path, url);
+                return RestrictedPathError("Path '%s' does not exist in Git repository %s.", path.rel(), repoPath);
         };
     }
 
@@ -747,7 +754,7 @@ struct GitInputScheme : InputScheme
         ref<SourceAccessor> accessor =
             repo->getAccessor(repoInfo.workdirInfo,
                 exportIgnore,
-                makeNotAllowedError(repoInfo.locationToArg()));
+                makeNotAllowedError(repoPath));
 
         /* If the repo has submodules, return a mounted input accessor
            consisting of the accessor for the top-level repo and the

src/libflake/flake/flake.cc

@@ -13,6 +13,7 @@
 #include "value-to-json.hh"
 #include "local-fs-store.hh"
 #include "fetch-to-store.hh"
+#include "mounted-source-accessor.hh"
 
 #include <nlohmann/json.hpp>
 
@@ -90,7 +91,9 @@ static StorePath copyInputToStore(
 {
     auto storePath = fetchToStore(*state.store, accessor, FetchMode::Copy, input.getName());
 
-    state.allowPath(storePath);
+    state.allowPath(storePath); // FIXME: should just whitelist the entire virtual store
+
+    state.storeFS->mount(CanonPath(state.store->printStorePath(storePath)), accessor);
 
     auto narHash = state.store->queryPathInfo(storePath)->narHash;
     input.attrs.insert_or_assign("narHash", narHash.to_string(HashFormat::SRI, true));

src/libutil/meson.build

@@ -224,6 +224,7 @@ headers = [config_h] + files(
   'logging.hh',
   'lru-cache.hh',
   'memory-source-accessor.hh',
+  'mounted-source-accessor.hh',
   'muxable-pipe.hh',
   'os-string.hh',
   'pool.hh',

src/libutil/mounted-source-accessor.cc

@@ -1,12 +1,12 @@
-#include "source-accessor.hh"
+#include "mounted-source-accessor.hh"
 
 namespace nix {
 
-struct MountedSourceAccessor : SourceAccessor
+struct MountedSourceAccessorImpl : MountedSourceAccessor
 {
     std::map<CanonPath, ref<SourceAccessor>> mounts;
 
-    MountedSourceAccessor(std::map<CanonPath, ref<SourceAccessor>> _mounts)
+    MountedSourceAccessorImpl(std::map<CanonPath, ref<SourceAccessor>> _mounts)
         : mounts(std::move(_mounts))
     {
         displayPrefix.clear();
@@ -23,6 +23,12 @@ struct MountedSourceAccessor : SourceAccessor
         return accessor->readFile(subpath);
     }
 
+    Stat lstat(const CanonPath & path) override
+    {
+        auto [accessor, subpath] = resolve(path);
+        return accessor->lstat(subpath);
+    }
+
     std::optional<Stat> maybeLstat(const CanonPath & path) override
     {
         auto [accessor, subpath] = resolve(path);
@@ -69,11 +75,17 @@ struct MountedSourceAccessor : SourceAccessor
         auto [accessor, subpath] = resolve(path);
         return accessor->getPhysicalPath(subpath);
     }
+
+    void mount(CanonPath mountPoint, ref<SourceAccessor> accessor) override
+    {
+        // FIXME: thread-safety
+        mounts.insert_or_assign(std::move(mountPoint), accessor);
+    }
 };
 
-ref<SourceAccessor> makeMountedSourceAccessor(std::map<CanonPath, ref<SourceAccessor>> mounts)
+ref<MountedSourceAccessor> makeMountedSourceAccessor(std::map<CanonPath, ref<SourceAccessor>> mounts)
 {
-    return make_ref<MountedSourceAccessor>(std::move(mounts));
+    return make_ref<MountedSourceAccessorImpl>(std::move(mounts));
 }
 
 }

src/libutil/mounted-source-accessor.hh

@@ -0,0 +1,14 @@
+#pragma once
+
+#include "source-accessor.hh"
+
+namespace nix {
+
+struct MountedSourceAccessor : SourceAccessor
+{
+    virtual void mount(CanonPath mountPoint, ref<SourceAccessor> accessor) = 0;
+};
+
+ref<MountedSourceAccessor> makeMountedSourceAccessor(std::map<CanonPath, ref<SourceAccessor>> mounts);
+
+}

src/libutil/source-accessor.hh

@@ -118,7 +118,7 @@ struct SourceAccessor : std::enable_shared_from_this<SourceAccessor>
         std::string typeString();
     };
 
-    Stat lstat(const CanonPath & path);
+    virtual Stat lstat(const CanonPath & path);
 
     virtual std::optional<Stat> maybeLstat(const CanonPath & path) = 0;
 
@@ -214,8 +214,6 @@ ref<SourceAccessor> getFSSourceAccessor();
  */
 ref<SourceAccessor> makeFSSourceAccessor(std::filesystem::path root);
 
-ref<SourceAccessor> makeMountedSourceAccessor(std::map<CanonPath, ref<SourceAccessor>> mounts);
-
 /**
  * Construct an accessor that presents a "union" view of a vector of
  * underlying accessors. Earlier accessors take precedence over later.

src/nix/flake.cc

@@ -1095,7 +1095,7 @@ struct CmdFlakeArchive : FlakeCommand, MixJSON, MixDryRun
                         storePath =
                             dryRun
                             ? (*inputNode)->lockedRef.input.computeStorePath(*store)
-                            : (*inputNode)->lockedRef.input.fetchToStore(store).first;
+                            : std::get<0>((*inputNode)->lockedRef.input.fetchToStore(store));
                         sources.insert(*storePath);
                     }
                     if (json) {

tests/functional/flakes/meson.build

@@ -29,7 +29,8 @@ suites += {
     'non-flake-inputs.sh',
     'relative-paths.sh',
     'symlink-paths.sh',
-    'debugger.sh'
+    'debugger.sh',
+    'source-paths.sh',
   ],
   'workdir': meson.current_source_dir(),
 }

tests/functional/flakes/source-paths.sh

@@ -0,0 +1,57 @@
+#!/usr/bin/env bash
+
+source ./common.sh
+
+requireGit
+
+repo=$TEST_ROOT/repo
+
+createGitRepo "$repo"
+
+cat > "$repo/flake.nix" <<EOF
+{
+  outputs = { ... }: {
+    x = 1;
+    y = assert false; 1;
+    z = builtins.readFile ./foo;
+    a = import ./foo;
+    b = import ./dir;
+  };
+}
+EOF
+
+expectStderr 1 nix eval "$repo#x" | grepQuiet "error: Path 'flake.nix' in the repository \"$repo\" is not tracked by Git."
+
+git -C "$repo" add flake.nix
+
+[[ $(nix eval "$repo#x") = 1 ]]
+
+expectStderr 1 nix eval "$repo#y" | grepQuiet "at $repo/flake.nix:"
+
+git -C "$repo" commit -a -m foo
+
+expectStderr 1 nix eval "git+file://$repo?ref=master#y" | grepQuiet "at «git+file://$repo?ref=master&rev=.*»/flake.nix:"
+
+expectStderr 1 nix eval "$repo#z" | grepQuiet "error: Path 'foo' does not exist in Git repository \"$repo\"."
+expectStderr 1 nix eval "git+file://$repo?ref=master#z" | grepQuiet "error: '«git+file://$repo?ref=master&rev=.*»/foo' does not exist"
+expectStderr 1 nix eval "$repo#a" | grepQuiet "error: Path 'foo' does not exist in Git repository \"$repo\"."
+
+echo 123 > "$repo/foo"
+
+expectStderr 1 nix eval "$repo#z" | grepQuiet "error: Path 'foo' in the repository \"$repo\" is not tracked by Git."
+expectStderr 1 nix eval "$repo#a" | grepQuiet "error: Path 'foo' in the repository \"$repo\" is not tracked by Git."
+
+git -C "$repo" add "$repo/foo"
+
+[[ $(nix eval --raw "$repo#z") = 123 ]]
+
+expectStderr 1 nix eval "$repo#b" | grepQuiet "error: Path 'dir' does not exist in Git repository \"$repo\"."
+
+mkdir -p "$repo/dir"
+echo 456 > "$repo/dir/default.nix"
+
+expectStderr 1 nix eval "$repo#b" | grepQuiet "error: Path 'dir' in the repository \"$repo\" is not tracked by Git."
+
+git -C "$repo" add "$repo/dir/default.nix"
+
+[[ $(nix eval "$repo#b") = 456 ]]