Added property to omit event type information in logs #813

RodolfoAndre · 2023-11-21T13:42:57Z

This PR adds the property "Logger.OmitEventTypeInLogs" that will make event type information not be prepended in log messages when set to true (default false).

kwwall

@RodolfoAndre - Please kindly address the code review comments. You may want to wait until @jeremiahjstacey adds his comments as he is the author and expert of the revised ESAPI logging.

kwwall · 2023-11-21T17:37:42Z

src/main/java/org/owasp/esapi/logging/appender/LogPrefixAppender.java

     */
-    public LogPrefixAppender(boolean logUserInfo, boolean logClientInfo, boolean logServerIp, boolean logApplicationName, String appName) {
+    public LogPrefixAppender(boolean logUserInfo, boolean logClientInfo, boolean logServerIp, boolean logApplicationName, String appName, boolean omitEventTypeInLogs) {


We cannot change the signatures of any public or protected methods as that could potentially break someone's code. Instead, On line 39, just initialize this there instead of in the CTOR. You can do it it a static initializer block if you need to catch exceptions. The default value for omitEventTypeInLogs must be false in case the property value Logger.OmitEventTypeInLogs is not found in ESAPI.properties at all, which will not be uncommon because based on our experience, many ESAPI users never seem to update their ESAPI.properties file with the latest version. But that is the only way we can make this work.

An alternative--one that I don't like as much, but may find acceptable assuming that @jeremiahjstacey is okay with it is to add a new CTOR rather than just replacing this one and then have this existing CTOR, just invoke:

this(logUserInfo, logClientInfo, logServerIp, logApplicaionName, appname, false);

That at least, can't break existing code, although it does at a bit of complexity to this class.

Ok, I have changed back. I have a question where should I get the OmitEventTypeInLogs property?

I could create a static variable in Slf4JLogFactory and use the static method to get this property and create a setOmitEventTypeInLogs method in LogPrefixAppender to set the value;
I can do that when the LogPrefixAppender instanced in Slf4JLogFactory static method. So I would get the property and create a setOmitEventTypeInLogs method in LogPrefixAppender to set the value.

Which approach do you think would be better or do you have a suggestion?

ESAPI generally makes the assumption (except when running JUnit tests) that the value of a a specific property provided in ESAPI.properties doesn't change in the middle of your application, so as a result, it's generally fine to assume one a property is set, you can store its value in a static class variable.

That said, what I was thinking in the context of LogPrefixAppender was rather than changing the CTOR (or adding a new one), change your declaration of:

private final boolean omitEventTypeInLogs;

to

import static org.owasp.esapi.PropNames.OMIT_EVENT_TYPE_IN_LOGS; ... private static boolean omitEventTypeInLogs; static { try { omitEventTypeInLogs = ESAPI.securityConfiguration().getBooleanProp( OMIT_EVENT_TYPE_IN_LOGS ); } catch (Exception ex) { // Usually this will be ConfigurationException omitEventTypeInLogs = false; } }

Not sure if that is what you were asking though.

Yes! thank you. With this solution most of the comments bellow are fulfilled. I will send another patch shortly.

kwwall · 2023-11-21T17:50:06Z

src/main/java/org/owasp/esapi/logging/appender/LogPrefixAppender.java

    }

    @Override
    public String appendTo(String logName, EventType eventType, String message) {
+        if (omitEventTypeInLogs) {


This does not look correct to me. This would also skip over all the other parameters collected in lines 78-81 (of your new code) . I may be missing something, but I think that around line 78, this should be changed to something like this:

String eventTypeMsg = ( omitEventTypeInLogs ? "" : eventTypeSupplier.get().trim() );

I think doing this they way you have it drops a lot more than just the Logger.EVENT_TYPE.

Yes, I was thinking in a pespective that everything inside the [] was related to event type. So I have made the change you suggested and then it will check if the prefix is empty, if so it will return just the message without the prefix, otherwise it will add the prefix in the message.

kwwall · 2023-11-21T18:04:02Z

src/main/java/org/owasp/esapi/logging/java/JavaLogFactory.java

@@ -79,7 +80,8 @@ public class JavaLogFactory implements LogFactory {
        boolean logApplicationName = ESAPI.securityConfiguration().getBooleanProp(LOG_APPLICATION_NAME);
        String appName = ESAPI.securityConfiguration().getStringProp(APPLICATION_NAME);
        boolean logServerIp = ESAPI.securityConfiguration().getBooleanProp(LOG_SERVER_IP);
-        JAVA_LOG_APPENDER = createLogAppender(logUserInfo, logClientInfo, logServerIp, logApplicationName, appName);
+        boolean omitEventTypeInLogs = ESAPI.securityConfiguration().getBooleanProp(OMIT_EVENT_TYPE_IN_LOGS);


For reasons noted in a comment in the LogPrefixAppender CTOR that you modifed, there is a (probably good) possibility that this property will not be found at all in the ESAPI.properties file, so you need to be prepared to catch ConfigurationException here and set omitEventTypeInlogs to false should that happen. Because we can't let a ConfigurationException propagate back to the caller if Logger.OmitEventTypeInLogs is not found. That would result in breaking people's code and as a result is just not acceptable.

:( Catching a runtime exception to avoid breaking feels dirty to me, but I do agree it has the unfavorable result of breaking implementations when new properties are required.

It is worth noting that there are actually 2 cases that the Exception may occur:

the value is not set

the value is not of the expected type (cannot be converted to a boolean)

In the second case, catching this exception may result in hiding a problem with a property value. I do think it is a less likely scenario, but still worth noting.

@Override public Boolean getBooleanProp(String propertyName) throws ConfigurationException { try { return esapiPropertyManager.getBooleanProp(propertyName); } catch (ConfigurationException ex) { String property = properties.getProperty( propertyName ); if ( property == null ) { throw new ConfigurationException( "SecurityConfiguration for " + propertyName + " not found in ESAPI.properties"); } if ( property.equalsIgnoreCase("true") || property.equalsIgnoreCase("yes" ) ) { return true; } if ( property.equalsIgnoreCase("false") || property.equalsIgnoreCase( "no" ) ) { return false; } throw new ConfigurationException( "SecurityConfiguration for " + propertyName + " has incorrect " + "type"); } }

@jeremiahjstacey - True, but the more common error is just going to be that people don't update their ESAPI.properties file regularly, so the property would be missing and I don't want ESAPI to start failing (and thus breaking code) because of that. We could have the ConfigurationException logged via logSpecial(), but I'd prefer dirty over adding a new CTOR and all the other ugliness that brings into ESAPI code. We can also note it in a comment there why this particular ugly choice is made over a different ugly choice.

We can incorporate the check of property value as you mentitoned and ignore it if the property is not present. What do you think?

if (property != null) { if (property.equalsIgnoreCase("true") || property.equalsIgnoreCase("yes")) { omitEventTypeInLogs = true; } else if (property.equalsIgnoreCase("false") || property.equalsIgnoreCase("no")) { omitEventTypeInLogs = false; } else if (property != null) { throw new ConfigurationException("SecurityConfiguration for " + OMIT_EVENT_TYPE_IN_LOGS + " has incorrect " + "type"); } }

The code I posted is the default implementation of getBooleanProp from the DefaultSecurityProvider. I just wanted to identify the alternative case that is being swallowed.

It is known and is still preferred to the probability of breaking an environment due to a missing property.

@RodolfoAndre You do not need to make any additional changes for this as far as I am concerned.

kwwall · 2023-11-21T18:09:28Z

src/main/java/org/owasp/esapi/logging/java/JavaLogFactory.java

-    /*package*/ static LogAppender createLogAppender(boolean logUserInfo, boolean logClientInfo, boolean logServerIp, boolean logApplicationName, String appName) {
-        return new LogPrefixAppender(logUserInfo, logClientInfo, logServerIp, logApplicationName, appName);
+    /*package*/ static LogAppender createLogAppender(boolean logUserInfo, boolean logClientInfo, boolean logServerIp, boolean logApplicationName, String appName, boolean omitEventTypeInLogs) {
+        return new LogPrefixAppender(logUserInfo, logClientInfo, logServerIp, logApplicationName, appName, omitEventTypeInLogs);


May wish to change this, depending @jeremiahjstacey's take on my comments on the LogPrefixAppender CTOR. But at even if this remains the same here, we can't simple remove the old CTOR as the CTOR is public and thus off limits. (Once we start using Java 9 or later, we can better control this with Java modules, but until then, no changes to signatures of public or protected methods unless it is absolutely required to address a vulnerability.)

kwwall · 2023-11-21T18:11:11Z

src/main/java/org/owasp/esapi/logging/slf4j/Slf4JLogFactory.java

@@ -69,7 +71,8 @@ public class Slf4JLogFactory implements LogFactory {
        boolean logApplicationName = ESAPI.securityConfiguration().getBooleanProp(LOG_APPLICATION_NAME);
        String appName = ESAPI.securityConfiguration().getStringProp(APPLICATION_NAME);
        boolean logServerIp = ESAPI.securityConfiguration().getBooleanProp(LOG_SERVER_IP);
-        SLF4J_LOG_APPENDER = createLogAppender(logUserInfo, logClientInfo, logServerIp, logApplicationName, appName);
+        boolean omitEventTypeInLogs = ESAPI.securityConfiguration().getBooleanProp(OMIT_EVENT_TYPE_IN_LOGS);


Same comment here as for JavaLogFactory class... for reasons noted in a comment in the LogPrefixAppender CTOR that you modifed, there is a (probably good) possibility that this property will not be found at all in the ESAPI.properties file, so you need to be prepared to catch ConfigurationException here and set omitEventTypeInlogs to false should that happen. Because we can't let a ConfigurationException propagate back to the caller if Logger.OmitEventTypeInLogs is not found. That would result in breaking people's code and as a result is just not acceptable.

kwwall · 2023-11-21T18:14:06Z

src/test/java/org/owasp/esapi/logging/appender/LogPrefixAppenderTest.java

@@ -63,7 +64,7 @@ public void testCtrArgTruePassthroughToDelegates() throws Exception {
        whenNew(ClientInfoSupplier.class).withNoArguments().thenReturn(cisSpy);
        whenNew(ServerInfoSupplier.class).withArguments(testLoggerName).thenReturn(sisSpy);

-        LogPrefixAppender lpa = new LogPrefixAppender(true, true,true,true, testApplicationName);


@jeremiahjstacey - I think we need to keep both tests, do we not?

kwwall · 2023-11-21T18:14:26Z

src/test/java/org/owasp/esapi/logging/appender/LogPrefixAppenderTest.java

@@ -84,7 +85,7 @@ public void testCtrArgFalsePassthroughToDelegates() throws Exception {
        whenNew(ClientInfoSupplier.class).withNoArguments().thenReturn(cisSpy);
        whenNew(ServerInfoSupplier.class).withArguments(testLoggerName).thenReturn(sisSpy);

-        LogPrefixAppender lpa = new LogPrefixAppender(false, false, false, false, null);
+        LogPrefixAppender lpa = new LogPrefixAppender(false, false, false, false, null, false);


Ditto previous comment.

kwwall · 2023-11-21T18:16:41Z

src/test/java/org/owasp/esapi/logging/appender/LogPrefixAppenderTest.java

@@ -102,51 +103,61 @@ public void testDelegateCtrArgs() throws Exception {
        whenNew(ClientInfoSupplier.class).withNoArguments().thenReturn(cisSpy);
        whenNew(ServerInfoSupplier.class).withArguments(logNameCapture.capture()).thenReturn(sisSpy);

-        LogPrefixAppender lpa = new LogPrefixAppender(true, true,true,true, testApplicationName);


Again, same comment. I think it would just be best to copy the existing test(s) and then make a new test.

Also, you can now see why I don't like the whole idea of changing the CTOR.

src/test/java/org/owasp/esapi/logging/appender/LogPrefixAppenderTest.java

kwwall · 2023-11-21T18:20:11Z

src/test/java/org/owasp/esapi/logging/java/JavaLogFactoryTest.java

@@ -171,23 +171,25 @@ public void checkScrubberWithoutEncoding() throws Exception {
     */
    @Test
    public void checkPassthroughAppenderConstruct() throws Exception {
-        LogPrefixAppender stubAppender = new LogPrefixAppender(true, true, true, true, "");
+        LogPrefixAppender stubAppender = new LogPrefixAppender(true, true, true, true, "", false);


@jeremiahjstacey - Ditto here. I will trust your code review here. No further comments from me in this file.

kwwall · 2023-11-21T18:25:21Z

@jeremiahjstacey - Please code review this when you have a moment. I have already made a lot of comments, but I'm not sure I understand the JUnit tests well enough unless I really dive into the gritty details of how they work. Note this addresses GitHub Issue #811.

…ppender, updated the prefix building and improved the tests

jeremiahjstacey · 2023-11-22T22:49:27Z

configuration/esapi/ESAPI.properties

I believe there are also ESAPI.properties files in the test scope that should also be updated
https://github.com/ESAPI/esapi-java-legacy/blob/develop/src/test/resources/esapi/ESAPI.properties

Absolutely correct. I thought at the time that I originally reviewed this, he had updated the src/test/resources/esapi/ESAPI.properties file to add that new property. Maybe he undid it in the last commit or maybe I just was imagining it, but yeah, it needs to be there too.

src/main/java/org/owasp/esapi/logging/appender/LogPrefixAppender.java

jeremiahjstacey · 2023-11-22T23:05:16Z

src/main/java/org/owasp/esapi/logging/appender/LogPrefixAppender.java

+        if (logPrefix.toString().trim().isEmpty()) {
+            // if there isn't any log prefix we just send back the message without touching it
+            return message;
+        }
+
+        return String.format(RESULT_FORMAT, logPrefix.toString().trim(), message);


I would feel better about this if it was a single return as a ternary:

String prefix = logPrefix.toString().trim(); return prefix.isEmpty() ? message : String.format(RESULT_FORMAT, prefix, message);

I'm good with that.

jeremiahjstacey · 2023-11-22T23:10:02Z

src/test/java/org/owasp/esapi/logging/appender/LogPrefixAppenderTest.java

+        // Using reflection API to set omitEventTypeInLogs field in LogPrefixAppender.
+        setOmitEventTypeInLogsFieldUsingReflection(lpa, omitEventTypeInLogs);


From my previous comments about making the variable a member instance, we will not need reflection here.

System.setProperty(PropNames.OMIT_EVENT_TYPE_IN_LOGS, omitEventTypeInLogs); //Since everything is mocked these booleans don't much matter aside from the later verifies LogPrefixAppender lpa = new LogPrefixAppender(false, false, false, false, null);

Except, setting this is a System property still won't make that property get picked up by ESAPI.securityConfiguration().getBooleanProp().

@RodolfoAndre - We have a "trick" to make ESAPI properties work with JUnit tests so we can simulate various values that @jeremiahjstacey may have forgotten about. We use the org.owasp.esapi.SecurityConfigurationWrapper class for that. Rather than trying to describe how to use that for JUnit tests, I will instead refer you to
https://github.com/ESAPI/esapi-java-legacy/blob/develop/src/test/java/org/owasp/esapi/reference/validation/HTMLValidationRuleCleanTest.java
as a typical example of how we use it. If after looking at that, you still can't figure it out, then let me know.

My initial context is invalid for removing reflection utility, but if we're able to do that I think it would be a good thing.
Thank you for the context @kwwall

@kwwall I could change the property just once. After the first initalization the class won't get the property again. So I can't have test for true and false in the same class.

It will work if I change the test to another file or create a static class inside this one just for testing the flag. Do you have any idea what may I do?

@RodolfoAndre - The way I handled it was to create 2 separate JUnit classes, each of which tested one of the possible values. For example, for the property Validator.HtmlValidationAction, which can have the value "clean" or "throw", I respectively created:

https://github.com/ESAPI/esapi-java-legacy/blob/develop/src/test/java/org/owasp/esapi/reference/validation/HTMLValidationRuleCleanTest.java

https://github.com/ESAPI/esapi-java-legacy/blob/develop/src/test/java/org/owasp/esapi/reference/validation/HTMLValidationRuleThrowsTest.java

(although for the latter, it looks as though I may have botched line 53, although I'm not sure it matters since it is set correctly in the @Before setup method on line 84).

But that explains the general approach of how I tested both variations. There's probably some other way to do it, but that worked. It's also why the JUnit file are named as they are. HTH.

Ok. So I've added the new file with the tests and reverted the other one just adding a test to check if it is working with the flag off. It is in the new commits that I've sent.

src/main/java/org/owasp/esapi/PropNames.java

…roperties test file

kwwall · 2024-05-24T01:26:23Z

@RodolfoAndre - There are requested changes here. Any chance you can make them before Sunday, 2024/05/26 as I'm considering doing a release this long holiday weekend. (See Discussion #834.) If not, I will leave this PR unmerged and it will have to wait until a future release. Thanks.

Added property to omit event type information in logs

eeda7ff

kwwall requested changes Nov 21, 2023

View reviewed changes

Applied some review changes, moved the property reading to LogPrefixA…

fe4841c

…ppender, updated the prefix building and improved the tests

jeremiahjstacey reviewed Nov 22, 2023

View reviewed changes

src/main/java/org/owasp/esapi/logging/appender/LogPrefixAppender.java Show resolved Hide resolved

jeremiahjstacey reviewed Nov 22, 2023

View reviewed changes

src/main/java/org/owasp/esapi/PropNames.java Show resolved Hide resolved

RodolfoAndre added 2 commits November 23, 2023 20:55

Update LogPrefixAppender, added the omitEventTypeInLogs property to p…

49e2e8e

…roperties test file

Remove of unused imports

29db8c7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added property to omit event type information in logs #813

Added property to omit event type information in logs #813

RodolfoAndre commented Nov 21, 2023

kwwall left a comment

kwwall Nov 21, 2023

RodolfoAndre Nov 21, 2023

kwwall Nov 21, 2023 •

edited

Loading

RodolfoAndre Nov 22, 2023

kwwall Nov 21, 2023

RodolfoAndre Nov 22, 2023

kwwall Nov 21, 2023

jeremiahjstacey Nov 22, 2023

kwwall Nov 23, 2023 •

edited

Loading

RodolfoAndre Nov 23, 2023

jeremiahjstacey Nov 23, 2023

kwwall Nov 21, 2023

kwwall Nov 21, 2023

kwwall Nov 21, 2023

kwwall Nov 21, 2023

kwwall Nov 21, 2023

kwwall Nov 21, 2023

kwwall commented Nov 21, 2023 •

edited

Loading

jeremiahjstacey Nov 22, 2023

kwwall Nov 23, 2023

jeremiahjstacey Nov 22, 2023

kwwall Nov 23, 2023

jeremiahjstacey Nov 22, 2023

kwwall Nov 23, 2023

jeremiahjstacey Nov 23, 2023

RodolfoAndre Nov 23, 2023

kwwall Nov 24, 2023

RodolfoAndre Nov 25, 2023

kwwall commented May 24, 2024

		// Using reflection API to set omitEventTypeInLogs field in LogPrefixAppender.
		setOmitEventTypeInLogsFieldUsingReflection(lpa, omitEventTypeInLogs);

Added property to omit event type information in logs #813

Are you sure you want to change the base?

Added property to omit event type information in logs #813

Conversation

RodolfoAndre commented Nov 21, 2023

kwwall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwwall Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwwall Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwwall commented Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwwall commented May 24, 2024

kwwall Nov 21, 2023 •

edited

Loading

kwwall Nov 23, 2023 •

edited

Loading

kwwall commented Nov 21, 2023 •

edited

Loading