fix(sec): zeroize EmbeddingConfig.api_key on drop (partial LF-001)#6190
Conversation
|
Thanks for your first pull request! 🎉 A maintainer will review it soon. While you wait, please make sure:
We aim to provide initial feedback within 7 days. See CONTRIBUTING.md for details. |
github-actions
Bot
added
area/docs
|
Note: cargo test --workspace and cargo clippy --workspace fail on the current codebase |
github-actions
Bot
added
the
has-conflicts
github-actions
Bot
added
ready-for-review
|
Two issues for maintainer attention before this lands: Branch name mismatch. The head branch is Overlaps with #6193. Both PRs remove the same five Generated by Claude Code |
houko
left a comment
houko
left a comment
There was a problem hiding this comment.
The PR title ("chore(deps): remove 5 orphaned email dependencies after sidecar migration") and the description ("Removes lettre, imap, rustls-connector, mailparse, and rustls-pemfile from the root Cargo.toml") do not match the actual diff.
The diff touches only two files — crates/librefang-runtime/src/embedding.rs and CHANGELOG.md — and makes no changes to Cargo.toml or deny.toml. The real change is wrapping EmbeddingConfig.api_key in Zeroizing<String> so the credential is zeroed on drop, which is a worthwhile security fix. The branch name fix/zeroize-embedding-api-key correctly describes what was actually changed.
Please update the PR title and description to reflect the Zeroizing change. A reviewer scanning the title/description currently has no idea what code was modified.
Generated by Claude Code
houko
left a comment
houko
left a comment
There was a problem hiding this comment.
The PR title and body do not match the actual diff.
Title: "chore(deps): remove 5 orphaned email dependencies after sidecar migration"
Body: describes removing lettre, imap, rustls-connector, mailparse, rustls-pemfile from Cargo.toml
Actual diff: wraps EmbeddingConfig.api_key in Zeroizing<String> for credential zeroization on drop (crates/librefang-runtime/src/embedding.rs), matching the branch name fix/zeroize-embedding-api-key.
The CHANGELOG entry inside the diff itself correctly describes the change as the zeroizing fix (@mrchn), and the branch name is consistent with the diff. The title/body appear to be from a different PR.
Suggested title: fix(sec): wrap EmbeddingConfig.api_key in Zeroizing<String> to zeroize API key on drop
Please update the title and body so reviewers and git log consumers get accurate context.
Generated by Claude Code
BunnyMoth
changed the title
github-actions
Bot
added
has-conflicts
houko
left a comment
houko
left a comment
There was a problem hiding this comment.
LGTM on the change itself — wrapping EmbeddingConfig.api_key in Zeroizing<String> is the right fix and brings the config struct in line with how the driver structs already store the key. Construction sites and tests are updated correctly, and the Zeroizing::new(config.api_key) → config.api_key simplifications at the driver boundary are correct now that the field is already zeroizing.
One thing before it can land: the PR is currently conflicting with main (the CHANGELOG [Unreleased] section moved). Please rebase onto latest main and resolve the CHANGELOG hunk — the code is approved.
github-actions
Bot
added
ready-for-review
…missing The detect-secrets step used to soft-skip when the tool was not installed, trusting CI as the authoritative gate. That is exactly how baseline drift reached main: a commit changes a fixture so a previously-flagged line no longer matches (#6190 swapped a plain-string api_key for Zeroizing::new("bogus")), the committer's hook silently skips because they never installed detect-secrets, and the secrets CI job then goes red on every push until someone regenerates .secrets.baseline. Now the hook errors and refuses the commit when detect-secrets is absent, moving the failure to the one place that can fix it cheaply. A genuinely tool-less host (the Docker-only dev box in CLAUDE.md > Build & Verify) can opt out with LIBREFANG_SKIP_SECRETS_SCAN=1, mirroring the rustfmt soft-skip above it and pre-push's LIBREFANG_PREPUSH_SKIP escape hatch.
…commit when detect-secrets missing (#6259) * chore(ci): regenerate .secrets.baseline to clear drift from #6190 PR #6190 (fix(sec): zeroize EmbeddingConfig.api_key on drop) changed a test fixture in crates/librefang-runtime/src/embedding.rs from a plain-string api_key to Zeroizing::new("bogus".to_string()), so detect-secrets no longer flags that line. The committed .secrets.baseline was not regenerated, leaving a stale "Secret Keyword" entry (40ce4379) that a fresh CI scan no longer reproduces. The secrets job enforces a byte-for-byte match between a fresh scan and the committed baseline (ignoring generated_at / version / line_number), so main has been red on every push since #6190 merged. Regenerating the baseline drops the phantom entry; the remaining diff is line-number churn that the CI comparison already strips. * fix(hooks): hard-error pre-commit secret scan when detect-secrets is missing The detect-secrets step used to soft-skip when the tool was not installed, trusting CI as the authoritative gate. That is exactly how baseline drift reached main: a commit changes a fixture so a previously-flagged line no longer matches (#6190 swapped a plain-string api_key for Zeroizing::new("bogus")), the committer's hook silently skips because they never installed detect-secrets, and the secrets CI job then goes red on every push until someone regenerates .secrets.baseline. Now the hook errors and refuses the commit when detect-secrets is absent, moving the failure to the one place that can fix it cheaply. A genuinely tool-less host (the Docker-only dev box in CLAUDE.md > Build & Verify) can opt out with LIBREFANG_SKIP_SECRETS_SCAN=1, mirroring the rustfmt soft-skip above it and pre-push's LIBREFANG_PREPUSH_SKIP escape hatch. --------- Co-authored-by: Evan <[email protected]>
2 participants
Wraps EmbeddingConfig.api_key in Zeroizing so the credential is wiped from memory when the struct is dropped. Zeroizing was already imported in the crate. Updated 7 construction sites (2 production, 5 test). Removed double-wrapping in OpenAIEmbeddingDriver::new and CohereEmbeddingDriver::new.
All librefang-runtime tests pass.
Note: cargo test --workspace and cargo clippy --workspace fail on the current codebase due to a pre-existing MSRV mismatch ([email protected] requires rustc 1.95, toolchain is 1.94.1). This failure exists on unmodified main and is unrelated to this change.
Changes: