frankenbit/release-toolkit

Fork 0

audit: security pass (token leaks, injection vectors, trust model, sed-rewrite hardening) #156

New issue

Closed

opened 2026-06-27 01:56:27 +02:00 by quartermaster · 1 comment

quartermaster commented

2026-06-27 01:56:27 +02:00

Owner

Why now

Pre-1.0 readiness: the toolkit publishes to releases via API, manipulates tokens, runs user-controlled inputs (commit messages, PR titles, version strings) through shell, and #148's planned mechanism introduces sed-based YAML rewrites. One deliberate security pass before locking in the 1.0 schema is appropriate.

Audit surfaces

Token handling

Echo statements + log output: scan all scripts for echo / printf / log() lines that interpolate $GITHUB_TOKEN / $RELEASE_TOOLKIT_TOKEN / $FORGEJO_TOKEN_* / similar. The fail-loud paths are a common forget-mode site (a stack trace shows the env).
Error paths: when an API call fails, what gets logged? Curl error output can include Authorization: Bearer <token> headers if -v is used. Audit forgejo-api.sh specifically.
Token-fallback patterns: the ${RELEASE_TOKEN_OVERRIDE:-$GITHUB_TOKEN_DEFAULT} shape appears in multiple places (ADR-0007); confirm no fallback path silently swaps to a more-privileged token.
Tracker comment posts: the probe today posted to a tracker comment using the GITHUB_TOKEN — confirm the toolkit's own scripts don't post secrets-bearing content to public issues/comments.

Bash injection vectors

User-controlled inputs reaching shell:

Commit messages (parsed by conventional-commits.sh) — could contain backticks / $(...) if reached unquoted
PR titles + bodies (parsed for size labels + bump labels) — same risk
Version strings (parsed by semver.sh) — $NEXT_VERSION reaches sed in #148's planned mechanism
Branch names + tag names (interpolated into shell commands)
Manifest JSON values (parsed by jq, then potentially interpolated)

Audit each user-input → shell-interpolation path for proper quoting.

post_bump_hooks trust model

release-toolkit.yml's post_bump_hooks: lets a consumer specify arbitrary shell commands that run during the cut. Documented trust model:

The consumer trusts their OWN config (they wrote it)
The toolkit doesn't sandbox the hooks

Audit:

