From b3c424f5a6a4d7c16a42e54de736698d5b21fad5 Mon Sep 17 00:00:00 2001 From: Alex Zero Date: Mon, 12 Jul 2021 02:07:53 +0100 Subject: [PATCH] Fix follows paths in subordinate lockfiles --- src/libexpr/flake/flake.cc | 67 ++++++++++++++++++++++++++++--------- src/libexpr/flake/flake.hh | 10 +++++- src/libfetchers/fetchers.hh | 3 ++ src/libfetchers/path.cc | 22 ++++++++++-- 4 files changed, 82 insertions(+), 20 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, const InputPath & inputPathPrefix, - std::shared_ptr oldNode)> + std::shared_ptr oldNode, + const LockParent parent, const Path parentPath)> computeLocks; computeLocks = [&]( const FlakeInputs & flakeInputs, std::shared_ptr node, const InputPath & inputPathPrefix, - std::shared_ptr oldNode) + std::shared_ptr 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(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 ref; bool isFlake = true; // true = process flake to get outputs, false = (fetched) static source path std::optional follows; - bool absolute = false; // whether 'follows' is relative to the flake root FlakeInputs overrides; }; @@ -125,6 +124,15 @@ struct LockFlags std::set 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 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 fetch(ref 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); - auto storePath = store->maybeParseStorePath(path); + // the path isn't relative, prefix it + absPath = canonPath(parent + "/" + 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)),