From 739aa2a6fc938dd6c0f8ffc88c5aa479baf4ba27 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Thu, 12 Nov 2026 06:15:54 +0000 Subject: [PATCH 1/2] feat(origin): accept the SSH login in a +ssh location git+ssh://git@host/path, hg+ssh:// and svn+ssh:// carry a user name; it is part of the identity and of the address the VCS executable is given. A password anywhere and a user name on any other transport stay refused. Co-Authored-By: jason.han --- README.md | 8 +- changes/unreleased/ssh-user-name.added.md | 6 + cmd/syspi/main.go | 8 +- docs/packages.md | 62 +++++++++- internal/install/install.go | 7 +- internal/install/install_test.go | 3 + internal/origin/origin.go | 70 ++++++++--- internal/origin/origin_test.go | 93 +++++++++++++- internal/vcs/vcs.go | 15 ++- internal/vcs/vcs_test.go | 68 +++++++++++ tests/e2e/e2e_test.go | 140 ++++++++++++++++++++++ 11 files changed, 449 insertions(+), 31 deletions(-) create mode 100644 changes/unreleased/ssh-user-name.added.md diff --git a/README.md b/README.md index da22cf2..e60d6ae 100644 --- a/README.md +++ b/README.md @@ -10,14 +10,18 @@ what package managers install. ## The contract, on one screen - **Identity** — an ECMA-427 Package-URL of type `generic` with the Annex B.3.5 `vcs_url` - qualifier: `pkg:generic/?vcs_url=+:///[@][#]`, + qualifier: `pkg:generic/?vcs_url=+://[@]/[@][#]`, the qualifier percent-encoded (`+` → `%2B`, `/` → `%2F`, `@` → `%40`, `#` → `%23`, `:` kept), e.g. `pkg:generic/OpenSysML-Extensions-Library?vcs_url=git%2Bhttps:%2F%2Fgithub.com%2FOpen-MBEE%2FOpenSysML-Extensions-Library%401.0.0`. `` is `git`, `svn` or `hg`, `` one of Annex B.3.5's (`git`, `https`, `http`, `ssh` for git; `http`, `https`, `static-http`, `ssh` for hg; `svn`, `http`, `https`, `ssh` for svn) plus `file` for a repository on this machine; `` is the repository's last path segment. + `@` is the SSH login and is accepted on `+ssh` only + (`git+ssh://git@github.com/owner/repo@1.2.0`); it is part of the identity, so it is written to + `.project.json` and the lock as `…git%2Bssh:%2F%2Fgit%40github.com%2F…`, and the same location + without it is another identity. A password, or a user name on any other transport, is refused. Two shorter spellings are accepted as input and normalised to it: `pkg:git//[@][#]` - (git over https) and the bare notation `git+https://host/path@1.2.0`, `svn://host/repo@1.2.0`, + (git over https) and the bare notation `git+https://host/path@1.2.0`, `git+ssh://git@host/path@1.2.0`, `svn://host/repo@1.2.0`, `hg+https://…`, `git+file:///srv/repo@1.2.0` (`git+file:///C:/repo@1.2.0` on Windows). Refused with a message naming those three: `pkg:svn/…`, `pkg:hg/…`, `pkg:github/…`, plain clone URLs (`https://host/path.git`, `ssh://…`, `git@host:path`, `file:///path@1.2.0`) and a commit id as version. SysPI hand-parses this one diff --git a/changes/unreleased/ssh-user-name.added.md b/changes/unreleased/ssh-user-name.added.md new file mode 100644 index 0000000..b130263 --- /dev/null +++ b/changes/unreleased/ssh-user-name.added.md @@ -0,0 +1,6 @@ +- A user name is accepted before the host of a `+ssh://` location — + `git+ssh://git@github.com/owner/repo@1.0.0`, `hg+ssh://…`, `svn+ssh://…` — so an SSH remote no + longer needs a `User` entry in `ssh_config`. It is the SSH login, part of the identity + (`…git%2Bssh:%2F%2Fgit%40github.com%2F…` in `.project.json` and the lock) and of the address + the VCS executable is given. A password anywhere, and a user name on any other transport, + stay refused. diff --git a/cmd/syspi/main.go b/cmd/syspi/main.go index 2ee2eb2..2fc5c42 100644 --- a/cmd/syspi/main.go +++ b/cmd/syspi/main.go @@ -41,10 +41,12 @@ Commands: A package is any git, svn or hg repository with .project.json at a tag, identified by an ECMA-427 Package-URL (vcs: git, svn, hg; transports per Annex B.3.5, plus file): - pkg:generic/?vcs_url=%2B:%2F%2F%2F[%40][%23] + pkg:generic/?vcs_url=%2B:%2F%2F[%40]%2F[%40][%23] Accepted as input and normalised to it: - pkg:git//[@][#] - +:///[@][#] + pkg:git//[@][#] (git over https) + +://[@]/[@][#] +@ is the SSH login, accepted on +ssh only (git+ssh://git@github.com/owner/repo@1.0.0) +and part of the identity; a password, or a user name on another transport, is refused. Run "syspi -h" for the flags of a command. ` diff --git a/docs/packages.md b/docs/packages.md index 09daf1a..de29ac8 100644 --- a/docs/packages.md +++ b/docs/packages.md @@ -7,13 +7,14 @@ types and go in a qualifier) of type `generic` carrying the Annex B.3.5 `vcs_url ``` resource = "pkg:generic/" name "?vcs_url=" encode(vcs_url) -vcs_url = vcs "+" transport "://" host "/" path [ "@" version ] [ "#" subpath ] +vcs_url = vcs "+" transport "://" [ user "@" ] host "/" path [ "@" version ] [ "#" subpath ] vcs = "git" | "svn" | "hg" transport = git: "git" | "https" | "http" | "ssh" (Annex B.3.5; "git" and "git+git" are one) hg: "http" | "https" | "static-http" | "ssh" svn: "svn" | "http" | "https" | "ssh" ("svn" and "svn+svn" are one) any: "file" (a repository on this machine, pip's notation; host empty, path absolute) name = last segment of path, a ".git" suffix stripped +user = SSH login, on transport "ssh" only; as written (case kept), never with a password host = DNS name (lower-cased), with an optional port path = repository path on the host as written (a ".git" suffix stays), without a leading "/" version = semantic version (MAJOR.MINOR.PATCH[-pre][+build]) naming a tag; an exact constraint @@ -25,13 +26,14 @@ subpath = directory within the tagged tree that is the project directory percent-encoding is accepted on input. Examples: - `pkg:generic/OpenSysML-Extensions-Library?vcs_url=git%2Bhttps:%2F%2Fgithub.com%2FOpen-MBEE%2FOpenSysML-Extensions-Library%401.0.0` +- `pkg:generic/OpenSysML-Extensions-Library?vcs_url=git%2Bssh:%2F%2Fgit%40github.com%2FOpen-MBEE%2FOpenSysML-Extensions-Library%401.0.0` - `pkg:generic/rover?vcs_url=hg%2Bhttps:%2F%2Fhg.example.org%2Fmodels%2Frover%23packages%2Fpower` - `pkg:generic/rover?vcs_url=svn%2Bsvn:%2F%2Fsvn.example.org%2Frepos%2Frover%401.0.0` - `pkg:generic/rover?vcs_url=git%2Bfile:%2F%2F%2Fsrv%2Fgit%2Frover.git%401.0.0` Two input spellings normalise to it (and `syspi add` writes the canonical form into `.project.json`): `pkg:git//[@][#]`, meaning `git+https`; and the -bare vcs_url notation `git+https://…`, `git+ssh://…`, `git://…`, `hg+https://…`, +bare vcs_url notation `git+https://…`, `git+ssh://[@]…`, `git://…`, `hg+https://…`, `hg+static-http://…`, `svn://…`, `svn+https://…`, `svn+ssh://…`, `+file:///…`, each with an optional `@` and `#`. A repository on a Windows machine is `+file:///C:/path` (RFC 8089); `+file://C:/path` is accepted as input and normalised to @@ -44,12 +46,64 @@ two locations and the one your server serves is the one to write). Refused, each with a message naming the three accepted spellings: `pkg:svn/…` and `pkg:hg/…` (not Package-URL types), `pkg:github/…`, `pkg:gitlab/…` and other provider vocabularies (the host is part of the location), plain clone URLs as identities (`https://host/path.git@1.0.0`, -`ssh://…`, `git@host:path`, `file:///path@1.0.0`, `/path@1.0.0`), a user name or password in the -host (Annex B.3.5), a transport outside the lists above, `bzr` or any other VCS, a commit id or +`ssh://…`, `git@host:path`, `file:///path@1.0.0`, `/path@1.0.0`), a password in the host on any +transport and a user name in the host on any transport but `ssh` (Annex B.3.5; see below), a +transport outside the lists above, `bzr` or any other VCS, a commit id or `@latest` as version. A plain `http(s)` IRI is only ever dereferenced as a project interchange file — a `.kpar` — as KerML 1.1 §10.3 Table 12 describes `resource`; it is never cloned, and `user:password@` in it is refused (credentials belong in the netrc, never in `.project.json`). +## The user name of an SSH location + +`git+ssh://git@github.com/owner/repo@1.0.0`, `hg+ssh://hg@host/repo` and `svn+ssh://svn@host/repo` +are accepted: on the `ssh` transport the part before `@` is the login the server expects, the +same for everyone, and not a secret. Annex B.3.5 (informative) says "Using user names and password +in the host_name is not supported and should be reported by tools as an error", and in the same +clause that its syntax is pip's, whose own examples are `git+ssh://git@git.example.com/MyProject` +and `svn+ssh://user@svn.example.com/MyProject`. SysPI follows that recommendation wherever the +user name would be a credential and departs from it for this one case: + +| written | result | +| --- | --- | +| `+ssh://user@host/path` | accepted | +| `+ssh://user:password@host/path`, `user:@host` | refused: a password | +| `+://user@host/path` | refused: there a user name is a credential (a token) | +| `pkg:git/user@host/path` | refused: it means `git+https://user@host/path` | +| `git@host:path` (scp style) | refused: not a URL; the message names `git+ssh://git@host/path` | + +The alias and the bare notation agree: both are the same `vcs_url` once normalised, and the one +rule above is applied to that `vcs_url`. An SSH location is therefore always written as +`git+ssh://…` or as the canonical `pkg:generic/…?vcs_url=git%2Bssh:…`; `pkg:git/…` has no SSH form. + +**The user name is part of the identity.** RFC 3986 §3.2 makes userinfo a component of the +authority, and neither its normalisation rules (§6) nor Annex B.3.5 drop it, so SysPI keeps it: + +- `.project.json` and the lock's `resource` carry it inside the percent-encoded `vcs_url`, the + `@` as `%40` like every other one: + `pkg:generic/repo?vcs_url=git%2Bssh:%2F%2Fgit%40github.com%2Fowner%2Frepo`. The lock's `source` + is `ssh://git@github.com/owner/repo`, and that is the address the VCS executable is given + (`git ls-remote --tags -- ssh://git@github.com/owner/repo`). +- The user name is kept as written: its case is significant, and a character outside RFC 3986's + userinfo set stays percent-encoded in the address (`first.last%40corp@host`, hence `%2540` in + the `resource`). An empty user name (`git+ssh://@host/path`) is none. One starting with `-` is + refused, since `ssh` would read it as an option. +- `git+ssh://git@host/owner/repo` and `git+ssh://host/owner/repo` are **two identities**, exactly + as `git+https://host/owner/repo` and `git+ssh://host/owner/repo` are: SysPI cannot know that + the login `ssh_config` supplies for the second is the `git` of the first. A usage of one never + matches a lock entry of the other; each is resolved through its own address and locked under + its own `resource`. Changing a usage from one spelling to the other is changing the identity: + the next `install` resolves it anew over the network (`-offline` refuses, as for any identity + the lock does not hold) and replaces the lock entry. +- The installed directory does not carry it: `lib/-_` is named from the + package's own `.project.json`, never from the location. Two usages that differ only in the user + name — in one project, or one in the project and one in a dependency — therefore resolve to the + same directory, are locked as two entries with the same `revision` and `sha256`, and are + installed once. Were the two addresses to serve different contents for that publisher, name and + version, the run fails naming both (as for any two packages that collide on a directory) + instead of letting one overwrite the other. `lib/` is matched to a usage only through the + lock: an entry whose `resource` is the usage's identity, and an installed tree whose digest is + that entry's `sha256`. + ## What a package is The tagged tree — the repository root, or `#` within it — is the project directory. It diff --git a/internal/install/install.go b/internal/install/install.go index 90d05ff..d07bba0 100644 --- a/internal/install/install.go +++ b/internal/install/install.go @@ -417,7 +417,7 @@ func (r *resolver) lockedRepository(o *origin.Origin, c version.Constraint) *loc } // locationKey normalises a repository source for comparison: a + scheme -// prefix and host case do not matter, a file URL is its path; .git does. +// prefix and host case do not matter, a file URL is its path; .git and a user name do. func locationKey(src string) string { scheme, rest, ok := strings.Cut(src, "://") if !ok { @@ -430,8 +430,9 @@ func locationKey(src string) string { if scheme == "file" { return filepath.FromSlash(rest) } - host, p, _ := strings.Cut(rest, "/") - return scheme + "://" + strings.ToLower(host) + "/" + p + authority, p, _ := strings.Cut(rest, "/") + at := strings.LastIndexByte(authority, '@') + 1 + return scheme + "://" + authority[:at] + strings.ToLower(authority[at:]) + "/" + p } // reuseLocked keeps a locked archive entry when only the lock is being diff --git a/internal/install/install_test.go b/internal/install/install_test.go index 175e807..3389120 100644 --- a/internal/install/install_test.go +++ b/internal/install/install_test.go @@ -339,6 +339,7 @@ func TestLocationKey(t *testing.T) { same := [][2]string{ {"git+ssh://Example.org/team/repo.git", "ssh://example.org/team/repo.git"}, {"git+https://example.org/team/repo", "https://example.org/team/repo"}, + {"git+ssh://git@Example.org/team/repo", "ssh://git@example.org/team/repo"}, {"git+file:///srv/repo.git", filepath.FromSlash("/srv/repo.git")}, {"svn+file:///srv/repo", "file:///srv/repo"}, } @@ -351,6 +352,8 @@ func TestLocationKey(t *testing.T) { {"https://example.org/team/repo", "https://example.org/team/repo.git"}, {"ssh://example.org/team/repo", "https://example.org/team/repo"}, {"https://example.org/Team/repo", "https://example.org/team/repo"}, + {"ssh://git@example.org/team/repo", "ssh://example.org/team/repo"}, + {"ssh://Git@example.org/team/repo", "ssh://git@example.org/team/repo"}, } for _, c := range differ { if locationKey(c[0]) == locationKey(c[1]) { diff --git a/internal/origin/origin.go b/internal/origin/origin.go index 49c44e2..c1813df 100644 --- a/internal/origin/origin.go +++ b/internal/origin/origin.go @@ -6,9 +6,9 @@ // A version-controlled package is identified by an ECMA-427 Package-URL of // type generic carrying the Annex B.3.5 vcs_url qualifier: // -// pkg:generic/?vcs_url=+:///[@][#] +// pkg:generic/?vcs_url=+://[@]/[@][#] // -// with the qualifier value percent-encoded. pkg:git//[@v][#sub] +// with the qualifier value percent-encoded; @ is the SSH login, on +ssh only. pkg:git//[@v][#sub] // and the bare vcs_url notation are accepted as input and normalised to it. package origin @@ -73,13 +73,16 @@ var transports = map[string]map[string]bool{ // Spellings names the three accepted ways to write a repository package. const Spellings = "pkg:generic/?vcs_url=%2B:%2F%2F%2F[%40][%23], " + "pkg:git//[@][#], or +:///[@][#] " + - "(vcs: git, svn, hg; transports per ECMA-427 Annex B.3.5, plus file for a repository on this machine)" + "(vcs: git, svn, hg; transports per ECMA-427 Annex B.3.5, plus file for a repository on this machine; " + + "@ names the SSH login on +ssh only)" // Origin is a parsed resource. type Origin struct { Kind Kind // VCS and Transport are the vcs_url scheme parts for Kind == VCS. VCS, Transport string + // User is the SSH login written before the host of a +ssh location, "" when none. + User string // Host is the repository host (empty for a repository on this machine). Host string // Path is the repository path on the host as written, without a leading @@ -98,14 +101,22 @@ type Origin struct { Resource string } -// VCSURL is the Annex B.3.5 location: +:///[@v][#sub]. +// authority is [@], the user name escaped as RFC 3986 userinfo. +func authority(user, host string) string { + if user == "" { + return host + } + return url.User(user).String() + "@" + host +} + +// VCSURL is the Annex B.3.5 location: +://[@]/[@v][#sub]. func (o *Origin) VCSURL(withVersion bool) string { var b strings.Builder b.WriteString(o.VCS) b.WriteByte('+') b.WriteString(o.Transport) b.WriteString("://") - b.WriteString(o.Host) + b.WriteString(authority(o.User, o.Host)) b.WriteByte('/') b.WriteString(strings.TrimPrefix(o.Path, "/")) if withVersion && o.Version != "" { @@ -200,7 +211,7 @@ func Parse(resource string) (*Origin, error) { return nil, fmt.Errorf("resource %q: unsupported URL scheme; a repository is written as %s", res, Spellings) } if m := scpLike.FindStringSubmatch(res); m != nil && !filepath.IsAbs(res) && !strings.HasPrefix(res, ".") { - return nil, fmt.Errorf("resource %q: scp-style clone addresses are not identities; write git+ssh://%s/%s or another of %s", res, m[2], strings.TrimSuffix(m[3], ".git"), Spellings) + return nil, fmt.Errorf("resource %q: scp-style clone addresses are not identities; write git+ssh://%s%s/%s or another of %s", res, m[1], m[2], strings.TrimSuffix(m[3], ".git"), Spellings) } return parseLocal(res, res) } @@ -281,12 +292,42 @@ func parseGitAlias(res, tail string) (*Origin, error) { return parseVCSURL(res, "git+https://"+tail) } -// parseVCSURL handles [+]:///[@][#]. +// userInAuthority reports whether the last @ of spec lies in its authority, +// where it ends a user name instead of starting a version. +func userInAuthority(spec string) bool { + _, rest, _ := strings.Cut(spec, "://") + auth, tail, _ := strings.Cut(rest, "/") + return strings.Contains(auth, "@") && !strings.Contains(tail, "@") +} + +// sshUser is the user name of a vcs_url authority: accepted on ssh, where it +// is the login and not a secret; a password, or a user name elsewhere, is refused. +func sshUser(res, vcs, transport string, info *url.Userinfo) (string, error) { + if info == nil { + return "", nil + } + if _, has := info.Password(); has { + return "", fmt.Errorf("resource %q: a password in the host is not supported (ECMA-427 Annex B.3.5); use the credentials of your %s configuration", res, vcs) + } + if transport != "ssh" { + return "", fmt.Errorf("resource %q: a user name in the host is accepted only on %s+ssh, where it is the SSH login; on %s+%s it would be a credential (ECMA-427 Annex B.3.5) — use the credentials of your %s configuration", res, vcs, vcs, transport, vcs) + } + user := info.Username() + if strings.HasPrefix(user, "-") { + return "", fmt.Errorf("resource %q: user name %q starts with \"-\" and would be read as an option by ssh", res, user) + } + return user, nil +} + +// parseVCSURL handles [+]://[@]/[@][#]. func parseVCSURL(res, raw string) (*Origin, error) { spec, sub := cutFragment(raw) - spec, ver, err := cutVersion(res, spec) - if err != nil { - return nil, err + ver := "" + if !userInAuthority(spec) { + var err error + if spec, ver, err = cutVersion(res, spec); err != nil { + return nil, err + } } scheme, rest, ok := strings.Cut(spec, "://") if !ok { @@ -312,13 +353,14 @@ func parseVCSURL(res, raw string) (*Origin, error) { if err != nil { return nil, fmt.Errorf("resource %q: not a valid URL: %v", res, err) } - if u.User != nil { - return nil, fmt.Errorf("resource %q: a user name or password in the host is not supported (ECMA-427 Annex B.3.5); use the credentials of your %s configuration", res, vcs) + user, err := sshUser(res, vcs, transport, u.User) + if err != nil { + return nil, err } if u.RawQuery != "" { return nil, fmt.Errorf("resource %q: a vcs_url must not carry a query", res) } - o := &Origin{Kind: VCS, VCS: vcs, Transport: transport, Subpath: sub, Version: ver, Resource: res} + o := &Origin{Kind: VCS, VCS: vcs, Transport: transport, User: user, Subpath: sub, Version: ver, Resource: res} if transport == "file" { if u.Host != "" || !strings.HasPrefix(u.Path, "/") { return nil, fmt.Errorf("resource %q: a repository on this machine is %s+file:///absolute/path", res, vcs) @@ -348,7 +390,7 @@ func parseVCSURL(res, raw string) (*Origin, error) { return nil, fmt.Errorf("resource %q: repository path must not contain \"..\"", res) } o.Host, o.Path, o.Name = strings.ToLower(u.Host), p, strings.TrimSuffix(path.Base(p), ".git") - o.Source = source(vcs, transport, o.Host, p) + o.Source = source(vcs, transport, authority(user, o.Host), p) return o, nil } diff --git a/internal/origin/origin_test.go b/internal/origin/origin_test.go index a1842f0..860b272 100644 --- a/internal/origin/origin_test.go +++ b/internal/origin/origin_test.go @@ -80,12 +80,24 @@ func TestRefused(t *testing.T) { {"pkg:cvs/host/repo@1.0.0", "pkg:cvs is not supported"}, {"https://github.com/acme/widgets@1.0.0", "only dereferenced as a project interchange file (.kpar); a repository is written as pkg:generic"}, {"https://github.com/acme/widgets.git", "only dereferenced as a project interchange file"}, - {"git@github.com:acme/widgets.git@1.0.0", "write git+ssh://github.com/acme/widgets"}, + {"git@github.com:acme/widgets.git@1.0.0", "write git+ssh://git@github.com/acme/widgets.git@1.0.0 or another of"}, + {"example.org:acme/widgets.git", "write git+ssh://example.org/acme/widgets or another of"}, {"ssh://git@github.com/acme/widgets@1.0.0", "unsupported URL scheme; a repository is written as pkg:generic"}, {"file:///srv/git/widgets@1.0.0", "a repository on this machine is git+file://" + srvGitWidgets + "@1.0.0"}, {"/srv/git/widgets@1.0.0", "a repository on this machine is git+file://" + srvGitWidgets + "@1.0.0"}, {"pkg:git//srv/git/widgets@1.0.0", "a repository on this machine is git+file://" + srvGitWidgets + "@1.0.0"}, - {"git+https://user:pw@github.com/acme/widgets", "user name or password in the host is not supported (ECMA-427 Annex B.3.5)"}, + {"git+https://user:pw@github.com/acme/widgets", "a password in the host is not supported (ECMA-427 Annex B.3.5)"}, + {"git+ssh://git:pw@github.com/acme/widgets", "a password in the host is not supported (ECMA-427 Annex B.3.5)"}, + {"git+ssh://git:@github.com/acme/widgets", "a password in the host is not supported"}, + {"pkg:generic/widgets?vcs_url=hg%2Bssh:%2F%2Fhg:pw%40hg.example.org%2Fwidgets", "a password in the host is not supported"}, + {"git+https://git@github.com/acme/widgets@1.0.0", "a user name in the host is accepted only on git+ssh, where it is the SSH login; on git+https it would be a credential"}, + {"pkg:git/git@github.com/acme/widgets@1.0.0", "a user name in the host is accepted only on git+ssh, where it is the SSH login; on git+https it would be a credential"}, + {"git://git@github.com/acme/widgets", "on git+git it would be a credential"}, + {"hg+https://hg@hg.example.org/widgets", "accepted only on hg+ssh"}, + {"svn://svn@svn.example.org/widgets", "accepted only on svn+ssh"}, + {"git+file://git@/srv/git/widgets", "accepted only on git+ssh"}, + {"git+ssh://-oProxyCommand=x@github.com/acme/widgets", "starts with \"-\" and would be read as an option by ssh"}, + {"git+ssh://git@github.com", "repository URL has no path"}, {"hg://hg.example.org/repo", "hg+hg is not a vcs_url scheme for hg (ECMA-427 Annex B.3.5 allows hg+http, hg+https, hg+static-http, hg+ssh, hg+file)"}, {"git+ftp://host/repo", "git+ftp is not a vcs_url scheme for git"}, {"bzr+https://host/repo", "version-control system \"bzr\" is not supported; supported: git, svn, hg"}, @@ -106,6 +118,83 @@ func TestRefused(t *testing.T) { } } +// A user name before the host of a +ssh location is the SSH login: part of +// the identity, percent-encoded with the rest of vcs_url, and of the clone address. +func TestSSHUserName(t *testing.T) { + for _, tc := range []struct{ in, user, canonical, identity, source string }{ + {"git+ssh://git@github.com/acme/widgets@1.0.0", "git", + "pkg:generic/widgets?vcs_url=git%2Bssh:%2F%2Fgit%40github.com%2Facme%2Fwidgets%401.0.0", + "pkg:generic/widgets?vcs_url=git%2Bssh:%2F%2Fgit%40github.com%2Facme%2Fwidgets", + "ssh://git@github.com/acme/widgets"}, + {"git+ssh://git@GitHub.com/acme/widgets", "git", + "pkg:generic/widgets?vcs_url=git%2Bssh:%2F%2Fgit%40github.com%2Facme%2Fwidgets", + "pkg:generic/widgets?vcs_url=git%2Bssh:%2F%2Fgit%40github.com%2Facme%2Fwidgets", + "ssh://git@github.com/acme/widgets"}, + {"pkg:generic/widgets@1.0.0?vcs_url=git%2Bssh:%2F%2Fgit%40github.com%2Facme%2Fwidgets", "git", + "pkg:generic/widgets?vcs_url=git%2Bssh:%2F%2Fgit%40github.com%2Facme%2Fwidgets%401.0.0", + "pkg:generic/widgets?vcs_url=git%2Bssh:%2F%2Fgit%40github.com%2Facme%2Fwidgets", + "ssh://git@github.com/acme/widgets"}, + {"git+ssh://Deploy@git.example.org:2222/team/repo.git@1.2.0#packages/core", "Deploy", + "pkg:generic/repo?vcs_url=git%2Bssh:%2F%2FDeploy%40git.example.org:2222%2Fteam%2Frepo.git%401.2.0%23packages%2Fcore", + "pkg:generic/repo?vcs_url=git%2Bssh:%2F%2FDeploy%40git.example.org:2222%2Fteam%2Frepo.git%23packages%2Fcore", + "ssh://Deploy@git.example.org:2222/team/repo.git"}, + {"hg+ssh://hg@hg.example.org/models/rover@2.0.0", "hg", + "pkg:generic/rover?vcs_url=hg%2Bssh:%2F%2Fhg%40hg.example.org%2Fmodels%2Frover%402.0.0", + "pkg:generic/rover?vcs_url=hg%2Bssh:%2F%2Fhg%40hg.example.org%2Fmodels%2Frover", + "ssh://hg@hg.example.org/models/rover"}, + {"svn+ssh://svn@svn.example.org/repos/pkg", "svn", + "pkg:generic/pkg?vcs_url=svn%2Bssh:%2F%2Fsvn%40svn.example.org%2Frepos%2Fpkg", + "pkg:generic/pkg?vcs_url=svn%2Bssh:%2F%2Fsvn%40svn.example.org%2Frepos%2Fpkg", + "svn+ssh://svn@svn.example.org/repos/pkg"}, + // A user name holding a reserved character stays escaped as userinfo inside vcs_url. + {"git+ssh://first.last%40corp@git.example.org/team/repo", "first.last@corp", + "pkg:generic/repo?vcs_url=git%2Bssh:%2F%2Ffirst.last%2540corp%40git.example.org%2Fteam%2Frepo", + "pkg:generic/repo?vcs_url=git%2Bssh:%2F%2Ffirst.last%2540corp%40git.example.org%2Fteam%2Frepo", + "ssh://first.last%40corp@git.example.org/team/repo"}, + // An empty user name is no user name. + {"git+ssh://@github.com/acme/widgets@1.0.0", "", + "pkg:generic/widgets?vcs_url=git%2Bssh:%2F%2Fgithub.com%2Facme%2Fwidgets%401.0.0", + "pkg:generic/widgets?vcs_url=git%2Bssh:%2F%2Fgithub.com%2Facme%2Fwidgets", + "ssh://github.com/acme/widgets"}, + } { + o, err := Parse(tc.in) + if err != nil { + t.Errorf("%s: %v", tc.in, err) + continue + } + if o.User != tc.user || o.Canonical() != tc.canonical || o.Identity() != tc.identity || o.Source != tc.source { + t.Errorf("%s:\n user %q\n canonical %s\n identity %s\n source %s", tc.in, o.User, o.Canonical(), o.Identity(), o.Source) + continue + } + again, err := Parse(o.Canonical()) + if err != nil || again.Canonical() != o.Canonical() || again.Identity() != o.Identity() || again.Source != o.Source || again.User != o.User || again.Version != o.Version { + t.Errorf("%s: canonical form does not round-trip: %v %+v", tc.in, err, again) + } + } +} + +// The user name is part of the authority (RFC 3986 §3.2), so a location +// written with it and one written without are two identities and two keys. +func TestSSHUserNameIsIdentity(t *testing.T) { + key := func(res string) string { + o, err := Parse(res) + if err != nil { + t.Fatal(err) + } + return o.Key() + } + with, without := key("git+ssh://git@example.org/team/repo.git@1.0.0"), key("git+ssh://example.org/team/repo") + if with == without { + t.Fatalf("user name dropped from the key: %s", with) + } + if want := "pkg:generic/repo?vcs_url=git%2Bssh:%2F%2Fgit%40example.org%2Fteam%2Frepo"; with != want || key("git+ssh://git@example.org/team/repo") != want { + t.Fatalf("key %s, want %s", with, want) + } + if key("git+ssh://Git@example.org/team/repo") == with { + t.Fatal("user names are case-sensitive") + } +} + func TestEncode(t *testing.T) { if got := Encode("git+https://h/p q@1.0.0#s/t"); got != "git%2Bhttps:%2F%2Fh%2Fp%20q%401.0.0%23s%2Ft" { t.Fatal(got) diff --git a/internal/vcs/vcs.go b/internal/vcs/vcs.go index aa1e833..97e8425 100644 --- a/internal/vcs/vcs.go +++ b/internal/vcs/vcs.go @@ -117,11 +117,20 @@ func AuthHint(name, msg string) string { return "" } -var userInfo = regexp.MustCompile(`(://)([^/@\s]+)@`) +var userInfo = regexp.MustCompile(`([A-Za-z0-9+.-]*)(://)([^/@\s]+)@`) // redact strips the user-info part of any URL in s so credentials written -// into a clone address never reach a log or an error. -func redact(s string) string { return userInfo.ReplaceAllString(s, "${1}***@") } +// into a clone address never reach a log or an error. An SSH login without a +// password is no credential and stays readable. +func redact(s string) string { + return userInfo.ReplaceAllStringFunc(s, func(m string) string { + p := userInfo.FindStringSubmatch(m) + if (p[1] == "ssh" || strings.HasSuffix(p[1], "+ssh")) && !strings.Contains(p[3], ":") { + return m + } + return p[1] + p[2] + "***@" + }) +} func redactAll(args []string) []string { out := make([]string, len(args)) diff --git a/internal/vcs/vcs_test.go b/internal/vcs/vcs_test.go index abfae2c..e09bf2e 100644 --- a/internal/vcs/vcs_test.go +++ b/internal/vcs/vcs_test.go @@ -8,10 +8,12 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strings" "testing" "github.com/Open-MBEE/SysPI/internal/digest" + "github.com/Open-MBEE/SysPI/internal/origin" "github.com/Open-MBEE/SysPI/internal/version" ) @@ -239,6 +241,17 @@ func TestRedactAndCheckedOutTime(t *testing.T) { if got := redact("git clone -- https://user:secret@host/repo.git"); got != "git clone -- https://***@host/repo.git" { t.Fatalf("redact = %q", got) } + for in, want := range map[string]string{ + "git ls-remote --tags -- ssh://git@host/repo.git": "git ls-remote --tags -- ssh://git@host/repo.git", + "svn ls -- svn+ssh://svn@host/repo/tags": "svn ls -- svn+ssh://svn@host/repo/tags", + "git clone -- ssh://git:secret@host/repo.git": "git clone -- ssh://***@host/repo.git", + "fatal: unable to access 'https://token@host/repo/'": "fatal: unable to access 'https://***@host/repo/'", + "git@host: Permission denied (publickey).": "git@host: Permission denied (publickey).", + } { + if got := redact(in); got != want { + t.Errorf("redact(%q) = %q, want %q", in, got, want) + } + } src, _ := Open("git", "https://user:secret@no-such-host.invalid/repo.git") if _, err := src.ListVersions(context.Background()); err == nil || strings.Contains(err.Error(), "secret") { t.Fatalf("credentials in error: %v", err) @@ -266,3 +279,58 @@ func TestGitRefusals(t *testing.T) { t.Fatal("unknown tag exported") } } + +// fakeTools puts git, hg and svn stand-ins first on PATH; each appends its +// arguments, one per line and a blank line after, to the returned log and fails. +func fakeTools(t *testing.T) string { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("the stand-in executables are POSIX shell scripts") + } + dir := t.TempDir() + log := filepath.Join(dir, "argv.log") + script := "#!/bin/sh\n{ printf '%s\\n' \"$(basename \"$0\")\" \"$@\"; echo; } >> '" + log + "'\necho 'stand-in: no such repository' >&2\nexit 1\n" + for _, name := range []string{"git", "hg", "svn"} { + if err := os.WriteFile(filepath.Join(dir, name), []byte(script), 0o755); err != nil { + t.Fatal(err) + } + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + return log +} + +// The user name of a +ssh location reaches each executable inside the +// address, after "--", and nowhere else on the command line. +func TestSSHUserNameArgv(t *testing.T) { + log := fakeTools(t) + ctx := context.Background() + for _, tc := range []struct{ resource, argv, shown string }{ + {"git+ssh://git@github.com/Open-MBEE/OpenSysML-Extensions-Library@1.0.0", + "git\nls-remote\n--tags\n--\nssh://git@github.com/Open-MBEE/OpenSysML-Extensions-Library\n", + "git ls-remote --tags -- ssh://git@github.com/Open-MBEE/OpenSysML-Extensions-Library: stand-in: no such repository"}, + {"svn+ssh://svn@svn.example.org/repos/pkg", + "svn\nls\n--non-interactive\n--\nsvn+ssh://svn@svn.example.org/repos/pkg/tags\n", + "svn ls --non-interactive -- svn+ssh://svn@svn.example.org/repos/pkg/tags: stand-in: no such repository"}, + {"hg+ssh://hg@hg.example.org/models/rover", + "hg\n--noninteractive\nclone\n--quiet\n--noupdate\n--\nssh://hg@hg.example.org/models/rover\n", + "-- ssh://hg@hg.example.org/models/rover "}, + } { + os.Remove(log) + o, err := origin.Parse(tc.resource) + if err != nil { + t.Fatal(err) + } + src, err := Open(o.VCS, o.Source) + if err != nil { + t.Fatal(err) + } + _, err = src.ListVersions(ctx) + if err == nil || !strings.Contains(err.Error(), tc.shown) { + t.Errorf("%s: error %v\n want it to hold %q", tc.resource, err, tc.shown) + } + got, _ := os.ReadFile(log) + if !strings.HasPrefix(string(got), tc.argv) || strings.Count(string(got), "@") != 1 { + t.Errorf("%s: argv\n%s\nwant prefix\n%s", tc.resource, got, tc.argv) + } + } +} diff --git a/tests/e2e/e2e_test.go b/tests/e2e/e2e_test.go index 26a0cee..556f9b1 100644 --- a/tests/e2e/e2e_test.go +++ b/tests/e2e/e2e_test.go @@ -316,6 +316,146 @@ func TestEndToEnd(t *testing.T) { } } +// sshStandIn writes a program git runs in place of ssh: it records the +// destination it is given and runs the remote command on this machine. +func sshStandIn(t *testing.T, root string) (program, log string) { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("the ssh stand-in is a POSIX shell script") + } + program, log = filepath.Join(root, "ssh-stand-in"), filepath.Join(root, "ssh.log") + script := "#!/bin/sh\nfor a; do command=$a; done\necho \"$1\" >> '" + log + "'\nexec sh -c \"$command\"\n" + if err := os.WriteFile(program, []byte(script), 0o755); err != nil { + t.Fatal(err) + } + return program, log +} + +// A git+ssh usage written with a user name and one written without are two +// identities, each fetched through its own address, sharing one lib/ directory. +func TestSSHUserName(t *testing.T) { + requireOrSkip(t, "git") + bin := syspiBinary(t) + root := t.TempDir() + repo := filepath.ToSlash(gitFixture(t, root)) + ssh, sshLog := sshStandIn(t, root) + env := []string{"GIT_SSH_COMMAND=" + ssh, "GIT_SSH_VARIANT=simple"} + noSSH := []string{"GIT_SSH_COMMAND=false", "GIT_SSH_VARIANT=simple"} + withUser, without := "git+ssh://git@fixture.invalid"+repo, "git+ssh://fixture.invalid"+repo + destinations := func() string { + b, _ := os.ReadFile(sshLog) + os.Remove(sshLog) + return strings.Join(strings.Fields(string(b)), " ") + } + read := func(app, name string) string { + b, err := os.ReadFile(filepath.Join(app, name)) + if err != nil { + t.Fatal(err) + } + return string(b) + } + identity := func(res string) string { + o, err := origin.Parse(res) + if err != nil { + t.Fatal(err) + } + return o.Identity() + } + + app := filepath.Join(root, "app") + os.MkdirAll(app, 0o755) + sh(t, app, bin, "init", "-name", "app") + if out, err := shErr(app, env, bin, "add", withUser+"@1.0.0"); err != nil { + t.Fatalf("add %s: %v\n%s", withUser, err, out) + } + if got := destinations(); got == "" || strings.ReplaceAll(got, "git@fixture.invalid", "") != strings.Repeat(" ", strings.Count(got, " ")) { + t.Errorf("ssh was given %q, want git@fixture.invalid every time", got) + } + if want := "\"resource\": \"" + identity(withUser) + "%401.0.0\""; !strings.Contains(read(app, ".project.json"), want) || !strings.Contains(want, "git%2Bssh:%2F%2Fgit%40fixture.invalid%2F") { + t.Errorf(".project.json lacks %s:\n%s", want, read(app, ".project.json")) + } + lk := read(app, ".project.lock.json") + if got, want := lockField(t, lk, withUser, "source"), "ssh://git@fixture.invalid"+repo; got != want { + t.Errorf("source %q, want %q", got, want) + } + if strings.Contains(lk, identity(without)+"\"") { + t.Errorf("the lock holds the identity without the user name:\n%s", lk) + } + lib, err := os.ReadDir(filepath.Join(app, "lib")) + if err != nil || len(lib) != 1 || lib[0].Name() != "acme-widgets_1.0.0" { + t.Fatalf("lib/ = %v %v, want acme-widgets_1.0.0 only", lib, err) + } + + // Same spelling again: matched in the lock, lib/ reused, no connection. + out, err := shErr(app, noSSH, bin, "install", "-offline") + if err != nil || !strings.Contains(out, "0 installed, 1 already present") { + t.Fatalf("install -offline: %v\n%s", err, out) + } + + // The other spelling is another identity: its own fetch and lock entry, the same directory. + if out, err := shErr(app, env, bin, "add", without+"@1.0.0"); err != nil { + t.Fatalf("add %s: %v\n%s", without, err, out) + } + if got := destinations(); got == "" || strings.Contains(got, "@") { + t.Errorf("ssh was given %q, want fixture.invalid without a user name", got) + } + lk = read(app, ".project.lock.json") + for _, f := range []string{"sha256", "revision", "version"} { + if a, b := lockField(t, lk, withUser, f), lockField(t, lk, without, f); a == "" || a != b { + t.Errorf("%s differs between the two identities: %q vs %q", f, a, b) + } + } + if got, want := lockField(t, lk, without, "source"), "ssh://fixture.invalid"+repo; got != want { + t.Errorf("source %q, want %q", got, want) + } + if lib, _ := os.ReadDir(filepath.Join(app, "lib")); len(lib) != 1 { + t.Errorf("lib/ holds %d entries, want the one shared directory", len(lib)) + } + if out, err := shErr(app, noSSH, bin, "install", "-offline"); err != nil || !strings.Contains(out, "0 installed, 2 already present") { + t.Fatalf("install -offline with both spellings: %v\n%s", err, out) + } + + // A lock entry of one spelling does not satisfy a usage of the other. + other := filepath.Join(root, "other") + os.MkdirAll(other, 0o755) + sh(t, other, bin, "init", "-name", "other") + if out, err := shErr(other, env, bin, "add", withUser+"@1.0.0"); err != nil { + t.Fatalf("add: %v\n%s", err, out) + } + manifest := read(other, ".project.json") + write(t, other, ".project.json", strings.Replace(manifest, "git%40fixture.invalid", "fixture.invalid", 1)) + out, err = shErr(other, noSSH, bin, "install", "-offline") + if err == nil || !strings.Contains(out, "offline and "+identity(without)+" is not installed") { + t.Fatalf("install -offline after dropping the user name: %v\n%s", err, out) + } + destinations() + if out, err := shErr(other, env, bin, "install"); err != nil { + t.Fatalf("install: %v\n%s", err, out) + } + if got := destinations(); got == "" || strings.Contains(got, "@") { + t.Errorf("ssh was given %q, want fixture.invalid without a user name", got) + } + if lk := read(other, ".project.lock.json"); strings.Contains(lk, "git%40") || lockField(t, lk, without, "version") != "1.0.0" { + t.Errorf("the lock was not rewritten for the new identity:\n%s", lk) + } + + // A user name on another transport, and a password, never reach git. + for res, want := range map[string]string{ + "git+file://git@" + repo + "@1.0.0": "a user name in the host is accepted only on git+ssh", + "git+https://git@fixture.invalid/a/b@1.0.0": "a user name in the host is accepted only on git+ssh", + "pkg:git/git@fixture.invalid/a/b@1.0.0": "a user name in the host is accepted only on git+ssh", + "git+ssh://git:hunter2@fixture.invalid" + repo: "a password in the host is not supported", + } { + out, err := shErr(app, env, bin, "add", res) + if err == nil || !strings.Contains(out, want) || strings.Contains(out, "hunter2@") && !strings.Contains(out, "resource \"") { + t.Errorf("add %s: %v\n%s\nwant %q", res, err, out, want) + } + } + if got := destinations(); got != "" { + t.Errorf("a refused resource reached ssh: %q", got) + } +} + func TestSameTreeFromGitHgSvn(t *testing.T) { requireOrSkip(t, "git", "hg", "svn", "svnadmin") bin := syspiBinary(t) From 37861177ef426fcf08607030bf0349bbdcd47ff5 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Thu, 12 Nov 2026 06:19:56 +0000 Subject: [PATCH 2/2] fix(origin): keep the version of a file location whose path follows :// directly A +file location has no authority, so its last @ is always the version's; on Windows the path may be written with backslashes and no slash ends it. Co-Authored-By: jason.han --- internal/origin/origin.go | 5 ++++- internal/origin/origin_test.go | 8 ++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/internal/origin/origin.go b/internal/origin/origin.go index c1813df..29e639b 100644 --- a/internal/origin/origin.go +++ b/internal/origin/origin.go @@ -295,7 +295,10 @@ func parseGitAlias(res, tail string) (*Origin, error) { // userInAuthority reports whether the last @ of spec lies in its authority, // where it ends a user name instead of starting a version. func userInAuthority(spec string) bool { - _, rest, _ := strings.Cut(spec, "://") + scheme, rest, _ := strings.Cut(spec, "://") + if scheme == "file" || strings.HasSuffix(scheme, "+file") { + return false // no authority: on Windows the path may follow "://" directly + } auth, tail, _ := strings.Cut(rest, "/") return strings.Contains(auth, "@") && !strings.Contains(tail, "@") } diff --git a/internal/origin/origin_test.go b/internal/origin/origin_test.go index 860b272..6056505 100644 --- a/internal/origin/origin_test.go +++ b/internal/origin/origin_test.go @@ -250,10 +250,18 @@ func TestDriveLetterFileURIs(t *testing.T) { if o.Path != tc.path || o.Source != tc.src { t.Errorf("windows=%v %s: path %q source %q, want %q %q", tc.windows, tc.in, o.Path, o.Source, tc.path, tc.src) } + if o.Version != "1.0.0" { + t.Errorf("windows=%v %s: version %q, want 1.0.0", tc.windows, tc.in, o.Version) + } if _, err := Parse(o.Canonical()); err != nil { t.Errorf("windows=%v canonical %s: %v", tc.windows, o.Canonical(), err) } } + // Backslashes after "://" leave no "/" to end an authority: the @ is still the version's. + windowsPaths = true + if o, err := Parse(`git+file://C:\srv\widgets@1.0.0`); err != nil || o.Version != "1.0.0" || strings.Contains(o.Source, "@") { + t.Errorf(`git+file://C:\srv\widgets@1.0.0: %v %+v`, err, o) + } windowsPaths = false if _, err := Parse("file://C:/tools.kpar"); err == nil { t.Error("file://C:/… must be refused where C: is a host, not a drive")