From 119afad0cdf59bc6b92ce9c16d5914e81d361869 Mon Sep 17 00:00:00 2001 From: Orta Date: Tue, 24 Aug 2021 15:38:10 +0100 Subject: [PATCH 1/2] Allows closing PR/issues by an existence check --- README.md | 8 ++++++++ index.js | 35 +++++++++++++++++++++++++++-------- index.test.js | 16 +++++++++++++++- 3 files changed, 50 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index c798f81c..b028c891 100644 --- a/README.md +++ b/README.md @@ -59,6 +59,14 @@ Then you should be good to go. We force the use of [`pull_request_target`](https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/) as a workflow event to ensure that someone cannot change the CODEOWNER files at the same time as having that change be used to validate if they can merge. +### Issue / PR closing + +Merging a PR has strict security requirements, but closing a PR or Issue can have a weaker one. Anyone with a GitHub login in the CODEOWNER has the ability to close any PR / Issue via a comment/review which includes: + +``` +@github-actions close +``` + ### Extras You can use this label to set labels for specific sections of the codebase, by having square brackets to indicate labels to make: `[label]` diff --git a/index.js b/index.js index 2881452e..1bab6b13 100644 --- a/index.js +++ b/index.js @@ -7,7 +7,7 @@ const {readFileSync} = require("fs"); // Effectively the main function async function run() { - core.info("Running version 1.5.4") + core.info("Running version 1.6.0") // Tell folks they can merge if (context.eventName === "pull_request_target") { @@ -20,7 +20,7 @@ async function run() { if (bodyLower.includes("lgtm")) { new Actor().mergeIfHasAccess(); } else if (bodyLower.includes("@github-actions close")) { - new Actor().closeIfHasAccess(); + new Actor().closePROrIssueIfInCodeowners(); } else { console.log("Doing nothing because the body does not include a command") } @@ -125,6 +125,7 @@ class Actor { this.octokit = getOctokit(process.env.GITHUB_TOKEN) this.thisRepo = { owner: context.repo.owner, repo: context.repo.repo } this.issue = context.payload.issue || context.payload.pull_request + /** @type {string} - GitHub login */ this.sender = context.payload.sender.login } @@ -195,11 +196,11 @@ class Actor { } } - async closeIfHasAccess() { - const prInfo = await this.getTargetPRIfHasAccess() - if (!prInfo) { - return - } + async closePROrIssueIfInCodeowners() { + // Because closing a PR/issue does not mutate the repo, we can use a weaker + // authentication method: basically is the person in the codeowners? Then they can close + // an issue or PR. + if (!githubLoginIsInCodeowners(this.sender, this.cwd)) return const { octokit, thisRepo, issue, sender } = this; @@ -230,6 +231,22 @@ function getFilesNotOwnedByCodeOwner(owner, files, cwd) { return filesWhichArentOwned } + +/** + * This is a reasonable security measure for proving an account is specified in the codeowners + * but _SHOULD NOT_ be used for authentication for something which mutates the repo, + * + * @param {string} login + * @param {string} cwd + */ + function githubLoginIsInCodeowners(login, cwd) { + const codeowners = new Codeowners(cwd); + const contents = readFileSync(codeowners.codeownersFilePath, "utf8").toLowerCase() + + return contents.includes("@" + login.toLowerCase() + " ") || contents.includes("@" + login.toLowerCase() + "\n") +} + + /** * * @param {string[]} files @@ -301,9 +318,11 @@ async function createOrAddLabel(octokit, repoDeets, labelConfig) { }) } +// For tests module.exports = { getFilesNotOwnedByCodeOwner, - findCodeOwnersForChangedFiles + findCodeOwnersForChangedFiles, + githubLoginIsInCodeowners } // @ts-ignore diff --git a/index.test.js b/index.test.js index 82b391bc..a2fce138 100644 --- a/index.test.js +++ b/index.test.js @@ -1,4 +1,4 @@ -const { getFilesNotOwnedByCodeOwner, findCodeOwnersForChangedFiles } = require("."); +const { getFilesNotOwnedByCodeOwner, findCodeOwnersForChangedFiles, githubLoginIsInCodeowners } = require("."); test("determine who owns a set of files", () => { const noFiles = findCodeOwnersForChangedFiles(["root-codeowners/one.two.js"], "./test-code-owners-repo"); @@ -35,3 +35,17 @@ test("deciding if someone has access to merge", () => { expect(filesNotInCodeowners).toEqual(["random-path/file.ts"]); }); +describe(githubLoginIsInCodeowners, () => { + test("allows folks found in the codeowners", () => { + const ortaIn = githubLoginIsInCodeowners("orta", "."); + expect(ortaIn).toEqual(true); + }); + test("ignores case", () => { + const ortaIn = githubLoginIsInCodeowners("OrTa", "."); + expect(ortaIn).toEqual(true); + }); + test("denies other accounts", () => { + const ortaIn = githubLoginIsInCodeowners("dogman", "."); + expect(ortaIn).toEqual(false); + }); +}) From 7e54395832734f8f155a016acec817f28cdf1ab8 Mon Sep 17 00:00:00 2001 From: Orta Date: Tue, 24 Aug 2021 16:01:49 +0100 Subject: [PATCH 2/2] :Better tests --- index.test.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/index.test.js b/index.test.js index a2fce138..0dcc2b50 100644 --- a/index.test.js +++ b/index.test.js @@ -45,7 +45,11 @@ describe(githubLoginIsInCodeowners, () => { expect(ortaIn).toEqual(true); }); test("denies other accounts", () => { - const ortaIn = githubLoginIsInCodeowners("dogman", "."); - expect(ortaIn).toEqual(false); + const noDogMan = githubLoginIsInCodeowners("dogman", "."); + expect(noDogMan).toEqual(false); + }); + test("denies subsets of existing accounts", () => { + const noOrt = githubLoginIsInCodeowners("ort", "."); + expect(noOrt).toEqual(false); }); })