Merge pull request #13 from jglick/temp-dir-JENKINS-27152

[JENKINS-32943] Use a standardized temporary directory for secretFiles
jenkinsci · Mar 4, 2016 · ed93648 · ed93648
2 parents 689655d + 91d963e
commit ed93648
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 10 deletions.
diff --git a/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/FileBinding.java b/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/FileBinding.java
@@ -27,10 +27,9 @@
 import hudson.Extension;
 import hudson.FilePath;
 import hudson.Launcher;
-import hudson.model.Computer;
-import hudson.model.Node;
 import hudson.model.Run;
 import hudson.model.TaskListener;
+import hudson.slaves.WorkspaceList;
 import java.io.IOException;
 import java.util.UUID;
 
@@ -86,10 +85,12 @@ private static class UnbinderImpl implements Unbinder {
     }
 
     private static FilePath secretsDir(FilePath workspace) {
-        Computer computer = workspace.toComputer();
-        Node node = computer == null ? null : computer.getNode();
-        FilePath root = node == null ? workspace : node.getRootPath();
-        return root.child("secretFiles");
+        return tempDir(workspace).child("secretFiles");
+    }
+
+    // TODO 1.652 use WorkspaceList.tempDir
+    private static FilePath tempDir(FilePath ws) {
+        return ws.sibling(ws.getName() + System.getProperty(WorkspaceList.class.getName(), "@") + "tmp");
     }
 
     protected void copy(FilePath secret, FileCredentials credentials) throws IOException, InterruptedException {

diff --git a/src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html b/src/main/resources/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep/help.html
@@ -22,3 +22,37 @@
     usually as a <code>credentialsId</code> parameter,
     in which case this step is unnecessary.
 </p>
+<p>
+    For bindings which store a secret file, beware that
+</p>
+<pre><code>
+node {
+  dir('subdir') {
+    withCredentials([[$class: 'FileBinding', credentialsId: 'secret', variable: 'FILE']]) {
+      sh 'use $FILE'
+    }
+  }
+}</code></pre>
+<p>
+    is not safe, as <code>$FILE</code> might be inside the workspace (in <code>subdir@tmp/secretFiles/</code>),
+    and thus visible to anyone able to browse the job’s workspace.
+    If you need to run steps in a different directory than the usual workspace, you should instead use
+</p>
+<pre><code>
+node {
+  withCredentials([[$class: 'FileBinding', credentialsId: 'secret', variable: 'FILE']]) {
+    dir('subdir') {
+      sh 'use $FILE'
+    }
+  }
+}</code></pre>
+<p>
+    to ensure that the secrets are outside the workspace; or choose a different workspace entirely:
+</p>
+node {
+  ws {
+    withCredentials([[$class: 'FileBinding', credentialsId: 'secret', variable: 'FILE']]) {
+      sh 'use $FILE'
+    }
+  }
+}</code></pre>
diff --git a/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStepTest.java b/src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStepTest.java
@@ -40,6 +40,7 @@
 import hudson.slaves.DumbSlave;
 import hudson.slaves.NodeProperty;
 import hudson.slaves.RetentionStrategy;
+import hudson.slaves.WorkspaceList;
 import hudson.util.Secret;
 
 import java.io.File;
@@ -56,7 +57,6 @@
 import org.jenkinsci.plugins.authorizeproject.AuthorizeProjectProperty;
 import org.jenkinsci.plugins.authorizeproject.ProjectQueueItemAuthenticator;
 import org.jenkinsci.plugins.authorizeproject.strategy.SpecificUsersAuthorizationStrategy;
-import org.jenkinsci.plugins.authorizeproject.AuthorizeProjectStrategy;
 import org.jenkinsci.plugins.credentialsbinding.MultiBinding;
 import org.jenkinsci.plugins.plaincredentials.impl.FileCredentialsImpl;
 import org.jenkinsci.plugins.plaincredentials.impl.StringCredentialsImpl;
@@ -77,7 +77,6 @@
 import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.RestartableJenkinsRule;
-import org.jvnet.hudson.test.recipes.WithPlugin;
 
 public class BindingStepTest {
 
@@ -201,17 +200,23 @@ public class BindingStepTest {
                 }
                 story.j.assertBuildStatusSuccess(b);
                 story.j.assertLogNotContains(secret, b);
-                FilePath key = story.j.jenkins.getNode("myslave").getWorkspaceFor(p).child("key");
+                FilePath ws = story.j.jenkins.getNode("myslave").getWorkspaceFor(p);
+                FilePath key = ws.child("key");
                 assertTrue(key.exists());
                 assertEquals(secret, key.readToString());
-                FilePath secretFiles = story.j.jenkins.getNode("myslave").getRootPath().child("secretFiles");
+                FilePath secretFiles = tempDir(ws).child("secretFiles");
                 assertTrue(secretFiles.isDirectory());
                 assertEquals(Collections.emptyList(), secretFiles.list());
                 assertEquals(Collections.<String>emptySet(), grep(b.getRootDir(), secret));
             }
         });
     }
 
+    // TODO 1.652 use WorkspaceList.tempDir
+    private static FilePath tempDir(FilePath ws) {
+        return ws.sibling(ws.getName() + System.getProperty(WorkspaceList.class.getName(), "@") + "tmp");
+    }
+
     @Issue("JENKINS-27389")
     @Test public void grabEnv() {
         story.addStep(new Statement() {