fix(scm): resolve GitHub SSH aliases (case + negative cache)#2963
fix(scm): resolve GitHub SSH aliases (case + negative cache)#2963AllBeingsFuture 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
This is a solid follow-up on #2957: it keeps that PR's design and fixes its three real problems - case preservation for OpenSSH Host matching, no sticky negative cache, and leading-dash argument-injection hardening with ssh -G -- host. Test coverage is good (case-sensitive alias, failure-not-cached, alias-case-preserved-to-resolver, leading-dash rejected).
Overlap, important: this PR and #2965 are the exact same commit (both head SHA 2dc6bbc, git diff pr2963 pr2965 is empty). They are pure duplicates of each other and both supersede #2957 (they are stacked on fb36f0c and absorb it). Recommend consolidating the trio: land exactly one of #2963/#2965 and close the other plus #2957. There is no code difference to choose between #2963 and #2965.
Two non-blocking notes inline (both pre-existing, shared with #2957/#2965).
| resolved, ok := p.sshHostResolver(host) | ||
| resolved = strings.ToLower(strings.TrimSpace(resolved)) | ||
| if !ok || resolved == "" { | ||
| // Do not cache failures: transient ssh/timeout errors should recover on |
There was a problem hiding this comment.
The not-caching-failures behavior is correct for transient errors. One consequence to be aware of: a host that genuinely errors on every ssh -G (for example ssh binary missing) will re-shell-out with the 1s timeout on every observer poll for that remote, since nothing is cached. In practice ssh -G alias succeeds even with no matching Host block (it echoes the alias back as hostname), so that success path does get cached and the repeated shell-out only affects true ssh failures. Acceptable tradeoff, just worth a comment so it is not mistaken for a leak.
| 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 -- plus leading-dash rejection is the right hardening. Note a shared limitation with #2957/#2965 (not introduced here): only the git@-prefixed scp form and ssh:// URL form are resolved. An scp remote without the git@ user (for example work:owner/repo, valid when ssh_config sets User) skips the git@ branch and is not alias-resolved. Fine to leave for a follow-up, but flagging in case such remotes are expected.
5 participants
Summary
ssh -Gso remotes likegit@github-work:owner/repoare treated as GitHub origins for the SCM observer.ssh -Gand cache keys (OpenSSHHostpatterns are case-sensitive). Lowercase only when checkingisGitHubHost/ storing the resolved hostname.sshfailures/timeouts recover on the next poll without a daemon restart.ssh -G -- <host>and reject aliases that start with-.Why a new PR
#2957 lives on
ppanphper:fix/github-ssh-alias. This session account cannot push to that fork head, so the review fixes are delivered here with the full stack (alias resolution + the two bugfixes).Testing
GIT_CONFIG_GLOBAL=/dev/null go test -race ./internal/adapters/scm/github/ -count=1GitHub-Work), failure non-caching, leading-dash rejection, and resolver-seen host casing.Known risks / follow-ups
parseGitHubRepoNativepath forssh://aliases (out of scope).