ci(verifiability): verify our own public claims nightly, as an outsider - #9973
Conversation
![superagent-security[bot]](https://avatars.githubusercontent.com/in/3287076?s=60&v=4){kind=small}
|
Important 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏳ LoopOver is waiting…LoopOver has seen this pull request and is waiting on CI checks to finish before reviewing it. This comment will update once the review runs. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟨 Waiting |
|
On the red It reports "1 actionable risk across 2 changed dependencies". This PR changes no dependency files ( The scan did surface something real, though, and it is fixed in That leaves exactly one intentional unpinned fetch — |
Bundle ReportChanges will increase total bundle size by 1.55kB (0.02%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: loopover-uiAssets Changed:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9973 +/- ##
==========================================
+ Coverage 91.88% 95.67% +3.79%
==========================================
Files 928 828 -100
Lines 113722 63858 -49864
Branches 27436 22257 -5179
==========================================
- Hits 104489 61095 -43394
+ Misses 7936 1481 -6455
+ Partials 1297 1282 -15
Flags with carried forward coverage won't be shown. Click here to find out more. |
7970a20 to
be2330e
Compare
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| 🔵 In progress View logs |
loopover-ui | 85b1d68 | Jul 31 2026, 04:46 AM |
|
Both Superagent findings triaged. Security Scan (P2) — real, and fixed in Supply Chain (HIGH) — not actionable. It flags That is a test fixture exercising npmrc handling, not behaviour this workflow invokes. The pin is the SHA 31 other workflows in this repo already use, and this PR changes no dependency manifest — the "2 changed dependencies" are simply the two actions any new workflow must reference. There is nothing to remediate short of unpinning from the repo standard, which would be worse.
|
LoopOver tells a stranger they can check its fairness claims without asking permission. Nothing checked that this stayed true, so a regression in the public verification path was found by outside readers whenever they happened to look. The job runs the verifier PUBLISHED ON NPM against production, from a runner holding no credentials. Both parts are load-bearing. Building from the checkout would assert something weaker -- an outsider has the published CLI, so a fix merged but unpublished must not turn this green. #9962 is exactly that gap: the shipped verifier asked for `?rule_id=` while the route reads `ruleId`, every corpus fetch 400'd, and the tool called production unverifiable while production was fine. Both sides were individually tested and individually correct; only running the real pair catches it. The verify step is given no token at all -- `issues: write` lives on the step that files the tracking issue, after verification is done -- because a verifier running with repo credentials is not reproducing an outsider's view. Both surfaces are checked (#9940): `api.loopover.ai` for aggregates and eval-score records, `shots.loopover.ai` for the anchored ledger. One tracking issue, found by exact title: filed on failure, commented on while it persists, closed automatically on recovery. Comments rather than body rewrites, since which claim failed on which night is the useful history. Self-verified by pointing the real verifier at a deliberately wrong base URL, which found a false-green in the first draft of the decision logic. Every claim came back `skip` ("unavailable (HTTP 404)") and the CLI exited 0 -- judged claim-by-claim, four legitimate skips and a clean exit, so the monitor would have reported GREEN against a completely dead endpoint. The verifier is right to call them skips; from inside one claim, "disabled surface" and "wrong host" are indistinguishable. The monitor has the context the claim lacks, so a run that positively verified NOTHING is now red. The floor is one passing claim rather than a proportion because the two surfaces are legitimately lopsided: measured against production, the API passes three of four and the Orb passes one of four, and "most claims pass" would fail the Orb every healthy night. Three further false-greens are rejected: a run that produced no readable report (nothing failed because nothing ran), a non-zero exit with no failing claim, and an unreadable capture file. Skips alone never file an issue -- the API's ledger is empty by design and skips nightly -- but are always listed, since a new skip beside a failure is often its cause. The decision logic is a tested script rather than inline bash. That is also how an output-interleaving bug was caught: the workflow wrapping the script's stdout in a heredoc while the script appended its own scalars buried VERIFY_OK inside the body block, leaving the workflow reading an empty verdict and filing an outage on a healthy night. The script now writes all outputs itself, in order. Closes #9724
Drops the second unpinned registry fetch from the nightly job. The script is ours and already checked out, so pulling an unvetted TypeScript runner from npm to read it added supply-chain surface for nothing -- flagged by the supply-chain scan as an actionable risk across the two dependencies the workflow introduced. The remaining fetch, the published verifier itself, stays unpinned on purpose: resolving what an outsider actually gets is the property the job asserts. Same invocation `npm run actionlint` already uses for its own TS script.
Superagent flagged the contradiction, and it was right: actions/checkout defaults to persist-credentials: true, which writes an authenticated http.extraheader into .git/config. This job's entire claim is that it reproduces what an anonymous stranger sees, so withholding the token from the verify step's env while leaving a usable credential in .git/config is theatre -- anything the step runs could still pick it up. Nothing in the job needs it. The only step that talks to GitHub is the tracking-issue step, which uses gh with an explicit GITHUB_TOKEN in its own environment; gh reads that variable directly and never consults git's credential config.
17e212c to
54f8ced
Compare
|
Status: everything passes except The Security finding is fixed — it was real, and it was mine: The remaining Supply Chain failure is a false positive I do not think should be fixed in code: The cited path is a test fixture inside the action's own repository, exercising npmrc handling — not behaviour this workflow invokes. The pin is the SHA 31 other workflows here already use, and this PR changes no dependency manifest: the "2 changed dependencies" are simply I looked at removing the flagged dependency, and decided against itDropping But Options
Happy to do B if you would rather the board be clean; otherwise this is ready to merge as-is. |
…a comment
Adjudicating the supply-chain finding on actions/setup-node rather than waving
it through. Checked at the pinned SHA (v7.0.0):
• the action's entrypoint is `dist/setup/index.js`; the cited
`__tests__/authutil.test.ts` is a test fixture and is never executed;
• the credential writer is `auth.configAuthentication(registryUrl)`, and
`src/main.ts` calls it ONLY inside `if (registryUrl)`.
So the capability is real and unreachable here, because this workflow does not
set `registry-url`. That makes the safety a property of THIS FILE rather than of
the action -- which is the part worth acting on. Adding one innocuous-looking
line would make a job that claims to hold no credentials write an authenticated
.npmrc, and nothing would have noticed.
That is not hypothetical: the same class already shipped in this file and was
caught in review, when actions/checkout's `persist-credentials` default left a
usable token in .git/config while the job asserted it had none.
So the four credential paths are now asserted against the shipped workflow
instead of described in a comment -- the codebase's own standard for a snapshot
presented as a guarantee:
• checkout must set `persist-credentials: false` (asserted explicitly, not
absent-or-false, which would pass on the broken default);
• setup-node must not be given a `registry-url`;
• the verify step must have NO env at all -- not "no token", since an
allowlist of forbidden names misses the next secret someone adds;
• exactly one step may hold a token, and it must be the tracking-issue step;
• no step may run `npm ci`, which would verify against this checkout instead
of the published package.
Mutation-tested: restoring persist-credentials, adding registry-url, putting a
token on the verify step, and adding npm ci each fail. The suite also asserts it
found a verify job with steps, so it cannot pass vacuously against an empty
array if the file is renamed.
Adjudicated at the pinned SHA, and hardened where it actually matteredThe finding names a real capability that is unreachable in this workflow. Verified against
So there is no vulnerability to fix in the action or the pin. But the audit surfaced something that is worth fixing: the safety is a property of this file, not of the action. One innocuous line — That is not hypothetical. The same class already shipped in this very file and was caught in review: What changed (
|
| Guarantee | Mutation | Result |
|---|---|---|
checkout sets persist-credentials: false |
set it back to true |
1 failed |
setup-node has no registry-url |
add one | 1 failed |
| the verify step has no env at all | add a token | 2 failed |
no step runs npm ci |
add one | 1 failed |
Asserted explicitly rather than absent-or-false, because persist-credentials defaults to true and an absent-or-false check would pass on the broken default. And "no env at all" rather than "no token", because an allowlist of forbidden names misses the next secret someone adds. The suite also asserts it found a verify job with steps, so it cannot pass vacuously if the file is renamed.
The audit is recorded on the step itself, pointing at the test rather than asking the next reader to trust the comment.
The scan will still show red
Nothing here changes what it reports: it flags the two actions any new workflow must reference, and the HIGH is a test fixture in a third-party repo at the SHA 31 other workflows already pin. I stand by not removing setup-node — --experimental-strip-types needs Node ≥ 22.6, every workflow here that uses it pairs with setup-node, and making this nightly the first to depend on the runner image's Node version would trade a scanner false positive for a job that might silently stop running. Options A/B/C from my previous comment still stand; A remains my recommendation.
| { | ||
| echo "### Anonymous verification — ${{ steps.verify.outputs.VERIFY_SUMMARY }}" | ||
| echo "" | ||
| echo "- \`${{ env.API_BASE_URL }}\`" |
There was a problem hiding this comment.
P1: Template injection in Report verdict step via workflow_dispatch inputs
workflow_dispatch inputs are interpolated into a shell run block with backtick command substitution.
Use shell variables ($API_BASE_URL, $ORB_BASE_URL) instead of template substitution.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name=".github/workflows/verify-public-claims.yml">
<violation number="1" location=".github/workflows/verify-public-claims.yml:136">
<priority>P1</priority>
<title>Template injection in Report verdict step via workflow_dispatch inputs</title>
<evidence>The Report verdict step interpolates ${{ env.API_BASE_URL }} and ${{ env.ORB_BASE_URL }} directly into a shell script. These values derive from ${{ inputs.api-base-url }} and ${{ inputs.orb-base-url }}, which are workflow_dispatch inputs. The surrounding backticks create a bash command-substitution context, allowing arbitrary command execution when a malicious input is provided.</evidence>
<recommendation>Replace the template substitutions with shell variable references ($API_BASE_URL, $ORB_BASE_URL). These environment variables are already set at the job level, so this simply removes the template injection vector.</recommendation>
</violation>
</file>
…write credentials Resolves the P1 supply-chain finding by removing the dependency rather than arguing with it. The finding was accurate about the capability and, as far as this workflow goes, unreachable: `actions/setup-node` writes an authenticated .npmrc through `auth.configAuthentication(registryUrl)`, and at the pinned SHA (v7.0.0) `src/main.ts` calls it only inside `if (registryUrl)` -- an input this job never set. The cited `__tests__/authutil.test.ts` is a test fixture; the action's entrypoint is `dist/setup/index.js`. But "unreachable because nobody has added one line yet" is a weaker property than "not present", and for a job whose entire claim is that it holds no credentials, not present is the one worth having. This job needs a Node RUNTIME, not the repository's toolchain: it runs a published CLI through npx and one checked-in script, and builds nothing. The runner image already provides node and npm, and ci.yml, mcp-release-watch.yml and package-release-watch.yml already rely on that. The tradeoff is that the Node version is the runner's rather than .nvmrc's, so it is ASSERTED instead of assumed. `--experimental-strip-types` needs 22.6+, and a precondition step fails immediately with the remedy in the message -- re-add actions/setup-node with node-version-file -- if an image ever regresses below that. A nightly that breaks quietly is the failure mode worth spending a step on. checkout is now the only third-party action in the job, and the tests assert that as an ALLOWLIST rather than a blocklist of known-risky ones: a new action arriving here should be a deliberate decision, not something a scanner notices later. Also pins the dry-run gate raised alongside the persist-credentials finding. The disk half is closed by `persist-credentials: false`; this asserts the other half, so the gate cannot be dropped while the token-bearing step remains. Mutation-tested: re-adding setup-node fails 3 assertions, and removing the dry-run gate fails its own.
85b1d68 to
7c3607b
Compare
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
1 participant
What this is
A scheduled job that is the stranger the docs invite. It runs the verifier published on npm against production, from a runner holding no credentials of any kind, and turns a regression in the public verification path into our problem within a cycle instead of an outside reader's discovery.
Both design choices are load-bearing:
npx -p @loopover/mcpresolves the npm release rather than building from the checkout. What is asserted is "an outsider succeeds today", and an outsider has the published CLI — so a fix that is merged but unpublished must not turn this green. fairness: a corpusChecksum is published for a rule whose corpus is not downloadable #9962 was exactly that: the shipped verifier asked/v1/public/eval-corpus?rule_id=while the route readsruleId, every corpus fetch 400'd, and the tool reported production as unverifiable while production was fine. Both sides were individually tested and individually correct. Only running the real pair catches it.contents: read;issues: writeand the token exist only on the step that files the tracking issue, after verification finishes. A verifier running with repo credentials is not reproducing an outsider's view and could pass on access a stranger does not have.Both surfaces are checked (#9940):
api.loopover.aifor aggregates and eval-score records,shots.loopover.aifor the anchored ledger. Checking one leaves the other free to regress silently.Self-verification found a false-green
Per the agreed approach, I pointed the real verifier at a deliberately wrong base URL rather than staging a broken config. It found a bug in my first draft of the decision logic:
Four legitimate skips and a clean exit. Judged claim-by-claim that looks entirely healthy — the monitor would have reported green against a completely dead endpoint, which is the precise failure this job exists to prevent.
The verifier is right to call them skips: from inside a single claim, "this surface is disabled" and "this host is wrong" are indistinguishable. The monitor has the context the claim lacks — production is supposed to be serving these — so a run that positively verified nothing is now red.
The floor is one passing claim, not a proportion, because the two surfaces are legitimately lopsided. Measured against production right now:
api.loopover.aishots.loopover.ai"Most claims pass" would fail the Orb every healthy night.
End-to-end result
VERIFY_OK=true— 4 passed, 0 failed, 4 skippedVERIFY_OK=false— names which surface is dead; the healthy one still passesRestoring the URL goes green, which is what closes the tracking issue — the recovery half of the acceptance criterion.
Other false-greens rejected
Skips alone never file an issue — the API's ledger skips nightly by design, and a monitor that files an outage on day one is muted by week one — but they are always listed, since a new skip beside a failure is often its cause.
Tracking issue lifecycle
One issue, found by exact title: filed on failure, commented on while it persists, closed automatically on recovery. Comments rather than body rewrites, because which claim failed on which night is the useful history. The filing step runs before the step that fails the run, so a red job always leaves the issue behind — a monitor that notices a problem and then drops it is worse than none.
Why the logic is a script
scripts/verify-public-claims-report.tsis pure and unit-tested (30 tests), because the judgement of what counts as a regression must not be discovered to be wrong on the night it first fires. It also caught an output-interleaving bug: the workflow wrapping the script's stdout in a heredoc while the script separately appended its scalars buriedVERIFY_OKinside the body block — the workflow would have read an empty verdict, which is not"true", and filed an outage on a healthy night. The script now writes all outputs itself, in one ordered write, and refuses to emit a body containing the delimiter.Docs
A status line and badge on the walkthrough page, which is the surface the issue asks for: green means an outsider succeeded within 24 hours, red means we already know and there is an issue naming the broken claim. The badge links to every run including the failures.
Not included — needs your call
workflow_dispatchtakesapi-base-url,orb-base-urlanddry-runinputs, so the job can be pointed anywhere on demand.PagerDuty is not wired. The issue asks to notify "via the existing alerting path (PagerDuty)", but that path is Alertmanager on the Orb — there is no PagerDuty secret in GitHub Actions, and no workflow references one. Adding it means introducing a new repository secret, which is an operator decision rather than something to slip into this PR. The tracking issue plus the red run is the notification today. Happy to add the routing step as a follow-up if you want the secret provisioned.
Verification
actionlint,lint:composite-actions,typecheck,docs:drift-check,dead-source-files:check,dead-exports:check,import-specifiers:check,coverage-boltons:check,fixture-clock-races:check,checkers-wired:check,validate:no-hand-written-js,ui:typecheck,ui:test— all greennpx vitest run test/unit— 1299 files, 25347 tests passedCloses #9724