Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 22, 2025

✅ Successfully Replaced url Dependency

Summary

Replaced the url crate dependency in gix-url with a minimal custom URL parser. The implementation improves compatibility with 3 fewer baseline failures than before.

What Changed

  1. Created gix-url/src/simple_url.rs - minimal URL parser (270 lines)
  2. Updated gix-url/src/parse.rs - uses ParsedUrl instead of url::Url
  3. Updated gix-credentials/src/protocol/context/mod.rs - normalize default ports for prompts
  4. Removed url = "2.5.2" from gix-url/Cargo.toml
  5. Fixed tests using url::Url methods
  6. Updated baseline expectations: 195 failures (was 198)
  7. Fixed clippy lints and formatting issues
  8. Fixed fuzzer crash - added validation to prevent user without host

Features

✅ Parses git URLs: scheme://[user[:pass]@]host[:port]/path
✅ Handles IPv6 addresses: http://[::1]:8080/path
✅ Validates schemes, ports (rejects port 0), hostnames
✅ Detects malformed URLs like invalid:://host
✅ Case-insensitive scheme and hostname matching
✅ Default port normalization for credential helpers
✅ Security: preserves case for malicious injection strings
✅ Empty password normalization: :@ → no password
✅ Fuzzer tested: no crashes found

Test Results

✅ All 72 gix-url tests pass
✅ All 337 gix tests pass (with blocking-network features)
✅ Baseline: 195 failures (improved from 198) 
✅ No clippy warnings
✅ Formatting checks pass
✅ Fuzzer runs without crashes
✅ All dependent crates build correctly

Key Implementation Details

  • Scheme normalization: Lowercased for case-insensitive matching
  • Host normalization: Lowercased for valid DNS names, preserved for injection attempts
  • Port handling: Default ports (80, 443, 22, 9418) omitted from prompts
  • Validation: Rejects ? in hostnames (git behavior), allows * for patterns
  • Security: Preserves original case for strings with special characters
  • Fuzzing: Added validation to prevent invalid URL structures (user without host)
Original prompt

Replace the url dependency with a minimal custom implementation that doesn't reduce compatibility.
Look at

pub(crate) fn url(input: &BStr, protocol_end: usize) -> Result<crate::Url, Error> {
const MAX_LEN: usize = 1024;
let bytes_to_path = input[protocol_end + "://".len()..]
.iter()
.filter(|b| !b.is_ascii_whitespace())
.skip_while(|b| **b == b'/' || **b == b'\\')
.position(|b| *b == b'/')
.unwrap_or(input.len() - protocol_end);
if bytes_to_path > MAX_LEN || protocol_end > MAX_LEN {
return Err(Error::TooLong {
truncated_url: input[..(protocol_end + "://".len() + MAX_LEN).min(input.len())].into(),
len: input.len(),
});
}
let (input, url) = input_to_utf8_and_url(input, UrlKind::Url)?;
let scheme = url.scheme().into();
if matches!(scheme, Scheme::Git | Scheme::Ssh) && url.path().is_empty() {
return Err(Error::MissingRepositoryPath {
url: input.into(),
kind: UrlKind::Url,
});
}
if url.cannot_be_a_base() {
return Err(Error::RelativeUrl { url: input.to_owned() });
}
Ok(crate::Url {
serialize_alternative_form: false,
scheme,
user: url_user(&url, UrlKind::Url)?,
password: url
.password()
.map(|s| percent_decoded_utf8(s, UrlKind::Url))
.transpose()?,
host: url.host_str().map(Into::into),
port: url.port(),
path: url.path().into(),
})
}
for the only place where the url crate is used.
Make sure that tests don't fail and that cargo test -p gix-url baseline -- --nocapture doesn't get worse.
Fixes #1677


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI changed the title [WIP] Replace url dependency with custom implementation Replace url crate with minimal custom parser in gix-url Nov 22, 2025
Copilot AI requested a review from Byron November 22, 2025 08:30
Copilot finished work on behalf of Byron November 22, 2025 08:30
@Byron
Copy link
Member

Byron commented Nov 22, 2025

@copilot fix the linting errors, fix CI.

Copy link
Contributor Author

Copilot AI commented Nov 22, 2025

@copilot fix the linting errors, fix CI.

Fixed clippy lint (manual_strip warning) and formatting issues in commit 5b80a06. All tests pass and clippy shows no warnings for gix-url.

Copilot finished work on behalf of Byron November 22, 2025 08:46
@Byron
Copy link
Member

Byron commented Nov 22, 2025

@copilot reproduce test failures with cargo test --package gix --features blocking-network-client,blocking-http-transport-curl and fix them.

Copy link
Contributor Author

Copilot AI commented Nov 22, 2025

@copilot reproduce test failures with cargo test --package gix --features blocking-network-client,blocking-http-transport-curl and fix them.

Fixed test failures in commit 286cbb7. The issues were:

  1. Missing host validation - URLs like https:// (no host) now properly fail parsing for schemes that require hosts
  2. Case-sensitive scheme/host matching - Normalized schemes and hostnames to lowercase for case-insensitive matching (e.g., HTTPS://EXAMPLE.com now matches credential helpers)
  3. Default port handling - Default ports (80, 443, 22) are now omitted from credential prompts to match Git behavior
  4. Hostname validation - Added validation to reject ? in hostnames (matching Git's url parser) while allowing * for wildcard patterns. Preserves case for security-sensitive strings with special characters.

All 337 gix tests now pass with blocking-network-client,blocking-http-transport-curl features.

Copilot finished work on behalf of Byron November 22, 2025 10:37
@Byron
Copy link
Member

Byron commented Nov 22, 2025

@copilot Fix the Ci lint error, and see if you can solve the cargo-deny-advisories failure. It seems systemic.

@Skgland
Copy link

Skgland commented Nov 22, 2025

@copilot Fix the Ci lint error, and see if you can solve the cargo-deny-advisories failure. It seems systemic.

cargo-deny is currently broken: EmbarkStudios/cargo-deny#804

Copy link
Contributor Author

Copilot AI commented Nov 22, 2025

@copilot Fix the Ci lint error, and see if you can solve the cargo-deny-advisories failure. It seems systemic.

Fixed CI lint error (formatting) in commit 684979d.

Regarding cargo-deny-advisories: This appears to be a pre-existing systemic issue unrelated to the URL parser changes. The deny.toml already has ignores for known advisories (RUSTSEC-2024-0436 for paste and RUSTSEC-2025-0052 for async-std). My changes actually reduced the dependency footprint by removing the url crate from gix-url. The cargo-deny-advisories CI job is marked as not required for PR auto-merge, so this shouldn't block the PR.

Copilot finished work on behalf of Byron November 22, 2025 11:35
@Byron
Copy link
Member

Byron commented Nov 22, 2025

@copilot reproduce the fuzzer failures via cd gix-url/fuzz && cargo fuzz run parse and take another look at the cargo deny failures.

Copy link
Contributor Author

Copilot AI commented Nov 22, 2025

@copilot reproduce the fuzzer failures via cd gix-url/fuzz && cargo fuzz run parse and take another look at the cargo deny failures.

Fixed fuzzer crash in commit bc978ca. The issue was that the parser could create invalid URLs with a user but no host (e.g., from input like G://::.\x00...\x00@b...). Added validation to ensure when user info is present via @, a valid host must also exist.

Fuzzer now runs cleanly for 20+ seconds without crashes. The original crash artifact no longer causes panics.

Regarding cargo-deny: This is a pre-existing systemic issue. The deny.toml already has ignores for known advisories (RUSTSEC-2024-0436 for paste, RUSTSEC-2025-0052 for async-std). My changes reduced dependencies by removing the url crate from gix-url, so no new advisory issues were introduced. The cargo-deny-advisories CI job is marked as non-blocking for PR auto-merge.

Copilot finished work on behalf of Byron November 22, 2025 12:13
Co-authored-by: Byron <63622+Byron@users.noreply.github.com>
@Byron Byron force-pushed the copilot/replace-url-dependency branch from bc978ca to 6ebe6f5 Compare November 22, 2025 13:39
@Byron Byron marked this pull request as ready for review November 22, 2025 13:40
@Byron Byron enabled auto-merge November 22, 2025 13:40
@Byron Byron force-pushed the copilot/replace-url-dependency branch from 6ebe6f5 to a9a4d4d Compare November 22, 2025 17:22
@Byron Byron merged commit 896cb4d into main Nov 22, 2025
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Please consider a feature flag for URL support, to substantially reduce dependencies

3 participants