Skip to content

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

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

Merged
kwwall merged 10 commits into from
Sep 5, 2024
Merged

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

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8b68f75
Update the logging properties to opt-out of the prefix events #844
mickeyz07 Jun 25, 2024
d3f2a2f
Update the logging properties to opt-out of the prefix events, second…
mickeyz07 Jun 28, 2024
6d8f307
Update the logging properties to opt-out of the prefix events, third …
mickeyz07 Jul 1, 2024
7822d6f
Update the logging properties to opt-out of the prefix events #844 fo…
mickeyz07 Jul 1, 2024
2e2d0e5
Update the logging properties to opt-out of the prefix events #844 fi…
mickeyz07 Jul 10, 2024
9ca8473
Merge branch 'ESAPI:develop' into issue-844
mickeyz07 Jul 16, 2024
c9d5b78
Update the logging properties to opt-out of the prefix events #844 si…
mickeyz07 Jul 24, 2024
54a7463
Update the logging properties to opt-out of the prefix events #844 se…
mickeyz07 Jul 31, 2024
17219b7
Update the logging properties to opt-out of the prefix events ESAPI#8…
mickeyz07 Sep 3, 2024
0e9bb76
Update the logging properties to opt-out of the prefix events ESAPI#8…
mickeyz07 Sep 5, 2024
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
Prev Previous commit
Next Next commit
Update the logging properties to opt-out of the prefix events #844 fi… 
…fth iteration
  • Loading branch information
@mickeyz07
mickeyz07 committed Jul 10, 2024
commit 2e2d0e5d3488711ed31a92491f400d499ae3596b
18 changes: 18 additions & 0 deletions src/main/java/org/owasp/esapi/reference/DefaultSecurityConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1441,8 +1441,17 @@ public Boolean getBooleanProp(String propertyName) throws ConfigurationException
try {
return esapiPropertyManager.getBooleanProp(propertyName);
} catch (ConfigurationException ex) {

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

@kwwall kwwall Jul 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@jeremiahjstacey jeremiahjstacey Jul 14, 2024

Choose a reason for hiding this comment

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

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:

  1. Revert all changes to DefaultSecurityConfiguration.java
  2. 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.

Copy link
Contributor

@kwwall kwwall Jul 14, 2024

Choose a reason for hiding this comment

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

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.

if (propertyName.equals("Logger.LogEncodingRequired")) {
return Boolean.FALSE;
}
else {
return Boolean.TRUE;
}
}
throw new ConfigurationException( "SecurityConfiguration for " + propertyName + " not found in ESAPI.properties");
}
if ( property.equalsIgnoreCase("true") || property.equalsIgnoreCase("yes" ) ) {
Expand All @@ -1451,6 +1460,15 @@ public Boolean getBooleanProp(String propertyName) throws ConfigurationException
if ( property.equalsIgnoreCase("false") || property.equalsIgnoreCase( "no" ) ) {
return false;
}

if (propertyName.startsWith("Logger.")) {
if (propertyName.equals("Logger.LogEncodingRequired")) {
return Boolean.FALSE;
}
else {
return Boolean.TRUE;
}
}
throw new ConfigurationException( "SecurityConfiguration for " + propertyName + " has incorrect " +
"type");
}
Expand Down