Fix branch and tag name validation

The validation for branch and tag names has been
too limited. This led to errors in the frontend for
branches, that had been created using the
cli tools for git or hg but have not been seen as
valid by SCM-Manager.

To fix this, the patterns to validate branch and
tag names are relaxed and relate to the git
rules (https://git-scm.com/docs/git-check-ref-format).
Because these rules could not be expressed
using regular expressions alone, in addition
possible exceptions will be handled in the
git branch and tag commands.

Committed-by: Konstantin Schaper <konstantin.schaper@cloudogu.com>
Co-authored-by: Eduard Heimbuch <eduard.heimbuch@cloudogu.com>
Co-authored-by: René Pfeuffer <rene.pfeuffer@cloudogu.com>
Co-authored-by: Konstantin Schaper <konstantin.schaper@cloudogu.com>

scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBranchCommand.java

@@ -24,9 +24,11 @@
 
 package sonia.scm.repository.spi;
 
+import lombok.extern.slf4j.Slf4j;
 import org.eclipse.jgit.api.Git;
 import org.eclipse.jgit.api.errors.CannotDeleteCurrentBranchException;
 import org.eclipse.jgit.api.errors.GitAPIException;
+import org.eclipse.jgit.api.errors.InvalidRefNameException;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.Repository;
@@ -59,7 +61,9 @@ import static java.util.Collections.singleton;
 import static java.util.Collections.singletonList;
 import static org.eclipse.jgit.lib.ObjectId.zeroId;
 import static sonia.scm.ContextEntry.ContextBuilder.entity;
+import static sonia.scm.ScmConstraintViolationException.Builder.doThrow;
 
+@Slf4j
 public class GitBranchCommand extends AbstractGitCommand implements BranchCommand {
 
   private final HookContextFactory hookContextFactory;
@@ -88,6 +92,10 @@ public class GitBranchCommand extends AbstractGitCommand implements BranchComman
       Ref ref = git.branchCreate().setStartPoint(request.getParentBranch()).setName(request.getNewBranch()).call();
       eventBus.post(new PostReceiveRepositoryHookEvent(hookEvent));
       return Branch.normalBranch(request.getNewBranch(), GitUtil.getId(ref.getObjectId()));
+    } catch (InvalidRefNameException e) {
+      log.debug("got exception for invalid branch name {}", request.getNewBranch(), e);
+      doThrow().violation("Invalid branch name", "name").when(true);
+      return null;
     } catch (GitAPIException | IOException ex) {
       throw new InternalRepositoryException(repository, "could not create branch " + request.getNewBranch(), ex);
     }

scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitTagCommand.java

@@ -25,9 +25,11 @@
 package sonia.scm.repository.spi;
 
 import com.google.common.base.Strings;
+import lombok.extern.slf4j.Slf4j;
 import org.apache.shiro.SecurityUtils;
 import org.eclipse.jgit.api.Git;
 import org.eclipse.jgit.api.errors.GitAPIException;
+import org.eclipse.jgit.api.errors.InvalidTagNameException;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Ref;
@@ -68,7 +70,9 @@ import static org.eclipse.jgit.lib.ObjectId.fromString;
 import static org.eclipse.jgit.lib.ObjectId.zeroId;
 import static sonia.scm.ContextEntry.ContextBuilder.entity;
 import static sonia.scm.NotFoundException.notFound;
+import static sonia.scm.ScmConstraintViolationException.Builder.doThrow;
 
+@Slf4j
 public class GitTagCommand extends AbstractGitCommand implements TagCommand {
   public static final String REFS_TAGS_PREFIX = "refs/tags/";
   private final HookContextFactory hookContextFactory;
@@ -129,6 +133,10 @@ public class GitTagCommand extends AbstractGitCommand implements TagCommand {
       eventBus.post(new PostReceiveRepositoryHookEvent(hookEvent));
 
       return tag;
+    } catch (InvalidTagNameException e) {
+      log.debug("got exception for invalid tag name {}", request.getName(), e);
+      doThrow().violation("Invalid tag name", "name").when(true);
+      return null;
     } catch (IOException | GitAPIException ex) {
       throw new InternalRepositoryException(repository, "could not create tag " + name + " for revision " + revision, ex);
     }

scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBranchCommandTest.java

@@ -30,8 +30,8 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.ArgumentCaptor;
 import org.mockito.Mock;
-import org.mockito.invocation.InvocationOnMock;
 import org.mockito.junit.MockitoJUnitRunner;
+import sonia.scm.ScmConstraintViolationException;
 import sonia.scm.event.ScmEventBus;
 import sonia.scm.repository.Branch;
 import sonia.scm.repository.Changeset;
@@ -42,7 +42,6 @@ import sonia.scm.repository.PreProcessorUtil;
 import sonia.scm.repository.PreReceiveRepositoryHookEvent;
 import sonia.scm.repository.api.BranchRequest;
 import sonia.scm.repository.api.HookChangesetBuilder;
-import sonia.scm.repository.api.HookContext;
 import sonia.scm.repository.api.HookContextFactory;
 
 import java.io.IOException;
@@ -130,6 +129,15 @@ public class GitBranchCommandTest extends AbstractGitCommandTestBase {
     assertThrows(CannotDeleteDefaultBranchException.class, () -> command.deleteOrClose(branchToBeDeleted));
   }
 
+  @Test
+  public void shouldThrowViolationExceptionForInvalidBranchName() {
+    BranchRequest branchRequest = new BranchRequest();
+    branchRequest.setNewBranch("invalid..name");
+
+    GitBranchCommand command = createCommand();
+    assertThrows(ScmConstraintViolationException.class, () -> command.branch(branchRequest));
+  }
+
   private GitBranchCommand createCommand() {
     return new GitBranchCommand(createContext(), hookContextFactory, eventBus, converterFactory);
   }

scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitTagCommandTest.java

@@ -41,6 +41,7 @@ import org.junit.runner.RunWith;
 import org.mockito.ArgumentCaptor;
 import org.mockito.Mock;
 import org.mockito.junit.MockitoJUnitRunner;
+import sonia.scm.ScmConstraintViolationException;
 import sonia.scm.event.ScmEventBus;
 import sonia.scm.repository.Changeset;
 import sonia.scm.repository.GitChangesetConverter;
@@ -63,6 +64,7 @@ import java.util.List;
 import java.util.Optional;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.mock;
@@ -184,6 +186,14 @@ public class GitTagCommandTest extends AbstractGitCommandTestBase {
       .containsExactly("383b954b27e052db6880d57f1c860dc208795247");
   }
 
+  @Test
+  public void shouldThrowViolationExceptionForInvalidBranchName() {
+    TagCreateRequest tagRequest = new TagCreateRequest("592d797cd36432e591416e8b2b98154f4f163411", "invalid..name");
+
+    GitTagCommand command = createCommand();
+    assertThrows(ScmConstraintViolationException.class, () -> command.create(tagRequest));
+  }
+
   private GitTagCommand createCommand() {
     return new GitTagCommand(createContext(), hookContextFactory, eventBus, converterFactory);
   }