diff --git a/gradle/changelog/special_characters_for_branches_and_tags.yaml b/gradle/changelog/special_characters_for_branches_and_tags.yaml new file mode 100644 index 0000000000..419ea541c3 --- /dev/null +++ b/gradle/changelog/special_characters_for_branches_and_tags.yaml @@ -0,0 +1,2 @@ +- type: fixed + description: Branch and tag validation regarding special characters diff --git a/scm-core/src/main/java/sonia/scm/repository/Branch.java b/scm-core/src/main/java/sonia/scm/repository/Branch.java index bf13f9f50b..c99093c36e 100644 --- a/scm-core/src/main/java/sonia/scm/repository/Branch.java +++ b/scm-core/src/main/java/sonia/scm/repository/Branch.java @@ -45,9 +45,28 @@ import java.util.regex.Pattern; @XmlAccessorType(XmlAccessType.FIELD) public final class Branch implements Serializable, Validateable { - private static final String VALID_CHARACTERS_AT_START_AND_END = "\\w-,;\\]{}@&+=$#`|<>"; - private static final String VALID_CHARACTERS = VALID_CHARACTERS_AT_START_AND_END + "/."; - public static final String VALID_BRANCH_NAMES = "[" + VALID_CHARACTERS_AT_START_AND_END + "]([" + VALID_CHARACTERS + "]*[" + VALID_CHARACTERS_AT_START_AND_END + "])?"; + /* + The regex for branches is based on the rules for git branch names taken + from the reference format check (https://git-scm.com/docs/git-check-ref-format) + + Below you find the rules, taken from the site. Rules 3, 8 and 9 are not implemented, + because they cannot simply be checked using a regular expression. + + 1. They can include slash / for hierarchical (directory) grouping, but no slash-separated component can begin with a dot . or end with the sequence .lock. + 2. [not relevant for branches] + 3. They cannot have two consecutive dots .. anywhere. + 4. They cannot have ASCII control characters (i.e. bytes whose values are lower than \040, or \177 DEL), space, tilde ~, caret ^, or colon : anywhere. + 5. They cannot have question-mark ?, asterisk *, or open bracket [ anywhere. See the --refspec-pattern option below for an exception to this rule. + 6. They cannot begin or end with a slash / or contain multiple consecutive slashes (see the --normalize option below for an exception to this rule) + 7. They cannot end with a dot .. + 8. They cannot contain a sequence @{. + 9. They cannot be the single character @. + 10. They cannot contain a \. + */ + + private static final String ILLEGAL_CHARACTERS = "\\\\/\\s\\[~^:?*"; + private static final String VALID_PATH_PART = "[^." + ILLEGAL_CHARACTERS + "](?:[^" + ILLEGAL_CHARACTERS + "]*[^." + ILLEGAL_CHARACTERS + "])?"; + public static final String VALID_BRANCH_NAMES = VALID_PATH_PART + "(?:/" + VALID_PATH_PART + ")*"; public static final Pattern VALID_BRANCH_NAME_PATTERN = Pattern.compile(VALID_BRANCH_NAMES); private static final long serialVersionUID = -4602244691711222413L; diff --git a/scm-core/src/test/java/sonia/scm/repository/BranchTest.java b/scm-core/src/test/java/sonia/scm/repository/BranchTest.java new file mode 100644 index 0000000000..15f2c8625a --- /dev/null +++ b/scm-core/src/test/java/sonia/scm/repository/BranchTest.java @@ -0,0 +1,70 @@ +/* + * MIT License + * + * Copyright (c) 2020-present Cloudogu GmbH and Contributors + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +package sonia.scm.repository; + +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.mockito.junit.jupiter.MockitoExtension; + +import static org.assertj.core.api.Assertions.assertThat; + +@ExtendWith(MockitoExtension.class) +class BranchTest { + + @ParameterizedTest + @ValueSource(strings = { + "a", + "test", + "feature/nöice", + "😄", + "very_long/and/complex%branch+name" + }) + void shouldAcceptValidBranchName(String branchName) { + assertThat(branchName).matches(Branch.VALID_BRANCH_NAME_PATTERN); + } + + @ParameterizedTest + @ValueSource(strings = { + "/", + "/feature/ugly", + "./start", + ".hidden", + "full_stop.", + "very/.hidden", + "some\\place", + "some//place", + "some space", + "home/~", + "some_:", + "2^8", + "real?", + "find*all", + "some[set" + }) + void shouldRejectInvalidBranchName(String branchName) { + assertThat(branchName).doesNotMatch(Branch.VALID_BRANCH_NAME_PATTERN); + } +} diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBranchCommand.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBranchCommand.java index 49949d74cf..007fa61fea 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBranchCommand.java +++ b/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); } diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitTagCommand.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitTagCommand.java index 493d3b382a..1c0fa70b19 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitTagCommand.java +++ b/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); } diff --git a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBranchCommandTest.java b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBranchCommandTest.java index d697b37d27..9a61a461e3 100644 --- a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBranchCommandTest.java +++ b/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); } diff --git a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitTagCommandTest.java b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitTagCommandTest.java index f4258f2a58..fc8c7e51ce 100644 --- a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitTagCommandTest.java +++ b/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); } diff --git a/scm-ui/ui-components/src/validation.ts b/scm-ui/ui-components/src/validation.ts index eb4e4b6d98..7eadd60e16 100644 --- a/scm-ui/ui-components/src/validation.ts +++ b/scm-ui/ui-components/src/validation.ts @@ -28,7 +28,9 @@ export const isNameValid = (name: string) => { return nameRegex.test(name); }; -export const branchRegex = /^[\w-,;\]{}@&+=$#`|<>]([\w-,;\]{}@&+=$#`|<>/.]*[\w-,;\]{}@&+=$#`|<>])?$/; +// See validation regex in Java class "Branch" for further details +export const branchRegex = + /^[^.\\\s[~^:?*](?:[^\\\s[~^:?*]*[^.\\\s[~^:?*])?(?:\/[^.\\\s[~^:?*](?:[^\\\s[~^:?*]*[^.\\\s[~^:?*])?)*$/; export const isBranchValid = (name: string) => { return branchRegex.test(name); diff --git a/scm-ui/ui-webapp/src/repos/branches/components/BranchButtonGroup.tsx b/scm-ui/ui-webapp/src/repos/branches/components/BranchButtonGroup.tsx index 2a79a8d9b4..c14c1af378 100644 --- a/scm-ui/ui-webapp/src/repos/branches/components/BranchButtonGroup.tsx +++ b/scm-ui/ui-webapp/src/repos/branches/components/BranchButtonGroup.tsx @@ -25,6 +25,7 @@ import React from "react"; import { WithTranslation, withTranslation } from "react-i18next"; import { Branch, Repository } from "@scm-manager/ui-types"; import { Button, ButtonAddons } from "@scm-manager/ui-components"; +import { encodePart } from "../../sources/components/content/FileLink"; type Props = WithTranslation & { repository: Repository; @@ -35,10 +36,10 @@ class BranchButtonGroup extends React.Component { render() { const { repository, branch, t } = this.props; - const changesetLink = `/repo/${repository.namespace}/${repository.name}/branch/${encodeURIComponent( + const changesetLink = `/repo/${repository.namespace}/${repository.name}/branch/${encodePart( branch.name )}/changesets/`; - const sourcesLink = `/repo/${repository.namespace}/${repository.name}/sources/${encodeURIComponent(branch.name)}/`; + const sourcesLink = `/repo/${repository.namespace}/${repository.name}/sources/${encodePart(branch.name)}/`; return ( diff --git a/scm-ui/ui-webapp/src/repos/branches/components/BranchRow.tsx b/scm-ui/ui-webapp/src/repos/branches/components/BranchRow.tsx index 1d01439d14..2cd70a979d 100644 --- a/scm-ui/ui-webapp/src/repos/branches/components/BranchRow.tsx +++ b/scm-ui/ui-webapp/src/repos/branches/components/BranchRow.tsx @@ -33,6 +33,7 @@ import DefaultBranchTag from "./DefaultBranchTag"; import AheadBehindTag from "./AheadBehindTag"; import BranchCommitDateCommitter from "./BranchCommitDateCommitter"; import { useKeyboardIteratorTarget } from "@scm-manager/ui-shortcuts"; +import { encodePart } from "../../sources/components/content/FileLink"; type Props = { repository: Repository; @@ -63,7 +64,7 @@ const MobileFlowSpan = styled.span` `; const BranchRow: FC = ({ repository, baseUrl, branch, onDelete, details }) => { - const to = `${baseUrl}/${encodeURIComponent(branch.name)}/info`; + const to = `${baseUrl}/${encodePart(branch.name)}/info`; const [t] = useTranslation("repos"); const ref = useKeyboardIteratorTarget(); diff --git a/scm-ui/ui-webapp/src/repos/branches/containers/CreateBranch.tsx b/scm-ui/ui-webapp/src/repos/branches/containers/CreateBranch.tsx index f09030a306..52f5d7970b 100644 --- a/scm-ui/ui-webapp/src/repos/branches/containers/CreateBranch.tsx +++ b/scm-ui/ui-webapp/src/repos/branches/containers/CreateBranch.tsx @@ -29,6 +29,7 @@ import { Repository } from "@scm-manager/ui-types"; import { ErrorNotification, Loading, Subtitle } from "@scm-manager/ui-components"; import BranchForm from "../components/BranchForm"; import { useBranches, useCreateBranch } from "@scm-manager/ui-api"; +import { encodePart } from "../../sources/components/content/FileLink"; type Props = { repository: Repository; @@ -54,7 +55,7 @@ const CreateBranch: FC = ({ repository }) => { if (createdBranch) { return ( ); } diff --git a/scm-ui/ui-webapp/src/repos/containers/ChangesetsRoot.tsx b/scm-ui/ui-webapp/src/repos/containers/ChangesetsRoot.tsx index c97cebf962..c8fd0bcf7c 100644 --- a/scm-ui/ui-webapp/src/repos/containers/ChangesetsRoot.tsx +++ b/scm-ui/ui-webapp/src/repos/containers/ChangesetsRoot.tsx @@ -29,6 +29,7 @@ import CodeActionBar from "../codeSection/components/CodeActionBar"; import { urls } from "@scm-manager/ui-components"; import Changesets from "./Changesets"; import { RepositoryRevisionContextProvider } from "@scm-manager/ui-api"; +import { encodePart } from "../sources/components/content/FileLink"; type Props = { repository: Repository; @@ -53,14 +54,14 @@ const ChangesetRoot: FC = ({ repository, baseUrl, branches, selectedBranc const evaluateSwitchViewLink = () => { if (selectedBranch) { - return `${baseUrl}/sources/${encodeURIComponent(selectedBranch)}/`; + return `${baseUrl}/sources/${encodePart(selectedBranch)}/`; } return `${baseUrl}/sources/`; }; const onSelectBranch = (branch?: Branch) => { if (branch) { - history.push(`${baseUrl}/branch/${encodeURIComponent(branch.name)}/changesets/`); + history.push(`${baseUrl}/branch/${encodePart(branch.name)}/changesets/`); } else { history.push(`${baseUrl}/changesets/`); } diff --git a/scm-ui/ui-webapp/src/repos/sources/components/content/FileLink.tsx b/scm-ui/ui-webapp/src/repos/sources/components/content/FileLink.tsx index bb98d38e3a..d24d2513c7 100644 --- a/scm-ui/ui-webapp/src/repos/sources/components/content/FileLink.tsx +++ b/scm-ui/ui-webapp/src/repos/sources/components/content/FileLink.tsx @@ -48,11 +48,10 @@ const isLocalRepository = (repositoryUrl: string) => { }; export const encodePart = (part: string) => { - const encoded = encodeURIComponent(part); if (part.includes("%")) { - return encodeURIComponent(encoded); + return encodeURIComponent(part.replace(/%/g, "%25")); } - return encoded; + return encodeURIComponent(part); }; export const createRelativeLink = (repositoryUrl: string) => { diff --git a/scm-ui/ui-webapp/src/repos/sources/containers/Sources.tsx b/scm-ui/ui-webapp/src/repos/sources/containers/Sources.tsx index c87806d2e6..0e26258c89 100644 --- a/scm-ui/ui-webapp/src/repos/sources/containers/Sources.tsx +++ b/scm-ui/ui-webapp/src/repos/sources/containers/Sources.tsx @@ -34,6 +34,7 @@ import replaceBranchWithRevision from "../ReplaceBranchWithRevision"; import FileSearchButton from "../../codeSection/components/FileSearchButton"; import { isEmptyDirectory, isRootFile } from "../utils/files"; import CompareLink from "../../compare/CompareLink"; +import { encodePart } from "../components/content/FileLink"; type Props = { repository: Repository; @@ -65,7 +66,7 @@ const Sources: FC = ({ repository, branches, selectedBranch, baseUrl }) = if (branches && branches.length > 0 && !selectedBranch) { const defaultBranch = branches?.filter((b) => b.defaultBranch === true)[0]; history.replace( - `${baseUrl}/sources/${defaultBranch ? encodeURIComponent(defaultBranch.name) : encodeURIComponent(branches[0].name)}/` + `${baseUrl}/sources/${defaultBranch ? encodePart(defaultBranch.name) : encodePart(branches[0].name)}/` ); } }, [branches, selectedBranch, history, baseUrl]); @@ -93,10 +94,10 @@ const Sources: FC = ({ repository, branches, selectedBranch, baseUrl }) = let url; if (branch) { if (path) { - url = `${baseUrl}/sources/${encodeURIComponent(branch.name)}/${path}`; + url = `${baseUrl}/sources/${encodePart(branch.name)}/${path}`; url = !url.endsWith("/") ? url + "/" : url; } else { - url = `${baseUrl}/sources/${encodeURIComponent(branch.name)}/`; + url = `${baseUrl}/sources/${encodePart(branch.name)}/`; } } else { return; diff --git a/scm-ui/ui-webapp/src/repos/tags/components/TagRow.tsx b/scm-ui/ui-webapp/src/repos/tags/components/TagRow.tsx index 87814989ea..ba19388ce7 100644 --- a/scm-ui/ui-webapp/src/repos/tags/components/TagRow.tsx +++ b/scm-ui/ui-webapp/src/repos/tags/components/TagRow.tsx @@ -28,6 +28,7 @@ import classNames from "classnames"; import { Tag, Link } from "@scm-manager/ui-types"; import { Button, DateFromNow } from "@scm-manager/ui-components"; import { useKeyboardIteratorTarget } from "@scm-manager/ui-shortcuts"; +import { encodePart } from "../../sources/components/content/FileLink"; type Props = { tag: Tag; @@ -47,7 +48,7 @@ const TagRow: FC = ({ tag, baseUrl, onDelete }) => { ); } - const to = `${baseUrl}/${encodeURIComponent(tag.name)}/info`; + const to = `${baseUrl}/${encodePart(tag.name)}/info`; return (