Repository navigation
action: one workflow runs any combination of scan, conformance and probe - #52
Merged
Merged
Conversation
…he reporting Three checks meant three workflow files, each shelling out to npx and picking the JSON apart with inline jq. That put the payload logic inside every user's repository, where we cannot fix it: the conformance ingest fix shipped last week only changed what NEW repos would generate, so every repo that had already committed the old jq kept reporting a never-executed run as ~35 high-severity crypto defects. One `checks` input replaces those files. The seven useful combinations are just subsets of three, so there is nothing to enumerate. It defaults to "scan", which is exactly what v1 did, so an existing workflow keeps working untouched. qProbe and Sieve are now imported rather than spawned. That deletes the shell layer entirely: no quoting, no `> file.json`, no re-parsing a report we already hold as a typed object, no --format flag to get wrong. The zero-dependency invariant still holds, since both are @quantakrypto/* workspace packages. Reporting moves into platform.ts, versioned and tested. A conformance run whose implementation could not be started is posted as `failed` rather than as a verdict, so it stays out of badges and the posture series, and its findings collapse to the one actionable item. That guard reads counts.pass === 0 as well as the ERROR verdict, so it holds against Sieve releases that predate it. One audit run id belongs to one check: a workflow running all three still gets a single dispatch naming one of them, and only that check may post against the id. Otherwise the last result would overwrite the one the dashboard asked for. Probe targets are normalised to a bare host here too, so the fix holds however the workflow was authored, and credentials in a pasted URL never reach an ownership attestation.
The action ships metadata twice: action.yml at the root so the Marketplace can list it, and packages/action/action.yml for the canonical uses: coordinate. Both run the same bundle, and nothing checked they agreed. A drifting pair is silent and nasty: an input added to one file works via one coordinate and is ignored via the other, so the check quietly does not do what the workflow says. Adding four inputs in the previous commit doubled the surface for exactly that. Compared on input/output names and defaults, not the whole file, because name, description and runs.main differ by design. Hand-rolled reader rather than a YAML dependency, since adding one to a supply-chain gate in a zero-dependency repo would be its own irony. The suite includes a guard that the parser read anything at all, so a checker that silently parses nothing cannot pass forever.
Five parallel audits ran against the unified action. These are the fixes.
Integrity, and the worst of them: the platform was being sent post-baseline
findings alongside a pre-baseline score, so a repository could baseline every
known-vulnerable finding and mint a "post-quantum ready" badge from an empty
list while its own score said 40. Baseline is a CI convenience ("do not fail my
build on debt I have logged"); the platform records posture, which is a
different question. It now receives the full pre-baseline findings, matching the
reasoning that already stops a baseline waiving a mandate deadline.
The probe path bypassed qProbe's own parseTarget, which is the enforcement point
the threat model calls code-enforced. It refuses CIDR blocks, ranges, wildcards,
lists, URLs and embedded credentials; my normaliser silently accepted all of
them. our-api.example.com@evil.test would have been reduced to evil.test and
probed under a manufactured ownership claim while a reviewer reading the
workflow saw our-api.example.com. Now parsed by the function that owns the rule,
which also restores host:port so SSH targets work again.
The ownership attestation is no longer manufactured. iOwnThis was hardcoded
true; the CLI makes an operator type --i-own-this precisely because that
affirmative act is the control. It is now a required input, refused at config
time, so the attestation is a git-blameable act by a named human.
resultUrl was unvalidated, which made it an SSRF primitive: firing a
repository_dispatch needs contents:write, which any collaborator has, so
whoever composes that payload chose where a token-bearing POST landed. Pinned to
https on our own origin, redirects refused (the token is in the body, so
undici's cross-origin header stripping does not protect it and a 307 re-sends
it intact), a 10s timeout so an unreachable endpoint cannot burn the job's
billed minutes, and only a repository_dispatch may carry a payload at all.
The callback token is now masked. We never printed it, but the event payload it
arrives in sits in plaintext on disk for the whole job, and echoing
client_payload is a common debugging line in a world-readable log.
Three silent-hang paths now report instead: a dispatch for a check the workflow
is not configured to run, a configuration error thrown before the dispatch
context is read, and a failed POST. Each of those left a run sitting at
"running" for an hour before going stale, with the reason visible only to
whoever opened the Actions log.
Also: the findings cap comment claimed to match a server limit that is actually
500, and scoredResult carried a parameter it never used.
Left deliberately unfixed and documented in place: conformance executes a
workflow-named implementation with write access to the workspace BEFORE the scan
reads it, so an implementation could rewrite the crypto it was meant to be
tested against and launder a clean verdict. Closing it needs the build gate
deferred to the end of the run, which is a larger change than this commit.
Three findings from the docs audit, all on this branch's own change.
The baseline input's description in both action.yml files promised something
the loader does not accept: 'a previously written quantakrypto report'. The
action loads only the {version, fingerprints} file that qscan --write-baseline
produces, so pointing it at a SARIF or JSON report loads zero fingerprints and
suppresses nothing - a silent no-op with a log warning as its only trace. The
package README already had this right; the action.yml, which is what the
Marketplace shows, contradicted it.
The action README documented 14 inputs while action.yml defines 19, so the
headline feature of this branch was invisible to anyone reading the README of
the thing that has it. Five rows added and the intro corrected: it no longer
claims the action only runs qScan.
And the CHANGELOG had no entry for the unified action at all - the largest
change on the branch, unrecorded.
From the documentation audit. Each of these is something a reader would copy. qprobe has no -o flag, so the documented `--cbom -o endpoints.cbom.json` had the filename parsed as a target host, which then failed the owned-hosts check. It redirects stdout instead. CONTRIBUTING told the reader to cd into a directory the clone does not create. The action writes SARIF; the workflow uploads it. Saying the action uploads it means a reader who omits the upload-sarif step gets nothing in the Security tab and no indication why. Composer/PHP support shipped in 0.6.0 and the dependency counts were never updated: eight ecosystems and 90 entries, not seven and 77. There is no `qscan list_rules` subcommand, so that instruction errored as a scan path. COMPLIANCE claimed 0.9.0 was unpublished and listed gaps its sibling doc says are closed, and called FIPS 205 conformance a gap when the slh-dsa category exists. Also: the suggested baseline filename implied a SARIF file, which is exactly the confusion the baseline input description was fixed for in the previous commit.
eslint's no-regex-spaces, which CI caught and my local run did not because I truncated the output to the last line and read past the error.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three checks meant three workflow files, each shelling out to
npxand turning the JSON into a result payload with inlinejq. That put our reporting logic inside repositories we do not control, so a bug in it could never be fixed for anyone who had already committed it.That is not hypothetical. The conformance ingest fix shipped last week only changed what NEW repos generate; every repo already carrying the old
jqstill reports a never-executed run as ~35 high-severity crypto defects.What changes
Seven useful combinations are subsets of three, so there is nothing to enumerate.
checksdefaults toscan, exactly what v1 did, so an existing workflow keeps working untouched (verified: the diff to the scan path is strictly additive).qProbe and Sieve are imported rather than spawned, which deletes the shell layer entirely. The zero-dependency invariant still holds, since both are
@quantakrypto/*workspace packages.What five parallel audits found, and what I changed
Fable audits ran over security, correctness, docs, flows and architecture. The serious ones were all mine:
parseTarget. My normaliser accepted what qProbe exists to refuse:our-api.example.com@evil.testwould have been reduced toevil.testand probed under a manufactured ownership claim while a reviewer readour-api.example.com. The threat model calls that control code-enforced; my code made that false. Now parsed by the function that owns the rule, which also restoreshost:portso SSH targets work.iOwnThiswas hardcoded true. The CLI makes an operator type--i-own-thisbecause that affirmative act is the control. Now a required input, refused at config time.resultUrlwas an unvalidated SSRF primitive (firing a dispatch needscontents:write, which any collaborator has). Pinned to https on our own origin, redirects refused (the token is in the body, so undici's cross-origin header stripping does not protect it and a 307 re-sends it intact), 10s timeout, and only arepository_dispatchmay carry a payload.client_payloadis a common debugging line.Known hazard, documented not hidden
conformanceexecutes a workflow-named implementation with write access to the workspace before the scan reads it, so an implementation could rewrite the crypto it was meant to be tested against and launder a clean verdict. Closing it needs the build gate deferred to the end of the run, which is larger than this PR. It is commented in place at the call site.Also here
A CI gate for the two
action.ymlfiles, which had no check that they agree — a drifting pair means an input works via oneuses:coordinate and is silently ignored via the other. Plus documentation fixes for commands that fail as written (qprobe -odoes not exist;cd quantakrypto-toolsis not what the clone creates) and counts that drifted (Composer shipped in 0.6.0 and was never added: eight ecosystems, 90 entries).Release ordering, important
The live
v1tag has only the old 14 inputs. Nothing may documentchecks:until the tag moves, or those inputs silently no-op with an unknown-input warning. The website half (unified template, dual-layout detection, dispatch) must land in the same release, or a repo adopting the unified workflow goes dark: the dashboard probes three specific paths and would never offer the Run button.Verified
npm run build,npm run lint,npm run format:check,npm testacross all 8 packages, 0 failures. 95 action tests.check-zero-deps,check-offline-boundaryand the newcheck-action-yml-syncall pass.dist/index.jsrebundled and committed.