[rfc][icedtea-web] (PR1264) Run in Sandbox button

[Omair Majid]

* Andrew Azores <aazores at redhat.com> [2014-02-26 12:13]:
> "implementation" can be applied on its own, but it doesn't actually
> do anything. It essentially just sets up the classloader to be able
> to run applets sandboxed. "dialog" adds the ability to make the
> classloader do this by adding a button for it.

I will review them separately.

> diff --git a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java

The JNLPClassLoader class is hairy enough and this is making it more
complex. Would it be possible to extract all the
security-decision-making code into a new class that JNLPClassLoader can
delegate to, instead?

I mean it is probably okay to have conditionals based on
getRunInSandbox() this time, but one more conditional like this and the
code will be next to impossible to maintain.

> @@ -386,8 +391,12 @@
>  
>          // If security level is 'high' or greater, we must check if the user allows unsigned applets 
>          // when the JNLPClassLoader is created. We do so here, because doing so in the constructor 
> -        // causes unwanted side-effects for some applets
> -        if (!loader.getSigning() && file instanceof PluginBridge) {
> +        // causes unwanted side-effects for some applets. However, if the loader has been tagged
> +        // with "runInSandbox", then we do not show this dialog - since this tag indicates that
> +        // the user was already shown a CertWarning dialog and has chosen to run the applet sandboxed.
> +        // This means they've already agreed to running the applet and have specified with which
> +        // permission level to do it!
> +        if (!loader.getSigning() && !loader.getRunInSandbox() && file instanceof PluginBridge) {
>              UnsignedAppletTrustConfirmation.checkUnsignedWithUserIfRequired((PluginBridge)file);
>          }

I would create a method named `isUserAlreadyPrompted()` and use that
instead of `loader.getRunInSandbox()`

All in all, I am quite happy how this patch is not very invasive, given
that it touches one of the more sensitive classes in the codebase.

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681