fix(engine): second review of #198: a repository named as git's own folder fails its row, and a folder's redirect setting is refused too
- The first version refused the name in RepositoryPathResolver, which made the whole sync throw where on Windows only that row failed before. The resolver is as it was; the sync fails that row on every platform, and the WSL clone refuses the name. - A folder's own http.followRedirects is refused with its proxy settings; the message names the setting found. Refs #184 Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> Signed-off-by: James Maes <james@kof22.com>
This commit is contained in:
1 parent
f07acac965
commit
9ab47565eb
9 files changed
+87
-38
No files matched your search
@@ -85,8 +85,8 @@ flowchart LR
|
||||
| --- | --- | --- | --- | --- | --- | --- | --- |
|
||||
| T1 | Update feed | Tampering | Attacker serves a malicious update | Low | Critical | HTTPS to the project's releases on `git.kof22.com` only (ADR 0009); Velopack verifies each package against the size and hash in the feed. The feed and the packages come from the same server, and the updater does not check an Authenticode signature or an attestation: whoever controls that server, the repository's release permission, or the server's TLS certificate can deliver a package that installs without a warning. The mitigations are therefore the server's own: who may publish a release there, how its certificate and DNS are held. Releases are not immutable on Gitea. | Accepted for 1.1.1; depends on the server's access control |
|
||||
| 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, which also refuses a `.git` segment in any spelling Windows opens as that folder; `RepositoryPathResolver` containment, which refuses a repository named as git's own folder on every platform (#184); recovery by rename/skip or clone in WSL; `core.longpaths`. The recovery materializes the tree itself, so every write and delete it makes goes through `WorkingTree`: inside the working directory, never in `.git`, never through a link, and an existing file is replaced rather than written through (#141). Residual: the link check and the write are two steps, so a link swapped in between them by something already running as the user is not caught | 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 the remote contacted is the listed URL, not origin's spelling of it; a `url.<base>.insteadOf` rewrite in the folder's own configuration is refused, so is a proxy it names (`http.proxy`, `http.<url>.proxy`, `remote.<name>.proxy`; #184), 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. The recovery file `.git\gclo-recovery.json` is validated when it is loaded: a listed path that is not in the working tree fails the row instead of being deleted (#141) | Mitigated for the remote, the transport program, and the recovery file; other `.git` content of an existing folder is still trusted |
|
||||
| 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, which also refuses a `.git` segment in any spelling Windows opens as that folder; `RepositoryPathResolver` containment; a repository named as git's own folder fails its row on every platform, and the WSL clone refuses it (#184); recovery by rename/skip or clone in WSL; `core.longpaths`. The recovery materializes the tree itself, so every write and delete it makes goes through `WorkingTree`: inside the working directory, never in `.git`, never through a link, and an existing file is replaced rather than written through (#141). Residual: the link check and the write are two steps, so a link swapped in between them by something already running as the user is not caught | 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 the remote contacted is the listed URL, not origin's spelling of it; a `url.<base>.insteadOf` rewrite in the folder's own configuration is refused, so are a proxy it names and its own redirect setting (`http.proxy`, `http.<url>.proxy`, `remote.<name>.proxy`, `http.followRedirects`; #184), 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. The recovery file `.git\gclo-recovery.json` is validated when it is loaded: a listed path that is not in the working tree fails the row instead of being deleted (#141) | Mitigated for the remote, the transport program, and the recovery file; 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 |
|
||||
@@ -123,4 +123,4 @@ flowchart LR
|
||||
| 2026-11-10 | v1.1.1 | James Maes (@KofTwentyTwo) | The project's home moved to `git.kof22.com`: the Help menu, the About dialog, the CLI's error text and the documentation link there, and the Scoop bucket is added from there (the manifest that pins the download URL and its hash now comes from that server over HTTPS; T2 is unchanged in kind). Help > Share hands a fixed text (what gclo is, where to download it, the sender's gclo version) to the system's mail handler as a `mailto:` link with no recipient, or shows it and copies it to the clipboard; nothing about the user, their accounts, or their repositories is in it, and the address it is built from must be a plain http(s) host and path (#163, #165). The update feed and the release downloads named in package manifests are still GitHub Releases at this revision. |
|
||||
| 2026-11-10 | v1.1.1 | James Maes (@KofTwentyTwo) | Working tree (#174; the T3 and T4 rows are unchanged, this adds to what they rely on). On Windows, a forced checkout into a folder whose tree is not finished (the first checkout of a clone that was empty, and the completion of a checkout that was cut off) is preceded by refusals that change nothing: a link on the way or at a destination (except, at a destination where the repository itself has a symbolic link, a link that leads where the repository's does), a file where a folder is needed (also on the way to a submodule), a folder that is not empty where a file is needed, a file at a destination that is not what the unfinished checkout wrote there, and anything but an empty folder where the repository has a submodule. "What the unfinished checkout wrote" is the content, or the beginning of it, of the commit the branch is on or of a commit recorded in `gclo.checkoutwrittenbefore`, as stored or as the checkout filters make it, carriage returns aside. An unfinished checkout is a marked folder that has no index, or that does not hold every file of its commit; a marked folder that has an index and every file is updated like any clone, without force. A completion is refused when the index names a file the commit does not have (somebody added it with git, and the forced checkout would remove it). An unfinished checkout is completed at the commit its branch is on, before the branch moves, except where the branch must move first (a tree Windows cannot write, or a history origin rewrote); the commit it leaves is then recorded, and a path recovery of that folder accepts a file that is, whole, what a recorded commit has. A repository that has local branches is not treated as an empty clone; one without follows origin's default branch only when its folder holds nothing but `.git`. The folder is marked unfinished before its branch is created. One git operation per folder at a time within a process (not across processes, and not across spellings of a path that only the file system can resolve). The WSL clone is made in `~/gclo/.gclo-tmp` and renamed into place when complete (`mv -T`, after a check that nothing is there). Not covered: a file created in the folder between the check and the checkout that follows it; a file of the user's at a destination that is empty, or holds the repository's content or the beginning of it, which is replaced; a link of the user's that leads exactly where the repository's symbolic link at that path does, which is replaced by the repository's; a marked folder whose index somebody's git wrote and whose last file was cut off, which is taken for finished (git shows that file as changed); files of a commit the branch left that the new commit does not have, which stay in the folder untracked; an unfinished clone in which a folder of an earlier commit became a file, or that was cut off by a power loss leaving files of zeros, which is refused until the folder is cleared; platforms other than Windows, where the checkout is forced as before. |
|
||||
| 2026-11-10 | v1.1.1 | James Maes (@KofTwentyTwo) | Update feed and release downloads moved from GitHub Releases to the project's releases on `git.kof22.com` (ADR 0009, #170): the in-app updater reads the feed with Velopack's Gitea source, anonymously over HTTPS, and the Scoop and Chocolatey manifests download from there. The updater verifies packages against the hashes in the feed, as before; it has never checked a signature or an attestation. The trust that was placed in GitHub's release hosting (account security, immutable releases) is now placed in a self-hosted server: whoever can publish a release there, or holds its TLS certificate or DNS, can deliver an update that installs without a warning. T1 is rewritten to say so and its status changed from mitigated to accepted. The manifests still pin a SHA-256 (T2). |
|
||||
| 2026-11-10 | v1.1.1 | James Maes (@KofTwentyTwo) | T4: a proxy named in a folder's own git configuration is refused like a URL rewrite, so the folder does not choose the server a fetch is made through (the token was not exposed by it: TLS is end to end). T3: a repository whose name is git's own folder name, in any spelling Windows opens as that folder, is refused by `RepositoryPathResolver` on every platform, not only on the Windows sync path (#184). |
|
||||
| 2026-11-10 | v1.1.1 | James Maes (@KofTwentyTwo) | T4: a proxy named in a folder's own git configuration, and its own `http.followRedirects`, are refused like a URL rewrite, so the folder does not choose the server a fetch is made through or sent on to (the token was not exposed by the proxy: TLS is end to end). T3: a repository whose name is git's own folder name, in any spelling Windows opens as that folder, fails its row in the sync on every platform, not only on Windows, and is refused by the WSL clone (#184). |
|
||||
@@ -241,6 +241,28 @@ public sealed class OrgSyncEngineTests : IDisposable
|
||||
|
||||
|
||||
|
||||
[Theory]
|
||||
[InlineData(".git")]
|
||||
[InlineData(".GIT")]
|
||||
[InlineData("git~1")]
|
||||
[InlineData(".git.")]
|
||||
public async Task Sync_RepoNamedAsGitsOwnFolder_FailsThatRowWithoutTouchingGit_OnEveryPlatform(string name)
|
||||
{
|
||||
_lister.Repositories = Repos("alpha", name, "bravo");
|
||||
|
||||
SyncSummary summary = await CreateEngine().SyncAsync(CreateRequest(), _progress);
|
||||
|
||||
Assert.Equal(new SyncSummary(Total: 3, Cloned: 2, Updated: 0, Failed: 1, Canceled: 0, WasCanceled: false), summary);
|
||||
RepoProgress failed = _progress.LastFor(name)!;
|
||||
Assert.Equal(SyncStatus.Failed, failed.Status);
|
||||
Assert.Contains("git uses for its own folder", failed.Error);
|
||||
Assert.Contains("Rename the repository", failed.Error);
|
||||
Assert.DoesNotContain(name, _git.ClonedRepoNames, StringComparer.Ordinal);
|
||||
Assert.DoesNotContain(_git.ValidityChecks, p => string.Equals(Path.GetFileName(p), name, StringComparison.Ordinal));
|
||||
}
|
||||
|
||||
|
||||
|
||||
[Fact]
|
||||
public async Task Sync_RepoNamedLikeAWindowsDevice_FailsThatRowWithoutTouchingGit()
|
||||
{
|
||||
|
||||
@@ -305,7 +305,9 @@ public sealed class OriginVerificationTests : IDisposable
|
||||
[InlineData("http.https://example.test/.proxy")]
|
||||
[InlineData("remote.origin.proxy")]
|
||||
[InlineData("remote.gclo-sync.proxy")]
|
||||
public async Task Pull_FolderConfigurationNamesAProxy_IsRefused_BeforeAnythingIsFetched(string key)
|
||||
[InlineData("http.followRedirects")]
|
||||
[InlineData("http.https://example.test/.followRedirects")]
|
||||
public async Task Pull_FolderConfigurationNamesAProxy_OrSaysHowRedirectsAreFollowed_IsRefused_BeforeAnythingIsFetched(string key)
|
||||
{
|
||||
string source = CreateSourceRepo("source");
|
||||
string target = Path.Combine(_root, "clone");
|
||||
@@ -313,7 +315,7 @@ public sealed class OriginVerificationTests : IDisposable
|
||||
string trackingBefore = TrackingTip(target);
|
||||
using(var repo = new Repository(target))
|
||||
{
|
||||
// The fetch would be made through a server the folder chose (#184).
|
||||
// The fetch would be made through, or be sent on to, a server the folder chose (#184).
|
||||
repo.Config.Set(key, "http://127.0.0.1:9", ConfigurationLevel.Local);
|
||||
}
|
||||
CommitFile(source, "update.txt", "new content", "second commit");
|
||||
@@ -321,7 +323,8 @@ public sealed class OriginVerificationTests : IDisposable
|
||||
InvalidOperationException ex = await Assert.ThrowsAsync<InvalidOperationException>(
|
||||
() => _client.FetchAndPullAsync(source, target, Token, CancellationToken.None));
|
||||
|
||||
Assert.Contains("names a proxy", ex.Message);
|
||||
Assert.Contains("decides where a fetch is made", ex.Message);
|
||||
Assert.Contains(key[(key.LastIndexOf('.') + 1)..], ex.Message, StringComparison.OrdinalIgnoreCase); // the setting is named
|
||||
Assert.Equal(trackingBefore, TrackingTip(target));
|
||||
Assert.False(File.Exists(Path.Combine(target, "update.txt")));
|
||||
}
|
||||
|
||||
@@ -106,11 +106,6 @@ public sealed class RepositoryPathResolverTests
|
||||
[InlineData("name:stream", ":")]
|
||||
[InlineData("tab\there", "not allowed")]
|
||||
[InlineData("nul\0byte", "not allowed")]
|
||||
[InlineData(".git", "git uses for its own folder")] // #184: on every platform, for every caller
|
||||
[InlineData(".GIT", "git uses for its own folder")]
|
||||
[InlineData("git~1", "git uses for its own folder")]
|
||||
[InlineData(".git.", "git uses for its own folder")]
|
||||
[InlineData(".git ", "git uses for its own folder")]
|
||||
public void TryValidateName_UnsafeName_ReturnsFalseWithReason(string? name, string expectedReasonFragment)
|
||||
{
|
||||
Assert.False(RepositoryPathResolver.TryValidateName(name, out string? reason));
|
||||
|
||||
@@ -341,6 +341,22 @@ public sealed class WslClonerTests
|
||||
|
||||
|
||||
|
||||
[Theory]
|
||||
[InlineData(".git", "repo")]
|
||||
[InlineData("acme", ".git")]
|
||||
[InlineData("acme", "GIT~1")]
|
||||
[InlineData("acme", ".git.")]
|
||||
public async Task Clone_UnderGitsOwnFolderName_IsRefused_WithoutStartingWsl(string organization, string repository)
|
||||
{
|
||||
WslCloneException ex = await Assert.ThrowsAsync<WslCloneException>(
|
||||
() => Create().CloneAsync("https://github.com/acme/repo.git", organization, repository, Token, CancellationToken.None));
|
||||
|
||||
Assert.Contains("git uses for its own folder", ex.Message);
|
||||
Assert.Empty(_runner.Calls);
|
||||
}
|
||||
|
||||
|
||||
|
||||
[Theory]
|
||||
[InlineData(".gclo-tmp")]
|
||||
[InlineData(".GCLO-TMP")]
|
||||
|
||||
@@ -554,12 +554,14 @@ public sealed class LibGit2GitClient : IGitClient
|
||||
|
||||
/// <summary>
|
||||
/// A folder's own git configuration must not decide where a sync's fetch goes.
|
||||
/// Three settings could: <c>url.<base>.insteadOf</c>, which rewrites the URL
|
||||
/// These settings could: <c>url.<base>.insteadOf</c>, which rewrites the URL
|
||||
/// of any remote; a proxy (<c>http.proxy</c>, <c>http.<url>.proxy</c>,
|
||||
/// <c>remote.<name>.proxy</c>), through which the connection would be made
|
||||
/// (#184); and (see <see cref="RefuseSshCommandOfTheFolder"/>) the program used
|
||||
/// for SSH. All are refused when they come from the folder's own configuration;
|
||||
/// the user's global configuration is theirs and is honored (#139).
|
||||
/// <c>remote.<name>.proxy</c>), through which the connection would be made;
|
||||
/// <c>http.followRedirects</c>, which says how far a server's redirects are
|
||||
/// followed (#184); and (see <see cref="RefuseSshCommandOfTheFolder"/>) the
|
||||
/// program used for SSH. All are refused when they come from the folder's own
|
||||
/// configuration, whichever file of it they are written in; the user's global
|
||||
/// configuration is theirs and is honored (#139).
|
||||
/// </summary>
|
||||
private static void RefuseUrlRewritesOfTheFolder(Repository repo)
|
||||
{
|
||||
@@ -574,12 +576,12 @@ public sealed class LibGit2GitClient : IGitClient
|
||||
+ "so a fetch could go somewhere other than the listed repository. Remove that entry, then sync again.");
|
||||
}
|
||||
|
||||
if(entry.Level >= ConfigurationLevel.Local && NamesAProxy(entry.Key))
|
||||
if(entry.Level >= ConfigurationLevel.Local && DecidesWhereAFetchIsMade(entry.Key))
|
||||
{
|
||||
throw new InvalidOperationException(
|
||||
"This folder's own git configuration names a proxy (an http.proxy or remote.<name>.proxy entry in its .git/config), "
|
||||
+ "so a fetch would be made through a server the folder chose. Remove that entry, then sync again; "
|
||||
+ "a proxy that is yours to use belongs in your global git configuration.");
|
||||
$"This folder's own git configuration has a setting that decides where a fetch is made ('{entry.Key}' in its .git/config: "
|
||||
+ "a proxy, or how redirects are followed). Remove that entry, then sync again; "
|
||||
+ "if it is a setting you need, it belongs in your global git configuration.");
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -587,12 +589,17 @@ public sealed class LibGit2GitClient : IGitClient
|
||||
|
||||
|
||||
/// <summary>
|
||||
/// True for a configuration key that names a proxy for HTTP remotes:
|
||||
/// <c>http.proxy</c>, <c>http.<url>.proxy</c>, <c>remote.<name>.proxy</c>.
|
||||
/// True for a configuration key that names a proxy for HTTP remotes
|
||||
/// (<c>http.proxy</c>, <c>http.<url>.proxy</c>, <c>remote.<name>.proxy</c>)
|
||||
/// or says how redirects are followed (<c>http.followRedirects</c>,
|
||||
/// <c>http.<url>.followRedirects</c>).
|
||||
/// </summary>
|
||||
private static bool NamesAProxy(string key)
|
||||
=> key.EndsWith(".proxy", StringComparison.OrdinalIgnoreCase)
|
||||
&& (key.StartsWith("http.", StringComparison.OrdinalIgnoreCase) || key.StartsWith("remote.", StringComparison.OrdinalIgnoreCase));
|
||||
private static bool DecidesWhereAFetchIsMade(string key)
|
||||
{
|
||||
bool http = key.StartsWith("http.", StringComparison.OrdinalIgnoreCase);
|
||||
return (key.EndsWith(".proxy", StringComparison.OrdinalIgnoreCase) && (http || key.StartsWith("remote.", StringComparison.OrdinalIgnoreCase)))
|
||||
|| (http && key.EndsWith(".followredirects", StringComparison.OrdinalIgnoreCase));
|
||||
}
|
||||
|
||||
|
||||
|
||||
|
||||
@@ -124,6 +124,19 @@ public sealed class OrgSyncEngine
|
||||
{
|
||||
token.ThrowIfCancellationRequested();
|
||||
|
||||
///////////////////////////////////////////////////////////////////////////////
|
||||
// A repository named as git's own folder ('.git', 'git~1', in any spelling //
|
||||
// Windows opens as that folder) would be cloned where the target folder's //
|
||||
// own '.git' is, or would be. That row fails, on every platform; the rest of//
|
||||
// the run goes on (#184). //
|
||||
///////////////////////////////////////////////////////////////////////////////
|
||||
if(WorkingTree.IsGitDirectoryName(repo.Name))
|
||||
{
|
||||
throw new InvalidOperationException(
|
||||
$"Repository name '{repo.Name}' is the name git uses for its own folder, so there is no folder to clone it into. "
|
||||
+ "Rename the repository on GitHub.");
|
||||
}
|
||||
|
||||
// A name that is one safe segment (RepositoryPathResolver) can still be
|
||||
// impossible as a Windows folder: GitHub allows repositories called
|
||||
// 'aux' or 'con'. Fail the row with a plain reason instead of an opaque
|
||||
|
||||
@@ -77,8 +77,7 @@ public static class RepositoryPathResolver
|
||||
/// <summary>
|
||||
/// True when <paramref name="repoName"/> is a single, safe folder-name segment:
|
||||
/// not blank, not <c>.</c> or <c>..</c>, no directory separators (so never rooted
|
||||
/// or UNC), no drive or stream colon, no characters Windows rejects in a name, and
|
||||
/// not the name of git's own folder in any spelling Windows opens as that folder
|
||||
/// or UNC), no drive or stream colon, and no characters Windows rejects in a name
|
||||
/// (the same answer on every platform).
|
||||
/// </summary>
|
||||
/// <param name="repoName">The candidate name.</param>
|
||||
@@ -119,17 +118,6 @@ public static class RepositoryPathResolver
|
||||
return false;
|
||||
}
|
||||
|
||||
///////////////////////////////////////////////////////////////////////////////
|
||||
// git's own folder, as Windows would open it ('.git', '.GIT', 'git~1', with //
|
||||
// trailing dots or spaces): a repository of that name would be cloned into //
|
||||
// the place where the target folder's own '.git' is, or would be (#184). //
|
||||
///////////////////////////////////////////////////////////////////////////////
|
||||
if(WorkingTree.IsGitDirectoryName(repoName))
|
||||
{
|
||||
reason = "it is the name git uses for its own folder";
|
||||
return false;
|
||||
}
|
||||
|
||||
reason = null;
|
||||
return true;
|
||||
}
|
||||
|
||||
@@ -153,6 +153,11 @@ public sealed class WslCloner : IWslCloner
|
||||
{
|
||||
throw new WslCloneException($"Clone in WSL refused: '{name}' cannot be used as a folder name ({reason}).");
|
||||
}
|
||||
if(WorkingTree.IsGitDirectoryName(name))
|
||||
{
|
||||
// It would be cloned into, or as, the '.git' of a folder under ~/gclo (#184).
|
||||
throw new WslCloneException($"Clone in WSL refused: '{name}' is the name git uses for its own folder.");
|
||||
}
|
||||
}
|
||||
if(string.Equals(organization, TemporaryFolderName, StringComparison.OrdinalIgnoreCase))
|
||||
{
|
||||
|
||||
Reference in new issue
Block a user