Skip to content

Added property to omit event type information in logs #813

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

Added property to omit event type information in logs #813

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eeda7ff
Added property to omit event type information in logs
RodolfoAndre Nov 21, 2023
fe4841c
Applied some review changes, moved the property reading to LogPrefixA…
RodolfoAndre Nov 22, 2023
49e2e8e
Update LogPrefixAppender, added the omitEventTypeInLogs property to p…
RodolfoAndre Nov 23, 2023
29db8c7
Remove of unused imports
RodolfoAndre Nov 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions configuration/esapi/ESAPI.properties
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,8 @@ Logger.LogServerIP=true
Logger.UserInfo=true
# Determines whether ESAPI should log the session id and client IP.
Logger.ClientInfo=true
# Determines whether ESAPI should log the event type or not
Logger.OmitEventTypeInLogs=false

#===========================================================================
# ESAPI Intrusion Detection
Expand Down
1 change: 1 addition & 0 deletions src/main/java/org/owasp/esapi/PropNames.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ public final class PropNames {
public static final String LOG_ENCODING_REQUIRED = "Logger.LogEncodingRequired";
public static final String LOG_APPLICATION_NAME = "Logger.LogApplicationName";
public static final String LOG_SERVER_IP = "Logger.LogServerIP";
public static final String OMIT_EVENT_TYPE_IN_LOGS = "Logger.OmitEventTypeInLogs";

public static final String VALIDATION_PROPERTIES = "Validator.ConfigurationFile";
public static final String VALIDATION_PROPERTIES_MULTIVALUED = "Validator.ConfigurationFile.MultiValued";
Expand Down
27 changes: 24 additions & 3 deletions src/main/java/org/owasp/esapi/logging/appender/LogPrefixAppender.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,11 @@

package org.owasp.esapi.logging.appender;

import org.owasp.esapi.ESAPI;
import org.owasp.esapi.Logger.EventType;
import org.owasp.esapi.errors.ConfigurationException;

import static org.owasp.esapi.PropNames.OMIT_EVENT_TYPE_IN_LOGS;

/**
* LogAppender Implementation which can prefix the common logger information for
Expand All @@ -35,6 +39,18 @@ public class LogPrefixAppender implements LogAppender {
private final boolean logApplicationName;
/** Application Name to record. */
private final String appName;
/** Whether to omit event type in logs or not. */
private static boolean omitEventTypeInLogs;

static {

try {
omitEventTypeInLogs =
ESAPI.securityConfiguration().getBooleanProp(OMIT_EVENT_TYPE_IN_LOGS);
} catch (ConfigurationException ex) {
omitEventTypeInLogs = false;
}
}

/**
* Ctr.
Expand Down Expand Up @@ -67,7 +83,7 @@ public String appendTo(String logName, EventType eventType, String message) {
serverInfoSupplier.setLogServerIp(logServerIp);
serverInfoSupplier.setLogApplicationName(logApplicationName, appName);

String eventTypeMsg = eventTypeSupplier.get().trim();
String eventTypeMsg = omitEventTypeInLogs ? "" : eventTypeSupplier.get().trim();
String userInfoMsg = userInfoSupplier.get().trim();
String clientInfoMsg = clientInfoSupplier.get().trim();
String serverInfoMsg = serverInfoSupplier.get().trim();
Expand All @@ -81,7 +97,7 @@ public String appendTo(String logName, EventType eventType, String message) {
String[] optionalPrefixContent = new String[] {userInfoMsg + clientInfoMsg, serverInfoMsg};

StringBuilder logPrefix = new StringBuilder();
//EventType is always appended

logPrefix.append(eventTypeMsg);

for (String element : optionalPrefixContent) {
Expand All @@ -91,6 +107,11 @@ public String appendTo(String logName, EventType eventType, String message) {
}
}

return String.format(RESULT_FORMAT, logPrefix.toString(), message);
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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with that.

}
}
65 changes: 55 additions & 10 deletions src/test/java/org/owasp/esapi/logging/appender/LogPrefixAppenderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
import org.powermock.core.classloader.annotations.PrepareForTest;
import org.powermock.modules.junit4.PowerMockRunner;

import java.lang.reflect.Field;

@RunWith(PowerMockRunner.class)
@PrepareForTest(LogPrefixAppender.class)
public class LogPrefixAppenderTest {
Expand Down Expand Up @@ -109,44 +111,69 @@ public void testDelegateCtrArgs() throws Exception {
assertEquals(testLoggerName, logNameCapture.getValue());
}

@Test
public void testLongContentWithOmitEventTypeInLogs() throws Exception {
runTest(ETL_RESULT, EMPTY_RESULT, EMPTY_RESULT, EMPTY_RESULT, true, "");
}

@Test
public void testLongContentWithOmitEventTypeInLogsAndUserInfo() throws Exception {
runTest(ETL_RESULT, UIS_RESULT, EMPTY_RESULT, EMPTY_RESULT, true, "[USER_INFO]");
}

@Test
public void testLongContentWithOmitEventTypeInLogsAndClientInfo() throws Exception {
runTest(ETL_RESULT, EMPTY_RESULT, CIS_RESULT, EMPTY_RESULT, true, "[CLIENT_INFO]");
}

@Test
public void testLongContentWithOmitEventTypeInLogsAndServerInfo() throws Exception {
runTest(ETL_RESULT, EMPTY_RESULT, EMPTY_RESULT, SIS_RESULT, true, "[-> SERVER_INFO]");
}

@Test
public void testLongContentWithoutOmitEventTypeInLogs() throws Exception {
runTest(ETL_RESULT, EMPTY_RESULT, EMPTY_RESULT, EMPTY_RESULT, false, "[EVENT_TYPE]");
}

@Test
public void testLogContentWhenClientInfoEmpty() throws Exception {
runTest(ETL_RESULT, UIS_RESULT, EMPTY_RESULT,SIS_RESULT, "[EVENT_TYPE USER_INFO -> SERVER_INFO]");
runTest(ETL_RESULT, UIS_RESULT, EMPTY_RESULT,SIS_RESULT, false, "[EVENT_TYPE USER_INFO -> SERVER_INFO]");
}


@Test
public void testLogContentWhenUserInfoEmpty() throws Exception {
runTest(ETL_RESULT, EMPTY_RESULT, CIS_RESULT,SIS_RESULT, "[EVENT_TYPE CLIENT_INFO -> SERVER_INFO]");
runTest(ETL_RESULT, EMPTY_RESULT, CIS_RESULT,SIS_RESULT, false, "[EVENT_TYPE CLIENT_INFO -> SERVER_INFO]");
}

@Test
public void testLogContentWhenClientInfoEmptyAndServerInfoEmpty() throws Exception {
runTest(ETL_RESULT, UIS_RESULT, EMPTY_RESULT,EMPTY_RESULT, "[EVENT_TYPE USER_INFO]");
runTest(ETL_RESULT, UIS_RESULT, EMPTY_RESULT,EMPTY_RESULT, false, "[EVENT_TYPE USER_INFO]");
}

@Test
public void testLogContentWhenUserInfoEmptyAndServerInfoEmpty() throws Exception {
runTest(ETL_RESULT, EMPTY_RESULT, CIS_RESULT,EMPTY_RESULT, "[EVENT_TYPE CLIENT_INFO]");
runTest(ETL_RESULT, EMPTY_RESULT, CIS_RESULT,EMPTY_RESULT, false, "[EVENT_TYPE CLIENT_INFO]");
}

@Test
public void testLogContentWhenUserInfoAndClientInfoEmpty() throws Exception {
runTest(ETL_RESULT, EMPTY_RESULT, EMPTY_RESULT, SIS_RESULT, "[EVENT_TYPE -> SERVER_INFO]");
runTest(ETL_RESULT, EMPTY_RESULT, EMPTY_RESULT, SIS_RESULT, false, "[EVENT_TYPE -> SERVER_INFO]");
}

@Test
public void testLogContentWhenServerInfoEmpty() throws Exception {
runTest(ETL_RESULT, UIS_RESULT, CIS_RESULT, EMPTY_RESULT, "[EVENT_TYPE USER_INFO:CLIENT_INFO]");
runTest(ETL_RESULT, UIS_RESULT, CIS_RESULT, EMPTY_RESULT, false, "[EVENT_TYPE USER_INFO:CLIENT_INFO]");
}

@Test
public void testLogContentWhenUserInfoEmptyAndClientInfoEmptyAndServerInfoEmpty() throws Exception {
runTest(ETL_RESULT, EMPTY_RESULT, EMPTY_RESULT, EMPTY_RESULT, "[EVENT_TYPE]");
runTest(ETL_RESULT, EMPTY_RESULT, EMPTY_RESULT, EMPTY_RESULT, false, "[EVENT_TYPE]");
}


private void runTest(String typeResult, String userResult, String clientResult, String serverResult, String exResult) throws Exception{
private void runTest(String typeResult, String userResult, String clientResult, String serverResult, boolean omitEventTypeInLogs, String exResult) throws Exception{
when(etlsSpy.get()).thenReturn(typeResult);
when(uisSpy.get()).thenReturn(userResult);
when(cisSpy.get()).thenReturn(clientResult);
Expand All @@ -159,8 +186,26 @@ private void runTest(String typeResult, String userResult, String clientResult,

//Since everything is mocked these booleans don't much matter aside from the later verifies
LogPrefixAppender lpa = new LogPrefixAppender(false, false, false, false, null);
String result = lpa.appendTo(testLoggerName, testEventType, testLogMessage);

assertEquals(exResult + " " + testName.getMethodName() + "-MESSAGE", result);
// Using reflection API to set omitEventTypeInLogs field in LogPrefixAppender.
setOmitEventTypeInLogsFieldUsingReflection(lpa, omitEventTypeInLogs);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

(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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


String actualResult = lpa.appendTo(testLoggerName, testEventType, testLogMessage);

StringBuilder expectedResult = new StringBuilder();
if (!exResult.isEmpty()) {
expectedResult.append(exResult);
expectedResult.append(" ");
}
expectedResult.append(testName.getMethodName());
expectedResult.append("-MESSAGE");

assertEquals(expectedResult.toString() , actualResult);
}

private static void setOmitEventTypeInLogsFieldUsingReflection(LogPrefixAppender lpa, boolean omitEventTypeInLogs) throws NoSuchFieldException, IllegalAccessException {
Field omitEventTypeInLogsField = lpa.getClass().getDeclaredField("omitEventTypeInLogs");
omitEventTypeInLogsField.setAccessible(true);
omitEventTypeInLogsField.setBoolean(lpa,omitEventTypeInLogs);
}
}