Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .prettierignore
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ github-actions/org-file-sync/main.js
github-actions/post-approval-changes/main.js
github-actions/release/publish/main.js
github-actions/previews/pack-and-upload-artifact/inject-artifact-metadata.js
github-actions/previews/pack-and-upload-artifact/remove-preview-label.js
github-actions/previews/upload-artifacts-to-firebase/extract-artifact-metadata.js
github-actions/previews/upload-artifacts-to-firebase/fetch-workflow-artifact.js
github-actions/saucelabs/set-saucelabs-env.js
Expand Down
19 changes: 17 additions & 2 deletions github-actions/labeling/issue/main.js

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions github-actions/labeling/pull-request/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ ts_project(
"//github-actions/labeling/pull-request:__subpackages__",
],
deps = [
"//github-actions:utils",
"//github-actions/labeling:node_modules/@actions/core",
"//github-actions/labeling:node_modules/@actions/github",
"//github-actions/labeling:node_modules/@octokit/rest",
Expand Down
4 changes: 4 additions & 0 deletions github-actions/labeling/pull-request/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ inputs:
labels:
description: 'A map of labels to the paths that they should be applied to which are affected by the PR'
required: false
preview-labels:
description: 'Labels to automatically remove from pull requests if the author is not a member of the googlers organization.'
required: false
default: 'adev: preview'
runs:
using: 'node24'
main: 'main.js'
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {PullRequestLabeling as _PullRequestLabeling} from './pull-request-labeling.js';
import {Octokit} from '@octokit/rest';
import {utils} from '../../../utils.js';

class PullRequestLabeling extends _PullRequestLabeling {
setGit(git: any) {
Expand All @@ -13,6 +14,8 @@ describe('PullRequestLabeling', () => {
let getLabelsFromInputSpy: jasmine.Spy;

beforeEach(() => {
process.env['INPUT_PREVIEW-LABELS'] = 'adev: preview';

mockGit = jasmine.createSpyObj('Octokit', ['paginate', 'issues', 'pulls']);
mockGit.issues = jasmine.createSpyObj('issues', [
'listLabelsOnIssue',
Expand All @@ -36,14 +39,20 @@ describe('PullRequestLabeling', () => {
});

(mockGit.issues.listLabelsOnIssue as unknown as jasmine.Spy).and.resolveTo({data: []});
(mockGit.pulls.get as unknown as jasmine.Spy).and.resolveTo({data: {base: {ref: 'main'}}});
(mockGit.pulls.get as unknown as jasmine.Spy).and.resolveTo({
data: {base: {ref: 'main'}, user: {login: 'someuser'}},
});

getLabelsFromInputSpy = spyOn(PullRequestLabeling.prototype, 'getLabelsFromInput');

labeling = new PullRequestLabeling();
labeling.setGit(mockGit as unknown as Octokit);
});

afterEach(() => {
delete process.env['INPUT_PREVIEW-LABELS'];
});

it('should apply labels based on path configuration', async () => {
getLabelsFromInputSpy.and.returnValue({
'target: feature': ['feature/**'],
Expand Down Expand Up @@ -75,4 +84,68 @@ describe('PullRequestLabeling', () => {

expect(mockGit.issues.addLabels).not.toHaveBeenCalled();
});

describe('previewLabelAutoremoval', () => {
it('should remove preview label if author is not a Googler', async () => {
(mockGit.issues.listLabelsOnIssue as unknown as jasmine.Spy).and.resolveTo({
data: [{name: 'adev: preview'}, {name: 'target: feature'}],
});
spyOn(utils, 'isGooglerOrgMember').and.resolveTo(false);

await labeling.initialize();
await labeling.previewLabelAutoremoval();

expect(utils.isGooglerOrgMember).toHaveBeenCalledWith(
'someuser',
mockGit as unknown as Octokit,
);
expect(mockGit.issues.removeLabel).toHaveBeenCalledWith(
jasmine.objectContaining({
name: 'adev: preview',
}),
);
});

it('should keep preview label if author is a Googler', async () => {
(mockGit.issues.listLabelsOnIssue as unknown as jasmine.Spy).and.resolveTo({
data: [{name: 'adev: preview'}, {name: 'target: feature'}],
});
spyOn(utils, 'isGooglerOrgMember').and.resolveTo(true);

await labeling.initialize();
await labeling.previewLabelAutoremoval();

expect(utils.isGooglerOrgMember).toHaveBeenCalledWith(
'someuser',
mockGit as unknown as Octokit,
);
expect(mockGit.issues.removeLabel).not.toHaveBeenCalled();
});

it('should do nothing if PR does not have preview label', async () => {
(mockGit.issues.listLabelsOnIssue as unknown as jasmine.Spy).and.resolveTo({
data: [{name: 'target: feature'}],
});
spyOn(utils, 'isGooglerOrgMember').and.resolveTo(false);

await labeling.initialize();
await labeling.previewLabelAutoremoval();

expect(mockGit.issues.removeLabel).not.toHaveBeenCalled();
});

it('should do nothing if preview-labels input is empty', async () => {
process.env['INPUT_PREVIEW-LABELS'] = '';
(mockGit.issues.listLabelsOnIssue as unknown as jasmine.Spy).and.resolveTo({
data: [{name: 'adev: preview'}],
});
spyOn(utils, 'isGooglerOrgMember');

await labeling.initialize();
await labeling.previewLabelAutoremoval();

expect(utils.isGooglerOrgMember).not.toHaveBeenCalled();
expect(mockGit.issues.removeLabel).not.toHaveBeenCalled();
});
});
});
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as core from '@actions/core';
import {context} from '@actions/github';
import {Octokit, RestEndpointMethodTypes} from '@octokit/rest';
import {RestEndpointMethodTypes} from '@octokit/rest';
import {Commit, parseCommitMessage} from '../../../../ng-dev/commit-message/parse.js';
import {
actionLabels,
Expand All @@ -10,6 +10,7 @@ import {

import micromatch from 'micromatch';
import {ManagedRepositories} from '../../../../ng-dev/pr/common/labels/base.js';
import {utils} from '../../../utils.js';
import {Labeling} from '../../shared/labeling.js';

/** The type of the response data for a the pull request get method on from octokit. */
Expand Down Expand Up @@ -38,6 +39,7 @@ export class PullRequestLabeling extends Labeling {
await this.commitMessageBasedLabeling();
await this.pathBasedLabeling();
await this.pullRequestMetadataLabeling();
await this.previewLabelAutoremoval();
}

/**
Expand Down Expand Up @@ -122,6 +124,44 @@ export class PullRequestLabeling extends Labeling {
}
}

/**
* Automatically remove preview trigger labels if the PR author is not a member of the googlers
* organization.
*/
async previewLabelAutoremoval() {
const author = this.pullRequestMetadata?.user?.login;
if (!author) {
core.debug('No PR author found, skipping preview label autoremoval.');
return;
}

const previewLabels = core.getMultilineInput('preview-labels', {trimWhitespace: true});
if (previewLabels.length === 0) {
return;
}

core.info(`Checking if PR #${context.issue.number} author (${author}) is a Googler...`);
const isGoogler = await utils.isGooglerOrgMember(author, this.git);

if (isGoogler) {
core.info(`PR author ${author} is a member of the googlers org. Keeping preview label.`);
return;
}

// Find any preview label currently applied to the PR
const labelsToRemove = previewLabels.filter((label) => this.labels.has(label));
if (labelsToRemove.length === 0) {
core.debug('No matching preview label found on PR.');
return;
}

core.info(`PR author ${author} is NOT a member of the googlers org. Removing preview label...`);
for (const label of labelsToRemove) {
await this.removeLabel(label);
this.labels.delete(label);
}
}

/** Initialize the current labels and commits for the PR. */
async initialize() {
const {number, owner, repo} = context.issue;
Expand Down
58 changes: 56 additions & 2 deletions github-actions/labeling/pull-request/main.js

Large diffs are not rendered by default.

30 changes: 8 additions & 22 deletions github-actions/post-approval-changes/lib/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@ import * as core from '@actions/core';
import {context} from '@actions/github';
import {PullRequestEvent} from '@octokit/webhooks-types';
import {Octokit, RestEndpointMethodTypes} from '@octokit/rest';
import {ANGULAR_ROBOT, getAuthTokenFor, revokeActiveInstallationToken} from '../../utils.js';
import {
ANGULAR_ROBOT,
getAuthTokenFor,
isGooglerOrgMember,
revokeActiveInstallationToken,
} from '../../utils.js';

/** Allowlist of known Google owned robot accounts. */
const googleOwnedRobots = ['angular-robot'];
Expand Down Expand Up @@ -58,7 +63,7 @@ async function runPostApprovalChangesAction(

const actionUser = context.actor;

if (await isGooglerOrgMember(membershipCheckClient, actionUser)) {
if (await isGooglerOrgMember(actionUser, membershipCheckClient)) {
core.info(
'Action performed by an account in the Googler Github Org, skipping as post approval changes are allowed.',
);
Expand Down Expand Up @@ -104,7 +109,7 @@ async function runPostApprovalChangesAction(
continue;
}
// Only consider reviews by Googlers for this check.
if (!(await isGooglerOrgMember(membershipCheckClient, user))) {
if (!(await isGooglerOrgMember(user, membershipCheckClient))) {
continue;
}
knownReviewers.add(user);
Expand Down Expand Up @@ -142,25 +147,6 @@ async function runPostApprovalChangesAction(
});
}

/** Set of membership lookup results, used as cache for lookups. */
const isGooglerOrgMemberCache = new Map<string, boolean>([]);

async function isGooglerOrgMember(client: Octokit, username: string): Promise<boolean> {
if (isGooglerOrgMemberCache.has(username)) {
return isGooglerOrgMemberCache.get(username)!;
}
return await client.orgs
.checkMembershipForUser({org: 'googlers', username})
.then(
({status}) => (status as number) === 204,
() => false,
)
.then((result) => {
isGooglerOrgMemberCache.set(username, result);
return result;
});
}

// Only run if the action is executed in a repository with is in the Angular org. This is in place
// to prevent the action from actually running in a fork of a repository with this action set up.
// Runs triggered via 'workflow_dispatch' are also allowed to run.
Expand Down
33 changes: 17 additions & 16 deletions github-actions/post-approval-changes/main.js

Large diffs are not rendered by default.

14 changes: 0 additions & 14 deletions github-actions/previews/pack-and-upload-artifact/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,7 @@ ts_project(
srcs = glob(["lib/*.ts"]),
tsconfig = "//github-actions:tsconfig",
deps = [
":node_modules/@actions/core",
":node_modules/@octokit/rest",
":node_modules/@types/node",
"//github-actions:utils",
"//github-actions/previews:constants_lib",
],
)
Expand All @@ -28,14 +25,3 @@ esbuild_checked_in(
platform = "node",
target = "node24",
)

esbuild_checked_in(
name = "remove-preview-label",
srcs = [
":lib",
],
entry_point = "lib/remove-preview-label.ts",
format = "esm",
platform = "node",
target = "node24",
)
39 changes: 5 additions & 34 deletions github-actions/previews/pack-and-upload-artifact/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,56 +32,28 @@ inputs:
Project-relative path to the directory contents that should be deployed.
This is usually the distribution directory, like `dist/my-app/`.

angular-robot-key:
description: 'The private key for the Angular Robot Github app.'
required: true

triggering-label:
description: Label that triggers the preview deployment.
required: true

runs:
using: composite
steps:
- name: Automatically remove preview trigger label if PR author is not a Googler
if: contains(github.event.pull_request.labels.*.name, inputs.triggering-label)
shell: bash
env:
INPUT_ANGULAR-ROBOT-KEY: '${{inputs.angular-robot-key}}'
run: |
node ${{github.action_path}}/remove-preview-label.js \
'${{inputs.pull-number}}' \
'${{inputs.triggering-label}}'

- name: Copying artifact to temp directory to allow for metadata injection.
id: copy
if: contains(github.event.pull_request.labels.*.name, inputs.triggering-label)
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"
dir="$RUNNER_TEMP/pack-and-upload-tmp-dir/"
cp -R "${{inputs.deploy-directory}}" "$dir"
chmod -R u+w "$dir"
echo "deploy-dir=$dir" >> $GITHUB_OUTPUT
Comment on lines 38 to 45

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

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


- name: Injecting artifact metadata
if: contains(github.event.pull_request.labels.*.name, inputs.triggering-label)
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"
'${{steps.copy.outputs.deploy-dir}}' \
'${{inputs.pull-number}}' \
'${{inputs.artifact-build-revision}}'
Comment on lines 47 to +53

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

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"


- name: Creating compressed tarball of artifact
id: pack
if: contains(github.event.pull_request.labels.*.name, inputs.triggering-label)
shell: bash
run: |
pkg="$RUNNER_TEMP/deploy-artifact.tar.gz"
Expand All @@ -90,7 +62,6 @@ runs:
echo "artifact-path=$pkg" >> $GITHUB_OUTPUT

- uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
if: contains(github.event.pull_request.labels.*.name, inputs.triggering-label)
with:
name: '${{inputs.workflow-artifact-name}}'
path: '${{steps.pack.outputs.artifact-path}}'
Loading
Loading