From ca27ccbb3382b2d29ed1d05dd0957e3dba809e95 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 7 Mar 2016 14:58:10 -0500 Subject: [PATCH 1/9] Initial sketch of getTrustedRevision implementation. --- pom.xml | 2 +- .../github_branch_source/GitHubSCMSource.java | 38 ++++++++++++++++++- .../PullRequestSCMHead.java | 7 +++- 3 files changed, 44 insertions(+), 3 deletions(-) diff --git a/pom.xml b/pom.xml index 7e53dd472..567f4787b 100644 --- a/pom.xml +++ b/pom.xml @@ -33,7 +33,7 @@ org.jenkins-ci.plugins scm-api - 1.0 + 1.1-SNAPSHOT org.jenkins-ci.plugins diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java index 8f8e1be87..bcaaeefc3 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java @@ -277,7 +277,7 @@ private void doRetrieve(SCMHeadObserver observer, TaskListener listener, GHRepos int pullrequests = 0; for (GHPullRequest ghPullRequest : repo.getPullRequests(GHIssueState.OPEN)) { int number = ghPullRequest.getNumber(); - SCMHead head = new PullRequestSCMHead(number); + SCMHead head = new PullRequestSCMHead(number, null); final String branchName = head.getName(); listener.getLogger().format("%n Checking pull request %s%n", HyperlinkNote.encodeTo(ghPullRequest.getHtmlUrl().toString(), "#" + branchName)); // FYI https://developer.github.com/v3/pulls/#response-1 @@ -299,6 +299,10 @@ private void doRetrieve(SCMHeadObserver observer, TaskListener listener, GHRepos continue; } } + SCMRevision trustedBase = trustedReplacement(repo, ghPullRequest); + if (trustedBase != null) { + head = new PullRequestSCMHead(number, trustedBase); + } SCMRevision hash = new SCMRevisionImpl(head, ghPullRequest.getHead().getSha()); observer.observe(head, hash); if (!observer.isObserving()) { @@ -382,6 +386,38 @@ protected SCMRevision doRetrieve(SCMHead head, TaskListener listener, GHReposito return new SCMRevisionImpl(head, ref.getObject().getSha()); } + @Override + public SCMRevision getTrustedRevision(SCMRevision revision, TaskListener listener) throws IOException, InterruptedException { + if (revision.getHead() instanceof PullRequestSCMHead) { + SCMRevision replacement = ((PullRequestSCMHead) revision.getHead()).trustedBase; + if (replacement != null) { + listener.getLogger().println("Loading trusted files from target branch at " + ((SCMRevisionImpl) replacement).getHash() + " rather than " + ((SCMRevisionImpl) revision).getHash()); + return replacement; + } + } + return revision; + } + + /** + * Evaluates whether this pull request is coming from a trusted source. + * A PR filed… + * + * @param ghPullRequest a PR + * @return the base revision, for an untrusted PR; null for a trusted PR + * @see PR metadata + * @see organization membership + * @see base revision oddity + */ + private @CheckForNull SCMRevision trustedReplacement(@Nonnull GHRepository repo, @Nonnull GHPullRequest ghPullRequest) { + //ghPullRequest.getBase().getSha(); + return null; // TODO + } + @Extension public static class DescriptorImpl extends SCMSourceDescriptor { private static final Logger LOGGER = Logger.getLogger(DescriptorImpl.class.getName()); diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMHead.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMHead.java index 2f7fdf99c..5f6a51b6c 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMHead.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMHead.java @@ -24,7 +24,9 @@ package org.jenkinsci.plugins.github_branch_source; +import javax.annotation.CheckForNull; import jenkins.scm.api.SCMHead; +import jenkins.scm.api.SCMRevision; /** * Head corresponding to a pull request. @@ -36,8 +38,11 @@ public final class PullRequestSCMHead extends SCMHead { private static final long serialVersionUID = 1; - public PullRequestSCMHead(int number) { + public final @CheckForNull SCMRevision trustedBase; + + public PullRequestSCMHead(int number, @CheckForNull SCMRevision trustedBase) { super(PR_BRANCH_PREFIX + number); + this.trustedBase = trustedBase; } public int getNumber() { From 8784214de11556ca02b37d51dd13a3face1b5860 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 7 Mar 2016 16:28:52 -0500 Subject: [PATCH 2/9] Implementing getTrustedRevision to check collaborator API. --- pom.xml | 2 +- .../github_branch_source/GitHubSCMSource.java | 33 ++++++++++--------- .../PullRequestSCMHead.java | 4 +-- 3 files changed, 21 insertions(+), 18 deletions(-) diff --git a/pom.xml b/pom.xml index 567f4787b..77d60031c 100644 --- a/pom.xml +++ b/pom.xml @@ -50,7 +50,7 @@ org.jenkins-ci.plugins github-api - 1.71 + 1.72 org.jenkins-ci.plugins diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java index bcaaeefc3..148a3e21e 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java @@ -299,7 +299,7 @@ private void doRetrieve(SCMHeadObserver observer, TaskListener listener, GHRepos continue; } } - SCMRevision trustedBase = trustedReplacement(repo, ghPullRequest); + String trustedBase = trustedReplacement(repo, ghPullRequest); if (trustedBase != null) { head = new PullRequestSCMHead(number, trustedBase); } @@ -389,10 +389,11 @@ protected SCMRevision doRetrieve(SCMHead head, TaskListener listener, GHReposito @Override public SCMRevision getTrustedRevision(SCMRevision revision, TaskListener listener) throws IOException, InterruptedException { if (revision.getHead() instanceof PullRequestSCMHead) { - SCMRevision replacement = ((PullRequestSCMHead) revision.getHead()).trustedBase; + PullRequestSCMHead head = (PullRequestSCMHead) revision.getHead(); + String replacement = head.trustedBase; if (replacement != null) { - listener.getLogger().println("Loading trusted files from target branch at " + ((SCMRevisionImpl) replacement).getHash() + " rather than " + ((SCMRevisionImpl) revision).getHash()); - return replacement; + listener.getLogger().println("Loading trusted files from target branch at " + replacement + " rather than " + ((SCMRevisionImpl) revision).getHash()); + return new SCMRevisionImpl(head, replacement); } } return revision; @@ -400,22 +401,24 @@ public SCMRevision getTrustedRevision(SCMRevision revision, TaskListener listene /** * Evaluates whether this pull request is coming from a trusted source. - * A PR filed… - *
    - *
  • …from the origin repository would be trusted, though we are skipping those anyway. - *
  • …against a repository owned by a user account is untrusted. - *
  • …against a repository in an organization by a user with a membership in that organization is trusted. - *
  • …against an organization repository by an outside user is untrusted. - *
+ * Quickest is to check whether the author of the PR + * is a collaborator of the repository. + * By checking all collaborators + * it is possible to further ascertain if they are in a team which was specifically granted push permission, + * but this is potentially expensive as there might be multiple pages of collaborators to retrieve. + * TODO since the GitHub API wrapper currently supports neither, we list all collaborator names and check for membership, + * paying the performance penalty without the benefit of the accuracy. * @param ghPullRequest a PR * @return the base revision, for an untrusted PR; null for a trusted PR * @see PR metadata - * @see organization membership * @see base revision oddity */ - private @CheckForNull SCMRevision trustedReplacement(@Nonnull GHRepository repo, @Nonnull GHPullRequest ghPullRequest) { - //ghPullRequest.getBase().getSha(); - return null; // TODO + private @CheckForNull String trustedReplacement(@Nonnull GHRepository repo, @Nonnull GHPullRequest ghPullRequest) throws IOException { + if (repo.getCollaboratorNames().contains(ghPullRequest.getUser().getLogin())) { + return null; + } else { + return ghPullRequest.getBase().getSha(); + } } @Extension public static class DescriptorImpl extends SCMSourceDescriptor { diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMHead.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMHead.java index 5f6a51b6c..f1559c1b9 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMHead.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMHead.java @@ -38,9 +38,9 @@ public final class PullRequestSCMHead extends SCMHead { private static final long serialVersionUID = 1; - public final @CheckForNull SCMRevision trustedBase; + public final @CheckForNull String trustedBase; - public PullRequestSCMHead(int number, @CheckForNull SCMRevision trustedBase) { + public PullRequestSCMHead(int number, @CheckForNull String trustedBase) { super(PR_BRANCH_PREFIX + number); this.trustedBase = trustedBase; } From 3e23edf68e0c87b857c4923dabe998167cc26150 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 7 Mar 2016 17:08:52 -0500 Subject: [PATCH 3/9] =?UTF-8?q?[FIXED=20JENKINS-33256]=20Re=C3=ABnabling?= =?UTF-8?q?=20PRs=20on=20public=20repos.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../plugins/github_branch_source/GitHubSCMSource.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java index 148a3e21e..267c55d4b 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java @@ -272,7 +272,6 @@ private void doRetrieve(SCMHeadObserver observer, TaskListener listener, GHRepos } listener.getLogger().format("%n %d branches were processed%n", branches); - if (repo.isPrivate()) { listener.getLogger().format("%n Getting remote pull requests...%n"); int pullrequests = 0; for (GHPullRequest ghPullRequest : repo.getPullRequests(GHIssueState.OPEN)) { @@ -311,9 +310,6 @@ private void doRetrieve(SCMHeadObserver observer, TaskListener listener, GHRepos pullrequests++; } listener.getLogger().format("%n %d pull requests were processed%n", pullrequests); - } else { - listener.getLogger().format("%n Skipping pull requests for public repositories%n"); - } } From fc6a51d2d0b4cb07e3d0699ae937cfd399daac76 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 7 Mar 2016 18:48:21 -0500 Subject: [PATCH 4/9] A bit more logging. --- .../jenkinsci/plugins/github_branch_source/GitHubSCMSource.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java index 267c55d4b..4159c74da 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java @@ -300,6 +300,7 @@ private void doRetrieve(SCMHeadObserver observer, TaskListener listener, GHRepos } String trustedBase = trustedReplacement(repo, ghPullRequest); if (trustedBase != null) { + listener.getLogger().format(" (not from a trusted source)%n"); head = new PullRequestSCMHead(number, trustedBase); } SCMRevision hash = new SCMRevisionImpl(head, ghPullRequest.getHead().getSha()); From c8c1befbe333e063ca660b0af2c0ee1ff30e790e Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 7 Mar 2016 18:59:23 -0500 Subject: [PATCH 5/9] Test dep on workflow-multibranch for convenience. --- pom.xml | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/pom.xml b/pom.xml index 77d60031c..4fe575a02 100644 --- a/pom.xml +++ b/pom.xml @@ -21,6 +21,7 @@ 1.609.3 + 1.15-SNAPSHOT @@ -62,6 +63,19 @@ github 1.14.0 + + + org.jenkins-ci.plugins.workflow + workflow-aggregator + ${workflow.version} + test + + + org.jenkins-ci.plugins.workflow + workflow-multibranch + ${workflow.version} + test + From 3bacc2fab389ca943ce75dac9ec1812647d0f1d0 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 7 Mar 2016 19:17:15 -0500 Subject: [PATCH 6/9] Actually better to keep the baseHash in the SCMRevision, not the SCMHead. --- .../github_branch_source/GitHubSCMSource.java | 25 ++++++----- .../PullRequestSCMHead.java | 7 +--- .../UntrustedPullRequestSCMRevision.java | 42 +++++++++++++++++++ 3 files changed, 58 insertions(+), 16 deletions(-) create mode 100644 src/main/java/org/jenkinsci/plugins/github_branch_source/UntrustedPullRequestSCMRevision.java diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java index 4159c74da..c4a55b426 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java @@ -276,7 +276,7 @@ private void doRetrieve(SCMHeadObserver observer, TaskListener listener, GHRepos int pullrequests = 0; for (GHPullRequest ghPullRequest : repo.getPullRequests(GHIssueState.OPEN)) { int number = ghPullRequest.getNumber(); - SCMHead head = new PullRequestSCMHead(number, null); + SCMHead head = new PullRequestSCMHead(number); final String branchName = head.getName(); listener.getLogger().format("%n Checking pull request %s%n", HyperlinkNote.encodeTo(ghPullRequest.getHtmlUrl().toString(), "#" + branchName)); // FYI https://developer.github.com/v3/pulls/#response-1 @@ -299,11 +299,13 @@ private void doRetrieve(SCMHeadObserver observer, TaskListener listener, GHRepos } } String trustedBase = trustedReplacement(repo, ghPullRequest); - if (trustedBase != null) { + SCMRevision hash; + if (trustedBase == null) { + hash = new SCMRevisionImpl(head, ghPullRequest.getHead().getSha()); + } else { listener.getLogger().format(" (not from a trusted source)%n"); - head = new PullRequestSCMHead(number, trustedBase); + hash = new UntrustedPullRequestSCMRevision(head, ghPullRequest.getHead().getSha(), trustedBase); } - SCMRevision hash = new SCMRevisionImpl(head, ghPullRequest.getHead().getSha()); observer.observe(head, hash); if (!observer.isObserving()) { return; @@ -377,6 +379,11 @@ protected SCMRevision doRetrieve(SCMHead head, TaskListener listener, GHReposito if (head instanceof PullRequestSCMHead) { int number = ((PullRequestSCMHead) head).getNumber(); ref = repo.getRef("pull/" + number + "/merge"); + // TODO if we already had the GHPullRequest.user.login in a field in the PullRequestSCMHead, we could pass that directly and save an API call + String trustedBase = trustedReplacement(repo, repo.getPullRequest(number)); + if (trustedBase != null) { + return new UntrustedPullRequestSCMRevision(head, ref.getObject().getSha(), trustedBase); + } } else { ref = repo.getRef("heads/" + head.getName()); } @@ -385,13 +392,11 @@ protected SCMRevision doRetrieve(SCMHead head, TaskListener listener, GHReposito @Override public SCMRevision getTrustedRevision(SCMRevision revision, TaskListener listener) throws IOException, InterruptedException { - if (revision.getHead() instanceof PullRequestSCMHead) { + if (revision instanceof UntrustedPullRequestSCMRevision) { PullRequestSCMHead head = (PullRequestSCMHead) revision.getHead(); - String replacement = head.trustedBase; - if (replacement != null) { - listener.getLogger().println("Loading trusted files from target branch at " + replacement + " rather than " + ((SCMRevisionImpl) revision).getHash()); - return new SCMRevisionImpl(head, replacement); - } + UntrustedPullRequestSCMRevision rev = (UntrustedPullRequestSCMRevision) revision; + listener.getLogger().println("Loading trusted files from target branch at " + rev.baseHash + " rather than " + rev.getHash()); + return new SCMRevisionImpl(head, rev.baseHash); } return revision; } diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMHead.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMHead.java index f1559c1b9..2f7fdf99c 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMHead.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMHead.java @@ -24,9 +24,7 @@ package org.jenkinsci.plugins.github_branch_source; -import javax.annotation.CheckForNull; import jenkins.scm.api.SCMHead; -import jenkins.scm.api.SCMRevision; /** * Head corresponding to a pull request. @@ -38,11 +36,8 @@ public final class PullRequestSCMHead extends SCMHead { private static final long serialVersionUID = 1; - public final @CheckForNull String trustedBase; - - public PullRequestSCMHead(int number, @CheckForNull String trustedBase) { + public PullRequestSCMHead(int number) { super(PR_BRANCH_PREFIX + number); - this.trustedBase = trustedBase; } public int getNumber() { diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/UntrustedPullRequestSCMRevision.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/UntrustedPullRequestSCMRevision.java new file mode 100644 index 000000000..802d96657 --- /dev/null +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/UntrustedPullRequestSCMRevision.java @@ -0,0 +1,42 @@ +/* + * The MIT License + * + * Copyright 2016 CloudBees, Inc. + * + * 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 org.jenkinsci.plugins.github_branch_source; + +import jenkins.plugins.git.AbstractGitSCMSource; +import jenkins.scm.api.SCMHead; + +/** + * Revision of a pull request which should load sensitive files from the base branch. + */ +class UntrustedPullRequestSCMRevision extends AbstractGitSCMSource.SCMRevisionImpl { + + final String baseHash; + + UntrustedPullRequestSCMRevision(SCMHead head, String hash, String baseHash) { + super(head, hash); + this.baseHash = baseHash; + } + +} From 4d9f076a91204078ab64f7f904e7c82ae594b89d Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 7 Mar 2016 19:33:38 -0500 Subject: [PATCH 7/9] FindBugs --- .../UntrustedPullRequestSCMRevision.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/UntrustedPullRequestSCMRevision.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/UntrustedPullRequestSCMRevision.java index 802d96657..ebb4d821c 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/UntrustedPullRequestSCMRevision.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/UntrustedPullRequestSCMRevision.java @@ -39,4 +39,14 @@ class UntrustedPullRequestSCMRevision extends AbstractGitSCMSource.SCMRevisionIm this.baseHash = baseHash; } + @Override + public boolean equals(Object o) { + return super.equals(o) && baseHash.equals(((UntrustedPullRequestSCMRevision) o).baseHash); + } + + @Override + public int hashCode() { + return super.hashCode(); // good enough + } + } From b039721736e38734cb7733412a5a95601a8c9213 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 11 Mar 2016 16:12:30 -0500 Subject: [PATCH 8/9] Pipeline 1.15 released. --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index cb4d6ac73..6e85a496b 100644 --- a/pom.xml +++ b/pom.xml @@ -21,7 +21,7 @@ 1.609.3 - 1.15-SNAPSHOT + 1.15 From 6fa55c17d4c9c4d74da9758ba3b1f7aecf4f6868 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 11 Mar 2016 16:22:44 -0500 Subject: [PATCH 9/9] Inaccurate comment. --- .../jenkinsci/plugins/github_branch_source/GitHubSCMSource.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java index 6315f6aa7..e81dc2561 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java @@ -378,7 +378,7 @@ protected SCMRevision doRetrieve(SCMHead head, TaskListener listener, GHReposito if (head instanceof PullRequestSCMHead) { int number = ((PullRequestSCMHead) head).getNumber(); ref = repo.getRef("pull/" + number + "/merge"); - // TODO if we already had the GHPullRequest.user.login in a field in the PullRequestSCMHead, we could pass that directly and save an API call + // getPullRequests makes an extra API call, but we need its current .base.sha String trustedBase = trustedReplacement(repo, repo.getPullRequest(number)); if (trustedBase != null) { return new UntrustedPullRequestSCMRevision(head, ref.getObject().getSha(), trustedBase);