Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLI symlink fixes #12046

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

CLI symlink fixes #12046

wants to merge 7 commits into from

Conversation

roberth
Copy link
Member

@roberth roberth commented Dec 13, 2024

Motivation

PosixSourceAccessor does not operate on non-canonical paths, yet many createAtRoot calls did not canonicalize their inputs.

See commit messages and comments for details.

Probably more createAtRoot calls need this kind of treatment, but that's not a prerequisite for reviewing what's probably the bulk of the change, and the ideas behind it.

Context


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 documentation new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority labels Dec 13, 2024
@roberth roberth marked this pull request as ready for review December 13, 2024 18:19
src/libutil/file-system.cc Outdated Show resolved Hide resolved
@Mic92 Mic92 added the bug label Dec 14, 2024
@Mic92
Copy link
Member

Mic92 commented Dec 14, 2024

@mergify rebase

Copy link
Contributor

mergify bot commented Dec 14, 2024

rebase

✅ Branch has been successfully rebased

@Mic92
Copy link
Member

Mic92 commented Dec 15, 2024

Tests seem to pass now. @roberth do you want to squash things a bit, so we can backport patches?

@roberth roberth added the regression Something doesn't work anymore label Dec 16, 2024
@roberth
Copy link
Member Author

roberth commented Dec 16, 2024

  • added hydraJobs.tests.functional_symlinked-home

squash things a bit

  • rearranged the commits to squash all improvements that are internal to the PR. The only remaining refactor commit is relative to master and should be separate.

@Mic92 Mic92 force-pushed the cli-symlink-fixes branch 2 times, most recently from 5b521cf to 12d4c8d Compare December 16, 2024 13:49
* callers do not accidentally introduce symlink-related security vulnerabilities.
* Furthermore, `createAtRoot` does not know whether the file pointed to by
* `path` should be resolved if it is itself a symlink. In other words,
* `createAtRoot` can not decide between aforemention `canonical`, `makeParentCanonical`, etc. for its callers.
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
* `createAtRoot` can not decide between aforemention `canonical`, `makeParentCanonical`, etc. for its callers.
* `createAtRoot` can not decide between aforementioned `canonical`, `makeParentCanonical`, etc. for its callers.

src/nix/hash.cc Outdated
@@ -87,18 +87,35 @@ struct CmdHashBase : Command
return std::make_unique<HashSink>(hashAlgo);
};

auto path2 = PosixSourceAccessor::createAtRoot(path);
auto makeSourcePath = [&]() -> SourcePath {
return PosixSourceAccessor::createAtRoot(std::filesystem::weakly_canonical(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.

Suggested change
return PosixSourceAccessor::createAtRoot(std::filesystem::weakly_canonical(path));
return PosixSourceAccessor::createAtRoot(makeParentCanonical(path));

Needs test!

src/nix/hash.cc Outdated
h = hashSink->finish().first;
break;
}
case FileIngestionMethod::Git: {
auto sourcePath = PosixSourceAccessor::createAtRoot(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.

Suggested change
auto sourcePath = PosixSourceAccessor::createAtRoot(path);
auto sourcePath = makeSourcePath();

# nix hash --mode nar does not canonicalize a symlink argument.
# Otherwise it can't generate a NAR whose root is a symlink.
# If you want to follow the symlink, pass $(realpath -s ...) instead.
ln -s /non-existent-48cujwe8ndf4as0bne "$TEST_ROOT/symlink-to-nowhere"
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
ln -s /non-existent-48cujwe8ndf4as0bne "$TEST_ROOT/symlink-to-nowhere"
ln -s /non-existent-48cujwe8ndf4as0bne "$TEST_ROOT/symlink-to-nowhere"
[[ -e /bin ]] # assumption
ln -s /bin "$TEST_ROOT/symlink-to-bin"

or something, do both.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-12-16-nix-team-meeting-minutes-203/57483/1

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-12-18-nix-team-meeting-minutes-204/57602/1

Tested with

    nix run nix/2.3-maintenance#nix-store -- --add some_symlink
    nix run nix/2.3-maintenance#nix-store -- --add-fixed sha256 --recursive some_symlink
@Mic92
Copy link
Member

Mic92 commented Dec 23, 2024

Please undraft, if you need a new review.

@Mic92 Mic92 marked this pull request as draft December 23, 2024 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug documentation new-cli Relating to the "nix" command regression Something doesn't work anymore with-tests Issues related to testing. PRs with tests have some priority
Projects
Status: 🏁 Review
4 participants