fix(api-docs/rust): close template-injection on ref + pin cargo-doc-md#70
Conversation
H-1: route inputs.ref / github.ref_name through env: instead of
inlining via ${{ }}. Tag names can legally contain single quotes,
which would break out of the shell literal at template-expansion
time and run arbitrary code with the runner's checkout + secret
context. Env indirection keeps the value on the variable side of
the shell parser.
H-2: pin cargo-doc-md to 0.11.0. `cargo install --locked` only
locks transitive resolution — it does not pin the cargo-doc-md
package itself, so a future maintainer takeover or compromised
crates.io credentials would land arbitrary code in the runner.
The runner has read access to the source tree and DOCS_REPO_PR_TOKEN
in env, so this is a real supply-chain surface.
The same H-1 pattern exists in the sister templates (cpp, dotnet,
python, typescript) and should be fixed in follow-up PRs. They
were not changed here to keep this PR focused.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request improves the security of the Rust API documentation workflow by using environment variables to prevent shell injection and pinning the version of the cargo-doc-md tool. Review feedback identifies a potential environment variable injection vulnerability when writing user-controlled input to $GITHUB_ENV and suggests correcting an inaccuracy in the added comments regarding the scope of secret availability.
| env: | ||
| REF_RAW: ${{ inputs.ref || github.ref_name }} |
There was a problem hiding this comment.
While routing the ref through an environment variable successfully prevents shell injection in the run block, the subsequent logic that writes this value to $GITHUB_ENV (lines 61-63) remains vulnerable to environment variable injection. Since inputs.ref is user-controlled and can contain newlines, a malicious input could be used to set arbitrary environment variables for subsequent steps. Consider using a unique delimiter for the $GITHUB_ENV assignment or validating the input format.
| # The version is pinned because this binary runs with the | ||
| # repo's checkout in scope and the DOCS_REPO_PR_TOKEN secret in | ||
| # env. `--locked` alone only locks transitive resolution; it | ||
| # does NOT pin the cargo-doc-md package itself. Bumping | ||
| # requires reviewing the upstream release diff. |
There was a problem hiding this comment.
The comment states that DOCS_REPO_PR_TOKEN is in the environment (env), but this secret is only passed as an input to specific actions in later steps. It is not available in the environment of this cargo install step. While pinning the version is a good security practice, the comment should be updated for accuracy to avoid misleading future maintainers about secret exposure.
# The version is pinned because this binary runs with the
# repo's checkout in scope. Pinning prevents supply-chain
# attacks where a compromised package could attempt to
# exfiltrate secrets (like DOCS_REPO_PR_TOKEN) in later steps.
# --locked alone does NOT pin the package itself. Bumping
# requires reviewing the upstream release diff.…python/typescript (#71) Mirrors the rust-template fix (#70) across the remaining four sister templates. Each has the same `inputs.ref || github.ref_name` inlined into a single-quoted shell literal at template-expansion time. Tag names containing a single quote (Git permits them) would break out of the literal and run arbitrary shell with the runner's checkout and DOCS_REPO_PR_TOKEN in scope. Routes the value through a step-level env: REF_RAW: instead. Each template's existing slug-substitution rules are preserved (cpp uses \@/-, typescript uses \@/<empty>, dotnet/python only normalize /); only the source of $raw changes from interpolated literal to env var. DOCS_REF_NAME / DOCS_REF_SLUG outputs are byte-identical for every input ref. Behavior unchanged. Co-authored-by: Mike Odnis <mikeodnis3242004@gmail.com>
Summary
Two hardening fixes for
automation/source-repo-templates/api-docs.rust.yml. Both surfaced during a security review ofresq-software/crates/.github/workflows/api-docs.yml(the synced copy of this template).H-1 — Template-injection on
inputs.ref/github.ref_nameBefore:
The
${{ }}interpolation happens at workflow-template-expansion time, before shell parsing. A tag name containing a single quote (Git allows it — only space,~,^,:,?,*,[,\,..are forbidden ref chars) would terminate the literal and inject shell. The runner has the source-tree checkout andDOCS_REPO_PR_TOKENin env at that point.After:
Env indirection routes the value through the runner's env layer where shell metacharacters survive intact and cannot escape the variable boundary. This is the canonical zizmor
template-injectionremediation.H-2 —
cargo install cargo-doc-mdwas unpinnedBefore:
cargo install cargo-doc-md --locked--lockedonly locks the resolution graph for transitive dependencies. It does not pincargo-doc-mditself. Any future maintainer takeover or compromised crates.io credentials silently lands new code on the runner — which has read on the source tree andDOCS_REPO_PR_TOKENin env.After:
cargo install --locked --version 0.11.0 cargo-doc-mdPinned to the current latest stable. Bumping requires reviewing the upstream release diff.
Out of scope (follow-ups)
api-docs.cpp.yml,api-docs.dotnet.yml,api-docs.python.yml,api-docs.typescript.yml(lines 65-75 in each). Each should get its own focused PR. Not bundled here to keep the diff reviewable per-language.step-security/harden-runnereven though the org's reusable rust-ci workflow does. Adding it consistently across the templates is a separate, broader hardening change worth its own discussion.Effect on downstream
automation/sync-templates.shwill produce a sync PR inresq-software/crates(and only there — Rust template only ships there) on its next run. That sync PR will bring this fix into.github/workflows/api-docs.ymlautomatically.DOCS_REF_NAME/DOCS_REF_SLUGenv outputs are byte-identical for the same input ref.Test plan
actionlint .(run locally if you have it)raw=assignment + 1 changedcargo installline.automation/sync-templates.shor wait for the next cron) and confirm the resultingcratesPR contains the expected diff.resq-software/crates, fireworkflow_dispatchfor the synced workflow with a benign tag name to confirm no behavior regression.