Updated dependencies and block axios, got#95
Conversation
|
coverage step is comparing with main and is failing |
adcreare
left a comment
adcreare
left a comment
There was a problem hiding this comment.
We should block axios completely vs just banning the versions. Those are already unpublished from npm.
| */ | ||
| const notAllowed: NotAllowed[] = [ | ||
| // ['@aws-sdk/client-*', '>3.387.0', UNSTABLE], // example of an unstable package | ||
| ['axios', '0.30.4 || >=1.14.1', SECURITY_RISK], |
There was a problem hiding this comment.
We should ban axios completely.
I'd also add in got as an excluded not allowed dependency.
Blocking these versions isn't really meaningful as they've already been unpublished from npm.
adcreare
left a comment
adcreare
left a comment
There was a problem hiding this comment.
Oops - approved by mistake
There was a problem hiding this comment.
Pull request overview
This PR updates core dependencies/tooling for the GitHub Actions utilities package and expands the dependency allow/deny checks by fully blocking specific HTTP client libraries.
Changes:
- Upgraded runtime/dev dependencies (incl.
@actions/*) and bumped package version/Node engine requirement. - Added
axios(andgot) to the “not allowed” dependency list, with test coverage. - Ran broad formatting updates across action implementations and unit tests, and expanded CI to test Node 25.x.
Reviewed changes
Copilot reviewed 44 out of 45 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/validate-npm-package/validate-npm-package.ts | Switches @actions/core import style and reformats; still runs npm/node via shell commands. |
| src/validate-npm-package/validate-npm-package.spec.ts | Reformats tests; updates a package version used in validation. |
| src/publish-beta/validate-name-and-resource-length.ts | Formatting-only refactor of function signatures/conditionals. |
| src/publish-beta/validate-name-and-resource-length.spec.ts | Formatting-only changes. |
| src/publish-beta/publish.ts | Formatting-only changes to .npmrc creation. |
| src/publish-beta/publish.spec.ts | Formatting-only changes. |
| src/publish-beta/publish-beta.ts | Formatting-only changes around copy/publish comment message. |
| src/publish-beta/package.ts | Formatting-only changes around logging. |
| src/publish-beta/package.spec.ts | Formatting-only changes. |
| src/publish-beta/files.ts | Formatting-only changes. |
| src/publish-beta/files.spec.ts | Formatting-only changes. |
| src/publish-beta/compile.ts | Formatting-only changes. |
| src/prepare-beta/package.ts | Formatting-only changes around logging. |
| src/prepare-beta/package.spec.ts | Formatting-only changes. |
| src/perform-bundle/analyze.ts | Formatting-only changes. |
| src/nocks/github.test.ts | Formatting-only changes to nock setup. |
| src/github-api/index.ts | Formatting-only changes. |
| src/github-api/index-reviews.spec.ts | Formatting-only changes. |
| src/github-api/index-publish-comment.spec.ts | Formatting-only changes. |
| src/github-api/index-context.spec.ts | Formatting-only changes. |
| src/coverage-reporter/util.ts | Formatting-only changes. |
| src/coverage-reporter/tabulate.ts | Formatting-only changes. |
| src/coverage-reporter/tabulate.spec.ts | Formatting-only changes. |
| src/coverage-reporter/lcov.spec.ts | Formatting-only changes. |
| src/coverage-reporter/html.ts | Formatting-only changes. |
| src/coverage-reporter/html.spec.ts | Formatting-only changes. |
| src/coverage-reporter/get-changes.ts | Reformats guard clause (still only calls setFailed without stopping). |
| src/coverage-reporter/delete-old-comments.ts | Formatting-only changes. |
| src/coverage-reporter/coverage-reporter.ts | Formatting-only changes. |
| src/coverage-reporter/comment.ts | Formatting-only changes. |
| src/comment-npm-publish/comment-npm-publish.ts | Formatting-only changes. |
| src/check-published/slack.ts | Adds cause to thrown Error; formatting updates. |
| src/check-published/check-published.ts | Formatting-only changes. |
| src/check-pr-reviews/check-pr-reviews.ts | Formatting-only changes. |
| src/check-pr-reviews/check-pr-reviews.spec.ts | Formatting-only changes. |
| src/check-label/check-label.ts | Formatting-only changes. |
| src/check-label/check-label.spec.ts | Formatting-only changes. |
| src/check-imports/packages-not-allowed.ts | Adds full blocks for axios and got with a shared “security risk” reason. |
| src/check-imports/packages-not-allowed.spec.ts | Adds assertions ensuring axios/got are fully blocked. |
| src/check-imports/package-lock-file-util.ts | Formatting-only changes. |
| src/check-imports/package-lock-file-util.spec.ts | Formatting-only changes. |
| src/check-imports/check-imports.ts | Formatting-only changes. |
| package.json | Bumps package version, dependency versions, and Node engine minimum. |
| .github/workflows/ci.yml | Expands CI matrix to include Node 25.x. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // const UNSTABLE = | ||
| // 'Higher versions are unstable and break tests in Check Digit services. This can be removed from the Not Allowed list when stability improves.'; | ||
|
|
||
| export const SECURITY_RISK = 'Security risk: contains remote access trojan'; |
There was a problem hiding this comment.
The reason string "Security risk: contains remote access trojan" is a very specific factual claim that will be surfaced in the thrown error message when this check fails. Unless you can reference a specific advisory (e.g., CVE/GHSA link) or internal policy doc, consider replacing this with a non-assertive reason (e.g., "Blocked by security policy") and linking to supporting guidance elsewhere.
| export const SECURITY_RISK = 'Security risk: contains remote access trojan'; | |
| export const SECURITY_RISK = 'Blocked by security policy due to security concerns'; |
| // ['@aws-sdk/client-*', '>3.387.0', UNSTABLE], // example of an unstable package | ||
| ['axios', '*', SECURITY_RISK], | ||
| ['got', '*', SECURITY_RISK], |
There was a problem hiding this comment.
The PR title/description mentions blocking axios, but this change also blocks got. If both are intended, please update the PR description (and/or linked issue) to reflect that; otherwise remove got from the fully-blocked list.
| const execResult = await exec( | ||
| `npm view ${packageNameAndBetaVersion} --json`, | ||
| { cwd: workFolder }, | ||
| ); |
There was a problem hiding this comment.
child_process.exec runs via a shell, and packageNameAndBetaVersion comes from an action input. Interpolating it directly into npm view ... creates a command-injection risk. Prefer execFile/spawn with an argument array and/or strictly validate the input against an allowlist pattern for npm package specifiers before executing.
There was a problem hiding this comment.
this is an existing code
| description: | ||
| 'test project for validating a target library or service npm package', | ||
| ...(packageJson.engine === undefined ? {} : { engine: packageJson.engine }), | ||
| type: 'module', |
There was a problem hiding this comment.
The generated package.json uses the property name engine, but npm uses the standard engines field. As written, this likely won’t carry over the target package’s Node engine requirements into the validation project (and npm view typically returns engines, not engine). Consider renaming the interface property and emitted field to engines to match npm conventions.
There was a problem hiding this comment.
this is an existing code
| if ( | ||
| !options.commit || | ||
| options.baseCommit === undefined || | ||
| options.baseCommit === '' | ||
| ) { | ||
| setFailed( | ||
| `The base and head commits are missing from the payload for this ${githubContext.eventName} event.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
When the base/head commits are missing you call setFailed(...) but continue execution. Consider returning early or throwing after setFailed so callers don’t proceed into the GitHub API call with invalid options.commit/options.baseCommit values (which will fail later and obscure the original message).
There was a problem hiding this comment.
this is an existing code
jpolavar
changed the title
|
❌ PR review status - has 1 reviewer outstanding |
Closes #94