PR to fix #824 and reference to #823 #828
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| import static org.junit.Assert.assertEquals; | ||
|
|
||
| import org.junit.Ignore; | ||
| import org.junit.Test; | ||
|
|
||
| public class HTMLEntityCodecTest { | ||
|
|
@@ -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? |
||
| /** | ||
| * TODO: The following methods are unit tests I'm checking in for an issue to be worked and fixed. | ||
| */ | ||
| @Ignore("Pre check-in for issue #827") | ||
| public void testIssue827() { | ||
| String input = "/webapp/ux/home?d=1705914006565&status=login&ticket=1705914090394_HzJpTROVfhW-JhRW0OqDbHu7tWXXlgrKSUmOzIMsZNCcUIiYGMXX_Q%3D%3D&newsess=false&roleid=DP010101/0007&origin=ourprogram"; | ||
| String expected = input; | ||
| assertEquals(expected, codec.decode(input)); | ||
| } | ||
|
|
||
| @Test | ||
| @Ignore("Pre check-in for issue #827") | ||
| public void testIssue827OnlyOR() { | ||
| String input = "&origin=ourprogram"; | ||
| String expected = input; | ||
| assertEquals(expected, codec.decode(input)); | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,25 +15,21 @@ | |
| */ | ||
| package org.owasp.esapi.reference; | ||
|
|
||
| import static org.junit.Assert.assertNotEquals; | ||
|
|
||
| import java.io.IOException; | ||
| import java.io.UnsupportedEncodingException; | ||
| import java.net.URI; | ||
| import java.util.ArrayList; | ||
| import java.util.Arrays; | ||
| import java.util.List; | ||
|
|
||
| import org.junit.Ignore; | ||
| import org.owasp.esapi.ESAPI; | ||
| import org.owasp.esapi.Encoder; | ||
| import org.owasp.esapi.EncoderConstants; | ||
| import org.owasp.esapi.SecurityConfiguration; | ||
| import org.owasp.esapi.SecurityConfigurationWrapper; | ||
| import org.owasp.esapi.codecs.Codec; | ||
| import org.owasp.esapi.codecs.HTMLEntityCodec; | ||
| import org.owasp.esapi.codecs.MySQLCodec; | ||
|
|
@@ -45,8 +41,7 @@ | |
| import org.owasp.esapi.errors.EncodingException; | ||
| import org.owasp.esapi.errors.IntrusionException; | ||
| import org.owasp.esapi.Randomizer; | ||
|
|
||
|
|
||
| import junit.framework.Test; | ||
| import junit.framework.TestCase; | ||
|
|
@@ -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. |
||
| instance.decodeFromURL( "%3xridiculous" ); | ||
| fail(); | ||
| } catch( Exception e ) { | ||
|
|
@@ -985,6 +981,50 @@ public void testGetCanonicalizedUri() throws Exception { | |
| assertEquals(expectedUri, e.getCanonicalizedURI(uri)); | ||
|
|
||
| } | ||
|
|
||
| public void testGetCanonicalizedUriWithAnHTMLEntityCollision() throws Exception { | ||
| System.out.println("GetCanonicalizedUriWithAnHTMLEntityCollision"); | ||
| Encoder e = ESAPI.encoder(); | ||
|
|
||
| String expectedUri = "http://[email protected]/path_to/resource?foo=bar¶1=test"; | ||
| //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 | ||
| //not appear in the userinfo field. | ||
| String input = "http://[email protected]/path_to/resource?foo=bar¶1=test"; | ||
| URI uri = new URI(input); | ||
| System.out.println(uri.toString()); | ||
| assertEquals(expectedUri, e.getCanonicalizedURI(uri)); | ||
|
|
||
| } | ||
|
|
||
| @org.junit.Ignore("Pre-check in unit test for issue #826") | ||
|
There was a problem hiding this comment. Are you eventually planning on removing this Ignore annotation? |
||
| public void Issue826GetCanonicalizedUriWithMultipleEncoding() throws Exception { | ||
| System.out.println("GetCanonicalizedUriWithAnHTMLEntityCollision"); | ||
| Encoder e = ESAPI.encoder(); | ||
| String expectedUri = "http://[email protected]/path_to/resource?foo=bar¶1=&test"; | ||
| //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 | ||
| //not appear in the userinfo field. | ||
| String input = "http://[email protected]/path_to/resource?foo=bar¶1=&test"; | ||
| URI uri = new URI(input); | ||
| 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. |
||
| public void testGetCanonicalizedUriWithMultQueryParams() throws Exception { | ||
|
There was a problem hiding this comment. And looks like a leading tab here as well. |
||
| System.out.println("getCanonicalizedUri"); | ||
| Encoder e = ESAPI.encoder(); | ||
|
|
||
| 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. |
||
| //not appear in the userinfo field. | ||
| String input = "http://palpatine@foo%20bar.com/path_to/resource?foo=bar&bar=foo#frag"; | ||
| URI uri = new URI(input); | ||
| System.out.println(uri.toString()); | ||
| assertEquals(expectedUri, e.getCanonicalizedURI(uri)); | ||
|
|
||
| } | ||
|
|
||
| public void testGetCanonicalizedUriPiazza() throws Exception { | ||
| System.out.println("getCanonicalizedUriPiazza"); | ||
|
|
@@ -1000,6 +1040,41 @@ public void testGetCanonicalizedUriPiazza() throws Exception { | |
| assertEquals(expectedUri, e.getCanonicalizedURI(uri)); | ||
|
|
||
| } | ||
|
|
||
| public void testIssue824() throws Exception { | ||
| System.out.println("getCanonicalizedUriPiazza"); | ||
| 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? |
||
| //password information as in http://palpatine:[email protected], and this will | ||
| //not appear in the userinfo field. | ||
| String input = "/webapp/ux/home?d=1705914006565&status=login&ticket=1705914090394_HzJpTROVfhW-JhRW0OqDbHu7tWXXlgrKSUmOzIMsZNCcUIiYGMXX_Q%3D%3D&newsess=false&roleid=DP010101/0007&origin=ourprogram"; | ||
| URI uri = new URI(input); | ||
| System.out.println(uri.toString()); | ||
| 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. |
||
| public void Issue826GetCanonicalizedDoubleAmpersand() throws Exception { | ||
|
There was a problem hiding this comment. Leading tab character here. |
||
| System.out.println("getCanonicalizedDoubleAmpersand"); | ||
| Encoder e = ESAPI.encoder(); | ||
| String expectedUri = "http://127.0.0.1:3000/campaigns?goal=all§ion=active&sort-by=-id&status=Draft%2C&html=&contentLaunched"; | ||
| //http://127.0.0.1:3000/campaigns?goal=all§ion=active&sort-by=-id&status=Draft,&html=null&=null&contentLaunched=null | ||
| /* | ||
| * In this case, the URI class should break up the HTML entity in the query so | ||
| */ | ||
| String input = "http://127.0.0.1:3000/campaigns?goal=all§ion=active&sort-by=-id&status=Draft%2C&html=&&contentLaunched"; | ||
| URI uri = new URI(input); | ||
| System.out.println(uri.toString()); | ||
| try { | ||
| assertEquals(expectedUri, e.getCanonicalizedURI(uri)); | ||
| fail(); | ||
| } catch (Exception ex) { | ||
| //Expected | ||
| } | ||
jeremiahjstacey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| public void testGetCanonicalizedUriWithMailto() throws Exception { | ||
| System.out.println("getCanonicalizedUriWithMailto"); | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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