diff --git a/idris-dependents/README.adoc b/idris-dependents/README.adoc index 81f829ac..efd1b454 100644 --- a/idris-dependents/README.adoc +++ b/idris-dependents/README.adoc @@ -34,7 +34,7 @@ cargo run -- parse-ipkg path/to/pkg.ipkg # offline: name + depends as JSON + [cols="1,4"] |=== -|HIGH |`pack.toml` or `.gitmodules` points at one of our GitHub owners +|HIGH |`pack.toml` or `.gitmodules` points at one of our GitHub owners (GitHub must be the URL host; relative submodule URLs are resolved against the dependent repository) |MEDIUM |an `.ipkg` `depends` on a distinctive package name of ours |LOW |only ambiguous names (`proven`, `trope`) with no corroboration |=== diff --git a/idris-dependents/src/ipkg.rs b/idris-dependents/src/ipkg.rs index 2c16a310..663c8a31 100644 --- a/idris-dependents/src/ipkg.rs +++ b/idris-dependents/src/ipkg.rs @@ -108,13 +108,11 @@ pub fn lex(src: &str) -> (Vec, usize) { continue; } if c == '"' { - if let Some(end) = string_end(&s, i) { - toks.push(Tok::Str(s[i + 1..end - 1].iter().collect())); - i = end; - } else { - skipped += 1; - i += 1; - } + // An unterminated string runs to end of input, so its contents are + // never read as package syntax. + let end = string_end(&s, i).unwrap_or(s.len() + 1); + toks.push(Tok::Str(s[i + 1..end - 1].iter().collect())); + i = end; continue; } let (tok, len) = match (c, at(i + 1)) { diff --git a/idris-dependents/src/refs.rs b/idris-dependents/src/refs.rs index 5e87859f..3fdfc199 100644 --- a/idris-dependents/src/refs.rs +++ b/idris-dependents/src/refs.rs @@ -11,32 +11,74 @@ pub struct PackRef { pub url: String, } -/// Match a watched owner in a GitHub-style repository reference, including -/// HTTPS, SSH, and scp forms. Owner matching is ASCII case-insensitive and -/// requires a following `/`. -/// -/// This is a textual check, not full URL validation: `github.com` may follow -/// `/`, `@`, or `.` (or start the input), so subdomains and embedded paths can match. -pub fn points_at(url: &str, owners: &[String]) -> bool { - let url = url.to_ascii_lowercase(); - let mut from = 0; - while let Some(k) = url[from..].find("github.com") { - let at = from + k; - let host_start = at == 0 || matches!(url.as_bytes()[at - 1], b'/' | b'@' | b'.'); - let rest = &url[at + "github.com".len()..]; - if host_start && (rest.starts_with('/') || rest.starts_with(':')) { - let path = &rest[1..]; - if owners.iter().any(|o| { - let o = o.to_ascii_lowercase(); - path.strip_prefix(o.as_str()) - .is_some_and(|r| r.starts_with('/')) - }) { - return true; +/// The repository path of a GitHub URL (`scheme://[user@]host[:port]/path` or +/// scp-style `[user@]host:path`), lower-cased, when the host is GitHub itself. +fn github_path(url: &str) -> Option { + let url = url.trim().to_ascii_lowercase(); + let (authority, path) = match url.find("://") { + Some(i) => { + let rest = &url[i + 3..]; + // The authority ends at the first `/`, `?` or `#` (RFC 3986), so + // `https://evil.example#@github.com/...` has host `evil.example`. + let end = rest.find(['/', '?', '#'])?; + if !rest[end..].starts_with('/') { + return None; + } + (&rest[..end], &rest[end + 1..]) + } + None => { + let colon = url.find(':')?; + if url[..colon].contains('/') { + return None; } + (&url[..colon], url[colon + 1..].trim_start_matches('/')) + } + }; + let host = authority.rsplit('@').next()?.split(':').next()?; + matches!(host, "github.com" | "www.github.com").then(|| path.to_string()) +} + +/// Match a watched owner in a GitHub repository URL (HTTPS, SSH or scp form). +/// GitHub must be the URL's host, not merely appear in its path, and the owner +/// must be followed by a repository name. Matching is ASCII case-insensitive. +pub fn points_at(url: &str, owners: &[String]) -> bool { + github_path(url).is_some_and(|path| { + // A query or fragment is not part of the repository path. + let path = path.split(['?', '#']).next().unwrap_or(""); + let mut parts = path.split('/'); + let (Some(owner), Some(repo)) = (parts.next(), parts.next()) else { + return false; + }; + !repo.is_empty() && owners.iter().any(|o| o.eq_ignore_ascii_case(owner)) + }) +} + +/// Resolve a submodule URL relative to its superproject (`../x` and `./x`, as +/// git does against the superproject's remote), taking the remote to be +/// `https://github.com/`. Absolute URLs are returned unchanged. +/// Like git, `../` may climb past `owner/repo` into the host part, so +/// `../../../github.com/o/r` from `a/b` is `https://github.com/o/r`; climbing +/// past the host yields `None` (git's result there is never a GitHub URL). +fn resolve_submodule_url(url: &str, superproject: Option<&str>) -> Option { + if !(url.starts_with("../") || url.starts_with("./")) { + return Some(url.to_string()); + } + let mut base: Vec<&str> = std::iter::once("github.com") + .chain(superproject?.split('/').filter(|s| !s.is_empty())) + .collect(); + let mut rest = url; + loop { + if let Some(r) = rest.strip_prefix("../") { + base.pop()?; + rest = r; + } else if let Some(r) = rest.strip_prefix("./") { + rest = r; + } else { + break; } - from = at + 1; } - false + base.push(rest); + Some(format!("https://{}", base.join("/"))) } /// `pack.toml` custom entries pointing at `owners`, in file order. @@ -68,20 +110,35 @@ pub fn pack_toml_refs(text: &str, owners: &[String]) -> Result, tom /// Extract URLs matching `owners` from trimmed `url =` lines, in input order. /// Double-quoted values end at the next quote; unquoted values end at `#` or /// `;`. Sections and escapes are not interpreted, and duplicates are retained. +/// Relative URLs cannot be resolved without the superproject, so they are +/// skipped; see [`gitmodules_refs_in`]. pub fn gitmodules_refs(text: &str, owners: &[String]) -> Vec { + gitmodules_refs_in(text, owners, None) +} + +/// Like [`gitmodules_refs`], resolving relative submodule URLs against the +/// GitHub repository `superproject` (`owner/repo`). Keys are matched +/// case-insensitively, as git config does; relative matches are returned +/// resolved to absolute URLs. +pub fn gitmodules_refs_in( + text: &str, + owners: &[String], + superproject: Option<&str>, +) -> Vec { text.lines() .filter_map(|line| { let line = line.trim(); - let value = line - .strip_prefix("url")? - .trim_start() - .strip_prefix('=')? - .trim(); + let key = line.get(..3)?; + if !key.eq_ignore_ascii_case("url") { + return None; + } + let value = line[3..].trim_start().strip_prefix('=')?.trim(); let value = match value.strip_prefix('"') { Some(q) => q.split('"').next().unwrap_or(""), None => value.split(['#', ';']).next().unwrap_or("").trim(), }; - points_at(value, owners).then(|| value.to_string()) + let url = resolve_submodule_url(value, superproject)?; + points_at(&url, owners).then_some(url) }) .collect() } diff --git a/idris-dependents/src/scan.rs b/idris-dependents/src/scan.rs index 7117a64f..8f9571e8 100644 --- a/idris-dependents/src/scan.rs +++ b/idris-dependents/src/scan.rs @@ -27,7 +27,12 @@ pub struct Response { impl Response { fn into_body(self, url: &str) -> Result { if !(200..300).contains(&self.status) { - bail!("HTTP {} for {url}", self.status); + // GitHub puts the reason ("Bad credentials", secondary limits…) in the body. + let detail: String = self.body.trim().chars().take(300).collect(); + if detail.is_empty() { + bail!("HTTP {} for {url}", self.status); + } + bail!("HTTP {} for {url}: {detail}", self.status); } Ok(self.body) } @@ -325,7 +330,7 @@ pub fn run(opts: &Options, io: &mut I) -> Result { } }, Kind::Submodule => { - let r = refs::gitmodules_refs(&text, &opts.owners); + let r = refs::gitmodules_refs_in(&text, &opts.owners, Some(&it.full_name)); let found = !r.is_empty(); acc.sub_refs.extend(r); found diff --git a/idris-dependents/tests/rules_tests.rs b/idris-dependents/tests/rules_tests.rs index a4d06633..b0277d74 100644 --- a/idris-dependents/tests/rules_tests.rs +++ b/idris-dependents/tests/rules_tests.rs @@ -4,7 +4,7 @@ use idris_dependents::catalogue::{classify, is_distinctive, search_stems, Confidence}; use idris_dependents::ipkg::{depends, lex, package_name, Tok}; -use idris_dependents::refs::{gitmodules_refs, pack_toml_refs, points_at}; +use idris_dependents::refs::{gitmodules_refs, gitmodules_refs_in, pack_toml_refs, points_at}; fn fx(name: &str) -> String { std::fs::read_to_string(format!( @@ -570,3 +570,138 @@ url = "" ]) ); } + +// --- review findings on #628 (Codex, CodeRabbit) ------------------------------- + +#[test] +fn unterminated_string_hides_its_contents() { + // Codex: `brief = "depends = proven` must not report `proven`. + assert!(depends("package x\nbrief = \"depends = proven").is_empty()); + assert_eq!( + depends("package x\ndepends = ochrance\nbrief = \"oops depends = proven"), + s(&["ochrance"]) + ); +} + +#[test] +fn github_must_be_the_host_not_a_path_segment() { + // Codex: a mirror path containing github.com is not a GitHub URL. + let o = own(); + assert!(!points_at( + "https://example.com/mirrors/github.com/hyperpolymath/proven", + &o + )); + assert!(!points_at( + "git@example.com:github.com/hyperpolymath/proven", + &o + )); + assert!(points_at( + "ssh://git@github.com:22/hyperpolymath/proven.git", + &o + )); + assert!(points_at("git+https://github.com/hyperpolymath/proven", &o)); + assert!(points_at( + "https://user@github.com/Hyperpolymath/proven", + &o + )); +} + +#[test] +fn gitmodules_keys_are_case_insensitive() { + // Codex: git config keys are case-insensitive (`URL =` is valid). + let t = "[submodule \"p\"]\n\tURL = https://github.com/hyperpolymath/proven.git\n\tUrl=https://github.com/metadatastician/x"; + assert_eq!( + gitmodules_refs(t, &own()), + s(&[ + "https://github.com/hyperpolymath/proven.git", + "https://github.com/metadatastician/x" + ]) + ); + assert!(gitmodules_refs("urlx = https://github.com/hyperpolymath/p", &own()).is_empty()); +} + +#[test] +fn relative_submodule_urls_resolve_against_the_superproject() { + // Codex: `../../hyperpolymath/proven.git` from alice/app is hyperpolymath/proven. + let t = "[submodule \"p\"]\n\turl = ../../hyperpolymath/proven.git\n[submodule \"q\"]\n\turl = ../sibling.git"; + assert_eq!( + gitmodules_refs_in(t, &own(), Some("alice/app")), + s(&["https://github.com/hyperpolymath/proven.git"]) + ); + // A sibling of a hyperpolymath fork's own repo resolves to that owner too. + assert_eq!( + gitmodules_refs_in("url = ./../proven", &own(), Some("hyperpolymath/app")), + s(&["https://github.com/hyperpolymath/proven"]) + ); + // Without the superproject, or climbing past the host, nothing is claimed. + assert!(gitmodules_refs(t, &own()).is_empty()); + assert!(gitmodules_refs_in("url = ../../../x/y", &own(), Some("alice/app")).is_empty()); +} + +#[test] +fn relative_submodule_urls_match_git_resolution() { + // Codex on #644. Expected values are what `git submodule init` (2.43) + // registers for a superproject whose origin is https://github.com/alice/app.git. + let all = |u: &str| { + gitmodules_refs_in( + &format!("url = {u}"), + &s(&["hyperpolymath", "alice"]), + Some("alice/app"), + ) + }; + assert_eq!( + all("../../../github.com/hyperpolymath/proven.git"), + s(&["https://github.com/hyperpolymath/proven.git"]) + ); + assert_eq!( + all("../../hyperpolymath/proven.git"), + s(&["https://github.com/hyperpolymath/proven.git"]) + ); + assert_eq!(all("./../proven"), s(&["https://github.com/alice/proven"])); + // git: https://x/y, https:/too/far — never a GitHub URL. + assert!(all("../../../x/y").is_empty()); + assert!(all("../../../../too/far").is_empty()); +} + +#[test] +fn owner_pages_with_query_or_fragment_are_not_repositories() { + // Codex on #644: a profile URL names no repository. + let o = own(); + assert!(!points_at( + "https://github.com/hyperpolymath/?tab=repositories", + &o + )); + assert!(!points_at("https://github.com/hyperpolymath/#about", &o)); + assert!(!points_at( + "https://github.com/hyperpolymath?tab=repositories", + &o + )); + assert!(points_at( + "https://github.com/hyperpolymath/proven#readme", + &o + )); + assert!(points_at( + "https://github.com/hyperpolymath/proven/tree/main?x=1", + &o + )); +} + +#[test] +fn authority_ends_at_query_or_fragment() { + // CodeRabbit on #644: the host here is evil.example, not github.com. + let o = own(); + assert!(!points_at( + "https://evil.example#@github.com/hyperpolymath/proven", + &o + )); + assert!(!points_at( + "https://evil.example?@github.com/hyperpolymath/proven", + &o + )); + assert!(!points_at("https://github.com?x=/hyperpolymath/proven", &o)); + assert!(!points_at("https://github.com/hyperpolymath/#/proven", &o)); + assert!(points_at( + "https://user@github.com/hyperpolymath/proven", + &o + )); +} diff --git a/idris-dependents/tests/scan_edge_tests.rs b/idris-dependents/tests/scan_edge_tests.rs index 9f2d68c3..5f4437b6 100644 --- a/idris-dependents/tests/scan_edge_tests.rs +++ b/idris-dependents/tests/scan_edge_tests.rs @@ -391,6 +391,21 @@ fn raw_content_fetches_retry_rate_limits_and_propagate_hard_failures() { let error = run(&options(), &mut io).unwrap_err().to_string(); assert!(error.contains("HTTP 500 for contents/a"), "{error}"); io.done(); + + // A file deleted since GitHub indexed it is skipped, not fatal. + let mut io = Script::default(); + io.search( + QUERY, + 1, + 1, + false, + vec![item("alice/app", "a.ipkg", "contents/a")], + ); + io.respond("contents/a", true, Ok(response(404, &[], ""))); + let report = run(&options(), &mut io).unwrap(); + assert!(report.rows.is_empty()); + assert_eq!(report.skipped_urls, vec!["contents/a"]); + io.done(); } #[test] @@ -445,3 +460,27 @@ fn report_json_keeps_public_field_names_and_confidence_spelling() { ); io.done(); } + +#[test] +fn http_errors_carry_github_reason() { + // CodeRabbit: surface the body (e.g. "Bad credentials"), not just the status. + let mut io = Script::default(); + io.search( + QUERY, + 1, + 1, + false, + vec![item("alice/app", "a.ipkg", "contents/a")], + ); + io.respond( + "contents/a", + true, + Ok(response(500, &[], "{\"message\":\"Bad credentials\"}")), + ); + let error = run(&options(), &mut io).unwrap_err().to_string(); + assert!( + error.contains("HTTP 500 for contents/a: {\"message\":\"Bad credentials\"}"), + "{error}" + ); + io.done(); +}