Repository navigation
Commit fbbecc6
committed
fix(commit): reject identity fields that alter commit headers
`Commit._serialize` wrote `Actor.name` and `Actor.email` into the `author`
and `committer` headers exactly as given. Each identity is a single line of
the form `name <email> date`, so a line feed or an angle bracket inside
either field moves those boundaries.
A line feed in the author name ends the `author` line and the remainder is
read as further headers. `author` is written before `committer`, so a
`committer` line supplied this way is the one Git and
`Commit._deserialize` use, and an empty line ends the headers so the rest
becomes the message. With the committer pinned by a service and only the
display name taken from its user, the name
Eve <eve@user.example> 1700000000 +0000
committer Release Manager <release@corp.example> 1700000000 +0000
Approved-by: Release Manager
produced a commit that `git log` attributes to committer
`Release Manager <release@corp.example>` with the subject
`Approved-by: Release Manager`, and `git fsck` reported nothing for it.
Without a line feed, `<` in a name or `>` in an email still presents
another email: the name `Release Manager <release@corp.example> x` with the
email `eve@user.example` reads back as `release@corp.example` in both Git
and GitPython.
Git removes these three characters when it formats an identity
(`strbuf_addstr_without_crud()` in `ident.c`), so `git commit-tree` given
the same `GIT_AUTHOR_NAME` cannot write such a commit. Check the name and
email of both identities at the start of `_serialize` and raise
`ValueError` if one contains `<`, `>` or a line feed. `create_from_tree`,
`IndexFile.commit` and `replace` all store the commit through this method,
and the check runs before the object is stored and before `HEAD` or its
reflog are updated. It raises instead of removing the characters, like the
tree and index serializers that reject invalid entries, so a caller is not
left with an identity it did not ask for. The `back-to-the-roots` branch
(#2262) already refuses these characters before calling `git commit-tree`;
this covers the native serializer until then.
Identities without these characters are written as before. `_deserialize`
can return such a field only from a commit that `git fsck` already reports
as `badName` or `badEmail`, for example `A> B <a@b>` or `A <<a@b>>`.
Serializing a commit like that again, for example through `replace()`, now
raises too. None of the 5466 commits in this repository is affected and
`test_serialization` still reproduces every object ID it checks. NUL and
carriage return are left alone: neither moves a field boundary, and Git
keeps a carriage return inside a name.
`test_identity_cannot_alter_headers` covers both fields of both identities
through `create_from_tree` and `replace`. It fails on the previous code and
passes here. The full suite has no new failures on macOS with Python 3.11
(the six `nul\x00name` cases of
`test_submodule_rejects_unsafe_checkout_before_mutation` fail there with
and without this change), and `ruff`, `mypy`, `basedpyright --warnings` and
the documentation build are clean.1 parent 4f0f54d commit fbbecc6
2 files changed
Lines changed: 57 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -662,6 +662,10 @@ def create_from_tree( | |||
| 662 | 662 | :return: | |
| 663 | 663 | :class:`Commit` object representing the new commit. | |
| 664 | 664 | ||
| 665 | + :raise ValueError: | ||
| 666 | + If the name or email of the author or committer contains ``<``, ``>`` or a | ||
| 667 | + line feed, as these would change the identity headers of the commit. | ||
| 668 | + | ||
| 665 | 669 | :note: | |
| 666 | 670 | Additional information about the committer and author are taken from the | |
| 667 | 671 | environment or from the git configuration. See :manpage:`git-commit-tree(1)` | |
@@ -786,6 +790,16 @@ def create_from_tree( | |||
| 786 | 790 | # { Serializable Implementation | |
| 787 | 791 | ||
| 788 | 792 | def _serialize(self, stream: BytesIO) -> "Commit": | |
| 793 | + # An identity is written as "name <email> date" on a single header line, so a | ||
| 794 | + # line feed or an angle bracket inside a name or email moves those boundaries: | ||
| 795 | + # it can add header lines, end the headers early, or present another email. | ||
| 796 | + # Git drops these three characters when it writes an identity; refuse them | ||
| 797 | + # here before anything is written. | ||
| 798 | + for actor in (self.author, self.committer): | ||
| 799 | + for value in (actor.name, actor.email): | ||
| 800 | + if value and any(char in value for char in "<>\n"): | ||
| 801 | + raise ValueError("Commit identity %r must not contain '<', '>' or a line feed" % value) | ||
| 802 | + | ||
| 789 | 803 | write = stream.write | |
| 790 | 804 | write(("tree %s\n" % self.tree).encode("ascii")) | |
| 791 | 805 | for p in self.parents: | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -424,6 +424,49 @@ def test_invalid_commit(self): | |||
| 424 | 424 | self.assertEqual(cmt.author.name, "E.Azer Ko�o�o�oculu", cmt.author.name) | |
| 425 | 425 | self.assertEqual(cmt.author.email, "azer@kodfabrik.com", cmt.author.email) | |
| 426 | 426 | ||
| 427 | + @with_rw_directory | ||
| 428 | + def test_identity_cannot_alter_headers(self, rw_dir): | ||
| 429 | + """A name or email must not add header lines or present another identity.""" | ||
| 430 | + rw_repo = Repo.init(osp.join(rw_dir, "test_identity_headers")) | ||
| 431 | + path = osp.join(str(rw_repo.working_tree_dir), "hello.txt") | ||
| 432 | + touch(path) | ||
| 433 | + rw_repo.index.add([path]) | ||
| 434 | + tree = rw_repo.index.write_tree() | ||
| 435 | + service = Actor("Service", "service@example.com") | ||
| 436 | + forged = "committer Forged <forged@example.com> 0 +0000" | ||
| 437 | + | ||
| 438 | + for name, email in ( | ||
| 439 | + # A line feed ends the header line, so the remainder would become headers | ||
| 440 | + # of its own, which Git reads before the committer written after them. | ||
| 441 | + ("User <user@example.com> 0 +0000\n" + forged, "user@example.com"), | ||
| 442 | + ("User", "user@example.com> 0 +0000\n" + forged), | ||
| 443 | + # Angle brackets delimit the email, so these would present another one. | ||
| 444 | + ("Forged <forged@example.com>", "user@example.com"), | ||
| 445 | + ("User", "forged@example.com> <user@example.com"), | ||
| 446 | + ("User>", "user@example.com"), | ||
| 447 | + ("User", "<user@example.com"), | ||
| 448 | + ): | ||
| 449 | + with self.subTest(name=name, email=email): | ||
| 450 | + identity = Actor(name, email) | ||
| 451 | + with self.assertRaises(ValueError): | ||
| 452 | + Commit.create_from_tree(rw_repo, tree, "message", head=True, author=identity, committer=service) | ||
| 453 | + with self.assertRaises(ValueError): | ||
| 454 | + Commit.create_from_tree(rw_repo, tree, "message", head=True, author=service, committer=identity) | ||
| 455 | + | ||
| 456 | + # Nothing was committed along the way. | ||
| 457 | + assert not rw_repo.head.is_valid() | ||
| 458 | + | ||
| 459 | + # Other punctuation is still written as given. | ||
| 460 | + author = Actor("Dr. J. O'Neil-Smith, Jr.", "user+tag@example.com") | ||
| 461 | + commit = Commit.create_from_tree(rw_repo, tree, "message", head=True, author=author, committer=service) | ||
| 462 | + stored = Commit(rw_repo, commit.binsha) | ||
| 463 | + self.assertEqual(stored.author, author) | ||
| 464 | + self.assertEqual(stored.committer, service) | ||
| 465 | + self.assertEqual(stored.message, "message") | ||
| 466 | + | ||
| 467 | + with self.assertRaises(ValueError): | ||
| 468 | + commit.replace(author=Actor("User\n" + forged, "user@example.com")) | ||
| 469 | + | ||
| 427 | 470 | def test_gpgsig(self): | |
| 428 | 471 | cmt = self.rorepo.commit() | |
| 429 | 472 | with open(fixture_path("commit_with_gpgsig"), "rb") as fd: | |
0 commit comments