feat(action): add sbom input so SPDX output is reachable from the GitHub Action - #1099
feat(action): add sbom input so SPDX output is reachable from the GitHub Action#1099Ayush7614 wants to merge 2 commits into
Conversation
…Hub Action Closes OWASP#1078. The CLI supports --sbom cyclonedx|spdx but the Action only exposed the legacy cdx boolean, so SPDX SBOMs were unreachable in CI. This adds a first-class sbom input (cyclonedx|spdx, case-insensitive, with cdx/spdx2.3 aliases) plus sbom-inventory-only, keeps cdx as a deprecated fallback (sbom wins when both are set), and fails fast on unknown formats instead of silently scanning without an SBOM.
sonukapoor
left a comment
There was a problem hiding this comment.
The design here is the right one and I want to lead with that, because the security-sensitive part is done better than the surrounding code.
The user's value never reaches argv. You normalise it in a case and only ever append hardcoded literals, so there is nothing to escape. I threw spdx; rm -rf /, $(touch ...), backticks, embedded newlines and a bare * at the composed block and every one hit the *) branch and exited 1. Compare --ca-cert and --fail-on, which pass raw values through. Yours is the better pattern and I would like new enum inputs to follow it.
Backward compatibility checks out too. I ran the matrix and cdx: "true" on its own composes exactly what it does on main, so pinned workflows are unaffected. Including sbom-inventory-only when the issue only listed it as a "consider" was a good call, and allowing it alongside cdx is correct.
Four things before it goes in.
1. echo swallows -n, which defeats the fail-fast guard. action.yml:309. Bash's echo builtin treats -n, -e and -E as options:
in=<-n> echo -> <> printf -> <-n>
in=<-e> echo -> <> printf -> <-e>
So sbom: "-n" normalises to empty, the -n "$sbom_input" test at line 310 is false, the invalid-value guard never runs, and it falls through to the cdx branch. The job then scans with no SBOM and no error, which is the exact outcome your PR description says it prevents. printf '%s' "${INPUT_SBOM:-}" fixes it. Not ${INPUT_SBOM,,} please, that needs bash 4 and GitHub's macOS runners are still on 3.2.
2. The Action accepts four formats the CLI rejects. The CLI's alias table is exactly cyclonedx, spdx, spdx2.3. Verified:
--sbom cdx -> Unknown --sbom format "cdx"
--sbom cyclone-dx -> Unknown --sbom format "cyclone-dx"
--sbom spdx-2.3 -> Unknown --sbom format "spdx-2.3"
--sbom spdx2.3 -> works
So sbom: cdx succeeds in CI and the same thing fails on someone's laptop. I would narrow the case to cyclonedx|spdx|spdx2.3. Also the error message lists only cyclonedx, spdx and drops spdx2.3, which we document as first-class.
3. website/docs/spdx.md:93-105 still says this feature does not exist. It currently tells people the action has no SPDX input and gives an npx workaround. That is the page the issue named specifically. It wants to become an sbom: spdx example.
4. "Deprecated" on the cdx input contradicts the rest of the codebase. action.yml:63 and github-action.md:43. args.ts calls it a permanent alias, the SPDX docs call it "still supported", and the issue says it must keep working unchanged. Calling it deprecated in the Marketplace description signals removal to exactly the pinned-workflow users we are protecting. "Alias for sbom: cyclonedx" would be truer.
Smaller things, take or leave: there are em dashes in action.yml:63 and :329, and line 329 prints into CI logs. The sbom-inventory-only description says it requires sbom, but the code accepts cdx too. And a value like " spdx " hard-fails on whitespace.
One thought on the tests rather than a request. They pin exact strings like sbom_input=$(echo "${INPUT_SBOM:-}", so fixing item 1 will break them with no behaviour change. None of them parse the YAML or run the shell, and we have no actionlint step, so action.yml has no real guard today. Executing the block with the env vars set and asserting the composed args array is how I found item 1, and it would be worth more than more indexOf assertions.
- Use printf instead of echo so -n/-e/-E hit the invalid-value guard - Trim surrounding whitespace before matching - Narrow accepted values to exactly the CLI formats (cyclonedx, spdx, spdx2.3); drop cdx/cyclone-dx/spdx-2.3 aliases the CLI rejects - List spdx2.3 in the invalid-value error message - Describe cdx as an alias for sbom: cyclonedx, not deprecated - Fix sbom-inventory-only description (requires sbom or cdx) and remove em dashes from action.yml - Rewrite spdx.md GitHub Actions section as sbom: spdx example - Extend tests: YAML parse guard, printf/alias assertions, and executed shell-block behaviour (-n/-e handling, trimming, precedence)
|
Thank you for the thorough review, @sonukapoor — the Addressed in f7b5d81:
Smaller items also taken: whitespace-trimming (padded values no longer hard-fail), On the tests: kept the
|
sonukapoor
left a comment
There was a problem hiding this comment.
All four are done, and I checked by executing the block rather than reading the diff.
Item 1 in particular: I ran the extracted shell on bash 3.2.57, which is what GitHub's macOS runners actually have. -n, -e, -E and -neE all hit the invalid branch and exit 1 now. I also ran the old version to confirm the bug was real rather than theoretical, and INPUT_SBOM=-n did compose an empty args array and pass silently. Nothing bash-4-only crept in, and the whitespace trim is 3.2-safe.
Item 2 I checked in both directions against the real parser. Every value the Action now accepts parses, and every value it rejects is also rejected by the CLI. The two sets match exactly, which was the point.
You also went past what I asked. The whitespace trim and the YAML-parse guard were not requested, and replacing the indexOf assertions with a block that actually runs the shell through execFileSync is the thing I was hinting at in the last paragraph last time. That is a real improvement to how this file is tested.
One thing to fix and then this goes in.
Two smaller notes, neither blocking. The executed script in the test is a hand-copy rather than extracted from action.yml, so the behavioural tests validate the copy and only the string assertions guard the real file. Worth revisiting sometime, not now. And the PR body is stale: it still calls cdx a deprecated fallback, still lists the cdx alias you removed, and says 7 tests where there are now 14. Since we squash-merge, that body becomes the commit message, so it is worth a quick edit before this lands.
| ## GitHub Actions integration | ||
|
|
||
| The `OWASP/cve-lite-cli` action does not expose an SPDX input yet; its `cdx` input covers CycloneDX only. Run the CLI directly for SPDX output: | ||
| Use the `sbom` input (first-class since v1.28). It accepts `cyclonedx`, `spdx`, or `spdx2.3` (case-insensitive) and takes precedence over `cdx`: |
There was a problem hiding this comment.
This version number is wrong, and it fails in the one way this PR exists to prevent.
--sbom shipped in v1.34.0. This Action input is unreleased, so v1.35.0 at the earliest. The cdx input shipped in v1.16.0. So v1.28 matches nothing.
It matters because GitHub Actions silently ignores unknown with: inputs. Someone pinned to @v1.28 through @v1.34 reads this line, adds sbom: spdx, and gets a green scan with no SBOM and no error. That is exactly the silent-failure mode you fixed in item 1, reintroduced through the docs.
Either drop the parenthetical or say "available from v1.35.0".
Closes #1078.
Summary
The CLI supports
--sbom cyclonedx|spdxbut the GitHub Action only exposed the legacycdxboolean, so SPDX 2.3 SBOMs were unreachable in CI.Changes
sbominput (cyclonedx|spdx, case-insensitive,cdx/spdx2.3aliases). Takes precedence overcdx.sbom-inventory-onlyinput wired to--sbom-inventory-only(warns when no SBOM output is enabled).cdxkept as deprecated fallback (elif), so existing workflows keep working and setting both does not emit two SBOM flags.sbomvalues fail fast with::error::instead of silently scanning without an SBOM.website/docs/github-action.md.Verification
tests/github-action.test.ts: 7 tests pass (new: sbom wiring, cdx fallback precedence, invalid-format rejection, inventory-only, alias normalization).npm run buildpasses;action.ymlparses as valid YAML.