History

Merge pull request #2261 from GitoxideLabs/copilot/replace-url-dependency

Replace url crate with minimal custom parser in gix-url

Sebastian Thiel committed 11 months ago

Commit 896cb4d2 parents dc2794a a9a4d4d9 files · +313 −41Browse files

Unified · Split

9 files
  1. Cargo.lock +0 −1
  2. gix-credentials/src/protocol/context/mod.rs +6 −1
  3. gix-url/Cargo.toml +0 −1
  4. gix-url/src/lib.rs +3 −0
  5. gix-url/src/parse.rs +37 −24
  6. gix-url/src/scheme.rs +11 −0
  7. gix-url/src/simple_url.rs +250 −0
  8. gix-url/tests/url/baseline.rs +2 −2
  9. gix-url/tests/url/parse/file.rs +4 −12
Cargo.lock+0−1
@@ -2510,7 +2510,6 @@ dependencies = [ · 2509 unmodified lines
25102510 "percent-encoding",
25112511 "serde",
25122512 "thiserror 2.0.17",
2513− "url",
25142513 ]
25152514
25162515 [[package]]
gix-credentials/src/protocol/context/mod.rs+6−1
@@ -101,7 +101,12 @@ mod mutate { · 100 unmodified lines
101101 self.username = url.user().map(ToOwned::to_owned);
102102 self.password = url.password().map(ToOwned::to_owned);
103103 self.host = url.host().map(ToOwned::to_owned).map(|mut host| {
104− if let Some(port) = url.port {
104+ let port = url.port.filter(|port| {
105+ url.scheme
106+ .default_port()
107+ .is_none_or(|default_port| *port != default_port)
108+ });
109+ if let Some(port) = port {
105110 use std::fmt::Write;
106111 write!(host, ":{port}").expect("infallible");
107112 }
gix-url/Cargo.toml+0−1
@@ -24,7 +24,6 @@ gix-path = { version = "^0.10.21", path = "../gix-path" } · 23 unmodified lines
2424
2525 serde = { version = "1.0.114", optional = true, default-features = false, features = ["std", "derive"] }
2626 thiserror = "2.0.17"
27−url = "2.5.2"
2827 bstr = { version = "1.12.0", default-features = false, features = ["std"] }
2928 percent-encoding = "2.3.1"
3029
gix-url/src/lib.rs+3−0
@@ -22,6 +22,9 @@ mod impls; · 21 unmodified lines
2222 ///
2323 pub mod parse;
2424
25+/// Minimal URL parser to replace the `url` crate dependency
26+mod simple_url;
27+
2528 /// Parse the given `bytes` as a [git url](Url).
2629 ///
2730 /// # Note
gix-url/src/parse.rs+37−24
@@ -19,7 +19,7 @@ pub enum Error { · 18 unmodified lines
1919 Url {
2020 url: String,
2121 kind: UrlKind,
22− source: url::ParseError,
22+ source: crate::simple_url::UrlParseError,
2323 },
2424
2525 #[error("The host portion of the following URL is too long ({} bytes, {len} bytes total): {truncated_url:?}", truncated_url.len())]
@@ -99,30 +99,33 @@ pub(crate) fn url(input: &BStr, protocol_end: usize) -> Result<crate::Url, Error · 73 unmodified lines
9999 });
100100 }
101101 let (input, url) = input_to_utf8_and_url(input, UrlKind::Url)?;
102− let scheme = url.scheme().into();
102+ let scheme = Scheme::from(url.scheme.as_str());
103103
104− if matches!(scheme, Scheme::Git | Scheme::Ssh) && url.path().is_empty() {
104+ if matches!(scheme, Scheme::Git | Scheme::Ssh) && url.path.is_empty() {
105105 return Err(Error::MissingRepositoryPath {
106106 url: input.into(),
107107 kind: UrlKind::Url,
108108 });
109109 }
110110
111− if url.cannot_be_a_base() {
112− return Err(Error::RelativeUrl { url: input.to_owned() });
113− }
111+ // Normalize empty path to "/" for http/https URLs only
112+ let path = if url.path.is_empty() && matches!(scheme, Scheme::Http | Scheme::Https) {
113+ "/".into()
114+ } else {
115+ url.path.into()
116+ };
114117
115118 Ok(crate::Url {
116119 serialize_alternative_form: false,
117120 scheme,
118121 user: url_user(&url, UrlKind::Url)?,
119122 password: url
120− .password()
123+ .password
121124 .map(|s| percent_decoded_utf8(s, UrlKind::Url))
122125 .transpose()?,
123− host: url.host_str().map(Into::into),
124− port: url.port(),
125− path: url.path().into(),
126+ host: url.host,
127+ port: url.port,
128+ path,
126129 })
127130 }
128131
@@ -156,7 +159,8 @@ pub(crate) fn scp(input: &BStr, colon: usize) -> Result<crate::Url, Error> { · 27 unmodified lines
156159 // should never differ in any other way (ssh URLs should not contain a query or fragment part).
157160 // To avoid the various off-by-one errors caused by the `/` characters, we keep using the path
158161 // determined above and can therefore skip parsing it here as well.
159− let url = url::Url::parse(&format!("ssh://{host}")).map_err(|source| Error::Url {
162+ let url_string = format!("ssh://{host}");
163+ let url = crate::simple_url::ParsedUrl::parse(&url_string).map_err(|source| Error::Url {
160164 url: input.to_owned(),
161165 kind: UrlKind::Scp,
162166 source,
@@ -164,23 +168,23 @@ pub(crate) fn scp(input: &BStr, colon: usize) -> Result<crate::Url, Error> { · 1 unmodified line
164168
165169 Ok(crate::Url {
166170 serialize_alternative_form: true,
167− scheme: url.scheme().into(),
171+ scheme: Scheme::from(url.scheme.as_str()),
168172 user: url_user(&url, UrlKind::Scp)?,
169173 password: url
170− .password()
174+ .password
171175 .map(|s| percent_decoded_utf8(s, UrlKind::Scp))
172176 .transpose()?,
173− host: url.host_str().map(Into::into),
174− port: url.port(),
177+ host: url.host,
178+ port: url.port,
175179 path: path.into(),
176180 })
177181 }
178182
179−fn url_user(url: &url::Url, kind: UrlKind) -> Result<Option<String>, Error> {
180− if url.username().is_empty() && url.password().is_none() {
183+fn url_user(url: &crate::simple_url::ParsedUrl<'_>, kind: UrlKind) -> Result<Option<String>, Error> {
184+ if url.username.is_empty() && url.password.is_none() {
181185 Ok(None)
182186 } else {
183− Ok(Some(percent_decoded_utf8(url.username(), kind)?))
187+ Ok(Some(percent_decoded_utf8(url.username, kind)?))
184188 }
185189 }
186190
@@ -269,13 +273,22 @@ fn input_to_utf8(input: &BStr, kind: UrlKind) -> Result<&str, Error> { · 82 unmodified lines
269273 })
270274 }
271275
272−fn input_to_utf8_and_url(input: &BStr, kind: UrlKind) -> Result<(&str, url::Url), Error> {
276+fn input_to_utf8_and_url(input: &BStr, kind: UrlKind) -> Result<(&str, crate::simple_url::ParsedUrl<'_>), Error> {
273277 let input = input_to_utf8(input, kind)?;
274− url::Url::parse(input)
278+ crate::simple_url::ParsedUrl::parse(input)
275279 .map(|url| (input, url))
276− .map_err(|source| Error::Url {
277− url: input.to_owned(),
278− kind,
279− source,
280+ .map_err(|source| {
281+ // If the parser rejected it as RelativeUrlWithoutBase, map to Error::RelativeUrl
282+ // to match the expected error type for malformed URLs like "invalid:://"
283+ match source {
284+ crate::simple_url::UrlParseError::RelativeUrlWithoutBase => {
285+ Error::RelativeUrl { url: input.to_owned() }
286+ }
287+ _ => Error::Url {
288+ url: input.to_owned(),
289+ kind,
290+ source,
291+ },
292+ }
280293 })
281294 }
gix-url/src/scheme.rs+11−0
@@ -48,6 +48,17 @@ impl Scheme { · 47 unmodified lines
4848 Ext(name) => name.as_str(),
4949 }
5050 }
51+
52+ /// Return the default port for this scheme, or `None` if it is not known.
53+ pub fn default_port(&self) -> Option<u16> {
54+ match self {
55+ Scheme::Http => Some(80),
56+ Scheme::Https => Some(443),
57+ Scheme::Ssh => Some(22),
58+ Scheme::Git => Some(9418),
59+ _ => None,
60+ }
61+ }
5162 }
5263
5364 impl std::fmt::Display for Scheme {
gix-url/src/simple_url.rs+250−0
@@ -0,0 +1,250 @@
1+/// A minimal URL parser that extracts only what we need for git URLs.
2+/// This is a replacement for the `url` crate dependency.
3+#[derive(Debug)]
4+pub(crate) struct ParsedUrl<'a> {
5+ pub scheme: String, // Owned to allow normalization to lowercase
6+ pub username: &'a str,
7+ pub password: Option<&'a str>,
8+ pub host: Option<String>, // Owned to allow normalization to lowercase
9+ pub port: Option<u16>,
10+ pub path: &'a str,
11+}
12+
13+/// Minimal parse error type to replace url::ParseError
14+#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)]
15+#[allow(missing_docs)]
16+pub enum UrlParseError {
17+ #[error("relative URL without a base")]
18+ RelativeUrlWithoutBase,
19+ #[error("invalid port number - must be between 1-65535")]
20+ InvalidPort,
21+ #[error("invalid domain character")]
22+ InvalidDomainCharacter,
23+ #[error("Scheme requires host")]
24+ SchemeRequiresHost,
25+}
26+
27+/// Check if a character is valid in a URL scheme.
28+/// Valid scheme characters: alphanumeric, +, -, or .
29+fn is_valid_scheme_char(c: char) -> bool {
30+ c.is_ascii_alphanumeric() || c == '+' || c == '-' || c == '.'
31+}
32+
33+impl<'a> ParsedUrl<'a> {
34+ /// Parse a URL string into its components.
35+ /// Expected format: scheme://[user[:password]@]host[:port]/path
36+ pub(crate) fn parse(input: &'a str) -> Result<Self, UrlParseError> {
37+ // Find scheme by looking for first ':'
38+ let first_colon = input.find(':').ok_or(UrlParseError::RelativeUrlWithoutBase)?;
39+ let scheme_str = &input[..first_colon];
40+ // Normalize scheme to lowercase for case-insensitive matching (matches url crate behavior)
41+ let scheme = scheme_str.to_ascii_lowercase();
42+ let Some(after_scheme) = input[first_colon..].strip_prefix("://") else {
43+ return Err(UrlParseError::RelativeUrlWithoutBase);
44+ };
45+
46+ // Check for relative URL (scheme without proper authority)
47+ if scheme_str.is_empty() {
48+ return Err(UrlParseError::RelativeUrlWithoutBase);
49+ }
50+
51+ // Validate scheme characters (check original before lowercase conversion)
52+ if !scheme_str.chars().all(is_valid_scheme_char) {
53+ return Err(UrlParseError::RelativeUrlWithoutBase);
54+ }
55+
56+ // Find path start (first '/' after scheme)
57+ let path_start = after_scheme.find('/').unwrap_or(after_scheme.len());
58+ let authority = &after_scheme[..path_start];
59+ let path = if path_start < after_scheme.len() {
60+ &after_scheme[path_start..]
61+ } else {
62+ // No path specified - leave empty (caller can default to / if needed)
63+ ""
64+ };
65+
66+ // Parse authority: [user[:password]@]host[:port]
67+ let (username, password, host, port) = if let Some(at_pos) = authority.rfind('@') {
68+ // Has user info
69+ let user_info = &authority[..at_pos];
70+ let host_port = &authority[at_pos + 1..];
71+
72+ let (user, pass) = if let Some(colon_pos) = user_info.find(':') {
73+ let pass_str = &user_info[colon_pos + 1..];
74+ // Treat empty password as None
75+ let pass = if pass_str.is_empty() { None } else { Some(pass_str) };
76+ (&user_info[..colon_pos], pass)
77+ } else {
78+ (user_info, None)
79+ };
80+
81+ let (h, p) = Self::parse_host_port(host_port)?;
82+ // If we have user info, we must have a host
83+ if h.is_none() {
84+ return Err(UrlParseError::InvalidDomainCharacter);
85+ }
86+ (user, pass, h, p)
87+ } else {
88+ // No user info
89+ let (h, p) = Self::parse_host_port(authority)?;
90+ ("", None, h, p)
91+ };
92+
93+ // Standard schemes (http, https, git, ssh) require a host
94+ // Scheme is already lowercase at this point
95+ let requires_host = matches!(scheme.as_str(), "http" | "https" | "git" | "ssh" | "ftp" | "ftps");
96+ if requires_host && host.is_none() {
97+ return Err(UrlParseError::SchemeRequiresHost);
98+ }
99+
100+ Ok(ParsedUrl {
101+ scheme,
102+ username,
103+ password,
104+ host,
105+ port,
106+ path,
107+ })
108+ }
109+
110+ fn parse_host_port(host_port: &str) -> Result<(Option<String>, Option<u16>), UrlParseError> {
111+ if host_port.is_empty() {
112+ return Ok((None, None));
113+ }
114+
115+ // Handle IPv6 addresses: [::1] or [::1]:port
116+ if host_port.starts_with('[') {
117+ if let Some(bracket_end) = host_port.find(']') {
118+ // IPv6 addresses are case-insensitive, normalize to lowercase
119+ let host = Some(host_port[..=bracket_end].to_ascii_lowercase());
120+ let remaining = &host_port[bracket_end + 1..];
121+
122+ if remaining.is_empty() {
123+ return Ok((host, None));
124+ } else if let Some(port_str) = remaining.strip_prefix(':') {
125+ let port = port_str.parse::<u16>().map_err(|_| UrlParseError::InvalidPort)?;
126+ // Validate port is in valid range (1-65535, port 0 is invalid)
127+ if port == 0 {
128+ return Err(UrlParseError::InvalidPort);
129+ }
130+ return Ok((host, Some(port)));
131+ } else {
132+ return Err(UrlParseError::InvalidDomainCharacter);
133+ }
134+ } else {
135+ return Err(UrlParseError::InvalidDomainCharacter);
136+ }
137+ }
138+
139+ // Handle regular host:port
140+ // Use rfind to handle IPv6 addresses without brackets (edge case)
141+ if let Some(colon_pos) = host_port.rfind(':') {
142+ // Check if this looks like a port (all digits after colon)
143+ let potential_port = &host_port[colon_pos + 1..];
144+ if potential_port.is_empty() {
145+ // Empty port like "host:" - strip the trailing colon
146+ let host_str = &host_port[..colon_pos];
147+ return Ok((Some(Self::normalize_hostname(host_str)?), None));
148+ } else if potential_port.chars().all(|c| c.is_ascii_digit()) {
149+ let host_str = &host_port[..colon_pos];
150+ let host = Self::normalize_hostname(host_str)?;
151+ let port = potential_port.parse::<u16>().map_err(|_| UrlParseError::InvalidPort)?;
152+ // Validate port is in valid range (1-65535, port 0 is invalid)
153+ if port == 0 {
154+ return Err(UrlParseError::InvalidPort);
155+ }
156+ return Ok((Some(host), Some(port)));
157+ }
158+ }
159+
160+ // No port, just host
161+ Ok((Some(Self::normalize_hostname(host_port)?), None))
162+ }
163+
164+ /// Check if a string looks like a valid DNS hostname (for normalization purposes)
165+ /// Valid DNS names contain only alphanumeric, hyphens, dots, underscores, and wildcards
166+ fn is_normalizable_hostname(host: &str) -> bool {
167+ // Allow alphanumeric, -, ., _, and * (for patterns)
168+ host.chars()
169+ .all(|c| c.is_ascii_alphanumeric() || matches!(c, '-' | '.' | '_' | '*'))
170+ }
171+
172+ /// Validate and possibly normalize a hostname
173+ /// Valid DNS hostnames are normalized to lowercase
174+ /// Invalid strings (like injection attempts) are preserved as-is but ? is rejected
175+ fn normalize_hostname(host: &str) -> Result<String, UrlParseError> {
176+ // Reject ? character which git's url parser rejects
177+ if host.contains('?') {
178+ return Err(UrlParseError::InvalidDomainCharacter);
179+ }
180+
181+ // Only normalize if it looks like a valid DNS hostname
182+ // Preserve case for security checks if it contains special characters
183+ if Self::is_normalizable_hostname(host) {
184+ Ok(host.to_ascii_lowercase())
185+ } else {
186+ Ok(host.to_owned())
187+ }
188+ }
189+}
190+
191+#[cfg(test)]
192+mod tests {
193+ use super::*;
194+
195+ #[test]
196+ fn test_simple_url() {
197+ let url = ParsedUrl::parse("http://example.com/path").unwrap();
198+ assert_eq!(url.scheme, "http");
199+ assert_eq!(url.host.as_deref(), Some("example.com"));
200+ assert_eq!(url.path, "/path");
201+ assert_eq!(url.username, "");
202+ assert_eq!(url.password, None);
203+ assert_eq!(url.port, None);
204+ }
205+
206+ #[test]
207+ fn test_url_with_port() {
208+ let url = ParsedUrl::parse("http://example.com:8080/path").unwrap();
209+ assert_eq!(url.scheme, "http");
210+ assert_eq!(url.host.as_deref(), Some("example.com"));
211+ assert_eq!(url.port, Some(8080));
212+ assert_eq!(url.path, "/path");
213+ }
214+
215+ #[test]
216+ fn test_url_with_user() {
217+ let url = ParsedUrl::parse("http://user@example.com/path").unwrap();
218+ assert_eq!(url.scheme, "http");
219+ assert_eq!(url.username, "user");
220+ assert_eq!(url.host.as_deref(), Some("example.com"));
221+ assert_eq!(url.path, "/path");
222+ }
223+
224+ #[test]
225+ fn test_url_with_user_and_password() {
226+ let url = ParsedUrl::parse("http://user:pass@example.com/path").unwrap();
227+ assert_eq!(url.scheme, "http");
228+ assert_eq!(url.username, "user");
229+ assert_eq!(url.password, Some("pass"));
230+ assert_eq!(url.host.as_deref(), Some("example.com"));
231+ assert_eq!(url.path, "/path");
232+ }
233+
234+ #[test]
235+ fn test_url_with_ipv6() {
236+ let url = ParsedUrl::parse("http://[::1]/path").unwrap();
237+ assert_eq!(url.scheme, "http");
238+ assert_eq!(url.host.as_deref(), Some("[::1]"));
239+ assert_eq!(url.path, "/path");
240+ }
241+
242+ #[test]
243+ fn test_url_with_ipv6_and_port() {
244+ let url = ParsedUrl::parse("http://[::1]:8080/path").unwrap();
245+ assert_eq!(url.scheme, "http");
246+ assert_eq!(url.host.as_deref(), Some("[::1]"));
247+ assert_eq!(url.port, Some(8080));
248+ assert_eq!(url.path, "/path");
249+ }
250+}
gix-url/tests/url/baseline.rs+2−2
@@ -185,8 +185,8 @@ mod baseline { · 184 unmodified lines
185185
186186 pub fn max_num_failures(&self) -> usize {
187187 match self {
188− Kind::Unix => 198,
189− Kind::Windows => 198 + 6,
188+ Kind::Unix => 195,
189+ Kind::Windows => 195 + 6,
190190 }
191191 }
192192
gix-url/tests/url/parse/file.rs+4−12
@@ -141,13 +141,9 @@ mod windows { · 140 unmodified lines
141141
142142 #[test]
143143 fn url_from_absolute_path() -> crate::Result {
144+ // Test with a Windows path directly instead of using url::Url::from_directory_path
144145 assert_url(
145− url::Url::from_directory_path(r"c:\users\1")
146− .expect("valid")
147− .to_file_path()
148− .expect("valid path")
149− .to_string_lossy()
150− .as_ref(),
146+ r"C:\users\1\",
151147 url_alternate(Scheme::File, None, None, None, br"C:\users\1\"),
152148 )?;
153149 // A special hack to support URLs on windows that are prefixed with `/` even though absolute.
@@ -189,13 +185,9 @@ mod unix { · 35 unmodified lines
189185
190186 #[test]
191187 fn url_from_absolute_path() -> crate::Result {
188+ // Test with a simple file path instead of using url::Url::from_directory_path
192189 assert_url_roundtrip(
193− url::Url::from_directory_path("/users/foo")
194− .expect("valid")
195− .to_file_path()
196− .expect("valid path")
197− .to_string_lossy()
198− .as_ref(),
190+ "/users/foo/",
199191 url_alternate(Scheme::File, None, None, None, b"/users/foo/"),
200192 )
201193 }