Skip to content

fix(security): ci #103

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
SirSimon04 wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix(security): ci #103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f07a9a0
pin shas
SirSimon04 May 6, 2026
977f558
rm harden runner
SirSimon04 May 6, 2026
6519277
pr to pr_target
SirSimon04 May 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/actions/integration-tests/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,11 @@ runs:
fi
done

- uses: actions/checkout@v5
- uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5
with:
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
Copy link
Copy Markdown
Contributor

@hyperspace-insights hyperspace-insights Bot May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

with:
node-version: ${{ inputs.NODE_VERSION }}

Expand Down
4 changes: 0 additions & 4 deletions .github/workflows/check-changelog.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,6 @@ jobs:
name: Check Changelog Action
runs-on: ubuntu-latest
steps:
- name: Harden Runner
uses: step-security/harden-runner@8d3c67de8e2fe68ef647c8db1e6a09f647780f40 # v2.19.0
with:
egress-policy: audit
- uses: tarides/changelog-check-action@0189fc7eedec3ef3e9648c713908f6f2a6e99057 # v3
with:
changelog: CHANGELOG.md
6 changes: 1 addition & 5 deletions .github/workflows/issue.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,14 @@ jobs:
label_issues:
runs-on: ubuntu-latest
steps:
- name: Harden Runner
uses: step-security/harden-runner@8d3c67de8e2fe68ef647c8db1e6a09f647780f40 # v2.19.0
with:
egress-policy: audit
- run: gh issue edit "$NUMBER" --add-label "$LABELS"
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
GH_REPO: ${{ github.repository }}
NUMBER: ${{ github.event.issue.number }}
LABELS: New

- uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0
- uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9
with:
script: |
github.rest.issues.createComment({
Expand Down
4 changes: 0 additions & 4 deletions .github/workflows/lint-prettier.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,6 @@ jobs:
lint:
runs-on: ubuntu-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/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0
- run: npm i
Expand Down
4 changes: 0 additions & 4 deletions .github/workflows/prevent-issue-labeling.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,6 @@ jobs:
remove_new_label:
runs-on: ubuntu-latest
steps:
- name: Harden Runner
uses: step-security/harden-runner@8d3c67de8e2fe68ef647c8db1e6a09f647780f40 # v2.19.0
with:
egress-policy: audit
- name: Remove "New" label if applied by non-bot user
if: >
contains(github.event.issue.labels.*.name, 'New') &&
Expand Down
10 changes: 3 additions & 7 deletions .github/workflows/release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ jobs:
node-version: [20.x, 22.x]
cds-version: [latest]
steps:
- uses: actions/checkout@v6.0.2
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6
- name: Use Node.js ${{ matrix.node-version }}
uses: actions/setup-node@v6
uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6
with:
node-version: ${{ matrix.node-version }}
- run: npm i -g @sap/cds-dk@${{ matrix.cds-version }}
Expand All @@ -31,10 +31,6 @@ jobs:
runs-on: ubuntu-latest
environment: npm
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/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0
with:
Expand All @@ -45,7 +41,7 @@ jobs:
- run: npm run build
- name: get-version
id: package-version
uses: martinbeentjes/npm-get-version-action@v1.3.1
uses: martinbeentjes/npm-get-version-action@3cf273023a0dda27efcd3164bdfb51908dd46a5b # v1.3.1
- name: Parse changelog
id: parse-changelog
uses: schwma/parse-changelog-action@1c2b2005ccf594cc3a45d33c10af4ab924d3a1c5 # v1.2.0
Expand Down
36 changes: 18 additions & 18 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,61 +4,61 @@ on:
workflow_dispatch:
push:
branches: [main]
pull_request:
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')
Copy link
Copy Markdown
Contributor

@hyperspace-insights hyperspace-insights Bot May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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

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 }}
Comment on lines +7 to 37
Copy link
Copy Markdown
Contributor

@hyperspace-insights hyperspace-insights Bot May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Keep pull_request for the test job (which doesn't need secrets) and only use pull_request_target for jobs requiring secrets, or
  2. Ensure the test job's needs: requires-approval + the if: 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

- name: Use Node.js ${{ matrix.node-version }}
uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0
uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6
with:
node-version: ${{ matrix.node-version }}
- run: npm i -g @sap/cds-dk@${{ matrix.cds-version }}
- run: npm i
- run: npm run build
- run: cd tests/bookshop && npm run build
- run: npm run test

hybrid-tests:
runs-on: ubuntu-latest
needs: requires-approval
if: always() && (needs.requires-approval.result == 'success' || needs.requires-approval.result == 'skipped')
name: Integration Tests
# Requires approval via config
environment: CI
strategy:
fail-fast: false
matrix:
node-version: [20.x, 22.x]
cds-version: [latest]
profile: [hana, hana-process]
steps:
- name: Harden Runner
uses: step-security/harden-runner@8d3c67de8e2fe68ef647c8db1e6a09f647780f40 # v2.19.0
with:
egress-policy: audit
- name: Checkout repository
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
with:
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
Copy link
Copy Markdown
Contributor

@hyperspace-insights hyperspace-insights Bot May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Suggested change
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

with:
CF_API: ${{ secrets.CF_API }}
CF_USERNAME: ${{ secrets.CF_USERNAME }}
Expand Down
Loading