diff --git a/core/src/main/java/hudson/model/FileParameterValue.java b/core/src/main/java/hudson/model/FileParameterValue.java index 2547b00d42..c3e508d941 100644 --- a/core/src/main/java/hudson/model/FileParameterValue.java +++ b/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()) { - listener.getLogger().println("Copying file to " + location); - FilePath ws = build.getWorkspace(); - 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); - // force the build to fail - return null; - } - FilePath locationFilePath = ws.child(location); - locationFilePath.getParent().mkdirs(); + if (location != null && !location.isBlank() && file != null && file.getName() != null && !file.getName().isBlank()) { + try { + listener.getLogger().println("Copying file to " + location); + FilePath ws = build.getWorkspace(); + 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); + // force the build to fail + return null; + } + FilePath locationFilePath = ws.child(location); + locationFilePath.getParent().mkdirs(); - // TODO Remove this workaround after FILEUPLOAD-293 is resolved. - if (locationFilePath.exists() && !locationFilePath.isDirectory()) { - locationFilePath.delete(); + // TODO Remove this workaround after FILEUPLOAD-293 is resolved. + if (locationFilePath.exists() && !locationFilePath.isDirectory()) { + 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 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}. * diff --git a/core/src/test/java/hudson/model/FileParameterValueTest.java b/core/src/test/java/hudson/model/FileParameterValueTest.java deleted file mode 100644 index 26ce24c54f..0000000000 --- a/core/src/test/java/hudson/model/FileParameterValueTest.java +++ /dev/null @@ -1,68 +0,0 @@ -/* - * The MIT License - * - * Copyright 2014 Oleg Nenashev . - * - * 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); - } -} diff --git a/test/src/test/java/hudson/model/FileParameterValuePersistenceTest.java b/test/src/test/java/hudson/model/FileParameterValuePersistenceTest.java index 37acc41f6f..73f23fa8e7 100644 --- a/test/src/test/java/hudson/model/FileParameterValuePersistenceTest.java +++ b/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); + } + } + } diff --git a/test/src/test/java/hudson/model/FileParameterValueTest.java b/test/src/test/java/hudson/model/FileParameterValueTest.java index be0b72010d..7008fa5485 100644 --- a/test/src/test/java/hudson/model/FileParameterValueTest.java +++ b/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; + } }