[JENKINS-73161] Ensure file parameters are retained across Jenkins restarts (#11081)

core/src/main/java/hudson/model/FileParameterValue.java

@@ -26,9 +26,11 @@ package hudson.model;
 
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import hudson.EnvVars;
+import hudson.Extension;
 import hudson.FilePath;
 import hudson.Launcher;
 import hudson.Util;
+import hudson.model.queue.QueueListener;
 import hudson.tasks.BuildWrapper;
 import hudson.util.VariableResolver;
 import java.io.File;
@@ -39,12 +41,17 @@ import java.io.UnsupportedEncodingException;
 import java.nio.charset.Charset;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.List;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 import java.util.regex.Pattern;
+import jenkins.model.Jenkins;
 import jenkins.util.SystemProperties;
 import org.apache.commons.fileupload2.core.FileItem;
 import org.apache.commons.fileupload2.core.FileItemFactory;
 import org.apache.commons.fileupload2.core.FileItemHeaders;
 import org.apache.commons.fileupload2.core.FileItemHeadersProvider;
+import org.apache.commons.io.FileUtils;
 import org.apache.commons.io.FilenameUtils;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
@@ -58,6 +65,9 @@ import org.kohsuke.stapler.StaplerResponse2;
  * @author Kohsuke Kawaguchi
  */
 public class FileParameterValue extends ParameterValue {
+
+    private static final Logger LOGGER = Logger.getLogger(FileParameterValue.class.getName());
+
     private static final String FOLDER_NAME = "fileParameters";
     private static final Pattern PROHIBITED_DOUBLE_DOT = Pattern.compile(".*[\\\\/]\\.\\.[\\\\/].*");
     private static final long serialVersionUID = -143427023159076073L;
@@ -71,13 +81,16 @@ public class FileParameterValue extends ParameterValue {
     public static /* Script Console modifiable */ boolean ALLOW_FOLDER_TRAVERSAL_OUTSIDE_WORKSPACE =
             SystemProperties.getBoolean(FileParameterValue.class.getName() + ".allowFolderTraversalOutsideWorkspace");
 
-    private final transient FileItem file;
+    private transient FileItem file;
 
     /**
      * The name of the originally uploaded file.
      */
     private final String originalFileName;
 
+
+    private String tmpFileName;
+
     /**
      * Overrides the location in the build to place this file. Initially set to {@link #getName()}
      * The location could be directly the filename or also a hierarchical path.
@@ -106,6 +119,16 @@ public class FileParameterValue extends ParameterValue {
 
     protected FileParameterValue(String name, FileItem file, String originalFileName) {
         super(name);
+        try {
+            File dir = new File(Jenkins.get().getRootDir(), "fileParameterValueFiles");
+            Files.createDirectories(dir.toPath());
+            File tmp = Files.createTempFile(dir.toPath(), "jenkins", ".tmp").toFile();
+            FileUtils.copyInputStreamToFile(file.getInputStream(), tmp);
+            tmpFileName = tmp.getAbsolutePath();
+        } catch (IOException e) {
+            throw new RuntimeException(e);
+        }
+
         this.file = file;
         this.originalFileName = originalFileName;
         setLocation(name);
@@ -149,7 +172,17 @@ public class FileParameterValue extends ParameterValue {
         return originalFileName;
     }
 
+    private void createFile() {
+        if (file == null && tmpFileName != null) {
+            File tmp = new File(tmpFileName);
+            if (tmp.exists()) {
+                file = new FileItemImpl2(tmp);
+            }
+        }
+    }
+
     public FileItem getFile2() {
+        createFile();
         return file;
     }
 
@@ -163,31 +196,44 @@ public class FileParameterValue extends ParameterValue {
 
     @Override
     public BuildWrapper createBuildWrapper(AbstractBuild<?, ?> build) {
+        createFile();
         return new BuildWrapper() {
-            @SuppressFBWarnings(value = "NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE", justification = "TODO needs triage")
+            @SuppressFBWarnings(value = {"NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE", "PATH_TRAVERSAL_IN"}, justification = "TODO needs triage, False positive, the path is a temporary file")
             @Override
             public Environment setUp(AbstractBuild build, Launcher launcher, BuildListener listener) throws IOException, InterruptedException {
-                if (location != null && !location.isEmpty() && file.getName() != null && !file.getName().isEmpty()) {
+                if (location != null && !location.isBlank() && file != null && file.getName() != null && !file.getName().isBlank()) {
-                    listener.getLogger().println("Copying file to " + location);
+                    try {
-                    FilePath ws = build.getWorkspace();
+                        listener.getLogger().println("Copying file to " + location);
-                    if (ws == null) {
+                        FilePath ws = build.getWorkspace();
-                        throw new IllegalStateException("The workspace should be created when setUp method is called");
+                        if (ws == null) {
-                    }
+                            throw new IllegalStateException("The workspace should be created when setUp method is called");
-                    if (!ALLOW_FOLDER_TRAVERSAL_OUTSIDE_WORKSPACE && (PROHIBITED_DOUBLE_DOT.matcher(location).matches() || !ws.isDescendant(location))) {
+                        }
-                        listener.error("Rejecting file path escaping base directory with relative path: " + location);
+                        if (!ALLOW_FOLDER_TRAVERSAL_OUTSIDE_WORKSPACE && (PROHIBITED_DOUBLE_DOT.matcher(location).matches() || !ws.isDescendant(location))) {
-                        // force the build to fail
+                            listener.error("Rejecting file path escaping base directory with relative path: " + location);
-                        return null;
+                            // force the build to fail
-                    }
+                            return null;
-                    FilePath locationFilePath = ws.child(location);
+                        }
-                    locationFilePath.getParent().mkdirs();
+                        FilePath locationFilePath = ws.child(location);
+                        locationFilePath.getParent().mkdirs();
 
-                    // TODO Remove this workaround after FILEUPLOAD-293 is resolved.
+                        // TODO Remove this workaround after FILEUPLOAD-293 is resolved.
-                    if (locationFilePath.exists() && !locationFilePath.isDirectory()) {
+                        if (locationFilePath.exists() && !locationFilePath.isDirectory()) {
-                        locationFilePath.delete();
+                            locationFilePath.delete();
+                        }
+                        locationFilePath.copyFrom(file);
+                        locationFilePath.copyTo(new FilePath(getLocationUnderBuild(build)));
+                    } finally {
+                        if (tmpFileName != null) {
+                            File tmp = new File(tmpFileName);
+                            try {
+                                Files.deleteIfExists(tmp.toPath());
+                            } catch (IOException e) {
+                                LOGGER.log(Level.WARNING, "Unable to delete temporary file {0} for parameter {1} of job {2}",
+                                        new Object[]{tmp.getAbsolutePath(), getName(), build.getParent().getName()});
+                            }
+                        }
+                        tmpFileName = null;
                     }
-
-                    locationFilePath.copyFrom(file);
-                    locationFilePath.copyTo(new FilePath(getLocationUnderBuild(build)));
                 }
                 return new Environment() {};
             }
@@ -257,6 +303,36 @@ public class FileParameterValue extends ParameterValue {
         return new File(build.getRootDir(), FOLDER_NAME);
     }
 
+    @Extension
+    public static class CancelledQueueListener extends QueueListener {
+
+        @Override
+        public void onLeft(Queue.LeftItem li) {
+            if (li.isCancelled()) {
+                List<ParametersAction> actions = li.getActions(ParametersAction.class);
+                actions.forEach(a -> {
+                    a.getAllParameters().stream()
+                            .filter(p -> p instanceof FileParameterValue)
+                            .map(p -> (FileParameterValue) p)
+                            .forEach(this::deleteTmpFile);
+                });
+            }
+        }
+
+        @SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "False positive, the path is a temporary file")
+        private void deleteTmpFile(FileParameterValue p) {
+            if (p.tmpFileName != null) {
+                File tmp = new File(p.tmpFileName);
+                try {
+                    Files.deleteIfExists(tmp.toPath());
+                } catch (IOException e) {
+                    LOGGER.log(Level.WARNING, "Unable to delete temporary file {0} for parameter {1}",
+                            new Object[]{tmp.getAbsolutePath(), p.getName()});
+                }
+            }
+        }
+    }
+
     /**
      * Default implementation from {@link File}.
      *

core/src/test/java/hudson/model/FileParameterValueTest.java

@@ -1,68 +0,0 @@
-/*
- * The MIT License
- *
- * Copyright 2014 Oleg Nenashev <o.v.nenashev@gmail.com>.
- *
- * 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
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
- * THE SOFTWARE.
- */
-
-package hudson.model;
-
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertNotEquals;
-
-import java.io.File;
-import org.junit.jupiter.api.Test;
-import org.jvnet.hudson.test.Issue;
-
-/**
- * Test for {@link FileParameterValue}.
- * @author Oleg Nenashev
- */
-class FileParameterValueTest {
-
-    @Issue("JENKINS-19017")
-    @Test
-    void compareParamsWithSameName() {
-        final String paramName = "MY_FILE_PARAM"; // Same paramName (location) reproduces the bug
-        final FileParameterValue param1 = new FileParameterValue(paramName, new File("ws_param1.txt"), "param1.txt");
-        final FileParameterValue param2 = new FileParameterValue(paramName, new File("ws_param2.txt"), "param2.txt");
-
-        assertNotEquals(param1, param2, "Files with same locations should be considered as different");
-        assertNotEquals(param2, param1, "Files with same locations should be considered as different");
-    }
-
-    @Test
-    void compareNullParams() {
-        final String paramName = "MY_FILE_PARAM";
-        FileParameterValue nonNullParam = new FileParameterValue(paramName, new File("ws_param1.txt"), "param1.txt");
-        FileParameterValue nullParam1 = new FileParameterValue(null, new File("null_param1.txt"), "null_param1.txt");
-        FileParameterValue nullParam2 = new FileParameterValue(null, new File("null_param2.txt"), "null_param2.txt");
-
-        // Combine nulls
-        assertEquals(nullParam1, nullParam1);
-        assertEquals(nullParam1, nullParam2);
-        assertEquals(nullParam2, nullParam1);
-        assertEquals(nullParam2, nullParam2);
-
-        // Compare with non-null
-        assertNotEquals(nullParam1, nonNullParam);
-        assertNotEquals(nonNullParam, nullParam1);
-    }
-}

test/src/test/java/hudson/model/FileParameterValuePersistenceTest.java

@@ -5,21 +5,34 @@ import static org.hamcrest.Matchers.containsString;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
+import hudson.ExtensionList;
 import hudson.Functions;
+import hudson.model.queue.CauseOfBlockage;
+import hudson.model.queue.QueueTaskDispatcher;
 import hudson.tasks.BatchFile;
 import hudson.tasks.Shell;
 import java.io.File;
+import java.net.URL;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.Collections;
+import jenkins.model.Jenkins;
+import org.apache.commons.io.FileUtils;
+import org.htmlunit.FormEncodingType;
+import org.htmlunit.HttpMethod;
+import org.htmlunit.WebRequest;
 import org.htmlunit.html.HtmlForm;
 import org.htmlunit.html.HtmlInput;
 import org.htmlunit.html.HtmlPage;
+import org.htmlunit.util.KeyDataPair;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
 import org.junit.jupiter.api.io.TempDir;
 import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.MockAuthorizationStrategy;
+import org.jvnet.hudson.test.TestExtension;
 import org.jvnet.hudson.test.junit.jupiter.JenkinsSessionExtension;
 
 class FileParameterValuePersistenceTest {
@@ -78,4 +91,39 @@ class FileParameterValuePersistenceTest {
             assertThat(page.getWebResponse().getContentAsString(), containsString(FILENAME));
         }
     }
+
+    @Issue("JENKINS-73161")
+    @Test
+    void fileParameterValueIsRetained() throws Throwable {
+        sessions.then(r -> {
+            r.jenkins.setSecurityRealm(r.createDummySecurityRealm());
+            r.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy().grant(Jenkins.ADMINISTER).everywhere().to("admin"));
+            FreeStyleProject p = r.createFreeStyleProject("p");
+            p.addProperty(new ParametersDefinitionProperty(new FileParameterDefinition("FILE")));
+            WebRequest req = new WebRequest(new URL(r.getURL() + "job/p/buildWithParameters"), HttpMethod.POST);
+            File f = File.createTempFile("junit", null, tmp);
+            FileUtils.write(f, "uploaded content here", "UTF-8");
+            req.setEncodingType(FormEncodingType.MULTIPART);
+            req.setRequestParameters(Collections.singletonList(new KeyDataPair("FILE", f, "myfile.txt", "text/plain", "UTF-8")));
+            r.createWebClient().withBasicApiToken("admin").getPage(req);
+        });
+        sessions.then(r -> {
+            ExtensionList.lookupSingleton(Block.class).ready = true;
+            FreeStyleProject p = r.jenkins.getItemByFullName("p", FreeStyleProject.class);
+            r.waitUntilNoActivity();
+            FreeStyleBuild b = p.getBuildByNumber(1);
+            r.assertBuildStatusSuccess(b);
+        });
+    }
+
+    @TestExtension("fileParameterValueIsRetained")
+    public static final class Block extends QueueTaskDispatcher {
+        private boolean ready;
+
+        @Override
+        public CauseOfBlockage canTake(Node node, Queue.BuildableItem item) {
+            return ready ? null : new CauseOfBlockage.BecauseNodeIsBusy(node);
+        }
+    }
+
 }

test/src/test/java/hudson/model/FileParameterValueTest.java

@@ -31,16 +31,19 @@ import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasItem;
 import static org.hamcrest.Matchers.not;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import hudson.FilePath;
 import hudson.Functions;
 import java.io.File;
+import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.util.Arrays;
 import java.util.List;
 import java.util.stream.Collectors;
+import org.apache.commons.io.FileUtils;
 import org.htmlunit.Page;
 import org.htmlunit.html.HtmlPage;
 import org.htmlunit.util.NameValuePair;
@@ -427,4 +430,45 @@ class FileParameterValueTest {
             }
         }
     }
