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

StorePath: reject names starting with '.' #9095

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

edef1c
Copy link
Member

@edef1c edef1c commented Oct 4, 2023

Motivation

This has been the behaviour before Nix 2.4. It was dropped in a rewrite in 759947b, allowing the creation of store paths that aren't considered valid by older Nix versions or other Nix tooling.

Context

Nix 2.4 didn't ship in NixOS until 22.05, and stdenv.mkDerivation in nixpkgs drops leading periods since April 2022, so it's unlikely anyone is relying on the current lax behaviour.

@edef1c edef1c requested a review from thufschmitt as a code owner October 4, 2023 16:54
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Oct 4, 2023
@Ericson2314
Copy link
Member

Can you update Arbitrary<StorePathName> too? It didn't fail because all the property testing is kinda borked right now.


To be honest, I am not mustering much enthusiasm for this issue either way, because me wishing store paths didn't have names at all is drowning out any feelings on what those names should look like.

@edef1c edef1c force-pushed the reject-dot-paths branch 2 times, most recently from bb0c8a1 to 7e8c88f Compare October 4, 2023 17:08
@edef1c
Copy link
Member Author

edef1c commented Oct 4, 2023

Can you update Arbitrary<StorePathName> too? It didn't fail because all the property testing is kinda borked right now.

Done.

tvlbot pushed a commit to tvlfyi/tvix that referenced this pull request Oct 4, 2023
Nix has historically rejected these. The current behaviour was
accidentally introduced in Nix 2.4, and is considered a bug.

Link: NixOS/nix#9095
Change-Id: I38ffa911f0a413086479bd972de09671dbe85121
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9507
Reviewed-by: flokli <[email protected]>
Tested-by: BuildkiteCI
Autosubmit: edef <[email protected]>
@Ericson2314
Copy link
Member

Hmm I am surprised we didn't need the attribute explicit fall-through thing to appease a -Werror=....

@Ericson2314
Copy link
Member

@roberth said

This makes actual use of the "feature" rather unlikely albeit not entirely impossible.

Considering also there's a tiny but non-zero chance that someone's code relies on the rejection behavior for security, I'd rather treat it as a bug.

Tidy chance of security issue from relaxation > slightly less tiny chance of breakage retightening is fine enough reasoning for me. +1 on this approach then.

This has been the behaviour before Nix 2.4. It was dropped in a rewrite
in 759947b, allowing the creation of
store paths that aren't considered valid by older Nix versions or other
Nix tooling.

Nix 2.4 didn't ship in NixOS until 22.05, and stdenv.mkDerivation in
nixpkgs drops leading periods since April 2022, so it's unlikely anyone
is relying on the current lax behaviour.

Closes NixOS#9091.

Change-Id: I4a57bd9899e1b0dba56870ae5a1b680918a18ce9
@edef1c
Copy link
Member Author

edef1c commented Oct 4, 2023

Looks like I forgot to update the name regex, which the tests fortunately also check. Fixed.

@edolstra edolstra merged commit 6243495 into NixOS:master Oct 6, 2023
8 checks passed
@edef1c
Copy link
Member Author

edef1c commented Oct 8, 2023

Do we want to backport this to releases that shipped with the broken check?
cc @roberth

@roberth roberth added backport 2.13-maintenance backport 2.18-maintenance Automatically creates a PR against the branch labels Oct 8, 2023
@github-actions
Copy link

github-actions bot commented Oct 8, 2023

Backport failed for 2.13-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin 2.13-maintenance
git worktree add -d .worktree/backport-9095-to-2.13-maintenance origin/2.13-maintenance
cd .worktree/backport-9095-to-2.13-maintenance
git checkout -b backport-9095-to-2.13-maintenance
ancref=$(git merge-base 2f1c16dfa2378fd8616bff1b9b7cd0b4d42af69b 24bda0c7b381e1a017023c6f7cb9661fae8560bd)
git cherry-pick -x $ancref..24bda0c7b381e1a017023c6f7cb9661fae8560bd

1 similar comment
@github-actions
Copy link

github-actions bot commented Oct 8, 2023

Backport failed for 2.13-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin 2.13-maintenance
git worktree add -d .worktree/backport-9095-to-2.13-maintenance origin/2.13-maintenance
cd .worktree/backport-9095-to-2.13-maintenance
git checkout -b backport-9095-to-2.13-maintenance
ancref=$(git merge-base 2f1c16dfa2378fd8616bff1b9b7cd0b4d42af69b 24bda0c7b381e1a017023c6f7cb9661fae8560bd)
git cherry-pick -x $ancref..24bda0c7b381e1a017023c6f7cb9661fae8560bd

