From 665bd0f6fbdbbb772087f11c5b599e51df5c2b64 Mon Sep 17 00:00:00 2001 From: taskain7 Date: Fri, 29 Apr 2022 14:34:59 +0200 Subject: [PATCH] FINERACT-1483: fix Sonar vulnerabilities with `Minor` severity --- .../service/ReadReportingServiceImpl.java | 43 ++++++++-------- .../ContentRepositoryFactory.java | 10 +--- .../S3ContentRepository.java | 21 ++++---- .../utils/LogParameterEscapeUtil.java | 28 ++++++++++ .../config/MessagingConfiguration.java | 5 +- .../useradministration/domain/AppUser.java | 15 +++--- .../utils/LogParameterEscapeUtilTest.java | 51 +++++++++++++++++++ .../infrastructure.security.utils.feature | 13 +++++ 8 files changed, 131 insertions(+), 55 deletions(-) create mode 100644 fineract-provider/src/main/java/org/apache/fineract/infrastructure/security/utils/LogParameterEscapeUtil.java create mode 100644 fineract-provider/src/test/java/org/apache/fineract/infrastructure/security/utils/LogParameterEscapeUtilTest.java create mode 100644 fineract-provider/src/test/resources/features/infrastructure/infrastructure.security.utils.feature diff --git a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java index bb6e8c398..4b518014e 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java +++ b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java @@ -37,6 +37,8 @@ import java.util.Locale; import java.util.Map; import java.util.Set; import javax.ws.rs.core.StreamingOutput; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; import org.apache.fineract.infrastructure.core.domain.JdbcSupport; import org.apache.fineract.infrastructure.core.exception.PlatformDataIntegrityException; import org.apache.fineract.infrastructure.dataqueries.data.GenericResultsetData; @@ -49,36 +51,25 @@ import org.apache.fineract.infrastructure.dataqueries.exception.ReportNotFoundEx import org.apache.fineract.infrastructure.documentmanagement.contentrepository.FileSystemContentRepository; import org.apache.fineract.infrastructure.security.service.PlatformSecurityContext; import org.apache.fineract.infrastructure.security.service.SqlInjectionPreventerService; +import org.apache.fineract.infrastructure.security.utils.LogParameterEscapeUtil; import org.apache.fineract.useradministration.domain.AppUser; import org.owasp.esapi.ESAPI; import org.owasp.esapi.codecs.UnixCodec; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; -import org.springframework.beans.factory.annotation.Autowired; import org.springframework.jdbc.core.JdbcTemplate; import org.springframework.jdbc.core.RowMapper; import org.springframework.jdbc.support.rowset.SqlRowSet; import org.springframework.stereotype.Service; @Service +@Slf4j +@RequiredArgsConstructor public class ReadReportingServiceImpl implements ReadReportingService { - private static final Logger LOG = LoggerFactory.getLogger(ReadReportingServiceImpl.class); - private final JdbcTemplate jdbcTemplate; private final PlatformSecurityContext context; private final GenericDataService genericDataService; private final SqlInjectionPreventerService sqlInjectionPreventerService; - @Autowired - public ReadReportingServiceImpl(final PlatformSecurityContext context, final JdbcTemplate jdbcTemplate, - final GenericDataService genericDataService, SqlInjectionPreventerService sqlInjectionPreventerService) { - this.context = context; - this.jdbcTemplate = jdbcTemplate; - this.genericDataService = genericDataService; - this.sqlInjectionPreventerService = sqlInjectionPreventerService; - } - @Override public StreamingOutput retrieveReportCSV(final String name, final String type, final Map queryParams, final boolean isSelfServiceUserReport) { @@ -110,7 +101,7 @@ public class ReadReportingServiceImpl implements ReadReportingService { final StringBuilder writer = new StringBuilder(); final List columnHeaders = result.getColumnHeaders(); - LOG.info("NO. of Columns: {}", columnHeaders.size()); + log.info("NO. of Columns: {}", columnHeaders.size()); final Integer chSize = columnHeaders.size(); for (int i = 0; i < chSize; i++) { writer.append('"' + columnHeaders.get(i).getColumnName() + '"'); @@ -128,7 +119,7 @@ public class ReadReportingServiceImpl implements ReadReportingService { String currVal; final String doubleQuote = "\""; final String twoDoubleQuotes = doubleQuote + doubleQuote; - LOG.info("NO. of Rows: {}", data.size()); + log.info("NO. of Rows: {}", data.size()); for (ResultsetRowData element : data) { row = element.getRow(); rSize = row.size(); @@ -160,14 +151,20 @@ public class ReadReportingServiceImpl implements ReadReportingService { final boolean isSelfServiceUserReport) { final long startTime = System.currentTimeMillis(); - LOG.info("STARTING REPORT: {} Type: {}", name, type); + if (log.isDebugEnabled()) { + log.debug("STARTING REPORT: {} Type: {}", LogParameterEscapeUtil.escapeLogParameter(name), + LogParameterEscapeUtil.escapeLogParameter(type)); + } final String sql = getSQLtoRun(name, type, queryParams, isSelfServiceUserReport); final GenericResultsetData result = this.genericDataService.fillGenericResultSet(sql); final long elapsed = System.currentTimeMillis() - startTime; - LOG.info("FINISHING Report/Request Name: {} - {} Elapsed Time: {}", name, type, elapsed); + if (log.isDebugEnabled()) { + log.debug("FINISHING Report/Request Name: {} - {} Elapsed Time: {}", LogParameterEscapeUtil.escapeLogParameter(name), + type.replaceAll("[\n\r\t]", "_"), elapsed); + } return result; } @@ -252,7 +249,7 @@ public class ReadReportingServiceImpl implements ReadReportingService { final List data = result.getData(); List row; - LOG.info("NO. of Columns: {}", columnHeaders.size()); + log.info("NO. of Columns: {}", columnHeaders.size()); final Integer chSize = columnHeaders.size(); final Document document = new Document(PageSize.B0.rotate()); @@ -274,7 +271,7 @@ public class ReadReportingServiceImpl implements ReadReportingService { Integer rSize; String currColType; String currVal; - LOG.info("NO. of Rows: {}", data.size()); + log.info("NO. of Rows: {}", data.size()); for (ResultsetRowData element : data) { row = element.getRow(); rSize = row.size(); @@ -297,7 +294,7 @@ public class ReadReportingServiceImpl implements ReadReportingService { document.close(); return genaratePdf; } catch (final Exception e) { - LOG.error("error.msg.reporting.error:", e); + log.error("error.msg.reporting.error:", e); throw new PlatformDataIntegrityException("error.msg.exception.error", e.getMessage(), e); } } @@ -482,14 +479,14 @@ public class ReadReportingServiceImpl implements ReadReportingService { @Override public GenericResultsetData retrieveGenericResultSetForSmsEmailCampaign(String name, String type, Map queryParams) { final long startTime = System.currentTimeMillis(); - LOG.info("STARTING REPORT: {} Type: {}", name, type); + log.info("STARTING REPORT: {} Type: {}", name, type); final String sql = sqlToRunForSmsEmailCampaign(name, type, queryParams); final GenericResultsetData result = this.genericDataService.fillGenericResultSet(sql); final long elapsed = System.currentTimeMillis() - startTime; - LOG.info("FINISHING Report/Request Name: {} - {} Elapsed Time: {}", name, type, elapsed); + log.info("FINISHING Report/Request Name: {} - {} Elapsed Time: {}", name, type, elapsed); return result; } diff --git a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/contentrepository/ContentRepositoryFactory.java b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/contentrepository/ContentRepositoryFactory.java index 4158fe015..bdfcb5c69 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/contentrepository/ContentRepositoryFactory.java +++ b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/contentrepository/ContentRepositoryFactory.java @@ -18,27 +18,21 @@ */ package org.apache.fineract.infrastructure.documentmanagement.contentrepository; +import lombok.RequiredArgsConstructor; import org.apache.fineract.infrastructure.configuration.data.S3CredentialsData; import org.apache.fineract.infrastructure.configuration.domain.ConfigurationDomainService; import org.apache.fineract.infrastructure.configuration.service.ExternalServicesPropertiesReadPlatformService; import org.apache.fineract.infrastructure.documentmanagement.domain.StorageType; -import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.ApplicationContext; import org.springframework.stereotype.Component; @Component +@RequiredArgsConstructor public class ContentRepositoryFactory { private final ApplicationContext applicationContext; private final ExternalServicesPropertiesReadPlatformService externalServicesReadPlatformService; - @Autowired - public ContentRepositoryFactory(final ApplicationContext applicationContext, - final ExternalServicesPropertiesReadPlatformService externalServicesReadPlatformService) { - this.applicationContext = applicationContext; - this.externalServicesReadPlatformService = externalServicesReadPlatformService; - } - public ContentRepository getRepository() { final ConfigurationDomainService configurationDomainServiceJpa = this.applicationContext.getBean("configurationDomainServiceJpa", ConfigurationDomainService.class); diff --git a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/contentrepository/S3ContentRepository.java b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/contentrepository/S3ContentRepository.java index da999a7ba..37fbf4ff4 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/contentrepository/S3ContentRepository.java +++ b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/contentrepository/S3ContentRepository.java @@ -35,6 +35,7 @@ import java.io.File; import java.io.IOException; import java.io.InputStream; import java.util.Base64; +import lombok.extern.slf4j.Slf4j; import org.apache.fineract.infrastructure.core.domain.Base64EncodedImage; import org.apache.fineract.infrastructure.documentmanagement.command.DocumentCommand; import org.apache.fineract.infrastructure.documentmanagement.data.DocumentData; @@ -43,13 +44,11 @@ import org.apache.fineract.infrastructure.documentmanagement.data.ImageData; import org.apache.fineract.infrastructure.documentmanagement.domain.StorageType; import org.apache.fineract.infrastructure.documentmanagement.exception.ContentManagementException; import org.apache.fineract.infrastructure.documentmanagement.exception.DocumentNotFoundException; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; +import org.apache.fineract.infrastructure.security.utils.LogParameterEscapeUtil; +@Slf4j public class S3ContentRepository implements ContentRepository { - private static final Logger LOG = LoggerFactory.getLogger(S3ContentRepository.class); - private final String s3BucketName; private final AmazonS3 s3Client; @@ -154,23 +153,21 @@ public class S3ContentRepository implements ContentRepository { private void putObject(final String filename, final InputStream inputStream, final String s3UploadLocation) throws ContentManagementException { try { - LOG.info("Uploading a new object to S3 {}", s3UploadLocation); + if (log.isDebugEnabled()) { + log.debug("Uploading a new object to S3 {}", LogParameterEscapeUtil.escapeLogParameter(s3UploadLocation)); + } this.s3Client.putObject(new PutObjectRequest(this.s3BucketName, s3UploadLocation, inputStream, new ObjectMetadata())); - } catch (AmazonServiceException ase) { + } catch (AmazonClientException ase) { throw new ContentManagementException(filename, ase.getMessage(), ase); - } catch (final AmazonClientException ace) { - throw new ContentManagementException(filename, ace.getMessage(), ace); } } private S3Object getObject(String key) { try { - LOG.info("Downloading an object from Amazon S3 Bucket: {}, location: {}", this.s3BucketName, key); + log.info("Downloading an object from Amazon S3 Bucket: {}, location: {}", this.s3BucketName, key); return this.s3Client.getObject(new GetObjectRequest(this.s3BucketName, key)); - } catch (AmazonServiceException ase) { + } catch (AmazonClientException ase) { throw new ContentManagementException(key, ase.getMessage(), ase); - } catch (final AmazonClientException ace) { - throw new ContentManagementException(key, ace.getMessage(), ace); } } } diff --git a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/security/utils/LogParameterEscapeUtil.java b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/security/utils/LogParameterEscapeUtil.java new file mode 100644 index 000000000..1e249f4f8 --- /dev/null +++ b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/security/utils/LogParameterEscapeUtil.java @@ -0,0 +1,28 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * 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.apache.fineract.infrastructure.security.utils; + +public final class LogParameterEscapeUtil { + + private LogParameterEscapeUtil() {} + + public static String escapeLogParameter(String logParameter) { + return logParameter.replaceAll("[\n\r\t]", "_"); + } +} diff --git a/fineract-provider/src/main/java/org/apache/fineract/notification/config/MessagingConfiguration.java b/fineract-provider/src/main/java/org/apache/fineract/notification/config/MessagingConfiguration.java index a8d2b4633..bf26aea5b 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/notification/config/MessagingConfiguration.java +++ b/fineract-provider/src/main/java/org/apache/fineract/notification/config/MessagingConfiguration.java @@ -52,7 +52,7 @@ public class MessagingConfiguration { @Bean public ActiveMQConnectionFactory amqConnectionFactory() { - ActiveMQConnectionFactory amqConnectionFactory = new ActiveMQConnectionFactory(); + ActiveMQConnectionFactory amqConnectionFactory = new ActiveMQConnectionFactory(); // NOSONAR try { amqConnectionFactory.setBrokerURL(DEFAULT_BROKER_URL); } catch (Exception e) { @@ -63,8 +63,7 @@ public class MessagingConfiguration { @Bean public CachingConnectionFactory connectionFactory() { - CachingConnectionFactory connectionFactory = new CachingConnectionFactory(amqConnectionFactory()); - return connectionFactory; + return new CachingConnectionFactory(amqConnectionFactory()); } @Bean diff --git a/fineract-provider/src/main/java/org/apache/fineract/useradministration/domain/AppUser.java b/fineract-provider/src/main/java/org/apache/fineract/useradministration/domain/AppUser.java index 623c11c22..9f076e2bd 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/useradministration/domain/AppUser.java +++ b/fineract-provider/src/main/java/org/apache/fineract/useradministration/domain/AppUser.java @@ -39,6 +39,7 @@ import javax.persistence.Table; import javax.persistence.Temporal; import javax.persistence.TemporalType; import javax.persistence.UniqueConstraint; +import lombok.extern.slf4j.Slf4j; import org.apache.commons.lang3.StringUtils; import org.apache.fineract.infrastructure.core.api.JsonCommand; import org.apache.fineract.infrastructure.core.data.EnumOptionData; @@ -48,22 +49,20 @@ import org.apache.fineract.infrastructure.security.domain.PlatformUser; import org.apache.fineract.infrastructure.security.exception.NoAuthorizationException; import org.apache.fineract.infrastructure.security.service.PlatformPasswordEncoder; import org.apache.fineract.infrastructure.security.service.RandomPasswordGenerator; +import org.apache.fineract.infrastructure.security.utils.LogParameterEscapeUtil; import org.apache.fineract.organisation.office.domain.Office; import org.apache.fineract.organisation.staff.domain.Staff; import org.apache.fineract.portfolio.client.domain.Client; import org.apache.fineract.useradministration.service.AppUserConstants; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.authority.SimpleGrantedAuthority; import org.springframework.security.core.userdetails.User; +@Slf4j @Entity @Table(name = "m_appuser", uniqueConstraints = @UniqueConstraint(columnNames = { "username" }, name = "username_org")) public class AppUser extends AbstractPersistableCustom implements PlatformUser { - private static final Logger LOG = LoggerFactory.getLogger(AppUser.class); - @Column(name = "email", nullable = false, length = 100) private String email; @@ -622,16 +621,14 @@ public class AppUser extends AbstractPersistableCustom implements PlatformUser { public void validateHasPermissionTo(final String function) { if (hasNotPermissionTo(function)) { final String authorizationMessage = "User has no authority to: " + function; - LOG.info("Unauthorized access: userId: {} action: {} allowed: {}", getId(), function, getAuthorities()); + log.info("Unauthorized access: userId: {} action: {} allowed: {}", getId(), LogParameterEscapeUtil.escapeLogParameter(function), + getAuthorities()); throw new NoAuthorizationException(authorizationMessage); } } public void validateHasReadPermission(final String function, final Long userId) { - if ("USER".equalsIgnoreCase(function) && userId.equals(getId())) { - // abstain from validation as user allowed fetch their own data no - // matter what permissions they have. - } else { + if (!("USER".equalsIgnoreCase(function) && userId.equals(getId()))) { validateHasReadPermission(function); } } diff --git a/fineract-provider/src/test/java/org/apache/fineract/infrastructure/security/utils/LogParameterEscapeUtilTest.java b/fineract-provider/src/test/java/org/apache/fineract/infrastructure/security/utils/LogParameterEscapeUtilTest.java new file mode 100644 index 000000000..00860a4ba --- /dev/null +++ b/fineract-provider/src/test/java/org/apache/fineract/infrastructure/security/utils/LogParameterEscapeUtilTest.java @@ -0,0 +1,51 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * 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.apache.fineract.infrastructure.security.utils; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import io.cucumber.java8.En; + +public class LogParameterEscapeUtilTest implements En { + + private String logParameter; + + private String escapedLogParameter; + + public LogParameterEscapeUtilTest() { + Given("A simple log message without any special character", () -> { + logParameter = "This is a very simple String without any special character."; + }); + Given("A log message with new line, carriage return and tab characters", () -> { + logParameter = "This String contains new line\n, carriage return\r and tab\t characters."; + }); + + When("Log parameter escape util escaping the special characters", () -> { + escapedLogParameter = LogParameterEscapeUtil.escapeLogParameter(logParameter); + }); + + Then("The log message stays as it is", () -> { + assertEquals(logParameter, escapedLogParameter); + }); + Then("The escape util changes the special characters to `_`", () -> { + assertEquals("This String contains new line_, carriage return_ and tab_ characters.", escapedLogParameter); + }); + } + +} diff --git a/fineract-provider/src/test/resources/features/infrastructure/infrastructure.security.utils.feature b/fineract-provider/src/test/resources/features/infrastructure/infrastructure.security.utils.feature new file mode 100644 index 000000000..c144e4ae9 --- /dev/null +++ b/fineract-provider/src/test/resources/features/infrastructure/infrastructure.security.utils.feature @@ -0,0 +1,13 @@ +Feature: Security Utils Infrastructure + + @security + Scenario: Verify that log parameter escape util does not change strings that does not contain any special character + Given A simple log message without any special character + When Log parameter escape util escaping the special characters + Then The log message stays as it is + + @security + Scenario: Verify that log parameter escape util changes special characters in string + Given A log message with new line, carriage return and tab characters + When Log parameter escape util escaping the special characters + Then The escape util changes the special characters to `_`