feat(github-actions): autoremove preview labels in pull request labeling action

[josephperrott]

Move the automatic removal of preview labels on untrusted pull requests into the pull request labeling action.

• Remove the angular-robot-key input and preview label removal step from pack-and-upload-artifact so it remains secret-free in unprivileged pull_request workflows.
• Centralize isGooglerOrgMember in github-actions/utils.ts and update post-approval-changes to reuse it.
• Add previewLabelAutoremoval to PullRequestLabeling action, automatically removing configured preview-labels (default: adev: preview) for PR authors who are not members of the googlers organization.
• Add unit tests for previewLabelAutoremoval in pull-request-labeling.spec.ts.

[gemini-code-assist]

Code Review

This pull request refactors the preview label autoremoval logic by integrating it directly into the PullRequestLabeling class and moving the shared isGooglerOrgMember helper to github-actions/utils.ts. This allows for the removal of the standalone remove-preview-label.ts script and simplifies the pack-and-upload-artifact composite action. The review feedback highlights critical security and reliability issues in the updated action.yml, specifically command injection risks from direct input interpolation and a directory nesting bug due to the removal of the temp directory cleanup. Additionally, an issue in utils.ts was identified where a try/catch block is rendered dead code because of an earlier promise rejection handler.

[gemini-code-assist]

There are two issues in this step:

1. Directory Nesting Bug: Removing rm -rf "$dir" can cause issues on persistent or self-hosted runners. If pack-and-upload-tmp-dir already exists, cp -R will copy the source directory into it, creating a nested folder (e.g., pack-and-upload-tmp-dir/my-app/...) instead of placing the files at the root.
2. Command Injection Risk: Directly interpolating ${{inputs.deploy-directory}} into the shell script is a security risk (CWE-94). If the input contains shell metacharacters (like backticks or $()), they will be executed.

Using environment variables and restoring the rm -rf cleanup solves both issues safely.

    - name: Copying artifact to temp directory to allow for metadata injection.
      id: copy
      shell: bash
      env:
        DEPLOY_DIR_INPUT: ${{inputs.deploy-directory}}
      run: |
        dir="$RUNNER_TEMP/pack-and-upload-tmp-dir"
        rm -rf "$dir"
        cp -R "$DEPLOY_DIR_INPUT" "$dir"
        chmod -R u+w "$dir"
        echo "deploy-dir=$dir" >> $GITHUB_OUTPUT

[gemini-code-assist]

Directly interpolating GitHub Actions expressions like ${{ inputs.artifact-build-revision }} or ${{ steps.copy.outputs.deploy-dir }} into a shell script is a security risk (CWE-94: Code Injection).

If any of these inputs contain a single quote (e.g., sha''; malicious_command; ''), they can break out of the single quotes and execute arbitrary commands on the runner.

To prevent command injection, you should always pass these values to the step via environment variables and reference them as shell variables (e.g., "$BUILD_REVISION").

    - name: Injecting artifact metadata
      shell: bash
      env:
        DEPLOY_DIR: ${{steps.copy.outputs.deploy-dir}}
        PULL_NUMBER: ${{inputs.pull-number}}
        BUILD_REVISION: ${{inputs.artifact-build-revision}}
      run: |
        node ${{github.action_path}}/inject-artifact-metadata.js \
          "$DEPLOY_DIR" \
          "$PULL_NUMBER" \
          "$BUILD_REVISION"

[gemini-code-assist]

In the current implementation, the catch (e) block is effectively dead code for any errors thrown by checkMembershipForUser. This is because the .then(..., () => false) handler catches all rejections (including network errors, rate limits, or bad credentials) and resolves them to false, preventing the try/catch block from ever catching them.

If you want to log actual errors while still treating 404 Not Found (which indicates the user is not a member) as a valid false result, you can check the error status in the rejection handler and rethrow other errors so they can be caught and logged.

Here is a suggested improvement:

  try {
    const isMember = await client.orgs.checkMembershipForUser({org: 'googlers', username}).then(
      ({status}) => (status as number) === 204,
      (err: any) => {
        if (err.status === 404) {
          return false;
        }
        throw err;
      },
    );
    isGooglerOrgMemberCache.set(username, isMember);
    return isMember;
  } catch (e) {
    error('Could not check googlers org membership for ' + username + ': ' + e);
    return false;
  }