feat: compare paired same-runner baselines - #3
Conversation
There was a problem hiding this comment.
Review: same-runner paired baseline
Solid, well-tested feature — the baseline path mirrors the result path, validation is reused, and the report changes correctly thread the vs base/vs main provenance through to the rendered comment. README and action.yml docs match the implementation. One security finding worth addressing before merge, plus a couple of minor notes.
Security (please address)
Baseline source.url / source.sha are rendered into the privileged PR comment without identity pinning.
The trust model (.github/workflows/publish.yml) is: record runs in the untrusted CI context and emits artifacts (result.json + the new baseline.json), while render/publish runs on workflow_run with pull-requests: write and posts the bot PR comment. For the primary result, bindSource (src/commands.js:262) re-pins identity against expected-source-repository/expected-source-sha and overrides source.url with the trusted source-url from publish.yml.
The baseline gets none of this. bindSource is only called on loaded.results (src/commands.js:315); pairedBaseline comes straight from the untrusted baseline.json via entryFromResults(loaded.baselines, ...) (src/commands.js:322-326), and src/report.js:71 renders its source.url into a markdown link and source.sha.slice(0,12) as the link text. validURL (src/artifact.js:31) allows any https host, so a fork PR author can make the "base commit" link in the authoritative bot comment point to an arbitrary URL and display a fabricated SHA — a spoofing/phishing primitive in a trusted-context comment. (Markdown breakout itself is blocked: the URL sits in a <...> destination and validURL forbids <> and CR/LF, so impact is a misleading link/label rather than arbitrary HTML.)
Suggested fix: pin the baseline the same way the primary result is pinned — apply a bindSource-equivalent to loaded.baselines (e.g. expected-baseline-* / a trusted baseline-source-url), or at minimum constrain the rendered baseline URL host to GITHUB_SERVER_URL and the repository to the known base before rendering.
Minor
- Baseline
runUrl/timestampcopied from the current run (src/commands.js:200-203): the baselinesourcereuses the current run'srunUrlandtimestamp. This may be intentional for the same-runner model (this run measured the baseline), but it is easy to misread later.timestampcurrently only affectsmergeShardstie-breaking;runUrlis not surfaced in the report. A one-line comment recording the intent would prevent a future "fix" from breaking it. bindSourceintentionally does not re-bind baselines (src/commands.js:315): assuming the security finding above is resolved by explicitly pinning baselines, add a short comment documenting whichever invariant you land on, so the asymmetry between results and baselines is not mistaken for an oversight.
| ); | ||
| let comparisonNote; | ||
| if (baseline && sameRunner) { | ||
| comparisonNote = `_Compared with [\`${baseline.source.sha.slice(0, 12)}\`](<${baseline.source.url}>) measured in the same runner job._`; |
There was a problem hiding this comment.
The baseline's source.url and source.sha rendered here come straight from the untrusted baseline.json artifact — unlike the primary result, the baseline is never passed through bindSource (src/commands.js:315), so its identity is not pinned against expected-source-* and its URL is not replaced with a trusted value.
validURL (src/artifact.js:31) allows any https host, so a fork PR author can make this "base commit" link in the privileged bot comment point to an arbitrary URL and display a fabricated 12-hex SHA. Recommend pinning the baseline source (apply a bindSource-equivalent to loaded.baselines, e.g. expected-baseline-* / a trusted baseline-source-url) or constraining the URL host to GITHUB_SERVER_URL / the known base repository before rendering.
|
Addressed the baseline identity finding in b8112c9:
Also changed the reusable workflow to invoke |
f99a49d to
ba2187a
Compare
|
Follow-up in ba2187a: I removed the live exact-base-SHA lookup after checking the queued-workflow case. The target branch can advance between measurement and trusted publishing, so comparing against the PR API's current base SHA would reject a valid paired artifact. The final publisher follows the review's minimum safe boundary: it requires the artifact baseline repository to equal the PR target repository and always reconstructs the commit URL on |
Summary
vs baselabelThis lets a caller benchmark its base and head in one runner job, avoiding cross-machine variance without coupling the action to repository-specific benchmark commands.
Validation
npm run checknpm audit --omit=devnpm run builddistandpublish/distupdated