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

[mickeyz07]

Determines whether ESAPI should log the prefix of [EVENT_TYPE - APPLICATION NAME]. If all Logger.* settings in ESAPI.properties file are set to false and Logger.LogIgnorePrefix is set to true, then the log output would be the same like 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 is having benefit of sanitizing untrusted data used to construct log entries by using a safe logging mechanism in the OWASP ESAPI Logger without any extra logging inofrmation. However, only with changing couple of settings in ESAPI.properties file, a user can have a very detailed log if required.

[jeremiahjstacey]

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

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

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

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)

@mickeyz07https://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 @jeremiahjstaceyhttps://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 GitHubhttps://github.com/ESAPI/esapi-java-legacy/pull/845#issuecomment-2211785255, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACIQAQL6ZLKRZTPRV43QNTTZK76P5AVCNFSM6AAAAABJ36H722VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMJRG44DKMRVGU. You are receiving this because you were mentioned.Message ID: @.***>

[kwwall]

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

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?