perf+security: mimalloc, cargo-deny bans, refname validation, markdown escape, ureq Agent reuse#516
Open
BryanFRD wants to merge 6 commits into
Open
perf+security: mimalloc, cargo-deny bans, refname validation, markdown escape, ureq Agent reuse#516BryanFRD wants to merge 6 commits into
BryanFRD wants to merge 6 commits into
Conversation
… 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
Default allocator (glibc malloc on Linux, HeapAlloc on Windows) is suboptimal for alloc-heavy short-lived CLIs. mimalloc consistently shaves 5-15% wall time on workloads that match ours (TagIndex::build, revwalk + commit message decode, regex captures during conventional- commit parsing). Gated behind the cli feature so the wasm build doesn't pull it in. Adds ~200 KB to the release binary; net positive on perf benches. First item from #507.
cargo-deny already runs in CI (security job), but the bans section was empty. Add explicit denials for: - git2 / libgit2-sys: just migrated off in #487, prevent regression - openssl-sys / openssl-src: vendored via libgit2's old chain, the gix migration moved us to rustls. Reintroducing would double binary size and inherit OpenSSL's CVE cadence Closes #511.
Closes #512. ## Preview PR comment markdown escaping format_preview_comment was interpolating pkg.name / pkg.current_version / pkg.next_version / pkg.bump_type directly into a markdown table — all of which come from user-controlled .ferrflow + version files on the PRs HEAD. A package name like foo|<script> broke the table on github.com (rendered fine but visible) and triggered actual HTML on custom forge installs (Gitea, Forgejo with permissive markdown). Added escape_md_cell: encodes |, newline, <, >, backslash, backtick. 6 unit tests covering pipe-break, HTML injection, link injection, backtick code, newline-row-break. ## Refname validation New src/git/validate.rs::ensure_safe_refname_fragment rejects: - empty - leading - (flag confusion: --exec=ls etc.) - NUL byte - newline / carriage return - control characters except tab Called from create_tag, create_or_move_tag, push_branch, push_tags, verify_remote_branch, reset_branch_to_remote. The git tag invocations also gained a -- separator before the tag name so an exotic value cant be re-interpreted as a flag even if the validator misses something. ## Tests - 514 lib + 629 bin tests pass (was 514 + 616) - clippy -D warnings clean - New tests: 7 in validate.rs, 6 in preview.rs::tests
The bare ureq::get / ureq::post helpers create a fresh Agent (and TLS handshake) per call. A 50-pkg release does ~150 HTTPS round-trips against api.github.com (create_release × N, find_draft_release × N, publish_release × N, plus comment + auto-merge for PR mode) — each paying a fresh handshake. Build one Agent in build_forge() and store it on GitHubForge / GitLabForge. Subsequent calls reuse it via HTTP keep-alive. Expected 2-8 seconds saved on a 50-pkg release; smaller wins on single-pkg. Closes #509.
This was referenced May 24, 2026
…ns (#517) First half of #514. Closes the lock concern; the checkpoint/resume piece stays open (heavier design, separate PR). ## Problem Two concurrent ferrflow release invocations on the same repo (typical scenario: manually-triggered release racing the cron-driven auto-release workflow, or two CI runners on the same commit) competed on git refs. Observed symptoms: non-fast-forward rejects, half-pushed tag sets, draft releases created twice. ## Fix New src/monorepo/run/lock.rs: RAII lock guard backed by .git/ferrflow.lock created via O_CREAT|O_EXCL. Atomic. Released on drop, including panic unwind. Acquired at the top of run_release_logic for non-dry-run only. Read-only commands (check, status, version, tag) are not affected. ## Stale lock recovery A lockfile older than STALE_LOCK_TTL (30 min, longer than any realistic release) is treated as orphaned (process crashed without releasing) and taken over with a warning. Beyond TTL the user can also force-unlock manually by deleting the file; future PR may expose this via --force-unlock CLI flag. ## Tests - 6 unit tests in src/monorepo/run/lock.rs::tests: - acquire on clean repo succeeds - drop removes lockfile - second acquire fails while first held - force unlock takes over active lock - missing .git dir errors out cleanly - lockfile content starts with PID - 521 lib + 635 bin tests pass overall - cargo clippy -D warnings clean ## Out of scope The checkpoint/resume mechanism from #514 (write release-state.json at each step, resume if interrupted) is deferred — needs separate design review around atomic write + invalidation rules. Issue stays open.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on top of #505. Four quick-win items from the second audit pass.
Closes #509, #511, #512. First item of #507.
1. mimalloc allocator (first item of #507)
`#[global_allocator] static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc;` in `src/main.rs`. Gated behind the `cli` feature. Expected 5-15% wall-time on TagIndex::build / revwalk / regex captures. +200 KB binary.
2. cargo-deny bans (#511)
cargo-deny was already wired in CI but the bans list was empty. Added denials for `git2` / `libgit2-sys` (regression guard post-#487) and `openssl-sys` / `openssl-src` (replaced by rustls).
3. Markdown escape + refname validation (#512)
4. ureq Agent reuse (#509)
`ureq::get` / `ureq::post` / `ureq::patch` / `ureq::put` create a fresh Agent + TLS handshake per call. A 50-pkg release does ~150 HTTPS round-trips against `api.github.com` — each was paying a fresh handshake.
Built one `ureq::Agent` in `build_forge()`, stored on `GitHubForge` / `GitLabForge`, reused for every HTTP call. HTTP keep-alive saves 2-8 seconds on a 50-pkg release.
Test plan