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

[Jiri Vanek]

On 04/05/2013 06:49 PM, Adam Domurad wrote:
> 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
>
At least something ;) Thank you.