Skip to content

fix(ci): prevent shell injection via milestone title in code freeze workflows#5067

Open
kakkoyun wants to merge 1 commit into
mainfrom
fix/code-freeze-shell-injection
Open

fix(ci): prevent shell injection via milestone title in code freeze workflows#5067
kakkoyun wants to merge 1 commit into
mainfrom
fix/code-freeze-shell-injection

Conversation

@kakkoyun

Copy link
Copy Markdown
Member

What

The code freeze workflows passed the milestone title straight into a run: shell block via ${{ github.event.milestone.title }} (and, on start, the workflow_dispatch title input). GitHub Actions expands ${{ }} before bash sees the script, so a crafted milestone title runs as shell on the runner.

This routes those values through env: and reads them back as shell variables, so bash treats them as data. It is the same indirection already used for PR titles in pull-request-title-validation.yml.

Why it matters

Creating a milestone needs only the Triage role. The freeze job runs with id-token: write after dd-octo-sts mints a privileged token, so an injected command would run with that token in scope and could forge the code-freeze status check to bypass the merge gate. Low severity: insider-only, no external-attacker path. Still a privilege boundary worth closing.

Tracking: APMSP-3535. Introduced by #4276.

Test

  • make lint/action (actionlint) passes on the full workflow set.
  • No attacker-controllable ${{ github.event.* }} remains inside a run: block in either file.
  • The github.event_name / github.repository references left in the unrelated check_remaining step are trusted GitHub contexts (fixed event enum / owner/repo slug), not attacker input.
  • ${{ }} in the if: guards is evaluated by the Actions expression engine, not bash, so it is not an injection vector.

Note: actionlint has no script-injection rule (that is zizmor's domain), so the lint pass confirms workflow validity, not the security property. The fix relies on the env:-indirection pattern; zizmor .github/workflows/ can be run in CI to detect this class going forward.

…orkflows

The code freeze workflows interpolated the milestone title expression
directly inside their run: shell blocks. GitHub Actions substitutes
${{ }} expressions before handing the script to bash, so a milestone
title like `Code Freeze"; <payload> #` would execute on the runner. Any
user with the Triage role can create such a milestone. Because the job
runs after dd-octo-sts mints a privileged token (id-token: write), this
is an insider privilege-escalation path that bypasses branch protection.
The startsWith() guard only checks the prefix, so the rest of the title
stays attacker-controlled.

Route the untrusted values through env: and reference shell variables in
run:, so bash expands them at runtime as data instead of parsing them as
script. This matches the indirection already used for PR titles.

Introduced by #4276. Fixes APMSP-3535.
@kakkoyun
kakkoyun marked this pull request as ready for review July 24, 2026 11:15
@kakkoyun
kakkoyun requested a review from a team as a code owner July 24, 2026 11:15
@kakkoyun

Copy link
Copy Markdown
Member Author

@codex review for security regressions

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Breezy!

Reviewed commit: 7a89ed017a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@datadog-datadog-us1-prod

datadog-datadog-us1-prod Bot commented Jul 24, 2026

Copy link
Copy Markdown

Tests

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 62.83% (+11.41%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 7a89ed0 | Docs | Datadog PR Page | Give us feedback!

@pr-commenter

pr-commenter Bot commented Jul 24, 2026

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-07-24 11:40:49

Comparing candidate commit 7a89ed0 in PR branch fix/code-freeze-shell-injection with baseline commit 345341d in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 326 metrics, 0 unstable metrics, 1 flaky benchmarks without significant changes.

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

  • 🟩 = significantly better candidate vs. baseline
  • 🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

Known flaky benchmarks

These benchmarks are marked as flaky and will not trigger a failure. Modify FLAKY_BENCHMARKS_REGEX to control which benchmarks are marked as flaky.

Known flaky benchmarks without significant changes:

  • scenario:BenchmarkOTLPTraceWriterFlush

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant