[FIXED JENKINS-31321] protect against node-rename corruption

This change adds code to check that the user isn't attempting to rename an existing node with the name of another existing node. Previous to this change, such rename operations would succeed and would corrupt node configuration data. (cherry picked from commit 16d6429)
jenkinsci · Feb 3, 2016 · df2b7d9 · df2b7d9
1 parent 90b3ce5
commit df2b7d9
Show file tree

Hide file tree

Showing 2 changed files with 46 additions and 4 deletions.
diff --git a/core/src/main/java/hudson/model/Computer.java b/core/src/main/java/hudson/model/Computer.java
@@ -2,7 +2,8 @@
  * The MIT License
  *
  * Copyright (c) 2004-2010, Sun Microsystems, Inc., Kohsuke Kawaguchi,
- * Red Hat, Inc., Seiji Sogabe, Stephen Connolly, Thomas J. Black, Tom Huybrechts, CloudBees, Inc.
+ * Red Hat, Inc., Seiji Sogabe, Stephen Connolly, Thomas J. Black, Tom Huybrechts,
+ * CloudBees, Inc., Christopher Simons
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal
@@ -1384,13 +1385,19 @@ protected void _doScript(StaplerRequest req, StaplerResponse rsp, String view) t
     public void doConfigSubmit( StaplerRequest req, StaplerResponse rsp ) throws IOException, ServletException, FormException {
         checkPermission(CONFIGURE);
 
-        String name = Util.fixEmptyAndTrim(req.getSubmittedForm().getString("name"));
-        Jenkins.checkGoodName(name);
+        String proposedName = Util.fixEmptyAndTrim(req.getSubmittedForm().getString("name"));
+        Jenkins.checkGoodName(proposedName);
 
         Node node = getNode();
         if (node == null) {
             throw new ServletException("No such node " + nodeName);
         }
+
+        if ((!proposedName.equals(nodeName))
+                && Jenkins.getActiveInstance().getNode(proposedName) != null) {
+            throw new FormException(Messages.ComputerSet_SlaveAlreadyExists(proposedName), "name");
+        }
+
         Node result = node.reconfigure(req, req.getSubmittedForm());
         replaceBy(result);
 

diff --git a/test/src/test/java/hudson/model/ComputerTest.java b/test/src/test/java/hudson/model/ComputerTest.java
@@ -1,7 +1,7 @@
 /*
  * The MIT License
  *
- * Copyright (c) 2015 Red Hat, Inc.
+ * Copyright (c) 2015 Red Hat, Inc.; Christopher Simons
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal
@@ -23,16 +23,26 @@
  */
 package hudson.model;
 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.equalTo;
 import static org.junit.Assert.*;
 
+import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException;
+import com.gargoylesoftware.htmlunit.html.HtmlForm;
+import com.gargoylesoftware.htmlunit.html.HtmlPage;
+
 import java.io.File;
 
 import jenkins.model.Jenkins;
 import hudson.slaves.DumbSlave;
 
+import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.JenkinsRule.WebClient;
 
 public class ComputerTest {
 
@@ -52,4 +62,29 @@ public void discardLogsAfterDeletion() throws Exception {
 
         assertTrue("Slave log should be kept", keep.toComputer().getLogFile().exists());
     }
+
+    /**
+     * Verify we can't rename a node over an existing node.
+     */
+    @Issue("JENKINS-31321")
+    @Test
+    public void testProhibitRenameOverExistingNode() throws Exception {
+        final String NOTE = "Rename node to name of another node should fail.";
+
+        Node nodeA = j.createSlave("nodeA", null, null);
+        Node nodeB = j.createSlave("nodeB", null, null);
+
+        WebClient wc = j.createWebClient();
+        HtmlForm form = wc.getPage(nodeB, "configure").getFormByName("config");
+        form.getInputByName("_.name").setValueAttribute("nodeA");
+
+        try {
+            j.submit(form);
+            fail(NOTE);
+        } catch (FailingHttpStatusCodeException e) {
+            assertThat(NOTE, e.getStatusCode(), equalTo(400));
+            assertThat(NOTE, e.getResponse().getContentAsString(),
+                    containsString("Slave called ‘nodeA’ already exists"));
+        }
+    }
 }