fast-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 <[email protected]>

---

Quoted 14 lines · Elijah Newren via GitGitGadget

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 <[email protected]>

---

builtin/fast-import.c static int tree_content_set( · line 1469

Quoted 4 lines

builtin/fast-import.c +3 -0

@@ -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 <[email protected]>

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.

Quoted 11 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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.

I had the same thought, but I think the right code to be using is verify_path(). That's what ultimately is used to let names into the index from trees, from update-index, or from other tools like git-apply.

builtin/fast-import.c +0 -0

 	e->versions[0].mode = 0;
 	oidclr(&e->versions[0].oid, the_repository->hash_algo);
 	t->entries[t->entry_count++] = e;

t/t9300-fast-import.sh +4 -0

index 6224f54d4d2..caf3dc003a0 100755
@@ -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

Review

Quoted 2 lines

(quoted diff) +2 -0

+	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 <[email protected]>

---

w/Documentation/CodingGuidelines renamed from c/Documentation/CodingGuidelines +2 -2

@@ -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

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 <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---

Quoted 2 lines

+ - Unless there is a compelling reason not to, error messages from the
+ Porcelain command should be marked for _("translation").

Here you capitalize "Porcelain" but below, "plumbing" is all lowercase.

Documentation/CodingGuidelines Program Output · line 711

Quoted 3 lines

+ - Error messages from the plumbing commands are sometimes meant for

Documentation/CodingGuidelines +2 -0

+   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")

Quoted 2 lines

+ - 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.

Quoted 3 lines · Eric Sunshine

Quoted 1 line

+ 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.

Quoted 7 lines

Quoted 2 lines

+ - 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.