@github-actions
Copy link

github-actions bot commented Oct 8, 2023

Backport failed for 2.14-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin 2.14-maintenance
git worktree add -d .worktree/backport-9095-to-2.14-maintenance origin/2.14-maintenance
cd .worktree/backport-9095-to-2.14-maintenance
git checkout -b backport-9095-to-2.14-maintenance
ancref=$(git merge-base 2f1c16dfa2378fd8616bff1b9b7cd0b4d42af69b 24bda0c7b381e1a017023c6f7cb9661fae8560bd)
git cherry-pick -x $ancref..24bda0c7b381e1a017023c6f7cb9661fae8560bd

1 similar comment
@github-actions
Copy link

github-actions bot commented Oct 8, 2023

Backport failed for 2.14-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin 2.14-maintenance
git worktree add -d .worktree/backport-9095-to-2.14-maintenance origin/2.14-maintenance
cd .worktree/backport-9095-to-2.14-maintenance
git checkout -b backport-9095-to-2.14-maintenance
ancref=$(git merge-base 2f1c16dfa2378fd8616bff1b9b7cd0b4d42af69b 24bda0c7b381e1a017023c6f7cb9661fae8560bd)
git cherry-pick -x $ancref..24bda0c7b381e1a017023c6f7cb9661fae8560bd

@github-actions
Copy link

github-actions bot commented Oct 8, 2023

Backport failed for 2.13-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin 2.13-maintenance
git worktree add -d .worktree/backport-9095-to-2.13-maintenance origin/2.13-maintenance
cd .worktree/backport-9095-to-2.13-maintenance
git checkout -b backport-9095-to-2.13-maintenance
ancref=$(git merge-base 2f1c16dfa2378fd8616bff1b9b7cd0b4d42af69b 24bda0c7b381e1a017023c6f7cb9661fae8560bd)
git cherry-pick -x $ancref..24bda0c7b381e1a017023c6f7cb9661fae8560bd

@github-actions
Copy link

github-actions bot commented Oct 8, 2023

Backport failed for 2.15-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin 2.15-maintenance
git worktree add -d .worktree/backport-9095-to-2.15-maintenance origin/2.15-maintenance
cd .worktree/backport-9095-to-2.15-maintenance
git checkout -b backport-9095-to-2.15-maintenance
ancref=$(git merge-base 2f1c16dfa2378fd8616bff1b9b7cd0b4d42af69b 24bda0c7b381e1a017023c6f7cb9661fae8560bd)
git cherry-pick -x $ancref..24bda0c7b381e1a017023c6f7cb9661fae8560bd

1 similar comment
@github-actions
Copy link

github-actions bot commented Oct 8, 2023

Backport failed for 2.15-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin 2.15-maintenance
git worktree add -d .worktree/backport-9095-to-2.15-maintenance origin/2.15-maintenance
cd .worktree/backport-9095-to-2.15-maintenance
git checkout -b backport-9095-to-2.15-maintenance
ancref=$(git merge-base 2f1c16dfa2378fd8616bff1b9b7cd0b4d42af69b 24bda0c7b381e1a017023c6f7cb9661fae8560bd)
git cherry-pick -x $ancref..24bda0c7b381e1a017023c6f7cb9661fae8560bd

@github-actions
Copy link

github-actions bot commented Oct 8, 2023

Backport failed for 2.14-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin 2.14-maintenance
git worktree add -d .worktree/backport-9095-to-2.14-maintenance origin/2.14-maintenance
cd .worktree/backport-9095-to-2.14-maintenance
git checkout -b backport-9095-to-2.14-maintenance
ancref=$(git merge-base 2f1c16dfa2378fd8616bff1b9b7cd0b4d42af69b 24bda0c7b381e1a017023c6f7cb9661fae8560bd)
git cherry-pick -x $ancref..24bda0c7b381e1a017023c6f7cb9661fae8560bd

@github-actions
Copy link

github-actions bot commented Oct 8, 2023

Git push to origin failed for 2.16-maintenance with exitcode 1

@github-actions
Copy link

github-actions bot commented Oct 8, 2023

Backport failed for 2.14-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin 2.14-maintenance
git worktree add -d .worktree/backport-9095-to-2.14-maintenance origin/2.14-maintenance
cd .worktree/backport-9095-to-2.14-maintenance
git checkout -b backport-9095-to-2.14-maintenance
ancref=$(git merge-base 2f1c16dfa2378fd8616bff1b9b7cd0b4d42af69b 24bda0c7b381e1a017023c6f7cb9661fae8560bd)
git cherry-pick -x $ancref..24bda0c7b381e1a017023c6f7cb9661fae8560bd

