[rfc][icedtea-web] Permissions manifest attribute fix
Jiri Vanek
jvanek at redhat.com
Tue Apr 1 11:38:12 UTC 2014
On 03/31/2014 11:30 PM, Omair Majid wrote:
> * Andrew Azores <aazores at redhat.com> [2014-03-31 16:35]:
>> On 03/31/2014 04:10 PM, Andrew Azores wrote:
>>> On 03/31/2014 01:28 PM, Andrew Azores wrote:
>>> Small refactor. Rather than the new RequestedPermissionLevel being
>>> available from SecurityDesc and PluginBridge only, it's also
>>> available from JNLPFile. PluginBridge, being a JNLPFile subclass,
>>> then overrides the method and provides the correct implementation
>>> for HTML applets. This just makes things more coherent IMO.
>>>
>>> Thanks,
>>>
>>
>> And another, as discussed with Omair on IRC. Just extracted the
>> common checks for HTML and JNLP into a new method and call this
>> method once, before the split check for HTML/JNLP divergence. So
>> long as the spec doesn't diverge the two any further, then this
>> should be okay. Also remove an unnecessary typecast (made
>> unnecessary in the last refactor due to new method and override, but
>> forgot to fix).
>
> I am far from an expert on this code, so please take this 'review' with
> a grain of salt.
>
> Also, I am a little hesitant about such (potentially) invasive code
> going in at the last minute.
>
>> +++ b/netx/net/sourceforge/jnlp/runtime/ManifestAttributesChecker.java
>
>> + final RequestedPermissionLevel requested = file.getRequestedPermissionLevel();
>> + validateRequestedPermissionLevelMatchesManifestPermissions(requested, permissions);
>> + if (file instanceof PluginBridge) { // HTML applet
>> + if (requested == RequestedPermissionLevel.NONE) {
>> + if (permissions == ManifestBoolean.TRUE && signing != SigningState.NONE) {
>> + // http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/manifest.html#permissions
>> + // FIXME: attempting to follow the spec, but it is too late now to actually set the applet
>> + // to run in sandbox, so the applet will not be run at all, rather than run sandboxed!
>> + //securityDelegate.setRunInSandbox();
>
> I read this comment a few times and I couldn't figure out (without
> asking you) how it relates to the current state.
>
> The "applet will not be run at all" bit completely threw me off. I read
> this comment as: we can not set the sandbox state now (for some reason)
> so by leaving this commented out, the applet will not run at all. Can
> you clarify that not calling sandbox will let it run without any issues?
>
>> + if (requested == RequestedPermissionLevel.NONE) {
>> + if (permissions == ManifestBoolean.TRUE && signing != SigningState.NONE) {
>
> Maybe call this variable `sandbox` rather than `permissions` to clarify
> that TRUE means sandbox?
I think that all *except* changes in ManifestAttributesChecker can go in as seaprate changeset. They
are good an necessary to go.
Nits -
public enum RequestedPermissionLevel {
+ NONE, SANDBOX, J2SE, ALL
+ }
may have toString, or if differe - toJnlpString and toHtmlString which will return the correct
values, from which are created.
NONE = > null or "null"
SANDBOX => "sandbox"
J2SE
ALL
It is defintley necessary to avoid duplicated, hardcoded strings. I also think that those strings
already *are* somewhere in ITW! Well, I have not found it. So this can go later as separate
chnageset, which will replace all hardcoed Strings with RequestedPermissionLevel.toString (including
new, tests)
Unittests for above functionality are attached
As for ManifestAttributesChecker itself - why it is so complex???
I would expect you just repalce
AppletSecurityLevel level = AppletStartupSecuritySettings.getInstance().getSecurityLevel();
by
new getRequestedPermissionLevel
Why the if nstacneof ? it seesm tome completely redundant...
Thax for checking thi sissue and quick fix!
J.
-------------- next part --------------
diff -r 2670820a9609 tests/netx/unit/net/sourceforge/jnlp/JNLPFileTest.java
--- a/tests/netx/unit/net/sourceforge/jnlp/JNLPFileTest.java Tue Apr 01 11:34:16 2014 +0200
+++ b/tests/netx/unit/net/sourceforge/jnlp/JNLPFileTest.java Tue Apr 01 13:34:24 2014 +0200
@@ -277,4 +277,65 @@
Assert.assertFalse(downloadOptions.useExplicitPack());
Assert.assertFalse(downloadOptions.useExplicitVersion());
}
+
+
+ String minimalJnlp = "<?xml version='1.0'?>\n"
+ + "<jnlp spec='1.5' href='foo' codebase='.'>\n"
+ + " <information>\n"
+ + " <title>Parsing Test</title>\n"
+ + " <vendor>IcedTea</vendor>\n"
+ + " </information>\n"
+ + "<resources>\n"
+ + " </resources>\n"
+ + "SECURITY"
+ + "</jnlp>";
+
+ @Test
+ public void testGetRequestedPermissionLevel1() throws MalformedURLException, ParseException {
+ String jnlpContents = minimalJnlp.replace("SECURITY", "");
+ URL codeBase = new URL("http://icedtea.classpath.org");
+ InputStream is = new ByteArrayInputStream(jnlpContents.getBytes());
+ JNLPFile jnlpFile = new JNLPFile(is, codeBase, new ParserSettings(false, false, false));
+ Assert.assertEquals(SecurityDesc.RequestedPermissionLevel.NONE, jnlpFile.getRequestedPermissionLevel());
+ }
+
+ @Test
+ public void testGetRequestedPermissionLevel2() throws MalformedURLException, ParseException {
+ String jnlpContents = minimalJnlp.replace("SECURITY", "<security><all-permissions/></security>");
+
+ URL codeBase = new URL("http://icedtea.classpath.org");
+ InputStream is = new ByteArrayInputStream(jnlpContents.getBytes());
+ JNLPFile jnlpFile = new JNLPFile(is, codeBase, new ParserSettings(false, false, false));
+ Assert.assertEquals(SecurityDesc.RequestedPermissionLevel.ALL, jnlpFile.getRequestedPermissionLevel());
+ }
+
+ @Test
+ public void testGetRequestedPermissionLevel3() throws MalformedURLException, ParseException {
+ String jnlpContents = minimalJnlp.replace("SECURITY", "<security></security>");
+
+ URL codeBase = new URL("http://icedtea.classpath.org");
+ InputStream is = new ByteArrayInputStream(jnlpContents.getBytes());
+ JNLPFile jnlpFile = new JNLPFile(is, codeBase, new ParserSettings(false, false, false));
+ Assert.assertEquals(SecurityDesc.RequestedPermissionLevel.NONE, jnlpFile.getRequestedPermissionLevel());
+ }
+
+ @Test
+ public void testGetRequestedPermissionLevel4() throws MalformedURLException, ParseException {
+ String jnlpContents = minimalJnlp.replace("SECURITY", "<security>whatever</security>");
+
+ URL codeBase = new URL("http://icedtea.classpath.org");
+ InputStream is = new ByteArrayInputStream(jnlpContents.getBytes());
+ JNLPFile jnlpFile = new JNLPFile(is, codeBase, new ParserSettings(false, false, false));
+ Assert.assertEquals(SecurityDesc.RequestedPermissionLevel.NONE, jnlpFile.getRequestedPermissionLevel());
+ }
+
+ @Test
+ public void testGetRequestedPermissionLevel5() throws MalformedURLException, ParseException {
+ String jnlpContents = minimalJnlp.replace("SECURITY", "<security><j2ee-application-client-permissions/></security>");
+
+ URL codeBase = new URL("http://icedtea.classpath.org");
+ InputStream is = new ByteArrayInputStream(jnlpContents.getBytes());
+ JNLPFile jnlpFile = new JNLPFile(is, codeBase, new ParserSettings(false, false, false));
+ Assert.assertEquals(SecurityDesc.RequestedPermissionLevel.J2SE, jnlpFile.getRequestedPermissionLevel());
+ }
}
diff -r 2670820a9609 tests/netx/unit/net/sourceforge/jnlp/PluginBridgeTest.java
--- a/tests/netx/unit/net/sourceforge/jnlp/PluginBridgeTest.java Tue Apr 01 11:34:16 2014 +0200
+++ b/tests/netx/unit/net/sourceforge/jnlp/PluginBridgeTest.java Tue Apr 01 13:34:24 2014 +0200
@@ -107,6 +107,35 @@
assertEquals(desiredDomain + relativeLocation,
mockCreator.getJNLPHref().toExternalForm());
}
+
+ @Test
+ public void testgetRequestedPermissionLevel() throws MalformedURLException, Exception {
+ String desiredDomain = "http://desired.absolute.codebase.com";
+ URL codeBase = new URL(desiredDomain + "/undesired/sub/dir");
+ String relativeLocation = "/app/test/test.jnlp";
+ PluginParameters params = createValidParamObject();
+ params.put("jnlp_href", relativeLocation);
+ MockJNLPCreator mockCreator = new MockJNLPCreator();
+ PluginBridge pb = new PluginBridge(codeBase, null, "", "", 0, 0, params, mockCreator);
+ assertEquals(pb.getRequestedPermissionLevel(), SecurityDesc.RequestedPermissionLevel.NONE);
+
+ params.put("permissions","all-permissions");
+ pb = new PluginBridge(codeBase, null, "", "", 0, 0, params, mockCreator);
+ assertEquals(pb.getRequestedPermissionLevel(), SecurityDesc.RequestedPermissionLevel.ALL);
+
+ //unknown for applets!
+ params.put("permissions","j2ee-application-client-permissions");
+ pb = new PluginBridge(codeBase, null, "", "", 0, 0, params, mockCreator);
+ assertEquals(pb.getRequestedPermissionLevel(), SecurityDesc.RequestedPermissionLevel.NONE);
+
+ params.put("permissions","sandbox");
+ pb = new PluginBridge(codeBase, null, "", "", 0, 0, params, mockCreator);
+ assertEquals(pb.getRequestedPermissionLevel(), SecurityDesc.RequestedPermissionLevel.SANDBOX);
+
+ params.put("permissions","default");
+ pb = new PluginBridge(codeBase, null, "", "", 0, 0, params, mockCreator);
+ assertEquals(pb.getRequestedPermissionLevel(), SecurityDesc.RequestedPermissionLevel.NONE);
+ }
@Test
public void testBase64StringDecoding() throws Exception {
More information about the distro-pkg-dev
mailing list