+
+    @Issue("JENKINS-19017")
+    @Test
+    void compareParamsWithSameName() throws IOException {
+        final String paramName = "MY_FILE_PARAM"; // Same paramName (location) reproduces the bug
+        File ws_param1 = createParamFile("ws_param1.txt");
+        File ws_param2 = createParamFile("ws_param2.txt");
+
+        final FileParameterValue param1 = new FileParameterValue(paramName, ws_param1, "param1.txt");
+        final FileParameterValue param2 = new FileParameterValue(paramName, ws_param2, "param2.txt");
+
+        assertNotEquals(param1, param2, "Files with same locations should be considered as different");
+        assertNotEquals(param2, param1, "Files with same locations should be considered as different");
+    }
+
+    @Test
+    void compareNullParams() throws IOException {
+        final String paramName = "MY_FILE_PARAM";
+        File ws_param1 = createParamFile("ws_param1.txt");
+        File null_param1 = createParamFile("null_param1.txt");
+        File null_param2 = createParamFile("null_param2.txt");
+        FileParameterValue nonNullParam = new FileParameterValue(paramName, ws_param1, "param1.txt");
+        FileParameterValue nullParam1 = new FileParameterValue(null, null_param1, "null_param1.txt");
+        FileParameterValue nullParam2 = new FileParameterValue(null, null_param2, "null_param2.txt");
+
+        // Combine nulls
+        assertEquals(nullParam1, nullParam1);
+        assertEquals(nullParam1, nullParam2);
+        assertEquals(nullParam2, nullParam1);
+        assertEquals(nullParam2, nullParam2);
+
+        // Compare with non-null
+        assertNotEquals(nullParam1, nonNullParam);
+        assertNotEquals(nonNullParam, nullParam1);
+    }
+
+    private File createParamFile(String fileName) throws IOException {
+        File f = new File(tmp, fileName);
+        FileUtils.writeStringToFile(f, "content", StandardCharsets.UTF_8);
+        return f;
+    }
 }