From 030e040b6819d2628011520c207313cdc16f262d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 24 Sep 2025 08:47:24 +0200 Subject: [PATCH 1/2] fix: credential fill to allow protocol+host without URL Co-authored-by: Byron <63622+Byron@users.noreply.github.com> --- gitoxide-core/src/repository/credential.rs | 5 +- gix-credentials/src/program/main.rs | 12 ++- gix-credentials/src/protocol/context/mod.rs | 11 +- gix-credentials/src/protocol/mod.rs | 2 +- .../tests/program/main_validation.rs | 101 ++++++++++++++++++ gix-credentials/tests/program/mod.rs | 1 + gix-credentials/tests/protocol/context.rs | 49 +++++++++ 7 files changed, 173 insertions(+), 8 deletions(-) create mode 100644 gix-credentials/tests/program/main_validation.rs diff --git a/gitoxide-core/src/repository/credential.rs b/gitoxide-core/src/repository/credential.rs index c901f06f2ba..618446da013 100644 --- a/gitoxide-core/src/repository/credential.rs +++ b/gitoxide-core/src/repository/credential.rs @@ -15,8 +15,11 @@ pub fn function(repo: gix::Repository, action: gix::credentials::program::main:: std::io::stdin(), std::io::stdout(), |action, context| -> Result<_, Error> { + let url = context.url.clone().or_else(|| context.to_url()).ok_or_else(|| { + Error::Protocol(gix::credentials::protocol::Error::UrlMissing) + })?; let (mut cascade, _action, prompt_options) = repo.config_snapshot().credential_helpers(gix::url::parse( - context.url.as_ref().expect("framework assures URL is present").as_ref(), + url.as_ref(), )?)?; cascade .invoke( diff --git a/gix-credentials/src/program/main.rs b/gix-credentials/src/program/main.rs index 34f4163a01e..1c00cb6c70a 100644 --- a/gix-credentials/src/program/main.rs +++ b/gix-credentials/src/program/main.rs @@ -55,7 +55,7 @@ pub enum Error { Context(#[from] crate::protocol::context::decode::Error), #[error("Credentials for {url:?} could not be obtained")] CredentialsMissing { url: BString }, - #[error("The url wasn't provided in input - the git credentials protocol mandates this")] + #[error("Either 'url' field or both 'protocol' and 'host' fields must be provided")] UrlMissing, } @@ -89,15 +89,17 @@ pub(crate) mod function { let mut buf = Vec::::with_capacity(512); stdin.read_to_end(&mut buf)?; let ctx = Context::from_bytes(&buf)?; - if ctx.url.is_none() { + if ctx.url.is_none() && (ctx.protocol.is_none() || ctx.host.is_none()) { return Err(Error::UrlMissing); } let res = credentials(action, ctx).map_err(|err| Error::Helper { source: Box::new(err) })?; match (action, res) { (Action::Get, None) => { - return Err(Error::CredentialsMissing { - url: Context::from_bytes(&buf)?.url.expect("present and checked above"), - }) + let ctx_for_error = Context::from_bytes(&buf)?; + let url = ctx_for_error.url.clone() + .or_else(|| ctx_for_error.to_url()) + .expect("URL is available either directly or via protocol+host"); + return Err(Error::CredentialsMissing { url }) } (Action::Get, Some(ctx)) => ctx.write_to(stdout)?, (Action::Erase | Action::Store, None) => {} diff --git a/gix-credentials/src/protocol/context/mod.rs b/gix-credentials/src/protocol/context/mod.rs index 01ee151f306..329bcae1131 100644 --- a/gix-credentials/src/protocol/context/mod.rs +++ b/gix-credentials/src/protocol/context/mod.rs @@ -92,7 +92,16 @@ mod mutate { /// normally this isn't the case. #[allow(clippy::result_large_err)] pub fn destructure_url_in_place(&mut self, use_http_path: bool) -> Result<&mut Self, protocol::Error> { - let url = gix_url::parse(self.url.as_ref().ok_or(protocol::Error::UrlMissing)?.as_ref())?; + if self.url.is_none() { + // If URL is not present but protocol and host are, construct the URL + if let Some(constructed_url) = self.to_url() { + self.url = Some(constructed_url); + } else { + return Err(protocol::Error::UrlMissing); + } + } + + let url = gix_url::parse(self.url.as_ref().expect("URL is present after check above").as_ref())?; self.protocol = Some(url.scheme.as_str().into()); self.username = url.user().map(ToOwned::to_owned); self.password = url.password().map(ToOwned::to_owned); diff --git a/gix-credentials/src/protocol/mod.rs b/gix-credentials/src/protocol/mod.rs index 2920780bf95..7a71c9c4b3d 100644 --- a/gix-credentials/src/protocol/mod.rs +++ b/gix-credentials/src/protocol/mod.rs @@ -20,7 +20,7 @@ pub type Result = std::result::Result, Error>; pub enum Error { #[error(transparent)] UrlParse(#[from] gix_url::parse::Error), - #[error("The 'url' field must be set when performing a 'get/fill' action")] + #[error("Either 'url' field or both 'protocol' and 'host' fields must be provided")] UrlMissing, #[error(transparent)] ContextDecode(#[from] context::decode::Error), diff --git a/gix-credentials/tests/program/main_validation.rs b/gix-credentials/tests/program/main_validation.rs new file mode 100644 index 00000000000..83e84437746 --- /dev/null +++ b/gix-credentials/tests/program/main_validation.rs @@ -0,0 +1,101 @@ +use gix_credentials::program::main; +use std::io::Cursor; + +#[derive(Debug, thiserror::Error)] +#[error("Test error")] +struct TestError; + +#[test] +fn protocol_and_host_without_url_is_valid() { + let input = b"protocol=https\nhost=github.com\n"; + let mut output = Vec::new(); + + let result = main( + ["get".into()], + Cursor::new(input), + &mut output, + |_action, context| -> Result, TestError> { + // Verify the context has the expected fields + assert_eq!(context.protocol.as_deref(), Some("https")); + assert_eq!(context.host.as_deref(), Some("github.com")); + // Should return None to simulate no credentials found (which is expected in test) + Ok(None) + }, + ); + + // This should fail because our mock helper returned None (no credentials found) + // but it should NOT fail because of missing URL + match result { + Err(gix_credentials::program::main::Error::CredentialsMissing { .. }) => { + // This is the expected error - credentials missing, not URL missing + } + other => panic!("Expected CredentialsMissing error, got: {:?}", other), + } +} + +#[test] +fn missing_protocol_with_host_fails() { + let input = b"host=github.com\n"; + let mut output = Vec::new(); + + let result = main( + ["get".into()], + Cursor::new(input), + &mut output, + |_action, _context| -> Result, TestError> { Ok(None) }, + ); + + match result { + Err(gix_credentials::program::main::Error::UrlMissing) => { + // This is expected + } + other => panic!("Expected UrlMissing error, got: {:?}", other), + } +} + +#[test] +fn missing_host_with_protocol_fails() { + let input = b"protocol=https\n"; + let mut output = Vec::new(); + + let result = main( + ["get".into()], + Cursor::new(input), + &mut output, + |_action, _context| -> Result, TestError> { Ok(None) }, + ); + + match result { + Err(gix_credentials::program::main::Error::UrlMissing) => { + // This is expected + } + other => panic!("Expected UrlMissing error, got: {:?}", other), + } +} + +#[test] +fn url_alone_is_still_valid() { + let input = b"url=https://github.com\n"; + let mut output = Vec::new(); + + let result = main( + ["get".into()], + Cursor::new(input), + &mut output, + |_action, context| -> Result, TestError> { + // Verify the context has the expected fields + assert_eq!(context.url.as_deref().map(|b| &**b), Some("https://github.com".as_bytes())); + // Should return None to simulate no credentials found (which is expected in test) + Ok(None) + }, + ); + + // This should fail because our mock helper returned None (no credentials found) + // but it should NOT fail because of missing URL + match result { + Err(gix_credentials::program::main::Error::CredentialsMissing { .. }) => { + // This is the expected error - credentials missing, not URL missing + } + other => panic!("Expected CredentialsMissing error, got: {:?}", other), + } +} \ No newline at end of file diff --git a/gix-credentials/tests/program/mod.rs b/gix-credentials/tests/program/mod.rs index 3672dd18e38..393e2c6ee8f 100644 --- a/gix-credentials/tests/program/mod.rs +++ b/gix-credentials/tests/program/mod.rs @@ -1 +1,2 @@ mod from_custom_definition; +mod main_validation; diff --git a/gix-credentials/tests/protocol/context.rs b/gix-credentials/tests/protocol/context.rs index e60f3ebf850..58482094850 100644 --- a/gix-credentials/tests/protocol/context.rs +++ b/gix-credentials/tests/protocol/context.rs @@ -72,6 +72,55 @@ mod destructure_url_in_place { true, ); } + + #[test] + fn protocol_and_host_without_url_constructs_url_first() { + let mut ctx = Context { + protocol: Some("https".into()), + host: Some("github.com".into()), + ..Default::default() + }; + ctx.destructure_url_in_place(false).expect("should work with protocol and host"); + + // URL should be constructed from protocol and host + assert_eq!(ctx.url.as_deref().map(|b| &**b), Some("https://github.com".as_bytes())); + // Original fields should be preserved + assert_eq!(ctx.protocol.as_deref(), Some("https")); + assert_eq!(ctx.host.as_deref(), Some("github.com")); + } + + #[test] + fn protocol_and_host_with_path_without_url_constructs_full_url() { + let mut ctx = Context { + protocol: Some("https".into()), + host: Some("github.com".into()), + path: Some("org/repo".into()), + ..Default::default() + }; + ctx.destructure_url_in_place(false).expect("should work with protocol, host and path"); + + // URL should be constructed from all provided fields + assert_eq!(ctx.url.as_deref().map(|b| &**b), Some("https://github.com/org/repo".as_bytes())); + // Original fields should be preserved + assert_eq!(ctx.protocol.as_deref(), Some("https")); + assert_eq!(ctx.host.as_deref(), Some("github.com")); + assert_eq!(ctx.path.as_deref().map(|b| &**b), Some("org/repo".as_bytes())); + } + + #[test] + fn missing_protocol_or_host_without_url_fails() { + let mut ctx_no_protocol = Context { + host: Some("github.com".into()), + ..Default::default() + }; + assert!(ctx_no_protocol.destructure_url_in_place(false).is_err()); + + let mut ctx_no_host = Context { + protocol: Some("https".into()), + ..Default::default() + }; + assert!(ctx_no_host.destructure_url_in_place(false).is_err()); + } } mod to_prompt { From 2cc63ca95d6592292f6ae135c62904c1ae9ffb49 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 24 Sep 2025 08:43:01 +0200 Subject: [PATCH 2/2] refactor - fmt - clippy --- gitoxide-core/src/repository/credential.rs | 14 +-- gix-credentials/src/program/main.rs | 12 ++- gix-credentials/src/protocol/context/mod.rs | 9 +- gix-credentials/tests/program/main.rs | 94 ++++++++++++++++ .../tests/program/main_validation.rs | 101 ------------------ gix-credentials/tests/program/mod.rs | 2 +- gix-credentials/tests/protocol/context.rs | 37 +++---- 7 files changed, 127 insertions(+), 142 deletions(-) create mode 100644 gix-credentials/tests/program/main.rs delete mode 100644 gix-credentials/tests/program/main_validation.rs diff --git a/gitoxide-core/src/repository/credential.rs b/gitoxide-core/src/repository/credential.rs index 618446da013..9684250bd4d 100644 --- a/gitoxide-core/src/repository/credential.rs +++ b/gitoxide-core/src/repository/credential.rs @@ -15,12 +15,14 @@ pub fn function(repo: gix::Repository, action: gix::credentials::program::main:: std::io::stdin(), std::io::stdout(), |action, context| -> Result<_, Error> { - let url = context.url.clone().or_else(|| context.to_url()).ok_or_else(|| { - Error::Protocol(gix::credentials::protocol::Error::UrlMissing) - })?; - let (mut cascade, _action, prompt_options) = repo.config_snapshot().credential_helpers(gix::url::parse( - url.as_ref(), - )?)?; + let url = context + .url + .clone() + .or_else(|| context.to_url()) + .ok_or(Error::Protocol(gix::credentials::protocol::Error::UrlMissing))?; + let (mut cascade, _action, prompt_options) = repo + .config_snapshot() + .credential_helpers(gix::url::parse(url.as_ref())?)?; cascade .invoke( match action { diff --git a/gix-credentials/src/program/main.rs b/gix-credentials/src/program/main.rs index 1c00cb6c70a..ef7556c5977 100644 --- a/gix-credentials/src/program/main.rs +++ b/gix-credentials/src/program/main.rs @@ -92,14 +92,16 @@ pub(crate) mod function { if ctx.url.is_none() && (ctx.protocol.is_none() || ctx.host.is_none()) { return Err(Error::UrlMissing); } - let res = credentials(action, ctx).map_err(|err| Error::Helper { source: Box::new(err) })?; + let res = credentials(action, ctx.clone()).map_err(|err| Error::Helper { source: Box::new(err) })?; match (action, res) { (Action::Get, None) => { - let ctx_for_error = Context::from_bytes(&buf)?; - let url = ctx_for_error.url.clone() + let ctx_for_error = ctx; + let url = ctx_for_error + .url + .clone() .or_else(|| ctx_for_error.to_url()) - .expect("URL is available either directly or via protocol+host"); - return Err(Error::CredentialsMissing { url }) + .expect("URL is available either directly or via protocol+host which we checked for"); + return Err(Error::CredentialsMissing { url }); } (Action::Get, Some(ctx)) => ctx.write_to(stdout)?, (Action::Erase | Action::Store, None) => {} diff --git a/gix-credentials/src/protocol/context/mod.rs b/gix-credentials/src/protocol/context/mod.rs index 329bcae1131..ccbe9e0954a 100644 --- a/gix-credentials/src/protocol/context/mod.rs +++ b/gix-credentials/src/protocol/context/mod.rs @@ -93,14 +93,9 @@ mod mutate { #[allow(clippy::result_large_err)] pub fn destructure_url_in_place(&mut self, use_http_path: bool) -> Result<&mut Self, protocol::Error> { if self.url.is_none() { - // If URL is not present but protocol and host are, construct the URL - if let Some(constructed_url) = self.to_url() { - self.url = Some(constructed_url); - } else { - return Err(protocol::Error::UrlMissing); - } + self.url = Some(self.to_url().ok_or(protocol::Error::UrlMissing)?); } - + let url = gix_url::parse(self.url.as_ref().expect("URL is present after check above").as_ref())?; self.protocol = Some(url.scheme.as_str().into()); self.username = url.user().map(ToOwned::to_owned); diff --git a/gix-credentials/tests/program/main.rs b/gix-credentials/tests/program/main.rs new file mode 100644 index 00000000000..0a6c985e267 --- /dev/null +++ b/gix-credentials/tests/program/main.rs @@ -0,0 +1,94 @@ +use gix_credentials::program::main; +use std::io::Cursor; + +#[derive(Debug, thiserror::Error)] +#[error("Test error")] +struct TestError; + +#[test] +fn protocol_and_host_without_url_is_valid() { + let input = b"protocol=https\nhost=github.com\n"; + let mut output = Vec::new(); + + let mut called = false; + let result = main( + ["get".into()], + Cursor::new(input), + &mut output, + |_action, context| -> Result, TestError> { + assert_eq!(context.protocol.as_deref(), Some("https")); + assert_eq!(context.host.as_deref(), Some("github.com")); + assert_eq!(context.url, None, "the URL isn't automatically populated"); + called = true; + + Ok(None) + }, + ); + + // This should fail because our mock helper returned None (no credentials found) + // but it should NOT fail because of missing URL + match result { + Err(gix_credentials::program::main::Error::CredentialsMissing { .. }) => { + assert!( + called, + "The helper gets called, but as nothing is provided in the function it ulimately fails" + ); + } + other => panic!("Expected CredentialsMissing error, got: {other:?}"), + } +} + +#[test] +fn missing_protocol_with_only_host_or_protocol_fails() { + for input in ["host=github.com\n", "protocol=https\n"] { + let mut output = Vec::new(); + + let mut called = false; + let result = main( + ["get".into()], + Cursor::new(input), + &mut output, + |_action, _context| -> Result, TestError> { + called = true; + Ok(None) + }, + ); + + match result { + Err(gix_credentials::program::main::Error::UrlMissing) => { + assert!(!called, "the context is lacking, hence nothing gets called"); + } + other => panic!("Expected UrlMissing error, got: {other:?}"), + } + } +} + +#[test] +fn url_alone_is_valid() { + let input = b"url=https://github.com\n"; + let mut output = Vec::new(); + + let mut called = false; + let result = main( + ["get".into()], + Cursor::new(input), + &mut output, + |_action, context| -> Result, TestError> { + called = true; + assert_eq!(context.url.unwrap(), "https://github.com"); + assert_eq!(context.host, None, "not auto-populated"); + assert_eq!(context.protocol, None, "not auto-populated"); + + Ok(None) + }, + ); + + // This should fail because our mock helper returned None (no credentials found) + // but it should NOT fail because of missing URL + match result { + Err(gix_credentials::program::main::Error::CredentialsMissing { .. }) => { + assert!(called); + } + other => panic!("Expected CredentialsMissing error, got: {other:?}"), + } +} diff --git a/gix-credentials/tests/program/main_validation.rs b/gix-credentials/tests/program/main_validation.rs deleted file mode 100644 index 83e84437746..00000000000 --- a/gix-credentials/tests/program/main_validation.rs +++ /dev/null @@ -1,101 +0,0 @@ -use gix_credentials::program::main; -use std::io::Cursor; - -#[derive(Debug, thiserror::Error)] -#[error("Test error")] -struct TestError; - -#[test] -fn protocol_and_host_without_url_is_valid() { - let input = b"protocol=https\nhost=github.com\n"; - let mut output = Vec::new(); - - let result = main( - ["get".into()], - Cursor::new(input), - &mut output, - |_action, context| -> Result, TestError> { - // Verify the context has the expected fields - assert_eq!(context.protocol.as_deref(), Some("https")); - assert_eq!(context.host.as_deref(), Some("github.com")); - // Should return None to simulate no credentials found (which is expected in test) - Ok(None) - }, - ); - - // This should fail because our mock helper returned None (no credentials found) - // but it should NOT fail because of missing URL - match result { - Err(gix_credentials::program::main::Error::CredentialsMissing { .. }) => { - // This is the expected error - credentials missing, not URL missing - } - other => panic!("Expected CredentialsMissing error, got: {:?}", other), - } -} - -#[test] -fn missing_protocol_with_host_fails() { - let input = b"host=github.com\n"; - let mut output = Vec::new(); - - let result = main( - ["get".into()], - Cursor::new(input), - &mut output, - |_action, _context| -> Result, TestError> { Ok(None) }, - ); - - match result { - Err(gix_credentials::program::main::Error::UrlMissing) => { - // This is expected - } - other => panic!("Expected UrlMissing error, got: {:?}", other), - } -} - -#[test] -fn missing_host_with_protocol_fails() { - let input = b"protocol=https\n"; - let mut output = Vec::new(); - - let result = main( - ["get".into()], - Cursor::new(input), - &mut output, - |_action, _context| -> Result, TestError> { Ok(None) }, - ); - - match result { - Err(gix_credentials::program::main::Error::UrlMissing) => { - // This is expected - } - other => panic!("Expected UrlMissing error, got: {:?}", other), - } -} - -#[test] -fn url_alone_is_still_valid() { - let input = b"url=https://github.com\n"; - let mut output = Vec::new(); - - let result = main( - ["get".into()], - Cursor::new(input), - &mut output, - |_action, context| -> Result, TestError> { - // Verify the context has the expected fields - assert_eq!(context.url.as_deref().map(|b| &**b), Some("https://github.com".as_bytes())); - // Should return None to simulate no credentials found (which is expected in test) - Ok(None) - }, - ); - - // This should fail because our mock helper returned None (no credentials found) - // but it should NOT fail because of missing URL - match result { - Err(gix_credentials::program::main::Error::CredentialsMissing { .. }) => { - // This is the expected error - credentials missing, not URL missing - } - other => panic!("Expected CredentialsMissing error, got: {:?}", other), - } -} \ No newline at end of file diff --git a/gix-credentials/tests/program/mod.rs b/gix-credentials/tests/program/mod.rs index 393e2c6ee8f..12a307d0979 100644 --- a/gix-credentials/tests/program/mod.rs +++ b/gix-credentials/tests/program/mod.rs @@ -1,2 +1,2 @@ mod from_custom_definition; -mod main_validation; +mod main; diff --git a/gix-credentials/tests/protocol/context.rs b/gix-credentials/tests/protocol/context.rs index 58482094850..e12c4549523 100644 --- a/gix-credentials/tests/protocol/context.rs +++ b/gix-credentials/tests/protocol/context.rs @@ -73,38 +73,28 @@ mod destructure_url_in_place { ); } - #[test] - fn protocol_and_host_without_url_constructs_url_first() { - let mut ctx = Context { - protocol: Some("https".into()), - host: Some("github.com".into()), - ..Default::default() - }; - ctx.destructure_url_in_place(false).expect("should work with protocol and host"); - - // URL should be constructed from protocol and host - assert_eq!(ctx.url.as_deref().map(|b| &**b), Some("https://github.com".as_bytes())); - // Original fields should be preserved - assert_eq!(ctx.protocol.as_deref(), Some("https")); - assert_eq!(ctx.host.as_deref(), Some("github.com")); - } - #[test] fn protocol_and_host_with_path_without_url_constructs_full_url() { let mut ctx = Context { protocol: Some("https".into()), host: Some("github.com".into()), path: Some("org/repo".into()), + username: Some("user".into()), + password: Some("pass-to-be-ignored".into()), ..Default::default() }; - ctx.destructure_url_in_place(false).expect("should work with protocol, host and path"); - - // URL should be constructed from all provided fields - assert_eq!(ctx.url.as_deref().map(|b| &**b), Some("https://github.com/org/repo".as_bytes())); + ctx.destructure_url_in_place(false) + .expect("should work with protocol, host and path"); + + assert_eq!( + ctx.url.unwrap(), + "https://user@github.com/org/repo", + "URL should be constructed from all provided fields, except password" + ); // Original fields should be preserved assert_eq!(ctx.protocol.as_deref(), Some("https")); assert_eq!(ctx.host.as_deref(), Some("github.com")); - assert_eq!(ctx.path.as_deref().map(|b| &**b), Some("org/repo".as_bytes())); + assert_eq!(ctx.path.unwrap(), "org/repo"); } #[test] @@ -113,7 +103,10 @@ mod destructure_url_in_place { host: Some("github.com".into()), ..Default::default() }; - assert!(ctx_no_protocol.destructure_url_in_place(false).is_err()); + assert_eq!( + ctx_no_protocol.destructure_url_in_place(false).unwrap_err().to_string(), + "Either 'url' field or both 'protocol' and 'host' fields must be provided" + ); let mut ctx_no_host = Context { protocol: Some("https".into()),