Page MenuHomePhabricator
Create Task
Maniphest T337508

Investigate Gerrit git repositories having invalid references
Open, Needs TriagePublic
Actions

Description

Upstream bug https://bugs.eclipse.org/bugs/show_bug.cgi?id=582000

When doing a full replication of Gerrit repositories to a fresh empty replica, some errors occurred (full log P48560):

Error: Failed replicate of refs/master to gerrit2@gerrit2002.wikimedia.org:/srv/gerrit/git/labs/private.git, reason: funny refname
Error: Failed replicate of refs/master to gerrit2@gerrit2002.wikimedia.org:/srv/gerrit/git/mediawiki/services/ores/deploy.git, reason: funny refname
Error: Failed replicate of refs/for2.4.4 to gerrit2@gerrit2002.wikimedia.org:/srv/gerrit/git/mediawiki/skins/Metrolook.git, reason: funny refname
Error: Failed replicate of refs/for3.0-beta-9 to gerrit2@gerrit2002.wikimedia.org:/srv/gerrit/git/mediawiki/skins/Metrolook.git, reason: funny refname
Error: Failed replicate of refs/master to gerrit2@gerrit2002.wikimedia.org:/srv/gerrit/git/operations/debs/kubeyaml.git, reason: funny refname
Error: Failed replicate of refs/wmf-192fix to gerrit2@gerrit2002.wikimedia.org:/srv/gerrit/git/operations/software/nginx.git, reason: funny refname
Error: Failed replicate of refs/master to gerrit2@gerrit2002.wikimedia.org:/srv/gerrit/git/operations/software/puppet-compiler.git, reason: funny refname

That seems to come from jgit (Gerrit stable-3.5 has 74fa245b3):

org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java
/**
 * Check validity of a ref name. It must not contain character that has
 * a special meaning in a Git object reference expression. Some other
 * dangerous characters are also excluded.
 *
 * For portability reasons '\' is excluded
 *
 * @param refName a {@link java.lang.String} object.
 * @return true if refName is a valid ref name
 */
public static boolean isValidRefName(String refName) {
  final int len = refName.length();
  if (len == 0) {
    return false;
  }
  if (refName.endsWith(LOCK_SUFFIX)) {
    return false;
  }

  // Refs may be stored as loose files so invalid paths
  // on the local system must also be invalid refs.
  try {
    SystemReader.getInstance().checkPath(refName);
  } catch (CorruptObjectException e) {
    return false;
  }

  int components = 1;
  char p = '\0';
  for (int i = 0; i < len; i++) {
    final char c = refName.charAt(i);
    if (c <= ' ')
      return false;
    switch (c) {
    case '.':
      switch (p) {
      case '\0': case '/': case '.':
        return false;
      }
      if (i == len -1)
        return false;
      break;
    case '/':
      if (i == 0 || i == len - 1)
        return false;
      if (p == '/')
        return false;
      components++;
      break;
    case '{':
      if (p == '@')
        return false;
      break;
    case '~': case '^': case ':':
    case '?': case '[': case '*':
    case '\\':
    case '\u007F':
      return false;
    }
    p = c;
  }
  return components > 1;
}

Details

Related Changes in Gerrit:
SubjectRepoBranchLines +/-
Prevent pushing to funny refname refs/masterAll-Projectsrefs/meta/config+2 -0
Customize query in gerrit

Related Objects
Search...

Event Timeline

hashar created this task.May 25 2023, 5:42 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 25 2023, 5:42 PM
hashar mentioned this in T334521: upgrade gerrit servers to bullseye.May 25 2023, 5:45 PM
• thcipriani subscribed.EditedMay 25 2023, 5:57 PM

Superficially similar to T275946: Can't delete weird ref using git in Gerrit.

In that task, "funny refname" came from cgit. So, the replica's cgit in this case. That is, jgit accepts the ref name refs/master, but cgit does not.

The fix here is probably just to drop the bad ref names from the primary gerrit. They all are likely mistaken pushes. Bonus points for filing an upstream bug with jgit.

hashar added a comment.May 25 2023, 6:17 PM

