chores: Set free-form CI inputs as env vars #3133
Conversation
WalkthroughRefactors the GitHub Actions release workflow to route inputs and context values through explicit environment variables, replace direct GitHub context usages with env-derived references, standardize quoting, expand job dependency wiring, and update messaging/URL templating and validations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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. Comment |
pdp2121
requested review from
bthomee,
ckeshava,
kuan121 and
shichengripple001
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
616-616: Minor: Single quotes around env var interpolation in regex are unnecessary.Line 616 uses:
'${{ env.PACKAGE_VERSION }}'GitHub Actions substitutes
${{ env.PACKAGE_VERSION }}to its literal value (e.g.,"1.0.0") before bash processes this line. The single quotes then enclose the already-substituted string literal, which is redundant. The regex matching still works correctly, but the quoting can be simplified for clarity.- if [[ "$NPM_DIST_TAG" == "latest" ]] && ! [[ '${{ env.PACKAGE_VERSION }}' =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then - echo "With npmjs_dist_tag 'latest', version must be of the form x.y.z. Found '${{ env.PACKAGE_VERSION }}'." >&2 + if [[ "$NPM_DIST_TAG" == "latest" ]] && ! [[ "${{ env.PACKAGE_VERSION }}" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then + echo "With npmjs_dist_tag 'latest', version must be of the form x.y.z. Found ${{ env.PACKAGE_VERSION }}." >&2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release.yml(21 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-09-26T21:14:56.813Z
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2788
File: .github/workflows/nodejs.yml:25-25
Timestamp: 2024-09-26T21:14:56.813Z
Learning: In `.github/workflows/nodejs.yml`, the `build-and-lint` job's actions `actions/setup-node` and `actions/cache` are updated to `v4`.
Applied to files:
.github/workflows/release.yml
📚 Learning: 2025-10-09T20:16:27.834Z
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3101
File: .github/workflows/nodejs.yml:220-221
Timestamp: 2025-10-09T20:16:27.834Z
Learning: In `.github/workflows/nodejs.yml`, the `generate-documentation` job intentionally uses `if: startsWith(github.ref, 'refs/tags/xrpl@')` instead of `env.GIT_REF` to ensure documentation is only generated and deployed on actual tag push events, not when the workflow is called via `workflow_call` with a tag reference.
Applied to files:
.github/workflows/release.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: unit (22.x)
- GitHub Check: browser (24.x)
- GitHub Check: unit (20.x)
- GitHub Check: integration (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: integration (24.x)
- GitHub Check: unit (24.x)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (8)
.github/workflows/release.yml (8)
32-94: Correctly implements the security pattern for input validation.Environment variables are properly extracted from GitHub context (lines 33-36) and safely referenced in the run script with single-quoted substitution (lines 41, 50, 74). This prevents command injection even if user-supplied
package_nameornpmjs_dist_tagcontain special characters. The path traversal guards (..,/) and validation regexes provide defense-in-depth.
96-123: Correctly applies the env var pattern to package version retrieval.Input variables are extracted to env block (lines 98-100) and safely used in the run script (lines 103, 114). The version validation logic correctly ensures
latesttag semantics with the x.y.z format check.
179-194: Slack failure notification properly uses env vars for secrets and context.The
SLACK_TOKENsecret (line 182) is never exposed in the run script; the Bearer token is injected directly into the curl header via${{ env.SLACK_TOKEN }}(line 189), which GitHub Actions substitutes before bash sees it. The repository and run ID are also safely passed through env vars.
212-223: OWASP API upload correctly isolates sensitive context.The
OWASP_TOKENsecret andPACKAGE_VERSIONare both set as env vars (lines 213-215) and safely used in the curl command (line 218). No secrets are leaked to logs or error messages.
241-248: Artifact URL construction safely composes values from env vars.GitHub repository, run ID, and artifact ID are all extracted to env (lines 243-246) and safely interpolated into the artifact download URL. This prevents injection through any of these context values.
263-290: GitHub issue creation properly parameterizes all user-controlled input.Environment variables for package name, version, branch, and artifact URL are safely passed to the
ghCLI. The jq command correctly quoting all variables prevents shell injection when building JSON.
604-621: npm publish step correctly handles dist-tag input.The
NPMJS_DIST_TAG_INPUTis extracted to env (line 606) and safely assigned in the run script (line 612). Validation logic on lines 613–619 (including the regex check) correctly prevents invalid or dangerous dist-tag values.
474-502: Dev team Slack notification correctly parameterizes dynamic message content.Environment variables for package name, version, repo, run ID, and reviewers are extracted (lines 477–486) and safely composed into the Slack message (lines 489, 491, 493–494). The multi-line bash string construction preserves safety by using env var substitution before the message is passed to
jq.
bthomee
left a comment
bthomee
left a comment
There was a problem hiding this comment.
Thanks for doing this - it's looking good so far.
.github/workflows/release.yml
Outdated
| if: failure() | ||
| env: | ||
| SLACK_TOKEN: ${{ secrets.SLACK_TOKEN }} | ||
| PACKAGE_VERSION: ${{ env.PACKAGE_VERSION }} |
There was a problem hiding this comment.
Here and elsewhere, this environment variable (re)definition shouldn't be necessary as it's already an env var.
There was a problem hiding this comment.
Great catch! I removed redundant redefinition here: 9b731b7
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
360-360: Minor inconsistency in shell variable quoting style.Line 360 uses
$REPOand$RUN_IDwithout braces, whereas the codebase generally follows the${VAR}pattern (e.g., lines 474, 533, 647, 664) per the recommendation in past reviews to avoid string concatenation pitfalls. While functionally correct in this context (jq--argreceives the unquoted expansion), consider using${REPO}and${RUN_ID}for consistency.- --arg body "Automated PR for release **$VERSION** from **$RELEASE_BRANCH** → **main**. Workflow Run: https://github.com/$REPO/actions/runs/$RUN_ID" \ + --arg body "Automated PR for release **${VERSION}** from **${RELEASE_BRANCH}** → **main**. Workflow Run: https://github.com/${REPO}/actions/runs/${RUN_ID}" \
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release.yml(14 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2788
File: .github/workflows/nodejs.yml:25-25
Timestamp: 2024-09-26T21:14:56.813Z
Learning: In `.github/workflows/nodejs.yml`, the `build-and-lint` job's actions `actions/setup-node` and `actions/cache` are updated to `v4`.
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3101
File: .github/workflows/nodejs.yml:220-221
Timestamp: 2025-10-09T20:16:27.834Z
Learning: In `.github/workflows/nodejs.yml`, the `generate-documentation` job intentionally uses `if: startsWith(github.ref, 'refs/tags/xrpl@')` instead of `env.GIT_REF` to ensure documentation is only generated and deployed on actual tag push events, not when the workflow is called via `workflow_call` with a tag reference.
📚 Learning: 2024-09-26T21:14:56.813Z
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2788
File: .github/workflows/nodejs.yml:25-25
Timestamp: 2024-09-26T21:14:56.813Z
Learning: In `.github/workflows/nodejs.yml`, the `build-and-lint` job's actions `actions/setup-node` and `actions/cache` are updated to `v4`.
Applied to files:
.github/workflows/release.yml
📚 Learning: 2025-10-09T20:16:27.834Z
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3101
File: .github/workflows/nodejs.yml:220-221
Timestamp: 2025-10-09T20:16:27.834Z
Learning: In `.github/workflows/nodejs.yml`, the `generate-documentation` job intentionally uses `if: startsWith(github.ref, 'refs/tags/xrpl@')` instead of `env.GIT_REF` to ensure documentation is only generated and deployed on actual tag push events, not when the workflow is called via `workflow_call` with a tag reference.
Applied to files:
.github/workflows/release.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: integration (24.x)
- GitHub Check: unit (22.x)
- GitHub Check: browser (24.x)
- GitHub Check: unit (24.x)
- GitHub Check: integration (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: unit (20.x)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
.github/workflows/release.yml (1)
32-123: Security refactoring for command injection mitigation is well-executed.The changes correctly move
${{ github.event.inputs.* }}and other free-form CI inputs into explicitenv:blocks at the step level, then reference them via shell-style${VAR}expansion inrun:scripts. This follows the established pattern from rippled PR #5822 and prevents user-controlled data from being evaluated as shell code. All validation logic properly uses these environment variables without direct GitHub context interpolation. The approach is sound and comprehensive across the affected steps.
| SLACK_TOKEN: ${{ secrets.SLACK_TOKEN }} | ||
| CHANNEL: "#ripplex-security" | ||
| EXECUTOR: ${{ github.triggering_actor || github.actor }} | ||
| PACKAGE_NAME: ${{ needs.get_version.outputs.package_version && github.event.inputs.package_name }} |
There was a problem hiding this comment.
Questionable logic for PACKAGE_NAME assignment in ask_for_sec_team_review job.
The expression ${{ needs.get_version.outputs.package_version && github.event.inputs.package_name }} will coerce PACKAGE_NAME to the boolean false if the version is empty, rather than being unset or defaulting to a safe fallback. This appears inconsistent with line 527 (PACKAGE_VERSION assignment) and could produce misleading output in Slack messages (line 535) that interpolate PACKAGE_NAME.
Simplify to:
- PACKAGE_NAME: ${{ needs.get_version.outputs.package_version && github.event.inputs.package_name }}
+ PACKAGE_NAME: ${{ github.event.inputs.package_name }}Unless there is a specific requirement to conditionally omit PACKAGE_NAME, in which case clarify the intent with a comment.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| PACKAGE_NAME: ${{ needs.get_version.outputs.package_version && github.event.inputs.package_name }} | |
| PACKAGE_NAME: ${{ github.event.inputs.package_name }} |
🤖 Prompt for AI Agents
In .github/workflows/release.yml around line 526, the PACKAGE_NAME assignment
uses a logical-and expression that will produce a boolean false when
package_version is empty; replace this with an explicit, safer assignment so
PACKAGE_NAME is either the provided input or an empty/default value (use an
explicit conditional comparison against package_version rather than relying on
&&), and add a short comment if the intent is to omit PACKAGE_NAME when no
version exists.
There was a problem hiding this comment.
This seems a bit questionable indeed.
I think - after making the fix I proposed in another comment - that using {{ needs.get_version.outputs.package_version }} should be enough, since now the value will get correctly set.
If the output isn't always set (or can be empty), then you are more likely to want to use || here than &&. However, in that case you will want to apply the same login in other places too, e.g. when running the faucet test.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for other uses of && or || in GitHub Actions expressions in the release.yml file
rg -n '\$\{\{.*(\&\&|\|\|).*\}\}' .github/workflows/release.yml
Length of output: 617
#!/bin/bash
# Check how PACKAGE_NAME is set in other jobs in the release.yml file
rg -n 'PACKAGE_NAME:.*\$\{\{' .github/workflows/release.yml🧠 Learnings used
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2788
File: .github/workflows/nodejs.yml:25-25
Timestamp: 2024-09-26T21:14:56.813Z
Learning: In `.github/workflows/nodejs.yml`, the `build-and-lint` job's actions `actions/setup-node` and `actions/cache` are updated to `v4`.
- Move all ${{ github.* }} and ${{ inputs.* }} from run: scripts to env: blocks
- Use Shell-style ${VAR} pattern in run: blocks for better readability
- Remove redundant env redefinitions
- Follows rippled PR #5822 best practices
pdp2121
force-pushed
the
fix-command-injection
branch
from
f6ae21a to
89c3a8d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/release.yml (1)
526-526: Fix the logical-AND expression that produces stringfalsewhen version is empty.The
PACKAGE_NAMEassignment uses${{ needs.get_version.outputs.package_version && github.event.inputs.package_name }}, which evaluates to the string literalfalseif the version is empty, rather than defaulting to the input value. This causes downstream Slack messages (line 535) to interpolatefalseas the package name.Apply this diff:
- PACKAGE_NAME: ${{ needs.get_version.outputs.package_version && github.event.inputs.package_name }} + PACKAGE_NAME: ${{ github.event.inputs.package_name }}
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
355-361: Standardize shell variable syntax to use${VAR}consistently.Lines 357 and 360 use inconsistent shell variable syntax (
$VARwithout braces) while the rest of the workflow uses${VAR}. Per prior guidance in this codebase,${VAR}is preferred to avoid string-concatenation pitfalls and for consistency.Apply this diff:
CREATE_JSON="$(jq -n \ - --arg title "Release $VERSION: $RELEASE_BRANCH → main" \ + --arg title "Release ${VERSION}: ${RELEASE_BRANCH} → main" \ --arg head "$RELEASE_BRANCH" \ - --arg base "main" \ - --arg body "Automated PR for release **$VERSION** from **$RELEASE_BRANCH** → **main**. Workflow Run: https://github.com/$REPO/actions/runs/$RUN_ID" \ + --arg base "main" \ + --arg body "Automated PR for release **${VERSION}** from **${RELEASE_BRANCH}** → **main**. Workflow Run: https://github.com/${REPO}/actions/runs/${RUN_ID}" \ '{title:$title, head:$head, base:$base, body:$body}')"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release.yml(14 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2788
File: .github/workflows/nodejs.yml:25-25
Timestamp: 2024-09-26T21:14:56.813Z
Learning: In `.github/workflows/nodejs.yml`, the `build-and-lint` job's actions `actions/setup-node` and `actions/cache` are updated to `v4`.
📚 Learning: 2024-09-26T21:14:56.813Z
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2788
File: .github/workflows/nodejs.yml:25-25
Timestamp: 2024-09-26T21:14:56.813Z
Learning: In `.github/workflows/nodejs.yml`, the `build-and-lint` job's actions `actions/setup-node` and `actions/cache` are updated to `v4`.
Applied to files:
.github/workflows/release.yml
📚 Learning: 2025-10-09T20:16:27.834Z
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3101
File: .github/workflows/nodejs.yml:220-221
Timestamp: 2025-10-09T20:16:27.834Z
Learning: In `.github/workflows/nodejs.yml`, the `generate-documentation` job intentionally uses `if: startsWith(github.ref, 'refs/tags/xrpl@')` instead of `env.GIT_REF` to ensure documentation is only generated and deployed on actual tag push events, not when the workflow is called via `workflow_call` with a tag reference.
Applied to files:
.github/workflows/release.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: browser (24.x)
- GitHub Check: integration (22.x)
- GitHub Check: unit (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: integration (24.x)
- GitHub Check: unit (24.x)
- GitHub Check: unit (20.x)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (6)
.github/workflows/release.yml (6)
32-122: Validate inputs and version retrieval steps are secure and well-structured.The refactoring properly isolates inputs into environment variables (lines 33–36, 98–100) and all downstream references use safe shell syntax with proper quoting. The validation logic correctly rejects path traversal and version-tag confusion. The fallback to
${REF_NAME}when git branch info is unavailable (lines 40–42) is appropriate.
145-303: Pre-release job properly uses environment variables for Slack, OWASP, and artifact operations.The transition of
SLACK_TOKEN,OWASP_TOKEN,REPO,RUN_ID, and other sensitive values toenv:blocks is correct. Artifact URLs are safely constructed using${VAR}interpolation within jq--argcalls (line 246). Issue creation properly quotes the title and body with environment variables (line 286).
325-387: PR creation and reviewer fetching steps safely parameterize GitHub API calls.The
ensure_prstep properly passes GitHub context through environment variables (lines 333–338) and usesjq -n --argto safely construct the API payload (lines 356–361). Thefetch_reviewersfunction correctly retrieves environment configuration from the GitHub API using bearer-token authentication (lines 393–415).
425-487: Slack messaging steps use safe environment-variable interpolation.The release summary and dev-team notification steps correctly interpolate artifact IDs, branch names, and repository URLs via environment variables. The
MSGconstruction on line 481 usesprintf '%b'to safely handle multi-line content. All HTTP headers and request bodies are safely built with jq and shell variables.
589-605: npm publishing step correctly isolates dist-tag input and applies version validation.The
NPMJS_DIST_TAG_INPUTenvironment variable is safely referenced to build the effective tag (line 596). The fallback to"latest"and the semver-format check forlatestreleases (lines 600–603) prevent invalid release states.
607-624: Git tagging step safely constructs tag name from environment variables.Line 611 uses
${PACKAGE_NAME}and${PACKAGE_VERSION}with the@separator, which are validated inputs that have already undergone regex checks (e.g., line 51). The existence check and push are guarded appropriately.
pdp2121
force-pushed
the
fix-command-injection
branch
from
89c3a8d to
ab163e8
Compare
| - name: Validate inputs | ||
| env: | ||
| REF_NAME: ${{ github.ref_name }} | ||
| PACKAGE_NAME_INPUT: ${{ github.event.inputs.package_name }} |
There was a problem hiding this comment.
Some optional feedback, which is easy to fix but not a blocker in any way:
- In
env:you now havePACKAGE_NAME_INPUT: ${{ github.event.inputs.package_name }}. - In
run:you then doPKG_NAME="${PACKAGE_NAME_INPUT}".
You can easily combine this by:
- In
env:doPKG_NAME: ${{ github.event.inputs.package_name }}. - In
run:remove the assignment statement. ThePKG_NAMEcan be used directly in the script.
Separately, I noticed you sometimes refer to PACKAGE_XXX and sometimes to PKG_XXX for some XXX. You could consider doing a find-and-replace to make them all consistent. I'd suggest replacing all occurrences of PACKAGE by PKG since it's shorter and its meaning is clear.
| env: | ||
| REF_NAME: ${{ github.ref_name }} | ||
| PACKAGE_NAME_INPUT: ${{ github.event.inputs.package_name }} | ||
| NPMJS_DIST_TAG_INPUT: ${{ github.event.inputs.npmjs_dist_tag }} |
There was a problem hiding this comment.
The same applies to this one, where you define NPMJS_DIST_TAG_INPUT here and then in the code you assign it to NPM_DIST_TAG; you may as well use NPM_DIST_TAG here directly.
| run: | | ||
| curl -X POST \ | ||
| -H "X-Api-Key: ${{ secrets.OWASP_TOKEN }}" \ | ||
| -H "X-Api-Key: ${OWASP_TOKEN}" \ |
There was a problem hiding this comment.
FWIW I believe it's not a concern to directly reference secrets via ${{ secrets.XXX }} in a run: statement, since end users cannot manipulate them (unlike the GitHub object, according to Semgrep).
Either way, assigning it to an env var is perfectly fine, so you can leave this change as -is.
| fi | ||
| echo "PACKAGE_VERSION=$VERSION" >> "$GITHUB_ENV" | ||
| echo "version=$VERSION" >> "$GITHUB_OUTPUT" | ||
| echo "PACKAGE_VERSION=${VERSION}" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
| echo "PACKAGE_VERSION=${VERSION}" >> "$GITHUB_ENV" | |
| echo "package_version=${VERSION}" >> "$GITHUB_OUTPUT" |
In follow-up jobs you refer to ${{ needs.get_version.outputs.package_version }} but that will always be empty, since you're writing the version as an env var and not as an output.
| SLACK_TOKEN: ${{ secrets.SLACK_TOKEN }} | ||
| CHANNEL: "#ripplex-security" | ||
| EXECUTOR: ${{ github.triggering_actor || github.actor }} | ||
| PACKAGE_NAME: ${{ needs.get_version.outputs.package_version && github.event.inputs.package_name }} |
There was a problem hiding this comment.
This seems a bit questionable indeed.
I think - after making the fix I proposed in another comment - that using {{ needs.get_version.outputs.package_version }} should be enough, since now the value will get correctly set.
If the output isn't always set (or can be empty), then you are more likely to want to use || here than &&. However, in that case you will want to apply the same login in other places too, e.g. when running the faucet test.
Address some outstanding comments in #3133. Changes include: Use env variable names PKG_NAME, PKG_VERSION, and NPM_DIST_TAG consistently within the script and remove unnecessary intermediate variables. Write package version in line https://github.com/XRPLF/xrpl.js/blob/main/.github/workflows/release.yml#L122 into $GITHUB_OUTPUT instead of $GITHUB_ENV to be used in other jobs (instead of the same job). Fix line https://github.com/XRPLF/xrpl.js/blob/main/.github/workflows/release.yml#L526 to just use ${{ needs.get_version.outputs.package_name }} directly since this should be available and && operator does not make much sense in this context.
High Level Overview of Change
Changes made:
${{ github.* }},${{ inputs.* }}, and${{ secrets.* }}references from inline usage in bash scripts to theenv:block of each stepContext of Change
Semgrep scanning indicated:
Similar fix: rippled PR #5822
Type of Change
Did you update HISTORY.md?
Test Plan
CI releases pass.