Repository navigation
fix(forge): detect SaaS forges from the remote host only, and ignore empty push tokens - #1132
Conversation
…empty push tokens
There was a problem hiding this comment.
Detection change is correct: host-only matching with a label-boundary check, exact or subdomain, and the previously-wrong cases are covered by tests. The extract_host widening to any scheme:// is a strict improvement for ssh://host/... with no user, and file:///... still yields no host. Empty-token handling now matches forge::resolve_token.
One qualification on the description rather than the diff: token_for_url ends with github_token.map(...), which hands GITHUB_TOKEN to any host once detection returns None. For the motivating remote https://git.acme.com/tools/github.com-exporter.git, the push credential is therefore unchanged by this PR: the GitHub token still goes to git.acme.com. Pre-existing and out of scope here, but git::auth only actually benefits when the probe comes back GitLab, not in general.
Nit: extract_host does not lowercase, so https://GitHub.com/o/r.git detects nothing. Identical to the old behaviour, since contains was case-sensitive too, but valid_host would be the natural place to normalise if you want it closed.
| .strip_prefix("https://") | ||
| .or_else(|| url.strip_prefix("http://")) | ||
| { | ||
| if let Some((_, rest)) = url.split_once("://") { |
There was a problem hiding this comment.
Nit: routing detection through extract_host drops scp-style remotes that omit the user. github.com:owner/repo.git is valid git syntax and used to resolve through contains; the else if branch below requires an @, so it now yields no host, detect_forge_from_url returns None, and the probe fallback gets no host either.
Fix would be in the else if branch rather than here: split on the first : and strip userinfo from the left part, which also handles git@host:path/with@sign that the current rsplit('@') gets wrong. The cost is that a local C:\path would then parse as host C, which valid_host accepts, so it is a real tradeoff. Fine to leave if that remote form is out of scope.
There was a problem hiding this comment.
Closes #1131. Follow-up to the two review nits on #1124, which merged before they could go in.
Detect SaaS forges from the host only
detect_forge_from_urldidurl.contains("github.com")and friends, so a repository path such ashttps://git.acme.com/tools/gitlab.com-exporter.gitresolved to GitLab. It now takes the host throughextract_hostand matches it against the known SaaS hosts, exactly or as a subdomain:git.acme.com/tools/gitlab.com-exporter.git,git@git.acme.com:mirrors/github.com.git: no match, so self-hosted detection (probe or configuredforge) takes over as it shouldnotgithub.com,gitlab.com.evil.io: no match, since the suffix has to sit on a label boundaryssh.github.com,ssh://git@gitlab.com:22/...: still resolveThis is the one function behind the push credential (
git::auth), the API forge (forge_target),web_base_urlanddoctor, so the reviewer's "fix it insidedetect_forge_from_url" option covers every caller at once. The probe cache is already keyed by host, so it is unaffected.extract_hostnow accepts anyscheme://rather than onlyhttp(s)://. Before, anssh://URL only worked through theuser@host:fallback, which needs an@.file:///...still yields no host.Ignore empty token variables on the push side
GITHUB_TOKEN: ${{ secrets.MISSING }}exports an empty string.token_for_urlcounted it as present, paid a probe on a self-hosted host, and handed git an empty password. Empty values are now treated as unset, matchingforge::resolve_token.Tests
forge(pure, no network): a SaaS domain in the path picks no forge, lookalike hosts are rejected, and a subdomain and anssh://URL still resolve. The existingdetect_forge_from_urlcases pass unchanged.git: all three token variables set to""give no credential.Full suite: 2468 passed.
Not changed:
extract_repo_slugstill locates the slug withcontains("github.com/"). It only affects slug parsing on a self-hosted remote whose path contains that string, not which forge or credential is used, so it is left for a separate change if it ever matters.