refactor(ci): unify npm publish logic and improve workflows#58
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the continuous integration (CI) scripts responsible for publishing packages to both GitHub Packages and the public npm registry. By extracting the core npm publishing logic into a dedicated utility function, the change aims to reduce code duplication, enhance maintainability, and ensure consistent publishing behavior across different release workflows. Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors and unifies the npm publish logic into a new publishTarball utility function, reducing code duplication across github.js and npm.js action scripts and improving clarity through variable renaming. While the new publishTarball function is used safely, this refactoring has brought to light existing command injection vulnerabilities in the checkVersionExists utility function. This function interpolates package names and versions directly into shell commands, which could be exploited by a malicious contributor. I recommend updating utility functions to use safer alternatives like execFileSync that do not involve shell interpolation. Additionally, consider improving the robustness of the command construction within the new publishTarball function.
| const npmVerExists = checkVersionExists( | ||
| releasePackageJson.name, | ||
| version, | ||
| GITHUB_URL | ||
| ); |
There was a problem hiding this comment.
The checkVersionExists function (defined in scripts/utils/npm.js) is vulnerable to command injection because it interpolates its arguments directly into a shell command string. In this call, releasePackageJson.name and version are passed as arguments. These values are derived from package.json, which is an untrusted source in the context of CI/CD. An attacker could modify package.json to include shell metacharacters (e.g., ; touch /tmp/pwned), leading to arbitrary command execution in the CI environment. To remediate this, the checkVersionExists function should be updated to use execFileSync or spawnSync to pass arguments safely without shell interpolation.
| const npmVerExists = checkVersionExists( | ||
| releasePackageJson.name, | ||
| version, | ||
| NPM_URL | ||
| ); |
There was a problem hiding this comment.
The checkVersionExists function (defined in scripts/utils/npm.js) is vulnerable to command injection because it interpolates its arguments directly into a shell command string. In this call, releasePackageJson.name and version are passed as arguments. These values are derived from package.json, which is an untrusted source in the context of CI/CD. An attacker could modify package.json to include shell metacharacters, leading to arbitrary command execution in the CI environment. To remediate this, the checkVersionExists function should be updated to use execFileSync or spawnSync to pass arguments safely without shell interpolation.
scripts/utils/npm.js
Outdated
| const { provenance = false } = options; | ||
| const distTag = getNpmDistTag(version); | ||
| const tagArg = distTag === "latest" ? "" : ` --tag ${distTag}`; | ||
| const provenanceArg = provenance ? " --provenance" : ""; | ||
|
|
||
| exec( | ||
| `npm publish "${tarballPath}"${provenanceArg} --registry=${registry}${tagArg}`, | ||
| { | ||
| stdio: "inherit", | ||
| } | ||
| ); |
There was a problem hiding this comment.
While the current command construction works, it relies on conditional variables (provenanceArg, tagArg) containing a leading space. This can be a bit fragile and harder to maintain. A more robust approach is to build an array of arguments and then join them. This makes the separation of arguments explicit and improves readability and maintainability.
const { provenance = false } = options;
const distTag = getNpmDistTag(version);
const args = [];
if (provenance) {
args.push("--provenance");
}
args.push(`--registry=${registry}`);
if (distTag !== "latest") {
args.push("--tag", distTag);
}
exec(`npm publish "${tarballPath}" ${args.join(" ")}`, {
stdio: "inherit",
});
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out |
1 participant
No description provided.