From 01f1b1ac227f18396981c15d60f3ceae24944f47 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 5 Jun 2020 23:32:42 +0200 Subject: [PATCH 1/5] fix(dev-infra): incorrect token sanitization when no token is specified We recently moved over the git client from the merge script to the common dev-infra utils. This made specifying a token optional, but it looks like the logic for sanitizing messages doesn't account for that, and we currently add `` between every message character. e.g. ``` Executing: git git status ``` --- dev-infra/utils/git.ts | 23 +++++++++++++++++++---- dev-infra/utils/shelljs.ts | 9 ++++++--- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/dev-infra/utils/git.ts b/dev-infra/utils/git.ts index 1330ac158cb75..a89f06811c076 100644 --- a/dev-infra/utils/git.ts +++ b/dev-infra/utils/git.ts @@ -61,11 +61,21 @@ export class GitClient { private _projectRoot = getRepoBaseDir(); /** The OAuth scopes available for the provided Github token. */ private _oauthScopes: Promise|null = null; - /** Regular expression that matches the provided Github token. */ - private _tokenRegex = new RegExp(this._githubToken, 'g'); + /** + * Regular expression that matches the provided Github token. Used for + * sanitizing the token from Git child process output. + */ + private _githubTokenRegex: RegExp|null = null; constructor( - private _githubToken = '', private _config: Pick = getConfig()) { + private _githubToken?: string, private _config: Pick = getConfig()) { + // If a token has been specified, pass it to the Octokit API and also create + // a regular expression that can be used for sanitizing Git command output + // so that it does not print the token accidentally. + if (this._githubToken != null) { + this._githubTokenRegex = new RegExp(this._githubToken, 'g') + } + this.api = new Octokit({auth: _githubToken}); this.api.hook.error('request', error => { // Wrap API errors in a known error class. This allows us to @@ -137,7 +147,12 @@ export class GitClient { /** Sanitizes a given message by omitting the provided Github token if present. */ omitGithubTokenFromMessage(value: string): string { - return value.replace(this._tokenRegex, ''); + // If no token has been defined (i.e. no token regex), we just return the + // value as is. There is no secret value that needs to be omitted. + if (this._githubTokenRegex === null) { + return value; + } + return value.replace(this._githubTokenRegex, ''); } /** diff --git a/dev-infra/utils/shelljs.ts b/dev-infra/utils/shelljs.ts index 831dea929135a..0040b4135bce5 100644 --- a/dev-infra/utils/shelljs.ts +++ b/dev-infra/utils/shelljs.ts @@ -8,7 +8,10 @@ import {exec as _exec, ExecOptions, ShellString} from 'shelljs'; -/* Run an exec command as silent. */ -export function exec(cmd: string, opts?: ExecOptions&{async?: false}): ShellString { - return _exec(cmd, {silent: true, ...opts}); +/** + * Runs an given command as child process. By default, child process + * output will not be printed. + */ +export function exec(cmd: string, opts?: Omit): ShellString { + return _exec(cmd, {silent: true, ...opts, async: false}); } From e8e6f2864fa8d596e65883913872c20408974a9b Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 5 Jun 2020 23:41:44 +0200 Subject: [PATCH 2/5] fix(dev-infra): rebase pr script not working The dev-infra rebase PR script currently does not work due to the following issues: 1. The push refspec is incorrect. It refers to the `base` of the PR, and not to the `head` of the PR. 2. The push `--force-with-lease` option does not work in a detached head as no remote-tracking branch is set up. --- dev-infra/pr/rebase/index.ts | 38 +++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/dev-infra/pr/rebase/index.ts b/dev-infra/pr/rebase/index.ts index 7524b9a743626..0b9fa9fe77d33 100644 --- a/dev-infra/pr/rebase/index.ts +++ b/dev-infra/pr/rebase/index.ts @@ -13,13 +13,13 @@ import {getConfig, NgDevConfig} from '../../utils/config'; import {error, info, promptConfirm} from '../../utils/console'; import {GitClient} from '../../utils/git'; import {getPr} from '../../utils/github'; -import {exec} from '../../utils/shelljs'; /* GraphQL schema for the response body for each pending PR. */ const PR_SCHEMA = { state: graphQLTypes.string, maintainerCanModify: graphQLTypes.boolean, viewerDidAuthor: graphQLTypes.boolean, + headRefOid: graphQLTypes.string, headRef: { name: graphQLTypes.string, repository: { @@ -42,7 +42,7 @@ const PR_SCHEMA = { */ export async function rebasePr( prNumber: number, githubToken: string, config: Pick = getConfig()) { - const git = new GitClient(); + const git = new GitClient(githubToken); // TODO: Rely on a common assertNoLocalChanges function. if (git.hasLocalChanges()) { error('Cannot perform rebase of PR with local changes.'); @@ -57,11 +57,21 @@ export async function rebasePr( /* Get the PR information from Github. */ const pr = await getPr(PR_SCHEMA, prNumber, config.github); - const fullHeadRef = `${pr.headRef.repository.nameWithOwner}:${pr.headRef.name}`; - const fullBaseRef = `${pr.baseRef.repository.nameWithOwner}:${pr.baseRef.name}`; + const headRefName = pr.headRef.name; + const baseRefName = pr.baseRef.name; + const fullHeadRef = `${pr.headRef.repository.nameWithOwner}:${headRefName}`; + const fullBaseRef = `${pr.baseRef.repository.nameWithOwner}:${baseRefName}`; const headRefUrl = addAuthenticationToUrl(pr.headRef.repository.url, githubToken); const baseRefUrl = addAuthenticationToUrl(pr.baseRef.repository.url, githubToken); + // Note: Since we use a detached head for rebasing the PR and therefore do not have + // remote-tracking branches configured, we need to set our expected ref and SHA. This + // allows us to use `--force-with-lease` for the detached head while ensuring that we + // never accidentally override upstream changes that have been pushed in the meanwhile. + // See: + // https://git-scm.com/docs/git-push#Documentation/git-push.txt---force-with-leaseltrefnamegtltexpectgt + const forceWithLeaseFlag = `--force-with-lease=${headRefName}:${pr.headRefOid}`; + // If the PR does not allow maintainers to modify it, exit as the rebased PR cannot // be pushed up. if (!pr.maintainerCanModify && !pr.viewerDidAuthor) { @@ -74,20 +84,20 @@ export async function rebasePr( try { // Fetch the branch at the commit of the PR, and check it out in a detached state. info(`Checking out PR #${prNumber} from ${fullHeadRef}`); - exec(`git fetch ${headRefUrl} ${pr.headRef.name}`); - exec(`git checkout --detach FETCH_HEAD`); + git.run(['fetch', headRefUrl, headRefName]); + git.run(['checkout', '--detach', 'FETCH_HEAD']); // Fetch the PRs target branch and rebase onto it. info(`Fetching ${fullBaseRef} to rebase #${prNumber} on`); - exec(`git fetch ${baseRefUrl} ${pr.baseRef.name}`); + git.run(['fetch', baseRefUrl, baseRefName]); info(`Attempting to rebase PR #${prNumber} on ${fullBaseRef}`); - const rebaseResult = exec(`git rebase FETCH_HEAD`); + const rebaseResult = git.runGraceful(['rebase', 'FETCH_HEAD']); // If the rebase was clean, push the rebased PR up to the authors fork. - if (rebaseResult.code === 0) { + if (rebaseResult.status === 0) { info(`Rebase was able to complete automatically without conflicts`); info(`Pushing rebased PR #${prNumber} to ${fullHeadRef}`); - exec(`git push ${baseRefUrl} HEAD:${pr.baseRef.name} --force-with-lease`); + git.run(['push', headRefUrl, `HEAD:${headRefName}`, forceWithLeaseFlag]); info(`Rebased and updated PR #${prNumber}`); cleanUpGitState(); process.exit(0); @@ -107,7 +117,7 @@ export async function rebasePr( if (continueRebase) { info(`After manually completing rebase, run the following command to update PR #${prNumber}:`); - info(` $ git push ${pr.baseRef.repository.url} HEAD:${pr.baseRef.name} --force-with-lease`); + info(` $ git push ${pr.headRef.repository.url} HEAD:${headRefName} ${forceWithLeaseFlag}`); info(); info(`To abort the rebase and return to the state of the repository before this command`); info(`run the following command:`); @@ -123,11 +133,11 @@ export async function rebasePr( /** Reset git back to the original branch. */ function cleanUpGitState() { // Ensure that any outstanding rebases are aborted. - exec(`git rebase --abort`); + git.runGraceful(['rebase', '--abort'], {stdio: 'ignore'}); // Ensure that any changes in the current repo state are cleared. - exec(`git reset --hard`); + git.runGraceful(['reset', '--hard'], {stdio: 'ignore'}); // Checkout the original branch from before the run began. - exec(`git checkout ${originalBranch}`); + git.runGraceful(['checkout', originalBranch], {stdio: 'ignore'}); } } From 59458833f61ed4f115c63181138a1b06295244e6 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 5 Jun 2020 23:49:45 +0200 Subject: [PATCH 3/5] fix(dev-infra): local changes check not working Looks like we broke the `hasLocalChanges` check in the git client when we moved it over from the merge script. The problem is that we are using `git` in the first argument of `git.run`. That means that we under-the-hood run `git git <..>`. This commit fixes that, but also switches to a better variant for ensuring no local changes because it exits with non-zero when there are local changes. --- dev-infra/utils/git.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-infra/utils/git.ts b/dev-infra/utils/git.ts index a89f06811c076..56b8ff0647302 100644 --- a/dev-infra/utils/git.ts +++ b/dev-infra/utils/git.ts @@ -142,7 +142,7 @@ export class GitClient { /** Whether the repo has any local changes. */ hasLocalChanges(): boolean { - return !!this.runGraceful(['git', 'status', '--porcelain']).stdout.trim(); + return this.runGraceful(['diff-index', '--quiet', 'HEAD']).status !== 0; } /** Sanitizes a given message by omitting the provided Github token if present. */ From 14f32ef2b2d4d6295ef871d02e59e1bb48422840 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Mon, 8 Jun 2020 23:10:04 +0200 Subject: [PATCH 4/5] fixup! fix(dev-infra): incorrect token sanitization when no token is specified Address feedback --- dev-infra/utils/git.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/dev-infra/utils/git.ts b/dev-infra/utils/git.ts index 56b8ff0647302..4bb33c930320f 100644 --- a/dev-infra/utils/git.ts +++ b/dev-infra/utils/git.ts @@ -69,11 +69,11 @@ export class GitClient { constructor( private _githubToken?: string, private _config: Pick = getConfig()) { - // If a token has been specified, pass it to the Octokit API and also create - // a regular expression that can be used for sanitizing Git command output + // If a token has been specified (and is not empty), pass it to the Octokit API and + // also create a regular expression that can be used for sanitizing Git command output // so that it does not print the token accidentally. - if (this._githubToken != null) { - this._githubTokenRegex = new RegExp(this._githubToken, 'g') + if (_githubToken != null) { + this._githubTokenRegex = new RegExp(_githubToken, 'g') } this.api = new Octokit({auth: _githubToken}); From 975a7ced28ad722199f9b3024506cb7524473c46 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Tue, 9 Jun 2020 15:58:45 +0200 Subject: [PATCH 5/5] fixup! fix(dev-infra): incorrect token sanitization when no token is specified Fix lint --- dev-infra/utils/git.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-infra/utils/git.ts b/dev-infra/utils/git.ts index 4bb33c930320f..5ff7a17ab5291 100644 --- a/dev-infra/utils/git.ts +++ b/dev-infra/utils/git.ts @@ -73,7 +73,7 @@ export class GitClient { // also create a regular expression that can be used for sanitizing Git command output // so that it does not print the token accidentally. if (_githubToken != null) { - this._githubTokenRegex = new RegExp(_githubToken, 'g') + this._githubTokenRegex = new RegExp(_githubToken, 'g'); } this.api = new Octokit({auth: _githubToken});