Skip to content

Commit

Permalink
[VAS] 11330 : add sanity on html injection
Browse files Browse the repository at this point in the history
  • Loading branch information
oussamasic committed Apr 18, 2023
1 parent 4341c69 commit 6e0ea0b
Show file tree
Hide file tree
Showing 14 changed files with 182 additions and 90 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import fr.gouv.vitam.common.exception.InvalidParseOperationException;
import fr.gouv.vitam.common.model.export.transfer.TransferRequestParameters;
import fr.gouv.vitamui.archives.search.common.dto.ArchiveUnitsDto;
import fr.gouv.vitamui.commons.api.dtos.OntologyDto;
import fr.gouv.vitamui.archives.search.common.dto.RuleSearchCriteriaDto;
import fr.gouv.vitamui.archives.search.common.dto.TransferRequestDto;
import fr.gouv.vitamui.archives.search.common.rest.RestApi;
Expand All @@ -40,9 +39,9 @@
import fr.gouv.vitamui.commons.api.domain.IdDto;
import fr.gouv.vitamui.commons.api.domain.ServicesData;
import fr.gouv.vitamui.commons.api.dtos.CriteriaValue;
import fr.gouv.vitamui.commons.api.dtos.OntologyDto;
import fr.gouv.vitamui.commons.api.dtos.SearchCriteriaDto;
import fr.gouv.vitamui.commons.api.dtos.SearchCriteriaEltDto;
import fr.gouv.vitamui.commons.api.exception.InvalidSanitizeCriteriaException;
import fr.gouv.vitamui.commons.api.exception.PreconditionFailedException;
import fr.gouv.vitamui.commons.api.logger.VitamUILogger;
import fr.gouv.vitamui.commons.api.logger.VitamUILoggerFactory;
Expand Down Expand Up @@ -76,7 +75,7 @@

