diff --git a/pom.xml b/pom.xml index a11c36a5..b5a3bd2d 100644 --- a/pom.xml +++ b/pom.xml @@ -76,6 +76,11 @@ THE SOFTWARE. import pom + + org.jenkins-ci.plugins + scm-api + 636.veb_e3d6f5b_820 + @@ -212,6 +217,12 @@ THE SOFTWARE. junit test + + org.awaitility + awaitility + 4.2.0 + test + diff --git a/src/main/java/org/jenkinsci/plugins/workflow/multibranch/ReadTrustedStep.java b/src/main/java/org/jenkinsci/plugins/workflow/multibranch/ReadTrustedStep.java index a06fff47..052306fb 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/multibranch/ReadTrustedStep.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/multibranch/ReadTrustedStep.java @@ -177,7 +177,7 @@ public static class Execution extends AbstractSynchronousNonBlockingStepExecutio } build.addAction(new SCMRevisionAction(scmSource, tip)); } - SCMRevision trusted = scmSource.getTrustedRevision(tip, listener); + SCMRevision trusted = scmSource.getTrustedRevisionForBuild(tip, listener, build); boolean trustCheck = !tip.equals(trusted); String untrustedFile = null; String content; diff --git a/src/main/java/org/jenkinsci/plugins/workflow/multibranch/SCMBinder.java b/src/main/java/org/jenkinsci/plugins/workflow/multibranch/SCMBinder.java index 88ed877c..1a8851db 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/multibranch/SCMBinder.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/multibranch/SCMBinder.java @@ -25,17 +25,15 @@ package org.jenkinsci.plugins.workflow.multibranch; import edu.umd.cs.findbugs.annotations.NonNull; +import hudson.AbortException; import hudson.Extension; import hudson.Functions; -import hudson.MarkupText; -import hudson.console.ConsoleAnnotationDescriptor; -import hudson.console.ConsoleAnnotator; -import hudson.console.ConsoleNote; import hudson.model.Action; import hudson.model.Descriptor; import hudson.model.DescriptorVisibilityFilter; import hudson.model.ItemGroup; import hudson.model.Queue; +import hudson.model.Result; import hudson.model.TaskListener; import hudson.scm.SCM; import java.io.IOException; @@ -46,6 +44,7 @@ import jenkins.scm.api.SCMRevision; import jenkins.scm.api.SCMRevisionAction; import jenkins.scm.api.SCMSource; +import jenkins.util.SystemProperties; import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; import org.jenkinsci.plugins.workflow.cps.CpsScmFlowDefinition; import org.jenkinsci.plugins.workflow.flow.FlowDefinition; @@ -61,7 +60,11 @@ class SCMBinder extends FlowDefinition { /** Kill switch for JENKINS-33273 in case of problems. */ - static /* not final */ boolean USE_HEAVYWEIGHT_CHECKOUT = Boolean.getBoolean(SCMBinder.class.getName() + ".USE_HEAVYWEIGHT_CHECKOUT"); // TODO 2.4+ use SystemProperties + static /* not final */ boolean USE_HEAVYWEIGHT_CHECKOUT = SystemProperties.getBoolean(SCMBinder.class.getName() + ".USE_HEAVYWEIGHT_CHECKOUT"); + + /** Kill switch for making this as strict as {@link ReadTrustedStep} about untrusted modifications. */ + static /* not final */ boolean IGNORE_UNTRUSTED_EDITS = SystemProperties.getBoolean(SCMBinder.class.getName() + ".IGNORE_UNTRUSTED_EDITS"); + private String scriptPath = WorkflowBranchProjectFactory.SCRIPT; public Object readResolve() { @@ -100,7 +103,7 @@ public SCMBinder(String scriptPath) { SCM scm; if (tip != null) { build.addAction(new SCMRevisionAction(scmSource, tip)); - SCMRevision rev = scmSource.getTrustedRevision(tip, listener); + SCMRevision rev = scmSource.getTrustedRevisionForBuild(tip, listener, build); try (SCMFileSystem fs = USE_HEAVYWEIGHT_CHECKOUT ? null : SCMFileSystem.of(scmSource, head, rev)) { if (fs != null) { // JENKINS-33273 String script = null; @@ -111,10 +114,10 @@ public SCMBinder(String scriptPath) { listener.error("Could not do lightweight checkout, falling back to heavyweight").println(Functions.printThrowable(x).trim()); } if (script != null) { - if (!rev.equals(tip)) { - // Print a warning in builds where an untrusted contributor has tried to edit Jenkinsfile. - // If we fail to check this (e.g., due to heavyweight checkout), a warning will still be printed to the log - // by the SCM, but that is less apparent. + if (!IGNORE_UNTRUSTED_EDITS && !rev.equals(tip)) { + // Make a best effort to abort builds where an untrusted contributor has tried to edit Jenkinsfile. + // If we fail to check this (e.g., due to heavyweight checkout), a warning will be printed to the log + // and the build will continue with the trusted variant, which is safe but confusing. SCMFileSystem tipFS = SCMFileSystem.of(scmSource, head, tip); if (tipFS != null) { String tipScript = null; @@ -124,9 +127,8 @@ public SCMBinder(String scriptPath) { listener.error("Could not compare lightweight checkout of trusted revision").println(Functions.printThrowable(x).trim()); } if (tipScript != null && !script.equals(tipScript)) { - listener.annotate(new WarningNote()); - listener.getLogger().println(Messages.ReadTrustedStep__has_been_modified_in_an_untrusted_revis(scriptPath)); - // TODO JENKINS-45970 consider aborting instead, at least optionally + build.setResult(Result.NOT_BUILT); + throw new AbortException(Messages.ReadTrustedStep__has_been_modified_in_an_untrusted_revis(scriptPath)); } } } @@ -165,22 +167,4 @@ public SCMBinder(String scriptPath) { } - // TODO seems there is no general-purpose ConsoleNote which simply wraps markup in specified HTML - @SuppressWarnings("rawtypes") - public static class WarningNote extends ConsoleNote { - - @Override public ConsoleAnnotator annotate(Object context, MarkupText text, int charPos) { - text.addMarkup(0, text.length(), "", ""); - return null; - } - - @Extension public static final class DescriptorImpl extends ConsoleAnnotationDescriptor { - @NonNull - @Override public String getDisplayName() { - return "Multibranch warnings"; - } - } - - } - } diff --git a/src/test/java/org/jenkinsci/plugins/workflow/multibranch/SCMBinderTest.java b/src/test/java/org/jenkinsci/plugins/workflow/multibranch/SCMBinderTest.java index 49a03e65..1311393a 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/multibranch/SCMBinderTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/multibranch/SCMBinderTest.java @@ -27,12 +27,15 @@ import com.cloudbees.hudson.plugins.folder.computed.DefaultOrphanedItemStrategy; import edu.umd.cs.findbugs.annotations.NonNull; import hudson.Util; +import hudson.model.Cause; +import hudson.model.CauseAction; import hudson.model.Item; import hudson.model.Result; import hudson.model.TaskListener; import hudson.model.User; import hudson.plugins.git.util.BuildData; import hudson.scm.ChangeLogSet; +import hudson.security.ACL; import java.io.File; import java.io.IOException; import java.util.HashSet; @@ -55,17 +58,20 @@ import jenkins.scm.impl.subversion.SubversionSampleRepoRule; import org.acegisecurity.Authentication; +import static org.awaitility.Awaitility.await; import org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval; import org.jenkinsci.plugins.workflow.job.WorkflowJob; import org.jenkinsci.plugins.workflow.job.WorkflowRun; import org.jenkinsci.plugins.workflow.test.steps.SemaphoreStep; import org.junit.Test; import static org.junit.Assert.*; +import org.junit.Before; import org.junit.ClassRule; import org.junit.Rule; import org.jvnet.hudson.test.BuildWatcher; import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.MockAuthorizationStrategy; public class SCMBinderTest { @@ -74,6 +80,10 @@ public class SCMBinderTest { @Rule public GitSampleRepoRule sampleGitRepo = new GitSampleRepoRule(); @Rule public SubversionSampleRepoRule sampleSvnRepo = new SubversionSampleRepoRule(); + @Before public void runImmediately() throws Exception { + r.jenkins.setQuietPeriod(0); + } + @Test public void exactRevisionGit() throws Exception { sampleGitRepo.init(); ScriptApproval sa = ScriptApproval.get(); @@ -182,9 +192,7 @@ public static void assertRevisionAction(WorkflowRun build) { mp.getSourcesList().add(new BranchSource(new GitSCMSource(null, sampleGitRepo.toString(), "", "*", "", false))); WorkflowJob p = WorkflowMultiBranchProjectTest.scheduleAndFindBranchProject(mp, "master"); assertEquals(1, mp.getItems().size()); - r.waitUntilNoActivity(); - WorkflowRun b1 = p.getLastBuild(); - assertEquals(1, b1.getNumber()); + WorkflowRun b1 = await().until(p::getLastCompletedBuild, lb -> lb != null && lb.getNumber() == 1); sampleGitRepo.git("rm", "Jenkinsfile"); sampleGitRepo.git("commit", "--all", "--message=remove"); WorkflowRun b2 = r.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0).get()); @@ -209,9 +217,7 @@ public static void assertRevisionAction(WorkflowRun build) { mp.setOrphanedItemStrategy(new DefaultOrphanedItemStrategy(false, "", "")); WorkflowJob p = WorkflowMultiBranchProjectTest.scheduleAndFindBranchProject(mp, "feature"); assertEquals(2, mp.getItems().size()); - r.waitUntilNoActivity(); - WorkflowRun b1 = p.getLastBuild(); - assertEquals(1, b1.getNumber()); + WorkflowRun b1 = await().until(p::getLastCompletedBuild, lb -> lb != null && lb.getNumber() == 1); Authentication auth = User.getById("dev", true).impersonate(); assertFalse(p.getACL().hasPermission(auth, Item.DELETE)); assertTrue(p.isBuildable()); @@ -235,37 +241,76 @@ public static void assertRevisionAction(WorkflowRun build) { assertEquals(1, mp.getItems().size()); } + @Issue("JENKINS-46795") @Test public void untrustedRevisions() throws Exception { + r.jenkins.setSecurityRealm(r.createDummySecurityRealm()); + r.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy(). + grant(Jenkins.READ, Item.READ).everywhere().toAuthenticated(). + grant(Item.BUILD).everywhere().to("alice"). + grant(Item.BUILD, Item.CONFIGURE).everywhere().to("bob")); sampleGitRepo.init(); - sampleGitRepo.write("Jenkinsfile", "node {checkout scm; echo readFile('file')}"); + String masterJenkinsfile = "node {checkout scm; echo readFile('file')}"; + sampleGitRepo.write("Jenkinsfile", masterJenkinsfile); sampleGitRepo.write("file", "initial content"); sampleGitRepo.git("add", "Jenkinsfile"); sampleGitRepo.git("commit", "--all", "--message=flow"); WorkflowMultiBranchProject mp = r.jenkins.createProject(WorkflowMultiBranchProject.class, "p"); mp.getSourcesList().add(new BranchSource(new WarySource(null, sampleGitRepo.toString(), "", "*", "", false))); WorkflowJob p = WorkflowMultiBranchProjectTest.scheduleAndFindBranchProject(mp, "master"); - r.waitUntilNoActivity(); - WorkflowRun b = p.getLastBuild(); - assertNotNull(b); - assertEquals(1, b.getNumber()); + WorkflowRun b = await().until(p::getLastCompletedBuild, lb -> lb != null && lb.getNumber() == 1); assertRevisionAction(b); r.assertBuildStatusSuccess(b); r.assertLogContains("initial content", b); String branch = "some-other-branch-from-Norway"; sampleGitRepo.git("checkout", "-b", branch); sampleGitRepo.write("Jenkinsfile", "error 'ALL YOUR BUILD STEPS ARE BELONG TO US'"); - sampleGitRepo.write("file", "subsequent content"); sampleGitRepo.git("commit", "--all", "--message=big evil laugh"); p = WorkflowMultiBranchProjectTest.scheduleAndFindBranchProject(mp, branch); - r.waitUntilNoActivity(); - b = p.getLastBuild(); - assertNotNull(b); - assertEquals(1, b.getNumber()); + b = await().until(p::getLastCompletedBuild, lb -> lb != null && lb.getNumber() == 1); assertRevisionAction(b); - r.assertBuildStatusSuccess(b); + r.assertBuildStatus(Result.NOT_BUILT, b); r.assertLogContains(Messages.ReadTrustedStep__has_been_modified_in_an_untrusted_revis("Jenkinsfile"), b); + r.assertLogContains("not trusting", b); + SCMBinder.IGNORE_UNTRUSTED_EDITS = true; + try { + sampleGitRepo.write("file", "subsequent content"); + sampleGitRepo.git("commit", "--all", "--message=edits"); + p = WorkflowMultiBranchProjectTest.scheduleAndFindBranchProject(mp, branch); + b = await().until(p::getLastCompletedBuild, lb -> lb != null && lb.getNumber() == 2); + r.assertLogContains("subsequent content", b); + r.assertLogContains("not trusting", b); + } finally { + SCMBinder.IGNORE_UNTRUSTED_EDITS = false; + } + sampleGitRepo.write("Jenkinsfile", masterJenkinsfile); + sampleGitRepo.git("commit", "--all", "--message=meekly submitting"); + p = WorkflowMultiBranchProjectTest.scheduleAndFindBranchProject(mp, branch); + b = await().until(p::getLastCompletedBuild, lb -> lb != null && lb.getNumber() == 3); r.assertLogContains("subsequent content", b); r.assertLogContains("not trusting", b); + sampleGitRepo.write("Jenkinsfile", "node {checkout scm; echo readTrusted('file').toUpperCase()}"); + sampleGitRepo.git("commit", "--all", "--message=changes to be approved"); + p = WorkflowMultiBranchProjectTest.scheduleAndFindBranchProject(mp, branch); + b = await().until(p::getLastCompletedBuild, lb -> lb != null && lb.getNumber() == 4); + r.assertBuildStatus(Result.NOT_BUILT, b); + r.assertLogContains(Messages.ReadTrustedStep__has_been_modified_in_an_untrusted_revis("Jenkinsfile"), b); + r.assertLogContains("not trusting", b); + try (var ctx = ACL.as(User.getById("alice", true))) { + b = p.scheduleBuild2(0, new CauseAction(new Cause.UserIdCause())).get(); + } + assertEquals(5, b.getNumber()); + r.assertBuildStatus(Result.NOT_BUILT, b); + r.assertLogContains(Messages.ReadTrustedStep__has_been_modified_in_an_untrusted_revis("Jenkinsfile"), b); + r.assertLogContains("Not trusting build since user ‘alice’ lacks Run/Replay permission", b); + r.assertLogContains("not trusting", b); + try (var ctx = ACL.as(User.getById("bob", true))) { + b = p.scheduleBuild2(0, new CauseAction(new Cause.UserIdCause())).get(); + } + assertEquals(6, b.getNumber()); + r.assertBuildStatusSuccess(b); + r.assertLogContains("SUBSEQUENT CONTENT", b); + r.assertLogContains("Trusting build since it was started by user ‘bob’", b); + r.assertLogNotContains("not trusting", b); } public static class WarySource extends GitSCMSource { public WarySource(String id, String remote, String credentialsId, String includes, String excludes, boolean ignoreOnPushNotifications) {