diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index 7183937d2d..8629d44ae0 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -131,7 +131,7 @@ public class Util { public static String replaceMacro( @CheckForNull String s, @Nonnull Map properties) { return replaceMacro(s,new VariableResolver.ByMap(properties)); } - + /** * Replaces the occurrence of '$key' by resolver.get('key'). * @@ -143,7 +143,7 @@ public class Util { if (s == null) { return null; } - + int idx=0; while(true) { Matcher m = VARIABLE.matcher(s); @@ -370,6 +370,30 @@ public class Util { } } + /** + * A mostly accurate check of whether a path is a relative path or not. This is designed to take a path against + * an unknown operating system so may give invalid results. + * + * @param path the path. + * @return {@code true} if the path looks relative. + * @since 1.FIXME + */ + public static boolean isRelativePath(String path) { + if (path.startsWith("/")) + return false; + if (path.startsWith("\\\\") && path.length() > 3 && path.indexOf('\\', 3) != -1) + return false; // a UNC path which is the most absolute you can get on windows + if (path.length() >= 3 && ':' == path.charAt(1)) { + // never mind that the drive mappings can be changed between sessions, we just want to + // know if the 3rd character is a `\` (or a '/' is acceptable too) + char p = path.charAt(0); + if (('A' <= p && p <= 'Z') || ('a' <= p && p <= 'z')) { + return path.charAt(2) != '\\' && path.charAt(2) != '/'; + } + } + return true; + } + /** * Creates a new temporary directory. */ @@ -624,7 +648,7 @@ public class Util { * Converts a string into 128-bit AES key. * @since 1.308 */ - @Nonnull + @Nonnull public static SecretKey toAes128Key(@Nonnull String s) { try { // turn secretKey into 256 bit hash @@ -743,9 +767,9 @@ public class Util { /** * Combines number and unit, with a plural suffix if needed. - * - * @deprecated - * Use individual localization methods instead. + * + * @deprecated + * Use individual localization methods instead. * See {@link Messages#Util_year(Object)} for an example. * Deprecated since 2009-06-24, remove method after 2009-12-24. */ @@ -780,7 +804,7 @@ public class Util { * {@link #rawEncode(String)} should generally be used instead, though be careful to pass only * a single path component to that method (it will encode /, but this method does not). */ - @Nonnull + @Nonnull public static String encode(@Nonnull String s) { try { boolean escaped = false; @@ -888,7 +912,7 @@ public class Util { /** * Escapes HTML unsafe characters like <, & to the respective character entities. */ - @Nonnull + @Nonnull public static String escape(@Nonnull String text) { if (text==null) return null; StringBuilder buf = new StringBuilder(text.length()+64); @@ -1112,7 +1136,7 @@ public class Util { * @param symlinkPath * Where to create a symlink in (relative to {@code baseDir}) */ - public static void createSymlink(@Nonnull File baseDir, @Nonnull String targetPath, + public static void createSymlink(@Nonnull File baseDir, @Nonnull String targetPath, @Nonnull String symlinkPath, @Nonnull TaskListener listener) throws InterruptedException { try { if (createSymlinkJava7(baseDir, targetPath, symlinkPath)) { @@ -1346,7 +1370,7 @@ public class Util { * @deprecated since 2008-05-13. This method is broken (see ISSUE#1666). It should probably * be removed but I'm not sure if it is considered part of the public API * that needs to be maintained for backwards compatibility. - * Use {@link #encode(String)} instead. + * Use {@link #encode(String)} instead. */ @Deprecated public static String encodeRFC2396(String url) { @@ -1367,7 +1391,7 @@ public class Util { s = ""+s+""; return s; } - + /** * Returns the parsed string if parsed successful; otherwise returns the default number. * If the string is null, empty or a ParseException is thrown then the defaultNumber diff --git a/core/src/main/java/hudson/model/Slave.java b/core/src/main/java/hudson/model/Slave.java index 73f45f7e59..6a0a955af5 100644 --- a/core/src/main/java/hudson/model/Slave.java +++ b/core/src/main/java/hudson/model/Slave.java @@ -434,9 +434,7 @@ public abstract class Slave extends Node implements Serializable { if(value.startsWith("\\\\") || value.startsWith("/net/")) return FormValidation.warning(Messages.Slave_Network_Mounted_File_System_Warning()); - if (!value.contains("\\") && !value.startsWith("/")) { - // Unix-looking path that doesn't start with '/' - // TODO: detect Windows-looking relative path + if (Util.isRelativePath(value)) { return FormValidation.warning(Messages.Slave_Remote_Relative_Path_Warning()); } diff --git a/core/src/main/java/hudson/slaves/SlaveComputer.java b/core/src/main/java/hudson/slaves/SlaveComputer.java index 627f22d9e2..faadd7fc8c 100644 --- a/core/src/main/java/hudson/slaves/SlaveComputer.java +++ b/core/src/main/java/hudson/slaves/SlaveComputer.java @@ -499,7 +499,7 @@ public class SlaveComputer extends Computer { } String absoluteRemoteFs = node.getRemoteFS(); - if (isRelativePath(absoluteRemoteFs)) { + if (Util.isRelativePath(absoluteRemoteFs)) { absoluteRemoteFs = channel.call(new AbsolutePath(absoluteRemoteFs)); log.println("NOTE: Relative remote path resolved to: "+absoluteRemoteFs); } @@ -559,22 +559,6 @@ public class SlaveComputer extends Computer { Jenkins.getInstance().getQueue().scheduleMaintenance(); } - /*package*/ static boolean isRelativePath(String path) { - if (path.startsWith("/")) - return false; - if (path.startsWith("\\\\") && path.length() > 3 && path.indexOf('\\', 3) != -1) - return false; // a UNC path which is the most absolute you can get on windows - if (path.length() >= 3 && ':' == path.charAt(1)) { - // never mind that the drive mappings can be changed between sessions, we just want to - // know if the 3rd character is a `\` (or a '/' is acceptable too) - char p = path.charAt(0); - if (('A' <= p && p <= 'Z') || ('a' <= p && p <= 'z')) { - return path.charAt(2) != '\\' && path.charAt(2) != '/'; - } - } - return true; - } - @Override public Channel getChannel() { return channel; diff --git a/core/src/test/java/hudson/UtilTest.java b/core/src/test/java/hudson/UtilTest.java index 788be00ca2..5be69d004b 100644 --- a/core/src/test/java/hudson/UtilTest.java +++ b/core/src/test/java/hudson/UtilTest.java @@ -1,19 +1,19 @@ /* * The MIT License - * + * * Copyright (c) 2004-2010, Sun Microsystems, Inc., Kohsuke Kawaguchi, * Daniel Dyer, Erik Ramfelt, Richard Bair, id:cactusman - * + * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal * in the Software without restriction, including without limitation the rights * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell * copies of the Software, and to permit persons to whom the Software is * furnished to do so, subject to the following conditions: - * + * * The above copyright notice and this permission notice shall be included in * all copies or substantial portions of the Software. - * + * * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE @@ -32,7 +32,11 @@ import java.io.ByteArrayOutputStream; import java.io.File; import java.io.IOException; +import static org.hamcrest.CoreMatchers.not; import static org.junit.Assert.*; + +import org.hamcrest.BaseMatcher; +import org.hamcrest.Description; import org.junit.Assume; import org.junit.Test; import org.jvnet.hudson.test.Issue; @@ -74,7 +78,7 @@ public class UtilTest { assertEquals("asd$${AA}dd", Util.replaceMacro("asd$$$${AA}dd",m)); assertEquals("$", Util.replaceMacro("$$",m)); assertEquals("$$", Util.replaceMacro("$$$$",m)); - + // dots assertEquals("a.B", Util.replaceMacro("$A.B", m)); assertEquals("a-b", Util.replaceMacro("${A.B}", m)); @@ -136,7 +140,7 @@ public class UtilTest { String encoded = Util.encode(urlWithSpaces); assertEquals(encoded, "http://hudson/job/Hudson%20Job"); } - + /** * Test the rawEncode() method. */ @@ -190,16 +194,16 @@ public class UtilTest { System.err.println("log output: " + log); assertEquals(buf.toString(),Util.resolveSymlink(new File(d,"x"))); - - + + // test linking from another directory File anotherDir = new File(d,"anotherDir"); assertTrue("Couldn't create "+anotherDir,anotherDir.mkdir()); - + Util.createSymlink(d,"a","anotherDir/link",l); assertEquals("a",Util.resolveSymlink(new File(d,"anotherDir/link"))); - - // JENKINS-12331: either a bug in createSymlink or this isn't supposed to work: + + // JENKINS-12331: either a bug in createSymlink or this isn't supposed to work: //assertTrue(Util.isSymlink(new File(d,"anotherDir/link"))); File external = File.createTempFile("something", ""); @@ -213,11 +217,11 @@ public class UtilTest { Util.deleteRecursive(d); } } - + @Test public void testIsSymlink() throws IOException, InterruptedException { Assume.assumeTrue(!Functions.isWindows()); - + ByteArrayOutputStream baos = new ByteArrayOutputStream(); StreamTaskListener l = new StreamTaskListener(baos); File d = tmp.getRoot(); @@ -225,17 +229,17 @@ public class UtilTest { new FilePath(new File(d, "original")).touch(0); assertFalse(Util.isSymlink(new File(d, "original"))); Util.createSymlink(d,"original","link", l); - + assertTrue(Util.isSymlink(new File(d, "link"))); - + // test linking to another directory File dir = new File(d,"dir"); assertTrue("Couldn't create "+dir,dir.mkdir()); assertFalse(Util.isSymlink(new File(d,"dir"))); - + File anotherDir = new File(d,"anotherDir"); assertTrue("Couldn't create "+anotherDir,anotherDir.mkdir()); - + Util.createSymlink(d,"dir","anotherDir/symlinkDir",l); // JENKINS-12331: either a bug in createSymlink or this isn't supposed to work: // assertTrue(Util.isSymlink(new File(d,"anotherDir/symlinkDir"))); @@ -276,7 +280,7 @@ public class UtilTest { assertEquals("'"", Util.escape("'\"")); assertEquals("  ", Util.escape(" ")); } - + /** * Compute 'known-correct' digests and see if I still get them when computed concurrently * to another digest. @@ -286,19 +290,19 @@ public class UtilTest { public void testDigestThreadSafety() throws InterruptedException { String a = "abcdefgh"; String b = "123456789"; - + String digestA = Util.getDigestOf(a); String digestB = Util.getDigestOf(b); - + DigesterThread t1 = new DigesterThread(a, digestA); DigesterThread t2 = new DigesterThread(b, digestB); - + t1.start(); t2.start(); - + t1.join(); t2.join(); - + if (t1.error != null) { fail(t1.error); } @@ -306,18 +310,18 @@ public class UtilTest { fail(t2.error); } } - + private static class DigesterThread extends Thread { private String string; private String expectedDigest; - + private String error; public DigesterThread(String string, String expectedDigest) { this.string = string; this.expectedDigest = expectedDigest; } - + public void run() { for (int i=0; i < 1000; i++) { String digest = Util.getDigestOf(this.string); @@ -349,4 +353,56 @@ public class UtilTest { assertEquals(p.toString(), "va.l.ue", p.get("k.e.y")); assertEquals(p.toString(), 1, p.size()); } + + @Test + public void isRelativePathUnix() { + assertThat("/", not(aRelativePath())); + assertThat("/foo/bar", not(aRelativePath())); + assertThat("/foo/../bar", not(aRelativePath())); + assertThat("", aRelativePath()); + assertThat(".", aRelativePath()); + assertThat("..", aRelativePath()); + assertThat("./foo", aRelativePath()); + assertThat("./foo/bar", aRelativePath()); + assertThat("./foo/bar/", aRelativePath()); + } + + @Test + public void isRelativePathWindows() { + assertThat("\\", aRelativePath()); + assertThat("\\foo\\bar", aRelativePath()); + assertThat("\\foo\\..\\bar", aRelativePath()); + assertThat("", aRelativePath()); + assertThat(".", aRelativePath()); + assertThat(".\\foo", aRelativePath()); + assertThat(".\\foo\\bar", aRelativePath()); + assertThat(".\\foo\\bar\\", aRelativePath()); + assertThat("\\\\foo", aRelativePath()); + assertThat("\\\\foo\\", not(aRelativePath())); + assertThat("\\\\foo\\c", not(aRelativePath())); + assertThat("C:", aRelativePath()); + assertThat("z:", aRelativePath()); + assertThat("0:", aRelativePath()); + assertThat("c:.", aRelativePath()); + assertThat("c:\\", not(aRelativePath())); + assertThat("c:/", not(aRelativePath())); + } + + private static RelativePathMatcher aRelativePath() { + return new RelativePathMatcher(); + } + + private static class RelativePathMatcher extends BaseMatcher { + + @Override + public boolean matches(Object item) { + return Util.isRelativePath((String) item); + } + + @Override + public void describeTo(Description description) { + description.appendText("a relative path"); + } + } + } diff --git a/core/src/test/java/hudson/slaves/SlaveComputerTest.java b/core/src/test/java/hudson/slaves/SlaveComputerTest.java deleted file mode 100644 index 9c22642fb4..0000000000 --- a/core/src/test/java/hudson/slaves/SlaveComputerTest.java +++ /dev/null @@ -1,65 +0,0 @@ -package hudson.slaves; - -import org.hamcrest.BaseMatcher; -import org.hamcrest.Description; -import org.junit.Test; - -import static org.hamcrest.CoreMatchers.is; -import static org.hamcrest.CoreMatchers.not; -import static org.junit.Assert.assertThat; - -/** - * @author Stephen Connolly - */ -public class SlaveComputerTest { - @Test - public void isRelativePathUnix() { - assertThat("/", not(aRelativePath())); - assertThat("/foo/bar", not(aRelativePath())); - assertThat("/foo/../bar", not(aRelativePath())); - assertThat("", aRelativePath()); - assertThat(".", aRelativePath()); - assertThat("..", aRelativePath()); - assertThat("./foo", aRelativePath()); - assertThat("./foo/bar", aRelativePath()); - assertThat("./foo/bar/", aRelativePath()); - } - - @Test - public void isRelativePathWindows() { - assertThat("\\", aRelativePath()); - assertThat("\\foo\\bar", aRelativePath()); - assertThat("\\foo\\..\\bar", aRelativePath()); - assertThat("", aRelativePath()); - assertThat(".", aRelativePath()); - assertThat(".\\foo", aRelativePath()); - assertThat(".\\foo\\bar", aRelativePath()); - assertThat(".\\foo\\bar\\", aRelativePath()); - assertThat("\\\\foo", aRelativePath()); - assertThat("\\\\foo\\", not(aRelativePath())); - assertThat("\\\\foo\\c", not(aRelativePath())); - assertThat("C:", aRelativePath()); - assertThat("z:", aRelativePath()); - assertThat("0:", aRelativePath()); - assertThat("c:.", aRelativePath()); - assertThat("c:\\", not(aRelativePath())); - assertThat("c:/", not(aRelativePath())); - } - - private static RelativePathMatcher aRelativePath() { - return new RelativePathMatcher(); - } - - private static class RelativePathMatcher extends BaseMatcher { - - @Override - public boolean matches(Object item) { - return SlaveComputer.isRelativePath((String)item); - } - - @Override - public void describeTo(Description description) { - description.appendText("a relative path"); - } - } -} diff --git a/test/src/test/groovy/hudson/model/SlaveTest.groovy b/test/src/test/groovy/hudson/model/SlaveTest.groovy index 72ea36c73c..83620674f4 100644 --- a/test/src/test/groovy/hudson/model/SlaveTest.groovy +++ b/test/src/test/groovy/hudson/model/SlaveTest.groovy @@ -96,7 +96,7 @@ class SlaveTest { def d = j.jenkins.getDescriptorByType(DumbSlave.DescriptorImpl.class) assert d.doCheckRemoteFS("c:\\")==FormValidation.ok(); assert d.doCheckRemoteFS("/tmp")==FormValidation.ok(); - assert d.doCheckRemoteFS("relative/path").kind==ERROR; + assert d.doCheckRemoteFS("relative/path").kind==WARNING; assert d.doCheckRemoteFS("/net/foo/bar/zot").kind==WARNING; assert d.doCheckRemoteFS("\\\\machine\\folder\\foo").kind==WARNING; }