[JENKINS-24399] Don't allow class directories any more.

src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ClasspathEntry.java

@@ -65,15 +65,46 @@ static URL pathToURL(String path) throws MalformedURLException {
         }
     }
 
-    static String urlToPath(URL url) {
+    /** Returns {@code null} if another protocol or unable to perform the conversion. */
+    private static File urlToFile(@Nonnull URL url) {
         if (url.getProtocol().equals("file")) {
             try {
-                return new File(url.toURI()).getAbsolutePath();
+                return new File(url.toURI());
             } catch (URISyntaxException x) {
                 // ?
             }
         }
-        return url.toString();
+        return null;
+    }
+
+    static String urlToPath(URL url) {
+        final File file = urlToFile(url);
+        return file != null ? file.getAbsolutePath() : url.toString();
+    }
+
+    /**
+     * Checks whether an URL would be considered a class directory by {@link java.net.URLClassLoader}.
+     * According to its <a href="http://docs.oracle.com/javase/6/docs/api/java/net/URLClassLoader.html"specification</a>
+     * an URL will be considered an class directory if it ends with /.
+     * In the case the URL uses a {@code file:} protocol a check is performed to see if it is a directory as an additional guard
+     * in case a different class loader is used by other {@link Language} implementation.
+     */
+    static boolean isClassDirectoryURL(@Nonnull URL url) {
+        final File file = urlToFile(url);
+        if (file != null && file.isDirectory()) {
+            return true;
+            // If the URL is a file but does not exist we fallback to default behaviour
+            // as non existence will be dealt with when trying to use it.
+        }
+        return url.toExternalForm().endsWith("/");
+    }
+
+    /**
+     * Checks whether the entry would be considered a class directory.
+     * @see #isClassDirectoryURL(URL)
+     */
+    public boolean isClassDirectory() {
+        return isClassDirectoryURL(url);
     }
 
     public @Nonnull String getPath() {
@@ -83,7 +114,7 @@ static String urlToPath(URL url) {
     public @Nonnull URL getURL() {
         return url;
     }
-    
+
     private @CheckForNull URI getURI() {
         try {
             return url.toURI();

src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java

@@ -124,6 +124,16 @@ public String getHash() {
         public URL getURL() {
             return url;
         }
+
+        /**
+         * Checks whether the entry would be considered a class directory.
+         * @see ClasspathEntry#isClassDirectoryURL(URL)
+         */
+        boolean isClassDirectory() {
+            return ClasspathEntry.isClassDirectoryURL(url);
+        }
+
+
         @Override public int hashCode() {
             return hash.hashCode();
         }
@@ -328,6 +338,21 @@ public ScriptApproval() {
         if (pendingClasspathEntries == null) {
             pendingClasspathEntries = new TreeSet<PendingClasspathEntry>();
         }
+        // Check for loaded class directories
+        boolean changed = false;
+        for (Iterator<ApprovedClasspathEntry> i = approvedClasspathEntries.iterator(); i.hasNext();) {
+            if (i.next().isClassDirectory()) {
+                i.remove();
+                changed = true;
+            }
+        }
+        if (changed) {
+            try {
+                save();
+            } catch (IOException x) {
+                LOG.log(Level.WARNING, "Unable to save changes after cleaning accepted class directories", x);
+            }
+        }
     }
 
     private static String hash(String script, String language) {
@@ -439,6 +464,15 @@ public synchronized String using(@Nonnull String script, @Nonnull Language langu
      * @throws IllegalStateException {@link Jenkins} instance is not ready
      */
     public synchronized void configuring(@Nonnull ClasspathEntry entry, @Nonnull ApprovalContext context) {
+        // In order to try to minimize changes for existing class directories that could be saved
+        // - Class directories are ignored here (issuing a warning)
+        // - When trying to use them, the job will fail
+        // - Going to the configuration page you'll have the validation error in the classpath entry
+        if (entry.isClassDirectory()) {
+            LOG.log(Level.WARNING, "{0} is a class directory, which are not allowed. Ignored in configuration, use will be rejected",
+                    entry.getURL());
+            return;
+        }
         //TODO: better error propagation
         final Jenkins jenkins = getActiveJenkinsInstance();
         URL url = entry.getURL();
@@ -486,6 +520,9 @@ public synchronized void configuring(@Nonnull ClasspathEntry entry, @Nonnull App
     public synchronized FormValidation checking(@Nonnull ClasspathEntry entry) {
         //TODO: better error propagation
         final Jenkins jenkins = getActiveJenkinsInstance();
+        if (entry.isClassDirectory()) {
+            return FormValidation.error(Messages.ClasspathEntry_path_noDirsAllowed());
+        }
         URL url = entry.getURL();
         try {
             if (!jenkins.hasPermission(Jenkins.RUN_SCRIPTS) && !approvedClasspathEntries.contains(new ApprovedClasspathEntry(hashClasspathEntry(url), url))) {
@@ -512,14 +549,19 @@ public synchronized void using(@Nonnull ClasspathEntry entry) throws IOException
         String hash = hashClasspathEntry(url);
 
         if (!approvedClasspathEntries.contains(new ApprovedClasspathEntry(hash, url))) {
-            // Never approve classpath here.
-            ApprovalContext context = ApprovalContext.create();
-            if (pendingClasspathEntries.add(new PendingClasspathEntry(hash, url, context))) {
-                LOG.log(Level.FINE, "{0} ({1}) is pending.", new Object[] {url, hash});
-                try {
-                    save();
-                } catch (IOException x) {
-                    LOG.log(Level.WARNING, null, x);
+            // Don't add it to pending if it is a class directory
+            if (entry.isClassDirectory()) {
+                LOG.log(Level.WARNING, "{0} ({1}) is a class directory, which are not allowed.", new Object[] {url, hash});
+            } else {
+                // Never approve classpath here.
+                ApprovalContext context = ApprovalContext.create();
+                if (pendingClasspathEntries.add(new PendingClasspathEntry(hash, url, context))) {
+                    LOG.log(Level.FINE, "{0} ({1}) is pending.", new Object[] {url, hash});
+                    try {
+                        save();
+                    } catch (IOException x) {
+                        LOG.log(Level.WARNING, null, x);
+                    }
                 }
             }
             throw new UnapprovedClasspathException(url, hash);

src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/Messages.properties

@@ -1,2 +1,3 @@
 ClasspathEntry.path.notExists=Specified path does not exist
 ClasspathEntry.path.notApproved=This classpath entry is not approved. Require an approval before execution.
+ClasspathEntry.path.noDirsAllowed=Class directories are not allowed as classpath entries.

src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java

@@ -47,6 +47,7 @@
 import hudson.tasks.Publisher;
 import java.io.File;
 import java.io.FileFilter;
+import java.io.IOException;
 import java.net.URISyntaxException;
 import java.util.ArrayList;
 import java.util.List;
@@ -242,6 +243,25 @@ private List<File> getAllJarFiles() throws URISyntaxException {
         return ret;
     }
 
+    private List<File> copy2TempDir(Iterable<File> files) throws IOException {
+        final File tempDir = tmpFolderRule.newFolder();
+        final List copies = new ArrayList<File>();
+        for (File f: files) {
+            final File copy = new File(tempDir, f.getName());
+            FileUtils.copyFile(f, copy);
+            copies.add(copy);
+        }
+        return copies;
+    }
+
+    private List<ClasspathEntry> files2entries(Iterable<File> files) throws IOException {
+        final List entries = new ArrayList<ClasspathEntry>();
+        for (File f: files) {
+            entries.add(new ClasspathEntry(f.toURI().toURL().toExternalForm()));
+        }
+        return entries;
+    }
+
     private List<File> getAllUpdatedJarFiles() throws URISyntaxException {
         String testClassPath = String.format(StringUtils.join(getClass().getName().split("\\."), "/"));
         File testClassDir = new File(ClassLoader.getSystemResource(testClassPath).toURI()).getAbsoluteFile();
@@ -567,68 +587,10 @@ private List<File> getAllUpdatedJarFiles() throws URISyntaxException {
             assertNotEquals(testingDisplayName, b.getDisplayName());
         }
 
-        // Approve classpath.
-        {
-            List<ScriptApproval.PendingClasspathEntry> pcps = ScriptApproval.get().getPendingClasspathEntries();
-            assertNotEquals(0, pcps.size());
-            for(ScriptApproval.PendingClasspathEntry pcp: pcps) {
-                ScriptApproval.get().approveClasspathEntry(pcp.getHash());
-            }
-        }
-
-        // Success as approved.
-        {
-            FreeStyleBuild b = p.scheduleBuild2(0).get();
-            r.assertBuildStatusSuccess(b);
-            assertEquals(testingDisplayName, b.getDisplayName());
-        }
-
-        // add new file in tmpDir.
-        {
-            File f = tmpFolderRule.newFile();
-            FileUtils.copyFileToDirectory(f, tmpDir);
-        }
-
-        // Fail as the class directory is updated.
-        {
-            FreeStyleBuild b = p.scheduleBuild2(0).get();
-            r.assertBuildStatus(Result.FAILURE, b);
-            assertNotEquals(testingDisplayName, b.getDisplayName());
-        }
-
-        // Approve classpath.
+        // Unable to approve classpath.
         {
             List<ScriptApproval.PendingClasspathEntry> pcps = ScriptApproval.get().getPendingClasspathEntries();
-            assertNotEquals(0, pcps.size());
-            for(ScriptApproval.PendingClasspathEntry pcp: pcps) {
-                ScriptApproval.get().approveClasspathEntry(pcp.getHash());
-            }
-        }
-
-        // Success as approved.
-        {
-            FreeStyleBuild b = p.scheduleBuild2(0).get();
-            r.assertBuildStatusSuccess(b);
-            assertEquals(testingDisplayName, b.getDisplayName());
-        }
-
-        // add a new file in a subdirectory of the tmpDir.
-        {
-            File f = tmpFolderRule.newFile();
-            File targetDir = tmpDir.listFiles(new FileFilter(){
-                public boolean accept(File pathname) {
-                    return pathname.isDirectory();
-                }
-
-            })[0];
-            FileUtils.copyFileToDirectory(f, targetDir);
-        }
-
-        // Should fail as the class directory is updated.
-        {
-            FreeStyleBuild b = p.scheduleBuild2(0).get();
-            r.assertBuildStatus(Result.FAILURE, b);
-            assertNotEquals(testingDisplayName, b.getDisplayName());
+            assertEquals(0, pcps.size());
         }
     }
 
@@ -643,17 +605,7 @@ public boolean accept(File pathname) {
 
         final String testingDisplayName = "TESTDISPLAYNAME";
 
-        File tmpDir1 = tmpFolderRule.newFolder();
-
-        for (File jarfile: getAllJarFiles()) {
-            Expand e = new Expand();
-            e.setSrc(jarfile);
-            e.setDest(tmpDir1);
-            e.execute();
-        }
-
-        List<ClasspathEntry> classpath1 = new ArrayList<ClasspathEntry>();
-        classpath1.add(new ClasspathEntry(tmpDir1.getAbsolutePath()));
+        final List<File> jars = getAllJarFiles();
 
         FreeStyleProject p1 = r.createFreeStyleProject();
         p1.getPublishersList().add(new TestGroovyRecorder(new SecureGroovyScript(
@@ -662,7 +614,7 @@ public boolean accept(File pathname) {
                         + "BuildUtil.setDisplayNameWhitelisted(build, \"%s\");"
                         + "\"\"", testingDisplayName),
                 true,
-                classpath1
+                files2entries(jars)
         )));
 
         // Fail as the classpath is not approved.
@@ -688,40 +640,15 @@ public boolean accept(File pathname) {
             assertEquals(testingDisplayName, b.getDisplayName());
         }
 
-        File tmpDir2 = tmpFolderRule.newFolder();
-
-        for (File jarfile: getAllJarFiles()) {
-            Expand e = new Expand();
-            e.setSrc(jarfile);
-            e.setDest(tmpDir2);
-            e.execute();
-        }
-
-        // touch all files.
-        {
-            DirectoryScanner ds = new DirectoryScanner();
-            ds.setBasedir(tmpDir2);
-            ds.setIncludes(new String[]{ "**" });
-            ds.scan();
-
-            for (String relpath: ds.getIncludedFiles()) {
-                Touch t = new Touch();
-                t.setFile(new File(tmpDir2, relpath));
-                t.execute();
-            }
-        }
-
-        List<ClasspathEntry> classpath2 = new ArrayList<ClasspathEntry>();
-        classpath2.add(new ClasspathEntry(tmpDir2.getAbsolutePath()));
-
+        // New job with jars in other places.
         FreeStyleProject p2 = r.createFreeStyleProject();
         p2.getPublishersList().add(new TestGroovyRecorder(new SecureGroovyScript(
                 String.format(
                         "import org.jenkinsci.plugins.scriptsecurity.testjar.BuildUtil;"
                         + "BuildUtil.setDisplayNameWhitelisted(build, \"%s\");"
                         + "\"\"", testingDisplayName),
                 true,
-                classpath2
+                files2entries(copy2TempDir(jars))
         )));
 
         // Success as approved.

src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ClasspathEntryTest.java

@@ -25,11 +25,18 @@
 package org.jenkinsci.plugins.scriptsecurity.scripts;
 
 import hudson.Functions;
+
+import java.io.File;
 import java.net.URL;
+
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
 import static org.junit.Assert.*;
 
 public class ClasspathEntryTest {
+    @Rule public TemporaryFolder rule = new TemporaryFolder();
 
     @Test public void pathURLConversion() throws Exception {
         if (!Functions.isWindows()) {
@@ -39,9 +46,23 @@ public class ClasspathEntryTest {
         }
         assertEquals("jar:file:/tmp/x.jar!/subjar.jar", "jar:file:/tmp/x.jar!/subjar.jar");
     }
+
     private static void assertRoundTrip(String path, String url) throws Exception {
         assertEquals(path, ClasspathEntry.urlToPath(new URL(url)));
         assertEquals(url, ClasspathEntry.pathToURL(path).toString());
     }
 
+    @Test public void classDirDetected() throws Exception {
+        final File tmpDir = rule.newFolder();
+        assertTrue("Existing directory must be detected", ClasspathEntry.isClassDirectoryURL(tmpDir.toURI().toURL()));
+        tmpDir.delete();
+        final File notExisting = new File(tmpDir, "missing");
+        final URL missing = tmpDir.toURI().toURL();
+        assertFalse("Non-existing file is not considered class directory", ClasspathEntry.isClassDirectoryURL(missing));
+        final URL oneDir = new URL(missing.toExternalForm() + "/");
+        assertTrue("Non-existing file is considered class directory if ending in /", ClasspathEntry.isClassDirectoryURL(oneDir));
+        assertTrue("Generic URLs ending in / are considered class directories", ClasspathEntry.isClassDirectoryURL(new URL("http://example.com/folder/")));
+        assertFalse("Generic URLs ending in / are not considered class directories", ClasspathEntry.isClassDirectoryURL(new URL("http://example.com/file")));
+    }
+
 }