build: do not print github auth token if merge script fails (#19159)

Various improvements to the merge script:

1. No longer print Github auth token if a command fails.
2. No longer print Github auth token when verbose-printing executed git commands.
3. Better error if token does not have required permissions for merging
pull request (api-merge strategy)

Author: devversion

scripts/merge-script/cli.ts

@@ -12,8 +12,12 @@ import {isAbsolute, resolve} from 'path';
 
 import {Config, readAndValidateConfig} from './config';
 import {promptConfirm} from './console';
+import {GithubApiRequestError} from './git';
 import {MergeResult, MergeStatus, PullRequestMergeTask} from './index';
 
+/** URL to the Github page where personal access tokens can be generated. */
+const GITHUB_TOKEN_GENERATE_URL = `https://github.com/settings/tokens`;
+
 // Run the CLI.
 main();
 
@@ -30,14 +34,28 @@ async function main() {
   // Perform the merge. Force mode can be activated through a command line flag.
   // Alternatively, if the merge fails with non-fatal failures, the script
   // will prompt whether it should rerun in force mode.
-  const mergeResult = await api.merge(prNumber, force);
-
-  // Handle the result of the merge. If failures have been reported, exit
-  // the process with a non-zero exit code.
-  if (!await handleMergeResult(mergeResult, force)) {
+  if (!await performMerge(force)) {
     process.exit(1);
   }
 
+  /** Performs the merge and returns whether it was successful or not. */
+  async function performMerge(ignoreFatalErrors: boolean): Promise<boolean> {
+    try {
+      const result = await api.merge(prNumber, ignoreFatalErrors);
+      return await handleMergeResult(result, ignoreFatalErrors);
+    } catch (e) {
+      // Catch errors to the Github API for invalid requests. We want to
+      // exit the script with a better explanation of the error.
+      if (e instanceof GithubApiRequestError && e.status === 401) {
+        console.error(chalk.red('Github API request failed. ' + e.message));
+        console.error(chalk.yellow('Please ensure that your provided token is valid.'));
+        console.error(chalk.yellow(`You can generate a token here: ${GITHUB_TOKEN_GENERATE_URL}`));
+        process.exit(1);
+      }
+      throw e;
+    }
+  }
+
   /**
    * Prompts whether the specified pull request should be forcibly merged. If so, merges
    * the specified pull request forcibly (ignoring non-critical failures).
@@ -47,10 +65,7 @@ async function main() {
     if (await promptConfirm('Do you want to forcibly proceed with merging?')) {
       // Perform the merge in force mode. This means that non-fatal failures
       // are ignored and the merge continues.
-      const forceMergeResult = await api.merge(prNumber, true);
-      // Handle the merge result. Note that we disable the force merge prompt since
-      // a failed force merge will never succeed with a second force merge.
-      return await handleMergeResult(forceMergeResult, true);
+      return performMerge(true);
     }
     return false;
   }
@@ -130,6 +145,7 @@ function parseCommandLine():
     console.error(
         chalk.red('No Github token is set. Please set the `GITHUB_TOKEN` environment variable.'));
     console.error(chalk.red('Alternatively, pass the `--github-token` command line flag.'));
+    console.error(chalk.yellow(`You can generate a token here: ${GITHUB_TOKEN_GENERATE_URL}`));
     process.exit(1);
   }
 

scripts/merge-script/failures.ts

@@ -70,4 +70,9 @@ export class PullRequestFailure {
   static notFound() {
     return new this(`Pull request could not be found upstream.`);
   }
+
+  static insufficientPermissionsToMerge() {
+    return new this(`Insufficient Github API permissions to merge pull request. Please ` +
+        `ensure that your auth token has write access.`);
+  }
 }

scripts/merge-script/git.ts

@@ -10,10 +10,20 @@ import * as Octokit from '@octokit/rest';
 import {spawnSync, SpawnSyncOptions, SpawnSyncReturns} from 'child_process';
 import {Config} from './config';
 
+/** Error for failed Github API requests. */
+export class GithubApiRequestError extends Error {
+  constructor(public status: number, message: string) {
+    super(message);
+  }
+}
+
 /** Error for failed Git commands. */
 export class GitCommandError extends Error {
-  constructor(public commandArgs: string[]) {
-    super(`Command failed: git ${commandArgs.join(' ')}`);
+  constructor(client: GitClient, public args: string[]) {
+    // Errors are not guaranteed to be caught. To ensure that we don't
+    // accidentally leak the Github token that might be used in a command,
+    // we sanitize the command that will be part of the error message.
+    super(`Command failed: git ${client.omitGithubTokenFromMessage(args.join(' '))}`);
   }
 }
 
@@ -29,15 +39,23 @@ export class GitClient {
   /** Instance of the authenticated Github octokit API. */
   api: Octokit;
 
+  /** Regular expression that matches the provided Github token. */
+  private _tokenRegex = new RegExp(this._githubToken, 'g');
+
   constructor(private _githubToken: string, private _config: Config) {
     this.api = new Octokit({auth: _githubToken});
+    this.api.hook.error('request', error => {
+      // Wrap API errors in a known error class. This allows us to
+      // expect Github API errors better and in a non-ambiguous way.
+      throw new GithubApiRequestError(error.status, error.message);
+    });
   }
 
   /** Executes the given git command. Throws if the command fails. */
   run(args: string[], options?: SpawnSyncOptions): Omit<SpawnSyncReturns<string>, 'status'> {
     const result = this.runGraceful(args, options);
     if (result.status !== 0) {
-      throw new GitCommandError(args);
+      throw new GitCommandError(this, args);
     }
     // Omit `status` from the type so that it's obvious that the status is never
     // non-zero as explained in the method description.
@@ -46,21 +64,32 @@ export class GitClient {
 
   /**
    * Spawns a given Git command process. Does not throw if the command fails. Additionally,
-   * the "stderr" output is inherited and will be printed in case of errors. This makes it
-   * easier to debug failed commands.
+   * if there is any stderr output, the output will be printed. This makes it easier to
+   * debug failed commands.
    */
   runGraceful(args: string[], options: SpawnSyncOptions = {}): SpawnSyncReturns<string> {
-    // To improve the debugging experience in case something fails, we print
-    // all executed Git commands.
-    console.info('Executing: git', ...args);
-    return spawnSync('git', args, {
+    // To improve the debugging experience in case something fails, we print all executed
+    // Git commands. Note that we do not want to print the token if is contained in the
+    // command. It's common to share errors with others if the tool failed.
+    console.info('Executing: git', this.omitGithubTokenFromMessage(args.join(' ')));
+
+    const result = spawnSync('git', args, {
       cwd: this._config.projectRoot,
-      stdio: ['pipe', 'pipe', 'inherit'],
+      stdio: 'pipe',
       ...options,
       // Encoding is always `utf8` and not overridable. This ensures that this method
       // always returns `string` as output instead of buffers.
       encoding: 'utf8',
     });
+
+    if (result.stderr !== null) {
+      // Git sometimes prints the command if it failed. This means that it could
+      // potentially leak the Github token used for accessing the remote. To avoid
+      // printing a token, we sanitize the string before printing the stderr output.
+      process.stderr.write(this.omitGithubTokenFromMessage(result.stderr));
+    }
+
+    return result;
   }
 
   /** Whether the given branch contains the specified SHA. */
@@ -77,4 +106,9 @@ export class GitClient {
   hasUncommittedChanges(): boolean {
     return this.runGraceful(['diff-index', '--quiet', 'HEAD']).status !== 0;
   }
+
+  /** Sanitizes a given message by omitting the provided Github token if present. */
+  omitGithubTokenFromMessage(value: string): string {
+    return value.replace(this._tokenRegex, '<TOKEN>');
+  }
 }

scripts/merge-script/pull-request.ts

@@ -94,8 +94,13 @@ async function fetchPullRequestFromGithub(
   try {
     const result = await git.api.pulls.get({...git.repoParams, pull_number: prNumber});
     return result.data;
-  } catch {
-    return null;
+  } catch (e) {
+    // If the pull request could not be found, we want to return `null` so
+    // that the error can be handled gracefully.
+    if (e.status === 404) {
+      return null;
+    }
+    throw e;
   }
 }
 

scripts/merge-script/strategies/api-merge.ts

@@ -91,15 +91,32 @@ export class GithubApiMergeStrategy extends MergeStrategy {
       await this._promptCommitMessageEdit(pullRequest, mergeOptions);
     }
 
-    // Merge the pull request using the Github API into the selected base branch.
-    const {status, data: {sha}} = await this.git.api.pulls.merge(mergeOptions);
+    let mergeStatusCode: number;
+    let targetSha: string;
+
+    try {
+      // Merge the pull request using the Github API into the selected base branch.
+      const result = await this.git.api.pulls.merge(mergeOptions);
+
+      mergeStatusCode = result.status;
+      targetSha = result.data.sha;
+    } catch (e) {
+      // Note: Github usually returns `404` as status code if the API request uses a
+      // token with insufficient permissions. Github does this because it doesn't want
+      // to leak whether a repository exists or not. In our case we expect a certain
+      // repository to exist, so we always treat this as a permission failure.
+      if (e.status === 403 || e.status === 404) {
+        return PullRequestFailure.insufficientPermissionsToMerge();
+      }
+      throw e;
+    }
 
     // https://developer.github.com/v3/pulls/#response-if-merge-cannot-be-performed
     // Pull request cannot be merged due to merge conflicts.
-    if (status === 405) {
+    if (mergeStatusCode === 405) {
       return PullRequestFailure.mergeConflicts([githubTargetBranch]);
     }
-    if (status !== 200) {
+    if (mergeStatusCode !== 200) {
       return PullRequestFailure.unknownMergeError();
     }
 
@@ -120,7 +137,7 @@ export class GithubApiMergeStrategy extends MergeStrategy {
 
     // Cherry pick the merged commits into the remaining target branches.
     const failedBranches = await this.cherryPickIntoTargetBranches(
-        `${sha}~${targetCommitsCount}..${sha}`, cherryPickTargetBranches);
+        `${targetSha}~${targetCommitsCount}..${targetSha}`, cherryPickTargetBranches);
 
     // We already checked whether the PR can be cherry-picked into the target branches,
     // but in case the cherry-pick somehow fails, we still handle the conflicts here. The