fix(dev-infra): rebase pr script and token sanitization not working

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<NgDevConfig, 'github'> = 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'});
   }
 }
 

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<string[]>|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<NgDevConfig, 'github'> = getConfig()) {
+      private _githubToken?: string, private _config: Pick<NgDevConfig, 'github'> = getConfig()) {
+    // 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 (_githubToken != null) {
+      this._githubTokenRegex = new RegExp(_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
@@ -132,12 +142,17 @@ 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. */
   omitGithubTokenFromMessage(value: string): string {
-    return value.replace(this._tokenRegex, '<TOKEN>');
+    // 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, '<TOKEN>');
   }
 
   /**

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<ExecOptions, 'async'>): ShellString {
+  return _exec(cmd, {silent: true, ...opts, async: false});
 }