[rfc][icedtea-web] Archive URL wrongly resolved - PR1204
Jiri Vanek
jvanek at redhat.com
Fri Jun 14 07:37:10 PDT 2013
On 06/14/2013 03:45 PM, Andrew Azores wrote:
> Changelog:
>
> * netx/net/sourceforge/jnlp/cache/ResourceUrlCreator.java (getVersionedUrlUsingQuery): retain query
> strings in URLs
> * plugin/icedteanp/java/sun/applet/PluginAppletViewer.java (requestPluginProxyInfo): do not add '/'
> character between hostname and path if the path is absolute
>
> *
> tests/reproducers/simple/AbsolutePathAndQueryStringApplet/testcases/AbsolutePathAndQueryStringAppletTest.java:
> tests in-browser launching of applets with absolute JAR paths and query strings in HTML embed tags
> *
> tests/reproducers/simple/AbsolutePathAndQueryStringApplet/srcs/AbsolutePathAndQueryStringApplet.java: same
>
> *
> tests/reproducers/simple/AbsolutePathAndQueryStringApplet/resources/AbsolutePathAndQueryStringApplet.html:
> same
>
> This patch fixes a bug where archive URLs would be incorrectly resolved in certain cases. If the
> archive path specified in an HTML Embed tag was of the form "/foo/bar.jar?i=1234abcd", on host
> some.site.com, then the plugin would attempt to resolve "http://some.site.com//foo/bar.jar". This
> was incorrect due to the doubled '/' character after the hostname as well as the removal of the
> query string.
>
Hi! Looks moreover good :)
I was not deep thinking aboout, but looks like much more complex patch then I thought it would be
when I first time saw the bug.
My initial thoughts were - (1) found where the query is lost, and do not lost it (but take care
about caching)
- and remove any leading "/" (2) for resources.
As far as I have read. you are mostly doing it, but ...:)
The most worrying is the placement of your code. I would search for one shared place for both javaws
and applets... (see inline)
>
> fix.patch
>
>
> diff --git a/netx/net/sourceforge/jnlp/cache/ResourceUrlCreator.java b/netx/net/sourceforge/jnlp/cache/ResourceUrlCreator.java
> --- a/netx/net/sourceforge/jnlp/cache/ResourceUrlCreator.java
> +++ b/netx/net/sourceforge/jnlp/cache/ResourceUrlCreator.java
> @@ -38,6 +38,8 @@ exception statement from your version. *
> package net.sourceforge.jnlp.cache;
>
> import java.net.MalformedURLException;
> +import java.net.URI;
> +import java.net.URISyntaxException;
> import java.net.URL;
> import java.util.LinkedList;
> import java.util.List;
> @@ -152,23 +154,24 @@ public class ResourceUrlCreator {
> * @param resource the resource to get the url for
> */
> protected URL getVersionedUrlUsingQuery(Resource resource) {
> - String actualLocation = resource.getLocation().getProtocol() + "://"
> - + resource.getLocation().getHost();
> - if (resource.getLocation().getPort() != -1) {
> - actualLocation += ":" + resource.getLocation().getPort();
> + URL resourceUrl = resource.getLocation();
> + try {
> + String query = resourceUrl.getQuery();
> + if (query == null) query = "";
> + if (resource.requestVersion != null
> + && resource.requestVersion.isVersionId()) {
> + if (!query.isEmpty()) {
> + query += "&";
> + }
> + query += "version-id=" + resource.requestVersion;
> + }
> + URI uri = new URI(resourceUrl.getProtocol(), null, resourceUrl.getHost(), resourceUrl.getPort(), resourceUrl.getPath(), query, null);
> + return uri.toURL();
> + } catch (MalformedURLException e) {
> + return resourceUrl;
> + } catch (URISyntaxException e) {
> + return resourceUrl;
> }
This is really deep change. It would need huge unittesting. (dont forget to negative cases and
versioning)
The new approach looks correct, but I'm afraid it can affect caching and versioned ajrs. You will
have to prove that thay are still working correctly.
> - actualLocation += resource.getLocation().getPath();
> - if (resource.requestVersion != null
> - && resource.requestVersion.isVersionId()) {
> - actualLocation += "?version-id=" + resource.requestVersion;
> - }
> - URL versionedURL;
> - try {
> - versionedURL = new URL(actualLocation);
> - } catch (MalformedURLException e) {
> - return resource.getLocation();
> - }
> - return versionedURL;
> }
>
> }
> \ No newline at end of file
> diff --git a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> --- a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> +++ b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> @@ -1247,10 +1247,23 @@ public class PluginAppletViewer extends
>
> String scheme = uri.getScheme();
> String port = uri.getPort() != -1 ? ":" + uri.getPort() : "";
> - if (!uri.getScheme().startsWith("http") && !uri.getScheme().equals("ftp"))
> + if (!scheme.startsWith("http") && !scheme.equals("ftp"))
> scheme = "http";
>
> - requestURI = UrlUtil.encode(scheme + "://" + uri.getHost() + port + "/" + uri.getPath(), "UTF-8");
> + String baseURI = scheme + "://" + uri.getHost() + port;
> + if (!uri.getPath().startsWith("/")) {
> + baseURI += "/";
> + }
wrong indentation
> +
> + baseURI += uri.getPath();
> +
> + if (uri.getQuery() != null) {
> + baseURI += "?" + uri.getQuery();
> + }
This code looks like duplicating logic with ResourceUrlCreator hunk.
> +
> + requestURI = UrlUtil.encode(baseURI, "UTF-8");
> + System.out.println("##### PluginAppletViewer requestPluginProxyInfo uri: " + uri.toString());
> + System.out.println("##### PluginAppletViewer requestPluginProxyInfo requestURI: " + requestURI);
This debug should be removed, or moved to if debug, with some more informative output (eg was, is,
should be...) - I'm however for removal your choice :)
ANyway I think this is very wrong palce - especially just plugin (proxy?!?!?) is affected - This
chanage should affect both javaws and appelt. (and so the query change, but its place looks more
correct)
> } catch (Exception e) {
> PluginDebug.debug("Cannot construct URL from ", uri.toString(), " ... falling back to DIRECT proxy");
> e.printStackTrace();
>
>
> reproducer.patch
>
>
The reproducer is nice. To current one just two nits - there should be @bug annotation on top of
testcases with bug id (with PR) and links to this thread in mail archive.
Second nit is reusing of already existing jars. Last time you have already wrote applet aproximatly
same as your new AbsolutePathAndQueryStringApplet. So the best for this patch will be to get rid of
this new, and reuse the same :)
In jnlp and html files you can call the existing one.
Also this test needs to be enhanced -
- you need to test it for both javaws and for appelts.
- you need to check how this behaves with versioned jars
- you need to test if the cache is still ok (query)
Also you should try that the reproducers run ruin without regressions with this new patch.
> diff --git a/tests/reproducers/simple/AbsolutePathAndQueryStringApplet/resources/AbsolutePathAndQueryStringApplet.html b/tests/reproducers/simple/AbsolutePathAndQueryStringApplet/resources/AbsolutePathAndQueryStringApplet.html
> new file mode 100644
> --- /dev/null
> +++ b/tests/reproducers/simple/AbsolutePathAndQueryStringApplet/resources/AbsolutePathAndQueryStringApplet.html
> @@ -0,0 +1,48 @@
> +<!--
> +
> +This file is part of IcedTea.
> +
> +IcedTea is free software; you can redistribute it and/or modify
> +it under the terms of the GNU General Public License as published by
> +the Free Software Foundation; either version 2, or (at your option)
> +any later version.
> +
> +IcedTea is distributed in the hope that it will be useful, but
> +WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with IcedTea; see the file COPYING. If not, write to the
> +Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> +02110-1301 USA.
> +
> +Linking this library statically or dynamically with other modules is
> +making a combined work based on this library. Thus, the terms and
> +conditions of the GNU General Public License cover the whole
> +combination.
> +
> +As a special exception, the copyright holders of this library give you
> +permission to link this library with independent modules to produce an
> +executable, regardless of the license terms of these independent
> +modules, and to copy and distribute the resulting executable under
> +terms of your choice, provided that you also meet, for each linked
> +independent module, the terms and conditions of the license of that
> +module. An independent module is a module which is not derived from
> +or based on this library. If you modify this library, you may extend
> +this exception to your version of the library, but you are not
> +obligated to do so. If you do not wish to do so, delete this
> +exception statement from your version.
> +
> + -->
> +<html>
> + <head></head>
> + <body>
> + <embed code="AbsolutePathAndQueryStringApplet.class"
> + archive="/AbsolutePathAndQueryStringApplet.jar?i=1234abcd"
> + type="application/x-java-applet;version=1.6"
> + width="800"
> + height="600">
> + </embed>
> + </body>
> +</html>
> diff --git a/tests/reproducers/simple/AbsolutePathAndQueryStringApplet/srcs/AbsolutePathAndQueryStringApplet.java b/tests/reproducers/simple/AbsolutePathAndQueryStringApplet/srcs/AbsolutePathAndQueryStringApplet.java
> new file mode 100644
> --- /dev/null
> +++ b/tests/reproducers/simple/AbsolutePathAndQueryStringApplet/srcs/AbsolutePathAndQueryStringApplet.java
> @@ -0,0 +1,51 @@
> +/* StripHttpPathParams.java
> +Copyright (C) 2011 Red Hat, Inc.
> +
> +This file is part of IcedTea.
> +
> +IcedTea is free software; you can redistribute it and/or
> +modify it under the terms of the GNU General Public License as published by
> +the Free Software Foundation, version 2.
> +
> +IcedTea is distributed in the hope that it will be useful,
> +but WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with IcedTea; see the file COPYING. If not, write to
> +the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> +02110-1301 USA.
> +
> +Linking this library statically or dynamically with other modules is
> +making a combined work based on this library. Thus, the terms and
> +conditions of the GNU General Public License cover the whole
> +combination.
> +
> +As a special exception, the copyright holders of this library give you
> +permission to link this library with independent modules to produce an
> +executable, regardless of the license terms of these independent
> +modules, and to copy and distribute the resulting executable under
> +terms of your choice, provided that you also meet, for each linked
> +independent module, the terms and conditions of the license of that
> +module. An independent module is a module which is not derived from
> +or based on this library. If you modify this library, you may extend
> +this exception to your version of the library, but you are not
> +obligated to do so. If you do not wish to do so, delete this
> +exception statement from your version.
> + */
> +
> +import java.applet.Applet;
> +
> +public class AbsolutePathAndQueryStringApplet extends Applet {
> + private static final String appletCloseString = "*** APPLET FINISHED ***";
> +
> + public static void main(String[] args) {
> + System.out.println("running");
> + }
> +
> + @Override
> + public void init() {
> + System.out.println(appletCloseString);
> + }
init or start? :)
> +}
> diff --git a/tests/reproducers/simple/AbsolutePathAndQueryStringApplet/testcases/AbsolutePathAndQueryStringAppletTest.java b/tests/reproducers/simple/AbsolutePathAndQueryStringApplet/testcases/AbsolutePathAndQueryStringAppletTest.java
> new file mode 100644
> --- /dev/null
> +++ b/tests/reproducers/simple/AbsolutePathAndQueryStringApplet/testcases/AbsolutePathAndQueryStringAppletTest.java
> @@ -0,0 +1,60 @@
> +/* StripHttpPathParamsTest.java
> +Copyright (C) 2011 Red Hat, Inc.
> +
> +This file is part of IcedTea.
> +
> +IcedTea is free software; you can redistribute it and/or
> +modify it under the terms of the GNU General Public License as published by
> +the Free Software Foundation, version 2.
> +
> +IcedTea is distributed in the hope that it will be useful,
> +but WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with IcedTea; see the file COPYING. If not, write to
> +the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> +02110-1301 USA.
> +
> +Linking this library statically or dynamically with other modules is
> +making a combined work based on this library. Thus, the terms and
> +conditions of the GNU General Public License cover the whole
> +combination.
> +
> +As a special exception, the copyright holders of this library give you
> +permission to link this library with independent modules to produce an
> +executable, regardless of the license terms of these independent
> +modules, and to copy and distribute the resulting executable under
> +terms of your choice, provided that you also meet, for each linked
> +independent module, the terms and conditions of the license of that
> +module. An independent module is a module which is not derived from
> +or based on this library. If you modify this library, you may extend
> +this exception to your version of the library, but you are not
> +obligated to do so. If you do not wish to do so, delete this
> +exception statement from your version.
> + */
> +
> +import net.sourceforge.jnlp.ProcessResult;
> +import net.sourceforge.jnlp.ServerAccess.AutoClose;
> +import net.sourceforge.jnlp.annotations.KnownToFail;
> +import net.sourceforge.jnlp.annotations.NeedsDisplay;
> +import net.sourceforge.jnlp.annotations.TestInBrowsers;
> +import net.sourceforge.jnlp.browsertesting.BrowserTest;
> +import net.sourceforge.jnlp.browsertesting.Browsers;
> +import net.sourceforge.jnlp.closinglisteners.AutoOkClosingListener;
> +
> +import org.junit.Assert;
> +import org.junit.Test;
> +
> +public class AbsolutePathAndQueryStringAppletTest extends BrowserTest {
> +
> + private static final String appletCloseString = AutoOkClosingListener.MAGICAL_OK_CLOSING_STRING;
> +
> + @NeedsDisplay @Test @TestInBrowsers(testIn={Browsers.one})
just nit you dnon t need to follow - I think the annotations are better each on its own line
> + public void testAbsolutePathAndQueryStringAppletLaunch() throws Exception {
> + ProcessResult pr = server.executeBrowser("/AbsolutePathAndQueryStringApplet.html", AutoClose.CLOSE_ON_BOTH);
> + Assert.assertTrue("stdout should contain " + appletCloseString + " but did not", pr.stdout.contains(appletCloseString));
> + }
> +
> +}
>
More information about the distro-pkg-dev
mailing list