@github-actions
Copy link

github-actions bot commented Oct 8, 2023

Backport failed for 2.15-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin 2.15-maintenance
git worktree add -d .worktree/backport-9095-to-2.15-maintenance origin/2.15-maintenance
cd .worktree/backport-9095-to-2.15-maintenance
git checkout -b backport-9095-to-2.15-maintenance
ancref=$(git merge-base 2f1c16dfa2378fd8616bff1b9b7cd0b4d42af69b 24bda0c7b381e1a017023c6f7cb9661fae8560bd)
git cherry-pick -x $ancref..24bda0c7b381e1a017023c6f7cb9661fae8560bd

@github-actions
Copy link

github-actions bot commented Oct 8, 2023

Git push to origin failed for 2.16-maintenance with exitcode 1

@github-actions
Copy link

github-actions bot commented Oct 8, 2023

Git push to origin failed for 2.17-maintenance with exitcode 1

@github-actions
Copy link

github-actions bot commented Oct 8, 2023

Git push to origin failed for 2.18-maintenance with exitcode 1

@roberth
Copy link
Member

roberth commented Oct 8, 2023

Sorry for the spam. Looks the action did n² or something. Haven't seen it do that before.

I'd like to have backports back to 2.13 which is in the NixOS release. Beyond that I don't think we can (or should) support anything.

We might want to decide on a backport policy...

@roberth roberth mentioned this pull request Oct 8, 2023
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-10-06-nix-team-meeting-minutes-92/34001/1

@PetarKirov
Copy link

PetarKirov commented Dec 4, 2023

@edef1c This change broke the evaluation of my home-manager configuration. This happened while upgrading the CI of my config to Nix 2.19.1 (via cachix/install-nix-action@v24). I had no idea that my configuration was somehow depending on the lax path validation. Any idea how users like me can workaround this issue?

error:
       … while calling the 'derivationStrict' builtin

         at /derivation-internal.nix:9:12:

            8|
            9|   strict = derivationStrict drvAttrs;
             |            ^
           10|

       … while evaluating derivation 'home-manager-generation'
[... stack trace partially omitted]

       … while evaluating derivation 'home-manager-files'
         whose name attribute is located at /nix/store/gjd1iiwgwv7x29b21ng2dpysbgjp6kxr-source/pkgs/stdenv/generic/make-derivation.nix:348:7

[... long stack trace partially omitted]

       error: store path 'g6yc5a3ibnqwvs1ijxxwiij758384xg3-.gitconfig' starts with illegal character '.'

@ailocam
Copy link

ailocam commented Dec 14, 2023

This change breaks interpolating hidden file paths such as ${./.test.env} in strings on 2.19.1.
I did find a workaround ${builtins.path {path = ./.test.env; name = "test.env";} }

@jaen
Copy link

jaen commented Dec 15, 2023

I've already had paths with dots in my store and this change breaks basic commands such as nix-collect-garbage or nix-store --delete, so I can't even reliably get rid of those paths from my store now (other than downgrading nix).

I think at the very least this change shouldn't break basic commands like those.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-12-18-nix-team-meeting-minutes-113/37050/1

@accelbread
Copy link
Contributor

This change causes evaluation failures for existing Nix expressions that used paths to files starting with '.'. I've had to have others downgrade to 2.18 due to this. Should reverting this change be considered?

@roberth
Copy link
Member

roberth commented Jan 11, 2024

@edef1c How would you feel about changing - to call it that way - the Nix Specification to just allow names to start with a .?

It turns out that people have actually come to rely on it, so we might just call this an experiment with a negative outcome.

Furthermore it's causing real issues with Nix rejecting its store, and I feel like implementing a workaround is just a waste of time.

Also there's some utility to it: #912.

Considering also there's a tiny but non-zero chance that someone's code relies on the rejection behavior for security

And this does seem far fetched.

If you agree, I'll take care of reverting this, and communicating the change in the release notes.

@adrian-gierakowski
Copy link

This change broke my builds. Unless there is good reason not to support the dot I’d love to see this reverted.

@roberth
Copy link
Member

roberth commented Jan 27, 2024

I'd like to move ahead with #9095 (comment) in the form of

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/store-path-starts-with-illegal-character/42050/2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.18-maintenance Automatically creates a PR against the branch 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.

10 participants