Skip to content

[security] Harden workflows against script injection#1812

Merged
nebasuke merged 6 commits into
mainfrom
ci/bencher-project-env-indirection
Jun 5, 2026
Merged

[security] Harden workflows against script injection#1812
nebasuke merged 6 commits into
mainfrom
ci/bencher-project-env-indirection

Conversation

@nebasuke

@nebasuke nebasuke commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Small security fixes to not allow injection of bencherProject and check_binder variables. Relatively low risk, but easy fix.

@changeset-bot

changeset-bot Bot commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: c33f277

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@nebasuke nebasuke requested a review from Copilot June 1, 2026 10:32
Copilot AI reviewed Jun 1, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the benchmark GitHub Actions workflows by reducing the risk of shell script injection when propagating the bencherProject workflow input into the SLANG_BENCHER_PROJECT environment variable used by the infra bencher integration.

Changes:

  • Pass inputs.bencherProject via a step env: entry instead of directly interpolating it into the shell script.
  • Quote the $GITHUB_ENV path when appending SLANG_BENCHER_PROJECT.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
.github/workflows/benchmark_npm.yml Moves bencherProject into step env and writes SLANG_BENCHER_PROJECT into $GITHUB_ENV for npm benchmarks.
.github/workflows/benchmark_cargo_slang.yml Same hardening for the cargo slang benchmark workflow.
.github/workflows/benchmark_cargo_slang_v2.yml Same hardening for the cargo slang-v2 benchmark workflow.
.github/workflows/benchmark_cargo_cmp.yml Same hardening for the cargo comparison benchmark workflow.

Comment thread .github/workflows/benchmark_npm.yml Outdated
Comment thread .github/workflows/benchmark_cargo_slang.yml Outdated
Comment thread .github/workflows/benchmark_cargo_slang_v2.yml Outdated
Comment thread .github/workflows/benchmark_cargo_cmp.yml Outdated
@nebasuke nebasuke changed the title [security] Harden bencherProject input against script injection in benchmark workflows [security] Harden workflows against script injection Jun 1, 2026
@nebasuke nebasuke added the ci:perf Runs performance test dry-runs in a PR (rather than the smoke-tests) label Jun 1, 2026
@github-actions

github-actions Bot commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

Branchci/bencher-project-env-indirection
Testbedci

⚠️ WARNING: Truncated view!

The full continuous benchmarking report exceeds the maximum length allowed on this platform.

🚨 4 Alerts

🐰 View full continuous benchmarking report in Bencher

@github-actions

github-actions Bot commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

Branchci/bencher-project-env-indirection
Testbedci

⚠️ WARNING: Truncated view!

The full continuous benchmarking report exceeds the maximum length allowed on this platform.

⚠️ WARNING: No Threshold found!

Without a Threshold, no Alerts will ever be generated.

🚨 6 Alerts

🐰 View full continuous benchmarking report in Bencher

@github-actions

github-actions Bot commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

Branchci/bencher-project-env-indirection
Testbedci

⚠️ WARNING: Truncated view!

The full continuous benchmarking report exceeds the maximum length allowed on this platform.

⚠️ WARNING: No Threshold found!

Without a Threshold, no Alerts will ever be generated.

🐰 View full continuous benchmarking report in Bencher

@github-actions

github-actions Bot commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

Branchci/bencher-project-env-indirection
Testbedci

⚠️ WARNING: Truncated view!

The full continuous benchmarking report exceeds the maximum length allowed on this platform.

⚠️ WARNING: No Threshold found!

Without a Threshold, no Alerts will ever be generated.

🐰 View full continuous benchmarking report in Bencher

@nebasuke nebasuke requested a review from Copilot June 1, 2026 15:08
Copilot AI reviewed Jun 1, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Comment thread crates/solidity/testing/sourcify/src/command.rs Outdated
@nebasuke nebasuke marked this pull request as ready for review June 1, 2026 19:09
@nebasuke nebasuke requested review from a team as code owners June 1, 2026 19:09
teofr
teofr approved these changes Jun 5, 2026
View reviewed changes

@teofr teofr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@nebasuke nebasuke force-pushed the ci/bencher-project-env-indirection branch from 039925e to d72d315 Compare June 5, 2026 14:19
@nebasuke nebasuke removed the ci:perf Runs performance test dry-runs in a PR (rather than the smoke-tests) label Jun 5, 2026
nebasuke added 6 commits June 5, 2026 17:57
@nebasuke
Harden bencherProject input against script injection in benchmark wor…
c74eed0 
…kflows
@nebasuke
Harden check_binder input against script injection in sourcify workflow
90ae864
@nebasuke
Pass bencherProject via step env instead of $GITHUB_ENV
018d9bb 
Closes the residual env-file injection vector (a newline in the value
could append extra env entries) Copilot flagged on the echo-based
approach. Step env is set directly in the process, never parsed as a
shell or env-file. Infra now treats an empty SLANG_BENCHER_PROJECT as
unset so the built-in default still applies on non-dispatch runs.
@nebasuke
Add 'none' to check_binder CLI so the sourcify workflow can drop the …
d80e193 
…shell conditional

Mirrors the bencherProject change: instead of a bash array + if to
omit '--check-binder' when 'none', the binary now accepts
'--check-binder=none' (a new CheckBinderMode::None variant, the
default). The workflow passes the value as a quoted env var, so there's
no injection surface and no shell logic. Also aligns the CLI values with
the workflow_dispatch choice options (none/v1/v2/compare).
@nebasuke
Harden sourcify matrix aggregation against shell injection
6af269a 
`echo '<json>' > file` would break out of the single quotes on any
single quote in the aggregated shard results. Pass the JSON via env and
printf it instead. (Results are first-party, so not exploitable, but
it's the same class this PR hardens.)
@nebasuke
Use typed clap default for check_binder (default_value_t)
c33f277 
Per review: default_value = "none" is a runtime-parsed string that would
silently drift if the CheckBinderMode::None variant is renamed. Switch to
default_value_t = CheckBinderMode::None so the compiler keeps it in sync.
@nebasuke nebasuke force-pushed the ci/bencher-project-env-indirection branch from d72d315 to c33f277 Compare June 5, 2026 17:59
@nebasuke nebasuke added this pull request to the merge queue Jun 5, 2026
Merged via the queue into main with commit 6ac0964 Jun 5, 2026
20 of 21 checks passed
@nebasuke nebasuke deleted the ci/bencher-project-env-indirection branch June 5, 2026 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@teofr teofr teofr approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nebasuke @teofr