fix(security): ci#103
Conversation
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR migrates CI workflows from pull_request to pull_request_target with an approval gate, pins action references to commit SHAs, and removes harden-runner steps. The most critical issues raised are: (1) the pull_request_target + explicit PR head SHA checkout combination is a well-known supply chain attack vector even with the approval gate, (2) the reusable integration-tests action is pinned to a mutable @main ref while handling secrets, and (3) matrix.cds-version is silently unavailable inside the composite action and will always resolve to empty. These should be addressed before merging.
PR Bot Information
Version: 1.20.42
- Correlation ID:
e77adc3f-40d2-44e5-8edb-11e4a7f86081 - LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.opened - File Content Strategy: Full file content
| pull_request_target: | ||
| branches: [main] | ||
| types: [reopened, synchronize, opened] | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| jobs: | ||
| requires-approval: | ||
| runs-on: ubuntu-latest | ||
| name: Waiting for PR approval as this workflow runs on pull_request_target | ||
| if: github.event_name == 'pull_request_target' && github.event.pull_request.head.repo.owner.login != 'cap-js' | ||
| environment: pr-approval | ||
| steps: | ||
| - name: Approval Step | ||
| run: echo "This job has been approved!" | ||
|
|
||
| test: | ||
| name: Tests | ||
| runs-on: ubuntu-latest | ||
| needs: requires-approval | ||
| if: always() && (needs.requires-approval.result == 'success' || needs.requires-approval.result == 'skipped') | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| node-version: [20.x, 22.x] | ||
| cds-version: [latest] | ||
| steps: | ||
| - name: Harden Runner | ||
| uses: step-security/harden-runner@8d3c67de8e2fe68ef647c8db1e6a09f647780f40 # v2.19.0 | ||
| with: | ||
| egress-policy: audit | ||
| - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | ||
| - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 | ||
| with: | ||
| ref: ${{ github.event.pull_request.head.sha || github.sha }} |
There was a problem hiding this comment.
Security: pull_request_target with explicit ref checkout of PR head SHA is a known code injection risk
The workflow was changed from pull_request to pull_request_target (line 7), and the test job explicitly checks out the untrusted PR head SHA (ref: ${{ github.event.pull_request.head.sha || github.sha }}). The pull_request_target event runs with write permissions and repository secrets in the context of the base branch. Checking out and executing code from a fork PR head under pull_request_target exposes secrets and allows arbitrary code execution by the PR author, since the requires-approval gate only applies to PRs from forks — but secrets are passed to hybrid-tests and the checkout in test still runs untrusted code.
The requires-approval environment gate does protect the hybrid-tests job (which uses secrets), but the test job also checks out and runs the PR contributor's code (npm run test, npm run build, etc.) with pull_request_target's elevated context. An attacker could modify package.json scripts to exfiltrate secrets or environment variables.
Should either:
- Keep
pull_requestfor thetestjob (which doesn't need secrets) and only usepull_request_targetfor jobs requiring secrets, or - Ensure the
testjob'sneeds: requires-approval+ theif: always() && ...condition is guaranteed to block execution from fork PRs until explicitly approved — and verify that no secrets are inadvertently available in that job's environment.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| ref: ${{ github.event.pull_request.head.sha || github.sha }} | ||
| - name: Integration tests | ||
| uses: ./.github/actions/integration-tests | ||
| uses: cap-js/process/.github/actions/integration-tests@main |
There was a problem hiding this comment.
Security: Referencing a reusable action at a mutable @main branch ref is unsafe
cap-js/process/.github/actions/integration-tests@main resolves to whatever the main branch points to at the time of execution. This means a compromised or modified action in that repository could be injected into this workflow without any review. Since hybrid-tests passes secrets (CF credentials, etc.), this is a high-severity supply chain risk.
Should pin this to a specific commit SHA to ensure the action's code is immutable and auditable:
uses: cap-js/process/.github/actions/integration-tests@<commit-sha>| uses: cap-js/process/.github/actions/integration-tests@main | |
| uses: cap-js/process/.github/actions/integration-tests@<commit-sha> # vX.Y.Z | |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| name: Tests | ||
| runs-on: ubuntu-latest | ||
| needs: requires-approval | ||
| if: always() && (needs.requires-approval.result == 'success' || needs.requires-approval.result == 'skipped') |
There was a problem hiding this comment.
Logic Error: if: always() on test and hybrid-tests can allow jobs to run even when requires-approval failed
The condition if: always() && (needs.requires-approval.result == 'success' || needs.requires-approval.result == 'skipped') is intended to run the job when approval succeeded or when the requires-approval job was skipped (e.g. for pushes/workflow_dispatch). However, if requires-approval was cancelled — which can happen if another job in the workflow is cancelled — the condition evaluates to false and is blocked correctly. But if the job transitions to a failure state (e.g. approval rejected), it is also blocked correctly.
The concern is the 'skipped' branch: requires-approval is skipped only when github.event_name != 'pull_request_target' || github.event.pull_request.head.repo.owner.login == 'cap-js'. Verify this is the intended trust boundary — first-party pushes and cap-js org PRs skip approval and run directly. This seems correct, but it should be explicitly documented in the workflow to avoid future misunderstanding.
| if: always() && (needs.requires-approval.result == 'success' || needs.requires-approval.result == 'skipped') | |
| if: always() && (needs.requires-approval.result == 'success' || needs.requires-approval.result == 'skipped') # skipped = push/workflow_dispatch or cap-js org PR; success = fork PR approved via environment gate | |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| ref: ${{ github.event.pull_request.head.sha || github.sha }} | ||
| - name: Use Node.js ${{ inputs.NODE_VERSION }} | ||
| uses: actions/setup-node@v4 | ||
| uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 |
There was a problem hiding this comment.
Bug: matrix.cds-version is referenced inside this composite action but the matrix context is not available in composite actions
The composite action references ${{ matrix.cds-version }} in its run steps (e.g. line 71, 75, 90, 96, 98, 135 of the full file). The matrix context exists only in the calling workflow's job scope and is not passed into composite actions. At runtime this expression resolves to an empty string, meaning commands like npm i -g @sap/cds-dk@ will install the latest version regardless of the matrix value — producing inconsistent test results silently.
Should add a cds-version input to the composite action and pass ${{ matrix.cds-version }} from test.yml via with:, then reference ${{ inputs.cds-version }} throughout the action.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
1 participant
fix(security): Harden CI Workflows with Pinned Action Hashes and PR Approval Gate
Refactor / Security
🔒 This PR improves the security posture of the CI/CD pipelines by replacing mutable action version tags with pinned SHA commit hashes, removing the
step-security/harden-runnersteps, switching topull_request_target, and introducing a manual approval gate for external contributors.Changes
.github/workflows/test.yml: Switched trigger frompull_requesttopull_request_target. Added a newrequires-approvaljob that gates execution for PRs from forks (outside thecap-jsorg) via apr-approvalenvironment. Bothtestandhybrid-testsjobs now depend onrequires-approval. RemovedHarden Runnersteps. Updatedhybrid-teststo reference the shared action fromcap-js/processrepository instead of a local path..github/workflows/release.yml: Replaced mutable version tags (@v6.0.2,@v6,@v1.3.1) with pinned SHA hashes foractions/checkout,actions/setup-node, andmartinbeentjes/npm-get-version-action. RemovedHarden Runnerstep frompublish-npmjob..github/workflows/lint-prettier.yml: RemovedHarden Runnerstep..github/workflows/check-changelog.yml: RemovedHarden Runnerstep..github/workflows/issue.yml: RemovedHarden Runnerstep. Updatedactions/github-scriptcomment reference fromv9.0.0tov9..github/workflows/prevent-issue-labeling.yml: RemovedHarden Runnerstep..github/actions/integration-tests/action.yml: Pinnedactions/checkoutandactions/setup-nodeto their SHA-based references.PR Bot Information
Version:
1.20.42pull_request.openedanthropic--claude-4.6-sonnete77adc3f-40d2-44e5-8edb-11e4a7f86081