This is an automated email from the ASF dual-hosted git repository.
garydgregory pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-codec.git
The following commit(s) were added to refs/heads/master by this push:
new 87f6aa28 Reject ambiguous GitIdentifiers tree entry names
87f6aa28 is described below
commit 87f6aa2818836998528ff4b0d1ea35545a800ee5
Author: Gary Gregory <[email protected]>
AuthorDate: Fri Sep 18 06:09:35 2026 -0700
Reject ambiguous GitIdentifiers tree entry names
Reject empty, dot, NUL-containing, and malformed Unicode entry names.
Reject file/directory name conflicts when computing tree identifiers.
Add regression tests for serialization collisions and valid names,
and document the fix in the release notes.
---
src/changes/changes.xml | 1 +
.../commons/codec/digest/GitIdentifiers.java | 50 +++++---
.../commons/codec/digest/GitIdentifiersTest.java | 141 ++++++++++++++-------
3 files changed, 129 insertions(+), 63 deletions(-)
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index 8e3a2410..e59c6bcf 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -59,6 +59,7 @@ The <action> type attribute can be add,update,fix,remove.
<action type="fix" dev="ggregory" due-to="Gary Gregory">Grow Base58
accumulation buffers geometrically within the configured input limits and
validate actual accumulated length.</action>
<action type="fix" dev="ggregory" due-to="Gary Gregory">Implement Base58
encoded-length calculation and explicitly reject unsupported line
chunking.</action>
<action type="fix" dev="ggregory" due-to="Gary Gregory">Close the
underlying BaseNCodecOutputStream output even when final conversion or flushing
fails, preserving suppressed close exceptions.</action>
+ <action type="fix" dev="ggregory" due-to="Gary Gregory">Reject invalid
GitIdentifiers tree entry names and file/directory name conflicts to prevent
ambiguous tree serialization and colliding identifiers.</action>
<!-- ADD -->
<action type="add" dev="ggregory" due-to="Gary Gregory">Add and use
PhoneticEngine.Builder and deprecate old constructors.</action>
<action type="add" dev="ggregory" due-to="Gary Gregory">Add
BeiderMorseEncoder.Builder and deprecate old constructor.</action>
diff --git a/src/main/java/org/apache/commons/codec/digest/GitIdentifiers.java
b/src/main/java/org/apache/commons/codec/digest/GitIdentifiers.java
index f9e42311..db5087dd 100644
--- a/src/main/java/org/apache/commons/codec/digest/GitIdentifiers.java
+++ b/src/main/java/org/apache/commons/codec/digest/GitIdentifiers.java
@@ -70,6 +70,20 @@ public class GitIdentifiers {
*/
static class DirectoryEntry implements Comparable<DirectoryEntry> {
+ private static String requireValidName(final String name) {
+ Objects.requireNonNull(name, "name");
+ if (name.isEmpty() || ".".equals(name) || "..".equals(name)) {
+ throw new IllegalArgumentException("Entry name must not be
empty, '.' or '..'");
+ }
+ if (name.indexOf('/') >= 0 || name.indexOf('\0') >= 0) {
+ throw new IllegalArgumentException("Entry name must not
contain '/' or NUL");
+ }
+ if (!StandardCharsets.UTF_8.newEncoder().canEncode(name)) {
+ throw new IllegalArgumentException("Entry name must not
contain unpaired surrogates");
+ }
+ return name;
+ }
+
/**
* The entry name (file or directory name, no path separator).
*/
@@ -95,15 +109,12 @@ public class GitIdentifiers {
/**
* Constructs a new entry.
*
- * @param name The name of the entry, not containing {@code '/'}.
+ * @param name The nonempty entry name, not {@code .} or {@code ..},
without {@code '/'}, NUL or unpaired surrogates.
* @param type The type of the entry, not null.
* @param rawObjectId The id of the entry, not null.
*/
DirectoryEntry(final String name, final FileMode type, final byte[]
rawObjectId) {
- if (Objects.requireNonNull(name, "name").indexOf('/') >= 0) {
- throw new IllegalArgumentException("Entry name must not
contain '/': " + name);
- }
- this.name = name;
+ this.name = requireValidName(name);
this.type = Objects.requireNonNull(type, "type");
this.sortKey = (type == FileMode.DIRECTORY ? name + "/" :
name).getBytes(StandardCharsets.UTF_8);
this.rawObjectId = Objects.requireNonNull(rawObjectId,
"rawObjectId");
@@ -225,13 +236,6 @@ public class GitIdentifiers {
byte[] get() throws IOException;
}
- private static String requireNoParentTraversal(final String name) {
- if ("..".equals(name)) {
- throw new IllegalArgumentException("Path component not
allowed: " + name);
- }
- return name;
- }
-
private final Map<String, TreeIdBuilder> dirEntries = new HashMap<>();
private final Map<String, DirectoryEntry> fileEntries = new
HashMap<>();
private final MessageDigest messageDigest;
@@ -245,7 +249,7 @@ public class GitIdentifiers {
*
* @param name The relative path of the subdirectory in normalized
form (may contain {@code '/'}).
* @return The {@link TreeIdBuilder} for the subdirectory.
- * @throws IllegalArgumentException If any path component is {@code
".."}.
+ * @throws IllegalArgumentException If any path component is {@code
".."}, contains NUL or contains unpaired surrogates.
*/
public TreeIdBuilder addDirectory(final String name) {
TreeIdBuilder current = this;
@@ -254,7 +258,7 @@ public class GitIdentifiers {
if (component.isEmpty() || ".".equals(component)) {
continue;
}
- current =
current.dirEntries.computeIfAbsent(requireNoParentTraversal(component), k ->
new TreeIdBuilder(messageDigest));
+ current =
current.dirEntries.computeIfAbsent(DirectoryEntry.requireValidName(component),
k -> new TreeIdBuilder(messageDigest));
}
return current;
}
@@ -262,7 +266,8 @@ public class GitIdentifiers {
private void addFile(final FileMode mode, final String name, final
BlobIdSupplier blobId) throws IOException {
final int slash = name.lastIndexOf('/');
if (slash < 0) {
- fileEntries.put(name, new
DirectoryEntry(requireNoParentTraversal(name), mode, blobId.get()));
+ DirectoryEntry.requireValidName(name);
+ fileEntries.put(name, new DirectoryEntry(name, mode,
blobId.get()));
} else {
addDirectory(name.substring(0, slash)).addFile(mode,
name.substring(slash + 1), blobId);
}
@@ -277,7 +282,8 @@ public class GitIdentifiers {
* @param name The relative path of the entry in normalized form(may
contain {@code '/'}).
* @param data The file content.
* @throws IOException If an I/O error occurs.
- * @throws IllegalArgumentException If any path component is {@code
".."}.
+ * @throws IllegalArgumentException If the entry name is empty or
{@code "."}, or any path component is {@code ".."}, contains NUL or contains
unpaired
+ * surrogates.
*/
public void addFile(final FileMode mode, final String name, final
byte[] data) throws IOException {
addFile(mode, name, () -> blobId(messageDigest, data));
@@ -295,7 +301,8 @@ public class GitIdentifiers {
* @param dataSize The exact number of bytes in {@code data}.
* @param data The file content.
* @throws IOException If the stream cannot be read.
- * @throws IllegalArgumentException If any path component is {@code
".."}.
+ * @throws IllegalArgumentException If the entry name is empty or
{@code "."}, or any path component is {@code ".."}, contains NUL or contains
unpaired
+ * surrogates.
*/
public void addFile(final FileMode mode, final String name, final long
dataSize, final InputStream data) throws IOException {
addFile(mode, name, () -> blobId(messageDigest, dataSize, data));
@@ -309,7 +316,8 @@ public class GitIdentifiers {
* @param name The relative path of the entry in normalized form(may
contain {@code '/'}).
* @param target The target of the symbolic link.
* @throws IOException If an I/O error occurs.
- * @throws IllegalArgumentException If any path component is {@code
".."}.
+ * @throws IllegalArgumentException If the entry name is empty or
{@code "."}, or any path component is {@code ".."}, contains NUL or contains
unpaired
+ * surrogates.
*/
public void addSymbolicLink(final String name, final String target)
throws IOException {
addFile(FileMode.SYMBOLIC_LINK, name,
target.getBytes(StandardCharsets.UTF_8));
@@ -319,9 +327,15 @@ public class GitIdentifiers {
* Computes the Git tree identifier for this directory and all its
descendants.
*
* @return The raw tree identifier bytes.
+ * @throws IllegalStateException If a file and a directory have the
same name in this directory or any descendant.
*/
@Override
public byte[] get() {
+ for (final String name : dirEntries.keySet()) {
+ if (fileEntries.containsKey(name)) {
+ throw new IllegalStateException("File and directory have
the same name: " + name);
+ }
+ }
final Set<DirectoryEntry> entries = new
TreeSet<>(fileEntries.values());
dirEntries.forEach((k, v) -> entries.add(new DirectoryEntry(k,
FileMode.DIRECTORY, v.get())));
final ByteArrayOutputStream baos = new ByteArrayOutputStream();
diff --git
a/src/test/java/org/apache/commons/codec/digest/GitIdentifiersTest.java
b/src/test/java/org/apache/commons/codec/digest/GitIdentifiersTest.java
index bae2a761..0fbfeab1 100644
--- a/src/test/java/org/apache/commons/codec/digest/GitIdentifiersTest.java
+++ b/src/test/java/org/apache/commons/codec/digest/GitIdentifiersTest.java
@@ -222,51 +222,6 @@ class GitIdentifiersTest {
assertFalse(regular.equals("foo"));
}
- /**
- * Tree entry names are ordered by their UTF-8 bytes, which is not the
order {@link String#compareTo(String)} gives when a supplementary character
meets a
- * Basic Multilingual Plane character from U+E000 up: U+FF21 encodes to
{@code EF BC A1} and U+1F600 to {@code F0 9F 98 80}, so Git sorts U+FF21 first,
while
- * the UTF-16 code units place the surrogate pair of U+1F600 first.
- *
- * <p>The expected identifier is the one {@code git write-tree} produces
for a tree holding the same two entries.</p>
- */
- @Test
- void testTreeIdSortsSupplementaryPlaneNamesLikeGit(@TempDir final Path
tempDir) throws Exception {
- final String fullWidthA = "\uFF21";
- final String grinningFace = "\uD83D\uDE00";
- final byte[] content = "x".getBytes(StandardCharsets.UTF_8);
- final String expected = "9f9c1fc3580195f51d3e71b384ef1d57740e2151";
- final MessageDigest md = DigestUtils.getSha1Digest();
-
- // Entries are added in the wrong order on purpose, so only the sort
decides the result.
- final GitIdentifiers.TreeIdBuilder builder =
GitIdentifiers.treeIdBuilder(md);
- builder.addFile(GitIdentifiers.FileMode.REGULAR, grinningFace,
content);
- builder.addFile(GitIdentifiers.FileMode.REGULAR, fullWidthA, content);
- assertEquals(expected, Hex.encodeHexString(builder.get()));
-
- try {
- Files.write(tempDir.resolve(fullWidthA), content);
- Files.write(tempDir.resolve(grinningFace), content);
- } catch (final IOException e) {
- Assumptions.abort("Filesystem cannot hold the test entry names: "
+ e);
- }
- assertEquals(expected, Hex.encodeHexString(GitIdentifiers.treeId(md,
tempDir)));
- }
-
- /**
- * A lone surrogate encodes to {@code ?} in UTF-8, the same byte as a
question mark, so the two names share a sort key; both entries must stay in the
tree.
- */
- @Test
- void testTreeIdKeepsNamesWithTheSameUtf8Bytes() throws Exception {
- final byte[] content = "x".getBytes(StandardCharsets.UTF_8);
- final MessageDigest md = DigestUtils.getSha1Digest();
- final GitIdentifiers.TreeIdBuilder one =
GitIdentifiers.treeIdBuilder(md);
- one.addFile(GitIdentifiers.FileMode.REGULAR, "?", content);
- final GitIdentifiers.TreeIdBuilder both =
GitIdentifiers.treeIdBuilder(md);
- both.addFile(GitIdentifiers.FileMode.REGULAR, "?", content);
- both.addFile(GitIdentifiers.FileMode.REGULAR, "\uD800", content);
- assertNotEquals(Hex.encodeHexString(one.get()),
Hex.encodeHexString(both.get()));
- }
-
/**
* Entries should be sorted by Git sort rule.
*
@@ -284,6 +239,36 @@ class GitIdentifiersTest {
assertEquals(Arrays.asList(alpha, fooTxt, fooDir, foobar, zeta),
entries);
}
+ @ParameterizedTest
+ @ValueSource(strings = {"..", "bad\0dir", "\uD800", "\uDC00"})
+ void testRejectsInvalidDirectoryNames(final String name) {
+ final GitIdentifiers.TreeIdBuilder builder =
GitIdentifiers.treeIdBuilder(DigestUtils.getSha1Digest());
+ assertThrows(IllegalArgumentException.class, () ->
builder.addDirectory("parent/" + name));
+ assertThrows(IllegalArgumentException.class, () ->
builder.addFile(GitIdentifiers.FileMode.REGULAR, name + "/file",
HELLO_CONTENT));
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = {"", ".", "..", "a\0b", "\uD800", "\uDC00",
"a\uD800b", "\uD800\uD800", "\uDC00\uD800"})
+ void testRejectsInvalidEntryNames(final String name) {
+ assertThrows(IllegalArgumentException.class, () -> new
DirectoryEntry(name, GitIdentifiers.FileMode.REGULAR, ZERO_ID));
+ final GitIdentifiers.TreeIdBuilder builder =
GitIdentifiers.treeIdBuilder(DigestUtils.getSha1Digest());
+ assertThrows(IllegalArgumentException.class, () ->
builder.addFile(GitIdentifiers.FileMode.REGULAR, name, HELLO_CONTENT));
+ assertThrows(IllegalArgumentException.class,
+ () -> builder.addFile(GitIdentifiers.FileMode.REGULAR, name,
HELLO_CONTENT.length, new ByteArrayInputStream(HELLO_CONTENT)));
+ assertThrows(IllegalArgumentException.class, () ->
builder.addSymbolicLink(name, "target"));
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = {"?", "a\nb", "\uD83D\uDE00"})
+ void testTreeIdAcceptsValidEntryNames(final String name) throws Exception {
+ final GitIdentifiers.TreeIdBuilder builder =
GitIdentifiers.treeIdBuilder(DigestUtils.getSha1Digest());
+ builder.addFile(GitIdentifiers.FileMode.REGULAR, name, HELLO_CONTENT);
+ assertEquals(20, builder.get().length);
+ final GitIdentifiers.TreeIdBuilder directory =
GitIdentifiers.treeIdBuilder(DigestUtils.getSha1Digest());
+ directory.addDirectory(name).addFile(GitIdentifiers.FileMode.REGULAR,
"file", HELLO_CONTENT);
+ assertEquals(20, directory.get().length);
+ }
+
@ParameterizedTest
@MethodSource("virtualTreeProvider")
void testTreeIdBuilder(final String algorithm, final byte[] helloId, final
byte[] linkId, final byte[] linkTxtId, final byte[] runId,
@@ -420,4 +405,70 @@ class GitIdentifiersTest {
assertArrayEquals(mainTreeId, GitIdentifiers.treeId(md, tempDir));
assertArrayEquals(srcTreeId, GitIdentifiers.treeId(md, src));
}
+
+ @ParameterizedTest
+ @ValueSource(strings = {"x", "parent/x"})
+ void testTreeIdRejectsFileDirectoryConflicts(final String name) throws
Exception {
+ final GitIdentifiers.TreeIdBuilder fileFirst =
GitIdentifiers.treeIdBuilder(DigestUtils.getSha1Digest());
+ fileFirst.addFile(GitIdentifiers.FileMode.REGULAR, name,
HELLO_CONTENT);
+ fileFirst.addDirectory(name);
+ assertThrows(IllegalStateException.class, fileFirst::get);
+ final GitIdentifiers.TreeIdBuilder directoryFirst =
GitIdentifiers.treeIdBuilder(DigestUtils.getSha1Digest());
+ directoryFirst.addDirectory(name);
+ directoryFirst.addSymbolicLink(name, "target");
+ assertThrows(IllegalStateException.class, directoryFirst::get);
+ }
+
+ @Test
+ void testTreeIdRejectsNulSerializationCollision() throws Exception {
+ final MessageDigest md = DigestUtils.getSha1Digest();
+ final byte[] a = "2161978".getBytes(StandardCharsets.UTF_8);
+ final byte[] b = "payload".getBytes(StandardCharsets.UTF_8);
+ final byte[] blobId = GitIdentifiers.blobId(md, a);
+ assertEquals("615d6b396b134e0c1a617b0b7050632d627d154f",
Hex.encodeHexString(blobId));
+ final GitIdentifiers.TreeIdBuilder legitimate =
GitIdentifiers.treeIdBuilder(md);
+ legitimate.addFile(GitIdentifiers.FileMode.REGULAR, "a", a);
+ legitimate.addFile(GitIdentifiers.FileMode.REGULAR, "b", b);
+ assertEquals("cb1e930df28b6dc3ab8933ff7a8d233f1c189460",
Hex.encodeHexString(legitimate.get()));
+ // Without validation this single entry serializes identically to the
legitimate two-entry tree.
+ final String forgedName = "a\0" + new String(blobId,
StandardCharsets.UTF_8) + "100644 b";
+ final GitIdentifiers.TreeIdBuilder forged =
GitIdentifiers.treeIdBuilder(md);
+ assertThrows(IllegalArgumentException.class, () ->
forged.addFile(GitIdentifiers.FileMode.REGULAR, forgedName, b));
+ }
+
+ @Test
+ void testTreeIdRejectsTrailingSlash() {
+ final GitIdentifiers.TreeIdBuilder builder =
GitIdentifiers.treeIdBuilder(DigestUtils.getSha1Digest());
+ assertThrows(IllegalArgumentException.class, () ->
builder.addFile(GitIdentifiers.FileMode.REGULAR, "dir/", HELLO_CONTENT));
+ }
+
+ /**
+ * Tree entry names are ordered by their UTF-8 bytes, which is not the
order {@link String#compareTo(String)} gives when a supplementary character
meets a
+ * Basic Multilingual Plane character from U+E000 up: U+FF21 encodes to
{@code EF BC A1} and U+1F600 to {@code F0 9F 98 80}, so Git sorts U+FF21 first,
while
+ * the UTF-16 code units place the surrogate pair of U+1F600 first.
+ *
+ * <p>The expected identifier is the one {@code git write-tree} produces
for a tree holding the same two entries.</p>
+ */
+ @Test
+ void testTreeIdSortsSupplementaryPlaneNamesLikeGit(@TempDir final Path
tempDir) throws Exception {
+ final String fullWidthA = "\uFF21";
+ final String grinningFace = "\uD83D\uDE00";
+ final byte[] content = "x".getBytes(StandardCharsets.UTF_8);
+ final String expected = "9f9c1fc3580195f51d3e71b384ef1d57740e2151";
+ final MessageDigest md = DigestUtils.getSha1Digest();
+
+ // Entries are added in the wrong order on purpose, so only the sort
decides the result.
+ final GitIdentifiers.TreeIdBuilder builder =
GitIdentifiers.treeIdBuilder(md);
+ builder.addFile(GitIdentifiers.FileMode.REGULAR, grinningFace,
content);
+ builder.addFile(GitIdentifiers.FileMode.REGULAR, fullWidthA, content);
+ assertEquals(expected, Hex.encodeHexString(builder.get()));
+
+ try {
+ Files.write(tempDir.resolve(fullWidthA), content);
+ Files.write(tempDir.resolve(grinningFace), content);
+ } catch (final IOException e) {
+ Assumptions.abort("Filesystem cannot hold the test entry names: "
+ e);
+ }
+ assertEquals(expected, Hex.encodeHexString(GitIdentifiers.treeId(md,
tempDir)));
+ }
}
