PR to fix #824 and reference to #823 #828
Conversation
…hat the method takes into consideration canonicalization of mixed/multi encoded URLs as specified in ESAPI.props 'allowMixed' and 'allowMultiple' accordingly.
…javadoc to indicate that the method takes into consideration canonicalization of mixed/multi encoded URLs as specified in ESAPI.props 'allowMixed' and 'allowMultiple' accordingly.
…se block of the check to see whether or not we were dealing with a query segment.
|
@kwwall and @jeremiahjstacey It's my turn to wait for y'all :-D |
|
@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! |
There was a problem hiding this comment.
A few comments, but no showstoppers to keep me from approving this. Address them or not or see what @jeremiahjstacey thinks.
Lastly, here's some additional comments that I had that are on things that you didn't change as part of this PR and thus I couldn't comment them inline in the normal way. (Or if I could, it didn't seem to work for me.)
For
why did you use 'sg' for the object variable name instead of the more logical variable name of 'sc'?Also, for
, shouldn't you just use the default port for anything less than 0 and not just if it is "-1"?I mean, in theory, these may not be legitimate URIs, but could be something that a user enters and the application is trying to canonicalize, so an adversary might try passing something like
https://example.com:-42/douglas/adams?life=universe&everyrhing
and theyjust hope that that will break something somewhere. So any port < 0 is
invalid, right? Also may need to adjust line 557 to look for values < 0 for the
port rather than just equal to -1. Based on the method's Javadoc, it does mention that it doesn't test to see if the URI is RFC 3986 compliant, but this seems like some low hanging fruit to me, so maybe you want to consider it.
Also, I pity the fool who tries to write a regex to verify URIs being compliant with RFC 3986. It's hard enough just trying to do that with an http/https URL.
| @@ -48,4 +49,23 @@ public void testMixedBmpAndNonBmp(){ | |||
| String input = bmp + nonBMP; | |||
| assertEquals(expected, codec.encode(new char[0], input)); | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
How about you move this @test annotation to AFTER the comment since the comment refers to the next two tests?
| @@ -747,6 +742,7 @@ public void testDecodeFromURL() throws Exception { | |||
| fail(); | |||
| } | |||
| try { | |||
| //FIXME: Rewrite this to use expected Exceptions. | |||
There was a problem hiding this comment.
I figure at this point we might as well wait until we put off these FIXMEs and TODOs until 3SAPI, especially for the JUnit tests. I mean we have tons of them in the ESAPI source code and haven't even addressed them there yet.
| @@ -520,6 +520,9 @@ public byte[] decodeFromBase64(String input) throws IOException { | |||
| * This will extract each piece of a URI according to parse zone as specified in <a href="https://www.ietf.org/rfc/rfc3986.txt">RFC-3986</a> section 3, | |||
| * and it will construct a canonicalized String representing a version of the URI that is safe to | |||
| * run regex against. | |||
| * | |||
| * NOTE: This method will obey the ESAPI.properties configurations for allowing | |||
| * Mixed and Multiple Encoding URLs. | |||
There was a problem hiding this comment.
Was that not the intent and expectation all along? If not, they this will result in unexpected behavior because I think that is not intuitive and therefore it should probably be a WARNING rather than just a NOTE.
OTOH, if this was the intended behavior all along (which I think is the case), then I would contend that this NOTE is probably not needed because it is likely to be confusing to those reading the Javadoc because all you are really saying is "Hey, this now works like it was originally intended for relative URIs but it didn't before because of a bug that was present. See GitHub issues #823 and #824." So, I don't think we really need this comment in the Javadoc. I think it will cause confusion and cause people to ask "why is this here; isn't that obvious?".
Therefore, my recommendation is either delete it, or if you want to keep it change it to a non-Javadoc comment and reference the issues.
Make take on this
| System.out.println(uri.toString()); | ||
| assertEquals(expectedUri, e.getCanonicalizedURI(uri)); | ||
|
|
||
| } |
There was a problem hiding this comment.
Think there is a trailing tab on line 1013.
| assertEquals(expectedUri, e.getCanonicalizedURI(uri)); | ||
|
|
||
| } | ||
| public void testGetCanonicalizedUriWithMultQueryParams() throws Exception { |
There was a problem hiding this comment.
And looks like a leading tab here as well.
|
|
||
| String expectedUri = "http://palpatine@foo bar.com/path_to/resource?foo=bar&bar=foo#frag"; | ||
| //Please note that section 3.2.1 of RFC-3986 explicitly states not to encode | ||
| //password information as in http://palpatine:[email protected], and this will |
There was a problem hiding this comment.
Changing the 'foo.com' to 'bar.com' would make it better align with the code on line 1022. Also, I thought the RFCs were changed so that only ftp and ftps schemes supported passwords. I thought the support for passwords for http / https was dropped a while ago.
| Encoder e = ESAPI.encoder(); | ||
|
|
||
| String expectedUri = "/webapp/ux/home?d=1705914006565&status=login&ticket=1705914090394_HzJpTROVfhW-JhRW0OqDbHu7tWXXlgrKSUmOzIMsZNCcUIiYGMXX_Q==&newsess=false&roleid=DP010101/0007&origin=ourprogram"; | ||
| //Please note that section 3.2.1 of RFC-3986 explicitly states not to encode |
There was a problem hiding this comment.
Note sure why this comment is relevant to this particular test case. Am I missing something or was this just a remnant of a copy/paste error?
| assertEquals(expectedUri, e.getCanonicalizedURI(uri)); | ||
|
|
||
| } | ||
|
|
|
|
||
| } | ||
|
|
||
| @org.junit.Ignore("Pre-check in unit test for issue #826") |
There was a problem hiding this comment.
See comment re: the Ignore annotation as above.
| } | ||
|
|
||
| @org.junit.Ignore("Pre-check in unit test for issue #826") | ||
| public void Issue826GetCanonicalizedDoubleAmpersand() throws Exception { |
There was a problem hiding this comment.
Leading tab character here.
|
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. |
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.