Update the logging properties to opt-out of the prefix events #844 #845

mickeyz07 · 2024-06-25T14:12:19Z

Determines whether ESAPI should log the prefix of [EVENT_TYPE - APPLICATION NAME].
If all Logger.* settings in the ESAPI.properties file are false (including Logger.LogPrefix), then the log output would be the same as if no ESAPI was used.
That can be important, especially for clients that are using cloud providers. The charges for extra logging can quickly add up.
So, the user can sanitise untrusted data used to construct log entries by using a safe logging mechanism in the OWASP ESAPI Logger without any extra logging information.
However, only by changing a couple of settings in the ESAPI.properties file, a user can have a very detailed log if required.

configuration/esapi/ESAPI.properties

kwwall

So far, no objections, but I need to think about the tests a bit more. In the meantime, @mickeyz07, perhaps you can respond to my comments.

kwwall · 2024-06-25T23:48:27Z

configuration/esapi/ESAPI.properties

@@ -407,6 +407,10 @@ Logger.UserInfo=true
 # Determines whether ESAPI should log the session id and client IP.
 Logger.ClientInfo=true

+# Determines whether ESAPI should log the prefix of [EVENT_TYPE - APPLICATION NAME].
+# If all above Logger entries are set to false and LogIgnorePrefix is true, then the output would be the same like if no ESAPI was used
+Logger.LogIgnorePrefix=true


I agree with @jeremiahjstacey about the non-intuitive property name. But also we can't have this new property be set in a manner that is going to change the default log output as it has previously existed since ESAPI 2.0. As project co-lead, I'm flat out not going to allow that. So whatever the default is set to, the logs have to be formatted as they were before. The only times we break backward compatibility is to:

If doing so is the only way to fix a security issue.

Something behavior of a dependency forces us to do so.

Without first deprecating something (generally for a 2 year period) before changing the behavior.

When changing major # in semantic versioning (e.g., going from ESAPI 1.x to 2.x or 2.x to 3.x).

We've already learned that people do not read the release notes. Instead, they post GitHub issues, send the team private emails, post on Stack Overflow, etc. and we are left trying to provide answers. So, no, the default logging behavior is not going to change on my watch. If people like yourself want it changed, you will have to tweak the value in your application's private copy.

Also, VERY important note: We can't assume that people will update their ESAPI.properties file. They in fact often don't until something goes wrong. So be sure to test it to make sure that it behaves as it does in 2.5.4.0 and earlier even if the completely omit that new property file. We can't be changing behavior and more importantly, we don't want to throw a ConfigurationException if that property is missing.

kwwall · 2024-06-25T23:52:41Z

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

@@ -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 LOG_IGNORE_PREFIX                                                                = "Logger.LogIgnorePrefix";


If the property name changes as per @jeremiahjstacey's suggestion, then please rename this variable in a corresponding fashion as well.

kwwall · 2024-06-26T00:05:12Z

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

