Merge branch 'fix/subflake-follows-fix' of https://github.com/ArctarusLimited/nix

5 files changed, 175 insertions, 22 deletions

diff --git a/src/libexpr/flake/flake.cc b/src/libexpr/flake/flake.cc
index a2f100cbd..a380ae27f 100644
--- a/src/libexpr/flake/flake.cc
+++ b/src/libexpr/flake/flake.cc
@@ -329,21 +329,22 @@ LockedFlake lockFlake(
             const FlakeInputs & flakeInputs,
             std::shared_ptr<Node> node,
             const InputPath & inputPathPrefix,
-            std::shared_ptr<const Node> oldNode)>
+            std::shared_ptr<const Node> oldNode,
+            const LockParent parent, const Path parentPath)>
             computeLocks;
 
         computeLocks = [&](
             const FlakeInputs & flakeInputs,
             std::shared_ptr<Node> node,
             const InputPath & inputPathPrefix,
-            std::shared_ptr<const Node> oldNode)
+            std::shared_ptr<const Node> oldNode,
+            const LockParent parent, const Path parentPath)
         {
             debug("computing lock file node '%s'", printInputPath(inputPathPrefix));
 
             /* Get the overrides (i.e. attributes of the form
                'inputs.nixops.inputs.nixpkgs.url = ...'). */
-            // FIXME: check this
-            for (auto & [id, input] : flake.inputs) {
+            for (auto & [id, input] : flakeInputs) {
                 for (auto & [idOverride, inputOverride] : input.overrides) {
                     auto inputPath(inputPathPrefix);
                     inputPath.push_back(id);
@@ -379,15 +380,23 @@ LockedFlake lockFlake(
                        path we haven't processed yet. */
                     if (input.follows) {
                         InputPath target;
-                        if (hasOverride || input.absolute)
-                            /* 'follows' from an override is relative to the
-                               root of the graph. */
+
+                        if (parent.absolute && !hasOverride) {
                             target = *input.follows;
-                        else {
-                            /* Otherwise, it's relative to the current flake. */
-                            target = inputPathPrefix;
+                        } else {
+                            if (hasOverride)
+                            {
+                                target = inputPathPrefix;
+                                target.pop_back();
+                            }
+                            else
+                            {
+                                target = parent.path;
+                            }
+
                             for (auto & i : *input.follows) target.push_back(i);
                         }
+
                         debug("input '%s' follows '%s'", inputPathS, printInputPath(target));
                         node->inputs.insert_or_assign(id, target);
                         continue;
@@ -433,7 +442,7 @@ LockedFlake lockFlake(
                         if (hasChildUpdate) {
                             auto inputFlake = getFlake(
                                 state, oldLock->lockedRef, false, flakeCache);
-                            computeLocks(inputFlake.inputs, childNode, inputPath, oldLock);
+                            computeLocks(inputFlake.inputs, childNode, inputPath, oldLock, parent, parentPath);
                         } else {
                             /* No need to fetch this flake, we can be
                                lazy. However there may be new overrides on the
@@ -450,12 +459,11 @@ LockedFlake lockFlake(
                                 } else if (auto follows = std::get_if<1>(&i.second)) {
                                     fakeInputs.emplace(i.first, FlakeInput {
                                         .follows = *follows,
-                                        .absolute = true
                                     });
                                 }
                             }
 
-                            computeLocks(fakeInputs, childNode, inputPath, oldLock);
+                            computeLocks(fakeInputs, childNode, inputPath, oldLock, parent, parentPath);
                         }
 
                     } else {
@@ -467,7 +475,18 @@ LockedFlake lockFlake(
                             throw Error("cannot update flake input '%s' in pure mode", inputPathS);
 
                         if (input.isFlake) {
-                            auto inputFlake = getFlake(state, *input.ref, useRegistries, flakeCache);
+                            Path localPath = parentPath;
+                            FlakeRef localRef = *input.ref;
+
+                            // If this input is a path, recurse it down.
+                            // This allows us to resolve path inputs relative to the current flake.
+                            if (localRef.input.getType() == "path")
+                            {
+                                localRef.input.parent = parentPath;
+                                localPath = canonPath(parentPath + "/" + *input.ref->input.getSourcePath());
+                            }
+
+                            auto inputFlake = getFlake(state, localRef, useRegistries, flakeCache);
 
                             /* Note: in case of an --override-input, we use
                                the *original* ref (input2.ref) for the
@@ -488,6 +507,13 @@ LockedFlake lockFlake(
                             parents.push_back(*input.ref);
                             Finally cleanup([&]() { parents.pop_back(); });
 
+                            // Follows paths from existing inputs in the top-level lockfile are absolute,
+                            // whereas paths in subordinate lockfiles are relative to those lockfiles.
+                            LockParent newParent {
+                                .path = inputPath,
+                                .absolute = oldLock ? true : false
+                            };
+
                             /* Recursively process the inputs of this
                                flake. Also, unless we already have this flake
                                in the top-level lock file, use this flake's
@@ -497,7 +523,8 @@ LockedFlake lockFlake(
                                 oldLock
                                 ? std::dynamic_pointer_cast<const Node>(oldLock)
                                 : LockFile::read(
-                                    inputFlake.sourceInfo->actualPath + "/" + inputFlake.lockedRef.subdir + "/flake.lock").root);
+                                    inputFlake.sourceInfo->actualPath + "/" + inputFlake.lockedRef.subdir + "/flake.lock").root,
+                                newParent, localPath);
                         }
 
                         else {
@@ -515,9 +542,17 @@ LockedFlake lockFlake(
             }
         };
 
+        LockParent parent {
+            .path = {},
+            .absolute = true
+        };
+
+        // Bring in the current ref for relative path resolution if we have it
+        auto parentPath = canonPath(flake.sourceInfo->actualPath + "/" + flake.lockedRef.subdir);
+
         computeLocks(
             flake.inputs, newLockFile.root, {},
-            lockFlags.recreateLockFile ? nullptr : oldLockFile.root);
+            lockFlags.recreateLockFile ? nullptr : oldLockFile.root, parent, parentPath);
 
         for (auto & i : lockFlags.inputOverrides)
             if (!overridesUsed.count(i.first))
diff --git a/src/libexpr/flake/flake.hh b/src/libexpr/flake/flake.hh
index 15fd394f8..937cc021b 100644
--- a/src/libexpr/flake/flake.hh
+++ b/src/libexpr/flake/flake.hh
@@ -43,7 +43,6 @@ struct FlakeInput
     std::optional<FlakeRef> ref;
     bool isFlake = true;  // true = process flake to get outputs, false = (fetched) static source path
     std::optional<InputPath> follows;
-    bool absolute = false; // whether 'follows' is relative to the flake root
     FlakeInputs overrides;
 };
 
@@ -125,6 +124,15 @@ struct LockFlags
     std::set<InputPath> inputUpdates;
 };
 
+struct LockParent {
+    /* The path to this parent */
+    InputPath path;
+
+    /* Whether we are currently inside a top-level lockfile (inputs absolute)
+       or subordinate lockfile (inputs relative) */
+    bool absolute;
+};
+
 LockedFlake lockFlake(
     EvalState & state,
     const FlakeRef & flakeRef,
diff --git a/src/libfetchers/fetchers.hh b/src/libfetchers/fetchers.hh
index c839cf23b..c43b047a7 100644
--- a/src/libfetchers/fetchers.hh
+++ b/src/libfetchers/fetchers.hh
@@ -38,6 +38,9 @@ struct Input
     bool immutable = false;
     bool direct = true;
 
+    /* path of the parent of this input, used for relative path resolution */
+    std::optional<Path> parent;
+
 public:
     static Input fromURL(const std::string & url);
 
diff --git a/src/libfetchers/path.cc b/src/libfetchers/path.cc
index d1003de57..c4977834d 100644
--- a/src/libfetchers/path.cc
+++ b/src/libfetchers/path.cc
@@ -82,18 +82,34 @@ struct PathInputScheme : InputScheme
 
     std::pair<Tree, Input> fetch(ref<Store> store, const Input & input) override
     {
+        std::string absPath;
         auto path = getStrAttr(input.attrs, "path");
 
-        // FIXME: check whether access to 'path' is allowed.
+        if (path[0] != '/' && input.parent)
+        {
+            auto parent = canonPath(*input.parent);
+
+            // the path isn't relative, prefix it
+            absPath = canonPath(parent + "/" + path);
 
-        auto storePath = store->maybeParseStorePath(path);
+            // for security, ensure that if the parent is a store path, it's inside it
+            if (!parent.rfind(store->storeDir, 0) && absPath.rfind(store->storeDir, 0))
+                throw BadStorePath("relative path '%s' points outside of its parent's store path %s, this is a security violation", path, parent);
+        }
+        else
+        {
+            absPath = path;
+        }
+
+        // FIXME: check whether access to 'path' is allowed.
+        auto storePath = store->maybeParseStorePath(absPath);
 
         if (storePath)
             store->addTempRoot(*storePath);
 
         if (!storePath || storePath->name() != "source" || !store->isValidPath(*storePath))
             // FIXME: try to substitute storePath.
-            storePath = store->addToStore("source", path);
+            storePath = store->addToStore("source", absPath);
 
         return {
             Tree(store->toRealPath(*storePath), std::move(*storePath)),
diff --git a/tests/flakes.sh b/tests/flakes.sh
index 9e1b5b508..f5c7b6804 100644
--- a/tests/flakes.sh
+++ b/tests/flakes.sh
@@ -26,10 +26,15 @@ nonFlakeDir=$TEST_ROOT/nonFlake
 flakeA=$TEST_ROOT/flakeA
 flakeB=$TEST_ROOT/flakeB
 flakeGitBare=$TEST_ROOT/flakeGitBare
+flakeFollowsA=$TEST_ROOT/follows/flakeA
+flakeFollowsB=$TEST_ROOT/follows/flakeA/flakeB
+flakeFollowsC=$TEST_ROOT/follows/flakeA/flakeB/flakeC
+flakeFollowsD=$TEST_ROOT/follows/flakeA/flakeD
+flakeFollowsE=$TEST_ROOT/follows/flakeA/flakeE
 
-for repo in $flake1Dir $flake2Dir $flake3Dir $flake7Dir $templatesDir $nonFlakeDir $flakeA $flakeB; do
+for repo in $flake1Dir $flake2Dir $flake3Dir $flake7Dir $templatesDir $nonFlakeDir $flakeA $flakeB $flakeFollowsA; do
     rm -rf $repo $repo.tmp
-    mkdir $repo
+    mkdir -p $repo
     git -C $repo init
     git -C $repo config user.email "foobar@example.com"
     git -C $repo config user.name "Foobar"
@@ -681,3 +686,89 @@ git -C $flakeB commit -a -m 'Foo'
 
 # Test list-inputs with circular dependencies
 nix flake metadata $flakeA
+
+# Test flake follow paths
+mkdir -p $flakeFollowsB
+mkdir -p $flakeFollowsC
+mkdir -p $flakeFollowsD
+mkdir -p $flakeFollowsE
+
+cat > $flakeFollowsA/flake.nix <<EOF
+{
+    description = "Flake A";
+    inputs = {
+        B = {
+            url = "path:./flakeB";
+            inputs.foobar.follows = "D";
+        };
+
+        D.url = "path:./flakeD";
+        foobar.url = "path:./flakeE";
+    };
+    outputs = { ... }: {};
+}
+EOF
+
+cat > $flakeFollowsB/flake.nix <<EOF
+{
+    description = "Flake B";
+    inputs = {
+        foobar.url = "path:./../flakeE";
+        C = {
+            url = "path:./flakeC";
+            inputs.foobar.follows = "foobar";
+        };
+    };
+    outputs = { ... }: {};
+}
+EOF
+
+cat > $flakeFollowsC/flake.nix <<EOF
+{
+    description = "Flake C";
+    inputs = {
+        foobar.url = "path:./../../flakeE";
+    };
+    outputs = { ... }: {};
+}
+EOF
+
+cat > $flakeFollowsD/flake.nix <<EOF
+{
+    description = "Flake D";
+    inputs = {};
+    outputs = { ... }: {};
+}
+EOF
+
+cat > $flakeFollowsE/flake.nix <<EOF
+{
+    description = "Flake D";
+    inputs = {};
+    outputs = { ... }: {};
+}
+EOF
+
+git -C $flakeFollowsA add flake.nix flakeB/flake.nix \
+  flakeB/flakeC/flake.nix flakeD/flake.nix flakeE/flake.nix
+
+nix flake lock $flakeFollowsA
+
+[[ $(jq -c .nodes.B.inputs.C $flakeFollowsA/flake.lock) = '"C"' ]]
+[[ $(jq -c .nodes.B.inputs.foobar $flakeFollowsA/flake.lock) = '["D"]' ]]
+[[ $(jq -c .nodes.C.inputs.foobar $flakeFollowsA/flake.lock) = '["B","foobar"]' ]]
+
+# Ensure a relative path is not allowed to go outside the store path
+cat > $flakeFollowsA/flake.nix <<EOF
+{
+    description = "Flake A";
+    inputs = {
+        B.url = "path:./../../flakeB";
+    };
+    outputs = { ... }: {};
+}
+EOF
+
+git -C $flakeFollowsA add flake.nix
+
+nix flake lock $flakeFollowsA 2>&1 | grep 'this is a security violation'