[rfc][icedtea-web] u51 classpath manifest entry implementation
Andrew Azores
aazores at redhat.com
Wed Feb 5 13:00:11 PST 2014
Comments inline.
On 02/05/2014 01:13 PM, Jiri Vanek wrote:
> diff -r 1e0507976663 netx/net/sourceforge/jnlp/util/ClasspathMatcher.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/util/ClasspathMatcher.java Wed Feb 05 18:24:20 2014 +0100
> @@ -0,0 +1,372 @@
> +// Copyright (C) 2013 Red Hat, Inc.
Should be 2014? Also should this be named ClasspathMatcher? Seems more
like CodebaseMatcher... ? Or ManifestAttributeMatcher.
> + public static ClasspathMatchers compile(String s) {
> + if (s == null) {
> + return new ClasspathMatchers(new ArrayList<ClasspathMatcher>(0));
> + }
> + String[] splitted = s.trim().split("\\s+");
> + ArrayList<ClasspathMatcher> matchers = new ArrayList<ClasspathMatcher>(splitted.length);
> + for (String string : splitted) {
> + matchers.add(ClasspathMatcher.compile(string.trim()));
> + }
Loop body isn't properly indented here.
> + public boolean matches(URL s){
> + return or(s);
> + }
"){" should be ") {"
> +
> + private boolean or(URL s) {
> + for (ClasspathMatcher classpathMatcher : matchers) {
> + if (classpathMatcher.match(s)){
> + return true;
> + }
same
> +
> + }
> + public static final String PROTOCOL_DELIMITER = "://";
> + public static final String PATH_DELIMITER = "/";
> + public static final String PORT_DELIMITER = ":";
> + private final String source;
> + private Parts parts;
> +
> + static class Parts {
> +
> + String protocol;
> + String domain;
> + String port;
> + String path;
> + Pattern protocolRegEx;
> + Pattern domainRegEx;
> + Pattern portRegEx;
> + Pattern pathRegEx;
> +
> + @Override
> + public String toString() {
> + return protocol + "://" + domain + ":" + port + "/" + path;
> + }
Why not use your *_DELIMITER constants here?
> +
> + private boolean matchPath(String source) {
> + if (source.startsWith("/")) {
> + source = source.substring(1);
> + }
> + return generalMatch(source, pathRegEx);
> + }
PATH_DELIMITER again?
> +
> + static boolean hasProtocol(final String source) {
> + int indexOfProtocolMark = source.indexOf(PROTOCOL_DELIMITER);
> + if (indexOfProtocolMark < 0) {
> + return false;
> + }
> + /*
> + * Here is small trap
> + * We do not know, if protocol is specifed
> + * if so, the protocol://blah.blah/blah is already recognized
> + * but we must ensure that we have not found eg:
> + * blah.blah/blah://in/path - which is perfectly valid url...
> + */
> + //the most easy part - dot in url
> + int indexofFirstDot = source.indexOf(".");
> + if (indexofFirstDot >= 0) {
> + if (indexOfProtocolMark < indexofFirstDot) {
> + return true;
> + } else {
> + return false;
> + }
> + }
> +
> + //more nasty part - path specified
> + String degradedProtocol = source.replace("://", "%%%");
> + int indexofFirstPath = degradedProtocol.indexOf(PATH_DELIMITER);
Why isn't the first argument to "replace" also a DELIMITER? Sorry to
hammer on this point :(
Also why is the parameter here final, and basically nothing else in the
class is final? It doesn't seem any more necessary in this method...
either use "final" liberally and apply it wherever feasible, or use it
only when needed. I'd prefer the liberal use, but it's up to you.
Marking this parameter alone as final like this makes it stand out and
look like something special is happening with it, but nothing really is.
> + public static String sourceToRegExString(String s) {
> + //http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/manifest.html#codebase
> + if (s.equals("*")) {
> + return ".*";
> + }
> + if (s.startsWith("*") && s.endsWith("*")) {
> + return "^.*\\Q" + s.substring(1, s.length() - 1) + "\\E.*$";
> + } else if (s.endsWith("*")) {
> + return "^\\Q" + s.substring(0, s.length() - 1) + "\\E.*$";
> +
> + } else if (s.startsWith("*")) {
> + return "^.*\\Q" + s.substring(1) + "\\E$";
> +
> + } else {
> + return "^\\Q" + s + "\\E$";
> + }
> + }
Oh dear... maybe comments here explaining what these regexes do?
> +
> + @Test
> + public void extractPathTest() {
> + Assert.assertEquals("full/path", ClasspathMatcher.extractPath("some.correct.url:5050/full/path"));
> + Exception ex = null;
> + try {
> + ClasspathMatcher.extractPath("some.correct.url:5050");
> + } catch (Exception e) {
> + ex = e;
> + }
> + Assert.assertNotNull(ex);
> + ex = null;
> + Assert.assertEquals("full/path", ClasspathMatcher.extractPath("some.correct.url/full/path"));
> + Assert.assertEquals("full/path", ClasspathMatcher.extractPath("some.url/full/path"));
> + Assert.assertEquals("full/path", ClasspathMatcher.extractPath("some:5050/full/path"));
> + try {
> + ClasspathMatcher.extractPath("some");
> + } catch (Exception e) {
> + ex = e;
> + }
> + Assert.assertNotNull(ex);
> + ex = null;
> + //correct!
> + Assert.assertEquals("//", ClasspathMatcher.extractPath("some.correct.url:5050///"));
> + //incorrect, but hard to solve
> + Assert.assertEquals("/full/path", ClasspathMatcher.extractPath("some.correct.url:5050://full/path"));
> + Assert.assertEquals("/ect.url:5050/full/path", ClasspathMatcher.extractPath("some.corr://ect.url:5050/full/path"));
:/ I see you've commented that this case is hard to solve, but this ^
test in particular looks concerning. Is it not worthwhile to do some
stricter URL validation so we can avoid this kind of result? Because to
me, this input and its result don't really match up very well. Perhaps
there's some way you could leverage the standard URL class to do some of
the heavy lifting? I'm not sure how it will respond to having a protocol
like "some.corr" though - probably just going to throw an exception. But
it's probably worth looking into if you haven't already. You could do
some checking on the other components of the URL first maybe, and in
this case see that the protocol is bad, and then either construct a URL
without any protocol, or replace the protocol with dummy data, and then
get the path part from the URL instance. I'm not sure how well this
approach will work in general though, when you're extracting other parts
of the URL, and there are other various malformed pieces.
> +
> + //total trap url
> + @Test
> + public void madUrls() throws MalformedURLException {
> + String trapUrl1 = "*://:&:://%%20";
> + ClasspathMatcher.Parts p1 = ClasspathMatcher.splitToParts(trapUrl1);
> + Assert.assertEquals("*", p1.protocol);
> + Assert.assertEquals("", p1.domain);
> + Assert.assertEquals("&::", p1.port);
Perhaps there should be some validation that the port is numeric...
> diff -r 1e0507976663 netx/net/sourceforge/jnlp/JNLPFile.java
> --- a/netx/net/sourceforge/jnlp/JNLPFile.java Wed Feb 05 13:55:28 2014 +0100
> +++ b/netx/net/sourceforge/jnlp/JNLPFile.java Wed Feb 05 18:24:20 2014 +0100
> @@ -33,6 +33,7 @@
> import net.sourceforge.jnlp.cache.UpdatePolicy;
> import net.sourceforge.jnlp.runtime.JNLPClassLoader;
> import net.sourceforge.jnlp.runtime.JNLPRuntime;
> +import net.sourceforge.jnlp.util.ClasspathMatcher;
> import net.sourceforge.jnlp.util.logging.OutputController;
>
> /**
> @@ -56,13 +57,7 @@
> * @version $Revision: 1.21 $
> */
> public class JNLPFile {
> -
> -
> - public static final String APP_NAME = "Application-Name";
> - public static final String CALLER_ALLOWABLE = "Caller-Allowable-Codebase";
> - public static final String APP_LIBRARY_ALLOWABLE = "Application-Library-Allowable-Codebase";
> -
> -
> +
>
> // todo: save the update policy, then if file was not updated
> // then do not check resources for being updated.
> @@ -877,6 +872,14 @@
>
>
> public class ManifestsAttributes{
> +
> + public static final String APP_NAME = "Application-Name";
> + public static final String CALLER_ALLOWABLE = "Caller-Allowable-Codebase";
> + public static final String APP_LIBRARY_ALLOWABLE = "Application-Library-Allowable-Codebase";
> + public static final String PERMISSIONS = "Permissions";
> + public static final String CODEBASE = "Codebase";
> + public static final String TRUSTED_ONLY = "Trusted-Only";
> + public static final String TRUSTED_LIBRARY = "Trusted-Library";
> private JNLPClassLoader loader;
What's happening with the indentation here?
>
>
> @@ -912,34 +915,102 @@
> /**
> *http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/manifest.html#caller_allowable
> */
> - public String getCallerAllowableCodebase(){
> - return getAttribute(CALLER_ALLOWABLE);
> + public ClasspathMatcher.ClasspathMatchers getCallerAllowableCodebase() {
> + return getCodeBaseMatchersAttribute(CALLER_ALLOWABLE);
> }
>
> /**
> *http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/manifest.html#app_library
> */
> - public String getApplicationLibraryAllowableCodebase(){
> - return getAttribute(APP_LIBRARY_ALLOWABLE);
> + public ClasspathMatcher.ClasspathMatchers getApplicationLibraryAllowableCodebase() {
> + return getCodeBaseMatchersAttribute(APP_LIBRARY_ALLOWABLE);
> }
Here too, actually. Also "){" -> ") {" again.
> +
> + /**
> + *http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/manifest.html#permissions
> + */
> + public Boolean isSandboxForced() {
> + String s = getAttribute(PERMISSIONS);
> + if (s == null) {
> + return null;
> + } else if (s.trim().equalsIgnoreCase("sandbox")) {
> + return true;
> + } else if (s.trim().equalsIgnoreCase("all-permissions")) {
> + return false;
> + } else {
> + throw new IllegalArgumentException("Unknown value of " + PERMISSIONS + " attribute " + s + ". Expected sandbox or all-permissions");
> + }
> +
> +
> + }
Remove the extra whitespace at the bottom, please.
> +
> + private Boolean processBooleanAttribute(String id) throws IllegalArgumentException {
> + String s = getAttribute(id);
> + if (s == null) {
> + return null;
> + } else {
> + s = s.trim();
> + if (s.equalsIgnoreCase("true") || s.equalsIgnoreCase("false")) {
> + //return ((name != null) && name.equalsIgnoreCase("true"));
> + return Boolean.parseBoolean(s);
> + } else {
> + throw new IllegalArgumentException("Unknown value of " + id + " attribute " + s + ". Expected true or false");
> + }
> + }
> + }
Remove the commented out line here as well.
Next few comments are suggestions on Messages wording :)
> diff -r 1e0507976663 netx/net/sourceforge/jnlp/resources/Messages.properties
> --- a/netx/net/sourceforge/jnlp/resources/Messages.properties Wed Feb 05 13:55:28 2014 +0100
> +++ b/netx/net/sourceforge/jnlp/resources/Messages.properties Wed Feb 05 18:24:20 2014 +0100
> @@ -569,6 +569,14 @@
> SPLASHmissingInformation = Information element is missing, verify source rather
> SPLASHchainWas = This is the list of exceptions that occurred launching your applet. Please note, those exceptions can originate from multiple applets. For a helpful bug report, be sure to run only one applet.
>
> +CBCheckFile = This application is local file. The codebase check is disabled
The application is a local file. Codebase validation is disabled.
> +CBCheckNoEntry = This application do not have codebase manifest entry specified. Please verify vendor. Continuing
This application does not specify a Codebase in its manifest. Please
verify with the applet's vendor. Continuing
> +CBCheckUnsignedFail= Codebase DO NOT matches codebase manifest attribute, but application is unsigned. Continuing
The application's codebase does NOT match the codebase specified in its
manifest, but the application is unsigned. Continuing
> +CBCheckSignedAppletDontMatchException = java applet is not allowed to run when is signed, have codebase manifest entry ({0}), and this do not match to real codebase - {1}
Signed applets are not allowed to run when their actual Codebase does
not match the Codebase specified in their manifest. Expected: {0}.
Actual: {1}
> +CBCheckSignedFail = Codebase DO NOT matches codebase manifest attribute and application is signed. You are strongly encouraged to terminate application
Application Codebase does NOT match the Codebase specified in the
application's manifest, and this application is signed. You are strongly
discouraged from running this application.
> /**
> * When we're trying to load an applet, file.getSecurity() will return
> @@ -2299,6 +2294,51 @@
> public String getMainClass() {
> return mainClass;
> }
> +
> + private URL guessCodeBase() {
> + if (file.getCodeBase() != null) {
> + return file.getCodeBase();
> + } else {
> + //Fixme: codebase should be the codebase of the Main Jar not
> + //the location. Although, it still works in the current state.
> + return file.getResources().getMainJAR().getLocation();
> + }
> + }
One too many spaces of indentation on the method's declaration. Hawk-eye ;)
> +
> + /**
> + *http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/manifest.html#codebase
> + */
> + private void checkCodebaseAttribute() throws LaunchException {
> + if (file.getCodeBase() == null || file.getCodeBase().getProtocol().equals("file")) {
> + OutputController.getLogger().log(OutputController.Level.WARNING_ALL, Translator.R("CBCheckFile"));
> + return;
> + }
> + final Object securityType = security.getSecurityType();
> + final URL codebase = guessCodeBase();
> + final ClasspathMatchers codebaseAtt = file.getManifestsAttributes().getCodebase();
Nice use of 'final' ;)
And I think that's about all I have to say (so far). Nice work overall!
Thanks,
--
Andrew A
More information about the distro-pkg-dev
mailing list