{"thread":{"id":"61386","subject":"[PATCH 8/8] refs: check refnames as fully qualified when resolving","startedAt":"2024-04-29T08:35:34Z","lastAt":"2024-05-06T06:22:22Z","messageCount":6,"participants":["Jeff King","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"493619","messageId":"20240429083533.GG233423@coredump.intra.peff.net","threadId":"61386","inReplyTo":null,"subject":"[PATCH 8/8] refs: check refnames as fully qualified when resolving","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-29T08:35:33Z","receivedAt":"2024-04-29T08:35:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Most code paths for resolving refs end up in refs_resolve_ref_unsafe(),\nwhich checks the names using check_refname_format(). The names we have\nat this stage are always full refnames, but we pass the ALLOW_ONELEVEL\nflag so that the function allows pseudorefs like MERGE_HEAD. We should\ninstead pass the FULLY_QUALIFIED flag, which lets check_refname_format()\ndo some extra syntactic checks on those pseudorefs.\n\nWith this patch we'll refuse to read anything outside of refs/ that does\nnot match the usual pseudoref syntax (all caps plus dashes). This should\nnot be a loss of functionality (since such refs cannot be written as of\nthe previous commit), but may protect us from mischief. For example, you\ncan ask for silly things like \"info/refs\", \"rr-cache/<sha1>/postimage\",\nor \"objects/info/commit-graphs/commit-graph-chain\". It's doubtful you\ncan really do anything _too_ terrible there, but it seems like peeking\nat random files in .git in response to possibly untrusted input is\nsomething we should avoid.\n\nNote that because we can't actually write such refs using Git, our tests\nhave to munge the filesystem manually (and hence only run with the files\nbackend). They also can only check that resolution fails (and not a\nspecific error message), since we don't return the \"bad name\" detail all\nthe way to the caller.\n\nThe second test here, for \"main-worktree/bad\", actually fails even\nwithout this patch. The worktree-ref code enforces the pseudoref syntax\nitself via is_current_worktree_ref(), so the extra checks for this in\ncheck_refname_format() are redundant (though the parsing there is still\nnecessary so we know _not_ to reject \"main-worktree/HEAD\"). But either\nway it's good to have a test which makes sure this remains the case.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs.c                  |  4 ++--\n t/t1430-bad-ref-name.sh | 10 ++++++++++\n 2 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 57663cfe9e..96f489861b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1951,7 +1951,7 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \n \t*flags = 0;\n \n-\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n+\tif (check_refname_format(refname, REFNAME_FULLY_QUALIFIED)) {\n \t\tif (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||\n \t\t    !refname_is_safe(refname))\n \t\t\treturn NULL;\n@@ -2010,7 +2010,7 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \t\t\toidclr(oid);\n \t\t\treturn refname;\n \t\t}\n-\t\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n+\t\tif (check_refname_format(refname, REFNAME_FULLY_QUALIFIED)) {\n \t\t\tif (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||\n \t\t\t    !refname_is_safe(refname))\n \t\t\t\treturn NULL;\ndiff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\nindex 120e1557d7..5fb780cb08 100755\n--- a/t/t1430-bad-ref-name.sh\n+++ b/t/t1430-bad-ref-name.sh\n@@ -400,4 +400,14 @@ test_expect_success 'update-ref refuses non-underscore outside of refs/' '\n \ttest_grep \"refusing to update ref with bad name\" err\n '\n \n+test_expect_success REFFILES 'rev-parse refuses non-pseudoref outside of refs/' '\n+\tgit rev-parse HEAD >.git/bad &&\n+\ttest_must_fail git rev-parse --verify bad\n+'\n+\n+test_expect_success REFFILES 'rev-parse recognizes non-pseudoref via worktree' '\n+\tgit rev-parse HEAD >.git/bad &&\n+\ttest_must_fail git rev-parse --verify main-worktree/bad\n+'\n+\n test_done\n-- \n2.45.0.rc1.416.gbe2a76c799\n"},{"id":"493726","messageId":"ZjB5dPoEoq8D6qzJ@tanuki","threadId":"61386","inReplyTo":"20240429083533.GG233423@coredump.intra.peff.net","subject":"Re: [PATCH 8/8] refs: check refnames as fully qualified when resolving","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-30T04:54:12Z","receivedAt":"2024-04-30T04:54:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Apr 29, 2024 at 04:35:33AM -0400, Jeff King wrote:\n> Most code paths for resolving refs end up in refs_resolve_ref_unsafe(),\n> which checks the names using check_refname_format(). The names we have\n> at this stage are always full refnames, but we pass the ALLOW_ONELEVEL\n> flag so that the function allows pseudorefs like MERGE_HEAD. We should\n> instead pass the FULLY_QUALIFIED flag, which lets check_refname_format()\n> do some extra syntactic checks on those pseudorefs.\n> \n> With this patch we'll refuse to read anything outside of refs/ that does\n> not match the usual pseudoref syntax (all caps plus dashes). This should\n\ns/dashes/underscores\n\n> not be a loss of functionality (since such refs cannot be written as of\n> the previous commit), but may protect us from mischief. For example, you\n> can ask for silly things like \"info/refs\", \"rr-cache/<sha1>/postimage\",\n> or \"objects/info/commit-graphs/commit-graph-chain\". It's doubtful you\n> can really do anything _too_ terrible there, but it seems like peeking\n> at random files in .git in response to possibly untrusted input is\n> something we should avoid.\n\nAgreed.\n\n[snip]\n> diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\n> index 120e1557d7..5fb780cb08 100755\n> --- a/t/t1430-bad-ref-name.sh\n> +++ b/t/t1430-bad-ref-name.sh\n> @@ -400,4 +400,14 @@ test_expect_success 'update-ref refuses non-underscore outside of refs/' '\n>  \ttest_grep \"refusing to update ref with bad name\" err\n>  '\n>  \n> +test_expect_success REFFILES 'rev-parse refuses non-pseudoref outside of refs/' '\n> +\tgit rev-parse HEAD >.git/bad &&\n> +\ttest_must_fail git rev-parse --verify bad\n> +'\n> +\n> +test_expect_success REFFILES 'rev-parse recognizes non-pseudoref via worktree' '\n> +\tgit rev-parse HEAD >.git/bad &&\n> +\ttest_must_fail git rev-parse --verify main-worktree/bad\n> +'\n\nAre these really specific to the REFFILES backend? I would expect that\nthe reftable backend sohuld fail to parse those, too. The fact that we\nwrite into the repository directly during the test setup doesn't change\nthis, because all this patch is about is that we don't want to parse\nrandom files in the Git repo. And that is something we should want to\nenforce for all backends.\n\nPatrick\n"},{"id":"493750","messageId":"20240430104152.GF1279403@coredump.intra.peff.net","threadId":"61386","inReplyTo":"ZjB5dPoEoq8D6qzJ@tanuki","subject":"Re: [PATCH 8/8] refs: check refnames as fully qualified when resolving","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-30T10:41:52Z","receivedAt":"2024-04-30T10:41:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 30, 2024 at 06:54:12AM +0200, Patrick Steinhardt wrote:\n\n> > diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\n> > index 120e1557d7..5fb780cb08 100755\n> > --- a/t/t1430-bad-ref-name.sh\n> > +++ b/t/t1430-bad-ref-name.sh\n> > @@ -400,4 +400,14 @@ test_expect_success 'update-ref refuses non-underscore outside of refs/' '\n> >  \ttest_grep \"refusing to update ref with bad name\" err\n> >  '\n> >  \n> > +test_expect_success REFFILES 'rev-parse refuses non-pseudoref outside of refs/' '\n> > +\tgit rev-parse HEAD >.git/bad &&\n> > +\ttest_must_fail git rev-parse --verify bad\n> > +'\n> > +\n> > +test_expect_success REFFILES 'rev-parse recognizes non-pseudoref via worktree' '\n> > +\tgit rev-parse HEAD >.git/bad &&\n> > +\ttest_must_fail git rev-parse --verify main-worktree/bad\n> > +'\n> \n> Are these really specific to the REFFILES backend? I would expect that\n> the reftable backend sohuld fail to parse those, too. The fact that we\n> write into the repository directly during the test setup doesn't change\n> this, because all this patch is about is that we don't want to parse\n> random files in the Git repo. And that is something we should want to\n> enforce for all backends.\n\nSo this is where I will show my ignorance of reftables. I assume it\nstill has to implement FETCH_HEAD as a file (since it holds extra data).\nBut does it do the same for other names outside of \"refs/\"? I am\nassuming not in the paragraph below.\n\nI would expect the test to succeed after my patches on any ref backend,\nsince we'd enforce the syntax outside of the backend-specific code. But\nfor a backend which does not look for the root name \"foo\" directly in\n.git/, it is not an interesting test. The looked-for name does not\nexist for it, so even if we did try the lookup, it would fail. We cannot\ndistinguish the two cases from the outcome we see.\n\nSo I think dropping REFFILES it would still pass, but we are not really\ntesting anything that interesting for reftables. That said, I would be\nOK dropping the REFFILES in the name of simplicity and just documenting\nit in the commit message.\n\n-Peff\n"},{"id":"493755","messageId":"ZjDVLAKA0_4pTAS7@tanuki","threadId":"61386","inReplyTo":"20240430104152.GF1279403@coredump.intra.peff.net","subject":"Re: [PATCH 8/8] refs: check refnames as fully qualified when resolving","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-30T11:25:32Z","receivedAt":"2024-04-30T11:25:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Apr 30, 2024 at 06:41:52AM -0400, Jeff King wrote:\n> On Tue, Apr 30, 2024 at 06:54:12AM +0200, Patrick Steinhardt wrote:\n> \n> > > diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\n> > > index 120e1557d7..5fb780cb08 100755\n> > > --- a/t/t1430-bad-ref-name.sh\n> > > +++ b/t/t1430-bad-ref-name.sh\n> > > @@ -400,4 +400,14 @@ test_expect_success 'update-ref refuses non-underscore outside of refs/' '\n> > >  \ttest_grep \"refusing to update ref with bad name\" err\n> > >  '\n> > >  \n> > > +test_expect_success REFFILES 'rev-parse refuses non-pseudoref outside of refs/' '\n> > > +\tgit rev-parse HEAD >.git/bad &&\n> > > +\ttest_must_fail git rev-parse --verify bad\n> > > +'\n> > > +\n> > > +test_expect_success REFFILES 'rev-parse recognizes non-pseudoref via worktree' '\n> > > +\tgit rev-parse HEAD >.git/bad &&\n> > > +\ttest_must_fail git rev-parse --verify main-worktree/bad\n> > > +'\n> > \n> > Are these really specific to the REFFILES backend? I would expect that\n> > the reftable backend sohuld fail to parse those, too. The fact that we\n> > write into the repository directly during the test setup doesn't change\n> > this, because all this patch is about is that we don't want to parse\n> > random files in the Git repo. And that is something we should want to\n> > enforce for all backends.\n> \n> So this is where I will show my ignorance of reftables. I assume it\n> still has to implement FETCH_HEAD as a file (since it holds extra data).\n> But does it do the same for other names outside of \"refs/\"? I am\n> assuming not in the paragraph below.\n\nNo, that's why we originally introduced the \"special refs\" syntax, as\ndefined in gitglossary(7). There are only two files that behave like\nrefs, but circumvent the ref backend: FETCH_HEAD and MERGE_HEAD. Both of\nthese have special syntax and carry additional metadata, and as such\nthey cannot be stored generically in a ref backend.\n\nAll other root refs are stored via the ref backend.\n\n> I would expect the test to succeed after my patches on any ref backend,\n> since we'd enforce the syntax outside of the backend-specific code. But\n> for a backend which does not look for the root name \"foo\" directly in\n> .git/, it is not an interesting test. The looked-for name does not\n> exist for it, so even if we did try the lookup, it would fail. We cannot\n> distinguish the two cases from the outcome we see.\n> \n> So I think dropping REFFILES it would still pass, but we are not really\n> testing anything that interesting for reftables. That said, I would be\n> OK dropping the REFFILES in the name of simplicity and just documenting\n> it in the commit message.\n\nYeah, I'd prefer to drop it. We should only specify the REFFILES prereq\nas sparingly as possible to ensure that behaviour is as consistent as\npossible across the implementations.\n\nPatrick\n"},{"id":"494033","messageId":"20240503175553.GG3631237@coredump.intra.peff.net","threadId":"61386","inReplyTo":"ZjDVLAKA0_4pTAS7@tanuki","subject":"Re: [PATCH 8/8] refs: check refnames as fully qualified when resolving","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-03T17:55:53Z","receivedAt":"2024-05-03T17:55:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 30, 2024 at 01:25:32PM +0200, Patrick Steinhardt wrote:\n\n> > So this is where I will show my ignorance of reftables. I assume it\n> > still has to implement FETCH_HEAD as a file (since it holds extra data).\n> > But does it do the same for other names outside of \"refs/\"? I am\n> > assuming not in the paragraph below.\n> \n> No, that's why we originally introduced the \"special refs\" syntax, as\n> defined in gitglossary(7). There are only two files that behave like\n> refs, but circumvent the ref backend: FETCH_HEAD and MERGE_HEAD. Both of\n> these have special syntax and carry additional metadata, and as such\n> they cannot be stored generically in a ref backend.\n> \n> All other root refs are stored via the ref backend.\n\nOK, that matches what I guessed based on the existence of special refs. ;)\nThanks for confirming.\n\nPart of me does wonder if things would be simpler if ref backends only\nhandled refs/*, and pseudo/special/root refs remained as their own thing\nin the filesystem. They're a limited set, so we don't really care about\nscaling in the same way. And their point is to be somewhat ephemeral, so\neven if you wanted to be clever with a replicated database-backed refs\nstore, you probably don't care if CHERRY_PICK_HEAD goes away.\n\nAnd it's not clear to me what the path forward is for scripts which poke\nat .git/* to determine repo state. For example, I think git-prompt.sh\nlooks at CHERRY_PICK_HEAD and REVERT_HEAD to decide what we're doing.\nMaybe we just roll all of that into a command which returns all details\nof the repo state?\n\n> > So I think dropping REFFILES it would still pass, but we are not really\n> > testing anything that interesting for reftables. That said, I would be\n> > OK dropping the REFFILES in the name of simplicity and just documenting\n> > it in the commit message.\n> \n> Yeah, I'd prefer to drop it. We should only specify the REFFILES prereq\n> as sparingly as possible to ensure that behaviour is as consistent as\n> possible across the implementations.\n\nMakes sense. I'll change that for my next re-roll (which probably won't\nbe until next week, as I'll be offline for a bit).\n\n-Peff\n"},{"id":"494113","messageId":"Zjh3GIO52uh4WliK@tanuki","threadId":"61386","inReplyTo":"20240503175553.GG3631237@coredump.intra.peff.net","subject":"Re: [PATCH 8/8] refs: check refnames as fully qualified when resolving","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-06T06:22:16Z","receivedAt":"2024-05-06T06:22:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, May 03, 2024 at 01:55:53PM -0400, Jeff King wrote:\n> On Tue, Apr 30, 2024 at 01:25:32PM +0200, Patrick Steinhardt wrote:\n> \n> > > So this is where I will show my ignorance of reftables. I assume it\n> > > still has to implement FETCH_HEAD as a file (since it holds extra data).\n> > > But does it do the same for other names outside of \"refs/\"? I am\n> > > assuming not in the paragraph below.\n> > \n> > No, that's why we originally introduced the \"special refs\" syntax, as\n> > defined in gitglossary(7). There are only two files that behave like\n> > refs, but circumvent the ref backend: FETCH_HEAD and MERGE_HEAD. Both of\n> > these have special syntax and carry additional metadata, and as such\n> > they cannot be stored generically in a ref backend.\n> > \n> > All other root refs are stored via the ref backend.\n> \n> OK, that matches what I guessed based on the existence of special refs. ;)\n> Thanks for confirming.\n> \n> Part of me does wonder if things would be simpler if ref backends only\n> handled refs/*, and pseudo/special/root refs remained as their own thing\n> in the filesystem. They're a limited set, so we don't really care about\n> scaling in the same way. And their point is to be somewhat ephemeral, so\n> even if you wanted to be clever with a replicated database-backed refs\n> store, you probably don't care if CHERRY_PICK_HEAD goes away.\n\nI think this would have several downsides:\n\n  - You cannot perform atomic updates and reads of the whole\n    repository's ref state. Overall, the whole ref namespace is fully\n    contained by the ref database.\n\n  - Not having those loose refs can improve security because you do not\n    have to parse arbitrary paths in the Git repository, and those will\n    not contain arbitrary information or even be symbolic links in case\n    `core.preferSymlinkRefs` is set.\n\n  - Every file that is not a ref needs special treatment for garbage\n    collection.\n\n  - There is a weird mismatch where some refs can be surfaced via\n    tooling whereas others can't really. You either cannot use normal\n    plumbing commands to access those refs, or you must create hacks in\n    the ref layer. Any of those hacks is only going to be a partial\n    solution, and the cases in which reading those files as refs doesn't\n    work stick out like a sore thumb.\n\n  - Conceptually, on the UX side, it's totally weird that some refs are\n    more special than others. This is quite hard to explain to our\n    users. I see it as a benefit that we're now finally cleaning up this\n    mess and make things a lot more straight-forward.\n\nNow I don't fully disagree with what you're saying: I wish that a lot of\nthe state was more self-contained to the particular subsystem. The\ngit-bisect(1) state is a prime example, where we clutter the gitdir with\nvarious different files. But the end goal in my opinion should be that\nsomething is either a proper ref, in which case it is stored in the ref\nbackend, or it is not and cannot ever be accessed as one. The current\nin-between state is just plain weird.\n\n> And it's not clear to me what the path forward is for scripts which poke\n> at .git/* to determine repo state. For example, I think git-prompt.sh\n> looks at CHERRY_PICK_HEAD and REVERT_HEAD to decide what we're doing.\n\nThey shouldn't, in my opinion. It's one of the consequences of accepting\nmultiple ref backends into Git: tooling must not assume the on-disk file\nformat, and they should use Git plumbing commands to access the data\ninstead. I have already updated git-prompt.sh to do so.\n\n> Maybe we just roll all of that into a command which returns all details\n> of the repo state?\n\nThat indeed is something I have been thinking about quite a lot recently\nand that I would certainly love to see. Making the state as discussed\nhere more visible would be nice.\n\nIt would also allow us to fix the weirdness that git-rev-parse(1) has\nbecome. Its scope has gone way beyond parsing revs due to all those\nweird modes where it exercises the repository's state. Those are needed,\nsure, and we didn't have a better place to put those in the past. But\nideally, things like `--local-env-vars` or `--resolve-git-dir` have no\nreason to exist in git-rev-parse(1) at all.\n\nSo if we had a new plumbing command that allows us to query a repository\nfor repository-level information it would just be natural to move those\nover.\n\nWe do have this in our backlog at GitLab, but didn't yet get to it.\n\nPatrick\n"}]}