ci(workflows): use environment variables in scripts to prevent command injection #3564
Conversation
|
Caution Review failedThe pull request is closed. 📝 Walkthrough""" WalkthroughThe changes refactor several GitHub Actions workflows and related scripts to replace direct variable interpolation in shell commands with the use of environment variables. This adjustment is applied consistently across workflow files and scripts, affecting how inputs, secrets, and other variables are referenced within shell scripts, without altering the underlying logic or functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub Actions Workflow
participant Shell Script/Command
participant Environment Variables
GitHub Actions Workflow->>Environment Variables: Set variables (e.g., PR_NUMBER, PACKAGE_VERSION)
GitHub Actions Workflow->>Shell Script/Command: Run script referencing env vars (e.g., $PR_NUMBER)
Shell Script/Command->>Environment Variables: Access variables via $VAR
Shell Script/Command->>Shell Script/Command: Execute logic using env vars
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (6)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the publish-docs.js script and several GitHub Actions workflows to pass runtime parameters via environment variables instead of inline string interpolation, mitigating command‐injection risks.
- Removed direct
--authorName/--authorEmailflags and introducedCOMMIT_MESSAGEenv var inpublish-docs.js. - Updated
publish-docs.yml,pr-checks.yml,create-backport-branch.yml, andcleanup.ymlto export inputs as environment variables (e.g.DOCS_VERSION,PR_NUMBER) and reference them in shell steps. - Modified the S3 upload action to use a
PACKAGE_VERSIONenv var for the destination path.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| publish-docs.js | Removed author flags and now injects COMMIT_MESSAGE via env. |
| .github/workflows/publish-docs.yml | Introduced DOCS_VERSION and FORCE_PUSH env vars. |
| .github/workflows/pr-checks.yml | Moved PR number and token into PR_NUMBER/MERGE_TOKEN env. |
| .github/workflows/create-backport-branch.yml | Exported version inputs as LAST_RELEASE_VERSION/NEW_RELEASE_VERSION env. |
| .github/workflows/cleanup.yml | Added PR_NUMBER env var for --remove=PR-$PR_NUMBER. |
| .github/actions/upload-frontend-build-output-to-s3/action.yml | Added PACKAGE_VERSION env var for S3 path. |
Comments suppressed due to low confidence (2)
publish-docs.js:50
- The usage output should be updated to document the new
COMMIT_MESSAGEenvironment variable and remove references to the deprecated--authorName/--authorEmailflags to keep the help text accurate.
usage: npm run docs:publish [-- [--v=<version>] [--remove=<pattern>]
publish-docs.js:380
- Add unit or integration tests to cover the new commit logic using
COMMIT_MESSAGEto ensure the script behaves correctly when environment variables are set or missing.
function commit(message) {
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3564/ |
adrianschmidt
force-pushed
the
fix-command-injection-vulnerability
branch
from
e659399 to
a3cc3f7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
.github/workflows/create-backport-branch.yml (1)
53-60: 🧹 Nitpick (assertive)Apply same quoting to
$GITHUB_OUTPUT
Consistently wrap$GITHUB_OUTPUTin quotes for this step as well:- echo "new_major=$new_major" >> $GITHUB_OUTPUT - echo "new_minor=$new_minor" >> $GITHUB_OUTPUT + echo "new_major=$new_major" >> "$GITHUB_OUTPUT" + echo "new_minor=$new_minor" >> "$GITHUB_OUTPUT"
♻️ Duplicate comments (1)
.github/workflows/publish-docs.yml (1)
48-53: 🧹 Nitpick (assertive)Quote environment variables to prevent word splitting
While using environment variables is a good security practice, the variables should be quoted in the shell command to prevent word splitting or unintended globbing if they contain spaces or special characters.
- npm run docs:publish -- --v=$DOCS_VERSION $FORCE_PUSH_PARAM + npm run docs:publish -- --v="$DOCS_VERSION" "$FORCE_PUSH_PARAM"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
.github/actions/upload-frontend-build-output-to-s3/action.yml(1 hunks).github/workflows/cleanup.yml(1 hunks).github/workflows/create-backport-branch.yml(6 hunks).github/workflows/pr-checks.yml(1 hunks).github/workflows/publish-docs.yml(1 hunks)publish-docs.js(1 hunks)
🔇 Additional comments (8)
.github/actions/upload-frontend-build-output-to-s3/action.yml (1)
26-32: Good security improvement with environment variables!Using environment variables instead of direct variable interpolation in shell commands is an excellent security practice that prevents potential command injection vulnerabilities. This change correctly implements this pattern by setting the package version as an environment variable and referencing it in the AWS S3 command.
.github/workflows/cleanup.yml (1)
30-33: Proper security practice implementedExcellent change to use environment variables instead of direct interpolation in shell commands. This prevents command injection vulnerabilities by ensuring the PR number is properly passed to the script.
.github/workflows/pr-checks.yml (1)
97-100: Good security improvement for API callUsing environment variables for the PR number and merge token in the curl command is a significant security improvement. This prevents potential command injection attacks that could occur with direct variable interpolation in shell commands.
publish-docs.js (1)
385-388: Good use of environment variables for commit messageUsing an environment variable for the commit message is a solid security improvement that prevents command injection vulnerabilities. The implementation correctly passes the environment variable to the shell process.
.github/workflows/create-backport-branch.yml (4)
63-74: Approve environment variable propagation
Binding the step outputs (new_major,new_minor,last_major,last_minor) to environment variables improves readability and security. The comparison logic is unchanged and correctly uses numeric tests.
87-96: Approve branch naming via env vars
Usingenvto injectBUMP_TYPE,LAST_MAJOR, andLAST_MINORkeeps the script clean. The conditional logic for settingbranch_nameis clear and concise.
108-114: Approve optional git tag input handling
Mapping the optionallast_release_git_taginput intoLAST_RELEASE_GIT_TAG_INPUTand falling back tov{version}is robust. Environment variable usage is consistent throughout.
130-140: Approve tag-based branch creation steps
Fetching the tag, checking it out, branching, and pushing withBRANCH_NAMEfromenvcorrectly leverages the environment variables and avoids inlining${{ }}.
adrianschmidt
force-pushed
the
fix-command-injection-vulnerability
branch
from
a3cc3f7 to
c234c78
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
.github/actions/upload-frontend-build-output-to-s3/action.yml(1 hunks).github/workflows/cleanup.yml(1 hunks).github/workflows/create-backport-branch.yml(3 hunks).github/workflows/pr-checks.yml(1 hunks).github/workflows/publish-docs.yml(1 hunks)publish-docs.js(1 hunks)
🔇 Additional comments (6)
.github/workflows/create-backport-branch.yml (6)
41-47: Correctly source and quote last release version extraction
TheExtract Last Release Major and Minor Versionsstep now uses anenvdeclaration forLAST_RELEASE_VERSIONand properly quotes$GITHUB_OUTPUTwhen exportinglast_majorandlast_minor, preventing word-splitting or injection.
53-59: Correctly source and quote new release version extraction
TheExtract New Release Major and Minor Versionsstep cleanly pullsNEW_RELEASE_VERSIONviaenvand safely exportsnew_major/new_minorwith quoted redirections to$GITHUB_OUTPUT.
65-85: Use environment variables for bump type determination
TheDetermine Bump Typestep now sourcesNEW_MAJOR,NEW_MINOR,LAST_MAJOR, andLAST_MINORthroughenv, uses proper integer comparisons (-gt,-eq), and quotes$GITHUB_OUTPUTwhen settingbump_type.
89-107: Use environment variables for branch name computation
TheDetermine Branch Namestep consistently pulls inBUMP_TYPE,LAST_MAJOR, andLAST_MINORviaenvand safely writesbranch_nameto$GITHUB_OUTPUTwith proper quoting.
111-121: Use environment variables for setting the last release git tag
TheSet Last Release Git Tagstep cleanly differentiates between an explicit input and a default fallback by usingLAST_RELEASE_GIT_TAG_INPUT/LAST_RELEASE_VERSION_INPUTand quotes the output redirection correctly.
132-140: Use environment variables for branch creation commands
TheCreate Release Branchstep properly fetches, checks out, and pushes usingLAST_RELEASE_GIT_TAGandBRANCH_NAMEfromenv, and quotes these variables in the Git commands to guard against injection or splitting.
adrianschmidt
force-pushed
the
fix-command-injection-vulnerability
branch
from
96cd68b to
d4084fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/pr-checks.yml (1)
97-107: Multiline Automerge step for readability
The Automergecurlcommand has been refactored into a multiline block withPR_NUMBERandMERGE_TOKENinjected as environment variables, enhancing clarity and reducing quoting issues. This matches the previously suggested improvement.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
.github/actions/upload-frontend-build-output-to-s3/action.yml(1 hunks).github/workflows/cleanup.yml(1 hunks).github/workflows/create-backport-branch.yml(3 hunks).github/workflows/pr-checks.yml(1 hunks).github/workflows/publish-docs.yml(1 hunks)publish-docs.js(1 hunks)
🔇 Additional comments (8)
.github/workflows/create-backport-branch.yml (4)
65-85: Bump type determination is secure and clear
Inputs from previous steps are injected viaenv, andbump_typeis determined purely in shell with all outputs quoted. This eliminates inline interpolation and mitigates command injection risks.
89-107: Branch naming logic with environment variables
TheBUMP_TYPE,LAST_MAJOR, andLAST_MINORvalues are now sourced fromenv, and the generatedbranch_nameis echoed safely. This decouples logic from GitHub expressions and reduces injection surface.
110-122: Robust git tag fallback via environment variables
By injectingLAST_RELEASE_GIT_TAG_INPUTandLAST_RELEASE_VERSION_INPUT, you ensure a safe fallback tov{version}when no tag is provided. The conditional check and subsequentechoto"$GITHUB_OUTPUT"are properly quoted.
132-140: Safe branch creation steps
TheLAST_RELEASE_GIT_TAGandBRANCH_NAMEfrom prior steps are injected viaenvand used with proper quoting ingit fetch,git checkout, andgit push, preventing any command injection and handling branch names safely..github/workflows/cleanup.yml (1)
30-33: Safe removal of PR docs via environment variables
The pull request number is now passed asPR_NUMBERinenv, and the--removeflag value is quoted ("PR-$PR_NUMBER") to prevent word splitting or globbing. This aligns with secure shell command practices..github/actions/upload-frontend-build-output-to-s3/action.yml (1)
26-32: Secure upload to S3 with quoted arguments
ThePACKAGE_VERSIONinput is injected viaenv, and both the tar extraction ("$FILENAME") and theaws s3 cpdestination path ("s3://limecloud-static-files/packages/$PKG_NAME/$PACKAGE_VERSION") are properly quoted. This guards against special characters in file names and package versions..github/workflows/publish-docs.yml (1)
45-53: Conditional publish with safe flag injection
TheversionandforcePushinputs are now exposed asDOCS_VERSIONandFORCE_PUSHvariables. The script conditionally includes the--forcePushflag only when needed, avoiding empty arguments, and all values are properly quoted.publish-docs.js (1)
385-393: Excellent security improvement that prevents command injection!The code now properly passes the commit message as an environment variable instead of directly interpolating it into the shell command. This change effectively prevents command injection vulnerabilities. Additionally, the error handling has been improved to gracefully handle no-op commits by checking for "nothing to commit" and "no changes added to commit" messages.
There was a problem hiding this comment.
This workflow is not in active use. It can be run manually, but would need some updating to be more user friendly.
Successful test run here: https://github.com/Lundalogik/lime-elements/actions/runs/15205209129/job/42766624340
There was a problem hiding this comment.
There was a problem hiding this comment.
This workflow has obviously been run for this PR 😄
There was a problem hiding this comment.
This workflow has also been run for this PR.
There was a problem hiding this comment.
This script is called by the above workflow, so it has been run for this PR.
Result available here (until it's been cleaned up by cleanup.yml of course): https://lundalogik.github.io/lime-elements/versions/PR-3564/#/
adrianschmidt
force-pushed
the
fix-command-injection-vulnerability
branch
from
8f1eba6 to
4099d10
Compare
…ons workflows - Replace vulnerable `$(cat .nvmrc)` pattern with safe file reading - Create reusable composite action for secure Node.js setup - Update all workflows to use the new secure setup method fix: #3564
…ons workflows - Replace vulnerable `$(cat .nvmrc)` pattern with safe file reading - Create reusable composite action for secure Node.js setup - Update all workflows to use the new secure setup method fix: #3564
…ons workflows - Replace vulnerable `$(cat .nvmrc)` pattern with safe file reading - Create reusable composite action for secure Node.js setup - Update all workflows to use the new secure setup method fix: #3564
|
🎉 This PR is included in version 38.13.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
fix: #3561
Summary by CodeRabbit
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: