From d5da49b2a2802981fac5d4cefa88784414b6a4a1 Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 10 Nov 2026 12:29:09 -0600 Subject: [PATCH 1/2] fix(sync): pull only a clone of the listed repository, over the listed URL A folder with a repository's name used to be pulled through whatever origin it had. A same-named clone of a fork, of another owner's repository or of another host was reported Updated although the listed repository was never synced, and the git transport was whatever that folder's configuration said. - IGitClient.FetchAndPullAsync takes the clone URL the repository was listed with. A folder whose origin names a different repository fails its row before any request; the message names both, without any credentials stored in the origin. - The fetch always uses the listed URL. An origin that names the same repository in another spelling (SSH, plain HTTP, embedded credentials) is fetched through a remote that exists only for that call; origin is never rewritten, and tags and pruning behave as before. - The git credential callback answers only HTTPS requests for the listed URL's host and port. The default-branch retarget gets a provider of its own instead of sharing the fetch's replay counter. - WslCloner validates the organization and repository as single names, requires an HTTPS URL passed after "--", scopes the credential helper to the URL's host, and pulls an existing destination only when its origin is the listed URL. The view model passes the organization the table was loaded for. - The repository lister refuses a name that is not one plain name before making a request. New internal RemoteUrl holds the two decisions (same repository, may receive credentials) and is covered by table tests. Threat model: T14 added, T4, T8 and T9 updated. Closes #139 Co-Authored-By: Claude Fable 5.2 Signed-off-by: James Maes --- README.md | 2 +- docs/security/threat-model.md | 8 +- gclo.Cli.Tests/CliTestSupport.cs | 2 +- gclo.Engine.Tests/Fakes.cs | 6 +- gclo.Engine.Tests/GitHubListerTests.cs | 22 ++ gclo.Engine.Tests/GitTestHelpers.cs | 16 ++ gclo.Engine.Tests/LibGit2GitClientTests.cs | 30 +-- gclo.Engine.Tests/OrgSyncEngineTests.cs | 6 +- gclo.Engine.Tests/OriginVerificationTests.cs | 239 ++++++++++++++++++ gclo.Engine.Tests/PathRecoveryTests.cs | 24 +- gclo.Engine.Tests/RemoteUrlTests.cs | 172 +++++++++++++ .../WindowsPathValidationTests.cs | 2 +- gclo.Engine.Tests/WorkspaceViewModelTests.cs | 21 ++ gclo.Engine.Tests/WslClonerTests.cs | 79 +++++- gclo.Engine/GitHubRepositoryLister.cs | 12 + gclo.Engine/IGitClient.cs | 15 +- gclo.Engine/LibGit2GitClient.cs | 125 ++++++++- gclo.Engine/OrgSyncEngine.cs | 2 +- gclo.Engine/RemoteUrl.cs | 158 ++++++++++++ gclo.Engine/WslCloner.cs | 41 ++- gclo.ViewModels/WorkspaceViewModel.cs | 4 +- gclo/UiTestFixture.cs | 4 +- 22 files changed, 923 insertions(+), 67 deletions(-) create mode 100644 gclo.Engine.Tests/OriginVerificationTests.cs create mode 100644 gclo.Engine.Tests/RemoteUrlTests.cs create mode 100644 gclo.Engine/RemoteUrl.cs diff --git a/README.md b/README.md index a027f59..a2b00fc 100644 --- a/README.md +++ b/README.md @@ -186,7 +186,7 @@ Full reference — all options, output streams, Ctrl+C behavior, and more exampl ## How it works -The engine lists every repository through the GitHub REST API (Octokit, paginated 100 per page), then runs the set through `Parallel.ForEachAsync` bounded by the parallelism setting. Git operations use LibGit2Sharp: missing repos are cloned with object-transfer progress; existing repos get a fetch from `origin` followed by a **fast-forward-only** merge — if a local branch has diverged from its upstream, the repo is reported Failed rather than auto-merged, so gclo never manufactures merge commits. Archived repositories are included and sync normally. Every repo ends in exactly one terminal state — Done, Failed (with the error), or Canceled — and per-repo exceptions are contained so the rest of the run continues. The PAT is the only credential; on the git side it is sent as HTTPS basic credentials with username `x-access-token`. +The engine lists every repository through the GitHub REST API (Octokit, paginated 100 per page), then runs the set through `Parallel.ForEachAsync` bounded by the parallelism setting. Git operations use LibGit2Sharp: missing repos are cloned with object-transfer progress; existing repos get a fetch from `origin` followed by a **fast-forward-only** merge — if a local branch has diverged from its upstream, the repo is reported Failed rather than auto-merged, so gclo never manufactures merge commits. An existing folder is only updated when its `origin` is the repository gclo listed: a same-named folder that is a clone of something else (a fork, another owner's repository, another host) is reported Failed and left untouched. Archived repositories are included and sync normally. Every repo ends in exactly one terminal state — Done, Failed (with the error), or Canceled — and per-repo exceptions are contained so the rest of the run continues. The PAT is the only credential; on the git side it is sent as HTTPS basic credentials with username `x-access-token`, and only to the host the repository was listed on. ## Build from source diff --git a/docs/security/threat-model.md b/docs/security/threat-model.md index d676c74..07b0f43 100644 --- a/docs/security/threat-model.md +++ b/docs/security/threat-model.md @@ -86,15 +86,16 @@ flowchart LR | T1 | Update feed | Tampering | Attacker serves a malicious update | Low | Critical | HTTPS to this repository's Releases only; Velopack hash verification; immutable releases; provenance and SBOM attestations; Scorecard; Authenticode pending (EX-0002) | Mitigated (signing pending) | | T2 | Package managers / downloads | Tampering | Modified installer or CLI zip on a mirror | Low | High | `SHA256SUMS`; Scoop and Chocolatey manifests pin the hash; "Verifying a release" instructions | Mitigated | | T3 | Git remotes | Tampering / EoP | Repository with Windows-invalid or traversal paths (`..`, reserved names, case collisions) writes outside or corrupts the target | Medium | High | Pre-checkout validation of every incoming path; `RepositoryPathResolver` containment; recovery by rename/skip or clone in WSL; `core.longpaths` | Mitigated | -| T4 | Git remotes | Tampering | Hostile `.git` config or hooks in a pre-existing target repository | Low | High | gclo runs no hooks (libgit2, no shell); only fetch and fast-forward; `gclo.checkoutpending` marks incomplete trees | Mitigated | +| T4 | Git remotes | Tampering | Hostile `.git` config or hooks in a pre-existing target repository | Low | High | gclo runs no hooks (libgit2, no shell); only fetch and fast-forward; `gclo.checkoutpending` marks incomplete trees; a folder is fetched only when its `origin` names the listed repository, and always over the listed URL, so the folder's configuration chooses neither the remote nor the transport (`RemoteUrl`, #139) | Mitigated for the remote and transport; other `.git` content of an existing folder is still trusted | | T5 | PAT handling | Information disclosure | Token leaks to disk, logs, process listings, or UI | Medium | Critical | Memory-only in Quick Sync; Credential Manager otherwise; accounts may reference the default token (`TokenSource`), resolved at use time through one resolver and never copied; logs record lengths; no `--token`; WSL via env only; secret scanning + gitleaks + CodeQL in CI | Mitigated | | T6 | Credential Manager | Information disclosure | Another process as the same user reads the stored token | Medium | High | Documented boundary (SECURITY.md); vault scoped per data directory so test or portable instances never see the default profile's tokens | Accepted (OS boundary) | | T7 | GitHub API | Denial of service | Rate limits or abuse detection stop a sync | Medium | Low | Secondary-rate-limit and abuse handling with backoff; per-repo failure isolation; cancellation | Mitigated | -| T8 | GitHub API / remotes | Spoofing | A MITM impersonates GitHub | Low | Critical | HTTPS with system trust store (Octokit, libgit2); no HTTP fallback | Mitigated | -| T9 | WSL recovery | Elevation of privilege | Hostile org/repo names injected into the shell script | Low | High | Script fed on stdin with positional parameters; names validated as repository names first; no string interpolation into the script | Mitigated | +| T8 | GitHub API / remotes | Spoofing | A MITM impersonates GitHub | Low | Critical | HTTPS with system trust store (Octokit, libgit2); no HTTP fallback: an existing folder whose origin is `http://` or SSH is fetched over the listed HTTPS URL instead | Mitigated | +| T9 | WSL recovery | Elevation of privilege | Hostile org/repo names injected into the shell script, or used to leave `~/gclo` | Low | High | Script fed on stdin with positional parameters; organization and repository each validated as one plain name inside `WslCloner` (and the organization again before any listing request); the URL must be HTTPS and is passed after `--`; no string interpolation into the script | Mitigated | | T10 | Logs and crash output | Information disclosure | Exception text carries a token or private path | Low | Medium | Crash net logs messages only; log review in incident handling; tokens never in exception messages (tests) | Mitigated | | T11 | Release pipeline | Tampering | Compromised action or cache poisons a release | Low | Critical | Build runs in the standards' shared release workflow (SLSA Build L3 provenance, `--signer-repo KofTwentyTwo/standards`), SHA-pinned actions, no caches, read-only tokens, publishing tokens only in the tag-restricted `release` environment, `protect-release-tags`, required gates re-run on the tag | Mitigated | | T12 | Repository | Repudiation | Unattributed change reaches `main` | Low | High | Signed, signed-off commits; squash-only through gated pull requests; no bypass actors | Mitigated | +| T14 | PAT handling | Information disclosure | The token is offered to a host other than the one the repository was listed on: an existing folder whose `origin` points elsewhere, a redirect, or a server named by the repository's own configuration during the WSL clone (for example an LFS endpoint) | Low | Critical | The git credential callback answers only HTTPS requests for the listed URL's host and port, and refuses everything else with a clear error; a folder whose origin names another repository fails before any request; the WSL credential helper is scoped to the listed host (`credential.https://.helper`) and an existing WSL destination is pulled only when its origin is the listed URL (#139) | Mitigated | | T13 | CLI / data directory | Tampering | A malicious `accounts.json` or `settings.json` redirects targets | Low | Medium | Files live under the user profile; sanitized on load; target paths resolved and contained | Accepted (same-user boundary) | ## 7. Accepted risks @@ -115,3 +116,4 @@ flowchart LR | 2026-10-10 | v1.0.2 | James Maes (@KofTwentyTwo) | Local data directory moved to `%LOCALAPPDATA%\KofTwentyTwo\gclo`: the Velopack install root is deleted by Setup.exe, which destroyed accounts.json and settings.json (#91) | | 2026-10-10 | v1.0.2 | James Maes (@KofTwentyTwo) | T2/T11: releases are Authenticode-signed with Azure Artifact Signing over GitHub OIDC (no signing secret; federated credential bound to this repository's `release` environment; signer role scoped to the certificate profile) and the published assets are verified with `signtool verify /pa /all` (#57, ADR 0008) | | 2026-10-10 | v1.1.0 | James Maes (@KofTwentyTwo) | T5: accounts can reference the default token (`TokenSource = Default`) instead of holding a copy; one `AccountTokenResolver` serves the app and the CLI, so rotating the default token in Settings rotates every such account (#102, docs/plans/default-token-ux.md) | +| 2026-11-10 | v1.1.1 | James Maes (@KofTwentyTwo) | T14 added, T4/T8/T9 updated: an existing folder is fetched only when its origin names the listed repository and always over the listed URL; the git credential callback and the WSL credential helper answer only for the listed host over HTTPS; WSL organization and repository names validated as single names (#139) | diff --git a/gclo.Cli.Tests/CliTestSupport.cs b/gclo.Cli.Tests/CliTestSupport.cs index 1b95cea..69c7fa1 100644 --- a/gclo.Cli.Tests/CliTestSupport.cs +++ b/gclo.Cli.Tests/CliTestSupport.cs @@ -147,7 +147,7 @@ internal sealed class FakeGit : IGitClient - public Task FetchAndPullAsync(string path, string token, CancellationToken cancellationToken) + public Task FetchAndPullAsync(string url, string path, string token, CancellationToken cancellationToken) => OnPull(path, token, cancellationToken); diff --git a/gclo.Engine.Tests/Fakes.cs b/gclo.Engine.Tests/Fakes.cs index 576f12f..3369957 100644 --- a/gclo.Engine.Tests/Fakes.cs +++ b/gclo.Engine.Tests/Fakes.cs @@ -43,7 +43,7 @@ public sealed record CloneCall(string Url, string LocalPath, string Token); -public sealed record PullCall(string LocalPath, string Token); +public sealed record PullCall(string LocalPath, string Token, string Url); @@ -126,9 +126,9 @@ public sealed class FakeGitClient : IGitClient - public async Task FetchAndPullAsync(string path, string token, CancellationToken cancellationToken) + public async Task FetchAndPullAsync(string url, string path, string token, CancellationToken cancellationToken) { - _pullCalls.Enqueue(new PullCall(path, token)); + _pullCalls.Enqueue(new PullCall(path, token, url)); await FetchAndPullHandler(path, token, cancellationToken).ConfigureAwait(false); } diff --git a/gclo.Engine.Tests/GitHubListerTests.cs b/gclo.Engine.Tests/GitHubListerTests.cs index 59fabc0..cd9d5fe 100644 --- a/gclo.Engine.Tests/GitHubListerTests.cs +++ b/gclo.Engine.Tests/GitHubListerTests.cs @@ -206,6 +206,28 @@ public sealed class GitHubListerTests + [Theory] + [InlineData("../..")] + [InlineData("..")] + [InlineData("acme/other")] + [InlineData(@"acme\other")] + [InlineData("C:acme")] + public async Task Repos_NameThatIsNotOnePlainName_IsRefusedBeforeAnyRequest(string organization) + { + bool asked = false; + _gateway.OrgPages = (_, _) => { asked = true; return Task.FromResult>([]); }; + _gateway.AccountKind = _ => { asked = true; return Task.FromResult(GitHubAccountKind.User); }; + + GitHubAccessException ex = await Assert.ThrowsAsync( + () => RepoLister().ListOrganizationRepositoriesAsync(organization, "tok-1234567890")); + + Assert.Equal(GitHubAccessKind.NotFound, ex.Kind); + Assert.Contains($"'{organization}' is not an organization or account name", ex.Message); + Assert.False(asked, "no request may be made for a name that is not a name"); + } + + + [Fact] public async Task Repos_OrgNotFound_ButItIsAnOrganizationTheTokenCannotSee_FailsInsteadOfListingPublicRepos() { diff --git a/gclo.Engine.Tests/GitTestHelpers.cs b/gclo.Engine.Tests/GitTestHelpers.cs index 39ac731..68f82d5 100644 --- a/gclo.Engine.Tests/GitTestHelpers.cs +++ b/gclo.Engine.Tests/GitTestHelpers.cs @@ -51,6 +51,22 @@ internal static class GitTestHelpers + /// + /// Pulls the way a sync does for a folder that is a clone + /// of the listed repository: with the URL of its own origin. + /// + public static Task PullAsync(this LibGit2GitClient client, string workdir, string token, CancellationToken cancellationToken) + { + string url; + using(var repo = new Repository(workdir)) + { + url = repo.Network.Remotes["origin"].Url; + } + return client.FetchAndPullAsync(url, workdir, token, cancellationToken); + } + + + /// SHA of the commit at HEAD's tip. public static string HeadSha(string workdir) { diff --git a/gclo.Engine.Tests/LibGit2GitClientTests.cs b/gclo.Engine.Tests/LibGit2GitClientTests.cs index 8eb19f3..81e2013 100644 --- a/gclo.Engine.Tests/LibGit2GitClientTests.cs +++ b/gclo.Engine.Tests/LibGit2GitClientTests.cs @@ -165,14 +165,14 @@ public sealed class LibGit2GitClientTests : IDisposable _client.AfterFetchForTesting = cts.Cancel; await Assert.ThrowsAnyAsync( - () => _client.FetchAndPullAsync(target, Token, cts.Token)); + () => _client.PullAsync(target, Token, cts.Token)); Assert.Equal(tipBefore, HeadSha(target)); Assert.False(File.Exists(Path.Combine(target, "update.txt"))); // The fetch itself landed: a later uncanceled pull is a plain fast-forward. _client.AfterFetchForTesting = null; - await _client.FetchAndPullAsync(target, Token, CancellationToken.None); + await _client.PullAsync(target, Token, CancellationToken.None); Assert.True(File.Exists(Path.Combine(target, "update.txt"))); } @@ -249,7 +249,7 @@ public sealed class LibGit2GitClientTests : IDisposable string newSha = CommitFile(source, "update.txt", "new content", "second commit"); - await _client.FetchAndPullAsync(target, Token, CancellationToken.None); + await _client.PullAsync(target, Token, CancellationToken.None); Assert.Equal(newSha, HeadSha(target)); Assert.Equal("new content", File.ReadAllText(Path.Combine(target, "update.txt"))); @@ -265,7 +265,7 @@ public sealed class LibGit2GitClientTests : IDisposable await _client.CloneAsync(source, target, Token, null, CancellationToken.None); string tipBefore = HeadSha(target); - await _client.FetchAndPullAsync(target, Token, CancellationToken.None); + await _client.PullAsync(target, Token, CancellationToken.None); Assert.Equal(tipBefore, HeadSha(target)); } @@ -287,7 +287,7 @@ public sealed class LibGit2GitClientTests : IDisposable SetCheckoutPending(target, true); Assert.True(_client.IsValidRepository(target)); - await _client.FetchAndPullAsync(target, Token, CancellationToken.None); + await _client.PullAsync(target, Token, CancellationToken.None); Assert.Equal("hello from source", File.ReadAllText(Path.Combine(target, "readme.txt"))); Assert.Equal(tipBefore, HeadSha(target)); @@ -306,7 +306,7 @@ public sealed class LibGit2GitClientTests : IDisposable SetCheckoutPending(target, true); string newSha = CommitFile(source, "update.txt", "new content", "second commit"); - await _client.FetchAndPullAsync(target, Token, CancellationToken.None); + await _client.PullAsync(target, Token, CancellationToken.None); Assert.Equal(newSha, HeadSha(target)); Assert.Equal("hello from source", File.ReadAllText(Path.Combine(target, "readme.txt"))); @@ -335,7 +335,7 @@ public sealed class LibGit2GitClientTests : IDisposable } InvalidRepositoryPathsException ex = await Assert.ThrowsAsync( - () => _client.FetchAndPullAsync(target, Token, CancellationToken.None)); + () => _client.PullAsync(target, Token, CancellationToken.None)); Assert.Equal("bad:name.txt", Assert.Single(ex.Paths).RepoPath); Assert.True(IsCheckoutPending(target), "still incomplete; the marker must survive the failure"); @@ -356,7 +356,7 @@ public sealed class LibGit2GitClientTests : IDisposable CommitFile(target, "from-local.txt", "local change", "local commit"); InvalidOperationException ex = await Assert.ThrowsAsync( - () => _client.FetchAndPullAsync(target, Token, CancellationToken.None)); + () => _client.PullAsync(target, Token, CancellationToken.None)); // Actionable, not libgit2's "cannot fast-forward": names the branch, says what // gclo refuses to do, and gives the three ways out (#30). @@ -382,7 +382,7 @@ public sealed class LibGit2GitClientTests : IDisposable } InvalidOperationException ex = await Assert.ThrowsAsync( - () => _client.FetchAndPullAsync(target, Token, CancellationToken.None)); + () => _client.PullAsync(target, Token, CancellationToken.None)); Assert.Contains("detached", ex.Message, StringComparison.OrdinalIgnoreCase); Assert.Contains("Check out a branch", ex.Message); @@ -408,7 +408,7 @@ public sealed class LibGit2GitClientTests : IDisposable } string newSha = CommitFile(source, "after-rename.txt", "after", "commit on the renamed branch"); - await _client.FetchAndPullAsync(target, Token, CancellationToken.None); + await _client.PullAsync(target, Token, CancellationToken.None); using var after = new Repository(target); Assert.Equal("renamed-main", after.Head.FriendlyName); @@ -419,7 +419,7 @@ public sealed class LibGit2GitClientTests : IDisposable Assert.Null(after.Branches["origin/" + oldName]); // and so is its pruned tracking ref // The next pull is an ordinary up-to-date pull on the new branch. - await _client.FetchAndPullAsync(target, Token, CancellationToken.None); + await _client.PullAsync(target, Token, CancellationToken.None); Assert.Equal(newSha, HeadSha(target)); } @@ -439,7 +439,7 @@ public sealed class LibGit2GitClientTests : IDisposable } InvalidOperationException ex = await Assert.ThrowsAsync( - () => _client.FetchAndPullAsync(target, Token, CancellationToken.None)); + () => _client.PullAsync(target, Token, CancellationToken.None)); Assert.Contains("local commits", ex.Message); Assert.Contains("never discards local work", ex.Message); @@ -467,7 +467,7 @@ public sealed class LibGit2GitClientTests : IDisposable string remoteSha = CommitFile(source, "later.txt", "later content", "remote commit"); - await _client.FetchAndPullAsync(target, Token, CancellationToken.None); + await _client.PullAsync(target, Token, CancellationToken.None); using var after = new Repository(target); Assert.Equal("local-only", after.Head.FriendlyName); @@ -501,7 +501,7 @@ public sealed class LibGit2GitClientTests : IDisposable repo.Network.Push(repo.Network.Remotes["origin"], repo.Head.CanonicalName, new PushOptions()); } - await _client.FetchAndPullAsync(target, Token, CancellationToken.None); + await _client.PullAsync(target, Token, CancellationToken.None); using var after = new Repository(target); Assert.False(after.Info.IsHeadUnborn); @@ -522,7 +522,7 @@ public sealed class LibGit2GitClientTests : IDisposable await _client.CloneAsync(origin, target, Token, null, CancellationToken.None); // Nothing to pull yet: there is no branch to materialize, and that is not an error. - await _client.FetchAndPullAsync(target, Token, CancellationToken.None); + await _client.PullAsync(target, Token, CancellationToken.None); using var after = new Repository(target); Assert.True(after.Info.IsHeadUnborn); diff --git a/gclo.Engine.Tests/OrgSyncEngineTests.cs b/gclo.Engine.Tests/OrgSyncEngineTests.cs index b762f59..126ca08 100644 --- a/gclo.Engine.Tests/OrgSyncEngineTests.cs +++ b/gclo.Engine.Tests/OrgSyncEngineTests.cs @@ -62,7 +62,11 @@ public sealed class OrgSyncEngineTests : IDisposable } foreach(PullCall call in _git.PullCalls) { - Assert.Equal(Path.Combine(_targetRoot, Path.GetFileName(call.LocalPath)!), call.LocalPath); + string name = Path.GetFileName(call.LocalPath)!; + Assert.Equal(Path.Combine(_targetRoot, name), call.LocalPath); + // The listed clone URL travels with the pull, so the git client can refuse a + // folder that is a clone of something else (#139). + Assert.Equal($"https://example.test/acme/{name}.git", call.Url); Assert.Equal("test-token", call.Token); } diff --git a/gclo.Engine.Tests/OriginVerificationTests.cs b/gclo.Engine.Tests/OriginVerificationTests.cs new file mode 100644 index 0000000..9b11bd9 --- /dev/null +++ b/gclo.Engine.Tests/OriginVerificationTests.cs @@ -0,0 +1,239 @@ +/* + * Copyright (c) 2026 James Maes (KofTwentyTwo) + * SPDX-License-Identifier: MIT + */ + +using LibGit2Sharp; +using static gclo.Engine.Tests.GitTestHelpers; + + +namespace gclo.Engine.Tests; + + +/// +/// A pull is given the clone URL the repository was listed with, and only pulls a +/// folder that is a clone of that repository (#139). Real repositories on the local +/// file system, like . +/// +public sealed class OriginVerificationTests : IDisposable +{ + private const string Token = "test-token"; + + private readonly string _root = + Path.Combine(Path.GetTempPath(), "gclo-tests", Guid.NewGuid().ToString("N")); + + private readonly LibGit2GitClient _client = new(); + + + + public OriginVerificationTests() + { + Directory.CreateDirectory(_root); + } + + + + public void Dispose() => TryDeleteDirectory(_root); + + + + [Fact] + public async Task Pull_FolderIsACloneOfAnotherRepository_FailsWithoutFetching() + { + string listed = CreateSourceRepo("listed"); + string unrelated = CreateSourceRepo("unrelated"); + string target = Path.Combine(_root, "clone"); + await _client.CloneAsync(unrelated, target, Token, null, CancellationToken.None); + string tipBefore = HeadSha(target); + string trackingBefore = TrackingTip(target); + // Something to fetch from the folder's own origin, had it been contacted. + CommitFile(unrelated, "more.txt", "more", "second commit"); + + InvalidOperationException ex = await Assert.ThrowsAsync( + () => _client.FetchAndPullAsync(listed, target, Token, CancellationToken.None)); + + Assert.Contains("clone of a different repository", ex.Message); + Assert.Contains(unrelated, ex.Message); + Assert.Contains(listed, ex.Message); + Assert.Equal(tipBefore, HeadSha(target)); + Assert.Equal(trackingBefore, TrackingTip(target)); + } + + + + [Fact] + public async Task Pull_OriginSpelledDifferently_IsTheSameRepository_AndIsPulledFromTheListedUrl() + { + string source = CreateSourceRepo("source"); + string target = Path.Combine(_root, "clone"); + await _client.CloneAsync(source, target, Token, null, CancellationToken.None); + using(var repo = new Repository(target)) + { + // The same folder, written as a file URI: what counts is which repository + // the origin names, not how the URL is spelled. + repo.Network.Remotes.Update("origin", r => r.Url = new Uri(source).AbsoluteUri); + } + string newSha = CommitFile(source, "update.txt", "new content", "second commit"); + + await _client.FetchAndPullAsync(source, target, Token, CancellationToken.None); + + Assert.Equal(newSha, HeadSha(target)); + Assert.Equal(newSha, TrackingTip(target)); + Assert.Equal("new content", File.ReadAllText(Path.Combine(target, "update.txt"))); + using(var repo = new Repository(target)) + { + // The folder's configuration is as the user left it: origin keeps its own + // spelling, and the remote used for the fetch is gone. + Assert.Equal(new Uri(source).AbsoluteUri, repo.Network.Remotes["origin"].Url); + Assert.Equal("origin", Assert.Single(repo.Network.Remotes).Name); + } + } + + + + [Fact] + public async Task Pull_OriginSpelledDifferently_StillFollowsTags_AndPrunesDeletedBranches() + { + string source = CreateSourceRepo("source"); + using(var repo = new Repository(source)) + { + repo.CreateBranch("topic"); + } + string target = Path.Combine(_root, "clone"); + await _client.CloneAsync(source, target, Token, null, CancellationToken.None); + using(var repo = new Repository(target)) + { + repo.Network.Remotes.Update("origin", r => r.Url = new Uri(source).AbsoluteUri); + } + string newSha = CommitFile(source, "update.txt", "new content", "second commit"); + using(var repo = new Repository(source)) + { + repo.ApplyTag("v1.0.0"); + repo.Branches.Remove("topic"); + } + + await _client.FetchAndPullAsync(source, target, Token, CancellationToken.None); + + using(var repo = new Repository(target)) + { + Assert.Equal(newSha, repo.Head.Tip.Sha); + Assert.NotNull(repo.Tags["v1.0.0"]); + Assert.Null(repo.Branches["origin/topic"]); + // Removing the fetch's remote must not take origin's tracking refs with it. + Assert.NotNull(repo.Head.TrackedBranch?.Tip); + } + } + + + + [Fact] + public async Task Pull_OriginSpelledDifferently_ARemoteLeftByAKilledRun_IsReplaced_NotFetched() + { + string source = CreateSourceRepo("source"); + string target = Path.Combine(_root, "clone"); + await _client.CloneAsync(source, target, Token, null, CancellationToken.None); + using(var repo = new Repository(target)) + { + repo.Network.Remotes.Update("origin", r => r.Url = new Uri(source).AbsoluteUri); + // What a process killed mid-fetch leaves behind, pointing somewhere stale. + repo.Network.Remotes.Add("gclo-sync", Path.Combine(_root, "gone")); + } + string newSha = CommitFile(source, "update.txt", "new content", "second commit"); + + await _client.FetchAndPullAsync(source, target, Token, CancellationToken.None); + + Assert.Equal(newSha, HeadSha(target)); + using(var repo = new Repository(target)) + { + Assert.Equal("origin", Assert.Single(repo.Network.Remotes).Name); + } + } + + + + [Fact] + public async Task Pull_StillFollowsTags_AndPrunesDeletedBranches() + { + // The fetch names the listed URL rather than the remote "origin"; it must keep + // doing what a fetch of origin does: bring tags on fetched commits and drop + // tracking refs of branches origin no longer has. + string source = CreateSourceRepo("source"); + using(var repo = new Repository(source)) + { + repo.CreateBranch("topic"); + } + string target = Path.Combine(_root, "clone"); + await _client.CloneAsync(source, target, Token, null, CancellationToken.None); + using(var repo = new Repository(target)) + { + Assert.NotNull(repo.Branches["origin/topic"]); + } + CommitFile(source, "update.txt", "new content", "second commit"); + using(var repo = new Repository(source)) + { + repo.ApplyTag("v1.0.0"); + repo.Branches.Remove("topic"); + } + + await _client.PullAsync(target, Token, CancellationToken.None); + + using(var repo = new Repository(target)) + { + Assert.NotNull(repo.Tags["v1.0.0"]); + Assert.Null(repo.Branches["origin/topic"]); + } + } + + + + [Fact] + public async Task Pull_MismatchMessage_NeverShowsCredentialsStoredInTheOrigin() + { + string listed = CreateSourceRepo("listed"); + string target = Path.Combine(_root, "clone"); + await _client.CloneAsync(listed, target, Token, null, CancellationToken.None); + using(var repo = new Repository(target)) + { + repo.Network.Remotes.Update("origin", r => r.Url = "https://someone:hunter2@example.test/acme/other.git"); + } + + InvalidOperationException ex = await Assert.ThrowsAsync( + () => _client.FetchAndPullAsync(listed, target, Token, CancellationToken.None)); + + Assert.Contains("https://example.test/acme/other.git", ex.Message); + Assert.DoesNotContain("hunter2", ex.Message); + Assert.DoesNotContain("someone", ex.Message); + } + + + + // ---------------------------------------------------------------- helpers + + private string CreateSourceRepo(string name) + { + string path = Path.Combine(_root, name); + Repository.Init(path); + CommitFile(path, "readme.txt", "hello from " + name, "initial commit"); + return path; + } + + + + private static string CommitFile(string workdir, string fileName, string content, string message) + { + using var repo = new Repository(workdir); + File.WriteAllText(Path.Combine(workdir, fileName), content); + Commands.Stage(repo, fileName); + Signature signature = MakeSignature(); + return repo.Commit(message, signature, signature).Sha; + } + + + + /// Tip of the remote-tracking ref of the clone's current branch. + private static string TrackingTip(string workdir) + { + using var repo = new Repository(workdir); + return repo.Head.TrackedBranch.Tip.Sha; + } +} diff --git a/gclo.Engine.Tests/PathRecoveryTests.cs b/gclo.Engine.Tests/PathRecoveryTests.cs index 30a8c40..381d85c 100644 --- a/gclo.Engine.Tests/PathRecoveryTests.cs +++ b/gclo.Engine.Tests/PathRecoveryTests.cs @@ -217,7 +217,7 @@ public sealed class PathRecoveryTests : IDisposable // the pull fails typed, exactly like the original clone did. AppendForgedCommit(source, ("worse:new.txt", "new content")); InvalidRepositoryPathsException ex = await Assert.ThrowsAsync( - () => _client.FetchAndPullAsync(target, Token, CancellationToken.None)); + () => _client.PullAsync(target, Token, CancellationToken.None)); Assert.Contains(ex.Paths, p => string.Equals(p.RepoPath, "worse:new.txt", StringComparison.Ordinal)); // Applying a recovery that covers ONLY the new path merges with the stored one: @@ -245,7 +245,7 @@ public sealed class PathRecoveryTests : IDisposable Assert.True(File.Exists(Path.Combine(target, "gone.txt"))); ReplaceForgedEntry(source, remove: "gone.txt", add: ("new/nested.txt", "new")); - await _client.FetchAndPullAsync(target, Token, CancellationToken.None); + await _client.PullAsync(target, Token, CancellationToken.None); // A real checkout removes what left the tree; the materialization must too. Assert.False(File.Exists(Path.Combine(target, "gone.txt"))); @@ -254,7 +254,7 @@ public sealed class PathRecoveryTests : IDisposable // And a directory emptied by a later deletion is pruned too. ReplaceForgedEntry(source, remove: "new/nested.txt", add: ("other.txt", "o")); - await _client.FetchAndPullAsync(target, Token, CancellationToken.None); + await _client.PullAsync(target, Token, CancellationToken.None); Assert.False(Directory.Exists(Path.Combine(target, "new"))); } @@ -269,12 +269,12 @@ public sealed class PathRecoveryTests : IDisposable // A local edit is the witness: an unchanged tip must leave the tree alone. File.WriteAllText(Path.Combine(target, "keep.txt"), "locally edited"); - await _client.FetchAndPullAsync(target, Token, CancellationToken.None); + await _client.PullAsync(target, Token, CancellationToken.None); Assert.Equal("locally edited", File.ReadAllText(Path.Combine(target, "keep.txt"))); // A new upstream commit re-materializes through the mapping as before. ReplaceForgedEntry(source, remove: "keep.txt", add: ("keep.txt", "upstream edit")); - await _client.FetchAndPullAsync(target, Token, CancellationToken.None); + await _client.PullAsync(target, Token, CancellationToken.None); Assert.Equal("upstream edit", File.ReadAllText(Path.Combine(target, "keep.txt"))); } @@ -290,7 +290,7 @@ public sealed class PathRecoveryTests : IDisposable File.WriteAllText(file, "{ \"SegmentRenames\": { truncated"); InvalidOperationException pullError = await Assert.ThrowsAsync( - () => _client.FetchAndPullAsync(target, Token, CancellationToken.None)); + () => _client.PullAsync(target, Token, CancellationToken.None)); Assert.Contains("corrupt", pullError.Message); Assert.Contains(file, pullError.Message); Assert.Contains("Delete it", pullError.Message); @@ -302,7 +302,7 @@ public sealed class PathRecoveryTests : IDisposable // "null" is a corrupt document too. File.WriteAllText(file, "null"); InvalidOperationException nullError = await Assert.ThrowsAsync( - () => _client.FetchAndPullAsync(target, Token, CancellationToken.None)); + () => _client.PullAsync(target, Token, CancellationToken.None)); Assert.Contains("empty", nullError.Message); } @@ -337,7 +337,7 @@ public sealed class PathRecoveryTests : IDisposable // The repo must not silently report success with an empty working tree. InvalidRepositoryPathsException ex = await Assert.ThrowsAsync( - () => _client.FetchAndPullAsync(target, Token, CancellationToken.None)); + () => _client.PullAsync(target, Token, CancellationToken.None)); Assert.Contains(ex.Paths, p => string.Equals(p.RepoPath, "bad:file.txt", StringComparison.Ordinal)); Assert.True(IsCheckoutPending(target)); @@ -353,7 +353,7 @@ public sealed class PathRecoveryTests : IDisposable ReplaceForgedEntry(source, remove: "bad:file.txt", add: ("fixed.txt", "fixed content")); - await _client.FetchAndPullAsync(target, Token, CancellationToken.None); + await _client.PullAsync(target, Token, CancellationToken.None); Assert.Equal("good content", File.ReadAllText(Path.Combine(target, "good.txt"))); Assert.Equal("fixed content", File.ReadAllText(Path.Combine(target, "fixed.txt"))); @@ -376,7 +376,7 @@ public sealed class PathRecoveryTests : IDisposable // content at the mapped name. string newSha = AppendForgedCommit(source, ("bad:file.txt", "v2")); - await _client.FetchAndPullAsync(target, Token, CancellationToken.None); + await _client.PullAsync(target, Token, CancellationToken.None); Assert.Equal("v2", File.ReadAllText(Path.Combine(target, "bad_file.txt"))); Assert.Equal("g1", File.ReadAllText(Path.Combine(target, "good.txt"))); @@ -396,7 +396,7 @@ public sealed class PathRecoveryTests : IDisposable AppendForgedCommit(source, ("worse:new.txt", "not covered")); InvalidRepositoryPathsException ex = await Assert.ThrowsAsync( - () => _client.FetchAndPullAsync(target, Token, CancellationToken.None)); + () => _client.PullAsync(target, Token, CancellationToken.None)); Assert.Contains(ex.Paths, p => string.Equals(p.RepoPath, "worse:new.txt", StringComparison.Ordinal)); // The previously materialized working tree is untouched. @@ -415,7 +415,7 @@ public sealed class PathRecoveryTests : IDisposable Assert.False(IsCheckoutPending(target)); Assert.False(File.Exists(RecoveryFilePath(target))); - await _client.FetchAndPullAsync(target, Token, CancellationToken.None); + await _client.PullAsync(target, Token, CancellationToken.None); Assert.False(IsCheckoutPending(target)); Assert.False(File.Exists(RecoveryFilePath(target))); Assert.Equal("hello", File.ReadAllText(Path.Combine(target, "readme.txt"))); diff --git a/gclo.Engine.Tests/RemoteUrlTests.cs b/gclo.Engine.Tests/RemoteUrlTests.cs new file mode 100644 index 0000000..82c20e8 --- /dev/null +++ b/gclo.Engine.Tests/RemoteUrlTests.cs @@ -0,0 +1,172 @@ +/* + * Copyright (c) 2026 James Maes (KofTwentyTwo) + * SPDX-License-Identifier: MIT + */ + +namespace gclo.Engine.Tests; + + +/// +/// Pins the two decisions makes for a sync (#139): whether a +/// folder's origin is the listed repository, and whether a host may be given the token. +/// +public sealed class RemoteUrlTests +{ + private const string Listed = "https://github.com/acme/repo.git"; + + // ---------------------------------------------------------------- same repository + + [Theory] + [InlineData("https://github.com/acme/repo.git")] + [InlineData("https://github.com/acme/repo")] + [InlineData("https://github.com/acme/repo/")] + [InlineData("https://github.com/acme/repo.git/")] + [InlineData("https://GitHub.com/Acme/Repo.GIT")] + [InlineData(" https://github.com/acme/repo.git ")] + [InlineData("https://someone:secret@github.com/acme/repo.git")] + [InlineData("http://github.com/acme/repo.git")] + [InlineData("ssh://git@github.com/acme/repo.git")] + [InlineData("ssh://git@github.com:22/acme/repo.git")] + [InlineData("git@github.com:acme/repo.git")] + [InlineData("github.com:acme/repo")] + public void IsSameRepository_SameHostAndPath_HoweverTheOriginIsSpelled(string origin) + { + Assert.True(RemoteUrl.IsSameRepository(Listed, origin)); + } + + + + [Theory] + [InlineData("https://github.com/acme/other.git")] + [InlineData("https://github.com/someone-else/repo.git")] + [InlineData("https://github.com/acme/repo.git/extra")] + [InlineData("https://github.example.com/acme/repo.git")] + [InlineData("https://gitlab.com/acme/repo.git")] + [InlineData("git@bitbucket.org:acme/repo.git")] + [InlineData("ssh://git@old-host.example/acme/repo.git")] + [InlineData(@"C:\src\acme\repo")] + [InlineData("/srv/git/acme/repo.git")] + [InlineData("file:///srv/git/acme/repo.git")] + [InlineData("https://")] + [InlineData("")] + [InlineData(" ")] + public void IsSameRepository_AnotherRepository_OrNoRemoteAtAll_IsNotTheListedOne(string origin) + { + Assert.False(RemoteUrl.IsSameRepository(Listed, origin)); + } + + + + [Fact] + public void IsSameRepository_LocalPaths_CompareAsFullPaths() + { + string folder = Path.Combine(Path.GetTempPath(), "gclo-tests", "remote-url", "source"); + string other = Path.Combine(Path.GetTempPath(), "gclo-tests", "remote-url", "another"); + + Assert.True(RemoteUrl.IsSameRepository(folder, folder)); + Assert.True(RemoteUrl.IsSameRepository(folder, folder + Path.DirectorySeparatorChar)); + Assert.True(RemoteUrl.IsSameRepository(folder, new Uri(folder).AbsoluteUri)); + Assert.True(RemoteUrl.IsSameRepository(folder, Path.Combine(folder, "..", "source"))); + Assert.False(RemoteUrl.IsSameRepository(folder, other)); + // A listed local path is never satisfied by a network origin. + Assert.False(RemoteUrl.IsSameRepository(folder, Listed)); + } + + + + [Theory] + [InlineData("")] + [InlineData(" ")] + public void IsSameRepository_BlankExpectedUrl_IsACallerError(string expected) + { + Assert.Throws(() => RemoteUrl.IsSameRepository(expected, Listed)); + } + + + + // ---------------------------------------------------------------- credentials + + [Theory] + [InlineData("https://github.com/acme/repo.git")] + [InlineData("https://github.com/acme/repo.git/info/refs?service=git-upload-pack")] + [InlineData("https://GITHUB.com/another/path")] + [InlineData("https://github.com:443/acme/repo.git")] + public void MayReceiveCredentials_TheListedHostOverHttps_Yes(string requested) + { + Assert.True(RemoteUrl.MayReceiveCredentials(Listed, requested)); + } + + + + [Theory] + [InlineData("https://evil.example/acme/repo.git")] + [InlineData("https://github.com.evil.example/acme/repo.git")] + [InlineData("https://github.com:8443/acme/repo.git")] + [InlineData("http://github.com/acme/repo.git")] + [InlineData("ssh://git@github.com/acme/repo.git")] + [InlineData("git@github.com:acme/repo.git")] + [InlineData("github.com/acme/repo.git")] + [InlineData("")] + [InlineData(null)] + public void MayReceiveCredentials_AnyOtherHostPortOrScheme_No(string? requested) + { + Assert.False(RemoteUrl.MayReceiveCredentials(Listed, requested)); + } + + + + [Theory] + [InlineData("http://github.com/acme/repo.git")] + [InlineData(@"C:\src\acme\repo")] + [InlineData("ssh://git@github.com/acme/repo.git")] + public void MayReceiveCredentials_ListedUrlIsNotHttps_NobodyGetsTheToken(string listed) + { + Assert.False(RemoteUrl.MayReceiveCredentials(listed, listed)); + Assert.False(RemoteUrl.MayReceiveCredentials(listed, "https://github.com/acme/repo.git")); + } + + + + [Fact] + public void MayReceiveCredentials_BlankExpectedUrl_IsACallerError() + { + Assert.Throws(() => RemoteUrl.MayReceiveCredentials(" ", Listed)); + } + + + + // ---------------------------------------------------------------- authority + + [Theory] + [InlineData("https://github.com/acme/repo.git", "github.com")] + [InlineData("https://GitHub.com:443/acme/repo.git", "github.com")] + [InlineData("https://git.example.com:3000/acme/repo.git", "git.example.com:3000")] + [InlineData("http://github.com/acme/repo.git", null)] + [InlineData("git@github.com:acme/repo.git", null)] + [InlineData("", null)] + [InlineData(null, null)] + public void HttpsAuthority_IsTheHostATokenIsScopedTo_OrNullWithoutHttps(string? url, string? expected) + { + Assert.Equal(expected, RemoteUrl.HttpsAuthority(url)); + } + + + + // ---------------------------------------------------------------- redaction + + [Theory] + [InlineData("https://someone:hunter2@github.com/acme/repo.git", "https://github.com/acme/repo.git")] + [InlineData("https://token@github.com/acme/repo.git", "https://github.com/acme/repo.git")] + [InlineData("ssh://git@github.com", "ssh://github.com")] + [InlineData("https://github.com/acme/repo.git", "https://github.com/acme/repo.git")] + [InlineData("https://github.com/acme/user@example.git", "https://github.com/acme/user@example.git")] + [InlineData("https://github.com", "https://github.com")] + [InlineData(@"C:\src\acme\repo", @"C:\src\acme\repo")] + [InlineData("git@github.com:acme/repo.git", "git@github.com:acme/repo.git")] + [InlineData("", "")] + [InlineData(null, "")] + public void Redact_DropsCredentialsFromTheAuthority_AndNothingElse(string? url, string expected) + { + Assert.Equal(expected, RemoteUrl.Redact(url)); + } +} diff --git a/gclo.Engine.Tests/WindowsPathValidationTests.cs b/gclo.Engine.Tests/WindowsPathValidationTests.cs index 7c8a419..f2584fa 100644 --- a/gclo.Engine.Tests/WindowsPathValidationTests.cs +++ b/gclo.Engine.Tests/WindowsPathValidationTests.cs @@ -324,7 +324,7 @@ public sealed class WindowsPathValidationTests : IDisposable AppendForgedCommit(source, ("bad:incoming.txt", "x")); InvalidRepositoryPathsException ex = await Assert.ThrowsAsync( - () => _client.FetchAndPullAsync(target, Token, CancellationToken.None)); + () => _client.PullAsync(target, Token, CancellationToken.None)); Assert.Contains(ex.Paths, p => string.Equals(p.RepoPath, "bad:incoming.txt", StringComparison.Ordinal)); // The valid original working tree must be untouched. diff --git a/gclo.Engine.Tests/WorkspaceViewModelTests.cs b/gclo.Engine.Tests/WorkspaceViewModelTests.cs index 0f3df54..a713423 100644 --- a/gclo.Engine.Tests/WorkspaceViewModelTests.cs +++ b/gclo.Engine.Tests/WorkspaceViewModelTests.cs @@ -889,6 +889,27 @@ public sealed class WorkspaceViewModelTests : IDisposable + [Fact] + public async Task ResolvePaths_CloneInWsl_UsesTheOrganizationTheTableWasLoadedFor_NotAnEditedBox() + { + MakeAlphaFailWithInvalidPaths(); + _wsl.Availability = new WslAvailability(true, "git version 2.53.0"); + WorkspaceViewModel vm = await CreateLoadedViewModelAsync(Repo("alpha")); + await vm.SyncCommand.ExecuteAsync(null); + RepoItemViewModel row = vm.Repos[0]; + string loaded = vm.Organization; + vm.RecoveryInteraction = _ => Task.FromResult(new PathRecoveryDecision.CloneInWsl()); + + // Typed after the run: the row still belongs to the organization it was listed in. + vm.Organization = "../somewhere-else"; + await vm.ResolvePathsCommand.ExecuteAsync(row); + + WslCloneCall call = Assert.Single(_wsl.CloneCalls); + Assert.Equal(loaded, call.Organization); + } + + + [Fact] public async Task ResolvePaths_CloneInWsl_ShowsAnIndeterminateBar_AndLocksTheWorkspace_WhileInFlight() { diff --git a/gclo.Engine.Tests/WslClonerTests.cs b/gclo.Engine.Tests/WslClonerTests.cs index 73aac60..7caca70 100644 --- a/gclo.Engine.Tests/WslClonerTests.cs +++ b/gclo.Engine.Tests/WslClonerTests.cs @@ -133,7 +133,7 @@ public sealed class WslClonerTests ProcessCall call = Assert.Single(_runner.Calls); Assert.Equal(WslExe, call.FileName); - Assert.Equal(["-e", "sh", "-s", "acme", "repo", "https://github.com/acme/repo.git"], call.Arguments); + Assert.Equal(["-e", "sh", "-s", "acme", "repo", "https://github.com/acme/repo.git", "github.com"], call.Arguments); Assert.Equal(WslCloner.CloneScript, call.StandardInput); Assert.DoesNotContain(Token, string.Join(" ", call.Arguments)); Assert.DoesNotContain(Token, call.StandardInput); @@ -156,7 +156,7 @@ public sealed class WslClonerTests Assert.Contains("password=$" + WslCloner.TokenVariable, script); Assert.Contains("credential.helper=", script); - Assert.Contains("git clone", script.Replace("-c credential.helper= -c \"credential.helper=$helper\" ", "")); + Assert.Contains("git clone", script.Replace("-c credential.helper= -c \"credential.https://$host.helper=$helper\" ", "")); Assert.Contains("pull --ff-only", script); Assert.Contains("$HOME/gclo/$org/$repo", script); Assert.Contains("$WSL_DISTRO_NAME", script); @@ -268,6 +268,81 @@ public sealed class WslClonerTests + [Fact] + public void CloneScript_ScopesTheHelperToTheUrlsHost_AndRefusesAFolderClonedFromElsewhere() + { + string script = WslCloner.CloneScript; + + // Every git invocation first clears inherited helpers, then installs the token + // helper for the listed host only: nothing unscoped may remain (#139). + Assert.Equal(2, CountOf(script, "-c credential.helper= -c \"credential.https://$host.helper=$helper\"")); + Assert.Equal(0, CountOf(script, "\"credential.helper=$helper\"")); + Assert.Contains("host=\"$4\"", script); + // "--" ends option parsing, so a URL can never be read as an option. + Assert.Contains("clone --quiet -- \"$url\" \"$dest\"", script); + // An existing destination is only fast-forwarded when it is a clone of this URL. + Assert.Contains("remote get-url origin)\" != \"$url\"", script); + Assert.True( + script.IndexOf("remote get-url origin", StringComparison.Ordinal) < script.IndexOf("pull --ff-only", StringComparison.Ordinal), + "the origin check must come before the pull"); + } + + + + [Fact] + public async Task Clone_PassesTheHostWithItsPort_ForAServerOnANonDefaultPort() + { + _runner.Handler = (_, _) => Task.FromResult(new ProcessResult(0, "Ubuntu\n/home/me/gclo/acme/repo\n", "")); + + await Create().CloneAsync("https://git.example.com:3000/acme/repo.git", "acme", "repo", Token, CancellationToken.None); + + ProcessCall call = Assert.Single(_runner.Calls); + Assert.Equal(["-e", "sh", "-s", "acme", "repo", "https://git.example.com:3000/acme/repo.git", "git.example.com:3000"], call.Arguments); + } + + + + [Theory] + [InlineData("../..", "repo")] + [InlineData("..", "repo")] + [InlineData("acme/other", "repo")] + [InlineData("acme", "../../.ssh")] + [InlineData("acme", "re/po")] + [InlineData("acme", @"re\po")] + public async Task Clone_NameThatWouldLeaveTheGcloFolder_IsRefused_WithoutStartingWsl(string organization, string repository) + { + WslCloneException ex = await Assert.ThrowsAsync( + () => Create().CloneAsync("https://github.com/acme/repo.git", organization, repository, Token, CancellationToken.None)); + + Assert.StartsWith("Clone in WSL refused: '", ex.Message); + Assert.Contains("cannot be used as a folder name", ex.Message); + Assert.Empty(_runner.Calls); + } + + + + [Theory] + [InlineData("http://github.com/acme/repo.git")] + [InlineData("ssh://git@github.com/acme/repo.git")] + [InlineData("git@github.com:acme/repo.git")] + [InlineData("--upload-pack=touch /tmp/x")] + [InlineData(@"C:\src\acme\repo")] + public async Task Clone_UrlThatIsNotHttps_IsRefused_WithoutStartingWsl(string url) + { + WslCloneException ex = await Assert.ThrowsAsync( + () => Create().CloneAsync(url, "acme", "repo", Token, CancellationToken.None)); + + Assert.Equal("Clone in WSL refused: the clone URL is not an HTTPS URL.", ex.Message); + Assert.Empty(_runner.Calls); + } + + + + private static int CountOf(string text, string fragment) + => (text.Length - text.Replace(fragment, "", StringComparison.Ordinal).Length) / fragment.Length; + + + [Fact] public void Defaults_PointAtSystem32Wsl() { diff --git a/gclo.Engine/GitHubRepositoryLister.cs b/gclo.Engine/GitHubRepositoryLister.cs index 44961c8..ad0a333 100644 --- a/gclo.Engine/GitHubRepositoryLister.cs +++ b/gclo.Engine/GitHubRepositoryLister.cs @@ -38,6 +38,18 @@ public sealed class GitHubRepositoryLister : IRepositoryLister ArgumentException.ThrowIfNullOrWhiteSpace(token); cancellationToken.ThrowIfCancellationRequested(); + /////////////////////////////////////////////////////////////////////////// + // The name goes into request paths and, with the org-subfolder option and // + // the WSL recovery, into local paths. Anything that is not one plain name // + // is refused before a request is made (#139). // + /////////////////////////////////////////////////////////////////////////// + if(!RepositoryPathResolver.TryValidateName(organization, out string? invalidName)) + { + throw new GitHubAccessException( + GitHubAccessKind.NotFound, + $"'{organization}' is not an organization or account name ({invalidName}). Check the spelling."); + } + IGitHubGateway gateway = _gatewayFactory(token); // Pages manually (instead of one GetAll* call) so cancellation is honored diff --git a/gclo.Engine/IGitClient.cs b/gclo.Engine/IGitClient.cs index 30b7397..cd00349 100644 --- a/gclo.Engine/IGitClient.cs +++ b/gclo.Engine/IGitClient.cs @@ -24,8 +24,19 @@ public interface IGitClient - /// Fetches from origin and fast-forwards the current branch to its upstream. - Task FetchAndPullAsync(string path, string token, CancellationToken cancellationToken); + /// + /// Fetches into the repository at and + /// fast-forwards the current branch to its upstream. The folder's origin must + /// name the same repository as (scheme, credentials and a + /// trailing .git aside); a folder that is a clone of something else fails + /// without being fetched, so a sync never reports a different repository as updated. + /// The token is offered only over HTTPS to the host of . + /// + /// The clone URL the repository was listed with. + /// Local directory of the existing clone. + /// Access token used for git transport. + /// Cancels the pull. + Task FetchAndPullAsync(string url, string path, string token, CancellationToken cancellationToken); diff --git a/gclo.Engine/LibGit2GitClient.cs b/gclo.Engine/LibGit2GitClient.cs index 2c0401f..ef8d492 100644 --- a/gclo.Engine/LibGit2GitClient.cs +++ b/gclo.Engine/LibGit2GitClient.cs @@ -52,6 +52,20 @@ public sealed class LibGit2GitClient : IGitClient private static readonly JsonSerializerOptions s_recoveryJsonOptions = new() { WriteIndented = true }; + /// + /// Name of the remote that exists only for the duration of one fetch, when a + /// folder's origin names the listed repository but spells its URL differently + /// (see ). + /// + private const string SyncRemoteName = "gclo-sync"; + + /// + /// The sync remote's own refspec. It maps to a namespace nothing is ever fetched + /// into, so removing the remote (which deletes the tracking refs its refspecs + /// cover) cannot touch origin's. + /// + private const string SyncRemoteRefSpec = "+refs/heads/*:refs/remotes/" + SyncRemoteName + "/*"; + /// /// Test seam: invoked right after a fetch completed, before the cancellation /// check that precedes any working-tree work. Local-transport fixtures fire no @@ -79,9 +93,9 @@ public sealed class LibGit2GitClient : IGitClient /// - public Task FetchAndPullAsync(string path, string token, CancellationToken cancellationToken) + public Task FetchAndPullAsync(string url, string path, string token, CancellationToken cancellationToken) => Task.Factory.StartNew( - () => FetchAndPull(path, token, cancellationToken), + () => FetchAndPull(url, path, token, cancellationToken), cancellationToken, TaskCreationOptions.LongRunning, TaskScheduler.Default); @@ -106,7 +120,7 @@ public sealed class LibGit2GitClient : IGitClient // not a libgit2 one. On other platforms phase two is a plain checkout. Checkout = false, }; - options.FetchOptions.CredentialsProvider = MakeCredentialsProvider(token); + options.FetchOptions.CredentialsProvider = MakeCredentialsProvider(token, url); int lastPercent = -1; options.FetchOptions.OnTransferProgress = tp => { @@ -202,7 +216,7 @@ public sealed class LibGit2GitClient : IGitClient - private void FetchAndPull(string path, string token, CancellationToken ct) + private void FetchAndPull(string url, string path, string token, CancellationToken ct) { using var repo = new Repository(path); @@ -212,12 +226,11 @@ public sealed class LibGit2GitClient : IGitClient repo.Config.Set("core.longpaths", true, ConfigurationLevel.Local); } - Remote remote = repo.Network.Remotes["origin"] - ?? throw new InvalidOperationException("Repository has no 'origin' remote."); + Remote remote = OriginOfListedRepository(repo, url); var fetchOptions = new FetchOptions { - CredentialsProvider = MakeCredentialsProvider(token), + CredentialsProvider = MakeCredentialsProvider(token, url), OnTransferProgress = _ => !ct.IsCancellationRequested, // Drop tracking refs for branches origin deleted. Without this a renamed // default branch leaves refs/remotes/origin/ frozen at its last SHA, @@ -225,10 +238,9 @@ public sealed class LibGit2GitClient : IGitClient Prune = true, }; - var refSpecs = remote.FetchRefSpecs.Select(s => s.Specification).ToList(); try { - Commands.Fetch(repo, remote.Name, refSpecs, fetchOptions, logMessage: null); + FetchListedUrl(repo, remote, url, fetchOptions); } catch(UserCancelledException) when(ct.IsCancellationRequested) { @@ -296,7 +308,9 @@ public sealed class LibGit2GitClient : IGitClient // Upstream is configured but gone from origin (the fetch pruned it): the // default branch was renamed (master -> main) or deleted. Follow origin's // current default instead of reporting "up to date" forever (#31). - tracked = RetargetToRemoteDefaultBranch(repo, remote, fetchOptions.CredentialsProvider, ct); + // A provider of its own: the fetch already spent one of the two requests + // the replay guard allows. + tracked = RetargetToRemoteDefaultBranch(repo, remote, url, MakeCredentialsProvider(token, url), ct); } if(string.Equals(repo.Head.Tip?.Sha, tracked.Tip.Sha, StringComparison.Ordinal)) @@ -346,6 +360,78 @@ public sealed class LibGit2GitClient : IGitClient + /// + /// Fetches the listed into 's own + /// refspecs, so the tracking refs land exactly where a fetch of origin puts them + /// while the transport is always the URL the listing returned. + /// + /// + /// When origin's URL is the listed URL, which is every clone gclo made itself, this + /// is a plain fetch of origin. Otherwise the origin names the same repository in + /// another spelling: switched to SSH for pushing, plain HTTP, credentials typed + /// into it. Fetching that origin would let the folder's configuration choose the + /// transport (and, for SSH, the program that provides it). LibGit2Sharp cannot + /// fetch a bare URL, so the listed URL is fetched through a remote that exists + /// only for this call; origin itself is never rewritten (#139). + /// + private static void FetchListedUrl(Repository repo, Remote origin, string url, FetchOptions options) + { + var refSpecs = origin.FetchRefSpecs.Select(s => s.Specification).ToList(); + if(string.Equals(origin.Url, url, StringComparison.Ordinal)) + { + Commands.Fetch(repo, origin.Name, refSpecs, options, logMessage: null); + return; + } + + RemoveSyncRemote(repo); // left behind by a run that was killed mid-fetch + repo.Network.Remotes.Add(SyncRemoteName, url, SyncRemoteRefSpec); + try + { + Commands.Fetch(repo, SyncRemoteName, refSpecs, options, logMessage: null); + } + finally + { + RemoveSyncRemote(repo); + } + } + + + + private static void RemoveSyncRemote(Repository repo) + { + if(repo.Network.Remotes[SyncRemoteName] is not null) + { + repo.Network.Remotes.Remove(SyncRemoteName); + } + } + + + + /// + /// The folder's origin, once it is known to name the repository listed at + /// . A folder with the right name is not proof of the right + /// repository: it may be a fork, a same-named repository of another owner or host, + /// or something another tool put there. Pulling it would report the listed + /// repository as updated when it was not, so it fails here, before any network + /// request (#139). + /// + private static Remote OriginOfListedRepository(Repository repo, string url) + { + Remote remote = repo.Network.Remotes["origin"] + ?? throw new InvalidOperationException("Repository has no 'origin' remote."); + + if(!RemoteUrl.IsSameRepository(url, remote.Url)) + { + throw new InvalidOperationException( + $"This folder is a clone of a different repository: its origin is '{RemoteUrl.Redact(remote.Url)}', not '{url}'. " + + "Move or delete the folder, or point its origin at the listed repository, then sync again."); + } + + return remote; + } + + + /// /// The current branch's upstream no longer exists on origin. Asks origin for its /// current default branch (the remote HEAD symref), checks it out as the new @@ -355,12 +441,12 @@ public sealed class LibGit2GitClient : IGitClient /// tracking branch so the normal pull continues from it. /// private static Branch RetargetToRemoteDefaultBranch( - Repository repo, Remote remote, CredentialsHandler credentials, CancellationToken ct) + Repository repo, Remote remote, string url, CredentialsHandler credentials, CancellationToken ct) { const string Prefix = "refs/heads/"; string oldName = repo.Head.FriendlyName; - string? defaultRef = repo.Network.ListReferences(remote, credentials) + string? defaultRef = repo.Network.ListReferences(url, credentials) .FirstOrDefault(r => string.Equals(r.CanonicalName, "HEAD", StringComparison.Ordinal))?.TargetIdentifier; if(defaultRef is null || !defaultRef.StartsWith(Prefix, StringComparison.Ordinal)) { @@ -803,12 +889,23 @@ public sealed class LibGit2GitClient : IGitClient /// up with "too many redirects or authentication replays", once per repository. /// A correct token is never asked for a third time, so the third request means the /// token is bad and gets the same actionable message the API listers produce (#31). + /// The token is for 's host only: a request for any + /// other host (a redirect, a folder configured for another server) or over anything + /// but HTTPS is refused, so the token never leaves for somewhere the repository was + /// not listed on (#139). /// - private static CredentialsHandler MakeCredentialsProvider(string token) + private static CredentialsHandler MakeCredentialsProvider(string token, string expectedUrl) { int requests = 0; - return (_, _, _) => + return (requestedUrl, _, _) => { + if(!RemoteUrl.MayReceiveCredentials(expectedUrl, requestedUrl)) + { + throw new InvalidOperationException( + $"'{RemoteUrl.Redact(requestedUrl)}' asked for credentials, but the token is only sent over HTTPS " + + "to the host the repository was listed on. Nothing was sent."); + } + if(Interlocked.Increment(ref requests) > 2) { throw new InvalidOperationException( diff --git a/gclo.Engine/OrgSyncEngine.cs b/gclo.Engine/OrgSyncEngine.cs index 63c07c6..eaf5d43 100644 --- a/gclo.Engine/OrgSyncEngine.cs +++ b/gclo.Engine/OrgSyncEngine.cs @@ -141,7 +141,7 @@ public sealed class OrgSyncEngine if(_git.IsValidRepository(path)) { progress?.Report(new RepoProgress(repo.Name, SyncStatus.Pulling)); - await _git.FetchAndPullAsync(path, request.Token, token).WaitAsync(token).ConfigureAwait(false); + await _git.FetchAndPullAsync(repo.CloneUrl, path, request.Token, token).WaitAsync(token).ConfigureAwait(false); Interlocked.Increment(ref updated); } else diff --git a/gclo.Engine/RemoteUrl.cs b/gclo.Engine/RemoteUrl.cs new file mode 100644 index 0000000..997d166 --- /dev/null +++ b/gclo.Engine/RemoteUrl.cs @@ -0,0 +1,158 @@ +/* + * Copyright (c) 2026 James Maes (KofTwentyTwo) + * SPDX-License-Identifier: MIT + */ + +using System.Diagnostics.CodeAnalysis; +using System.Globalization; + + +namespace gclo.Engine; + + +/// +/// Decides two things about git remote URLs, both from the clone URL a listing returned +/// (the "expected" URL): whether an existing folder's origin names that same +/// repository, and whether a host that asks for credentials is the one the token is +/// meant for. A sync must never report a different repository as updated, and must +/// never hand the token to a host the listing did not come from (#139). +/// +internal static class RemoteUrl +{ + private static readonly char[] s_separators = ['/', '\\']; + + /// + /// True when (a folder's configured origin) names the same + /// repository as (the listed clone URL). Network URLs + /// are compared by host and path, ignoring scheme, credentials, port, letter case, + /// a trailing slash and a trailing .git, so an origin switched to SSH still + /// matches its HTTPS listing. Local paths (the offline test fixtures, or a caller's + /// own mirror) are compared as full paths. A network URL never matches a local path. + /// + public static bool IsSameRepository(string expected, string actual) + { + ArgumentException.ThrowIfNullOrWhiteSpace(expected); + if(string.IsNullOrWhiteSpace(actual)) + { + return false; + } + + (string Host, string Path)? expectedRemote = ParseNetwork(expected); + (string Host, string Path)? actualRemote = ParseNetwork(actual); + if(expectedRemote is null || actualRemote is null) + { + return expectedRemote is null + && actualRemote is null + && string.Equals(NormalizeLocal(expected), NormalizeLocal(actual), StringComparison.OrdinalIgnoreCase); + } + + return string.Equals(expectedRemote.Value.Host, actualRemote.Value.Host, StringComparison.OrdinalIgnoreCase) + && string.Equals(expectedRemote.Value.Path, actualRemote.Value.Path, StringComparison.OrdinalIgnoreCase); + } + + + + /// + /// True when a credential request for may be answered + /// with the token of a sync whose listed clone URL is : + /// both must be HTTPS and name the same host and port. Anything else (another host + /// after a redirect, plain HTTP, SSH) gets no token. + /// + public static bool MayReceiveCredentials(string expected, string? requested) + { + ArgumentException.ThrowIfNullOrWhiteSpace(expected); + return TryParseHttps(expected, out Uri? expectedUri) + && TryParseHttps(requested, out Uri? requestedUri) + && string.Equals(expectedUri.IdnHost, requestedUri.IdnHost, StringComparison.OrdinalIgnoreCase) + && expectedUri.Port == requestedUri.Port; + } + + + + /// + /// The host of an HTTPS , with its port when that is not 443; + /// null when the URL is not absolute HTTPS. This is the scope a token may be used in. + /// + public static string? HttpsAuthority(string? url) + => TryParseHttps(url, out Uri? uri) ? uri.IdnHost + (uri.IsDefaultPort ? "" : ":" + uri.Port.ToString(CultureInfo.InvariantCulture)) : null; + + + + /// + /// without any user:password@ part, for error messages + /// and logs: an origin may carry credentials someone typed into it. + /// + public static string Redact(string? url) + { + if(string.IsNullOrEmpty(url)) + { + return ""; + } + + int scheme = url.IndexOf("://", StringComparison.Ordinal); + if(scheme < 0) + { + return url; + } + + int authorityStart = scheme + 3; + int authorityEnd = url.IndexOf('/', authorityStart); + int at = url.IndexOf('@', authorityStart); + return at >= 0 && (authorityEnd < 0 || at < authorityEnd) + ? string.Concat(url.AsSpan(0, authorityStart), url.AsSpan(at + 1)) + : url; + } + + + + private static bool TryParseHttps(string? url, [NotNullWhen(true)] out Uri? uri) + => Uri.TryCreate(url, UriKind.Absolute, out uri) + && string.Equals(uri.Scheme, Uri.UriSchemeHttps, StringComparison.OrdinalIgnoreCase); + + + + /// Host and normalized path of a network git URL, or null for a local path. + private static (string Host, string Path)? ParseNetwork(string url) + { + string trimmed = url.Trim(); + int scheme = trimmed.IndexOf("://", StringComparison.Ordinal); + if(scheme > 0) + { + return Uri.TryCreate(trimmed, UriKind.Absolute, out Uri? uri) && !uri.IsFile && uri.IdnHost.Length > 0 + ? (uri.IdnHost, NormalizePath(Uri.UnescapeDataString(uri.AbsolutePath))) + : null; + } + + // The scp-like form git accepts for SSH: [user@]host:path. A single letter before + // the colon is a Windows drive ("C:\src\repo"), not a host, and a separator + // before the colon means the colon belongs to a local path. + int colon = trimmed.IndexOf(':', StringComparison.Ordinal); + if(colon < 0) + { + return null; + } + + string host = trimmed[(trimmed.LastIndexOf('@', colon) + 1)..colon]; + string path = trimmed[(colon + 1)..]; + return host.Length >= 2 && host.IndexOfAny(s_separators) < 0 && path.Length > 0 && path[0] != '\\' + ? (host, NormalizePath(path)) + : null; + } + + + + private static string NormalizePath(string path) + { + string trimmed = path.Trim('/'); + return trimmed.EndsWith(".git", StringComparison.OrdinalIgnoreCase) ? trimmed[..^4].TrimEnd('/') : trimmed; + } + + + + private static string NormalizeLocal(string path) + { + string local = Uri.TryCreate(path.Trim(), UriKind.Absolute, out Uri? uri) && uri.IsFile ? uri.LocalPath : path.Trim(); + return Path.TrimEndingDirectorySeparator(Path.GetFullPath(local)); + } + +} diff --git a/gclo.Engine/WslCloner.cs b/gclo.Engine/WslCloner.cs index 5443989..5553bb5 100644 --- a/gclo.Engine/WslCloner.cs +++ b/gclo.Engine/WslCloner.cs @@ -20,21 +20,29 @@ public sealed class WslCloner : IWslCloner public const string TokenVariable = "GCLO_GIT_TOKEN"; /// - /// The clone script. $1 organization, $2 repository, $3 clone URL. Prints the - /// distribution name and the destination as its last two lines on success. A folder - /// that already is a repository is fast-forwarded instead of failing, so the verb is - /// safe to use twice. + /// The clone script. $1 organization, $2 repository, $3 clone URL, $4 the URL's + /// host (with a non-default port). Prints the distribution name and the destination + /// as its last two lines on success. A folder that already is a clone of the same + /// URL is fast-forwarded instead of failing, so the verb is safe to use twice; a + /// folder that is a clone of anything else is refused. The credential helper is + /// scoped to $4, so git (and anything it starts, such as an LFS filter pointed at + /// another server by the repository's own configuration) gets the token for that + /// host only (#139). /// internal const string CloneScript = "set -e\n" - + "org=\"$1\"; repo=\"$2\"; url=\"$3\"\n" + + "org=\"$1\"; repo=\"$2\"; url=\"$3\"; host=\"$4\"\n" + "dest=\"$HOME/gclo/$org/$repo\"\n" + "helper='!f() { echo username=x-access-token; echo \"password=$" + TokenVariable + "\"; }; f'\n" + "if [ -d \"$dest/.git\" ]; then\n" - + " git -C \"$dest\" -c credential.helper= -c \"credential.helper=$helper\" pull --ff-only --quiet\n" + + " if [ \"$(git -C \"$dest\" remote get-url origin)\" != \"$url\" ]; then\n" + + " echo \"$dest is a clone of a different repository; move or delete it, then try again\" >&2\n" + + " exit 1\n" + + " fi\n" + + " git -C \"$dest\" -c credential.helper= -c \"credential.https://$host.helper=$helper\" pull --ff-only --quiet\n" + "else\n" + " mkdir -p \"$HOME/gclo/$org\"\n" - + " git -c credential.helper= -c \"credential.helper=$helper\" clone --quiet \"$url\" \"$dest\"\n" + + " git -c credential.helper= -c \"credential.https://$host.helper=$helper\" clone --quiet -- \"$url\" \"$dest\"\n" + "fi\n" + "printf '%s\\n%s\\n' \"$WSL_DISTRO_NAME\" \"$dest\"\n"; @@ -102,6 +110,23 @@ public sealed class WslCloner : IWslCloner ArgumentException.ThrowIfNullOrWhiteSpace(repositoryName); ArgumentNullException.ThrowIfNull(token); + ///////////////////////////////////////////////////////////////////////////// + // The organization and repository become path segments under ~/gclo, and // + // the URL's host is what the token is scoped to. None of the three is taken // + // on trust: a name with a separator or ".." would leave ~/gclo, and a URL // + // that is not HTTPS has no host the token could safely be tied to (#139). // + ///////////////////////////////////////////////////////////////////////////// + foreach(string name in new[] { organization, repositoryName }) + { + if(!RepositoryPathResolver.TryValidateName(name, out string? reason)) + { + throw new WslCloneException($"Clone in WSL refused: '{name}' cannot be used as a folder name ({reason})."); + } + } + + string host = RemoteUrl.HttpsAuthority(url) + ?? throw new WslCloneException("Clone in WSL refused: the clone URL is not an HTTPS URL."); + var environment = new Dictionary(StringComparer.Ordinal) { [TokenVariable] = token, @@ -115,7 +140,7 @@ public sealed class WslCloner : IWslCloner { result = await _runner.RunAsync( _wslExe, - ["-e", "sh", "-s", organization, repositoryName, url], + ["-e", "sh", "-s", organization, repositoryName, url, host], environment, CloneScript, cancellationToken).ConfigureAwait(false); diff --git a/gclo.ViewModels/WorkspaceViewModel.cs b/gclo.ViewModels/WorkspaceViewModel.cs index a2371dd..278b793 100644 --- a/gclo.ViewModels/WorkspaceViewModel.cs +++ b/gclo.ViewModels/WorkspaceViewModel.cs @@ -1102,8 +1102,10 @@ public sealed partial class WorkspaceViewModel : ObservableObject, IDisposable { _log.Info($"{item.Name}: cloning in WSL instead."); item.Status = SyncStatus.Pulling; // indeterminate bar; git in WSL reports no percentage here + // The organization the table was loaded for, not whatever the box says now: + // the row belongs to that organization, and its folder under ~/gclo should too. WslCloneResult result = await _wsl.CloneAsync( - item.Descriptor.CloneUrl, Organization.Trim(), item.Name, Token.Trim(), _lifetimeCts.Token); + item.Descriptor.CloneUrl, LoadedOrganization ?? Organization.Trim(), item.Name, Token.Trim(), _lifetimeCts.Token); item.Status = SyncStatus.Done; item.Error = null; diff --git a/gclo/UiTestFixture.cs b/gclo/UiTestFixture.cs index e8ce20f..7cf2388 100644 --- a/gclo/UiTestFixture.cs +++ b/gclo/UiTestFixture.cs @@ -90,8 +90,8 @@ internal static class UiTestFixture - public Task FetchAndPullAsync(string path, string token, CancellationToken cancellationToken) - => string.Equals(token, Token, StringComparison.Ordinal) ? Task.CompletedTask : real.FetchAndPullAsync(path, token, cancellationToken); + public Task FetchAndPullAsync(string url, string path, string token, CancellationToken cancellationToken) + => string.Equals(token, Token, StringComparison.Ordinal) ? Task.CompletedTask : real.FetchAndPullAsync(url, path, token, cancellationToken); -- 2.54.0 From 510d3e056184f61571a872b22215efb50d0b9021 Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 10 Nov 2026 12:54:51 -0600 Subject: [PATCH 2/2] fix(sync): refuse URL rewrites from the folder, keep the user's remotes safe Follow-up to the independent review of this branch. - A url..insteadOf entry in the folder's own configuration redirected the fetch of the listed URL to another repository. It is now refused before anything is fetched, and so is the folder's core.sshCommand when the fetch is not HTTPS. The user's global configuration still applies. - The temporary remote is marked as gclo's; a remote of that name the user created is never removed, and cleanup can no longer mask the fetch's own error. A marked leftover is removed on any later pull. The temporary fetch follows origin's tag option. - The mismatch message gives the git remote set-url command for a repository that was renamed or moved. - RemoteUrl: redaction rebuilds the URL from scheme, host, port and path, so no shape leaves user information, query or fragment behind; a non-default web port distinguishes two servers; an unreadable host is nobody's instead of an exception; IPv6 hosts keep their brackets. - The WSL script compares the stored origin URL, not the rewritten one. - The credential callback is tested directly. The threat model and README no longer claim more than the code does about redirects. Refs #139 Co-Authored-By: Claude Fable 5.2 Signed-off-by: James Maes --- README.md | 2 +- docs/security/threat-model.md | 8 +- gclo.Engine.Tests/GitCredentialsTests.cs | 114 ++++++++++++ gclo.Engine.Tests/OriginVerificationTests.cs | 120 +++++++++++- gclo.Engine.Tests/RemoteUrlTests.cs | 45 ++++- gclo.Engine.Tests/WslClonerTests.cs | 7 +- gclo.Engine/LibGit2GitClient.cs | 121 ++++++++++-- gclo.Engine/RemoteUrl.cs | 186 +++++++++++++------ gclo.Engine/WslCloner.cs | 4 +- 9 files changed, 522 insertions(+), 85 deletions(-) create mode 100644 gclo.Engine.Tests/GitCredentialsTests.cs diff --git a/README.md b/README.md index a2b00fc..5e5e848 100644 --- a/README.md +++ b/README.md @@ -186,7 +186,7 @@ Full reference — all options, output streams, Ctrl+C behavior, and more exampl ## How it works -The engine lists every repository through the GitHub REST API (Octokit, paginated 100 per page), then runs the set through `Parallel.ForEachAsync` bounded by the parallelism setting. Git operations use LibGit2Sharp: missing repos are cloned with object-transfer progress; existing repos get a fetch from `origin` followed by a **fast-forward-only** merge — if a local branch has diverged from its upstream, the repo is reported Failed rather than auto-merged, so gclo never manufactures merge commits. An existing folder is only updated when its `origin` is the repository gclo listed: a same-named folder that is a clone of something else (a fork, another owner's repository, another host) is reported Failed and left untouched. Archived repositories are included and sync normally. Every repo ends in exactly one terminal state — Done, Failed (with the error), or Canceled — and per-repo exceptions are contained so the rest of the run continues. The PAT is the only credential; on the git side it is sent as HTTPS basic credentials with username `x-access-token`, and only to the host the repository was listed on. +The engine lists every repository through the GitHub REST API (Octokit, paginated 100 per page), then runs the set through `Parallel.ForEachAsync` bounded by the parallelism setting. Git operations use LibGit2Sharp: missing repos are cloned with object-transfer progress; existing repos get a fetch from `origin` followed by a **fast-forward-only** merge — if a local branch has diverged from its upstream, the repo is reported Failed rather than auto-merged, so gclo never manufactures merge commits. An existing folder is only updated when its `origin` is the repository gclo listed: a same-named folder that is a clone of something else (a fork, another owner's repository, another host) is reported Failed and left untouched. A repository that was renamed or moved to another owner looks the same to gclo; the failure message gives the `git remote set-url` command that points the folder at its new URL. Archived repositories are included and sync normally. Every repo ends in exactly one terminal state — Done, Failed (with the error), or Canceled — and per-repo exceptions are contained so the rest of the run continues. The PAT is the only credential; on the git side it is sent as HTTPS basic credentials with username `x-access-token`, and only to the host the repository was listed on. ## Build from source diff --git a/docs/security/threat-model.md b/docs/security/threat-model.md index 07b0f43..c626c19 100644 --- a/docs/security/threat-model.md +++ b/docs/security/threat-model.md @@ -86,16 +86,16 @@ flowchart LR | T1 | Update feed | Tampering | Attacker serves a malicious update | Low | Critical | HTTPS to this repository's Releases only; Velopack hash verification; immutable releases; provenance and SBOM attestations; Scorecard; Authenticode pending (EX-0002) | Mitigated (signing pending) | | T2 | Package managers / downloads | Tampering | Modified installer or CLI zip on a mirror | Low | High | `SHA256SUMS`; Scoop and Chocolatey manifests pin the hash; "Verifying a release" instructions | Mitigated | | T3 | Git remotes | Tampering / EoP | Repository with Windows-invalid or traversal paths (`..`, reserved names, case collisions) writes outside or corrupts the target | Medium | High | Pre-checkout validation of every incoming path; `RepositoryPathResolver` containment; recovery by rename/skip or clone in WSL; `core.longpaths` | Mitigated | -| T4 | Git remotes | Tampering | Hostile `.git` config or hooks in a pre-existing target repository | Low | High | gclo runs no hooks (libgit2, no shell); only fetch and fast-forward; `gclo.checkoutpending` marks incomplete trees; a folder is fetched only when its `origin` names the listed repository, and always over the listed URL, so the folder's configuration chooses neither the remote nor the transport (`RemoteUrl`, #139) | Mitigated for the remote and transport; other `.git` content of an existing folder is still trusted | +| T4 | Git remotes | Tampering | Hostile `.git` config or hooks in a pre-existing target repository | Low | High | gclo runs no hooks (libgit2, no shell); only fetch and fast-forward; `gclo.checkoutpending` marks incomplete trees; a folder is fetched only when its `origin` names the listed repository, and the remote contacted is the listed URL, not origin's spelling of it; a `url..insteadOf` rewrite in the folder's own configuration is refused, and so is its `core.sshCommand` when the fetch is not HTTPS (`RemoteUrl`, #139). The user's global git configuration still applies, as it does to git itself | Mitigated for the remote and the transport program; other `.git` content of an existing folder is still trusted | | T5 | PAT handling | Information disclosure | Token leaks to disk, logs, process listings, or UI | Medium | Critical | Memory-only in Quick Sync; Credential Manager otherwise; accounts may reference the default token (`TokenSource`), resolved at use time through one resolver and never copied; logs record lengths; no `--token`; WSL via env only; secret scanning + gitleaks + CodeQL in CI | Mitigated | | T6 | Credential Manager | Information disclosure | Another process as the same user reads the stored token | Medium | High | Documented boundary (SECURITY.md); vault scoped per data directory so test or portable instances never see the default profile's tokens | Accepted (OS boundary) | | T7 | GitHub API | Denial of service | Rate limits or abuse detection stop a sync | Medium | Low | Secondary-rate-limit and abuse handling with backoff; per-repo failure isolation; cancellation | Mitigated | -| T8 | GitHub API / remotes | Spoofing | A MITM impersonates GitHub | Low | Critical | HTTPS with system trust store (Octokit, libgit2); no HTTP fallback: an existing folder whose origin is `http://` or SSH is fetched over the listed HTTPS URL instead | Mitigated | +| T8 | GitHub API / remotes | Spoofing | A MITM impersonates GitHub | Low | Critical | HTTPS with system trust store (Octokit, libgit2); no HTTP fallback: an existing folder whose origin is `http://` or SSH is fetched through the listed HTTPS URL instead (unless the user's own global configuration rewrites that URL) | Mitigated | | T9 | WSL recovery | Elevation of privilege | Hostile org/repo names injected into the shell script, or used to leave `~/gclo` | Low | High | Script fed on stdin with positional parameters; organization and repository each validated as one plain name inside `WslCloner` (and the organization again before any listing request); the URL must be HTTPS and is passed after `--`; no string interpolation into the script | Mitigated | | T10 | Logs and crash output | Information disclosure | Exception text carries a token or private path | Low | Medium | Crash net logs messages only; log review in incident handling; tokens never in exception messages (tests) | Mitigated | | T11 | Release pipeline | Tampering | Compromised action or cache poisons a release | Low | Critical | Build runs in the standards' shared release workflow (SLSA Build L3 provenance, `--signer-repo KofTwentyTwo/standards`), SHA-pinned actions, no caches, read-only tokens, publishing tokens only in the tag-restricted `release` environment, `protect-release-tags`, required gates re-run on the tag | Mitigated | | T12 | Repository | Repudiation | Unattributed change reaches `main` | Low | High | Signed, signed-off commits; squash-only through gated pull requests; no bypass actors | Mitigated | -| T14 | PAT handling | Information disclosure | The token is offered to a host other than the one the repository was listed on: an existing folder whose `origin` points elsewhere, a redirect, or a server named by the repository's own configuration during the WSL clone (for example an LFS endpoint) | Low | Critical | The git credential callback answers only HTTPS requests for the listed URL's host and port, and refuses everything else with a clear error; a folder whose origin names another repository fails before any request; the WSL credential helper is scoped to the listed host (`credential.https://.helper`) and an existing WSL destination is pulled only when its origin is the listed URL (#139) | Mitigated | +| T14 | PAT handling | Information disclosure | The token is offered to a host other than the one the repository was listed on: an existing folder whose `origin` points elsewhere, or a server named by the repository's own configuration during the WSL clone (for example an LFS endpoint) | Low | Critical | The git credential callback answers only a remote whose URL is HTTPS on the listed URL's host and port, and refuses every other remote with a clear error (a redirect that the listed host itself issues is followed by libgit2; that host already holds the token); a folder whose origin names another repository fails before any request; the WSL credential helper is scoped to the listed host (`credential.https://.helper`) and an existing WSL destination is pulled only when its stored origin is the listed URL (#139) | Mitigated | | T13 | CLI / data directory | Tampering | A malicious `accounts.json` or `settings.json` redirects targets | Low | Medium | Files live under the user profile; sanitized on load; target paths resolved and contained | Accepted (same-user boundary) | ## 7. Accepted risks @@ -116,4 +116,4 @@ flowchart LR | 2026-10-10 | v1.0.2 | James Maes (@KofTwentyTwo) | Local data directory moved to `%LOCALAPPDATA%\KofTwentyTwo\gclo`: the Velopack install root is deleted by Setup.exe, which destroyed accounts.json and settings.json (#91) | | 2026-10-10 | v1.0.2 | James Maes (@KofTwentyTwo) | T2/T11: releases are Authenticode-signed with Azure Artifact Signing over GitHub OIDC (no signing secret; federated credential bound to this repository's `release` environment; signer role scoped to the certificate profile) and the published assets are verified with `signtool verify /pa /all` (#57, ADR 0008) | | 2026-10-10 | v1.1.0 | James Maes (@KofTwentyTwo) | T5: accounts can reference the default token (`TokenSource = Default`) instead of holding a copy; one `AccountTokenResolver` serves the app and the CLI, so rotating the default token in Settings rotates every such account (#102, docs/plans/default-token-ux.md) | -| 2026-11-10 | v1.1.1 | James Maes (@KofTwentyTwo) | T14 added, T4/T8/T9 updated: an existing folder is fetched only when its origin names the listed repository and always over the listed URL; the git credential callback and the WSL credential helper answer only for the listed host over HTTPS; WSL organization and repository names validated as single names (#139) | +| 2026-11-10 | v1.1.1 | James Maes (@KofTwentyTwo) | T14 added, T4/T8/T9 updated: an existing folder is fetched only when its origin names the listed repository, through the listed URL, and the folder's own `url.*.insteadOf` and (for a non-HTTPS fetch) `core.sshCommand` are refused; the git credential callback and the WSL credential helper answer only for the listed host over HTTPS; WSL organization and repository names validated as single names (#139) | diff --git a/gclo.Engine.Tests/GitCredentialsTests.cs b/gclo.Engine.Tests/GitCredentialsTests.cs new file mode 100644 index 0000000..bc27e07 --- /dev/null +++ b/gclo.Engine.Tests/GitCredentialsTests.cs @@ -0,0 +1,114 @@ +/* + * Copyright (c) 2026 James Maes (KofTwentyTwo) + * SPDX-License-Identifier: MIT + */ + +using LibGit2Sharp; +using LibGit2Sharp.Handlers; + + +namespace gclo.Engine.Tests; + + +/// +/// The credential callback hands to libgit2 (#139). The +/// offline fixtures are local paths, which never ask for credentials, so the callback +/// is called here the way libgit2 calls it: with the URL of the remote that asks. +/// +public sealed class GitCredentialsTests +{ + private const string Token = "ghp_secret_token_value_1234567890"; + + private const string Listed = "https://github.com/acme/repo.git"; + + + + [Theory] + [InlineData("https://github.com/acme/repo.git")] + [InlineData("https://GitHub.com/acme/repo.git")] + public void ListedHostOverHttps_GetsTheTokenAsThePassword(string asking) + { + CredentialsHandler provider = LibGit2GitClient.MakeCredentialsProvider(Token, Listed); + + UsernamePasswordCredentials credentials = Assert.IsType(provider(asking, null, SupportedCredentialTypes.UsernamePassword)); + + Assert.Equal("x-access-token", credentials.Username); + Assert.Equal(Token, credentials.Password); + } + + + + [Theory] + [InlineData("https://evil.example/acme/repo.git")] + [InlineData("https://github.com.evil.example/acme/repo.git")] + [InlineData("https://github.com:8443/acme/repo.git")] + [InlineData("http://github.com/acme/repo.git")] + [InlineData("ssh://git@github.com/acme/repo.git")] + [InlineData("git@github.com:acme/repo.git")] + public void AnyOtherRemote_GetsNothing_AndTheErrorDoesNotCarryTheToken(string asking) + { + CredentialsHandler provider = LibGit2GitClient.MakeCredentialsProvider(Token, Listed); + + InvalidOperationException ex = Assert.Throws( + () => provider(asking, null, SupportedCredentialTypes.UsernamePassword)); + + Assert.Contains("Nothing was sent", ex.Message); + Assert.DoesNotContain(Token, ex.Message); + } + + + + [Fact] + public void ListedUrlThatIsNotHttps_NobodyGetsTheToken() + { + CredentialsHandler provider = LibGit2GitClient.MakeCredentialsProvider(Token, "http://git.example.com/acme/repo.git"); + + Assert.Throws( + () => provider("http://git.example.com/acme/repo.git", null, SupportedCredentialTypes.UsernamePassword)); + } + + + + [Fact] + public void ErrorForAnotherRemote_DoesNotShowCredentialsInItsUrl() + { + CredentialsHandler provider = LibGit2GitClient.MakeCredentialsProvider(Token, Listed); + + InvalidOperationException ex = Assert.Throws( + () => provider("https://someone:hunter2@evil.example/acme/repo.git", null, SupportedCredentialTypes.UsernamePassword)); + + Assert.Contains("https://evil.example/acme/repo.git", ex.Message); + Assert.DoesNotContain("hunter2", ex.Message); + } + + + + [Fact] + public void ThirdRequest_MeansTheTokenWasRejected_NotAnEndlessReplay() + { + CredentialsHandler provider = LibGit2GitClient.MakeCredentialsProvider(Token, Listed); + provider(Listed, null, SupportedCredentialTypes.UsernamePassword); + provider(Listed, null, SupportedCredentialTypes.UsernamePassword); + + InvalidOperationException ex = Assert.Throws( + () => provider(Listed, null, SupportedCredentialTypes.UsernamePassword)); + + Assert.Contains("rejected the token", ex.Message); + Assert.DoesNotContain(Token, ex.Message); + } + + + + [Fact] + public void RefusedRequests_DoNotCountTowardTheReplayLimit() + { + CredentialsHandler provider = LibGit2GitClient.MakeCredentialsProvider(Token, Listed); + for(int i = 0; i < 5; i++) + { + Assert.Throws( + () => provider("https://evil.example/x.git", null, SupportedCredentialTypes.UsernamePassword)); + } + + Assert.IsType(provider(Listed, null, SupportedCredentialTypes.UsernamePassword)); + } +} diff --git a/gclo.Engine.Tests/OriginVerificationTests.cs b/gclo.Engine.Tests/OriginVerificationTests.cs index 9b11bd9..ded7ae5 100644 --- a/gclo.Engine.Tests/OriginVerificationTests.cs +++ b/gclo.Engine.Tests/OriginVerificationTests.cs @@ -55,6 +55,9 @@ public sealed class OriginVerificationTests : IDisposable Assert.Contains("clone of a different repository", ex.Message); Assert.Contains(unrelated, ex.Message); Assert.Contains(listed, ex.Message); + // A renamed or transferred repository looks exactly like this; the message says + // how to point the folder at the new URL. + Assert.Contains($"git -C \"{target}\" remote set-url origin {listed}", ex.Message); Assert.Equal(tipBefore, HeadSha(target)); Assert.Equal(trackingBefore, TrackingTip(target)); } @@ -82,11 +85,16 @@ public sealed class OriginVerificationTests : IDisposable Assert.Equal("new content", File.ReadAllText(Path.Combine(target, "update.txt"))); using(var repo = new Repository(target)) { - // The folder's configuration is as the user left it: origin keeps its own - // spelling, and the remote used for the fetch is gone. + // Origin keeps the spelling the user gave it, and the remote used for the + // fetch is gone. Assert.Equal(new Uri(source).AbsoluteUri, repo.Network.Remotes["origin"].Url); Assert.Equal("origin", Assert.Single(repo.Network.Remotes).Name); } + // What was contacted is the listed URL, not origin's spelling of it: FETCH_HEAD + // records the URL of the remote a fetch used. + string fetchHead = File.ReadAllText(Path.Combine(target, ".git", "FETCH_HEAD")); + Assert.Contains(source.Replace('\\', '/'), fetchHead.Replace('\\', '/')); + Assert.DoesNotContain("file://", fetchHead); } @@ -137,6 +145,7 @@ public sealed class OriginVerificationTests : IDisposable repo.Network.Remotes.Update("origin", r => r.Url = new Uri(source).AbsoluteUri); // What a process killed mid-fetch leaves behind, pointing somewhere stale. repo.Network.Remotes.Add("gclo-sync", Path.Combine(_root, "gone")); + repo.Config.Set("remote.gclo-sync.gclo-temporary", true, ConfigurationLevel.Local); } string newSha = CommitFile(source, "update.txt", "new content", "second commit"); @@ -207,6 +216,113 @@ public sealed class OriginVerificationTests : IDisposable + [Fact] + public async Task Pull_ARemoteLeftByAKilledRun_IsAlsoRemoved_WhenOriginIsTheListedUrl() + { + string source = CreateSourceRepo("source"); + string target = Path.Combine(_root, "clone"); + await _client.CloneAsync(source, target, Token, null, CancellationToken.None); + using(var repo = new Repository(target)) + { + repo.Network.Remotes.Add("gclo-sync", Path.Combine(_root, "gone")); + repo.Config.Set("remote.gclo-sync.gclo-temporary", true, ConfigurationLevel.Local); + } + + await _client.PullAsync(target, Token, CancellationToken.None); + + using(var repo2 = new Repository(target)) + { + Assert.Equal("origin", Assert.Single(repo2.Network.Remotes).Name); + } + } + + + + [Fact] + public async Task Pull_TheUsersOwnRemoteNamedGcloSync_IsNeverRemoved() + { + string source = CreateSourceRepo("source"); + string elsewhere = CreateSourceRepo("elsewhere"); + string target = Path.Combine(_root, "clone"); + await _client.CloneAsync(source, target, Token, null, CancellationToken.None); + using(var repo = new Repository(target)) + { + repo.Network.Remotes.Add("gclo-sync", elsewhere); // theirs: no gclo marker + } + + // Origin is the listed URL: the name is not needed, and their remote stays. + await _client.PullAsync(target, Token, CancellationToken.None); + using(var repo = new Repository(target)) + { + Assert.Equal(elsewhere, repo.Network.Remotes["gclo-sync"].Url); + repo.Network.Remotes.Update("origin", r => r.Url = new Uri(source).AbsoluteUri); + } + + // Origin is spelled differently: the name is needed, so the pull says so instead + // of taking it over. + InvalidOperationException ex = await Assert.ThrowsAsync( + () => _client.FetchAndPullAsync(source, target, Token, CancellationToken.None)); + + Assert.Contains("remote of its own named 'gclo-sync'", ex.Message); + using(var repo = new Repository(target)) + { + Assert.Equal(elsewhere, repo.Network.Remotes["gclo-sync"].Url); + } + } + + + + [Fact] + public async Task Pull_FolderConfigurationRewritesUrls_IsRefused_BeforeAnythingIsFetched() + { + string source = CreateSourceRepo("source"); + string other = CreateSourceRepo("other"); + string target = Path.Combine(_root, "clone"); + await _client.CloneAsync(source, target, Token, null, CancellationToken.None); + string trackingBefore = TrackingTip(target); + using(var repo = new Repository(target)) + { + // url..insteadOf = : every fetch of the listed URL would + // really contact the other repository. + repo.Config.Set($"url.{other}.insteadof", source, ConfigurationLevel.Local); + } + CommitFile(source, "update.txt", "new content", "second commit"); + CommitFile(other, "planted.txt", "not what was listed", "planted commit"); + + InvalidOperationException ex = await Assert.ThrowsAsync( + () => _client.FetchAndPullAsync(source, target, Token, CancellationToken.None)); + + Assert.Contains("url..insteadOf", ex.Message); + Assert.Equal(trackingBefore, TrackingTip(target)); + Assert.False(File.Exists(Path.Combine(target, "planted.txt"))); + } + + + + [Fact] + public async Task Pull_FolderConfigurationNamesAnSshProgram_IsRefused_WhenTheFetchIsNotHttps() + { + // The fixtures are local paths, which stand in here for "not HTTPS": the case + // where the user's global configuration sends the fetch over SSH. + string source = CreateSourceRepo("source"); + string target = Path.Combine(_root, "clone"); + await _client.CloneAsync(source, target, Token, null, CancellationToken.None); + string trackingBefore = TrackingTip(target); + using(var repo = new Repository(target)) + { + repo.Config.Set("core.sshCommand", "calc.exe", ConfigurationLevel.Local); + } + CommitFile(source, "update.txt", "new content", "second commit"); + + InvalidOperationException ex = await Assert.ThrowsAsync( + () => _client.PullAsync(target, Token, CancellationToken.None)); + + Assert.Contains("core.sshCommand", ex.Message); + Assert.Equal(trackingBefore, TrackingTip(target)); + } + + + // ---------------------------------------------------------------- helpers private string CreateSourceRepo(string name) diff --git a/gclo.Engine.Tests/RemoteUrlTests.cs b/gclo.Engine.Tests/RemoteUrlTests.cs index 82c20e8..2032a8e 100644 --- a/gclo.Engine.Tests/RemoteUrlTests.cs +++ b/gclo.Engine.Tests/RemoteUrlTests.cs @@ -42,6 +42,8 @@ public sealed class RemoteUrlTests [InlineData("https://github.com/acme/repo.git/extra")] [InlineData("https://github.example.com/acme/repo.git")] [InlineData("https://gitlab.com/acme/repo.git")] + [InlineData("https://github.com:8443/acme/repo.git")] + [InlineData("https://\u00fc-/acme/repo.git")] [InlineData("git@bitbucket.org:acme/repo.git")] [InlineData("ssh://git@old-host.example/acme/repo.git")] [InlineData(@"C:\src\acme\repo")] @@ -101,6 +103,8 @@ public sealed class RemoteUrlTests [Theory] [InlineData("https://evil.example/acme/repo.git")] [InlineData("https://github.com.evil.example/acme/repo.git")] + [InlineData("https://github.com@evil.example/acme/repo.git")] + [InlineData("https://\u00fc-/acme/repo.git")] [InlineData("https://github.com:8443/acme/repo.git")] [InlineData("http://github.com/acme/repo.git")] [InlineData("ssh://git@github.com/acme/repo.git")] @@ -141,6 +145,8 @@ public sealed class RemoteUrlTests [InlineData("https://github.com/acme/repo.git", "github.com")] [InlineData("https://GitHub.com:443/acme/repo.git", "github.com")] [InlineData("https://git.example.com:3000/acme/repo.git", "git.example.com:3000")] + [InlineData("https://[::1]:3000/acme/repo.git", "[::1]:3000")] + [InlineData("https://\u00fc-/acme/repo.git", null)] [InlineData("http://github.com/acme/repo.git", null)] [InlineData("git@github.com:acme/repo.git", null)] [InlineData("", null)] @@ -157,16 +163,45 @@ public sealed class RemoteUrlTests [Theory] [InlineData("https://someone:hunter2@github.com/acme/repo.git", "https://github.com/acme/repo.git")] [InlineData("https://token@github.com/acme/repo.git", "https://github.com/acme/repo.git")] - [InlineData("ssh://git@github.com", "ssh://github.com")] + [InlineData(" https://someone:hunter2@github.com/acme/repo.git ", "https://github.com/acme/repo.git")] + [InlineData("https://someone:hun@ter2@github.com/acme/repo.git", "https://github.com/acme/repo.git")] + [InlineData("https://someone:hun/ter2@github.com/acme/repo.git", "https://github.com/acme/repo.git")] + [InlineData("https://github.com/acme/repo.git?access_token=hunter2", "https://github.com/acme/repo.git")] + [InlineData("https://github.com/acme/repo.git#hunter2", "https://github.com/acme/repo.git")] + [InlineData("https://someone:hunter2@git.example.com:3000/acme/repo.git", "https://git.example.com:3000/acme/repo.git")] + [InlineData("https://someone:hunter2@[::1]:3000/acme/repo.git", "https://[::1]:3000/acme/repo.git")] + [InlineData("ssh://git@github.com", "ssh://github.com/")] [InlineData("https://github.com/acme/repo.git", "https://github.com/acme/repo.git")] [InlineData("https://github.com/acme/user@example.git", "https://github.com/acme/user@example.git")] - [InlineData("https://github.com", "https://github.com")] + [InlineData("https://github.com", "https://github.com/")] + [InlineData("https://", "https://")] [InlineData(@"C:\src\acme\repo", @"C:\src\acme\repo")] - [InlineData("git@github.com:acme/repo.git", "git@github.com:acme/repo.git")] + [InlineData("git@github.com:acme/repo.git", "github.com:acme/repo.git")] + [InlineData("someone:hunter2@github.com:acme/repo.git", "github.com:acme/repo.git")] [InlineData("", "")] [InlineData(null, "")] - public void Redact_DropsCredentialsFromTheAuthority_AndNothingElse(string? url, string expected) + public void Redact_NeverShowsUserInformationQueryOrFragment(string? url, string expected) { - Assert.Equal(expected, RemoteUrl.Redact(url)); + string redacted = RemoteUrl.Redact(url); + + Assert.Equal(expected, redacted); + Assert.DoesNotContain("hunter2", redacted); + Assert.DoesNotContain("ter2", redacted); + } + + + + [Theory] + [InlineData("https://github.com/acme/repo.git", true)] + [InlineData("HTTPS://github.com", true)] + [InlineData("http://github.com/acme/repo.git", false)] + [InlineData("ssh://git@github.com/acme/repo.git", false)] + [InlineData("git@github.com:acme/repo.git", false)] + [InlineData(@"C:\src\acme\repo", false)] + [InlineData("https://\u00fc-/acme/repo.git", false)] + [InlineData(null, false)] + public void IsHttps_OnlyAnAbsoluteHttpsUrlWithAReadableHost(string? url, bool expected) + { + Assert.Equal(expected, RemoteUrl.IsHttps(url)); } } diff --git a/gclo.Engine.Tests/WslClonerTests.cs b/gclo.Engine.Tests/WslClonerTests.cs index 7caca70..ddc75e4 100644 --- a/gclo.Engine.Tests/WslClonerTests.cs +++ b/gclo.Engine.Tests/WslClonerTests.cs @@ -281,9 +281,12 @@ public sealed class WslClonerTests // "--" ends option parsing, so a URL can never be read as an option. Assert.Contains("clone --quiet -- \"$url\" \"$dest\"", script); // An existing destination is only fast-forwarded when it is a clone of this URL. - Assert.Contains("remote get-url origin)\" != \"$url\"", script); + // The stored URL is compared, not "remote get-url": that one applies the user's + // url.*.insteadOf rewrites and would refuse a destination gclo itself cloned. + Assert.Contains("config --get remote.origin.url)\" != \"$url\"", script); + Assert.DoesNotContain("remote get-url", script); Assert.True( - script.IndexOf("remote get-url origin", StringComparison.Ordinal) < script.IndexOf("pull --ff-only", StringComparison.Ordinal), + script.IndexOf("config --get remote.origin.url", StringComparison.Ordinal) < script.IndexOf("pull --ff-only", StringComparison.Ordinal), "the origin check must come before the pull"); } diff --git a/gclo.Engine/LibGit2GitClient.cs b/gclo.Engine/LibGit2GitClient.cs index ef8d492..851b1b8 100644 --- a/gclo.Engine/LibGit2GitClient.cs +++ b/gclo.Engine/LibGit2GitClient.cs @@ -66,6 +66,12 @@ public sealed class LibGit2GitClient : IGitClient /// private const string SyncRemoteRefSpec = "+refs/heads/*:refs/remotes/" + SyncRemoteName + "/*"; + /// + /// Marks the sync remote as gclo's, so cleanup never removes a remote of the same + /// name that the user created. It goes away with the remote. + /// + private const string SyncRemoteMarkerKey = "remote." + SyncRemoteName + ".gclo-temporary"; + /// /// Test seam: invoked right after a fetch completed, before the cancellation /// check that precedes any working-tree work. Local-transport fixtures fire no @@ -363,45 +369,123 @@ public sealed class LibGit2GitClient : IGitClient /// /// Fetches the listed into 's own /// refspecs, so the tracking refs land exactly where a fetch of origin puts them - /// while the transport is always the URL the listing returned. + /// while the remote that is contacted is the one the listing returned. /// /// /// When origin's URL is the listed URL, which is every clone gclo made itself, this /// is a plain fetch of origin. Otherwise the origin names the same repository in /// another spelling: switched to SSH for pushing, plain HTTP, credentials typed /// into it. Fetching that origin would let the folder's configuration choose the - /// transport (and, for SSH, the program that provides it). LibGit2Sharp cannot - /// fetch a bare URL, so the listed URL is fetched through a remote that exists - /// only for this call; origin itself is never rewritten (#139). + /// transport. LibGit2Sharp cannot fetch a bare URL, so the listed URL is fetched + /// through a remote that exists only for this call; origin itself is never + /// rewritten. The user's own (global) git configuration still applies to that + /// remote, as it does to everything git does for them (#139). /// private static void FetchListedUrl(Repository repo, Remote origin, string url, FetchOptions options) { + TryRemoveSyncRemote(repo); // left behind by a run that was killed mid-fetch + var refSpecs = origin.FetchRefSpecs.Select(s => s.Specification).ToList(); if(string.Equals(origin.Url, url, StringComparison.Ordinal)) { + RefuseSshCommandOfTheFolder(repo, origin.Url); Commands.Fetch(repo, origin.Name, refSpecs, options, logMessage: null); return; } - RemoveSyncRemote(repo); // left behind by a run that was killed mid-fetch - repo.Network.Remotes.Add(SyncRemoteName, url, SyncRemoteRefSpec); + if(repo.Network.Remotes[SyncRemoteName] is not null) + { + // Not marked as gclo's, or TryRemoveSyncRemote would have removed it. + throw new InvalidOperationException( + $"This repository has a remote of its own named '{SyncRemoteName}', a name gclo needs for a moment " + + "to fetch the listed URL. Rename that remote, then sync again."); + } + + Remote sync = repo.Network.Remotes.Add(SyncRemoteName, url, SyncRemoteRefSpec); + repo.Config.Set(SyncRemoteMarkerKey, true, ConfigurationLevel.Local); try { + RefuseSshCommandOfTheFolder(repo, sync.Url); + // The fetch behaves like a fetch of origin in every respect but the URL. + options.TagFetchMode = origin.TagFetchMode; Commands.Fetch(repo, SyncRemoteName, refSpecs, options, logMessage: null); } finally { - RemoveSyncRemote(repo); + TryRemoveSyncRemote(repo); } } - private static void RemoveSyncRemote(Repository repo) + /// + /// Removes the remote adds, when it is there and carries + /// gclo's marker; a remote of that name the user made themselves is left alone. + /// Best effort: a failure here (another program holding .git/config open) must not + /// replace the outcome of the fetch, and the next sync removes what is left. + /// + private static void TryRemoveSyncRemote(Repository repo) { - if(repo.Network.Remotes[SyncRemoteName] is not null) + try { - repo.Network.Remotes.Remove(SyncRemoteName); + if(repo.Network.Remotes[SyncRemoteName] is not null && repo.Config.Get(SyncRemoteMarkerKey)?.Value == true) + { + repo.Network.Remotes.Remove(SyncRemoteName); + } + } + catch(LibGit2SharpException) + { + // See the summary: never mask the fetch's own result. + } + } + + + + /// + /// A folder's own git configuration must not decide where a sync's fetch goes. Two + /// settings could: url.<base>.insteadOf, which rewrites the URL of any + /// remote, and (see ) the program used for + /// SSH. Both are refused when they come from the folder's own configuration; the + /// user's global configuration is theirs and is honored (#139). + /// + private static void RefuseUrlRewritesOfTheFolder(Repository repo) + { + foreach(ConfigurationEntry entry in repo.Config) + { + if(entry.Level >= ConfigurationLevel.Local + && entry.Key.StartsWith("url.", StringComparison.OrdinalIgnoreCase) + && entry.Key.EndsWith(".insteadof", StringComparison.OrdinalIgnoreCase)) + { + throw new InvalidOperationException( + "This folder's own git configuration rewrites remote URLs (a url..insteadOf entry in its .git/config), " + + "so a fetch could go somewhere other than the listed repository. Remove that entry, then sync again."); + } + } + } + + + + /// + /// When the URL a fetch will really use is not HTTPS (the user's global git + /// configuration rewrites it to SSH), the transport is an external SSH program. + /// Which program must not be up to the folder: a core.sshCommand in its own + /// configuration is refused (#139). + /// + private static void RefuseSshCommandOfTheFolder(Repository repo, string effectiveUrl) + { + if(RemoteUrl.IsHttps(effectiveUrl)) + { + return; + } + + foreach(ConfigurationEntry entry in repo.Config) + { + if(entry.Level >= ConfigurationLevel.Local && string.Equals(entry.Key, "core.sshcommand", StringComparison.OrdinalIgnoreCase)) + { + throw new InvalidOperationException( + "This folder's own git configuration sets core.sshCommand, and this fetch would go over SSH. " + + "gclo does not run a program chosen by the folder it is syncing. Remove that entry from its .git/config, then sync again."); + } } } @@ -417,14 +501,18 @@ public sealed class LibGit2GitClient : IGitClient /// private static Remote OriginOfListedRepository(Repository repo, string url) { + RefuseUrlRewritesOfTheFolder(repo); + Remote remote = repo.Network.Remotes["origin"] ?? throw new InvalidOperationException("Repository has no 'origin' remote."); if(!RemoteUrl.IsSameRepository(url, remote.Url)) { + string folder = Path.TrimEndingDirectorySeparator(repo.Info.WorkingDirectory ?? repo.Info.Path); throw new InvalidOperationException( $"This folder is a clone of a different repository: its origin is '{RemoteUrl.Redact(remote.Url)}', not '{url}'. " - + "Move or delete the folder, or point its origin at the listed repository, then sync again."); + + "If the repository was renamed or moved to another owner, point the folder at it with " + + $"'git -C \"{folder}\" remote set-url origin {url}'. Otherwise move or delete the folder. Then sync again."); } return remote; @@ -889,12 +977,13 @@ public sealed class LibGit2GitClient : IGitClient /// up with "too many redirects or authentication replays", once per repository. /// A correct token is never asked for a third time, so the third request means the /// token is bad and gets the same actionable message the API listers produce (#31). - /// The token is for 's host only: a request for any - /// other host (a redirect, a folder configured for another server) or over anything - /// but HTTPS is refused, so the token never leaves for somewhere the repository was - /// not listed on (#139). + /// The token is for 's host only: a remote on any + /// other host, or one reached over anything but HTTPS, is refused, so the token is + /// never offered to a server the repository was not listed on (#139). libgit2 asks + /// with the remote's own URL; a redirect the listed host itself issues is that + /// host's doing, and it already holds the token. /// - private static CredentialsHandler MakeCredentialsProvider(string token, string expectedUrl) + internal static CredentialsHandler MakeCredentialsProvider(string token, string expectedUrl) { int requests = 0; return (requestedUrl, _, _) => diff --git a/gclo.Engine/RemoteUrl.cs b/gclo.Engine/RemoteUrl.cs index 997d166..d8553bb 100644 --- a/gclo.Engine/RemoteUrl.cs +++ b/gclo.Engine/RemoteUrl.cs @@ -13,21 +13,25 @@ namespace gclo.Engine; /// /// Decides two things about git remote URLs, both from the clone URL a listing returned /// (the "expected" URL): whether an existing folder's origin names that same -/// repository, and whether a host that asks for credentials is the one the token is +/// repository, and whether a remote that asks for credentials is one the token is /// meant for. A sync must never report a different repository as updated, and must -/// never hand the token to a host the listing did not come from (#139). +/// never hand the token to a remote on another host (#139). /// internal static class RemoteUrl { private static readonly char[] s_separators = ['/', '\\']; + + /// /// True when (a folder's configured origin) names the same /// repository as (the listed clone URL). Network URLs - /// are compared by host and path, ignoring scheme, credentials, port, letter case, - /// a trailing slash and a trailing .git, so an origin switched to SSH still - /// matches its HTTPS listing. Local paths (the offline test fixtures, or a caller's - /// own mirror) are compared as full paths. A network URL never matches a local path. + /// are compared by host and path, ignoring scheme, credentials, letter case, a + /// trailing slash and a trailing .git, so an origin switched to SSH still + /// matches its HTTPS listing; the port counts only between two HTTP(S) URLs, where + /// it tells two servers on one host apart. Local paths (the offline test fixtures, + /// or a caller's own mirror) are compared as full paths. A network URL never + /// matches a local path, and a URL that cannot be read matches nothing. /// public static bool IsSameRepository(string expected, string actual) { @@ -37,50 +41,72 @@ internal static class RemoteUrl return false; } - (string Host, string Path)? expectedRemote = ParseNetwork(expected); - (string Host, string Path)? actualRemote = ParseNetwork(actual); - if(expectedRemote is null || actualRemote is null) + Location expectedLocation = Parse(expected); + Location actualLocation = Parse(actual); + if(expectedLocation.Kind != actualLocation.Kind || expectedLocation.Kind == LocationKind.Unreadable) { - return expectedRemote is null - && actualRemote is null - && string.Equals(NormalizeLocal(expected), NormalizeLocal(actual), StringComparison.OrdinalIgnoreCase); + return false; } - return string.Equals(expectedRemote.Value.Host, actualRemote.Value.Host, StringComparison.OrdinalIgnoreCase) - && string.Equals(expectedRemote.Value.Path, actualRemote.Value.Path, StringComparison.OrdinalIgnoreCase); + if(expectedLocation.Kind == LocationKind.Local) + { + return string.Equals( + expectedLocation.Path, actualLocation.Path, + OperatingSystem.IsWindows() ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal); + } + + return string.Equals(expectedLocation.Host, actualLocation.Host, StringComparison.OrdinalIgnoreCase) + && string.Equals(expectedLocation.Path, actualLocation.Path, StringComparison.OrdinalIgnoreCase) + && (expectedLocation.WebPort is null || actualLocation.WebPort is null || expectedLocation.WebPort == actualLocation.WebPort); } /// - /// True when a credential request for may be answered - /// with the token of a sync whose listed clone URL is : - /// both must be HTTPS and name the same host and port. Anything else (another host - /// after a redirect, plain HTTP, SSH) gets no token. + /// True when a credential request from a remote whose URL is + /// may be answered with the token of a sync whose listed + /// clone URL is : both must be HTTPS and name the same + /// host and port. A remote on another host, plain HTTP, and SSH get no token. /// public static bool MayReceiveCredentials(string expected, string? requested) { ArgumentException.ThrowIfNullOrWhiteSpace(expected); - return TryParseHttps(expected, out Uri? expectedUri) - && TryParseHttps(requested, out Uri? requestedUri) - && string.Equals(expectedUri.IdnHost, requestedUri.IdnHost, StringComparison.OrdinalIgnoreCase) + return TryParseHttps(expected, out Uri? expectedUri, out string? expectedHost) + && TryParseHttps(requested, out Uri? requestedUri, out string? requestedHost) + && string.Equals(expectedHost, requestedHost, StringComparison.OrdinalIgnoreCase) && expectedUri.Port == requestedUri.Port; } + /// True when is an absolute HTTPS URL with a readable host. + public static bool IsHttps(string? url) => TryParseHttps(url, out _, out _); + + + /// - /// The host of an HTTPS , with its port when that is not 443; - /// null when the URL is not absolute HTTPS. This is the scope a token may be used in. + /// The host of an HTTPS , with its port when that is not 443 + /// and with the brackets of an IPv6 literal; null when the URL is not absolute HTTPS. + /// This is the scope a token may be used in. /// public static string? HttpsAuthority(string? url) - => TryParseHttps(url, out Uri? uri) ? uri.IdnHost + (uri.IsDefaultPort ? "" : ":" + uri.Port.ToString(CultureInfo.InvariantCulture)) : null; + { + if(!TryParseHttps(url, out Uri? uri, out string? host)) + { + return null; + } + + string authority = uri.HostNameType == UriHostNameType.IPv6 ? $"[{host}]" : host; + return uri.IsDefaultPort ? authority : authority + ":" + uri.Port.ToString(CultureInfo.InvariantCulture); + } /// - /// without any user:password@ part, for error messages - /// and logs: an origin may carry credentials someone typed into it. + /// in a form that is safe to show and log: an origin may + /// carry credentials someone typed into it. A readable network URL is rebuilt from + /// its scheme, host, port and path, so user information, query and fragment are + /// gone; anything else loses everything up to its last @. /// public static string Redact(string? url) { @@ -89,58 +115,103 @@ internal static class RemoteUrl return ""; } - int scheme = url.IndexOf("://", StringComparison.Ordinal); - if(scheme < 0) + string trimmed = url.Trim(); + int scheme = trimmed.IndexOf("://", StringComparison.Ordinal); + if(scheme > 0 && Uri.TryCreate(trimmed, UriKind.Absolute, out Uri? uri) && !uri.IsFile && TryGetHost(uri, out string? host)) { - return url; + string authority = uri.HostNameType == UriHostNameType.IPv6 ? $"[{host}]" : host; + string port = uri.IsDefaultPort ? "" : ":" + uri.Port.ToString(CultureInfo.InvariantCulture); + return $"{uri.Scheme}://{authority}{port}{uri.AbsolutePath}"; } - int authorityStart = scheme + 3; - int authorityEnd = url.IndexOf('/', authorityStart); - int at = url.IndexOf('@', authorityStart); - return at >= 0 && (authorityEnd < 0 || at < authorityEnd) - ? string.Concat(url.AsSpan(0, authorityStart), url.AsSpan(at + 1)) - : url; + int at = trimmed.LastIndexOf('@'); + if(at < 0) + { + return trimmed; + } + + return scheme > 0 ? string.Concat(trimmed.AsSpan(0, scheme + 3), trimmed.AsSpan(at + 1)) : trimmed[(at + 1)..]; } - private static bool TryParseHttps(string? url, [NotNullWhen(true)] out Uri? uri) - => Uri.TryCreate(url, UriKind.Absolute, out uri) - && string.Equals(uri.Scheme, Uri.UriSchemeHttps, StringComparison.OrdinalIgnoreCase); + private static bool TryParseHttps(string? url, [NotNullWhen(true)] out Uri? uri, [NotNullWhen(true)] out string? host) + { + host = null; + return Uri.TryCreate(url, UriKind.Absolute, out uri) + && string.Equals(uri.Scheme, Uri.UriSchemeHttps, StringComparison.OrdinalIgnoreCase) + && TryGetHost(uri, out host); + } - /// Host and normalized path of a network git URL, or null for a local path. - private static (string Host, string Path)? ParseNetwork(string url) + /// + /// The host in its ASCII (punycode) form. throws for a + /// host that parses as a URI but is not a valid internationalized name; such a + /// host is nobody's, not an error. + /// + private static bool TryGetHost(Uri uri, [NotNullWhen(true)] out string? host) + { + try + { + host = uri.IdnHost; + return host.Length > 0; + } + catch(UriFormatException) + { + host = null; + return false; + } + } + + + + private static Location Parse(string url) { string trimmed = url.Trim(); int scheme = trimmed.IndexOf("://", StringComparison.Ordinal); if(scheme > 0) { - return Uri.TryCreate(trimmed, UriKind.Absolute, out Uri? uri) && !uri.IsFile && uri.IdnHost.Length > 0 - ? (uri.IdnHost, NormalizePath(Uri.UnescapeDataString(uri.AbsolutePath))) - : null; + if(!Uri.TryCreate(trimmed, UriKind.Absolute, out Uri? uri)) + { + return new Location(LocationKind.Unreadable, "", "", null); + } + + if(uri.IsFile) + { + return Local(uri.LocalPath); + } + + bool web = uri.Scheme is "http" or "https"; + return TryGetHost(uri, out string? host) + ? new Location(LocationKind.Network, host, NormalizePath(Uri.UnescapeDataString(uri.AbsolutePath)), web ? (uri.IsDefaultPort ? 0 : uri.Port) : null) + : new Location(LocationKind.Unreadable, "", "", null); } // The scp-like form git accepts for SSH: [user@]host:path. A single letter before // the colon is a Windows drive ("C:\src\repo"), not a host, and a separator // before the colon means the colon belongs to a local path. int colon = trimmed.IndexOf(':', StringComparison.Ordinal); - if(colon < 0) + if(colon >= 0) { - return null; + string host = trimmed[(trimmed.LastIndexOf('@', colon) + 1)..colon]; + string path = trimmed[(colon + 1)..]; + if(host.Length >= 2 && host.IndexOfAny(s_separators) < 0 && path.Length > 0 && path[0] != '\\') + { + return new Location(LocationKind.Network, host, NormalizePath(path), null); + } } - string host = trimmed[(trimmed.LastIndexOf('@', colon) + 1)..colon]; - string path = trimmed[(colon + 1)..]; - return host.Length >= 2 && host.IndexOfAny(s_separators) < 0 && path.Length > 0 && path[0] != '\\' - ? (host, NormalizePath(path)) - : null; + return Local(trimmed); } + private static Location Local(string path) + => new(LocationKind.Local, "", Path.TrimEndingDirectorySeparator(Path.GetFullPath(path)), null); + + + private static string NormalizePath(string path) { string trimmed = path.Trim('/'); @@ -149,10 +220,19 @@ internal static class RemoteUrl - private static string NormalizeLocal(string path) + private enum LocationKind { - string local = Uri.TryCreate(path.Trim(), UriKind.Absolute, out Uri? uri) && uri.IsFile ? uri.LocalPath : path.Trim(); - return Path.TrimEndingDirectorySeparator(Path.GetFullPath(local)); + Network, + Local, + Unreadable, } + + + /// Where a git URL points: a host and path, or a local full path. + /// Network, local, or not readable as either. + /// ASCII host of a network URL; empty otherwise. + /// Normalized repository path, or the local full path. + /// Port of an HTTP(S) URL, 0 for the scheme's default (so HTTP and HTTPS on their usual ports agree); null for SSH, scp-like and local. + private readonly record struct Location(LocationKind Kind, string Host, string Path, int? WebPort); } diff --git a/gclo.Engine/WslCloner.cs b/gclo.Engine/WslCloner.cs index 5553bb5..311173f 100644 --- a/gclo.Engine/WslCloner.cs +++ b/gclo.Engine/WslCloner.cs @@ -35,8 +35,8 @@ public sealed class WslCloner : IWslCloner + "dest=\"$HOME/gclo/$org/$repo\"\n" + "helper='!f() { echo username=x-access-token; echo \"password=$" + TokenVariable + "\"; }; f'\n" + "if [ -d \"$dest/.git\" ]; then\n" - + " if [ \"$(git -C \"$dest\" remote get-url origin)\" != \"$url\" ]; then\n" - + " echo \"$dest is a clone of a different repository; move or delete it, then try again\" >&2\n" + + " if [ \"$(git -C \"$dest\" config --get remote.origin.url)\" != \"$url\" ]; then\n" + + " echo \"$dest exists but is not a clone of this repository; move or delete it, then try again\" >&2\n" + " exit 1\n" + " fi\n" + " git -C \"$dest\" -c credential.helper= -c \"credential.https://$host.helper=$helper\" pull --ff-only --quiet\n" -- 2.54.0