[rfc][icedtea-web] Strip parameters from document-base

[Adam Domurad]

On 04/05/2013 11:32 AM, Jiri Vanek wrote:
> On 04/03/2013 04:09 PM, Adam Domurad wrote:
>> On 03/28/2013 07:53 AM, Jiri Vanek wrote:
>>  > On 03/27/2013 09:20 PM, Adam Domurad wrote:
>>  >> It appears having parameters in the URL for the stored document-base
>>  >> can cause problems with some
>>  >> applets, namely the Oracle LMS applet.
>>  >>
>>  >> A small bit of refactoring is needed to move the URL stripping code:
>>  >>
>>  >> Refactoring ChangeLog:
>>  >> 2013-03-26 Adam Domurad <adomurad at redhat.com>
>>  >>
>>  >> *
>>  >> 
>> netx/net/sourceforge/jnlp/security/appletextendedsecurity/UnsignedAppletTrustConfirmation.java
>>  >>
>>  >> (normalizeUrlAndStripParams): Moved.
>>  >> * netx/net/sourceforge/jnlp/util/UrlUtils.java
>>  >> (normalizeUrlAndStripParams): New, moved from
>>  >> UnsignedAppletTrustConfirmation.
>>  >> *
>>  >> 
>> tests/netx/unit/net/sourceforge/jnlp/security/appletextendedsecurity/UnsignedAppletTrustConfirmationTest.java
>>  >>
>>  >>
>>  >> (testNormalizeUrlAndStripParams): Moved.
>>  >> * tests/netx/unit/net/sourceforge/jnlp/util/UrlUtilsTest.java:
>>  >> New, has (testNormalizeUrlAndStripParams) from
>>  >> UnsignedAppletTrustConfirmationTest.
>>  >>
>>  >>
>>  >> And the fix itself:
>>  >> 2013-03-26 Adam Domurad <adomurad at redhat.com>
>>  >>
>>  >> * netx/net/sourceforge/jnlp/NetxPanel.java
>>  >> (NetxPanel): Ensure documentURL has stripped parameters
>>  >>
>>  >> Happy hacking,
>>  >> -Adam
>>  >
>>  > The refactoring is ok. But to use completely stripped codeabse in 
>> base
>>  > codebase sounds to me as quite big change. As far as i looked, I was
>>  > not able to judge all the impact and would rather stay with more
>>  > conservative change or with very deep testing of this change on your
>>  > side.
>>
>> Actually, stripping in the code-base was always done. The bug was that
>> URL's such as:
>>
>> http://example.com/?test/
>>
>> Were not being stripped properly, because the last part looked like a
>> directory.
>> However after further testing it looks like Oracle does not strip the
>> document-base. It was a bit ugly but I have managed to ensure that our
>> documentbase & codebase are always the same as Oracle's.
>>
>> There is some refactoring attached that moves normalizeUrl from
>> ResourceTracker to UrlUtils, as well as adding an option whether to
>> encode file:// based URL's or not. For compatibility purposes, we must
>> encode URL's in the code-base & document-base, even if local. This is a
>> good idea for matching purposes too.
>>
>> However, for actually accessing these URL's, the file:// based URL's
>> should not be encoded.
>>
>> add-url-utils.patch ChangeLog:
>> 2013-04-02 Adam Domurad <adomurad at redhat.com>
>>
>> * netx/net/sourceforge/jnlp/cache/ResourceTracker.java: Remove no
>> longer used constants. Remove (normalizeUrl). Update calls.
>> * netx/net/sourceforge/jnlp/cache/CacheUtil.java: Expand imports.
>> Update calls.
>> *
>> netx/net/sourceforge/jnlp/security/appletextendedsecurity/UnsignedAppletTrustConfirmation.java: 
>>
>> Ensure file://-protocol URLs are encoded.
>> * netx/net/sourceforge/jnlp/util/UrlUtils.java: Add (normalizeUrl),
>> and related utility methods. Allow for optionally encoding file://
>> URLs.
>>
> Thanx for deep checks!
>
>> There is some non-ideal code in the fix itself, because first we pass a
>> stripped document-base, and then restore it to its original form. I
>> blame this on the rigidity of inheriting from AppletViewerPanel, but I
>> did not want to remove this inheritance in this patch.
>>
>> properly-strip-codebase.patch:
>> 2013-04-02 Adam Domurad <adomurad at redhat.com>
>>
>> Ensure code-base is stripped of parameters that look like
>> directories.
>> * netx/net/sourceforge/jnlp/NetxPanel.java
>> (NetxPanel): Ensure code-base is created from stripped
>> document-base.
>> Don't strip document-base itself.
>> (runLoader): Ensure URL used for resource loaded is not
>> encoded.
>>
>> The biggest change I think is the fact that the document-base &
>> code-base are now encoded, otherwise code-bases were (almost) always
>> stripped already. The code-base is decoded for resource-loading
>> purposes, as it was before.
>>
>> It looks fine from my testing (so far).
>
>
> Ok, I thnk this look ok even for upcoming release. Anyway - before 
> push please add unittests for all new static methods. and also enhance 
> already existing test in UrlUtilsTest for new corner cases 
> (file/http/ftp x , true/false...) .

I think the only corner case here is file:// + true/false. Included 
additional tests.

>
> +    /* Decode a percent-encoded URL. Catch checked exceptions and 
> log. */
> +    public static URL decodeUrlQuietly(URL url) {
> +        try {
> +            return new URL(URLDecoder.decode(url.toString(), UTF8));
> +        } catch (IOException e) {
> +            return url;
> +        }
> +    }
> +
>
> Where is the log? if it is adding of ex.printStacktrace then ok 
> without further review.

Good catch, I'll add this before pushing.

>
> J.

Unit tests attached.
ChangeLog:
2013-XX-XX  Adam Domurad  <adomurad at redhat.com>

     * tests/netx/unit/net/sourceforge/jnlp/util/UrlUtilsTest.java:
     Added tests for decodeUrlQuietly, normalizeUrl, normalizeUrlQuietly.


-Adam

-------------- next part --------------
A non-text attachment was scrubbed...
Name: url-utils-unit-tests.patch
Type: text/x-patch
Size: 2345 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130405/67d9953c/url-utils-unit-tests.patch