I found that code following the jGit code path to push. The components variable is initialized to 1 it is incremented for each / found after refs/. Which enforces refs/*/* and thus rejects a push to refs/master.

labs/private.git has always been hosted on Gerrit, so apparently Gerrit receive-pack accepts it just fine, but it later can't be replicated. That sounds like a fun bug to report to Upstream

I guess it can be tested against test/gerrit-ping:

  • Watch the replication_log file
  • git push origin HEAD:refs/master

If the push is accepted and the replication of the update fails, then there is a bug, or at least an inconsistency between what Gerrit is willing to receive and what the replication plugin is willing to replicate.

Another possibility is that it is no more possible to push refs/master and the references we have date from an older Gerrit version. There might still be an issue though if we mirror some upstream repository having such a refs, the imported ref would not be replicated which also sounds like a bug.

Fun times.

• thcipriani added a comment.May 25 2023, 6:42 PM
In T337508#8881101, @hashar wrote:

I found that code following the jGit code path to push. The components variable is initialized to 1 it is incremented for each / found after refs/. Which enforces refs/*/* and thus rejects a push to refs/master.

eh...that's not the way I read it. Looks like it just enforces 1×/ somewhere.

Played with it in P48569 jgit-fiddling.java. Output was:

(°▽°)❥ java jgit-fiddling.java
Testing valid ref: refs/heads/master...true
Testing valid ref: refs/master...true
Testing valid ref: 💩...false

Which is different than what you found cgit does in T275946#6875226

hashar added a comment.May 26 2023, 1:36 PM

Ah thanks for actually running the code! So I was off by one slash and refs/master is accepted. So I don't know from where the funny name error comes :/ Fun to see I already went in the cgit version of that rabbit hole two years ago!

hashar mentioned this in T275946: Can't delete weird ref using git in Gerrit.May 26 2023, 1:58 PM
hashar added a comment.May 26 2023, 2:06 PM

When the replication plugin uses SSH for transport it establishes a ssh connection (on port 22, not the Gerrit daemon ssh server on 29418) and then use git-receive-pack / git-upload-pack:

src/main/resources/Documentation/config.md
remote.NAME.receivepack
:   Path of the `git-receive-pack` executable on the remote
    system, if using the SSH transport.

    Defaults to `git-receive-pack`.

remote.NAME.uploadpack
:   Path of the `git-upload-pack` executable on the remote system,
    if using the SSH transport.

    Defaults to `git-upload-pack`.

The objects are then streamed over ssh to the stdin of the cgit command which I guess lands us to the code at T275946#6875226:

refs.c
static int check_or_sanitize_refname(const char *refname, int flags,
                                     struct strbuf *sanitized)
{
...
        if (!(flags & REFNAME_ALLOW_ONELEVEL) && component_count < 2)
                return -1; /* Refname has only one component. */
        return 0;

cgit has no problem pushing a refs/onelevel and Gerrit accepts it just fine:

$ git remote -v
origin	ssh://gerrit.wikimedia.org:29418/test/gerrit-ping.git (fetch)
origin	ssh://gerrit.wikimedia.org:29418/test/gerrit-ping.git (push)

$ git push origin HEAD:refs/firstlevel
Locking support detected on remote "origin". Consider enabling it with:
  $ git config lfs.https://gerrit.wikimedia.org/test/gerrit-ping.git/info/lfs.locksverify true
Total 0 (delta 0), reused 0 (delta 0), pack-reused 0
remote: Processing changes: refs: 1, done    
To ssh://gerrit.wikimedia.org:29418/test/gerrit-ping.git
 * [new reference]   HEAD -> refs/firstlevel

Then if I attempt to replicate it:

)$ ssh -p 29418 gerrit.wikimedia.org replication start test/gerrit-ping --wait
Error: Failed replicate of refs/firstlevel to gerrit2@gerrit2002.wikimedia.org:/srv/gerrit/git/test/gerrit-ping.git, reason: funny refname
Replicate test/gerrit-ping ref ..all.. to gerrit2002.wikimedia.org, FAILED! (REJECTED_OTHER_REASON)
Replicate test/gerrit-ping ref ..all.. to github.com, Succeeded! (OK)
Replication of test/gerrit-ping ref ..all.. completed to 2 nodes, 
----------------------------------------------
Replication completed with some errors!

cgit on the Gerrit replica reject it.

