Skip to content

security: strip git trace env, single-quote credential helper, harden URL parsing - #505

Merged
BryanFRD merged 1 commit into
mainfrom
fix/security-hardening-and-cwd-bugs
Jun 3, 2026
Merged

BryanFRD merged 1 commit into
mainfrom
fix/security-hardening-and-cwd-bugs

Conversation

@BryanFRD

Copy link
Copy Markdown
Contributor

Closes #489 (security hardening umbrella), #490 (detached HEAD branch resolve), #491 (fetch_and_rebase destructive checkout).

Summary

7 hardening fixes from the audit, batched as one PR because they share the test-suite + the same files (`src/git/auth.rs` is touched by 3 of them).

Security

  1. Strip `GIT_TRACE` / `GIT_TRACE_CURL` / `GIT_CURL_VERBOSE` / `GCM_TRACE` etc. on every `git` subprocess. Without this, CI with `GIT_CURL_VERBOSE=1` causes git to dump the `Authorization` header to stderr, which ferrflow forwards into `anyhow!` display strings. Token-pattern scrubber on stderr propagation as defense in depth (`ghs_/ghp_/gho_/ghu_/glpat_/github_pat_`).
  2. Switch credential helper escaping from double-quote to single-quoted sh literals with proper `'` → `'''` encoding. A token containing `$`, backtick, `;`, `&` no longer reaches `sh -c` as code.
  3. Hook subprocesses `env_remove` `GITHUB_TOKEN` / `FERRFLOW_TOKEN` / `GITLAB_TOKEN` before exec. Bot mode sets these process-wide for git's credential helper, but a malicious user hook is no longer one `$GITHUB_TOKEN echo` away from exfiltration.
  4. `extract_host` strips userinfo (`user[:pwd]@`) before returning the host. Closes the `https://attacker.com#@github.com/...\` confusion vector.
  5. TS config loader writes its wrapper into `tempfile::tempdir()` instead of next to the user's `.ts`. Closes a symlink TOCTOU.

Bug fixes

  1. `resolve_current_branch` only uses `GITHUB_REF` when prefixed `refs/heads/`. Previously a CI in detached-HEAD with `GITHUB_REF=refs/tags/v1.2.3` used `v1.2.3` as a branch name everywhere downstream.
  2. `fetch_and_rebase` hard-fails on detached HEAD instead of silently rewriting whichever local branch matches the target name. The previous `.ok().unwrap_or_default() == local_ref` collapsed "detached HEAD" and "different branch" into the destructive path.

Test plan

  • 514 lib tests + 616 bin tests pass
  • `cargo clippy --features cli -- -D warnings` clean
  • New tests: `configure_git_command_single_quote_escapes_dangerous_token_chars` (uses an `evil';rm -rf /;#` token) and `configure_git_command_strips_git_trace_env`
  • Release-bot flow against a real GitHub remote with `GIT_CURL_VERBOSE=1` set — verify Authorization header doesn't leak into error output

… URL parsing

Closes #489 (security hardening umbrella), #490 (detached HEAD fallback),
#491 (fetch_and_rebase swallows symbolic-ref error).

## Security

- Strip GIT_TRACE / GIT_TRACE_CURL / GIT_CURL_VERBOSE / GCM_TRACE etc.
  on every Command::new("git") that talks to a remote. Without this,
  CI that sets GIT_CURL_VERBOSE=1 (or a Datadog APM agent that injects
  GIT_TRACE_CURL transparently) causes git to dump the Authorization
  header to stderr, which ferrflow then forwarded into anyhow::Error
  display strings. Added a token-pattern scrubber on stderr propagation
  as a defense in depth (masks ghs_/ghp_/gho_/ghu_/glpat_/github_pat_).
- Switch credential helper escaping from double-quote escaping to
  single-quoted sh literals with proper ' -> '\'' encoding. A token
  containing dollar, backtick, semicolon, ampersand no longer reaches
  sh -c as code. Added unit test with an evil token.
- Hook subprocesses now env_remove GITHUB_TOKEN / FERRFLOW_TOKEN /
  GITLAB_TOKEN before exec. Bot mode sets these process-wide so git
  picks them up via credential helper, but a malicious hook can no
  longer exfiltrate the token by echoing the env var.
- extract_host (src/forge/mod.rs) now strips userinfo (user[:pwd]@)
  before returning the host. Previously a remote like
  https://attacker.com#@github.com/owner/repo resolved host to
  "attacker.com#@github.com" which downstream forge construction
  could mis-route the token to.
- TS config loader writes its wrapper into tempfile::tempdir() instead
  of the directory of the user .ts. Closes a symlink TOCTOU where
  a cohabiting process could create .ferrflow-loader.mjs as a symlink
  to ~/.bashrc before ferrflow fs::write follows it.

## Bug fixes

- resolve_current_branch (src/git/repo.rs) no longer falls back to
  GITHUB_REF_NAME when the CI is in detached HEAD mode and that env
  var contains a tag name. Only use GITHUB_REF when it starts with
  refs/heads/. Same logic for CI_COMMIT_REF_NAME (skip when
  CI_COMMIT_TAG is set).
- fetch_and_rebase (src/git/push.rs) no longer silently rewrites a
  branch ref when symbolic-ref fails. Previously a detached HEAD
  state caused .ok().unwrap_or_default() to compare empty to the
  target ref, dropping into a destructive update-ref plus checkout -f
  that could overwrite the user local state. Now hard-fails with a
  clear HEAD is detached error and an actionable hint.
- formats/gomod.rs no longer falls through to the process CWD when
  file_path.parent() is the empty Path.

## Tests

- 514 lib tests plus 616 bin tests pass, cargo clippy -D warnings clean
- New tests: single-quote-escape an evil token, strip GIT_TRACE env
Copilot AI review requested due to automatic review settings May 22, 2026 19:09

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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@github-actions github-actions Bot 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.

Benchmark

Details
Benchmark suite Current: 577336d Previous: 0b5fe57 Ratio
git_collect_tags/single_tag 19778 ns/iter (± 51)

This comment was automatically generated by workflow using github-action-benchmark.

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.

security: harden subprocess execution and token handling

2 participants