Hi 2011/8/1 Curtiss Howard <[email protected]>: > Hi, > > One of the teams we work with was recently complaining that whenever > they pass an already-encoded URL to <h:graphicImage> that they get a > double encoding, which never happened before in our previous release > (which corresponded to the 1.2 timeframe). It appears that this is > the result of a bug in encodeURIAttribute(), introduced after 1.2 and > before 2.0: > > else if (c == '%') > { > if (i + 2 < string.length()) > { > char c1 = string.charAt(i+1); > char c2 = string.charAt(i+2); > if ((( c1 >= '0' && c1 <='9') || (c1 >='A' && c1 > <='Z')) && > (( c2 >= '0' && c2 <='9') || (c2 >='A' && c2 > <='Z'))) > { > // do not percent encode, because it could be > already encoded > // and we don't want encode it twice > } > else > { > app = percentEncode(c, UTF8); > } > } > else > { > app = percentEncode(c, UTF8); > } > } > > (Forgive me if the formatting doesn't come out right...)
Just for reference the issue you are talking about is MYFACES-1841 HtmlResponseWriterImpl.writeURIAttribute does not perform proper URLs encoding ( ex: & should be encoded in &) > > So whenever we find a %, we try to look ahead and see if the next two > characters constitute two hex characters. If so, we leave the % alone > since this is clearly a portion of the string that has already been > encoded. > > There are two problems with that code however: > > 1. Checking for '0' <= c <= '9' && 'A' <= c <= 'Z' means that "%XX" > would remain unchanged, when really it needs to be "%2AXX". We should > be checking for 'A' <= c <= 'F' since we're talking about hex > characters. Are you mixing some javascript here? In theory if the percent encoding was already done, there is no reason to do it again, because this could break the generated URI. The algorithm check that condition and do this is intentional. Maybe the problem could be solved detecting this special condition when h:graphicImage do its job. > 2. The problem our colleagues saw: we aren't handling lowercase characters! > > So I just want to make sure everyone is in agreement with my > assessment that we need to change the character range from A..Z to > A..F and also check for a..f. If so, I'll make the change. I checked it and rfc 2141 specify lower case chars are valid in this case, so this change is valid. See: http://www.ietf.org/rfc/rfc2141.txt regards, Leonardo Uribe > > Thanks, > > > Curtiss Howard >
