[rfc][icedtea-web] fix the empty string in codebase

[Jacob Wisor]

On 09/16/2014 at 03:32 PM, Jiri Vanek wrote:
> I have recently encountered an app, which failed to run, because of malformed
> jnlp. They had codebase=""  and apparently wonted to have ".". Afaik ITW should
> work around this (oracle plugin does :(  )  This patch is handling the "" for
> codebase as it was "."

Yeah, too bad Oracle's plug-in accepts this sort of "URL" because it is actually 
invalid. :-( An empty string URL is actually not equal nor equivalent to a 
missing attribute (null string). I suppose Oracle's plug-in turns those URLs 
into File objects internally...

> The app failed for me in alaca dialogue on NPE, teh patch contains small
> workaround around possible NPE here too + unittest to affected method
>
> ok for head? 1.5?
>
>
> Accordingly with investigating this failure, I have enhanced
> http://icedtea.classpath.org/wiki/IcedTea-Web#Filing_bugs for offline debugging.
>
> Volunteer for reproducer?-)
>
> Thanx,
> J.

> diff -r d8e057783109 netx/net/sourceforge/jnlp/Parser.java
> --- a/netx/net/sourceforge/jnlp/Parser.java	Mon Sep 15 14:09:30 2014 -0400
> +++ b/netx/net/sourceforge/jnlp/Parser.java	Tue Sep 16 15:25:44 2014 +0200
> @@ -1065,10 +1065,21 @@
>       * @throws ParseException if the JNLP file is invalid
>       */
>      public URL getURL(Node node, String name, URL base) throws ParseException {
> -        String href = getAttribute(node, name, null);
> -        if (href == null)
> +        String href = null;
> +        if ("codebase".equals(name)) {

Hmm, strange that the JNLP parser does not have an enum of or a /list/ of valid 
JNLP element and attribute name constants.

> +            href = getCleanAttribute(node, name);
> +            if (href != null) {// so that code can throw an exceptionlater
> +                //some bogus jnlps have codebase as "" and expect it behaving as "."
> +                if (href.trim().isEmpty()) {

Try "href != null && href.trim().isEmpty()". ;-) It can be even further 
optimized into one statement.

> +                    href = ".";
> +                }
> +            }
> +        } else {
> +            href = getAttribute(node, name, null);
> +        }
> +        if (href == null) {
>              return null; // so that code can throw an exception if attribute was required
> -
> +        }
>          try {
>              if (base == null)
>                  return new URL(href);
> @@ -1254,11 +1265,17 @@
>      public String getAttribute(Node node, String name, String defaultValue) {
>          // SAX
>          // String result = ((Element) node).getAttribute(name);
> +        String result = getCleanAttribute(node, name);
> +
> +        if (result == null || result.length() == 0) {
> +            return defaultValue;

I am not sure this is correct at all. What if an attribute was intended to 
accept an empty string? The default value actually depends on the particular 
attribute's semantics. As I said before, a missing attribute equates to null and 
thus always to a default value. An empty string value equates either to a 
default value or /the/ empty string value, depending on the attribute's semantics.

> +        }
> +
> +        return result;
> +    }
> +
> +    public String getCleanAttribute(Node node, String name) {

Err, what is a clean attribute? Did it wash before? :-D So, please check your 
naming. Besides, documentation is missing on a public method. And, judging by 
what it seems to be doing maybe it should be even private.

>          String result = node.getAttribute(name);
> -
> -        if (result == null || result.length() == 0)
> -            return defaultValue;
> -
>          return result;
>      }
>
> diff -r d8e057783109 netx/net/sourceforge/jnlp/security/SecurityDialogs.java
> --- a/netx/net/sourceforge/jnlp/security/SecurityDialogs.java	Mon Sep 15 14:09:30 2014 -0400
> +++ b/netx/net/sourceforge/jnlp/security/SecurityDialogs.java	Tue Sep 16 15:25:44 2014 +0200
> @@ -44,6 +44,8 @@
>  import java.net.URL;
>  import java.security.AccessController;
>  import java.security.PrivilegedAction;
> +import java.util.Arrays;
> +import java.util.Comparator;
>  import java.util.Set;
>  import java.util.concurrent.Semaphore;
>
> @@ -285,7 +287,13 @@
>
>          SecurityDialogMessage message = new SecurityDialogMessage();
>          message.dialogType = DialogType.MISSING_ALACA;
> -        message.extras = new Object[]{title, codeBase.toString(), UrlUtils.setOfUrlsToHtmlList(remoteUrls)};
> +         String urlToShow = "unknown url";
> +         if (codeBase != null) {
> +             urlToShow = codeBase.toString();
> +         } else {
> +             OutputController.getLogger().log("Warning, null codebase wants to show in ALACA!");
> +         }

Formatting.

> +        message.extras = new Object[]{title, urlToShow, UrlUtils.setOfUrlsToHtmlList(remoteUrls)};
>          Object selectedValue = getUserResponse(message);
>          return getIntegerResponseAsBoolean(selectedValue);
>      }
> diff -r d8e057783109 tests/netx/unit/net/sourceforge/jnlp/ParserTest.java
> --- a/tests/netx/unit/net/sourceforge/jnlp/ParserTest.java	Mon Sep 15 14:09:30 2014 -0400
> +++ b/tests/netx/unit/net/sourceforge/jnlp/ParserTest.java	Tue Sep 16 15:25:44 2014 +0200
> @@ -1413,4 +1413,30 @@
>
>          Assert.assertEquals(overwrittenCodebase.toExternalForm(), parser.getCodeBase().toExternalForm());
>      }
> +
> +
> +    @Test
> +    public void testEmptyCodebase() throws Exception {
> +        String data = "<?xml version=\"1.0\"?>\n"
> +                + "<jnlp spec=\"1.5+\"\n"
> +                + "codebase=\"\"  aaa=\"\" "
> +                + ">\n"
> +                + "</jnlp>";

Although this is technically not wrong, it could probably go into a .jnlp file, 
just for clarity and maintainability.

> +
> +        Node root = Parser.getRootNode(new ByteArrayInputStream(data.getBytes()), defaultParser);
> +        Assert.assertEquals("Root name is not jnlp", "jnlp", root.getNodeName());
> +        MockJNLPFile file = new MockJNLPFile(LANG_LOCALE);
> +        Parser parser = new Parser(file, null, root, defaultParser, null);
> +        ParseException eex = null;
> +        //non codebase element is unaffected
> +        URL u = parser.getURL(root, "aaa", null);
> +        Assert.assertEquals(null, u);
> +        try {
> +            parser.getURL(root, "codebase", null);
> +        } catch (ParseException ex) {
> +            eex = ex;
> +        }
> +        Assert.assertEquals(true, eex != null);
> +        Assert.assertEquals(true, eex instanceof ParseException);
> +    }
>  }