Integration: git credentials - #329
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
secretspec | 088fe40 | Commit Preview URL Branch Preview URL |
Aug 19 2026, 11:36 AM |
|
This looks great. One UX change I would like before merging: make a small Git-specific At the moment the helper does: match &args.file {
Some(path) => Secrets::load_from(path),
None => Secrets::load(),
}I would prefer: match &args.file {
Some(path) => Secrets::load_from(path),
None => Secrets::load_embedded_git_credentials(),
}In particular, the helper should not walk the current directory when no file was selected. That makes the default deterministic for The embedded manifest would contain declarations, not values—something like an optional The resulting UX would be: $ secretspec git configure --url https://github.com --username USERwith no manifest path recorded. Advanced/custom setups would retain the current behavior: $ secretspec --file company-git.toml git configure \
--url https://github.com \
--token-secret GITHUB_TOKENBecause the helper is intentionally read-only, the embedded default also needs an explicit way to populate and remove its values, e.g. I would add coverage for both important cases: no |
|
One additional integration worth supporting is SMTP credentials for Git already uses the credential-helper protocol when See A minimal implementation could:
For example, the setup could look like: $ git config --global sendemail.smtpServer smtp.example.com
$ git config --global sendemail.smtpServerPort 587
$ git config --global sendemail.smtpEncryption tls
$ git config --global sendemail.smtpUser user@example.com
$ secretspec git configure --url smtp://smtp.example.com:587 --username user@example.com --global
$ secretspec git login smtp://smtp.example.com:587The explicit |
8456ed4 to
394c2ec
Compare
a544f0e to
7d31cb9
Compare
7d31cb9 to
3441d3d
Compare
domenkozar
left a comment
There was a problem hiding this comment.
Review at effort high over the full diff: every hunk of secretspec/src/integration/git.rs and secretspec/src/cli/git.rs, the CLI and lib wiring, both new test files, and the docs. cargo check --all-targets is clean and the 15 new tests pass.
The security critical paths hold up under probing: validate_target, canonical_target and embedded_identity, target_matches (path prefix boundaries and SMTP username scoping), Request::apply_url replacing every field, and the managed file ensure_unchanged plus rollback sequencing. I checked unicode, space, percent encoded, and double slash paths against real git config --get-urlmatch behaviour, including the cases where git itself over matches and the helper's independent URL re check is what saves it.
Six inline comments below. The two I am most confident about are the ambient SECRETSPEC_PROFILE leaking into the persisted helper command, and git login writing to a provider the helper will not read from.
Posted by Claude Code on behalf of @domenkozar; see cli/cli#13904 for why the GitHub API cannot attribute this itself.
| if let Some(profile) = &options.profile { | ||
| secrets.set_profile(profile); | ||
| } | ||
| let profile = secrets.resolve_profile_name(None); |
There was a problem hiding this comment.
An ambient SECRETSPEC_PROFILE gets baked into the Git helper.
git configure's profile arg carries env = "SECRETSPEC_PROFILE", and unlike file (gated on typed.file at L166) it is passed through raw, so the profile resolved here is written permanently into the helper command at L302.
That contradicts the comment at L283 (only options the user typed belong in it) and the embedded branch below, which rejects a typed --profile outright. With SECRETSPEC_PROFILE=production exported:
secretspec --file company.toml git configure --url https://github.com --token-secret GITHUB_TOKEN
writes --profile 'production' into credential.https://github.com.helper, so every later git fetch from any shell resolves the production profile.
exported_variables_neither_block_commands_nor_reach_git_configuration (tests/git_configure.rs:670) asserts no --profile leaks, but it exercises the embedded path where profile is always None, so the --file branch is uncovered. Gating on options.typed.profile the way provider and reason already are would make the three consistent.
| let mut login = format!("secretspec git login {}", shell_quote(&target)); | ||
| if let Some(provider) = persisted_provider { | ||
| login.push_str(" --provider "); | ||
| login.push_str(&shell_quote(provider)); | ||
| } | ||
| println!("Store the credential with: {login}"); | ||
| if persisted_provider.is_none() && options.provider.is_some() { | ||
| println!( | ||
| "Note: SECRETSPEC_PROVIDER was not recorded in the Git helper; pass --provider to pin it." | ||
| ); | ||
| } |
There was a problem hiding this comment.
git login can write to a different provider than the helper reads from.
This warning correctly flags that an ambient SECRETSPEC_PROVIDER was not recorded in the helper, but the secretspec git login command printed just above carries no --provider, and login does honour the ambient variable through embedded_cli_secrets (L414).
So in one shell with SECRETSPEC_PROVIDER set: login stores PASSWORD_<id> into the ambient provider while the helper reads the default provider. git fetch then finds nothing, and no step along the way reports an error.
tests/git_configure.rs:707 runs exactly this sequence (its ambient array points at file://.../ambient-store) and only asserts success, so the mismatch is currently exercised and accepted. Emitting the same "not recorded" note from login and logout would close the loop.
| fn main() -> Result<()> { | ||
| secretspec::integration::git::main() | ||
| } |
There was a problem hiding this comment.
Missing the SIGPIPE default disposition restore.
secretspec.rs gained this in 25b752a (landed on main after this branch's base), so the two entry points diverge once this merges. Rust ignores SIGPIPE, so an early close of the helper's stdout, for example git-credential-secretspec ... get | head while debugging, or a reader that stops at the blank terminator, surfaces Broken pipe (os error 32) or a stdout panic instead of the quiet signal 13 exit the project just standardized on.
The same four libc::signal lines apply here.
| fn add_include(scope: Scope, path: &Path) -> Result<()> { | ||
| run_git( | ||
| [ | ||
| "config".into(), | ||
| scope.git_arg().into(), | ||
| "--add".into(), | ||
| "include.path".into(), | ||
| path.as_os_str().into(), | ||
| ], | ||
| "Failed to register the SecretSpec Git configuration", | ||
| ) | ||
| } |
There was a problem hiding this comment.
For Scope::Local this could register a relative include.path.
managed_path returns $GIT_COMMON_DIR/secretspec-credentials, which is always a sibling of the config file that includes it, and Git resolves a relative include.path against the including file's own directory. Registering the dunce::canonicalized absolute path instead means that renaming or moving the repository, or reaching it through a different symlink, leaves a dangling include: the helper silently stops being invoked, and unconfigure --all then recomputes the new path, deletes the file, and leaves the stale entry in .git/config for good.
A bare secretspec-credentials for the local scope survives the move. Global scope has no equivalent anchor, so it still needs the absolute path.
| if let Some(provider) = &options.provider { | ||
| secrets.set_provider(provider); | ||
| } |
There was a problem hiding this comment.
This set_provider has no effect.
secrets is never read after this point: the --file branch already consumed it at L242 to L250, and the embedded branch never uses it, since the helper gets persisted_provider instead.
The visible consequence is that secretspec git configure --url ... --provider <typo> succeeds and records --provider '<typo>' in the Git helper, with nothing surfacing until a later git fetch fails. Either validate the provider here or drop the mut binding along with the call.
| let markers = config_values(path, MARKER_KEY)?; | ||
| if markers != [FORMAT_VERSION.to_string()] { | ||
| return Err(unmanaged_file_error(path)); | ||
| } |
There was a problem hiding this comment.
An unknown format version leaves no way to unconfigure.
Any marker other than the compiled in FORMAT_VERSION is a hard error for every secretspec git subcommand in that scope, unconfigure --all included. Running a newer SecretSpec once, which writes version 2, and then falling back to an older binary, trivially easy with a devenv or nix shell on PATH, leaves hand editing Git config as the only escape.
Letting unconfigure remove an unknown but well formed managed file, or at minimum printing the manual removal steps in the error, would avoid the dead end.
No description provided.