Replication to Github succeeded cause we only push refs/heads/* and refs/tags/*.

I think the issue is that JGit should match cgit behavior and rejects those as well. Aka the Java code in the task description should be updated.

hashar added a comment.May 26 2023, 2:23 PM

From https://gerrit.googlesource.com/jgit in org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ValidRefNameTest.java I have added a test:

@Test
public void testMustRejectOneLevel() {
    assertValid(false, "refs/firstlevel");
}

Ran the test:

$ bazel test //org.eclipse.jgit.test:org_eclipse_jgit_lib_ValidRefNameTest
...
Test output for //org.eclipse.jgit.test:org_eclipse_jgit_lib_ValidRefNameTest:
JUnit4 Test Runner
........E...............
Time: 0.14
There was 1 failure:
1) testMustRejectOneLevel(org.eclipse.jgit.lib.ValidRefNameTest)
java.lang.AssertionError: "refs/firstlevel" expected:<false> but was:<true>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:120)
	at org.eclipse.jgit.junit.Assert.assertEquals(Assert.java:44)
	at org.eclipse.jgit.lib.ValidRefNameTest.assertValid(ValidRefNameTest.java:25)
	at org.eclipse.jgit.lib.ValidRefNameTest.testMustRejectOneLevel(ValidRefNameTest.java:98)

It I adjust the higher level test for ReceivePack to make it test with refs/master instead of refs/heads/master:

--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackTest.java
@@ -55,12 +55,12 @@ public class ReceivePackTest {
        public void parseCommand() throws Exception {
                String o = "0000000000000000000000000000000000000000";
                String n = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef";
-               String r = "refs/heads/master";
+               String r = "refs/master";
                ReceiveCommand cmd = ReceivePack.parseCommand(o + " " + n + " " + r);
                assertEquals(ObjectId.zeroId(), cmd.getOldId());
                assertEquals("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
                                cmd.getNewId().name());
-               assertEquals("refs/heads/master", cmd.getRefName());
+               assertEquals("refs/master", cmd.getRefName());
 
                assertParseCommandFails(null);
                assertParseCommandFails("");

It passes:

$ bazel test //org.eclipse.jgit.test:org_eclipse_jgit_transport_ReceivePackTest
...
//org.eclipse.jgit.test:org_eclipse_jgit_transport_ReceivePackTest       PASSED in 0.6s

Executed 1 out of 1 test: 1 test passes.

So we gotta teach jgit to reject those one level refs. I am not sure why it is a flag in cgit there might be some case for which it should be accepted (one case is attempt to deleting a ref pointing to a null sha1 (0000...).

hashar added a project: Upstream.May 30 2023, 1:58 PM

One of the issue is a lot of repositories have push permission on refs/* which thus lets refs/master slide in or refs/head/master (singular head instead of plural heads) when most probably the default should be only refs/heads/* and refs/for/*. But I digress, jgit should reject the cgit invalid refs/master.

From Matthias Sohn (Upstream):

JGit should follow https://git-scm.com/docs/git-check-ref-format, looks like there's some room for improvements

I will look at crafting a bug at https://bugs.eclipse.org/bugs/

hashar updated the task description. (Show Details)May 30 2023, 2:52 PM
hashar moved this task from Backlog to Reported Upstream on the Upstream board.

Reported Upstream as https://bugs.eclipse.org/bugs/show_bug.cgi?id=582000

hashar claimed this task.May 30 2023, 5:04 PM
hashar moved this task from Reported Upstream to Patch proposed upstream on the Upstream board.May 30 2023, 8:52 PM

And I have send the patch upstream: https://git.eclipse.org/r/c/jgit/jgit/+/202190

• thcipriani edited projects, added Release-Engineering-Team (Seen); removed Release-Engineering-Team.Jul 23 2025, 4:01 PM
hashar added a parent task: T275946: Can't delete weird ref using git in Gerrit.Oct 7 2025, 9:59 AM
gerritbot added a comment.Oct 7 2025, 10:05 AM

Change #1194149 had a related patch set uploaded (by Hashar; author: Hashar):

[All-Users@refs/meta/config] Prevent pushing to funny refname refs/master

https://gerrit.wikimedia.org/r/1194149

gerritbot added a project: Patch-For-Review.Oct 7 2025, 10:05 AM
hashar added a comment.Oct 7 2025, 10:16 AM

I went to propose https://gerrit.wikimedia.org/r/c/All-Users/+/1194149 which only blocks refs/master. It is not sufficient, we had pushes to some other names and we should ideally block them.

I went wondering whether we could use refs/*, assuming * does not match / but it does. I went to read the documentation at https://gerrit.wikimedia.org/r/Documentation/config-project-config.html#access-subsection:

access subsections define access rules for a ref or a ref namespace.

The ref or ref namespace is specified as the subsection name and can be:

  • a concrete ref (e.g. refs/heads/master)
  • a ref pattern (last path segment is '*', e.g. refs/heads/*)
  • a regular expression (must start with '^', e.g. ^refs/heads/rel-.*).

Since any first level ref is invalid for cgit, we can go with a regex. Tentatively block pushes to ^refs/[^/]* instead.

hashar added a comment.Oct 7 2025, 10:21 AM

On https://gerrit.wikimedia.org/r/admin/repos/test/gerrit-ping,access if I try to enter a ref ^refs/[^/]* I get:

Error 400 (Bad Request): com.google.gerrit.exceptions.InvalidNameException: Invalid Name: ^refs/[^/]*
Endpoint: /projects/*/access

