patchfast-import: disallow "." and ".." path components

> If a user specified e.g.
>    M 100644 :1 ../some-file
> then fast-import previously would happily create a git history where
> there is a tree in the top-level directory named "..", and with a file
> inside that directory named "some-file".  The top-level ".." directory
> causes problems.  While git checkout will die with errors and fsck will
> report hasDotdot problems, the user is going to have problems trying to
> remove the problematic file.  Simply avoid creating this bad history in
> the first place.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> diff --git a/builtin/fast-import.c b/builtin/fast-import.c
> @@ -1466,6 +1466,9 @@ static int tree_content_set(
>         e->name = to_atom(p, n);
> +       if (!strcmp(e->name->str_dat, ".") || !strcmp(e->name->str_dat, "..")) {
> +               die("path %s contains invalid component", p);
> +       }

>
> On Mon, Nov 25, 2024 at 12:58 PM Elijah Newren via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > If a user specified e.g.
> >    M 100644 :1 ../some-file
> > then fast-import previously would happily create a git history where
> > there is a tree in the top-level directory named "..", and with a file
> > inside that directory named "some-file".  The top-level ".." directory
> > causes problems.  While git checkout will die with errors and fsck will
> > report hasDotdot problems, the user is going to have problems trying to
> > remove the problematic file.  Simply avoid creating this bad history in
> > the first place.
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> > diff --git a/builtin/fast-import.c b/builtin/fast-import.c
> > @@ -1466,6 +1466,9 @@ static int tree_content_set(
> >         e->name = to_atom(p, n);
> > +       if (!strcmp(e->name->str_dat, ".") || !strcmp(e->name->str_dat, "..")) {
> > +               die("path %s contains invalid component", p);
> > +       }
>
> Probably not worth a reroll, but is_dot_or_dotdot() might be usable here.
>
> (And -- style nit -- the braces could be dropped.)

> From: Elijah Newren <newren@gmail.com>
> 
> If a user specified e.g.
>    M 100644 :1 ../some-file
> then fast-import previously would happily create a git history where
> there is a tree in the top-level directory named "..", and with a file
> inside that directory named "some-file".  The top-level ".." directory
> causes problems.  While git checkout will die with errors and fsck will
> report hasDotdot problems, the user is going to have problems trying to
> remove the problematic file.  Simply avoid creating this bad history in
> the first place.

> On Mon, Nov 25, 2024 at 07:00:48PM +0000, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> > 
> > If a user specified e.g.
> >    M 100644 :1 ../some-file
> > then fast-import previously would happily create a git history where
> > there is a tree in the top-level directory named "..", and with a file
> > inside that directory named "some-file".  The top-level ".." directory
> > causes problems.  While git checkout will die with errors and fsck will
> > report hasDotdot problems, the user is going to have problems trying to
> > remove the problematic file.  Simply avoid creating this bad history in
> > the first place.
> 
> Makes sense.
> 
> More generally this made me wonder whether we should maybe extract some
> bits out of "fsck.c" so that we don't have to duplicate the checks done
> there in git-fast-import(1). This would for example include checks for
> ".git" and its HFS/NTFS variants as well as tree entry length checks for
> names longer than 4096 characters.

> From: Elijah Newren <newren@gmail.com>
> [...]
> diff --git a/builtin/fast-import.c b/builtin/fast-import.c
> index 76d5c20f141..995ef76f9d6 100644
> --- a/builtin/fast-import.c
> +++ b/builtin/fast-import.c
> @@ -1466,6 +1466,8 @@ static int tree_content_set(
>  		root->tree = t = grow_tree_content(t, t->entry_count);
>  	e = new_tree_entry();
>  	e->name = to_atom(p, n);
> +	if (is_dot_or_dotdot(e->name->str_dat))
> +		die("path %s contains invalid component", p);

>  	e->versions[0].mode = 0;
>  	oidclr(&e->versions[0].oid, the_repository->hash_algo);
>  	t->entries[t->entry_count++] = e;
> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
> index 6224f54d4d2..caf3dc003a0 100755
> --- a/t/t9300-fast-import.sh
> +++ b/t/t9300-fast-import.sh
> @@ -522,6 +522,26 @@ test_expect_success 'B: fail on invalid committer (5)' '
>  	test_must_fail git fast-import <input
>  '
>
> +test_expect_success 'B: fail on invalid file path' '
> +	cat >input <<-INPUT_END &&
> +	blob
> +	mark :1
> +	data <<EOF
> +	File contents
> +	EOF
> +
> +	commit refs/heads/badpath
> +	committer Name <email> $GIT_COMMITTER_DATE
> +	data <<COMMIT
> +	Commit Message
> +	COMMIT
> +	M 100644 :1 ../invalid-path

>> +	if (is_dot_or_dotdot(e->name->str_dat))
>> +		die("path %s contains invalid component", p);
>
> Nit: single-quoting the path seems more common:
>
>     $ git grep "\"path '%s'" ':!po/' | wc -l
>     17
>     $ git grep "\"path %s" ':!po/' | wc -l
>     4

> [PATCH] CodingGuidelines: a handful of error message guidelines
>
> It is more efficient to have something in the coding guidelines
> document to point at, when we want to review and comment on a new
> message in the codebase to make sure it "fits" in the set of
> existing messages.
>
> Let's write down established best practice we are aware of.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines
> @@ -703,16 +703,22 @@ Program Output
>  Error Messages
>
> - - Do not end error messages with a full stop.
> + - Do not end a single-sentence error message with a full stop.
>
>   - Do not capitalize the first word, only because it is the first word
> -   in the message ("unable to open %s", not "Unable to open %s").  But
> +   in the message ("unable to open '%s'", not "Unable to open '%s'").  But
>     "SHA-3 not supported" is fine, because the reason the first word is
>     capitalized is not because it is at the beginning of the sentence,
>     but because the word would be spelled in capital letters even when
>     it appeared in the middle of the sentence.
>
> - - Say what the error is first ("cannot open %s", not "%s: cannot open")
> + - Say what the error is first ("cannot open '%s'", not "%s: cannot open").
> +
> + - Enclose the subject of an error inside a pair of single quotes,
> +   e.g. `die(_("unable to open '%s'"), path)`.

> It is more efficient to have something in the coding guidelines
> document to point at, when we want to review and comment on a new
> message in the codebase to make sure it "fits" in the set of
> existing messages.
>
> Let's write down established best practice we are aware of.
>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> @@ -689,16 +689,29 @@ Program Output
>  Error Messages
>
> - - Say what the error is first ("cannot open %s", not "%s: cannot open")
> + - Say what the error is first ("cannot open '%s'", not "%s: cannot open").
> +
> + - Enclose the subject of an error inside a pair of single quotes,
> +   e.g. `die(_("unable to open '%s'"), path)`.
> +
> + - Unless there is a compelling reason not to, error messages from the
> +   Porcelain command should be marked for `_("translation")`.

>> + - Error messages from the plumbing commands are sometimes meant for
>> +   machine consumption and should not be marked for `_("translation")`
>> +   to keep them 'grep'-able.
>
> Using the same example, `_("translation")`, for both the "should be"
> and "should not be" cases may very well confuse readers. (It certainly
> confused me.) Perhaps mirroring the example of an item earlier in the
> list would be clearer:
>
>     - Unless there is a compelling reason not to, error messages from
>       porcelain commands should be marked for translation, e.g.
>       `die(_("bad revision"))`
>
>     - Error messages from plumbing commands are sometimes meant for
>       machine consumption, thus should not be marked for translation,
>       e.g. `die("bad revision")`

>> + - BUG("message") are for communicating the specific error to
>> +   developers, and not to be translated.
>
> Okay, although could be slightly more explicit:
>
>     - BUG("message") is for communicating a specific failure to
>       developers, not end-users, thus should not be translated.

> Eric Sunshine <sunshine@sunshineco.com> writes:
> >> +   Porcelain command should be marked for `_("translation")`.
> >
> > Here you capitalize "Porcelain" but below, "plumbing" is all lowercase.
>
> ;-) I think that is how we spell them in our documentation when we
> contrast them against each other.

> >> + - BUG("message") are for communicating the specific error to
> >> +   developers, and not to be translated.
> >
> > Okay, although could be slightly more explicit:
> >
> >     - BUG("message") is for communicating a specific failure to
> >       developers, not end-users, thus should not be translated.
>
> The way I read your rewrite is that the "communitation" mentioned is
> between the program and the user who saw the message.  I wanted to
> say that the message is seen first by an end-user, and then is
> communicated to developers.  And not translating is one way to make
> sure the message is not mangled, and stays grep-able, during the
> game of telephone.
>
> Would this work better?
>
>   - In order to help the user who saw BUG("message") to accurately
>     communicate it to developers, do not mark them for translation.