contract_sha is checked for shape but not for provenance, and cd-apply runs what it points at on self-hosted hardware with id-token: write

[rldyourmnd]

Seven actions/untrusted-checkout findings arrived with code scanning (#6). Six of them are low-stakes. The one in cd-apply.yml is not, and it is worth separating them.

The chain

cd-apply.yml validates its input carefully:

[[ "$CONTRACT_SHA" =~ ^[0-9a-f]{40}$ ]]

That constrains the shape of the SHA. It does not constrain where the SHA came from. The next step checks out that ref and the step after executes code from it:

- uses: actions/checkout@…
  with:
    repository: NDDev-OpenNetwork/cd-workflows
    ref: ${{ inputs.contract_sha }}
    path: _cd
- uses: ./_cd/.github/actions/lifecycle      # executes the checked-out code

Every commit pushed to a pull request — including from a fork — is reachable in the base repository's object store. refs/pull/N/head is fetchable, and a bare SHA resolves regardless of which branch, if any, contains it. So "a valid 40-hex SHA in this repository" includes commits nobody reviewed and nobody merged.

Why it matters here specifically

	cd-evidence, cd-plan, cd-verify, cd-resume, cd-rollback	cd-apply
runner	ubuntu-latest (hosted, disposable)	[self-hosted, cd-apply-out-of-band]
permissions	contents: read	contents: read, id-token: write
environment	none	cd-apply

So for cd-apply the consequence of pointing contract_sha at unreviewed code is execution on the estate's own hardware, with the ability to mint OIDC tokens. That is the exact risk ci-workflows/scripts/_runners.py was written to prevent for public repositories, arriving through a different door.

persist-credentials: false is set on both checkouts and is doing real work — no git token is left on disk. It does not help against the executed code itself.

The environment provides no gate today

environment: cd-apply reads like an approval boundary. It is not one yet:

GET /repos/NDDev-OpenNetwork/cd-workflows/environments  ->  (empty)
GET /repos/NDDev-OpenNetwork/cd-workflows/environments/cd-apply  ->  404

The repository has no environments at all. GitHub creates one implicitly the first time a job references it, with no protection rules and no required reviewers. So the line that looks like the control is currently a label.

What would actually close it

Validating provenance rather than shape. In rough order of strength:

1. Require the SHA to be an ancestor of main. One git merge-base --is-ancestor against a shallow-fetched main, before the second checkout. Cheap, and it excludes every unmerged commit by construction.
2. Require a signed tag and resolve the SHA from it, so the contract version is something someone deliberately published.
3. Pin the contract in the reusable rather than accepting it as an input, and let callers upgrade by bumping their uses: pin — which is reviewable in their diff.

And independently of the choice: create the cd-apply environment with required reviewers, so the declaration means what it reads as.

Scope

Filed rather than fixed. Which of the three is right is a decision about what contract_sha is for — if callers are meant to pin an arbitrary contract revision, (1) narrows that deliberately; if they are meant to track releases, (2) or (3) is closer to the intent. That is not a mechanical edit.

The two actions/code-injection findings from the same scan were mechanical and are fixed in #7.

[rldyourmnd]

The control suggested as option (1) already exists in the estate, in github-actions, which is worth knowing before choosing between the three.

github-actions/.github/workflows/ci.yml checks out a ref taken from its own release descriptor and CodeQL raised actions/untrusted-checkout there too. It is a false positive, because the step immediately before the checkout is:

- name: Refuse provider source drift after the release commit
  env:
    SOURCE_COMMIT: ${{ steps.release.outputs.source_commit }}
    HEAD_COMMIT: ${{ github.sha }}
  run: |
    git merge-base --is-ancestor "${SOURCE_COMMIT}" "${HEAD_COMMIT}"

One line, and it converts "a valid 40-hex SHA" into "a commit reachable from the branch we are on". A fork-PR commit is not reachable from main, so it is excluded by construction rather than by enumeration. That workflow then goes further and refuses the run if any provider source path changed between the two commits.

So for cd-apply the smallest correct change is the same ancestry assertion against main, added to the existing validation step next to the ^[0-9a-f]{40}$ test — the difference being that cd-workflows checks out a different repository, so it needs a shallow fetch of that repository's main to have something to compare against.

That does not settle whether contract_sha should remain caller-chosen at all, which is still the design question above. It does mean the estate already has a worked example of the guard, so option (1) is not new ground.

The github-actions alert has been dismissed with this reasoning recorded on it.