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
598 stars 363 forks source link

PR to fix #824 and reference to #823 #828

Closed xeno6696 closed 1 month ago

xeno6696 commented 5 months ago

Couple of bugs discovered with DefaultEncoder.getCanonicalizedURI(URI) where

1.) We weren't fully handling relative URLs 2.) A canonicalize call was occurring twice always, when logically the intent was to treat queries as their own special case of canonicalization. Logic now splits between checking if we're dealing with a URL query and does not canonicalize twice.

The prior bug made ESAPI prone to false positives depending on whether or not there was the presence of URL queries that were in collision with named HTMLEntities.

xeno6696 commented 4 months ago

@kwwall and @jeremiahjstacey It's my turn to wait for y'all :-D

kwwall commented 4 months ago

@xeno6696 - I will look at this tonight, but do we have a corresponding GH issue that we can reference in this PR? If not, could you take a crack at writing up a short one. I usually reference the closed GH issues in our release notes so this will make it easier for me not to forget about this important fix. Thanks.

xeno6696 commented 4 months ago

@xeno6696 - I will look at this tonight, but do we have a corresponding GH issue that we can reference in this PR? If not, could you take a crack at writing up a short one. I usually reference the closed GH issues in our release notes so this will make it easier for me not to forget about this important fix. Thanks.

Yeah... the issue number is in the PR title.

kwwall commented 4 months ago

@xeno6696 - I will look at this tonight, but do we have a corresponding GH issue that we can reference in this PR? If not, could you take a crack at writing up a short one. I usually reference the closed GH issues in our release notes so this will make it easier for me not to forget about this important fix. Thanks.

Yeah... the issue number is in the PR title.

Duh. Sorry. Missed that; even the 2nd time I looked at it, I saw that issue #823 was already closed and neglected to read the issue to the end where you referenced #824. That's what I get for trying to respond while taking a quick coffee break. Guess I should have waited until after the coffee kicked in!

kwwall commented 1 month ago

I'm going to go ahead and approve this and assume that @jeremiahjstacey is okay with that decision so we I can get it into the next release. We can clean up the test in a later follow-up, but I don't see that as a showstopper.