#912 allow leading period #9867

roberth · 2024-01-27T10:40:51Z

Motivation

#9095 seems to have caused more harm than good. See #9095 (comment) for argumentation.

Closes Support file paths with a dotted basename #912

Context

Preceding

Document changes in store path name validity #9091
StorePath: reject names starting with '.' #9095
[Backport 2.13-maintenance] StorePath: reject names starting with '.', Fix typo in error message of too long store path #9224
[Backport 2.14-maintenance] [Backport 2.15-maintenance] StorePath: reject names starting with '.', Fix typo in error message of too long store path #9219
[Backport 2.15-maintenance] StorePath: reject names starting with '.', Fix typo in error message of too long store path #9130
[Backport 2.16-maintenance] StorePath: reject names starting with '.' #9120
[Backport 2.17-maintenance] StorePath: reject names starting with '.' #9121
[Backport 2.18-maintenance] StorePath: reject names starting with '.' #9122

Backports of this

[Backport 2.20-maintenance] #912 allow leading period #9891

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

This reverts commit 24bda0c.

edolstra · 2024-01-29T11:33:41Z

I think this sets a bad precedent of having to support bugs that accidentally snuck in forever. Also, flip-flopping on this behaviour again just makes the situation even more confusing.

roberth · 2024-01-29T12:39:39Z

bug

It was requested as a feature, #912

a bad precedent of having to support bugs that accidentally snuck in forever.

What if denying . is the bug?

a bad precedent

Since when do we make decisions based on precedent?
You're basically suggesting that we're just imitating the past, and we're not allowed to correct our mistakes?

The author of #9095 has shown no interest in helping users with the consequences of their PR. I don't blame them for that; they can have all sorts of reasons, and it's a risk I took when accepting their PR.
However, unlike them, I am a maintainer, and that makes me responsible for this issue by default.

If I were to hand responsibility to you, how would you deal with the problem? We have users with broken stores now, who can't upgrade their Nix. Do you plan to fix the problem in a different way? What about #912?

roberth · 2024-01-30T17:50:09Z

@Ericson2314 this fixes two Arbitrary instances. Some more still remain.

Ericson2314 · 2024-01-30T17:58:53Z

Glad to hear it!

doc/manual/rl-next/leading-period.md

Ericson2314 · 2024-01-31T17:18:44Z

src/libstore/path-regex.hh

@@ -3,6 +3,6 @@

 namespace nix {

-static constexpr std::string_view nameRegexStr = R"([0-9a-zA-Z\+\-_\?=][0-9a-zA-Z\+\-\._\?=]*)";
+static constexpr std::string_view nameRegexStr = R"((?!\.\.?(-|$))[0-9a-zA-Z\+\-\._\?=]+)";


I don't quite understand this one. Maybe it needs some comments?

Ericson2314 · 2024-01-31T17:21:44Z

src/libstore/path.cc

+        if (name.size() == 1)
+            throw BadStorePath("store path '%s' has invalid name '%s'", path, name);
+        if (name[1] == '-')
+            throw BadStorePath("store path '%s' has invalid name '%s': first dash-separated component must not be '%s'", path, name, ".");


Could pull this out into a lambda (with param for . or ..) and use twice

They're slightly different though, because of the second error message.
I don't think the lambda would help with understanding or reuse.

Ericson2314

Looks good! None of my nits be low I care very much about :)

Gen::just is the constant generator. Don't just return that!

As discussed in the maintainer meeting on 2024-01-29. Mainly this is to avoid a situation where the name is parsed and treated as a file name, mostly to protect users. .-* and ..-* are also considered invalid because they might strip on that separator to remove versions. Doesn't really work, but that's what we decided, and I won't argue with it, because .-* probably doesn't seem to have a real world application anyway. We do still permit a 1-character name that's just "-", which still poses a similar risk in such a situation. We can't start disallowing trailing -, because a non-zero number of users will need it and we've seen how annoying and painful such a change is. What matters most is preventing a situation where . or .. can be injected, and to just get this done.

github-actions · 2024-01-31T18:11:16Z

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

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.19-maintenance
git worktree add -d .worktree/backport-9867-to-2.19-maintenance origin/2.19-maintenance
cd .worktree/backport-9867-to-2.19-maintenance
git switch --create backport-9867-to-2.19-maintenance
git cherry-pick -x 9ddd0f2af8fd95e1380027a70d0aa650ea2fd5e4 b13e6a76b4f289c6db69ffaa7bd35b7e44f2a391 69bbd5852af9b2f0b794162bd1debcdf64fc6648 8406da28773f050e00a006e4812e3ecbf919a2a9 f1b4663805a9dbcb1ace64ec110092d17c9155e0

