From 78bd829ae4b2d46ca2755bdb34250a8375099fd2 Mon Sep 17 00:00:00 2001 From: poketopa Date: Sun, 23 Aug 2026 22:14:19 +0900 Subject: [PATCH] fix(remote): validate credential fields before use --- src/remote.rs | 154 ++++++++++++++++++++++++++++++++++++++----- src/repository.rs | 2 +- src/submodule.rs | 4 +- tests/remote.spec.ts | 38 +++++++++++ 4 files changed, 179 insertions(+), 19 deletions(-) diff --git a/src/remote.rs b/src/remote.rs index 115ef31..22cae0b 100644 --- a/src/remote.rs +++ b/src/remote.rs @@ -78,34 +78,149 @@ pub struct Credential { } impl Credential { + fn username(&self) -> &str { + self.username.as_deref().unwrap_or("git") + } + + fn validate(&self) -> Result<()> { + if let Some((field_name, credential_type)) = self.missing_required_field() { + return Err(Error::new( + Status::InvalidArg, + required_credential_field_message(field_name, credential_type), + )); + } + Ok(()) + } + + fn missing_required_field(&self) -> Option<(&'static str, &'static str)> { + match self.r#type { + CredentialType::SSHKeyFromPath if self.private_key_path.is_none() => { + Some(("credential.privateKeyPath", "SSHKeyFromPath")) + } + CredentialType::SSHKey if self.private_key.is_none() => Some(("credential.privateKey", "SSHKey")), + CredentialType::Plain if self.password.is_none() => Some(("credential.password", "Plain")), + _ => None, + } + } + pub(crate) fn to_git2_cred(&self) -> std::result::Result { - let fallback = "git".to_string(); + let username = self.username(); let cred = match self.r#type { CredentialType::Default => git2::Cred::default(), - CredentialType::SSHKeyFromAgent => { - git2::Cred::ssh_key_from_agent(self.username.to_owned().unwrap_or(fallback).as_ref()) - } + CredentialType::SSHKeyFromAgent => git2::Cred::ssh_key_from_agent(username), CredentialType::SSHKeyFromPath => git2::Cred::ssh_key( - self.username.to_owned().unwrap_or(fallback).as_ref(), + username, self.public_key_path.as_ref().map(Path::new), - Path::new(&self.private_key_path.to_owned().unwrap()), + Path::new(required_credential_field( + &self.private_key_path, + "credential.privateKeyPath", + "SSHKeyFromPath", + )?), self.passphrase.as_ref().map(String::as_ref), ), CredentialType::SSHKey => git2::Cred::ssh_key_from_memory( - self.username.to_owned().unwrap_or(fallback).as_ref(), + username, self.public_key.as_ref().map(String::as_ref), - &self.private_key.to_owned().unwrap(), + required_credential_field(&self.private_key, "credential.privateKey", "SSHKey")?, self.passphrase.as_ref().map(String::as_ref), ), CredentialType::Plain => git2::Cred::userpass_plaintext( - self.username.to_owned().unwrap_or(fallback).as_ref(), - &self.password.to_owned().unwrap(), + username, + required_credential_field(&self.password, "credential.password", "Plain")?, ), }?; Ok(cred) } } +fn required_credential_field<'a>( + value: &'a Option, + field_name: &str, + credential_type: &str, +) -> std::result::Result<&'a str, git2::Error> { + value + .as_deref() + .ok_or_else(|| git2::Error::from_str(&required_credential_field_message(field_name, credential_type))) +} + +fn required_credential_field_message(field_name: &str, credential_type: &str) -> String { + format!("{field_name} is required for {credential_type} credentials") +} + +#[cfg(test)] +mod credential_tests { + use super::{Credential, CredentialType}; + + fn credential(r#type: CredentialType) -> Credential { + Credential { + r#type, + username: None, + public_key_path: None, + public_key: None, + private_key_path: None, + private_key: None, + passphrase: None, + password: None, + } + } + + fn assert_missing_field_error(credential: Credential, message: &str) { + let git2_error = match credential.to_git2_cred() { + Ok(_) => panic!("credential conversion should fail"), + Err(error) => error, + }; + assert_eq!(git2_error.message(), message); + } + + #[test] + fn missing_private_key_path_returns_an_error() { + assert_missing_field_error( + credential(CredentialType::SSHKeyFromPath), + "credential.privateKeyPath is required for SSHKeyFromPath credentials", + ); + } + + #[test] + fn missing_private_key_returns_an_error() { + assert_missing_field_error( + credential(CredentialType::SSHKey), + "credential.privateKey is required for SSHKey credentials", + ); + } + + #[test] + fn missing_password_returns_an_error() { + assert_missing_field_error( + credential(CredentialType::Plain), + "credential.password is required for Plain credentials", + ); + } + + #[test] + fn defaults_username_to_git() { + assert_eq!(credential(CredentialType::Plain).username(), "git"); + + let mut credential = credential(CredentialType::Plain); + credential.username = Some("custom".to_string()); + assert_eq!(credential.username(), "custom"); + } + + #[test] + fn converts_valid_credentials() { + let mut ssh_key_from_path = credential(CredentialType::SSHKeyFromPath); + ssh_key_from_path.private_key_path = Some("private-key".to_string()); + assert!(ssh_key_from_path.to_git2_cred().is_ok()); + + let mut ssh_key = credential(CredentialType::SSHKey); + ssh_key.private_key = Some("private-key".to_string()); + assert!(ssh_key.to_git2_cred().is_ok()); + + let mut plain = credential(CredentialType::Plain); + plain.password = Some("password".to_string()); + assert!(plain.to_git2_cred().is_ok()); + } +} + #[napi(object)] pub struct ProxyOptions { /// Try to auto-detect the proxy from the git configuration. @@ -410,10 +525,11 @@ impl<'a> ApplyRemoteCallbacks for git2::RemoteCallbacks<'a> { } impl<'a> FetchOptions { - pub(crate) fn to_git2_fetch_options(&'a self) -> git2::FetchOptions<'a> { + pub(crate) fn to_git2_fetch_options(&'a self) -> Result> { let mut fetch = git2::FetchOptions::new(); let mut callbacks = git2::RemoteCallbacks::new(); if let Some(cred) = &self.credential { + cred.validate()?; // TODO: support credential callback callbacks.credentials(move |_url, _username, _cred| cred.to_git2_cred()); } @@ -439,7 +555,7 @@ impl<'a> FetchOptions { if let Some(custom_headers) = &self.custom_headers { fetch.custom_headers(&custom_headers.iter().map(|x| x.as_str()).collect::>()); } - fetch + Ok(fetch) } } @@ -470,10 +586,11 @@ pub struct PushOptions { } impl<'a> PushOptions { - pub(crate) fn to_git2_push_options(&'a self) -> git2::PushOptions<'a> { + pub(crate) fn to_git2_push_options(&'a self) -> Result> { let mut push = git2::PushOptions::new(); let mut callbacks = git2::RemoteCallbacks::new(); if let Some(cred) = &self.credential { + cred.validate()?; // TODO: support credential callback callbacks.credentials(move |_url, _username, _cred| cred.to_git2_cred()); } @@ -496,7 +613,7 @@ impl<'a> PushOptions { if let Some(remote_options) = &self.remote_options { push.remote_push_options(&remote_options.iter().map(|x| x.as_str()).collect::>()); } - push + Ok(push) } } @@ -536,7 +653,7 @@ impl Task for FetchRemoteTask { .write() .map_err(|e| Error::new(Status::GenericFailure, format!("{e}")))?; let mut fetch_options = match &self.options { - Some(FetchRemoteOptions { fetch: Some(fetch), .. }) => Some(fetch.to_git2_fetch_options()), + Some(FetchRemoteOptions { fetch: Some(fetch), .. }) => Some(fetch.to_git2_fetch_options()?), _ => None, }; let reflog_msg = match &self.options { @@ -576,7 +693,11 @@ impl Task for PushRemoteTask { .remote .write() .map_err(|e| Error::new(Status::GenericFailure, format!("{e}")))?; - let mut push_options = self.options.as_ref().map(|x| x.to_git2_push_options()); + let mut push_options = self + .options + .as_ref() + .map(PushOptions::to_git2_push_options) + .transpose()?; remote .inner .push(&self.refspecs, push_options.as_mut()) @@ -610,6 +731,7 @@ impl Task for PruneRemoteTask { Some(PruneOptions { credential: Some(cred), .. }) => { + cred.validate()?; let mut callbacks = git2::RemoteCallbacks::new(); callbacks.credentials(move |_url, _username, _cred| cred.to_git2_cred()); Some(callbacks) diff --git a/src/repository.rs b/src/repository.rs index cb25b10..9b04f0b 100644 --- a/src/repository.rs +++ b/src/repository.rs @@ -770,7 +770,7 @@ impl Task for CloneRepositoryTask { builder.branch(branch); } if let Some(fetch) = &opts.fetch { - let fetch_options = fetch.to_git2_fetch_options(); + let fetch_options = fetch.to_git2_fetch_options()?; builder.fetch_options(fetch_options); } if let Some(true) = &opts.recursive { diff --git a/src/submodule.rs b/src/submodule.rs index e2c12c4..bbd4910 100644 --- a/src/submodule.rs +++ b/src/submodule.rs @@ -323,7 +323,7 @@ impl Task for SubmoduleUpdateTask { opts.checkout(checkout.clone().into()); } if let Some(fetch) = &options.fetch { - opts.fetch(fetch.to_git2_fetch_options()); + opts.fetch(fetch.to_git2_fetch_options()?); } if let Some(allow_fetch) = options.allow_fetch { opts.allow_fetch(allow_fetch); @@ -360,7 +360,7 @@ impl Task for SubmoduleCloneTask { opts.checkout(checkout.clone().into()); } if let Some(fetch) = &options.fetch { - opts.fetch(fetch.to_git2_fetch_options()); + opts.fetch(fetch.to_git2_fetch_options()?); } if let Some(allow_fetch) = options.allow_fetch { opts.allow_fetch(allow_fetch); diff --git a/tests/remote.spec.ts b/tests/remote.spec.ts index 6cfc31c..d7dc4f0 100644 --- a/tests/remote.spec.ts +++ b/tests/remote.spec.ts @@ -4,6 +4,14 @@ import { isTarget } from './env'; import { useFixture } from './fixtures'; import { makeTmpDir } from './tmp'; +async function createLocalRemote() { + const localPath = await makeTmpDir('credential-validation-local'); + const remotePath = await makeTmpDir('credential-validation-remote'); + const repo = await initRepository(localPath); + await initRepository(remotePath, { bare: true }); + return repo.createRemote('origin', remotePath); +} + describe('remote', () => { it('get remote names', { skip: isTarget('linux', undefined, 'gnu') }, async () => { const p = await makeTmpDir('clone'); @@ -122,4 +130,34 @@ describe('remote', () => { expect(packProgress).toHaveBeenCalled(); }); }); + + it.each([ + { + credential: { type: 'SSHKeyFromPath' }, + message: 'credential.privateKeyPath is required for SSHKeyFromPath credentials', + }, + { + credential: { type: 'SSHKey' }, + message: 'credential.privateKey is required for SSHKey credentials', + }, + { + credential: { type: 'Plain' }, + message: 'credential.password is required for Plain credentials', + }, + ])('rejects missing required fields for $credential.type credentials', async ({ credential, message }) => { + const remote = await createLocalRemote(); + await expect(remote.fetch([], { fetch: { credential: credential as any } })).rejects.toMatchObject({ + code: 'InvalidArg', + message, + }); + }); + + it.each([ + { type: 'SSHKeyFromPath', privateKeyPath: 'synthetic-private-key-path' }, + { type: 'SSHKey', privateKey: 'synthetic-private-key' }, + { type: 'Plain', password: 'synthetic-password' }, + ])('accepts valid $type credentials', async credential => { + const remote = await createLocalRemote(); + await expect(remote.fetch([], { fetch: { credential: credential as any } })).resolves.toBeUndefined(); + }); });