From c9d5d5878cc84484ec266ac4905aca570d8e5600 Mon Sep 17 00:00:00 2001 From: Pierre Date: Thu, 15 Jun 2023 11:27:36 +0200 Subject: [PATCH] NO-JIRA improve log to not be sensitive to line return from user input --- server/sonar-server-common/build.gradle | 1 + .../email/EmailNotificationChannel.java | 15 +++++++++--- .../email/EmailNotificationChannelTest.java | 18 +++++++++++++++ .../event/AuthenticationEventImpl.java | 23 ++++++++++++------- .../event/AuthenticationEventImplTest.java | 12 +++++++++- 5 files changed, 57 insertions(+), 12 deletions(-) rename server/{sonar-webserver-core => sonar-server-common}/src/test/java/org/sonar/server/notification/email/EmailNotificationChannelTest.java (95%) diff --git a/server/sonar-server-common/build.gradle b/server/sonar-server-common/build.gradle index 3b872c8cd3f..67dec37f8cc 100644 --- a/server/sonar-server-common/build.gradle +++ b/server/sonar-server-common/build.gradle @@ -29,6 +29,7 @@ dependencies { testImplementation 'org.elasticsearch.plugin:transport-netty4-client' testImplementation 'ch.qos.logback:logback-core' testImplementation 'com.google.code.findbugs:jsr305' + testImplementation 'org.subethamail:subethasmtp' testImplementation 'com.squareup.okhttp3:mockwebserver' testImplementation 'com.squareup.okio:okio' testImplementation 'com.tngtech.java:junit-dataprovider' diff --git a/server/sonar-server-common/src/main/java/org/sonar/server/notification/email/EmailNotificationChannel.java b/server/sonar-server-common/src/main/java/org/sonar/server/notification/email/EmailNotificationChannel.java index 4181eada966..3c0cf7981b9 100644 --- a/server/sonar-server-common/src/main/java/org/sonar/server/notification/email/EmailNotificationChannel.java +++ b/server/sonar-server-common/src/main/java/org/sonar/server/notification/email/EmailNotificationChannel.java @@ -23,19 +23,20 @@ import java.net.MalformedURLException; import java.net.URL; import java.util.Objects; import java.util.Set; +import java.util.regex.Pattern; import javax.annotation.CheckForNull; import org.apache.commons.lang.StringUtils; import org.apache.commons.mail.Email; import org.apache.commons.mail.EmailException; import org.apache.commons.mail.HtmlEmail; import org.apache.commons.mail.SimpleEmail; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.sonar.api.config.EmailSettings; import org.sonar.api.notifications.Notification; import org.sonar.api.notifications.NotificationChannel; import org.sonar.api.user.User; import org.sonar.api.utils.SonarException; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import org.sonar.db.DbClient; import org.sonar.db.DbSession; import org.sonar.db.user.UserDto; @@ -64,6 +65,8 @@ public class EmailNotificationChannel extends NotificationChannel { */ private static final int SOCKET_TIMEOUT = 30_000; + private static final Pattern PATTERN_LINE_BREAK = Pattern.compile("[\n\r]"); + /** * Email Header Field: "List-ID". * Value of this field should contain mailing list identifier as specified in RFC 2919. @@ -211,7 +214,9 @@ public class EmailNotificationChannel extends NotificationChannel { Thread.currentThread().setContextClassLoader(getClass().getClassLoader()); try { - LOG.trace("Sending email: {}", emailMessage); + LOG.atTrace().setMessage("Sending email: {}") + .addArgument(() -> sanitizeLog(emailMessage.getMessage())) + .log(); String host = resolveHost(); Email email = createEmailWithMessage(emailMessage); @@ -226,6 +231,10 @@ public class EmailNotificationChannel extends NotificationChannel { } } + private static String sanitizeLog(String message) { + return PATTERN_LINE_BREAK.matcher(message).replaceAll("_"); + } + private static Email createEmailWithMessage(EmailMessage emailMessage) throws EmailException { if (emailMessage.isHtml()) { return new HtmlEmail().setHtmlMsg(emailMessage.getMessage()); diff --git a/server/sonar-webserver-core/src/test/java/org/sonar/server/notification/email/EmailNotificationChannelTest.java b/server/sonar-server-common/src/test/java/org/sonar/server/notification/email/EmailNotificationChannelTest.java similarity index 95% rename from server/sonar-webserver-core/src/test/java/org/sonar/server/notification/email/EmailNotificationChannelTest.java rename to server/sonar-server-common/src/test/java/org/sonar/server/notification/email/EmailNotificationChannelTest.java index 74096770cbf..05a23e9df74 100644 --- a/server/sonar-webserver-core/src/test/java/org/sonar/server/notification/email/EmailNotificationChannelTest.java +++ b/server/sonar-server-common/src/test/java/org/sonar/server/notification/email/EmailNotificationChannelTest.java @@ -35,10 +35,14 @@ import javax.mail.internet.MimeMessage; import org.apache.commons.mail.EmailException; import org.junit.After; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; +import org.slf4j.event.Level; import org.sonar.api.config.EmailSettings; import org.sonar.api.notifications.Notification; +import org.sonar.api.testfixtures.log.LogTester; +import org.sonar.api.utils.log.LoggerLevel; import org.sonar.server.issue.notification.EmailMessage; import org.sonar.server.issue.notification.EmailTemplate; import org.sonar.server.notification.email.EmailNotificationChannel.EmailDeliveryRequest; @@ -61,6 +65,8 @@ public class EmailNotificationChannelTest { private static final String SUBJECT_PREFIX = "[SONARQUBE]"; + @Rule + public LogTester logTester = new LogTester(); private Wiser smtpServer; private EmailSettings configuration; @@ -68,6 +74,7 @@ public class EmailNotificationChannelTest { @Before public void setUp() { + logTester.setLevel(LoggerLevel.DEBUG); smtpServer = new Wiser(0); smtpServer.start(); @@ -117,6 +124,17 @@ public class EmailNotificationChannelTest { assertThat((String) email.getContent()).startsWith("This is a test message from SonarQube.\r\n\r\nMail sent from: http://nemo.sonarsource.org"); } + @Test + public void sendTestEmailShouldSanitizeLog() throws Exception { + logTester.setLevel(LoggerLevel.TRACE); + configure(); + underTest.sendTestEmail("user@nowhere", "Test Message from SonarQube", "This is a message \n containing line breaks \r that should be sanitized when logged."); + + assertThat(logTester.logs(Level.TRACE)).isNotEmpty() + .contains("Sending email: This is a message _ containing line breaks _ that should be sanitized when logged.__Mail sent from: http://nemo.sonarsource.org"); + + } + @Test public void shouldThrowAnExceptionWhenUnableToSendTestEmail() { configure(); diff --git a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/event/AuthenticationEventImpl.java b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/event/AuthenticationEventImpl.java index da58355b179..c40a164be2a 100644 --- a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/event/AuthenticationEventImpl.java +++ b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/event/AuthenticationEventImpl.java @@ -21,10 +21,11 @@ package org.sonar.server.authentication.event; import com.google.common.base.Joiner; import java.util.Collections; +import java.util.regex.Pattern; import javax.annotation.Nullable; -import org.sonar.api.server.http.HttpRequest; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.sonar.api.server.http.HttpRequest; import org.sonar.core.util.stream.MoreCollectors; import static java.util.Objects.requireNonNull; @@ -32,18 +33,20 @@ import static java.util.Objects.requireNonNull; public class AuthenticationEventImpl implements AuthenticationEvent { private static final Logger LOGGER = LoggerFactory.getLogger("auth.event"); private static final int FLOOD_THRESHOLD = 128; + private static final Pattern PATTERN_LINE_BREAK = Pattern.compile("[\n\r]"); @Override public void loginSuccess(HttpRequest request, @Nullable String login, Source source) { checkRequest(request); requireNonNull(source, "source can't be null"); - if (!LOGGER.isDebugEnabled()) { - return; - } - LOGGER.debug("login success [method|{}][provider|{}|{}][IP|{}|{}][login|{}]", - source.getMethod(), source.getProvider(), source.getProviderName(), - request.getRemoteAddr(), getAllIps(request), - preventLogFlood(emptyIfNull(login))); + LOGGER.atDebug().setMessage("login success [method|{}][provider|{}|{}][IP|{}|{}][login|{}]") + .addArgument(source::getMethod) + .addArgument(source::getProvider) + .addArgument(source::getProviderName) + .addArgument(request::getRemoteAddr) + .addArgument(() -> getAllIps(request)) + .addArgument(() -> preventLogFlood(sanitizeLog(emptyIfNull(login)))) + .log(); } private static String getAllIps(HttpRequest request) { @@ -103,4 +106,8 @@ public class AuthenticationEventImpl implements AuthenticationEvent { return str; } + private static String sanitizeLog(String message) { + return PATTERN_LINE_BREAK.matcher(message).replaceAll("_"); + } + } diff --git a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/event/AuthenticationEventImplTest.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/event/AuthenticationEventImplTest.java index b47c7d0f461..d607ccb720e 100644 --- a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/event/AuthenticationEventImplTest.java +++ b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/event/AuthenticationEventImplTest.java @@ -82,7 +82,17 @@ public class AuthenticationEventImplTest { underTest.loginSuccess(request, "login", Source.sso()); - verifyNoInteractions(request); + assertThat(logTester.logs()).isEmpty(); + } + + @Test + public void login_success_message_is_sanitized() { + logTester.setLevel(LoggerLevel.DEBUG); + + underTest.loginSuccess(mockRequest("1.2.3.4"), "login with \n malicious line \r return", Source.sso()); + + assertThat(logTester.logs()).isNotEmpty() + .contains("login success [method|SSO][provider|SSO|sso][IP|1.2.3.4|][login|login with _ malicious line _ return]"); } @Test -- 2.39.5