fix(scm): SSH alias case + no sticky negative cache (follow-up to #2957)#2965
fix(scm): SSH alias case + no sticky negative cache (follow-up to #2957)#2965AllBeingsFuture wants to merge 2 commits into
Conversation
OpenSSH Host patterns are case-sensitive, so keep the original alias casing for ssh -G and cache keys; lowercase only resolved hostnames for isGitHubHost. Do not cache resolution failures so a later poll can recover without a daemon restart. Also invoke ssh as `ssh -G -- <host>` and reject aliases that start with '-'.
|
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 2dc6bbc. Ran from a clean checkout:
go test ./internal/adapters/scm/github -count=1-> okgo vet ./internal/adapters/scm/github-> cleango build ./...-> ok
As the stated follow-up to #2957, this does the right thing: it keeps #2957's resolver design and fixes its three concrete defects - lowercasing the alias before ssh -G (broke case-sensitive OpenSSH Host matching), the sticky negative cache (failed lookups were cached until daemon restart), and the missing --/leading-dash guard (argument injection from a crafted remote). The added tests cover each: case sensitive ssh alias, TestParseRepositoryDoesNotCacheSSHHostFailures, TestParseRepositoryPreservesSSHAliasCaseForResolver, and TestResolveSSHConfigHostRejectsLeadingDash.
Overlap, important: #2965 and #2963 are the exact same commit (identical head SHA 2dc6bbc; git diff pr2963 pr2965 is empty). They are duplicates of each other, and both fully supersede #2957 (stacked on fb36f0c, absorbing it). Recommend consolidating the trio down to one PR: land either this or #2963 (no code difference between them) and close the other two. Since this one is the labelled follow-up with the more complete history, it is the natural one to keep.
Two non-blocking notes inline.
|
|
||
| func (p *Provider) resolveSSHHost(host string) (string, bool) { | ||
| // Keep original casing: OpenSSH Host patterns are case-sensitive. | ||
| host = strings.TrimSpace(host) |
There was a problem hiding this comment.
Good: keeping original casing here and only lowercasing for the GitHub-host comparison and normalized key is exactly what fixes #2957's case bug. The double-checked-locking on cache re-read after the resolver call (checking p.sshHosts[host] again under lock) also correctly collapses concurrent first-time lookups. No change requested.
| defer cancel() | ||
| // "--" keeps hosts that start with "-" from being parsed as ssh flags | ||
| // (those are also rejected above; this is defense in depth). | ||
| out, err := aoprocess.CommandContext(ctx, "ssh", "-G", "--", host).Output() |
There was a problem hiding this comment.
The ssh -G -- host plus leading-dash rejection in both resolveSSHHost and here is the correct hardening. One shared limitation with #2957/#2963 (not introduced by this PR): only git@-prefixed scp remotes and ssh:// URLs get alias resolution; an scp remote without the git@ user (for example work:owner/repo relying on ssh_config User) is not resolved. Fine as a follow-up if such remotes matter.
5 participants
Summary
Follow-up / re-host of #2957 with review bugs fixed.
Original head is
ppanphper:fix/github-ssh-alias(cross-repo); no push access from this account.Fixes
ssh -G(OpenSSHHostpatterns are case-sensitive).Includes original #2957 work + fix commit
2dc6bbcb.Test plan
GitHub-Work→ github.com) resolvesssh -Gfailure does not permanently disable aliasgo testfor github SCM adapter