search

ESAPI / esapi-java-legacy

ESAPI (The OWASP Enterprise Security API) is a free, open source, web application security control library that makes it easier for programmers to write lower-risk applications.
https://owasp.org/www-project-enterprise-security-api/
Other
603 stars 364 forks source link

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

Closed JosephWitthuhnTR closed 1 year ago

JosephWitthuhnTR commented 1 year ago

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: https://github.com/ESAPI/esapi-java-legacy/discussions/768#discussioncomment-4672270

jeremiahjstacey commented 1 year ago

I have no concerns with the code or tests as written.

I think the value of this update is more related to the intended content of the next ESAPI release.

@kwwall -- do we plan on addressing the javax/jakarta package updates in any other way for the next ESAPI release?

If we do, then this change does not matter. The order of the logic isn't going to make a difference if we plan to account for it another way. It is change for the sake of change, which I tend to question.

OTOH if we want to create a release with this content before any other updates are made then it will provide the value that @JosephWitthuhnTR has identified.

Either way the code can be merged, depending on the release plan I'm not sure it should be due to the possible overlap with the intended value.

@JosephWitthuhnTR - I do not intend to discount your contribution. I appreciate the fact that you're calling out in the PR description this is not a long-term fix for the problem, as well as the fact that you are invested enough to make time to offer the changes back to the project. Thank you for your feedback and contributions to the project.

JosephWitthuhnTR commented 1 year ago

@kwwall We've tested our application with this after making this modification to our JAR, and we don't run into any other issues. Again, this is for the limited use case where we are just using the logging part of the ESAPI library and we turn the IP logging off. So I am reasonably sure that this fixes our use case (and also pretty certain that it doesn't address the larger problem). What I can't speak to is how many other users (if any) use the JAR for as limited of a use case as we do. This could either fix it for nobody else, or it could help a bunch of people. I did notice that the other individual who submitted the bug (issue #767 and discussion #768) (@guadgarcia) was running into the exact same stack trace / use case as our team was, so it seems that it may help them too.

@jeremiahjstacey I 100% agree. If a new release with the larger fix is "imminent" or "soon", then there is no reason for this modification (and no reason to merge this PR). If that is still weeks/months off, this change "lets us get by" with a smaller fix in the meantime while we wait for the larger fix.

kwwall commented 1 year ago

@JosephWitthuhnTR - While I approve this, it would be nice if you could sign your commit with your GPG key, If that is too much trouble, I can bypass that, but Ive added it as part of the branch protection so ideally you can sign a commit that would be better.

JosephWitthuhnTR commented 1 year ago

@kwwall I've made a mess of this trying to get this right. I'll make a new PR.