:(

hashar added a comment.Oct 7 2025, 1:49 PM

The trailing * causes InvalidNameException somehow. I workaround it replacing it with a + to form: ^refs/[^/]+.

On test/gerrit-ping I went to block Create Reference (rather than Push):

project.config
[access "^refs/[^/]+"]
	create = block group Registered Users

I can't create refs/master anymore:

$ git push origin origin/master:refs/master
Total 0 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
remote: error: branch refs/master:
remote: You need 'Create' rights to create new references.
remote: User: hashar
remote: Contact an administrator to fix the permissions
remote: Processing changes: refs: 1, done    
To ssh://gerrit.wikimedia.org:29418/test/gerrit-ping.git
 ! [remote rejected] origin/master -> refs/master (prohibited by Gerrit: not permitted: create)
error: failed to push some refs to 'ssh://gerrit.wikimedia.org:29418/test/gerrit-ping.git'

I can still create a branch:

$ git push origin origin/master:refs/heads/master3
Total 0 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
remote: Processing changes: refs: 1, done    
To ssh://gerrit.wikimedia.org:29418/test/gerrit-ping.git
 * [new branch]      origin/master -> master3

Or forge a review:

$ git commit -m 'Can I forge a review?' --allow-empty
[master e8464dc] Can I forge a review?
$ git push origin HEAD:refs/for/master
Enumerating objects: 1, done.
Counting objects: 100% (1/1), done.
Writing objects: 100% (1/1), 231 bytes | 231.00 KiB/s, done.
Total 1 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
remote: Processing changes: refs: 1, new: 1, done    
remote: 
remote: SUCCESS
remote: 
remote:   https://gerrit.wikimedia.org/r/c/test/gerrit-ping/+/1194200 Can I forge a review? [NEW]
remote: 
To ssh://gerrit.wikimedia.org:29418/test/gerrit-ping.git
 * [new reference]   HEAD -> refs/for/master
gerritbot added a comment.Nov 4 2025, 1:10 PM

Change #1194149 merged by Hashar:

[All-Users@refs/meta/config] Prevent pushing to funny refname refs/master

https://gerrit.wikimedia.org/r/1194149

gerritbot added a comment.Nov 4 2025, 1:17 PM

Change #1201603 had a related patch set uploaded (by Hashar; author: Hashar):

[All-Projects@refs/meta/config] Prevent pushing to funny refname refs/master

https://gerrit.wikimedia.org/r/1201603

gerritbot added a comment.Nov 4 2025, 1:17 PM

Change #1201603 merged by Hashar:

[All-Projects@refs/meta/config] Prevent pushing to funny refname refs/master

https://gerrit.wikimedia.org/r/1201603

Stashbot added a comment.Nov 4 2025, 1:18 PM

Mentioned in SAL (#wikimedia-releng) [2025-11-04T13:18:56Z] <hashar> gerrit: blocked pushes to refs/[^/]+ , first level references are considered invalid by cgit (funny refname) # T275946 T337508

Maintenance_bot removed a project: Patch-For-Review.Nov 4 2025, 1:34 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits