fix(scm): resolve GitHub SSH aliases#2957
Conversation
|
Thanks for contributing to Agent Orchestrator. This PR is being picked up by the current external contributor on-call pair: If someone is already working on this, please continue as usual. For faster context or live questions, you can also join the AO Discord. Join the session here: Come by if you want to see what is being built, ask questions, or just hang around with the community. |
Pulkit7070
left a comment
Pulkit7070
left a comment
There was a problem hiding this comment.
Reviewed at head fb36f0c. Ran from a clean checkout of this branch:
go test ./internal/adapters/scm/github -count=1-> ok (0.66s)go vet ./internal/adapters/scm/github-> cleango build ./...-> ok
The approach (inject SSHHostResolver, cache resolutions, ssh -G in prod) is sound and the tests are well structured.
Overlap with #2963 and #2965: both of those are stacked directly on this commit (fb36f0c) and are byte-for-byte identical to each other (same head SHA 2dc6bbc, git diff pr2963 pr2965 is empty). They keep this PR's design and fix the three issues I flag below. If #2963/#2965 lands, this PR is fully absorbed. Recommendation: consolidate. Prefer landing one of #2963/#2965 (they are the same commit) over this one, or fold their fixes into this branch and close the other two. Right now three PRs are competing to change the same three files.
Three correctness issues that #2963/#2965 already address:
- Alias case is lowercased before
ssh -G, which breaks case-sensitive OpenSSHHostmatching (line 664). - Failed lookups are cached permanently, a sticky negative cache (line 683).
ssh -G hostis run without--, so an alias beginning with-is parsed as an ssh flag (argument injection); no leading-dash guard (line 691).
Details inline.
| } | ||
|
|
||
| func (p *Provider) resolveSSHHost(host string) (string, bool) { | ||
| host = strings.ToLower(strings.TrimSpace(host)) |
There was a problem hiding this comment.
strings.ToLower on the alias before resolution breaks case-sensitive lookup. OpenSSH Host patterns are case-sensitive, so an alias configured as Host GitHub-Work will not match after being lowercased to github-work, and ssh -G github-work returns the alias itself as hostname (no match), so resolution silently fails for any mixed-case alias. Your test scp ssh alias does not catch this because its injected resolver ignores the host argument. #2963/#2965 keep original casing for the resolver and lowercase only for the GitHub-host comparison and the normalized key.
| if p.sshHosts == nil { | ||
| p.sshHosts = make(map[string]sshHostResolution) | ||
| } | ||
| p.sshHosts[host] = sshHostResolution{host: resolved, ok: ok} |
There was a problem hiding this comment.
This caches failures too (ok=false is stored), so a transient ssh -G failure (timeout, ssh briefly unavailable) is cached for the lifetime of the Provider and never retried. A GitHub-backed alias then stays unresolved until the daemon restarts. This is exactly the sticky negative cache #2965 calls out and fixes by not caching failures, letting the next observer poll recover.
| func resolveSSHConfigHost(host string) (string, bool) { | ||
| ctx, cancel := context.WithTimeout(context.Background(), time.Second) | ||
| defer cancel() | ||
| out, err := aoprocess.CommandContext(ctx, "ssh", "-G", host).Output() |
There was a problem hiding this comment.
ssh -G host runs without a -- terminator and there is no guard rejecting hosts that start with -. A remote such as git@-oProxyCommand=...:owner/repo reaches here as host -oProxyCommand=... and ssh parses it as an option (argument injection from a remote URL). #2963/#2965 reject leading-dash hosts in both resolveSSHHost and resolveSSHConfigHost and pass ssh -G -- host. Worth adopting.
|
Team review: this PR is a subset of #2965 (SSH alias case + no sticky negative cache). Please close in favor of #2965 when convenient — AllBeingsFuture cannot close this head (author is @ppanphper). |
5 participants
Summary
ssh -Gbefore parsing GitHub repository originsssh://remotes, rejected non-GitHub aliases, failures, and cachingWhy
Projects using remotes such as
git@github-work:owner/repo.gitwere reported as having no supported SCM origin. As a result, the observer never discovered merged pull requests and affected AO sessions remained open.Testing
GIT_CONFIG_GLOBAL=/dev/null go test -race ./internal/adapters/scm/github ./internal/observe/scm ./internal/service/session ./internal/daemonGIT_CONFIG_GLOBAL=/dev/null npm run lintGIT_CONFIG_GLOBAL=/dev/nullisolates the suite from local Git URL rewrite rules.