diff --git a/uberfire-rest/uberfire-rest-backend/pom.xml b/uberfire-rest/uberfire-rest-backend/pom.xml index 272a123493..5021094d9d 100644 --- a/uberfire-rest/uberfire-rest-backend/pom.xml +++ b/uberfire-rest/uberfire-rest-backend/pom.xml @@ -72,6 +72,11 @@ slf4j-api + + org.apache.commons + commons-text + + jakarta.annotation diff --git a/uberfire-rest/uberfire-rest-backend/src/main/java/org/guvnor/rest/backend/ProjectResource.java b/uberfire-rest/uberfire-rest-backend/src/main/java/org/guvnor/rest/backend/ProjectResource.java index f563bc8f2d..e7ace11262 100644 --- a/uberfire-rest/uberfire-rest-backend/src/main/java/org/guvnor/rest/backend/ProjectResource.java +++ b/uberfire-rest/uberfire-rest-backend/src/main/java/org/guvnor/rest/backend/ProjectResource.java @@ -46,6 +46,7 @@ import javax.ws.rs.core.UriInfo; import javax.ws.rs.core.Variant; +import org.apache.commons.text.StringEscapeUtils; import org.guvnor.common.services.project.model.WorkspaceProject; import org.guvnor.common.services.project.service.WorkspaceProjectService; import org.guvnor.rest.client.AddBranchJobRequest; @@ -373,7 +374,7 @@ public Response addBranch(@PathParam("spaceName") String spaceName, jobRequest.setJobId(id); jobRequest.setSpaceName(spaceName); jobRequest.setProjectName(projectName); - jobRequest.setNewBranchName(addBranchRequest.getNewBranchName()); + jobRequest.setNewBranchName(escapeHtmlInput(addBranchRequest.getNewBranchName())); jobRequest.setBaseBranchName(addBranchRequest.getBaseBranchName()); jobRequest.setUserIdentifier(sessionInfo.getIdentity().getIdentifier()); addAcceptedJobResult(id); @@ -454,6 +455,16 @@ private ProjectResponse getProjectResponse(WorkspaceProject workspaceProject) { return projectResponse; } + private String escapeHtmlInput(String input) { + if (input != null) { + String escapedInput = StringEscapeUtils.escapeHtml4(input); + escapedInput = escapedInput.replace("'", ""); + return escapedInput; + } else { + return null; + } + } + @POST @Produces(MediaType.APPLICATION_JSON) @Path("/spaces/{spaceName}/projects/{projectName}/maven/compile") @@ -684,7 +695,7 @@ public Response createSpace(Space space) { jobRequest.setJobId(id); jobRequest.setSpaceName(space.getName()); jobRequest.setDescription(space.getDescription()); - jobRequest.setOwner(space.getOwner()); + jobRequest.setOwner(escapeHtmlInput(space.getOwner())); jobRequest.setDefaultGroupId(space.getDefaultGroupId()); addAcceptedJobResult(id); @@ -709,7 +720,7 @@ public Response updateSpace(Space space) { jobRequest.setJobId(id); jobRequest.setSpaceName(space.getName()); jobRequest.setDescription(space.getDescription()); - jobRequest.setOwner(space.getOwner()); + jobRequest.setOwner(escapeHtmlInput(space.getOwner())); jobRequest.setDefaultGroupId(space.getDefaultGroupId()); addAcceptedJobResult(id); diff --git a/uberfire-rest/uberfire-rest-backend/src/test/java/org/guvnor/rest/backend/ProjectResourceJobTest.java b/uberfire-rest/uberfire-rest-backend/src/test/java/org/guvnor/rest/backend/ProjectResourceJobTest.java index aed0157335..3b4f768b57 100644 --- a/uberfire-rest/uberfire-rest-backend/src/test/java/org/guvnor/rest/backend/ProjectResourceJobTest.java +++ b/uberfire-rest/uberfire-rest-backend/src/test/java/org/guvnor/rest/backend/ProjectResourceJobTest.java @@ -315,6 +315,28 @@ public void updateSpace() throws Exception { assertEquals(JobStatus.ACCEPTED, jobResultArgumentCaptor.getValue().getStatus()); } + @Test + public void updateSpaceWithXSSOwer() throws Exception { + String xssOwner = ""; + Space testedSpace = new Space(); + testedSpace.setOwner(xssOwner); + projectResource.updateSpace(testedSpace); + + verify(jobManager).putJob(jobResultArgumentCaptor.capture()); + assertEquals(JobStatus.ACCEPTED, jobResultArgumentCaptor.getValue().getStatus()); + } + + @Test + public void createSpaceWithXSSOwner() throws Exception { + String xssOwner = ""; + Space testedSpace = new Space(); + testedSpace.setOwner(xssOwner); + projectResource.createSpace(testedSpace); + + verify(jobManager).putJob(jobResultArgumentCaptor.capture()); + assertEquals(JobStatus.ACCEPTED, jobResultArgumentCaptor.getValue().getStatus()); + } + @Test public void deleteSpace() throws Exception { @@ -334,6 +356,19 @@ public void addBranch() { assertEquals(JobStatus.ACCEPTED, jobResultArgumentCaptor.getValue().getStatus()); } + @Test + public void addBranchWithXSSName() { + AddBranchRequest addBranchRequest = new AddBranchRequest(); + addBranchRequest.setNewBranchName(""); + + projectResource.addBranch("spaceName", + "projectName", + addBranchRequest); + + verify(jobManager).putJob(jobResultArgumentCaptor.capture()); + assertEquals(JobStatus.ACCEPTED, jobResultArgumentCaptor.getValue().getStatus()); + } + @Test public void removeBranch() { projectResource.removeBranch("spaceName", diff --git a/uberfire-structure/uberfire-structure-backend/pom.xml b/uberfire-structure/uberfire-structure-backend/pom.xml index 568e4754e0..c849864127 100644 --- a/uberfire-structure/uberfire-structure-backend/pom.xml +++ b/uberfire-structure/uberfire-structure-backend/pom.xml @@ -140,6 +140,11 @@ commons-fileupload + + org.apache.commons + commons-text + + org.uberfire uberfire-security-management-api diff --git a/uberfire-structure/uberfire-structure-backend/src/main/java/org/guvnor/structure/backend/InputEscapeUtils.java b/uberfire-structure/uberfire-structure-backend/src/main/java/org/guvnor/structure/backend/InputEscapeUtils.java new file mode 100644 index 0000000000..2a441a2090 --- /dev/null +++ b/uberfire-structure/uberfire-structure-backend/src/main/java/org/guvnor/structure/backend/InputEscapeUtils.java @@ -0,0 +1,53 @@ +/* + * Copyright 2023 Red Hat, Inc. and/or its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.guvnor.structure.backend; + +import java.util.ArrayList; +import java.util.Collection; + +import org.apache.commons.text.StringEscapeUtils; +import org.uberfire.security.Contributor; + +/** + * Utility class to provide input escaping or other + * functionality needed by backend services to sanitize inputs. + */ +public class InputEscapeUtils { + + private InputEscapeUtils() { + // non-instantiable class + } + + public static Collection escapeContributorsNames(Collection contributors) { + Collection escapedContributors = new ArrayList<>(); + contributors.forEach((contributor -> { + String escapedName = escapeHtmlInput(contributor.getUsername()); + escapedContributors.add(new Contributor(escapedName, contributor.getType())); + })); + return escapedContributors; + } + + public static String escapeHtmlInput(String input) { + if (input != null) { + String escapedInput = StringEscapeUtils.escapeHtml4(input); + escapedInput = escapedInput.replace("'", ""); + return escapedInput; + } else { + return null; + } + } + +} diff --git a/uberfire-structure/uberfire-structure-backend/src/main/java/org/guvnor/structure/backend/organizationalunit/OrganizationalUnitServiceImpl.java b/uberfire-structure/uberfire-structure-backend/src/main/java/org/guvnor/structure/backend/organizationalunit/OrganizationalUnitServiceImpl.java index f106eec3bd..439b1d26fd 100644 --- a/uberfire-structure/uberfire-structure-backend/src/main/java/org/guvnor/structure/backend/organizationalunit/OrganizationalUnitServiceImpl.java +++ b/uberfire-structure/uberfire-structure-backend/src/main/java/org/guvnor/structure/backend/organizationalunit/OrganizationalUnitServiceImpl.java @@ -64,6 +64,8 @@ import org.uberfire.spaces.Space; import org.uberfire.spaces.SpacesAPI; +import static org.guvnor.structure.backend.InputEscapeUtils.escapeContributorsNames; + @Service @ApplicationScoped public class OrganizationalUnitServiceImpl implements OrganizationalUnitService { @@ -320,7 +322,7 @@ public OrganizationalUnit createOrganizationalUnit(final String name, final SpaceInfo spaceInfo = new SpaceInfo(name, description, _defaultGroupId, - contributors, + escapeContributorsNames(contributors), getRepositoryAliases(repositories), Collections.emptyList()); spaceConfigStorageRegistry.get(name).saveSpaceInfo(spaceInfo); @@ -378,7 +380,7 @@ public OrganizationalUnit updateOrganizationalUnit(String name, spaceInfo.setDefaultGroupId(_defaultGroupId); if (contributors != null) { - spaceInfo.setContributors(contributors); + spaceInfo.setContributors(escapeContributorsNames(contributors)); } if (description != null) { diff --git a/uberfire-structure/uberfire-structure-backend/src/main/java/org/guvnor/structure/backend/repositories/RepositoryServiceImpl.java b/uberfire-structure/uberfire-structure-backend/src/main/java/org/guvnor/structure/backend/repositories/RepositoryServiceImpl.java index 9657010df3..ae2c5c506f 100644 --- a/uberfire-structure/uberfire-structure-backend/src/main/java/org/guvnor/structure/backend/repositories/RepositoryServiceImpl.java +++ b/uberfire-structure/uberfire-structure-backend/src/main/java/org/guvnor/structure/backend/repositories/RepositoryServiceImpl.java @@ -67,6 +67,7 @@ import org.uberfire.spaces.Space; import org.uberfire.spaces.SpacesAPI; +import static org.guvnor.structure.backend.InputEscapeUtils.escapeContributorsNames; import static org.guvnor.structure.repositories.EnvironmentParameters.CRYPT_PREFIX; import static org.guvnor.structure.repositories.EnvironmentParameters.SECURE_PREFIX; import static org.guvnor.structure.repositories.EnvironmentParameters.SCHEME; @@ -539,7 +540,7 @@ public void updateContributors(final Repository repository, thisRepositoryConfig.ifPresent(config -> { config.getConfiguration().add("contributors", - contributors); + escapeContributorsNames(contributors)); this.saveRepositoryConfig(repository.getSpace().getName(), config); repositoryContributorsUpdatedEvent.fire(new RepositoryContributorsUpdatedEvent(getRepositoryFromSpace(repository.getSpace(), diff --git a/uberfire-structure/uberfire-structure-backend/src/test/java/org/guvnor/structure/backend/InputEscapeUtilsTest.java b/uberfire-structure/uberfire-structure-backend/src/test/java/org/guvnor/structure/backend/InputEscapeUtilsTest.java new file mode 100644 index 0000000000..5f428c8878 --- /dev/null +++ b/uberfire-structure/uberfire-structure-backend/src/test/java/org/guvnor/structure/backend/InputEscapeUtilsTest.java @@ -0,0 +1,33 @@ +package org.guvnor.structure.backend; + +import org.apache.commons.text.StringEscapeUtils; +import org.assertj.core.api.Assertions; +import org.junit.Test; + +import static org.guvnor.structure.backend.InputEscapeUtils.escapeHtmlInput; + +public class InputEscapeUtilsTest { + + @Test + public void testEscapeHtmlInput() { + final String xssString = ""; + final String expectedResult = StringEscapeUtils.escapeHtml4(xssString).replace("'", ""); + final String result = escapeHtmlInput(xssString); + Assertions.assertThat(result).isEqualTo(expectedResult); + } + + @Test + public void testEscapeHtmlInputWithSingleQoutes() { + final String xssString = ""; + final String expectedResult = StringEscapeUtils.escapeHtml4(xssString).replace("'", ""); + final String result = escapeHtmlInput(xssString); + Assertions.assertThat(result).isEqualTo(expectedResult); + } + + @Test + public void testEscapeHtmlInputNull() { + final String xssString = null; + final String result = escapeHtmlInput(xssString); + Assertions.assertThat(result).isNull(); + } +} diff --git a/uberfire-structure/uberfire-structure-backend/src/test/java/org/guvnor/structure/backend/organizationalunit/OrganizationalUnitServiceTest.java b/uberfire-structure/uberfire-structure-backend/src/test/java/org/guvnor/structure/backend/organizationalunit/OrganizationalUnitServiceTest.java index ad4f813252..54fd6bda48 100644 --- a/uberfire-structure/uberfire-structure-backend/src/test/java/org/guvnor/structure/backend/organizationalunit/OrganizationalUnitServiceTest.java +++ b/uberfire-structure/uberfire-structure-backend/src/test/java/org/guvnor/structure/backend/organizationalunit/OrganizationalUnitServiceTest.java @@ -63,6 +63,7 @@ import org.uberfire.spaces.Space; import org.uberfire.spaces.SpacesAPI; +import static org.guvnor.structure.backend.InputEscapeUtils.escapeHtmlInput; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; @@ -268,6 +269,78 @@ public void createValidOrganizationalUnitTest() { assertEquals(SPACE_DESCRIPTION, ou.getDescription()); assertEquals(DEFAULT_GROUP_ID, ou.getDefaultGroupId()); assertEquals(contributors, ou.getContributors()); + Assertions.assertThat(ou.getContributors()).hasSize(1); + Assertions.assertThat(ou.getContributors()).hasOnlyOneElementSatisfying((contributor) -> { + contributor.getUsername().equals(escapeHtmlInput(ADMIN)); + }); + } + + @Test + public void createOrganizationalUnitWithPersistentXssInContributorTest() { + final String persistentXssContributor = ""; + final String escapedPersistentXssContributor = escapeHtmlInput(persistentXssContributor); + + List contributors = new ArrayList<>(); + contributors.add(new Contributor(persistentXssContributor, + ContributorType.ADMIN)); + + setOUCreationPermission(true); + + final OrganizationalUnit ou = organizationalUnitService.createOrganizationalUnit(SPACE_NAME, + DEFAULT_GROUP_ID, + new ArrayList<>(), + contributors, + SPACE_DESCRIPTION); + + assertNotNull(ou); + verify(organizationalUnitFactory).newOrganizationalUnit(any()); + assertEquals(SPACE_NAME, ou.getName()); + assertEquals(SPACE_DESCRIPTION, ou.getDescription()); + assertEquals(DEFAULT_GROUP_ID, ou.getDefaultGroupId()); + + Assertions.assertThat(ou.getContributors()).hasSize(1); + Assertions.assertThat(ou.getContributors()).hasOnlyOneElementSatisfying((contributor) -> { + contributor.getUsername().equals(escapedPersistentXssContributor); + }); + } + + @Test + public void createOrganizationalUnitWithPersistentXssAndValidContributorTest() { + final String persistentXssContributor = ""; + final String escapedPersistentXssContributor = escapeHtmlInput(persistentXssContributor); + final String escapedAdminContributor = escapeHtmlInput(ADMIN); + final String regularContributor = "head_technician_junior-intern"; + final String escapedRegularContributor = escapeHtmlInput(regularContributor); + + List contributors = new ArrayList<>(); + contributors.add(new Contributor(persistentXssContributor, + ContributorType.CONTRIBUTOR)); + contributors.add(new Contributor(ADMIN, + ContributorType.ADMIN)); + contributors.add(new Contributor(regularContributor, + ContributorType.OWNER)); + + setOUCreationPermission(true); + + final OrganizationalUnit ou = organizationalUnitService.createOrganizationalUnit(SPACE_NAME, + DEFAULT_GROUP_ID, + new ArrayList<>(), + contributors, + SPACE_DESCRIPTION); + + assertNotNull(ou); + verify(organizationalUnitFactory).newOrganizationalUnit(any()); + assertEquals(SPACE_NAME, ou.getName()); + assertEquals(SPACE_DESCRIPTION, ou.getDescription()); + assertEquals(DEFAULT_GROUP_ID, ou.getDefaultGroupId()); + + Assertions.assertThat(ou.getContributors()).hasSize(3); + Assertions.assertThat(ou.getContributors()).containsExactly(new Contributor(escapedPersistentXssContributor, + ContributorType.CONTRIBUTOR), + new Contributor(escapedAdminContributor, + ContributorType.ADMIN), + new Contributor(escapedRegularContributor, + ContributorType.OWNER)); } @Test @@ -326,6 +399,40 @@ public void testUpdateOrganizationalUnit() { verify(spaceConfigStorage).endBatch(); } + @Test + public void testContributorsPersistentXssOnUpdateOrganizationalUnit() { + final String persistentXssContributor = ""; + final String escapedPersistentXssContributor = escapeHtmlInput(persistentXssContributor); + + OrganizationalUnit organizationalUnit = + organizationalUnitService.updateOrganizationalUnit(SPACE_NAME, + DEFAULT_GROUP_ID, + Collections.singletonList( + new Contributor( + persistentXssContributor, + ContributorType.ADMIN + ) + ) + ); + + Assertions.assertThat(organizationalUnit) + .hasFieldOrPropertyWithValue("name", SPACE_NAME) + .hasFieldOrPropertyWithValue("defaultGroupId", DEFAULT_GROUP_ID); + + Assertions.assertThat(organizationalUnit.getContributors()).hasSize(1); + Assertions.assertThat(organizationalUnit.getContributors()).hasOnlyOneElementSatisfying((contributor) -> { + contributor.getUsername().equals(escapedPersistentXssContributor); + }); + + Assertions.assertThat(spaceInfo) + .hasFieldOrPropertyWithValue("name", SPACE_NAME) + .hasFieldOrPropertyWithValue("defaultGroupId", DEFAULT_GROUP_ID); + + verify(spaceConfigStorage).startBatch(); + verify(spaceConfigStorage).saveSpaceInfo(eq(spaceInfo)); + verify(spaceConfigStorage).endBatch(); + } + @Test public void testCheckChildrenRepositoryContributors() { OrganizationalUnit organizationalUnit = new OrganizationalUnitImpl(SPACE_NAME, DEFAULT_GROUP_ID); diff --git a/uberfire-structure/uberfire-structure-backend/src/test/java/org/guvnor/structure/backend/repositories/RepositoryServiceImplTest.java b/uberfire-structure/uberfire-structure-backend/src/test/java/org/guvnor/structure/backend/repositories/RepositoryServiceImplTest.java index 3eef397a94..b85e4fa331 100644 --- a/uberfire-structure/uberfire-structure-backend/src/test/java/org/guvnor/structure/backend/repositories/RepositoryServiceImplTest.java +++ b/uberfire-structure/uberfire-structure-backend/src/test/java/org/guvnor/structure/backend/repositories/RepositoryServiceImplTest.java @@ -39,6 +39,7 @@ import org.uberfire.spaces.Space; import org.uberfire.spaces.SpacesAPI; +import static org.guvnor.structure.backend.InputEscapeUtils.escapeHtmlInput; import static org.junit.Assert.*; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; @@ -171,6 +172,53 @@ public void updateContributorsTest() { verify(spaceConfigStorage).endBatch(); } + @Test + public void updateContributorsWithXSSNameTest() { + + final Space space = new Space("space"); + doReturn(space).when(repository).getSpace(); + doReturn("alias").when(repository).getAlias(); + + doReturn(repository).when(configuredRepositories).getRepositoryByRepositoryAlias(any(), + any()); + + final SpaceConfigStorage spaceConfigStorage = mock(SpaceConfigStorage.class); + doReturn(new SpaceInfo("space", + "Test space", + "defaultGroupId", + Collections.emptyList(), + new ArrayList<>(Arrays.asList(new RepositoryInfo("alias", + false, + new RepositoryConfiguration()))), + Collections.emptyList())).when(spaceConfigStorage).loadSpaceInfo(); + + when(registry.get(anyString())).thenReturn(spaceConfigStorage); + when(registry.getBatch(anyString())).thenReturn(new SpaceConfigStorageRegistryImpl.SpaceStorageBatchImpl(spaceConfigStorage)); + + final String xssName = ""; + final String escapedXssName = escapeHtmlInput(xssName); + repositoryService.updateContributors(repository, + Collections.singletonList(new Contributor(xssName, + ContributorType.OWNER))); + + verify(updatedEvent).fire(captor.capture()); + assertEquals("alias", + captor.getValue().getRepository().getAlias()); + assertEquals("space", + captor.getValue().getRepository().getSpace().getName()); + verify(repositoryService).saveRepositoryConfig(eq("space"), + configCaptor.capture()); + + assertEquals(escapedXssName, + configCaptor.getValue().getContributors().get(0).getUsername()); + assertEquals(ContributorType.OWNER, + configCaptor.getValue().getContributors().get(0).getType()); + + verify(spaceConfigStorage).startBatch(); + verify(spaceConfigStorage).saveSpaceInfo(any()); + verify(spaceConfigStorage).endBatch(); + } + @Test public void testDoRemoveInOrder() {