@ExtendWith(MockitoExtension.class)
@WebMvcTest(controllers = {ArchivesSearchExternalController.class})
public class ArchivesSearchExternalControllerTest extends ApiArchiveSearchExternalControllerTest<IdDto> {
class ArchivesSearchExternalControllerTest extends ApiArchiveSearchExternalControllerTest<IdDto> {

private static final VitamUILogger LOGGER =
VitamUILoggerFactory.getInstance(ArchivesSearchExternalControllerTest.class);
Expand Down Expand Up @@ -143,7 +142,7 @@ protected String getRessourcePrefix() {


@Test
void when_searchArchiveUnitsByCriteria_Srvc_ok_should_return_ok() throws InvalidParseOperationException,
void test_searchArchiveUnitsByCriteria_with_ok_criteria_should_return_ok() throws InvalidParseOperationException,
PreconditionFailedException {

SearchCriteriaDto query = new SearchCriteriaDto();
Expand All @@ -156,7 +155,7 @@ void when_searchArchiveUnitsByCriteria_Srvc_ok_should_return_ok() throws Invalid
}

@Test
void when_searchArchiveUnitsByCriteria_Srvc_ok_should_return_ko() {
void test_searchArchiveUnitsByCriteria_with_invalid_criteria_should_return_ko() {

SearchCriteriaDto query = new SearchCriteriaDto();
SearchCriteriaEltDto nodeCriteria = new SearchCriteriaEltDto();
Expand All @@ -171,7 +170,8 @@ void when_searchArchiveUnitsByCriteria_Srvc_ok_should_return_ko() {
.thenReturn(expectedResponse);

assertThatCode(() -> archivesSearchExternalController.searchArchiveUnitsByCriteria(query))
.isInstanceOf(InvalidSanitizeCriteriaException.class);
.isInstanceOf(PreconditionFailedException.class)
.hasMessage("The object is not valid ");
}

@Test
Expand All @@ -191,7 +191,7 @@ void testSearchFilingHoldingSchemeResultsThanReturnVitamUISearchResponseDto() {


@Test
void when_exportCsvArchiveUnitsByCriteria_Srvc_ok_should_return_ok()
void test_exportCsvArchiveUnitsByCriteria_with_valid_criteria_should_return_ok()
throws InvalidParseOperationException, PreconditionFailedException, IOException {
// Given
SearchCriteriaDto query = new SearchCriteriaDto();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
import fr.gouv.vitamui.commons.api.dtos.OntologyDto;
import fr.gouv.vitamui.commons.api.dtos.SearchCriteriaDto;
import fr.gouv.vitamui.commons.api.dtos.SearchCriteriaEltDto;
import fr.gouv.vitamui.commons.api.exception.InvalidSanitizeCriteriaException;
import fr.gouv.vitamui.commons.api.exception.PreconditionFailedException;
import fr.gouv.vitamui.commons.api.logger.VitamUILogger;
import fr.gouv.vitamui.commons.api.logger.VitamUILoggerFactory;
Expand All @@ -61,7 +60,7 @@

@ExtendWith(MockitoExtension.class)
@WebMvcTest(controllers = {TransactionArchiveUnitExternalController.class})
public class ProjectArchiveUnitExternalControllerTest extends ApiCollectExternalControllerTest<IdDto> {
class ProjectArchiveUnitExternalControllerTest extends ApiCollectExternalControllerTest<IdDto> {

private static final VitamUILogger LOGGER =
VitamUILoggerFactory.getInstance(ProjectArchiveUnitExternalControllerTest.class);
Expand Down Expand Up @@ -109,7 +108,7 @@ protected String getRessourcePrefix() {
}

@Test
void when_searchCollectUnitsByCriteria_Service_ko_should_return_ko() {
void test_searchCollectUnitsByCriteria_with_invalid_criteria_should_return_ko() {

SearchCriteriaDto query = new SearchCriteriaDto();
SearchCriteriaEltDto nodeCriteria = new SearchCriteriaEltDto();
Expand All @@ -124,11 +123,12 @@ void when_searchCollectUnitsByCriteria_Service_ko_should_return_ko() {
.thenReturn(expectedResponse);

assertThatCode(() -> transactionArchiveUnitExternalController.searchArchiveUnits("projectId", query))
.isInstanceOf(InvalidSanitizeCriteriaException.class);
.isInstanceOf(PreconditionFailedException.class)
.hasMessage("The object is not valid ");
}

@Test
void when_searchArchiveUnitsByCriteria_Srvc_ok_should_return_ok() throws InvalidParseOperationException,
void test_searchArchiveUnitsByCriteria_with_valid_criteria_should_return_ok() throws InvalidParseOperationException,
PreconditionFailedException {

SearchCriteriaDto query = new SearchCriteriaDto();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,33 @@
/*
* Copyright French Prime minister Office/SGMAP/DINSIC/Vitam Program (2015-2022)
*
* contact.vitam@culture.gouv.fr
*
* This software is a computer program whose purpose is to implement a digital archiving back-office system managing
* high volumetry securely and efficiently.
*
* This software is governed by the CeCILL 2.1 license under French law and abiding by the rules of distribution of free
* software. You can use, modify and/ or redistribute the software under the terms of the CeCILL 2.1 license as
* circulated by CEA, CNRS and INRIA at the following URL "https://cecill.info".
*
* As a counterpart to the access to the source code and rights to copy, modify and redistribute granted by the license,
* users are provided only with a limited warranty and the software's author, the holder of the economic rights, and the
* successive licensors have only limited liability.
*
* In this respect, the user's attention is drawn to the risks associated with loading, using, modifying and/or
* developing or reproducing the software by the user in light of its specific status of free software, that may mean
* that it is complicated to manipulate, and that also therefore means that it is reserved for developers and
* experienced professionals having in-depth computer knowledge. Users are therefore encouraged to load and test the
* software's suitability as regards their requirements in conditions enabling the security of their systems and/or data
* to be ensured and, more generally, to use and operate it in the same conditions as regards security.
*
* The fact that you are presently reading this means that you have had knowledge of the CeCILL 2.1 license and that you
* accept its terms.
*
*
*/


package fr.gouv.vitamui.iam.external.server.rest;

import com.google.common.collect.ImmutableMap;
Expand Down Expand Up @@ -63,7 +93,7 @@ public void testCheckExistByName() {
public void testCheckExistByBadCriteriaScriptThenReturnBadRequest() {
Mockito.when(tenantExternalService.checkExists(any(String.class))).thenReturn(true);
final QueryDto criteria = QueryDto.criteria().addCriterion("name", "tenantName<s></s>", CriterionOperator.EQUALS);
super.performHead(CommonConstants.PATH_CHECK, ImmutableMap.of("criteria", criteria.toJson()), status().isBadRequest());
super.performHead(CommonConstants.PATH_CHECK, ImmutableMap.of("criteria", criteria.toJson()), status().isPreconditionFailed());
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ public static void checkSafeFilePath(String path) {
* using File.separator FileSystem String
* @throws IOException thrown when any check fails with UnChecked or Runtime exception
*/
public static void checkSafeFilePath(String rootPath, String... subPaths) {
public static void checkSafeFilePath(String rootPath, String... subPaths) throws InvalidFileSanitizeException {
try {
checkNullParameter(rootPath);
String finalPath = rootPath;
Expand All @@ -86,7 +86,7 @@ public static void checkSafeFilePath(String rootPath, String... subPaths) {
}
checkSafeFilePath(finalPath);
} catch (Exception e) {
throw e;
throw new InvalidFileSanitizeException(e.getMessage());
}
}

Expand Down Expand Up @@ -145,7 +145,7 @@ private static void doFilenameCheck(String pathName) {
if(!pathName.matches(FILENAME_PATTERN)) {
LOGGER.error("Invalid pathName {} ", pathName);
throw new InvalidFileSanitizeException(String
.format("Invalid filename (%s) (has unauthorized characters in part %s", pathName));
.format("Invalid filename (%s) : has unauthorized characters", pathName));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,6 @@
*/
public class SanityChecker {

private static final String INVALID_IDENTIFIER_SANITIZE = "Sanitizing failed; Invalid input identifier : ";
private static final String INVALID_HEADER_SANITIZE = "Sanitizing failed; Invalid request header : ";
private static final String INVALID_CRITERIA = "Criteria failed when sanitizing, it may contains insecure data : ";
private static final String JSON_IS_NOT_VALID_FROM_SANITIZE_CHECK = "Json is not valid from Sanitize check";
private static final int DEFAULT_LIMIT_PARAMETER_SIZE = 5000;
Expand All @@ -63,11 +61,7 @@ public class SanityChecker {
private static final long DEFAULT_LIMIT_FILE_SIZE = 8000000000L;

public static final String HTTP_PARAMETER_VALUE = "HTTPParameterValue";
private static final String HTTP_PARAMETER_NAME = "HTTPParameterName";
private static final String HTTP_HEADER_NAME = "HTTPHeaderName";
private static final String HTTP_HEADER_VALUE = "HTTPHeaderValue";

// TODO : verify the difference between this defined limit and the previous ones
private static final int REQUEST_LIMIT = 10000;

/**
Expand Down Expand Up @@ -108,14 +102,10 @@ public static boolean isValidParameter(String value) {
return !isStringInfected(value, HTTP_PARAMETER_VALUE) || PARAMETERS_KEYS_OF_DSL_QUERY_WHITELIST.contains(value);
}

public static boolean isValidParameterName(String value) {
return !isStringInfected(value, HTTP_PARAMETER_NAME);
}

/**
* Sanitize the fileName
*
* @param fileName
* @param fileName : name of a file
* @return true/false
*/
public static void isValidFileName(String fileName) throws PreconditionFailedException {
Expand Down Expand Up @@ -170,7 +160,7 @@ public static void checkJsonAll(String json) throws InvalidParseOperationExcepti
* checkSecureParameter : Check sanity of String: no javascript/xml tag, neither html tag
* check if the string is not infected or contains illegal characters
*
* @param params
* @param params : sequence of strings de check
* @throws PreconditionFailedException
* @throws InvalidParseOperationException
*/
Expand Down Expand Up @@ -227,11 +217,11 @@ private static void checkSecureParam(String param)
try {
checkSanityTags(param, getLimitParamSize());
checkHtmlPattern(param);
} catch (InvalidParseOperationException exception) {
} catch (InvalidParseOperationException | PreconditionFailedException exception) {
throw new InvalidParseOperationException("Error with the parameter ", exception);
}
} else {
throw new PreconditionFailedException("the parameter is not valid");
throw new PreconditionFailedException("the parameter " + param + " is not valid");
}
}

Expand Down Expand Up @@ -299,11 +289,11 @@ private static void checkSanityTags(String dataLine, String invalidTag)
* checkHtmlPattern : check against Html Pattern within value (not allowed)
*
* @param param
* @throws InvalidParseOperationException when Sanity Check is in error
* @throws PreconditionFailedException when Sanity Check is in error
*/
private static void checkHtmlPattern(String param) throws InvalidParseOperationException {
public static void checkHtmlPattern(String param) throws PreconditionFailedException {
if (StringUtils.HTML_PATTERN.matcher(param).find()) {
throw new InvalidParseOperationException("HTML PATTERN found");
throw new PreconditionFailedException("the parameter : " + param + " is not valid");
}
}

Expand Down Expand Up @@ -334,7 +324,7 @@ public static void checkJsonSanity(JsonNode json) throws InvalidParseOperationEx
while (fields.hasNext()) {
final Map.Entry<String, JsonNode> entry = fields.next();
final String key = entry.getKey();
if (isValidParameterName(key) || PARAMETERS_KEYS_OF_DSL_QUERY_WHITELIST.contains(key)) {
if (isValidParameter(key)) {
checkSanityTags(key, getLimitFieldSize());
} else {
throw new PreconditionFailedException("Invalid JSON key: " + key);
Expand All @@ -352,7 +342,11 @@ public static void checkJsonSanity(JsonNode json) throws InvalidParseOperationEx
}
} else if (!value.isValueNode()) {
checkJsonSanity(value);
} else {
} else if(value.isTextual()) {
checkHtmlPattern(value.textValue());
checkSanityTags(value.textValue(), getLimitParamSize());
}
else {
validateJSONField(value);
}
}
Expand All @@ -378,20 +372,6 @@ private static void checkJsonFileSize(String json) throws InvalidParseOperationE
}
}

/*
* @return the limit File Size (XML or JSON)
*/
public static long getLimitFileSize() {
return limitFileSize;
}

/**
* @param limitFileSize the limit File Size to set (XML or JSON)
*/
public static void setLimitFileSize(long limitFileSize) {
SanityChecker.limitFileSize = limitFileSize;
}

/**
* @return the limit Size of a Json
*/
Expand All @@ -413,13 +393,6 @@ public static int getLimitFieldSize() {
return limitFieldSize;
}

/**
* @param limitFieldSize the limit Size of a Field in a Json to set
*/
public static void setLimitFieldSize(int limitFieldSize) {
SanityChecker.limitFieldSize = limitFieldSize;
}

/**
* @return the limit Size of a parameter
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,12 @@
public class SafeFileCheckerTest {
private final String VALID_ROOT_PATH = "mydirectory";
private final String INVALID_ROOT_PATH = "my|filena?me?query=<code>danger</code>.json";
private final String VALID_SUBPATH = "good_json_sanity";
private final String VALID_SUBPATH = "good_json_sanity.json";
private final String INVALID_PATH = "file#..$$/text";

private static List<String> validPaths = new ArrayList<>();
private static List<String> invalidPaths = new ArrayList<>();
private static List<String> validFilenames = new ArrayList<>();
private static final List<String> validPaths = new ArrayList<>();
private static final List<String> invalidPaths = new ArrayList<>();
private static final List<String> validFilenames = new ArrayList<>();

@BeforeClass
public static void setUpBeforeClass() {
Expand Down
Loading

0 comments on commit 6e0ea0b

Please sign in to comment.