Are the docs explicit enough that a fresh adopter doesn't mistakenly think hooks are sandboxed?
Could a malicious release-toolkit.yml injected via a PR (where the workflow runs against the PR's content) execute hooks?
The pull_request trigger's permissions vs push: main permissions — are they aligned with the trust model?

sed-based YAML rewrites (#148)

#148's planned mechanism uses sed to rewrite the ref: value in _release.yml:

sed -i "s|ref: '[^']*'  # release-toolkit-build-ref|ref: '$NEXT_VERSION'  # release-toolkit-build-ref|"

Audit:

What if $NEXT_VERSION contains | (sed delimiter)? ' (string boundary)? Shell metacharacters?
Constraint: $NEXT_VERSION comes from semver-derived strings; should be v[0-9.]+(-rc.[0-9]+)?. Confirm the sanitization is enforced + documented.
If the sanitization layer is bypassed (someone injects $NEXT_VERSION=...|;rm -rf /;|... via env), what's the blast radius?

Consumer config trust model

release-toolkit.yml is read by the toolkit's scripts. Confirm:

The config parser (config.sh) doesn't eval or source consumer-provided values
Field values like version_files: [...] are validated (path traversal? glob expansion?)
release_type enum is validated (no fall-through to arbitrary string)

Composition

#148 build-bake refactor: the sed-rewrite mechanism is a NEW security surface; this audit pass should run BEFORE or alongside #148's implementation, not after
#149/#150 Unicode cleanups: not security-relevant by themselves but the Unicode characters won't appear in interpolation-sensitive paths after cleanup; aligned with this audit's scope
#152 examples: the audit findings should inform what the examples DO and DON'T do (e.g., explicit "untrusted PR config" warnings)

Implementation surface

Audit-only PR (no code changes) producing:

docs/security.md summarizing the audit findings
A bats test seam for any "this MUST be sanitized" invariant
Issues filed for each substantive vulnerability found (if any)
A simple SECURITY.md at repo root with reporting instructions

If vulnerabilities are found: tracker-each + remediate in follow-up PRs (don't bundle the audit doc with fixes).

Estimated scope: 1-2 days of focused work + however many follow-up trackers materialize.

What this PR does NOT do

Does NOT fix vulnerabilities inline — audit reports findings; remediation is per-finding follow-ups
Does NOT add sandboxing to post_bump_hooks — that's a substrate-feature decision, not audit scope
Does NOT change the consumer-config trust model — captures it explicitly; changes are post-audit decisions
Does NOT cover supply-chain (Forgejo Actions runner integrity, etc.) — out of release-toolkit scope; that's alcatraz-infra concern

Refs

Operator engagement 2026-06-27: triple-audit suite recommendation accepted
Composition: #148 (build-bake sed-rewrite), #149/#150 (Unicode cleanups), #152 (examples)
Related: ADR-0007 (token-override + bash-level fallback discipline)

## Why now Pre-1.0 readiness: the toolkit publishes to releases via API, manipulates tokens, runs user-controlled inputs (commit messages, PR titles, version strings) through shell, and `#148`'s planned mechanism introduces sed-based YAML rewrites. One deliberate security pass before locking in the 1.0 schema is appropriate. ## Audit surfaces ### Token handling - **Echo statements + log output**: scan all scripts for `echo` / `printf` / `log()` lines that interpolate `$GITHUB_TOKEN` / `$RELEASE_TOOLKIT_TOKEN` / `$FORGEJO_TOKEN_*` / similar. The fail-loud paths are a common forget-mode site (a stack trace shows the env). - **Error paths**: when an API call fails, what gets logged? Curl error output can include `Authorization: Bearer <token>` headers if `-v` is used. Audit `forgejo-api.sh` specifically. - **Token-fallback patterns**: the `${RELEASE_TOKEN_OVERRIDE:-$GITHUB_TOKEN_DEFAULT}` shape appears in multiple places (ADR-0007); confirm no fallback path silently swaps to a more-privileged token. - **Tracker comment posts**: the probe today posted to a tracker comment using the GITHUB_TOKEN — confirm the toolkit's own scripts don't post secrets-bearing content to public issues/comments. ### Bash injection vectors User-controlled inputs reaching shell: - Commit messages (parsed by `conventional-commits.sh`) — could contain backticks / `$(...)` if reached unquoted - PR titles + bodies (parsed for size labels + bump labels) — same risk - Version strings (parsed by `semver.sh`) — `$NEXT_VERSION` reaches sed in #148's planned mechanism - Branch names + tag names (interpolated into shell commands) - Manifest JSON values (parsed by `jq`, then potentially interpolated) Audit each user-input → shell-interpolation path for proper quoting. ### post_bump_hooks trust model `release-toolkit.yml`'s `post_bump_hooks:` lets a consumer specify arbitrary shell commands that run during the cut. Documented trust model: - The consumer trusts their OWN config (they wrote it) - The toolkit doesn't sandbox the hooks Audit: - Are the docs explicit enough that a fresh adopter doesn't mistakenly think hooks are sandboxed? - Could a malicious `release-toolkit.yml` injected via a PR (where the workflow runs against the PR's content) execute hooks? - The `pull_request` trigger's permissions vs `push: main` permissions — are they aligned with the trust model? ### sed-based YAML rewrites (#148) #148's planned mechanism uses sed to rewrite the `ref:` value in `_release.yml`: ```bash sed -i "s|ref: '[^']*' # release-toolkit-build-ref|ref: '$NEXT_VERSION' # release-toolkit-build-ref|" ``` Audit: - What if `$NEXT_VERSION` contains `|` (sed delimiter)? `'` (string boundary)? Shell metacharacters? - Constraint: `$NEXT_VERSION` comes from semver-derived strings; should be `v[0-9.]+(-rc.[0-9]+)?`. Confirm the sanitization is enforced + documented. - If the sanitization layer is bypassed (someone injects `$NEXT_VERSION=...|;rm -rf /;|...` via env), what's the blast radius? ### Consumer config trust model `release-toolkit.yml` is read by the toolkit's scripts. Confirm: - The config parser (`config.sh`) doesn't `eval` or `source` consumer-provided values - Field values like `version_files: [...]` are validated (path traversal? glob expansion?) - `release_type` enum is validated (no fall-through to arbitrary string) ## Composition - **#148 build-bake refactor**: the sed-rewrite mechanism is a NEW security surface; this audit pass should run BEFORE or alongside #148's implementation, not after - **#149/#150 Unicode cleanups**: not security-relevant by themselves but the Unicode characters won't appear in interpolation-sensitive paths after cleanup; aligned with this audit's scope - **#152 examples**: the audit findings should inform what the examples DO and DON'T do (e.g., explicit "untrusted PR config" warnings) ## Implementation surface Audit-only PR (no code changes) producing: - `docs/security.md` summarizing the audit findings - A bats test seam for any "this MUST be sanitized" invariant - Issues filed for each substantive vulnerability found (if any) - A simple SECURITY.md at repo root with reporting instructions If vulnerabilities are found: tracker-each + remediate in follow-up PRs (don't bundle the audit doc with fixes). Estimated scope: 1-2 days of focused work + however many follow-up trackers materialize. ## What this PR does NOT do - **Does NOT fix vulnerabilities inline** — audit reports findings; remediation is per-finding follow-ups - **Does NOT add sandboxing** to post_bump_hooks — that's a substrate-feature decision, not audit scope - **Does NOT change the consumer-config trust model** — captures it explicitly; changes are post-audit decisions - **Does NOT cover supply-chain** (Forgejo Actions runner integrity, etc.) — out of release-toolkit scope; that's alcatraz-infra concern ## Refs - **Operator engagement 2026-06-27**: triple-audit suite recommendation accepted - **Composition**: [#148](https://git.frankenbit.de/frankenbit/release-toolkit/issues/148) (build-bake sed-rewrite), [#149](https://git.frankenbit.de/frankenbit/release-toolkit/issues/149)/[#150](https://git.frankenbit.de/frankenbit/release-toolkit/issues/150) (Unicode cleanups), [#152](https://git.frankenbit.de/frankenbit/release-toolkit/issues/152) (examples) - **Related**: ADR-0007 (token-override + bash-level fallback discipline)

quartermaster referenced this issue

2026-06-27 01:56:55 +02:00

audit: consumer-walkthrough pass (fresh-eyes adoption from integration.md to first cut) #157

quartermaster referenced this issue

2026-06-27 01:57:26 +02:00

audit: documentation drift pass (integration.md, ADRs, README, inline comments vs current substrate) #158

quartermaster referenced this issue

2026-06-27 02:05:09 +02:00

feat: structured JSON logging substrate (observability foundation per operator engagement) #159

quartermaster referenced this issue

2026-06-27 02:24:30 +02:00

chore(cleanup): ASCII-only sweep + .keep audit (closes #149 + #150 + #151) #161

quartermaster self-assigned this

2026-06-27 12:08:10 +02:00

quartermaster referenced this issue

2026-06-27 12:11:03 +02:00

security: verify sed-rewrite injection surface in bake_toolkit_ref (Sprint 5 security audit finding) #180

quartermaster referenced this issue

2026-06-27 12:11:13 +02:00

security: validate version_files paths against traversal (Sprint 5 security audit finding) #181

quartermaster referenced this issue from a commit

2026-06-27 14:27:13 +02:00

fix(build_bake): defense-in-depth ref-shape validation (closes #180)

~~quartermaster referenced this issue 2026-06-27 14:27:35 +02:00~~

chore(release): v0.13.2 #189

quartermaster referenced this issue from a commit

2026-06-27 14:32:27 +02:00

fix(config): reject path-traversal in version_files (closes #181)

quartermaster referenced this issue

2026-06-27 14:32:47 +02:00

fix(config): reject path-traversal in version_files (closes #181) #190

quartermaster referenced this issue from a commit

2026-06-27 14:33:26 +02:00

fix(build_bake): defense-in-depth ref-shape validation (closes #180)

quartermaster referenced this issue from a commit

2026-06-27 14:44:38 +02:00

fix(build_bake): defense-in-depth ref-shape validation (closes #180)

surveyor referenced this issue

2026-06-27 17:40:03 +02:00

docs(audit): doc-drift audit pass — ADR + inline-comment fixes (#158) #217

surveyor referenced this issue

2026-06-28 13:24:41 +02:00

docs(integration): fix int-1/2/3 doc-drift from #158 audit #228

quartermaster commented

2026-06-28 20:19:48 +02:00

Author

Owner

Closing — security audit umbrella. Sub-items shipped:

#180 (sed-injection mitigation in lib/build_bake.sh::bake_toolkit_ref — ref-shape allowlist [A-Za-z0-9._/-]+)
#181 (path-traversal validation in lib/config.sh::config_validate — rejects absolute paths + .. boundary patterns)

No additional security vectors surfaced beyond these during the audit pass. Ongoing chamber-discipline (substrate-state-care + Surveyor's verify-at-source on each PR review) keeps the security mindset present in normal review flow. If a new vector surfaces empirically, fresh tracker.

Operator confirmation: no remaining security gaps to close (2026-06-28).

Closing — security audit umbrella. Sub-items shipped: - **#180** (sed-injection mitigation in `lib/build_bake.sh::bake_toolkit_ref` — ref-shape allowlist `[A-Za-z0-9._/-]+`) - **#181** (path-traversal validation in `lib/config.sh::config_validate` — rejects absolute paths + `..` boundary patterns) No additional security vectors surfaced beyond these during the audit pass. Ongoing chamber-discipline (substrate-state-care + Surveyor's verify-at-source on each PR review) keeps the security mindset present in normal review flow. If a new vector surfaces empirically, fresh tracker. Operator confirmation: no remaining security gaps to close (2026-06-28).

quartermaster closed this issue

2026-06-28 20:19:49 +02:00