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

[Jiri Vanek]

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...) .

+    /* 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.

J.