PR to fix #824 and reference to #823 by xeno6696 · Pull Request #828 · ESAPI/esapi-java-legacy · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 16 additions & 9 deletions src/main/java/org/owasp/esapi/reference/DefaultEncoder.java
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Copy link
Copy Markdown
Contributor

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

*
* @param dirtyUri
* @return Canonicalized URI string.
Expand Down Expand Up @@ -548,7 +551,6 @@ public String getCanonicalizedURI(URI dirtyUri) throws IntrusionException{
parseMap.put(UriSegment.SCHEME, dirtyUri.getScheme());
//authority = [ userinfo "@" ] host [ ":" port ]
parseMap.put(UriSegment.AUTHORITY, dirtyUri.getRawAuthority());
parseMap.put(UriSegment.SCHEMSPECIFICPART, dirtyUri.getRawSchemeSpecificPart());
parseMap.put(UriSegment.HOST, dirtyUri.getHost());
//if port is undefined, it will return -1
Integer port = new Integer(dirtyUri.getPort());
Expand All @@ -557,9 +559,6 @@ public String getCanonicalizedURI(URI dirtyUri) throws IntrusionException{
parseMap.put(UriSegment.QUERY, dirtyUri.getRawQuery());
parseMap.put(UriSegment.FRAGMENT, dirtyUri.getRawFragment());

//Now we canonicalize each part and build our string.
StringBuilder sb = new StringBuilder();

//Replace all the items in the map with canonicalized versions.

Set<UriSegment> set = parseMap.keySet();
Expand All @@ -568,8 +567,7 @@ public String getCanonicalizedURI(URI dirtyUri) throws IntrusionException{
boolean allowMixed = sg.getBooleanProp("Encoder.AllowMixedEncoding");
boolean allowMultiple = sg.getBooleanProp("Encoder.AllowMultipleEncoding");
for(UriSegment seg: set){
String value = canonicalize(parseMap.get(seg), allowMultiple, allowMixed);
value = value == null ? "" : value;
String value = "";
//In the case of a uri query, we need to break up and canonicalize the internal parts of the query.
if(seg == UriSegment.QUERY && null != parseMap.get(seg)){
StringBuilder qBuilder = new StringBuilder();
Expand Down Expand Up @@ -597,6 +595,10 @@ public String getCanonicalizedURI(URI dirtyUri) throws IntrusionException{
} catch (UnsupportedEncodingException e) {
logger.debug(Logger.EVENT_FAILURE, "decoding error when parsing [" + dirtyUri.toString() + "]");
}
} else {
String extractedInput = parseMap.get(seg);
value = canonicalize(extractedInput, allowMultiple, allowMixed);
value = value == null ? "" : value;
}
//Check if the port is -1, if it is, omit it from the output.
if(seg == UriSegment.PORT){
Expand All @@ -618,11 +620,16 @@ public String getCanonicalizedURI(URI dirtyUri) throws IntrusionException{
*/
protected String buildUrl(Map<UriSegment, String> parseMap){
StringBuilder sb = new StringBuilder();
sb.append(parseMap.get(UriSegment.SCHEME))
.append("://")
boolean schemePresent = parseMap.get(UriSegment.SCHEME).equals("") ? false : true;

if(schemePresent) {
sb.append(parseMap.get(UriSegment.SCHEME))
.append("://");
}

//can't use SCHEMESPECIFICPART for this, because we need to canonicalize all the parts of the query.
//USERINFO is also deprecated. So we technically have more than we need.
.append(parseMap.get(UriSegment.AUTHORITY) == null || parseMap.get(UriSegment.AUTHORITY).equals("") ? "" : parseMap.get(UriSegment.AUTHORITY))
sb.append(parseMap.get(UriSegment.AUTHORITY) == null || parseMap.get(UriSegment.AUTHORITY).equals("") ? "" : parseMap.get(UriSegment.AUTHORITY))
.append(parseMap.get(UriSegment.PATH) == null || parseMap.get(UriSegment.PATH).equals("") ? "" : parseMap.get(UriSegment.PATH))
.append(parseMap.get(UriSegment.QUERY) == null || parseMap.get(UriSegment.QUERY).equals("")
? "" : "?" + parseMap.get(UriSegment.QUERY))
Expand Down
20 changes: 20 additions & 0 deletions src/test/java/org/owasp/esapi/codecs/HTMLEntityCodecTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import static org.junit.Assert.assertEquals;

import org.junit.Ignore;
import org.junit.Test;

public class HTMLEntityCodecTest {
Expand Down Expand Up @@ -48,4 +49,23 @@ public void testMixedBmpAndNonBmp(){
String input = bmp + nonBMP;
assertEquals(expected, codec.encode(new char[0], input));
}

@Test

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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));
}
}
95 changes: 85 additions & 10 deletions src/test/java/org/owasp/esapi/reference/EncoderTest.java