github-actions · 2024-01-31T18:11:19Z

Successfully created backport PR for 2.20-maintenance:

[Backport 2.20-maintenance] #912 allow leading period #9891

thufschmitt · 2024-02-02T10:39:46Z

(bit late, but): Discussed during the Nix maintainers meeting on 2024-01-30.
Agreement to merge with a few extra restrictions (forbid things like . or ..).

Path with a leading . used to be forbidden, then got allowed by accident, and forbidden again
- Problematic because users might now have invalid paths in their store, that Nix can't do anything with
- Suggestion by @roberth: Allow them for good
@edolstra: Is it just a matter of a few people having them in their store? Because that can be worked around
@thufschmitt: What are the cost of allowing that for good?
- @roberth: 2.3 doesn't support them
- Might be security-relevant?
  - Dotfiles being hidden, this might be relied on for security reasons
  - Example: Some nixpkgs code somewhat relying on things not starting with a dot
@edolstra: These names are meant to be package names, not arbitrary ones (and we actually forbid a lot of characters already). What would it mean to have a package starting with a .?
@roberth: Nix is also used as a config manager. Shouldn't we use it for that?
@edolstra: Is there any package manager that allows package names starting with a .
- @tomberek: At least RPM does
@thufschmitt: If we don't allow these, how do make sure that people who already have them don't get bitten?
- No real solution except allowing them internally (just forbid creating new ones)
Need to forbid things like ., .., ..-* and the like
- hopefully reasonable restriction: Require at least one alphanumeric character before the a
- also update the Allow gc-ing with a rootless daemon #5380 copy of the regex

Agreement

nixos-discourse · 2024-02-02T11:32:44Z

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

https://discourse.nixos.org/t/2024-01-30-nix-team-meeting-minutes-119/39185/1

ryantm · 2024-03-21T20:11:16Z

Did this get rolled back? I'm seeing

error: store path '1gzd3gdn5ff8ndinqdxsbpbw08yzcl87-.gitignore' starts with illegal character '.'

when running sudo nix-collect-garbage -d
in nix 2.18.2 and nix 2.21.0

roberth · 2024-03-21T22:13:49Z

@ryantm It didn't get reverted. Maybe you're getting this from a daemon or remote builder?

ryantm · 2024-03-22T02:18:51Z

@roberth Thanks for the quick reply, that was a good hint. I had:

$ nix store ping
Store URL: daemon
Version: 2.18.2
Trusted: 1

then I updated my NixOS nix to be

$ nix store info
Store URL: daemon
Version: 2.21.0
Trusted: 1

and garbage collection worked fine.

nixos-discourse · 2024-03-23T09:47:34Z

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

nixos-discourse · 2024-03-23T11:58:41Z

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/3

github-actions · 2024-03-26T10:09:27Z

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

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.18-maintenance
git worktree add -d .worktree/backport-9867-to-2.18-maintenance origin/2.18-maintenance
cd .worktree/backport-9867-to-2.18-maintenance
git switch --create backport-9867-to-2.18-maintenance
git cherry-pick -x 9ddd0f2af8fd95e1380027a70d0aa650ea2fd5e4 b13e6a76b4f289c6db69ffaa7bd35b7e44f2a391 69bbd5852af9b2f0b794162bd1debcdf64fc6648 8406da28773f050e00a006e4812e3ecbf919a2a9 f1b4663805a9dbcb1ace64ec110092d17c9155e0

github-actions · 2024-03-26T10:09:28Z

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

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.19-maintenance
git worktree add -d .worktree/backport-9867-to-2.19-maintenance origin/2.19-maintenance
cd .worktree/backport-9867-to-2.19-maintenance
git switch --create backport-9867-to-2.19-maintenance
git cherry-pick -x 9ddd0f2af8fd95e1380027a70d0aa650ea2fd5e4 b13e6a76b4f289c6db69ffaa7bd35b7e44f2a391 69bbd5852af9b2f0b794162bd1debcdf64fc6648 8406da28773f050e00a006e4812e3ecbf919a2a9 f1b4663805a9dbcb1ace64ec110092d17c9155e0

github-actions · 2024-03-26T10:09:29Z

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

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.20-maintenance
git worktree add -d .worktree/backport-9867-to-2.20-maintenance origin/2.20-maintenance
cd .worktree/backport-9867-to-2.20-maintenance
git switch --create backport-9867-to-2.20-maintenance
git cherry-pick -x 9ddd0f2af8fd95e1380027a70d0aa650ea2fd5e4 b13e6a76b4f289c6db69ffaa7bd35b7e44f2a391 69bbd5852af9b2f0b794162bd1debcdf64fc6648 8406da28773f050e00a006e4812e3ecbf919a2a9 f1b4663805a9dbcb1ace64ec110092d17c9155e0

