Skip to content

Commit

Permalink
RHPAM-4719: Persistent Cross-Site Scripting (XSS) (#1393)
Browse files Browse the repository at this point in the history
* RHPAM-4719: Persistent Cross-Site Scripting (XSS)

Fixes RHPAM-4716 & RHPAM-4717 by using StringEscapeUtils::
escapeHtml4() method in ProjectResource and by implementing
helper method, using escapeHtml4(), to escape conrtributors
names in OrganizationalUnitServiceImpl

* RHPAM-4719: Add unit test cases for XSS data

* RHPAM-4719: Replace single qoute with nothing

* RHPAM-4917: Expand escaping to RepositoryService

Refactors unit tests to use same methods as in main classes
Add some unit tests

* Fix code duplication

Moves methods for escaping out of services

* Increase coverage and remove code smells
  • Loading branch information
domhanak authored Jul 13, 2023
1 parent 783b3f3 commit a3396ef
Show file tree
Hide file tree
Showing 10 changed files with 306 additions and 6 deletions.
5 changes: 5 additions & 0 deletions uberfire-rest/uberfire-rest-backend/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,11 @@
<artifactId>slf4j-api</artifactId>
</dependency>

<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-text</artifactId>
</dependency>

<!-- REST dependencies -->
<dependency>
<groupId>jakarta.annotation</groupId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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);

Expand All @@ -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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,28 @@ public void updateSpace() throws Exception {
assertEquals(JobStatus.ACCEPTED, jobResultArgumentCaptor.getValue().getStatus());
}

@Test
public void updateSpaceWithXSSOwer() throws Exception {
String xssOwner = "<img/src/onerror=alert(\"XSS\")>";
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 = "<img/src/onerror=alert(document.cookie)>";
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 {

Expand All @@ -334,6 +356,19 @@ public void addBranch() {
assertEquals(JobStatus.ACCEPTED, jobResultArgumentCaptor.getValue().getStatus());
}

@Test
public void addBranchWithXSSName() {
AddBranchRequest addBranchRequest = new AddBranchRequest();
addBranchRequest.setNewBranchName("<img/src/onerror=alert(\"Xss\")>");

projectResource.addBranch("spaceName",
"projectName",
addBranchRequest);

verify(jobManager).putJob(jobResultArgumentCaptor.capture());
assertEquals(JobStatus.ACCEPTED, jobResultArgumentCaptor.getValue().getStatus());
}

@Test
public void removeBranch() {
projectResource.removeBranch("spaceName",
Expand Down
5 changes: 5 additions & 0 deletions uberfire-structure/uberfire-structure-backend/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,11 @@
<artifactId>commons-fileupload</artifactId>
</dependency>

<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-text</artifactId>
</dependency>

<dependency>
<groupId>org.uberfire</groupId>
<artifactId>uberfire-security-management-api</artifactId>
Expand Down
Original file line number Diff line number Diff line change
@@ -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<Contributor> escapeContributorsNames(Collection<Contributor> contributors) {
Collection<Contributor> 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;
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(),
Expand Down
Original file line number Diff line number Diff line change
@@ -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 = "<img/src/onerror=alert(\"XSS\")>";
final String expectedResult = StringEscapeUtils.escapeHtml4(xssString).replace("'", "");
final String result = escapeHtmlInput(xssString);
Assertions.assertThat(result).isEqualTo(expectedResult);
}

@Test
public void testEscapeHtmlInputWithSingleQoutes() {
final String xssString = "<img/src/onerror=alert('XSS')>";
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();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 = "<img/src/onerror=alert(\"XSS\")>";
final String escapedPersistentXssContributor = escapeHtmlInput(persistentXssContributor);

List<Contributor> 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 = "<img/src/onerror=alert(\"XSS\")>";
final String escapedPersistentXssContributor = escapeHtmlInput(persistentXssContributor);
final String escapedAdminContributor = escapeHtmlInput(ADMIN);
final String regularContributor = "head_technician_junior-intern";
final String escapedRegularContributor = escapeHtmlInput(regularContributor);

List<Contributor> 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
Expand Down Expand Up @@ -326,6 +399,40 @@ public void testUpdateOrganizationalUnit() {
verify(spaceConfigStorage).endBatch();
}

@Test
public void testContributorsPersistentXssOnUpdateOrganizationalUnit() {
final String persistentXssContributor = "<img/src/onerror=alert(\"XSS\")>";
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);
Expand Down
Loading

0 comments on commit a3396ef

Please sign in to comment.