Re: [PATCH v5 6/9] ref: add escape check for the referent of symref
- From
shejialuo <shejialuo@gmail.com>
- Date
- Oct 7, 2024, 08:44 UTC
- Message-ID
- <ZwOffN9UWX1gP0gy@ArchLinux>
- In-Reply-To
- <ZwOGr4Tv8K_wemtD@pks.im>
On Mon, Oct 07, 2024 at 08:58:55AM +0200, Patrick Steinhardt wrote:
Show 18 quoted lines
> On Sun, Sep 29, 2024 at 03:16:21PM +0800, shejialuo wrote: > > Ideally, we want to the users use "git symbolic-ref" to create symrefs > > instead of writing raw contents into the filesystem. However, "git > > symbolic-ref" is strict with the refname but not strict with the > > referent. For example, we can make the "referent" located at the > > "$(gitdir)/logs/aaa" and manually write the content into this where we > > can still successfully parse this symref by using "git rev-parse". > > > > $ git init repo && cd repo && git commit --allow-empty -mx > > $ git symbolic-ref refs/heads/test logs/aaa > > $ echo $(git rev-parse HEAD) > .git/logs/aaa > > $ git rev-parse test > > Oh, curious. This should definitely be tightened in git-symbolic-ref(1) > itself. The target should either be a root ref or something starting > with "refs/". Anyway, that is of course outside of the scope of this > patch series. >
I am curious here too when I did experiments when writing the code. Because Junio have told me this could happen, so I dive into this. However, it's not reasonable. If we want to tighten the rule, we need to also let "git symbolic-ref" to align with the behavior. That's another question though.
[snip]
Show 14 quoted lines
> > diff --git a/Documentation/fsck-msgids.txt b/Documentation/fsck-msgids.txt > > index e0e4519334..223974057d 100644 > > --- a/Documentation/fsck-msgids.txt > > +++ b/Documentation/fsck-msgids.txt > > @@ -52,6 +52,14 @@ > > `emptyName`:: > > (WARN) A path contains an empty name. > > > > +`escapeReferent`:: > > + (INFO) The referent of a symref is outside the "ref" directory. > > Proposal: 'The referent of a symbolic reference points neither to a root > reference nor to a reference starting with "refs/".' >
That's much better.
Show 6 quoted lines
> I'd also rename this to e.g. "symrefTargetIsNotAReference" or something > like that, because it's not really about whether or not the referent is > "escaping". It's a bit of a mouthful, but I don't really have a better > name. So feel free to pick something different that describes the error > better. >
I guess "symrefTargetIsNotAReference" is a little too long. If we decide to convert it to error later. Why not just put it into the "badReferent" fsck message?
So, I do not think we need to rename. As I have talked about, we don't need to map error case to fsck message id one by one.
Show 22 quoted lines
> > diff --git a/refs/files-backend.c b/refs/files-backend.c
> > index 57ac466b64..bd215c8d08 100644
> > --- a/refs/files-backend.c
> > +++ b/refs/files-backend.c
> > @@ -3520,6 +3520,13 @@ static int files_fsck_symref_target(struct fsck_options *o,
> > orig_last_byte = referent->buf[orig_len - 1];
> > strbuf_rtrim(referent);
> >
> > + if (!starts_with(referent->buf, "refs/")) {
> > + ret = fsck_report_ref(o, report,
> > + FSCK_MSG_ESCAPE_REFERENT,
> > + "referent '%s' is outside of refs/",
> > + referent->buf);
> > + }
> > +
> > if (check_refname_format(referent->buf, 0)) {
> > ret = fsck_report_ref(o, report,
> > FSCK_MSG_BAD_REFERENT,
>
> This check is invalid, because referents can also point to root refs. So
> you should probably also add a call to `is_root_ref()` here.
> Thanks, I omit this situation here.
> We also have `is_pseudo_ref()`, and one might be tempted to also allow > that. But pseudo refs aren't proper refs, so I'd argue that a symref > pointing to a pseudo ref is invalid, too. >
I agree.
> Patrick
Thanks, Jialuo