@@ -30,18 +30,27 @@ public class EventTypeLogSupplier // implements Supplier<String>
 {
    /** EventType reference to supply log representation of. */
    private final EventType eventType;
+    /** Whether to log or not the event type */
+    private boolean ignoreLogEventType = false;


If you take @jeremiahjstacey's suggestion, maybe instead it makes more sense to call this keepLogEventType (or even just logEventType it that's not used and you don't think it would be confusing) and set it to true. Of course, you'd have to invert all your logic, but long term, it probably would result in more readability, especially if the property is renamed as @jeremiahjstacey recommends.

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

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

kwwall · 2024-06-26T00:38:45Z

configuration/esapi/ESAPI.properties

@@ -407,6 +407,10 @@ Logger.UserInfo=true
 # Determines whether ESAPI should log the session id and client IP.
 Logger.ClientInfo=true

+# Determines whether ESAPI should log the prefix of [EVENT_TYPE - APPLICATION NAME].
+# If all above Logger entries are set to false and LogIgnorePrefix is true, then the output would be the same like if no ESAPI was used


I guess I'm missing something here. Are you saying that for this new property to have the effect of omitting the event type, that ALL of these other Logger.* property names from lines 400 through 408 inclusive MUST be false? That seems overly complicated to me.

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

kwwall · 2024-06-26T01:17:43Z

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

+    public EventTypeLogSupplierIgnoreEventTypeTest(Logger.EventType eventType, String result) {
+        this.eventType = eventType;
+        this.expectedResult = result;
+    }


Suggestion: Add blank line here for improved readability.

kwwall · 2024-06-26T01:21:12Z

src/test/resources/esapi/ESAPI.properties

@@ -439,6 +439,10 @@ Logger.UserInfo=true
 # Determines whether ESAPI should log the session id and client IP.
 Logger.ClientInfo=true

+# Determines whether ESAPI should log the prefix of [EVENT_TYPE - APPLICATION NAME].
+# If all above Logger entries are set to false and LogIgnorePrefix is true, then the output would be the same like if no ESAPI was used
+Logger.LogIgnorePrefix=true


Change this property name in accordance with @jeremiahjstacey's suggestion in configuration/esapi/ESAPI.properties.

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

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

… iteration for ESAPI#844

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

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

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

…iteration

… fourt iteration

jeremiahjstacey · 2024-07-01T15:33:49Z

@kwwall - This all looks good to me. if your concerns have been addressed please merge or leave a comment and I will follow up and get it merged this week

kwwall · 2024-07-04T21:56:12Z

@kwwall - This all looks good to me. if your concerns have been addressed please merge or leave a comment and I will follow up and get it merged this week

@jeremiahjstacey - I will take care of merging this, but probably won't get to it by today though, as I had hoped. But I want to run a few dynamic tests first to make sure the defaults are all set so as to be backward compatible. I think they are, but it's a lot easier to try it than try to judge based on the code. I hope to have it merged by this weekend unless I find something that needs fixed

kwwall · 2024-07-06T14:38:32Z

@mickeyz07 - Sorry I didn't notice this before. Jeremiah and I were focused on the code review itself. However, I just noticed that you have not signed your commits with a GPG / PGP key. For commits that will be merged to the 'develop' or 'main' branches, we require those commits to be signed.

From our CONTRIBUTING-TO-ESAPI.txt file:

A Special Note Regarding Making Commits for PRs
    Shortly after the 2.5.1.0 ESAPI release in late November 2022, the ESAPI
    team decided to lock down the 'develop' amd 'main' branches. Merges from
    PRs are done to the 'develop' branch. That means that if you intend to
    contribute to ESAPI, you must be signing your commits. Please see the
    GitHub instructions at
        https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits
    for details.

There are detailed instructions on how to set this up in our ESAPI Release Steps document. Unfortunately, I seem to recall this before. GitHub doesn't warn us about this until after we have 2 people from the team approve the PR. I think it would then probably then force you to go back and set up signing and you'd have to made at least one commit to all that you've modified and then we'd have to get 2 of us to approve it again since there would be a commit after the approval.

So, if you could, could you take care of that. I'm not sure if it would fix it though, because I think ti may require ALL commits to be signed, not just the last one in a PR. (@xeno6696 or @jeremiahjstacey - do either of you know?)

I'm guessing it may not have flagged this because you forked it and created it on your 'issue-844' branch (which, was as instructed). I think there is an option where I can set up our repo to require signed commits on ALL branches, but I didn't do that because I figured it my break things like Dependabot and Snyk generated PRs.

Anyhow, can you see if you can somehow fix this? Thanks.

xeno6696 · 2024-07-06T14:46:05Z

There's no reason we couldn't have a "signature" commit, just a simple merge with a text change detailing it as a signature. Just to get it into history and I think that ought to be sufficient. Going back and resigning old commits is more trouble than its worth.

…

________________________________ From: Kevin W. Wall ***@***.***> Sent: Saturday, July 6, 2024 7:38 AM To: ESAPI/esapi-java-legacy ***@***.***> Cc: Matt Seil ***@***.***>; Mention ***@***.***> Subject: Re: [ESAPI/esapi-java-legacy] Update the logging properties to opt-out of the prefix events #844 (PR #845) @mickeyz07<https://github.com/mickeyz07> - Sorry I didn't notice this before. Jeremiah and I were focused on the code review itself. However, I just noticed that you have not signed your commits with a GPG / PGP key. For commits that will be merged to the 'develop' or 'main' branches, we require those commits to be signed. From our CONTRIBUTING-TO-ESAPI.txt file: A Special Note Regarding Making Commits for PRs Shortly after the 2.5.1.0 ESAPI release in late November 2022, the ESAPI team decided to lock down the 'develop' amd 'main' branches. Merges from PRs are done to the 'develop' branch. That means that if you intend to contribute to ESAPI, you must be signing your commits. Please see the GitHub instructions at https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits for details. There are detailed instructions on how to set this up in our ESAPI Release Steps document.<https://github.com/ESAPI/esapi-java-legacy/blob/develop/documentation/ESAPI-release-steps.pdf> Unfortunately, I seem to recall this before. GitHub doesn't warn us about this until after we have 2 people from the team approve the PR. I think it would then probably then force you to go back and set up signing and you'd have to made at least one commit to all that you've modified and then we'd have to get 2 of us to approve it again since there would be a commit after the approval. So, if you could, could you take care of that. I'm not sure if it would fix it though, because I think ti may require ALL commits to be signed, not just the last one in a PR. ***@***.***<https://github.com/xeno6696> or @jeremiahjstacey<https://github.com/jeremiahjstacey> - do either of you know?) I'm guessing it may not have flagged this because you forked it and created it on your 'issue-844' branch (which, was as instructed). I think there is an option where I can set up our repo to require signed commits on ALL branches, but I didn't do that because I figured it my break things like Dependabot and Snyk generated PRs. Anyhow, can you see if you can somehow fix this? Thanks. — Reply to this email directly, view it on GitHub<#845 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACIQAQL6ZLKRZTPRV43QNTTZK76P5AVCNFSM6AAAAABJ36H722VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMJRG44DKMRVGU>. You are receiving this because you were mentioned.Message ID: ***@***.***>

kwwall · 2024-07-06T14:52:06Z

@xeno6696 - That's sort of what I was thinking of having him do, but I wasn't sure if that would pass the muster of the GitHub signature verification. I thought it required every single commit to be signed if it was to end up on the 'develop' or 'main' branch. That said, maybe it would work if we did a squash-merge? I don't know.

kwwall · 2024-07-06T15:12:34Z

So, @mickeyz07 found a severe must fix issue in your code. I made this simple change to simulate users NOT updating their ESAPI.properties file (which I can guarantee you that many will do):

$ git diff src/test/resources/esapi/ESAPI.properties
diff --git a/src/test/resources/esapi/ESAPI.properties b/src/test/resources/esapi/ESAPI.properties
index 8ffc61f6..a84328ab 100644
--- a/src/test/resources/esapi/ESAPI.properties
+++ b/src/test/resources/esapi/ESAPI.properties
@@ -441,7 +441,7 @@ Logger.ClientInfo=true
 
 # Determines whether ESAPI should log the prefix of [EVENT_TYPE - APPLICATION NAME].
 # If all above Logger entries are set to false, as well as LogPrefix, then the output would be the same as if no ESAPI was used
-Logger.LogPrefix=true
+# Logger.LogPrefix=true
 
 #===========================================================================
 # ESAPI Intrusion Detection

And when I re-ran 'mvn test' I got this:

[ERROR] Tests run: 4256, Failures: 7, Errors: 2434, Skipped: 0

A similar error happens if one sets the value of Logger.LogPrefix to anything other than true or false. They are all throwing ConfigurationExceptions some of which end up as ExceptionInInitializerError. That's why I said it was important that regardless of the value, I want to treat the absence of this property value or any invalid property value as it was in 2.5.4.0. So, it they put in:

Logger.LogPrefix=off

or even something absurd as:

Logger.LogPrefix=scoopy_do

you have to treat is as though they had:

Logger.LogPrefix=true

It's okay if you want to output an error to stdout using logSpecial or whatever it is that we normally use for that, but we can't be throwing exceptions that will kill their application just because they haven't followed instructions. Because many won't.

Make sense?

kwwall

Changes discussed in #845 (comment) are required before this will be approved.

… fifth iteration

mickeyz07 · 2024-07-10T16:43:31Z

@kwwall @jeremiahjstacey I have made changes and signed the commit with GPG key.
Currently if any of the Logger.* properties is missing or it has value different then true, false, yes, no the tests are failing.
So to reiterate - the problem is not only with the Logger.LogPrefix.
As you could see in the code I have made provision for all Logger.* properties.
Only Logger.LogEncodingRequired=false.
All other Boolean Logger.* properties have the default value of true.
Not sure if you require to extend that behavior to other properties?
So if the value for Boolean properties is not valid, instead of throwing an exception, now reverts the value for the property to the default value?
In that case we could have another function with additional default parameter:
public Boolean getBooleanProp(String propertyName, boolean value)
That would require additional testing.
So for this PR, the changes that I made are taking care of the Logger.* properties.
All tests that Kevin "broke" with commenting out Logger.* are now working correctly.

kwwall

Not going to approve this unless Matt or Jeremiah find this kludge acceptable.

kwwall · 2024-07-14T18:23:03Z

src/main/java/org/owasp/esapi/reference/DefaultSecurityConfiguration.java

            String property = properties.getProperty( propertyName );
            if ( property == null ) {
+                if (propertyName.startsWith("Logger.")) {


@mickeyz07 -- IMO, this is a major implementation kludge to me. Generally, I was expecting in whatever static initializer that checks the value for the Logger.LogPrefix property, that any error / exception that would be thrown that you would just treat it as 'true' and then log something via logSpecial. If for some reason that approach turns out to be impossible, then the next best alternative would be to create a special dedicated getLogPrefix method. Another preferred alternative (something I've been meaning to do for a while now) is to create a new Boolean getBooleanProp(String propName, boolean default) and use that, and just pass in 'true' for the default, so if the specified property name is not a boolean, it defaults to the specified default value (which should be true for Logger.LogPrefix. I am also very confused why this is even written the way that it it; it appears that you could omit the entire:

if (propertyName.startsWith("Logger.")) {

and just went with the specific property name check that follows. (And why this specific check? I would have expected Logger.LogPrefix here, since that's the new property. So why Logger.LogEncodingRequired? I'm confused.)

@jeremiahjstacey and @xeno6696 - I'd like your opinion on this, but I don't like this at all. Can either of you think of any alternatives?

I think I agree @kwwall - Updates in this file will lead to additional scope that I think we'll be better off not trying to tackle in this effort.

@mickeyz07 - My understanding of what the desire here is to try to solve the problem at the lowest level possible. I think I agree with the intent, but I think you can agree that it's not the cleanest or most maintainable due to current restrictions in the library's interfaces.
To facilitate a faster integration of this feedback, I would like to propose the following:

Revert all changes to DefaultSecurityConfiguration.java

Update JavaLogFactory and SLF4JLogFactory implementations to default to TRUE, attempt to read from properties, and useLogSpecial on failure

boolean logPrefix = true; try { logPrefix = ESAPI.securityConfiguration().getBooleanProp(LOG_PREFIX); } catch (ConfigurationException ex) { ESAPI.securityConfiguration().logSpecial("Failed to read Log Prefix configuration. Defaulting to enabled. ", ex); }

It's not the best possible solution, but I think that's the best we can do in the current library implementation.

Okay, my thoughts, exactly, and what you described was my preferred approach at resolving this.

@mickeyz07 - Please make the change that @jeremiahjstacey detailed above. Thanks.

kwwall

Changes requested. See previous comments by Jeremiah and myself. It should be pretty straightforward since he pretty much laid out the code changes.

… sixt iteration

mickeyz07 · 2024-07-24T10:55:27Z

@kwwall @jeremiahjstacey I have added the new method:
Boolean getBooleanProp(String propertyName, Boolean defaultValue)
I have used that method only for Logger.LogPrefix, e.g.
boolean logServerIp = ESAPI.securityConfiguration().getBooleanProp(LOG_SERVER_IP);
boolean logPrefix = ESAPI.securityConfiguration().getBooleanProp(LOG_PREFIX, true);
All the tests run successfully for the following values:

Logger.LogPrefix=true
#Logger.LogPrefix=true
Logger.LogPrefix=blabla

kwwall · 2024-07-26T00:39:31Z

@mickeyz07 We are getting close, so please don't give up. But there are 2 things that I see missing right out of the gate:

You need to add the new getBooleanProp to src/test/java/org/owasp/esapi/SecurityConfigurationWrapper.java
You need to add some JUnit tests for the new method. Probably put those tests in the following files where other tests for the original getBooleanProp method resides:
- src/test/java/org/owasp/esapi/configuration/EsapiPropertyManagerTest.java
- src/test/java/org/owasp/esapi/configuration/StandardEsapiPropertyLoaderTest.java
- src/test/java/org/owasp/esapi/configuration/XmlEsapiPropertyLoaderTest.java

… seventh iteration

mickeyz07 · 2024-07-31T12:43:25Z

@kwwall I have added the required tests for all 3 configuration test files you noted. Please check.

kwwall · 2024-08-01T02:25:32Z

src/main/java/org/owasp/esapi/configuration/EsapiPropertyLoader.java

@@ -33,6 +33,13 @@ public interface EsapiPropertyLoader {
     */
    public Boolean getBooleanProp(String propertyName) throws ConfigurationException;

+    /**
+     * Get any Boolean type property from security configuration.
+     * If property does not exist in configuration or has incorrect type, defaultValue is returned


From the Javadoc: for this interface: "or has incorrect type".

Well, that is the $64k question, isn't it? What is the INTUITIVE behavior if one calls something like:

Boolean propValue = getBooleanProp("SomePropName", true)

but somewhere, say in ESAPI.properties or elsewhere, we have defined

SomePropName=This is not a boolean property

Then what is the expected / intuitive behavior for that call? I think that I'd expect a ConfigurationException to be thrown rather than having propValue silently being set to true. Or maybe I shouldn't say that is the intuitive behavior, but rather the safe behavior. I think because copy/paste errors abound and it would be easy to make a typo with the property name, that's the only way that we're going to help people catch these bizarre accidental edge cases. Otherwise, we're going to cause them to curse at us because they'll have to figure up a debugger to figure out what the hell is going on in cases like this.

@xeno6696 and @jeremiahjstacey - I'd like your opinion here. I know this requires yet another code change, but if we do a release and document it this way this (likely bug, IMO) will become a feature because changing it later on would break backward compatibility. If we were requiring JDK 9 or later we could just not export this method and keep it hidden, but since Java 8 is the minimal version, once we add this, it becomes available to the world.

I personally am leaning towards only using the default value if the property is not set at all; that is if

System.getProperty("SomePropValue");

returns null, but I'd like to know what the 2 of you think. If you both think it's more intuitive have it return the default boolean value if the property name is found to have a non-Boolean value I will reluctantly go along with that.

Overall, I think this is more scope than we should put time into to modify the opt-out request from the original issue. I appreciate the initiative; however, we're adding this case to expand for something that could have been solved in two copies of an 6 line block.

This is overengineering in this scope. We're blocking value, and a new possible release for this API change.

My suggestion is to take it out. Use the duplicate snippet from my previous comment in the 2 factories and revert the changes in the DefaultSecurityConfiguration. Create another task for the default API that we can address (reuse this code if you want), and complete it in a non-blocking scope. We don't need this to fix the original problem, and as you can see the discussion of edge cases and caveats will continue to grow. Let's get the opt-out behavior in in a first form, then improve upon it in another task to expand the SecurityConfiguration API to allow defaults to be specified.

I concur. If in the future, you want to do a new PR to get a new interface for

public Boolean getBooleanProp(String propertyName, Boolean defaultValue) throws ConfigurationException

I am fine with that, but as my review of this as surfaced, this is a little more nuanced than intuition alone might have us believe. If we are going to add that a future time, I think we need to start out with a GitHub issue and discuss the specifications there first, before waiting for an implementation to discuss them. So, to proceed with this PR, please rip that part of the code out and follow Jeremiah's suggestion.

kwwall · 2024-08-01T02:44:01Z

src/main/java/org/owasp/esapi/configuration/EsapiPropertyManager.java

+    @Override
+    public Boolean getBooleanProp(String propertyName, Boolean defaultValue) {
+        for (AbstractPrioritizedPropertyLoader loader : loaders) {
+            return loader.getBooleanProp(propertyName, defaultValue);


I think the way this is currently implemented, I don't think it will ever reach the final return on line 87 here. Maybe I'm wrong about that. Do we have a test for that case? Of course, if we change it as per my previous comment, we may still need it. Or may need to add a try / catch here and just re-throw any ConfigurationException.

kwwall · 2024-08-01T02:49:29Z

src/main/java/org/owasp/esapi/configuration/StandardEsapiPropertyLoader.java

+        if (property.equalsIgnoreCase("false") || property.equalsIgnoreCase("no")) {
+            return false;
+        }
+        return defaultValue;


Depending on if we decide to change the behavior here in accordance to my first comment, we may wish to change line 88 to something along the following lines:

if ( property != null ) { throw new ConfigurationException("Property name '" + propertyName + "' has non-Boolean value of '" + property + "'."); }

kwwall · 2024-08-01T02:50:41Z

src/main/java/org/owasp/esapi/configuration/XmlEsapiPropertyLoader.java

+        if (property.equalsIgnoreCase("false") || property.equalsIgnoreCase("no")) {
+            return false;
+        }
+        return defaultValue;


Ditto here. Similar to comment on StandardEsapiPropertyLoader.java.

kwwall · 2024-08-01T03:18:13Z

src/main/java/org/owasp/esapi/reference/DefaultSecurityConfiguration.java

+            if ( property.equalsIgnoreCase("false") || property.equalsIgnoreCase("no") ) {
+                return false;
+            }
+            return defaultValue;


Ditto comment in EsapiPropertyManager.java about throwing ConfigurationException here.

kwwall

@mickeyz07 - I'm holding off on requesting changes until I get @xeno6696 and @jeremiahjstacey feedback about how getBooleanProp(String, Boolean) should treat a property that it encounters that has a non-Boolean value. I think we need to resolve that first. If we decide to do that, I will give you the option to finish it up, but if you don't, I'll probably approve this and then do those final changes myself in a separate PR. But getting this behavior of this new method is important and we'll likely only get one chance at getting it right.

@xeno6696 and @jeremiahjstacey -- waiting to hear back from you on this.

kwwall · 2024-08-01T03:20:27Z

src/test/java/org/owasp/esapi/configuration/EsapiPropertyManagerTest.java

+        // then
+        assertEquals(expectedPropertyValue, propertyValue);
+    }
+


If we decide to revise this behavior to throw a ConfigurationException if getBooleanProperty(String, Boolean) is called and a non-Boolean property if found, we will need a new test here.

Ditto in StandardEsapiPropertyLoaderTest.java and XmlEsapiProertperLoaderTest.java.

… eigth iteration

kwwall

Two lines where I'd like to see some very minor tweaks, otherwise LGTM.

kwwall · 2024-09-05T03:51:13Z

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

+        try {
+            logPrefix = ESAPI.securityConfiguration().getBooleanProp(LOG_PREFIX);
+        } catch (ConfigurationException ex) {
+            System.out.println("ESAPI: Failed to read Log Prefix configuration. Defaulting to enabled" +


Please change the output to something that explicitly mentions the property name on the output sent to stderr. E.g., something like this:

System.out.println("ESAPI: Failed to read Log Prefix configuration, " + LOG_PREFIX + ". Defaulting to enabled" +

That will make it easier for those getting this failure to know exactly what property to look for if they wish to change it in their ESAPI.properties file.

kwwall · 2024-09-05T03:51:50Z

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

+        try {
+            logPrefix = ESAPI.securityConfiguration().getBooleanProp(LOG_PREFIX);
+        } catch (ConfigurationException ex) {
+            System.out.println("ESAPI: Failed to read Log Prefix configuration. Defaulting to enabled" +


Same as previous comment.

… ninth iteration

kwwall

LGTM.

jeremiahjstacey

Looks good to me as well

kwwall · 2024-09-05T23:13:46Z

I'm going to go ahead and bypass the 'branch protection' and do the 'squash and merge' because @mickeyz07 signed most of his commits and we reviewed all the code, so someone usurped his GitHub username and snuck something malicious past us that's one us.

Update the logging properties to opt-out of the prefix events ESAPI#844

8b68f75

jeremiahjstacey reviewed Jun 25, 2024

View reviewed changes

configuration/esapi/ESAPI.properties Outdated Show resolved Hide resolved

kwwall reviewed Jun 26, 2024

View reviewed changes

jeremiahjstacey reviewed Jun 26, 2024

View reviewed changes

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

jeremiahjstacey reviewed Jun 26, 2024

View reviewed changes

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

Update the logging properties to opt-out of the prefix events, second…

d3f2a2f

… iteration for ESAPI#844

jeremiahjstacey reviewed Jun 28, 2024

View reviewed changes

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

jeremiahjstacey reviewed Jun 28, 2024

View reviewed changes

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

jeremiahjstacey reviewed Jun 28, 2024

View reviewed changes

src/test/java/org/owasp/esapi/logging/appender/LogPrefixAppenderTest.java Outdated Show resolved Hide resolved

mickeyz07 added 2 commits July 1, 2024 14:23

Update the logging properties to opt-out of the prefix events, third …

6d8f307

…iteration

Update the logging properties to opt-out of the prefix events ESAPI#844…

7822d6f

… fourt iteration

kwwall requested changes Jul 6, 2024

View reviewed changes

Update the logging properties to opt-out of the prefix events ESAPI#844…

2e2d0e5

… fifth iteration

kwwall reviewed Jul 14, 2024

View reviewed changes

kwwall requested changes Jul 14, 2024

View reviewed changes

mickeyz07 and others added 2 commits July 16, 2024 17:00

Merge branch 'ESAPI:develop' into issue-844

9ca8473

Update the logging properties to opt-out of the prefix events ESAPI#844…

c9d5b78

… sixt iteration

Update the logging properties to opt-out of the prefix events ESAPI#844…

54a7463

… seventh iteration

kwwall reviewed Aug 1, 2024

View reviewed changes

Update the logging properties to opt-out of the prefix events ESAPI#844…

17219b7

… eigth iteration

kwwall requested changes Sep 5, 2024

View reviewed changes

Update the logging properties to opt-out of the prefix events ESAPI#844…

0e9bb76

… ninth iteration

kwwall approved these changes Sep 5, 2024

View reviewed changes

jeremiahjstacey approved these changes Sep 5, 2024

View reviewed changes

kwwall merged commit 3a78d6d into ESAPI:develop Sep 5, 2024
2 checks passed

This was referenced Oct 5, 2024

Update the logging properties to opt-out of the prefix events #844

Closed

Release prep 2.5.5.0 #856

Merged

Update the logging properties to opt-out of the prefix events #844 #845

Update the logging properties to opt-out of the prefix events #844 #845

Conversation

mickeyz07 commented Jun 25, 2024 • edited Loading

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

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

jeremiahjstacey commented Jul 1, 2024

kwwall commented Jul 4, 2024

kwwall commented Jul 6, 2024

xeno6696 commented Jul 6, 2024 via email

kwwall commented Jul 6, 2024

kwwall commented Jul 6, 2024

kwwall left a comment

Choose a reason for hiding this comment

mickeyz07 commented Jul 10, 2024

kwwall left a comment

Choose a reason for hiding this comment

kwwall Jul 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwwall left a comment

Choose a reason for hiding this comment

mickeyz07 commented Jul 24, 2024 • edited Loading

kwwall commented Jul 26, 2024

mickeyz07 commented Jul 31, 2024

kwwall Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwwall Aug 1, 2024 • 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

kwwall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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 left a comment

Choose a reason for hiding this comment

jeremiahjstacey left a comment

Choose a reason for hiding this comment

kwwall commented Sep 5, 2024

mickeyz07 commented Jun 25, 2024 •

edited

Loading

kwwall Jul 14, 2024 •

edited

Loading

mickeyz07 commented Jul 24, 2024 •

edited

Loading

kwwall Aug 1, 2024 •

edited

Loading

kwwall Aug 1, 2024 •

edited

Loading