thufschmitt · 2024-03-26T10:09:39Z

This doesn't seem to have been backported to 2.18? We need to

edef1c · 2024-04-16T17:58:15Z

The author of #9095 has shown no interest in helping users with the consequences of their PR. I don't blame them for that; they can have all sorts of reasons, and it's a risk I took when accepting their PR.
However, unlike them, I am a maintainer, and that makes me responsible for this issue by default.

To be clear, my initial proposal (#9091) was to document and codify the change in behaviour, rather than revert it, given that it was likely to have snuck into real-world use already, as experience has now indeed borne out.

I can't spend all my time glued to GitHub currently, so this is the first time I'm seeing the fallout. It seemed like a perhaps risky but worthwhile experiment, that you endorsed as a maintainer (before code for the stricter validation was even written), and I'm fine with how this has concluded.

We don't have an "ecosystem test run" environment like Rust's Crater, so I would not (and could not) have undertaken a live-fire test like this without a maintainer taking responsibility for it. Building a Crater equivalent seems abstractly worthwhile, but right now I don't have the bandwidth for such an undertaking, and I don't particularly expect anyone to volunteer.

As far as I can tell, the only alternative unexplored was making it a warning and treating it as an explicitly deprecated behaviour, but I don't think that would have done much good.

My sole goal here was to ensure that there is clear agreement on what constitutes in-spec Nix behaviour, so that other implementers (such as the TVL nix-compat Rust library, which I co-maintain) can match it in behaviour exactly, since we lack a formal specification.

roberth · 2024-04-17T10:42:16Z

Hi @edef1c and thank you for your response.
We all did the best we could do with our limited resources.

You're making good points about the ecosystem test run and a formal specification. We're trying to improve both, but for the formal specification, that takes a different form. Making a separate spec document is an effort that's hard to justify (for the Nix team), but we are making the documentation more precise. A spec is also reference documentation, so I think there's a lot that can be done about this issue, just by improving the Nix manual. So, contributions are welcomed; especially small and incremental ones.

Nix 2.4 accidentally permitted this behaviour, but the revert came too late to beat Hyrum's law. It is now considered permissible. Link: NixOS/nix#9867 Change-Id: Ie97777af6765fe1c12c8aa593afe1c9b69125775 Reviewed-on: https://cl.tvl.fyi/c/depot/+/11553 Tested-by: BuildkiteCI Reviewed-by: flokli <[email protected]>

nixos-discourse · 2024-06-27T13:02:33Z

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

https://discourse.nixos.org/t/broken-nix-store-path-starts-with-illegal-character/47776/1

vcunat · 2024-09-17T10:01:55Z

I believe this hasn't been applied to 2.19 (yet). Though 9ddd0f2 would at least apply cleanly.

roberth · 2024-09-17T10:04:28Z

We don't actively support 2.19, but feel free to open a PR

vcunat · 2024-09-17T10:11:41Z

Sad that the current Hydra is based on 2.19.

Revert "StorePath: reject names starting with '.'"

9ddd0f2

This reverts commit 24bda0c.

roberth requested review from thufschmitt and edolstra as code owners January 27, 2024 10:40

roberth mentioned this pull request Jan 27, 2024

StorePath: reject names starting with '.' #9095

Merged

roberth added the feature Feature request or proposal label Jan 27, 2024

roberth force-pushed the issue-912 branch from a3ca189 to b315325 Compare January 27, 2024 10:47

roberth added the bug label Jan 29, 2024

roberth force-pushed the issue-912 branch from 3bcda83 to 86235ce Compare January 30, 2024 17:49

roberth requested review from Ericson2314 and removed request for edolstra and thufschmitt January 30, 2024 17:49

roberth added backport 2.19-maintenance Automatically creates a PR against the branch backport 2.20-maintenance Automatically creates a PR against the branch labels Jan 30, 2024

Ericson2314 reviewed Jan 31, 2024

View reviewed changes

doc/manual/rl-next/leading-period.md Outdated Show resolved Hide resolved

Ericson2314 reviewed Jan 31, 2024

View reviewed changes

Ericson2314 approved these changes Jan 31, 2024

View reviewed changes

roberth added 4 commits January 31, 2024 18:35

parseStorePath: Support leading period

b13e6a7

test: Generate distinct path names

69bbd58

Gen::just is the constant generator. Don't just return that!

test: Generate distinct hashes

8406da2

Gen::just is the constant generator. Don't just return that!

roberth force-pushed the issue-912 branch from 86235ce to f1b4663 Compare January 31, 2024 17:36

roberth enabled auto-merge January 31, 2024 17:38

roberth merged commit 4072a8f into NixOS:master Jan 31, 2024
8 checks passed

github-actions bot mentioned this pull request Jan 31, 2024

[Backport 2.20-maintenance] #912 allow leading period #9891

Merged

roberth mentioned this pull request Feb 9, 2024

Store paths in db should not be validated for leading . #9624

Closed

zarybnicky mentioned this pull request Mar 23, 2024

Nix 2.19 compatibility: remove leading dot from local file pnp.loader.mjs madjam002/yarnpnp2nix#18

Closed

thufschmitt added the backport 2.18-maintenance Automatically creates a PR against the branch label Mar 26, 2024

This was referenced Mar 26, 2024

Cannot collect garbage: store path contains illegal character '!' #5176

Closed

[Backport 2.18-maintenance] allow leading period #10327

Merged

Ma27 mentioned this pull request Apr 1, 2024

nixVersions.nix_2_{3,20}: update patch versions NixOS/nixpkgs#296709

Merged

13 tasks

Theaninova mentioned this pull request Apr 9, 2024

error: store path starts with illegal character '.' nix-community/vulnix#98

Open

Ma27 mentioned this pull request Apr 14, 2024

Backport of fix for leading dot in store paths to 2.17? #10497

Closed

ysndr mentioned this pull request Oct 9, 2024

refactor(activate): shfmt most scripts flox/flox#2215

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#912 allow leading period #9867

#912 allow leading period #9867

roberth commented Jan 27, 2024 •

edited

Loading

edolstra commented Jan 29, 2024

roberth commented Jan 29, 2024

roberth commented Jan 30, 2024

Ericson2314 commented Jan 30, 2024

Ericson2314 Jan 31, 2024

roberth Jan 31, 2024

Ericson2314 Jan 31, 2024

roberth Jan 31, 2024

Ericson2314 left a comment

github-actions bot commented Jan 31, 2024

github-actions bot commented Jan 31, 2024

thufschmitt commented Feb 2, 2024 •

edited by fricklerhandwerk

Loading

nixos-discourse commented Feb 2, 2024

ryantm commented Mar 21, 2024

roberth commented Mar 21, 2024

ryantm commented Mar 22, 2024

nixos-discourse commented Mar 23, 2024

nixos-discourse commented Mar 23, 2024

github-actions bot commented Mar 26, 2024

github-actions bot commented Mar 26, 2024

github-actions bot commented Mar 26, 2024

thufschmitt commented Mar 26, 2024

edef1c commented Apr 16, 2024 •

edited

Loading

roberth commented Apr 17, 2024

nixos-discourse commented Jun 27, 2024

vcunat commented Sep 17, 2024

roberth commented Sep 17, 2024

vcunat commented Sep 17, 2024

#912 allow leading period #9867

#912 allow leading period #9867

Conversation

roberth commented Jan 27, 2024 • edited Loading

Motivation

Context

Priorities and Process

edolstra commented Jan 29, 2024

roberth commented Jan 29, 2024

roberth commented Jan 30, 2024

Ericson2314 commented Jan 30, 2024

Ericson2314 Jan 31, 2024

Choose a reason for hiding this comment

roberth Jan 31, 2024

Choose a reason for hiding this comment

Ericson2314 Jan 31, 2024

Choose a reason for hiding this comment

roberth Jan 31, 2024

Choose a reason for hiding this comment

Ericson2314 left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 31, 2024

github-actions bot commented Jan 31, 2024

thufschmitt commented Feb 2, 2024 • edited by fricklerhandwerk Loading

nixos-discourse commented Feb 2, 2024

ryantm commented Mar 21, 2024

roberth commented Mar 21, 2024

ryantm commented Mar 22, 2024

nixos-discourse commented Mar 23, 2024

nixos-discourse commented Mar 23, 2024

github-actions bot commented Mar 26, 2024

github-actions bot commented Mar 26, 2024

github-actions bot commented Mar 26, 2024

thufschmitt commented Mar 26, 2024

edef1c commented Apr 16, 2024 • edited Loading

roberth commented Apr 17, 2024

nixos-discourse commented Jun 27, 2024

vcunat commented Sep 17, 2024

roberth commented Sep 17, 2024

vcunat commented Sep 17, 2024

roberth commented Jan 27, 2024 •

edited

Loading

thufschmitt commented Feb 2, 2024 •

edited by fricklerhandwerk

Loading

edef1c commented Apr 16, 2024 •

edited

Loading