From 094df5a916db6060fa7f7d8c1f61cc0b970a9433 Mon Sep 17 00:00:00 2001 From: Robin Rosenberg Date: Sun, 23 Sep 2012 15:18:19 +0200 Subject: [PATCH] Allow @ in branch names and tighten syntax checking Valid refs are defined by git-check-ref-format(1). In addition we will not try to perform a lookup of an invalid ref name in Repository.resolve(). Reported by R Shapiro in the Eclipse JGit Forum. Change-Id: I0b098eec9ecb98a9ce16b1cfb476729aaf2fb190 --- .../jgit/lib/RepositoryResolveTest.java | 53 +++++++++++++++++++ .../src/org/eclipse/jgit/lib/Repository.java | 40 +++++++++++--- 2 files changed, 86 insertions(+), 7 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryResolveTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryResolveTest.java index defd93b10..91364ce98 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryResolveTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryResolveTest.java @@ -47,14 +47,18 @@ package org.eclipse.jgit.lib; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import java.io.IOException; import org.eclipse.jgit.api.Git; +import org.eclipse.jgit.errors.AmbiguousObjectException; import org.eclipse.jgit.errors.IncorrectObjectTypeException; +import org.eclipse.jgit.errors.RevisionSyntaxException; import org.eclipse.jgit.revwalk.RevCommit; import org.junit.Test; @@ -133,9 +137,11 @@ public class RepositoryResolveTest extends SampleDataRepositoryTestCase { public void testDistance_past_root() throws IOException { assertEquals("42e4e7c5e507e113ebbb7801b16b52cf867b7ce1",db.resolve("6462e7d8024396b14d7651e2ec11e2bbf07a05c4~1").name()); assertNull(db.resolve("6462e7d8024396b14d7651e2ec11e2bbf07a05c4~~")); + assertNull(db.resolve("6462e7d8024396b14d7651e2ec11e2bbf07a05c4^^")); assertNull(db.resolve("6462e7d8024396b14d7651e2ec11e2bbf07a05c4~2")); assertNull(db.resolve("6462e7d8024396b14d7651e2ec11e2bbf07a05c4~99")); assertNull(db.resolve("42e4e7c5e507e113ebbb7801b16b52cf867b7ce1~~")); + assertNull(db.resolve("42e4e7c5e507e113ebbb7801b16b52cf867b7ce1^^")); assertNull(db.resolve("42e4e7c5e507e113ebbb7801b16b52cf867b7ce1~2")); assertNull(db.resolve("42e4e7c5e507e113ebbb7801b16b52cf867b7ce1~99")); assertEquals("42e4e7c5e507e113ebbb7801b16b52cf867b7ce1",db.resolve("master~6").name()); @@ -289,6 +295,53 @@ public class RepositoryResolveTest extends SampleDataRepositoryTestCase { assertEquals("refs/remotes/origin/main", db.simplify("@{upstream}")); } + @Test + public void invalidNames() throws AmbiguousObjectException, IOException { + assertTrue(Repository.isValidRefName("x/a")); + assertTrue(Repository.isValidRefName("x/a.b")); + assertTrue(Repository.isValidRefName("x/a@b")); + assertTrue(Repository.isValidRefName("x/a@b{x}")); + assertTrue(Repository.isValidRefName("x/a/b")); + assertTrue(Repository.isValidRefName("x/a]b")); // odd, yes.. + assertTrue(Repository.isValidRefName("x/\u00a0")); // unicode is fine, + // even hard space + assertFalse(Repository.isValidRefName("x/.a")); + assertFalse(Repository.isValidRefName("x/a.")); + assertFalse(Repository.isValidRefName("x/a..b")); + assertFalse(Repository.isValidRefName("x//a")); + assertFalse(Repository.isValidRefName("x/a/")); + assertFalse(Repository.isValidRefName("x/a//b")); + assertFalse(Repository.isValidRefName("x/a[b")); + assertFalse(Repository.isValidRefName("x/a^b")); + assertFalse(Repository.isValidRefName("x/a*b")); + assertFalse(Repository.isValidRefName("x/a?b")); + assertFalse(Repository.isValidRefName("x/a~1")); + assertFalse(Repository.isValidRefName("x/a\\b")); + assertFalse(Repository.isValidRefName("x/a\u0000")); + + assertUnparseable("."); + assertUnparseable("x@{3"); + assertUnparseable("x[b"); + assertUnparseable("x y"); + assertUnparseable("x."); + assertUnparseable(".x"); + assertUnparseable("a..b"); + assertUnparseable("x\\b"); + assertUnparseable("a~b"); + assertUnparseable("a^b"); + assertUnparseable("a\u0000"); + } + + private void assertUnparseable(String s) throws AmbiguousObjectException, + IOException { + try { + db.resolve(s); + fail("'" + s + "' should be unparseable"); + } catch (RevisionSyntaxException e) { + // good + } + } + private static ObjectId id(String name) { return ObjectId.fromString(name); } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java index 16f8cdfef..76f537817 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java @@ -429,7 +429,12 @@ public abstract class Repository { case '^': if (rev == null) { if (name == null) - name = new String(revChars, done, i); + if (done == 0) + name = new String(revChars, done, i); + else { + done = i + 1; + break; + } rev = parseSimple(rw, name); name = null; if (rev == null) @@ -471,7 +476,7 @@ public abstract class Repository { rev = commit.getParent(pnum - 1); } i = j - 1; - done = i; + done = j; break; case '{': int k; @@ -512,7 +517,6 @@ public abstract class Repository { } else throw new IncorrectObjectTypeException(rev, Constants.TYPE_COMMIT); - } } else { rev = rw.peel(rev); @@ -526,11 +530,17 @@ public abstract class Repository { throw new IncorrectObjectTypeException(rev, Constants.TYPE_COMMIT); } + done = i + 1; break; case '~': if (rev == null) { if (name == null) - name = new String(revChars, done, i); + if (done == 0) + name = new String(revChars, done, i); + else { + done = i + 1; + break; + } rev = parseSimple(rw, name); name = null; if (rev == null) @@ -568,10 +578,13 @@ public abstract class Repository { --dist; } i = l - 1; + done = l; break; case '@': if (rev != null) throw new RevisionSyntaxException(revstr); + if (i + 1 < revChars.length && revChars[i + 1] != '{') + continue; int m; String time = null; for (m = i + 2; m < revChars.length; ++m) { @@ -588,6 +601,8 @@ public abstract class Repository { // Currently checked out branch, HEAD if // detached name = Constants.HEAD; + if (!Repository.isValidRefName("x/" + name)) + throw new RevisionSyntaxException(revstr); Ref ref = getRef(name); name = null; if (ref == null) @@ -636,6 +651,8 @@ public abstract class Repository { name = new String(revChars, done, i); if (name.equals("")) name = Constants.HEAD; + if (!Repository.isValidRefName("x/" + name)) + throw new RevisionSyntaxException(revstr); Ref ref = getRef(name); name = null; if (ref == null) @@ -680,7 +697,11 @@ public abstract class Repository { return rev.copy(); if (name != null) return name; + if (rev == null && done == revstr.length()) + return null; name = revstr.substring(done); + if (!Repository.isValidRefName("x/" + name)) + throw new RevisionSyntaxException(revstr); if (getRef(name) != null) return name; return resolveSimple(name); @@ -709,9 +730,11 @@ public abstract class Repository { if (ObjectId.isId(revstr)) return ObjectId.fromString(revstr); - Ref r = getRefDatabase().getRef(revstr); - if (r != null) - return r.getObjectId(); + if (Repository.isValidRefName("x/" + revstr)) { + Ref r = getRefDatabase().getRef(revstr); + if (r != null) + return r.getObjectId(); + } if (AbbreviatedObjectId.isId(revstr)) return resolveAbbreviation(revstr); @@ -1132,6 +1155,8 @@ public abstract class Repository { case '/': if (i == 0 || i == len - 1) return false; + if (p == '/') + return false; components++; break; case '{': @@ -1141,6 +1166,7 @@ public abstract class Repository { case '~': case '^': case ':': case '?': case '[': case '*': case '\\': + case '\u007F': return false; } p = c;