From c5910f02e6e49be67ded2cc1f632e6d01c7a8994 Mon Sep 17 00:00:00 2001 From: Klaus Kiefer Date: Tue, 5 Mar 2024 11:45:17 +0100 Subject: [PATCH 01/10] add methods for sanitized logging --- .../uaa/logging/SanitizedLogFactory.java | 36 +++++++++++++++---- 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java index 81956f292a0..71564682b1f 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java @@ -1,4 +1,4 @@ -package org.cloudfoundry.identity.uaa.logging; +package org.coundfoundry.identity.uaa.logging; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -16,6 +16,10 @@ public static class SanitizedLog { private Logger fallback; public SanitizedLog(Logger logger) { + setFallback(logger); + } + + public void setFallback(Logger logger) { this.fallback = logger; } @@ -24,19 +28,39 @@ public boolean isDebugEnabled() { } public void info(String message) { - fallback.info(LogSanitizerUtil.sanitize(message)); + fallback.info(sanitizeLog(message)); + } + + public void info(String message, Throwable t) { + fallback.info(sanitizeLog(message), t); } public void warn(String message) { - fallback.warn(LogSanitizerUtil.sanitize(message)); + fallback.warn(sanitizeLog(message)); + } + + public void warn(String message, Throwable t) { + fallback.warn(sanitizeLog(message), t); } public void debug(String message) { - fallback.debug(LogSanitizerUtil.sanitize(message)); + fallback.debug(sanitizeLog(message)); } public void debug(String message, Throwable t) { - fallback.debug(LogSanitizerUtil.sanitize(message), t); + fallback.debug(sanitizeLog(message), t); + } + + public void error(String message) { + fallback.error(sanitizeLog(message)); + } + + public void error(String message, Throwable t) { + fallback.error(sanitizeLog(message), t); + } + + public static String sanitizeLog(String message) { + return LogSanitizerUtil.sanitize(message); } } -} \ No newline at end of file +} From bd1fad291870a7da5997514fad65061c1a42313e Mon Sep 17 00:00:00 2001 From: Klaus Kiefer Date: Tue, 5 Mar 2024 11:54:17 +0100 Subject: [PATCH 02/10] add trace methods --- .../identity/uaa/logging/SanitizedLogFactory.java | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java index 71564682b1f..cf22af29729 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java @@ -1,9 +1,9 @@ -package org.coundfoundry.identity.uaa.logging; +package org.cloudfoundry.identity.uaa.logging; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -/** +/** TODO remove fork * Returns Log instance that replaces \n, \r, \t with a | to prevent log forging. */ public class SanitizedLogFactory { @@ -59,7 +59,15 @@ public void error(String message, Throwable t) { fallback.error(sanitizeLog(message), t); } - public static String sanitizeLog(String message) { + public void trace(String message) { + fallback.trace(sanitizeLog(message)); + } + + public void trace(String message, Throwable t) { + fallback.trace(sanitizeLog(message), t); + } + + protected static String sanitizeLog(String message) { return LogSanitizerUtil.sanitize(message); } } From 7b45858474f2233ec9e30a92758f8774148498cd Mon Sep 17 00:00:00 2001 From: Klaus Kiefer Date: Tue, 5 Mar 2024 11:55:11 +0100 Subject: [PATCH 03/10] remove comment --- .../cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java index cf22af29729..ec7eaf26880 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java @@ -3,7 +3,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -/** TODO remove fork +/** * Returns Log instance that replaces \n, \r, \t with a | to prevent log forging. */ public class SanitizedLogFactory { From e6d82932945698e9da33960f54c0c910261b64ce Mon Sep 17 00:00:00 2001 From: Klaus Kiefer Date: Tue, 5 Mar 2024 12:30:29 +0100 Subject: [PATCH 04/10] additional test methods --- .../uaa/logging/SanitizedLogFactoryTest.java | 96 +++++++++++++++---- 1 file changed, 77 insertions(+), 19 deletions(-) diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java index 60bf5e795be..3f49ccf209e 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java @@ -1,61 +1,119 @@ package org.cloudfoundry.identity.uaa.logging; import org.slf4j.Logger; +import org.junit.Assert; import org.junit.Before; import org.junit.Test; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; public class SanitizedLogFactoryTest { + private final String dirtyMessage = "one\ntwo\tthree\rfour"; + private final String sanitizedMsg = "one|two|three|four[SANITIZED]"; + private final String cleanMessage = "one two three four"; + Logger mockLog; + SanitizedLogFactory.SanitizedLog log; + Exception ex; @Before public void setUp() { mockLog = mock(Logger.class); + when(mockLog.isDebugEnabled()).thenReturn(true); + log = new SanitizedLogFactory.SanitizedLog(mockLog); + ex = new Exception(); } @Test public void testSanitizeDebug() { - SanitizedLogFactory.SanitizedLog log = new SanitizedLogFactory.SanitizedLog(mockLog); - log.debug("one\ntwo\tthree\rfour"); - verify(mockLog).debug("one|two|three|four[SANITIZED]"); + Assert.assertTrue(log.isDebugEnabled()); + log.debug(dirtyMessage); + verify(mockLog).debug(sanitizedMsg); + log.debug(dirtyMessage, ex); + verify(mockLog).debug(sanitizedMsg, ex); } @Test public void testSanitizeDebugCleanMessage() { - SanitizedLogFactory.SanitizedLog log = new SanitizedLogFactory.SanitizedLog(mockLog); - log.debug("one two three four"); - verify(mockLog).debug("one two three four"); + Assert.assertTrue(log.isDebugEnabled()); + log.debug(cleanMessage); + verify(mockLog).debug(cleanMessage); + log.debug(cleanMessage, ex); + verify(mockLog).debug(cleanMessage, ex); } @Test public void testSanitizeInfo() { - SanitizedLogFactory.SanitizedLog log = new SanitizedLogFactory.SanitizedLog(mockLog); - log.info("one\ntwo\tthree\rfour"); - verify(mockLog).info("one|two|three|four[SANITIZED]"); + log.info(dirtyMessage); + verify(mockLog).info(sanitizedMsg); + log.info(dirtyMessage, ex); + verify(mockLog).info(sanitizedMsg, ex); } @Test public void testSanitizeInfoCleanMessage() { - SanitizedLogFactory.SanitizedLog log = new SanitizedLogFactory.SanitizedLog(mockLog); - log.info("one two three four"); - verify(mockLog).info("one two three four"); + log.info(cleanMessage); + verify(mockLog).info(cleanMessage); + log.info(cleanMessage, ex); + verify(mockLog).info(cleanMessage, ex); } @Test public void testSanitizeWarn() { - SanitizedLogFactory.SanitizedLog log = new SanitizedLogFactory.SanitizedLog(mockLog); - log.warn("one\ntwo\tthree\rfour"); - verify(mockLog).warn("one|two|three|four[SANITIZED]"); + log.warn(dirtyMessage); + verify(mockLog).warn(sanitizedMsg); + log.warn(dirtyMessage, ex); + verify(mockLog).warn(sanitizedMsg, ex); } @Test public void testSanitizeWarnCleanMessage() { - SanitizedLogFactory.SanitizedLog log = new SanitizedLogFactory.SanitizedLog(mockLog); - log.warn("one two three four"); - verify(mockLog).warn("one two three four"); + log.warn(cleanMessage); + verify(mockLog).warn(cleanMessage); + log.warn(cleanMessage, ex); + verify(mockLog).warn(cleanMessage, ex); + } + + @Test + public void testSanitizeError() { + log.error(dirtyMessage); + verify(mockLog).error(sanitizedMsg); + log.error(dirtyMessage, ex); + verify(mockLog).error(sanitizedMsg, ex); } -} \ No newline at end of file + @Test + public void testSanitizeErrorCleanMessage() { + log.error(cleanMessage); + verify(mockLog).error(cleanMessage); + log.error(cleanMessage, ex); + verify(mockLog).error(cleanMessage, ex); + } + + @Test + public void testSanitizeTrace() { + log.trace(dirtyMessage); + verify(mockLog).trace(sanitizedMsg); + log.trace(dirtyMessage, ex); + verify(mockLog).trace(sanitizedMsg, ex); + } + + @Test + public void testSanitizeTraceCleanMessage() { + log.trace(cleanMessage); + verify(mockLog).trace(cleanMessage); + log.trace(cleanMessage, ex); + verify(mockLog).trace(cleanMessage, ex); + } + + @Test + public void testSanitizeLog() { + String logMsg = SanitizedLogFactory.SanitizedLog.sanitizeLog(dirtyMessage); + Assert.assertEquals(sanitizedMsg, logMsg); + logMsg = SanitizedLogFactory.SanitizedLog.sanitizeLog(cleanMessage); + Assert.assertEquals(cleanMessage, logMsg); + } +} From fe5f11480323f6412757dc80a9e98482933ea47c Mon Sep 17 00:00:00 2001 From: Klaus Kiefer Date: Tue, 5 Mar 2024 12:32:56 +0100 Subject: [PATCH 05/10] remove method --- .../identity/uaa/logging/SanitizedLogFactory.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java index ec7eaf26880..5566a4be5c6 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java @@ -16,10 +16,6 @@ public static class SanitizedLog { private Logger fallback; public SanitizedLog(Logger logger) { - setFallback(logger); - } - - public void setFallback(Logger logger) { this.fallback = logger; } From da19b79321e7c93bf193dc646c2c55ed6b4aefda Mon Sep 17 00:00:00 2001 From: D023954 Date: Tue, 5 Mar 2024 15:58:07 +0100 Subject: [PATCH 06/10] use random exception text --- .vscode/settings.json | 3 +++ .../identity/uaa/logging/SanitizedLogFactoryTest.java | 4 +++- 2 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 .vscode/settings.json diff --git a/.vscode/settings.json b/.vscode/settings.json new file mode 100644 index 00000000000..849f79e6b5e --- /dev/null +++ b/.vscode/settings.json @@ -0,0 +1,3 @@ +{ + "java.compile.nullAnalysis.mode": "automatic" +} diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java index 3f49ccf209e..d4b316fdd39 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java @@ -9,6 +9,8 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import org.apache.commons.lang3.RandomStringUtils; + public class SanitizedLogFactoryTest { private final String dirtyMessage = "one\ntwo\tthree\rfour"; @@ -24,7 +26,7 @@ public void setUp() { mockLog = mock(Logger.class); when(mockLog.isDebugEnabled()).thenReturn(true); log = new SanitizedLogFactory.SanitizedLog(mockLog); - ex = new Exception(); + ex = new Exception(RandomStringUtils.randomAlphanumeric(8)); } @Test From 2b9b3e959170062683d3b98565014e33e41435bf Mon Sep 17 00:00:00 2001 From: Klaus Kiefer Date: Tue, 5 Mar 2024 17:50:42 +0100 Subject: [PATCH 07/10] Delete .vscode/settings.json --- .vscode/settings.json | 3 --- 1 file changed, 3 deletions(-) delete mode 100644 .vscode/settings.json diff --git a/.vscode/settings.json b/.vscode/settings.json deleted file mode 100644 index 849f79e6b5e..00000000000 --- a/.vscode/settings.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "java.compile.nullAnalysis.mode": "automatic" -} From 56df3a5605f44433202250155d4b480fe6a6a1c0 Mon Sep 17 00:00:00 2001 From: Klaus Kiefer Date: Wed, 6 Mar 2024 09:33:06 +0100 Subject: [PATCH 08/10] suppress false warnings --- .../cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java | 1 + 1 file changed, 1 insertion(+) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java index 5566a4be5c6..3a7e6f09c9b 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java @@ -6,6 +6,7 @@ /** * Returns Log instance that replaces \n, \r, \t with a | to prevent log forging. */ +@SuppressWarnings({"javasecurity:S5145", "javasecurity:S2629"}) // sanitize log messages public class SanitizedLogFactory { public static SanitizedLog getLog(Class clazz) { From d7724376a0b1582feb35cf33a9f0d99cb65c3d4e Mon Sep 17 00:00:00 2001 From: Klaus Kiefer Date: Wed, 6 Mar 2024 10:42:19 +0100 Subject: [PATCH 09/10] suppress performance warning --- .../cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java index 3a7e6f09c9b..3b369590f22 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java @@ -6,7 +6,7 @@ /** * Returns Log instance that replaces \n, \r, \t with a | to prevent log forging. */ -@SuppressWarnings({"javasecurity:S5145", "javasecurity:S2629"}) // sanitize log messages +@SuppressWarnings({"javasecurity:S5145", "java:S2629"}) // sanitize log messages public class SanitizedLogFactory { public static SanitizedLog getLog(Class clazz) { From c57dad7ace41ecbb65f75d1fafa335fa5e07c45c Mon Sep 17 00:00:00 2001 From: strehle Date: Thu, 7 Mar 2024 12:10:08 +0100 Subject: [PATCH 10/10] improve coverage --- .../uaa/logging/SanitizedLogFactory.java | 3 +- .../uaa/logging/SanitizedLogFactoryTest.java | 36 +++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java index 81ed016f5c1..509d7482f9c 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactory.java @@ -8,8 +8,7 @@ */ public class SanitizedLogFactory { - private SanitizedLogFactory() { - } + private SanitizedLogFactory() { } public static SanitizedLog getLog(Class clazz) { return new SanitizedLog(LogManager.getLogger(clazz)); diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java index 4601270a47e..f464c56f936 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/logging/SanitizedLogFactoryTest.java @@ -1,10 +1,12 @@ package org.cloudfoundry.identity.uaa.logging; import org.apache.logging.log4j.Logger; +import org.junit.Assert; import org.junit.Before; import org.junit.Test; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -27,6 +29,11 @@ public void setUp() { ex = new Exception(RandomStringUtils.randomAlphanumeric(8)); } + @Test + public void testInit() { + Assert.assertNotNull(SanitizedLogFactory.getLog(SanitizedLogFactoryTest.class)); + } + @Test public void testSanitizeInfo() { when(mockLog.isInfoEnabled()).thenReturn(true); @@ -63,6 +70,17 @@ public void testSanitizeDebugCleanMessage() { verify(mockLog).debug(cleanMessage, ex); } + @Test + public void testSanitizeDebugCleanMessageNotEnabled() { + when(mockLog.isDebugEnabled()).thenReturn(false); + log.debug(cleanMessage); + verify(mockLog, never()).debug(cleanMessage); + log.debug(cleanMessage, ex); + verify(mockLog, never()).debug(cleanMessage, ex); + Assert.assertFalse(log.isDebugEnabled()); + } + + @Test public void testSanitizeWarn() { when(mockLog.isWarnEnabled()).thenReturn(true); @@ -81,6 +99,15 @@ public void testSanitizeWarnCleanMessage() { verify(mockLog).warn(cleanMessage, ex); } + @Test + public void testSanitizeWarnCleanMessageNotEnabled() { + when(mockLog.isWarnEnabled()).thenReturn(false); + log.warn(cleanMessage); + verify(mockLog, never()).warn(cleanMessage); + log.warn(cleanMessage, ex); + verify(mockLog, never()).warn(cleanMessage, ex); + } + @Test public void testSanitizeError() { when(mockLog.isErrorEnabled()).thenReturn(true); @@ -116,4 +143,13 @@ public void testSanitizeTraceCleanMessage() { log.trace(cleanMessage, ex); verify(mockLog).trace(cleanMessage, ex); } + + @Test + public void testSanitizeTraceCleanMessageWhenNotEnabled() { + when(mockLog.isTraceEnabled()).thenReturn(false); + log.trace(cleanMessage); + verify(mockLog, never()).trace(cleanMessage); + log.trace(cleanMessage, ex); + verify(mockLog, never()).trace(cleanMessage, ex); + } }