Skip to content
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.

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

remove dependency on servlet API (use case where not needed) #776

Merged
merged 2 commits into from
Feb 6, 2023

Conversation

JosephWitthuhnTR
Copy link
Contributor

I have recreated PR #772, except this time I have properly signed the commit.

This change deals with issue #767.

It does not attempt to resolve the overall issue with Servlet API version compatibility. What it does is remove the dependency on Servlet API for the use case where a user is trying to use the logger functionality, but has "logServerIP" set to false. When not logging the server IP, there is no reason for this code to depend on Servlet API at all.

This allows us to avoid runtime exceptions if we use this code with the new Jakarta package version of Servlet API, in cases where logServerIP is disabled.

Again, this is not a fix for the overall problem. This is a small change that will let some limited use cases not fail due to the problem (so that those use cases can get by until this problem is solved in a more holistic way).

The change stems from part of this discussion:
#768 (comment)

@JosephWitthuhnTR
Copy link
Contributor Author

@kwwall / @jeremiahjstacey I have recreated PR #772 , except this time I have properly signed the commit.

Copy link
Contributor

@kwwall kwwall left a comment

Choose a reason for hiding this comment

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

Approved. See comments on withdraw PR #772

@kwwall
Copy link
Contributor

kwwall commented Feb 6, 2023

@JosephWitthuhnTR - Looks like this is failing for some reason on one of the surefire plugins. I don't have time to investigate it at lunch time. Will try to look at it later today.

@JosephWitthuhnTR
Copy link
Contributor Author

@kwwall The failure looks related to my change. I'll investigate:

ServerInfoSupplierTest.verifyOutputNullAppName:84 expected:<[LOCAL_ADDR:99999]/null/verifyOutputNu...> but was:<[]/null/verifyOutputNu...>

@JosephWitthuhnTR
Copy link
Contributor Author

JosephWitthuhnTR commented Feb 6, 2023

@kwwall I had it right on my original PR, but somehow messed it up when I recreated this PR with the signed commit. Either way, I've now fixed the unit test on this PR.

@kwwall
Copy link
Contributor

kwwall commented Feb 6, 2023

@kwwall I had it right on my original PR, but somehow messed it up when I recreated this PR with the signed commit. Either way, I've now fixed the unit test on this PR.

Dang. I should have probably looked closer, but I assumed you just copy/pasted from the old one or used the same old files, so my bad. Guess that;s why we have these GitHub actions as a sanity check.

@kwwall kwwall merged commit 3298247 into ESAPI:develop Feb 6, 2023
@ebarry-vp
Copy link

Any idea on when this fix will be released soon? Thanks!

@kwwall
Copy link
Contributor

kwwall commented Mar 11, 2023

The bottleneck for this before doing this next new release is that I need to write up:

  1. Release note (generally takes 3+ hours)
  2. Write up a security bulletin in regards to the CVE-2023-24998 vulnerability from the commons-fileupload-1.4.jar dependency. (Probably also at least 3 hrs or more, depending on how long it takes me to analyze their commons-fileupload code to see if the CVE is exploitable through the way that ESAPI uses it.

Thus far, I have not be able to find enough free time to do either of those things and I'm not going to do a release until I can produce those artifacts as part of the new release. And given that tax time is upon us, that's something else that I need to attend to so free time is scarce right now.

P.S.- In reality, I think you should be able to work around the problem you were having by just changing the ESAPI logger to use SLF4J rather than using the default JUL for logging.

@ebarry-vp
Copy link

P.S.- In reality, I think you should be able to work around the problem you were having by just changing the ESAPI logger to use SLF4J rather than using the default JUL for logging.

@kwwall - Please can you help us how to do this work around to avoid this error: java.lang.NoClassDefFoundError: javax/servlet/ServletResponse

I tried using this: ESAPI.Logger=org.owasp.esapi.logging.slf4j.Slf4JLogFactory
That still didn't prevent the code from running into this issue.

We really need the jakarta changes and folks that need javax, can still point to older versions in my opinion. Any thoughts?

Thanks!

@kwwall
Copy link
Contributor

kwwall commented Mar 13, 2023

@ebarry-vp and others - Disregard that last remark about using SLF4J. My memory was not serving me well and I mistakenly thought that the class that was the PR was for was one belonging to the org.owasp.esapi.logging.java package which would have limited the effect to JUL, but it was not. The PR was for the class org.owasp.esapi.logger.appender.ServerInfoSupplier.

However, the Maven Shade approach that @planetlevel mentioned here or the Jakarta EE transformer that @arjantijms mentioned here for Eclipse ought to theoretically work. If those approaches don't work, then a lot of developers are going to be totally screwed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants