From c4f2e7258a194d12b15c20fb27908923b5a3612c Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 10 Nov 2026 13:02:54 -0600 Subject: [PATCH 1/2] fix(recovery): keep every write and delete inside the working tree Path recovery materializes the working tree itself instead of using a git checkout, and it was less careful than one: the list of previously written files in .git\gclo-recovery.json decided what to delete without being checked, a path segment named .git was valid, writes followed links, and cleanup of a failed clone followed directory links. - New internal WorkingTree is the only way the recovery writes or deletes a file. The target must resolve inside the working directory, must not be in a .git folder (in any spelling Windows opens as that folder: letter case, the GIT~1 short name, trailing dots and spaces, a stream suffix), and must not be reached through a link. An existing file is removed and created anew, never written through. - The recovery file's stored paths are validated when it is loaded. A file that lists a path outside the working tree fails the row with a message that says how to rebuild it; nothing is deleted. - WindowsPathValidator treats .git and its aliases as an invalid segment, with a suggested replacement, so a tree entry or a rename can no longer land in git's own folder. - Cleanup of a failed clone skips links when clearing attributes. Threat model: T3 and T4 updated. Closes #141 Co-Authored-By: Claude Fable 5.2 Signed-off-by: James Maes --- docs/security/threat-model.md | 5 +- gclo.Engine.Tests/PathRecoveryTests.cs | 78 ++++ .../WindowsPathValidationTests.cs | 31 ++ gclo.Engine.Tests/WorkingTreeTests.cs | 369 ++++++++++++++++++ gclo.Engine/LibGit2GitClient.cs | 87 ++--- gclo.Engine/WindowsPathValidator.cs | 7 + gclo.Engine/WorkingTree.cs | 170 ++++++++ 7 files changed, 700 insertions(+), 47 deletions(-) create mode 100644 gclo.Engine.Tests/WorkingTreeTests.cs create mode 100644 gclo.Engine/WorkingTree.cs diff --git a/docs/security/threat-model.md b/docs/security/threat-model.md index c626c19..8dee00f 100644 --- a/docs/security/threat-model.md +++ b/docs/security/threat-model.md @@ -85,8 +85,8 @@ 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 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 | +| 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; 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) | 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..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 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 | @@ -117,3 +117,4 @@ flowchart LR | 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, 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) | +| 2026-11-10 | v1.1.1 | James Maes (@KofTwentyTwo) | T3/T4 updated: path recovery writes and deletes only through `WorkingTree` (inside the working directory, never in `.git`, never through a link); `.git` and its Windows aliases are invalid path segments; the stored recovery file's paths are validated on load; cleanup of a failed clone does not follow links (#141) | diff --git a/gclo.Engine.Tests/PathRecoveryTests.cs b/gclo.Engine.Tests/PathRecoveryTests.cs index 381d85c..630b401 100644 --- a/gclo.Engine.Tests/PathRecoveryTests.cs +++ b/gclo.Engine.Tests/PathRecoveryTests.cs @@ -4,6 +4,7 @@ */ using System.Text; +using System.Text.Json.Nodes; using LibGit2Sharp; using static gclo.Engine.Tests.GitTestHelpers; @@ -234,6 +235,83 @@ public sealed class PathRecoveryTests : IDisposable + // ---------------------------------------------------------------- inside the working tree only (#141) + + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task RecoveryFileListingAFileOutsideTheRepository_IsRefused_AndThatFileSurvives(bool relative) + { + string source = CreateForgedRepo(_root, ("bad:file.txt", "bad"), ("keep.txt", "keep")); + string target = await CloneExpectingInvalidPathsAsync(source, "outside-" + relative); + await _client.ApplyRecoveryAsync(target, Recovery(renames: [("bad:file.txt", "bad_file.txt")]), CancellationToken.None); + string victim = NewPath("victim-" + relative + ".txt"); + File.WriteAllText(victim, "untouched"); + // .git\gclo-recovery.json sits on disk between syncs; this is it after someone + // (or something) added a path the recovery never wrote. The list of materialized + // paths is what the next apply deletes from. + string recoveryFile = RecoveryFilePath(target); + JsonNode document = JsonNode.Parse(File.ReadAllText(recoveryFile))!; + document["MaterializedPaths"]!.AsArray().Add(relative ? "../" + Path.GetFileName(victim) : victim); + File.WriteAllText(recoveryFile, document.ToJsonString()); + ReplaceForgedEntry(source, remove: "keep.txt", add: ("other.txt", "o")); + + InvalidOperationException pull = await Assert.ThrowsAsync( + () => _client.PullAsync(target, Token, CancellationToken.None)); + InvalidOperationException apply = await Assert.ThrowsAsync( + () => _client.ApplyRecoveryAsync(target, Recovery(skips: ["bad:file.txt"]), CancellationToken.None)); + + Assert.Contains("lists a file that is not in this repository's working tree", pull.Message); + Assert.Contains("Delete it and use Resolve", pull.Message); + Assert.Contains("lists a file that is not in this repository's working tree", apply.Message); + Assert.Equal("untouched", File.ReadAllText(victim)); + // Nothing of the half-trusted file was acted on: the tree is as it was. + Assert.Equal("keep", File.ReadAllText(Path.Combine(target, "keep.txt"))); + Assert.Equal("bad", File.ReadAllText(Path.Combine(target, "bad_file.txt"))); + } + + + + [Theory] + [InlineData(".git")] + [InlineData(".GIT")] + [InlineData("git~1")] + public async Task ApplyRecovery_RenameThatWouldLandInGitsFolder_IsStillInvalid_AndWritesNothingThere(string gitFolderName) + { + string source = CreateForgedRepo(_root, ("bad:dir/poc.txt", "payload"), ("keep.txt", "keep")); + string target = await CloneExpectingInvalidPathsAsync(source, "into-git-" + gitFolderName.Replace('~', '-').Trim('.')); + + InvalidRepositoryPathsException ex = await Assert.ThrowsAsync( + () => _client.ApplyRecoveryAsync(target, Recovery(renames: [("bad:dir", gitFolderName)]), CancellationToken.None)); + + InvalidPathInfo info = Assert.Single(ex.Paths); + Assert.Equal(gitFolderName, info.RepoPath); + Assert.Contains("reserved for the repository's own data", info.Reason); + Assert.False(File.Exists(Path.Combine(target, ".git", "poc.txt"))); + Assert.False(File.Exists(Path.Combine(target, "keep.txt"))); // validated before any write + } + + + + [Fact] + public async Task ApplyRecovery_FolderInTheTreeIsALinkToElsewhere_IsRefused_AndNothingIsWrittenThere() + { + string source = CreateForgedRepo(_root, ("bad:file.txt", "bad"), ("vendor/lib.txt", "payload")); + string target = await CloneExpectingInvalidPathsAsync(source, "link-in-tree"); + string elsewhere = NewPath("elsewhere"); + Directory.CreateDirectory(elsewhere); + // The folder the tree is about to be written into already exists, as a link. + WorkingTreeTests.LinkDirectory(Path.Combine(target, "vendor"), elsewhere); + + InvalidOperationException ex = await Assert.ThrowsAsync( + () => _client.ApplyRecoveryAsync(target, Recovery(renames: [("bad:file.txt", "bad_file.txt")]), CancellationToken.None)); + + Assert.Contains("is a link to another location", ex.Message); + Assert.Empty(Directory.EnumerateFileSystemEntries(elsewhere)); + } + + + // ---------------------------------------------------------------- persistence hardening (#31) [Fact] diff --git a/gclo.Engine.Tests/WindowsPathValidationTests.cs b/gclo.Engine.Tests/WindowsPathValidationTests.cs index f2584fa..99fec21 100644 --- a/gclo.Engine.Tests/WindowsPathValidationTests.cs +++ b/gclo.Engine.Tests/WindowsPathValidationTests.cs @@ -60,6 +60,37 @@ public sealed class WindowsPathValidationTests : IDisposable + [Theory] + [InlineData(".git", "_git")] + [InlineData(".GIT", "_GIT")] + [InlineData(".Git", "_Git")] + [InlineData("git~1", "git_1")] + [InlineData("GIT~12", "GIT_12")] + public void ValidatePaths_GitsOwnFolderName_IsInvalid_AsAnySegment(string segment, string expectedSuggestion) + { + // git refuses such a tree entry at checkout. The recovery materializes the tree + // itself, and a rename can produce the name too, so the validator refuses it (#141). + foreach(string path in new[] { segment, $"{segment}/config", $"vendor/lib/{segment}/hooks/pre-commit" }) + { + InvalidPathInfo info = Assert.Single(WindowsPathValidator.ValidatePaths([path])); + + Assert.EndsWith(segment, info.RepoPath, StringComparison.Ordinal); + Assert.Contains("reserved for the repository's own data", info.Reason); + Assert.Equal(expectedSuggestion, info.SuggestedName); + } + } + + + + [Fact] + public void ValidatePaths_NamesThatOnlyStartLikeGitsFolder_AreFine() + { + Assert.Empty(WindowsPathValidator.ValidatePaths( + [".github/workflows/ci.yml", ".gitignore", ".gitattributes", ".gitmodules", "git/readme.md", "git~x/readme.md", "docs/.git-notes.md"])); + } + + + [Fact] public void Validate_FlagsCaseOnlyCollision() { diff --git a/gclo.Engine.Tests/WorkingTreeTests.cs b/gclo.Engine.Tests/WorkingTreeTests.cs new file mode 100644 index 0000000..7362d3c --- /dev/null +++ b/gclo.Engine.Tests/WorkingTreeTests.cs @@ -0,0 +1,369 @@ +/* + * Copyright (c) 2026 James Maes (KofTwentyTwo) + * SPDX-License-Identifier: MIT + */ + +using System.Diagnostics; +using static gclo.Engine.Tests.GitTestHelpers; + + +namespace gclo.Engine.Tests; + + +/// +/// keeps path recovery's writes and deletes inside the +/// working directory, out of .git, and off links (#141). +/// +public sealed class WorkingTreeTests : IDisposable +{ + private readonly string _root = + Path.Combine(Path.GetTempPath(), "gclo-tests", Guid.NewGuid().ToString("N")); + + private readonly string _workdir; + + private readonly string _outside; + + + + public WorkingTreeTests() + { + _workdir = Path.Combine(_root, "work"); + _outside = Path.Combine(_root, "outside"); + Directory.CreateDirectory(_workdir); + Directory.CreateDirectory(_outside); + } + + + + public void Dispose() => TryDeleteDirectory(_root); + + + + // ---------------------------------------------------------------- resolve + + [Theory] + [InlineData("readme.txt")] + [InlineData("src/app/main.cs")] + [InlineData(".github/workflows/ci.yml")] + [InlineData(".gitignore")] + [InlineData(".gitattributes")] + [InlineData("docs/git~notes.md")] + [InlineData("git~x/file.txt")] + [InlineData("git~/file.txt")] + public void Resolve_PathInsideTheWorkingTree_IsItsFullPath(string repoPath) + { + string expected = Path.Combine(_workdir, repoPath.Replace('/', Path.DirectorySeparatorChar)); + + Assert.Equal(expected, WorkingTree.Resolve(_workdir, repoPath)); + // A trailing separator on the working directory changes nothing. + Assert.Equal(expected, WorkingTree.Resolve(_workdir + Path.DirectorySeparatorChar, repoPath)); + } + + + + [Theory] + [InlineData("../outside/victim.txt")] + [InlineData("src/../../outside/victim.txt")] + [InlineData("..")] + [InlineData(".")] + [InlineData("src/..")] + public void Resolve_PathThatLeavesTheWorkingTree_IsRefused(string repoPath) + { + InvalidOperationException ex = Assert.Throws(() => WorkingTree.Resolve(_workdir, repoPath)); + + Assert.Contains("not a path inside the repository's working directory", ex.Message); + } + + + + [Fact] + public void Resolve_RootedPath_IsRefused_EvenInsideTheWorkingTreesParent() + { + // Path.Combine returns a rooted second argument unchanged: the classic way a + // stored path escapes its base. + string victim = Path.Combine(_outside, "victim.txt"); + + Assert.Throws(() => WorkingTree.Resolve(_workdir, victim)); + Assert.Throws(() => WorkingTree.Resolve(_workdir, victim.Replace('\\', '/'))); + // A sibling whose name merely starts with the working directory's name. + Assert.Throws(() => WorkingTree.Resolve(_workdir, "../work-other/file.txt")); + } + + + + [Theory] + [InlineData(".git/config")] + [InlineData(".GIT/config")] + [InlineData(".Git/hooks/pre-commit")] + [InlineData("sub/module/.git/config")] + [InlineData(".git")] + [InlineData("git~1/config")] + [InlineData("GIT~1/config")] + [InlineData("sub/git~12/config")] + [InlineData("src/../.git/config")] + public void Resolve_PathInsideAGitFolder_AtAnyDepthOrSpelling_IsRefused(string repoPath) + { + InvalidOperationException ex = Assert.Throws(() => WorkingTree.Resolve(_workdir, repoPath)); + + Assert.Contains("'.git' folder", ex.Message); + } + + + + [Theory] + [InlineData(".git./config")] + [InlineData(".git /config")] + [InlineData(".git.. ./config")] + [InlineData("GIT~1./config")] + [InlineData("sub/.GIT. /hooks/pre-commit")] + public void Resolve_TrailingDotsAndSpaces_CannotDisguiseTheGitFolder(string repoPath) + { + // Windows drops trailing dots and spaces when it opens a path, so these all open + // the real .git folder there. + InvalidOperationException ex = Assert.Throws(() => WorkingTree.Resolve(_workdir, repoPath)); + + Assert.Contains("'.git' folder", ex.Message); + } + + + + [Theory] + [InlineData("", "file.txt")] + [InlineData(" ", "file.txt")] + public void Resolve_BlankWorkingDirectory_IsACallerError(string workdir, string repoPath) + { + Assert.Throws(() => WorkingTree.Resolve(workdir, repoPath)); + } + + + + [Theory] + [InlineData("")] + [InlineData(" ")] + public void Resolve_BlankPath_IsACallerError(string repoPath) + { + Assert.Throws(() => WorkingTree.Resolve(_workdir, repoPath)); + } + + + + [Theory] + [InlineData(".git", true)] + [InlineData(".GIT", true)] + [InlineData("git~1", true)] + [InlineData("GIT~27", true)] + [InlineData(".git.", true)] + [InlineData(".git ", true)] + [InlineData(".GIT. .", true)] + [InlineData("git~1.", true)] + [InlineData(".git::$INDEX_ALLOCATION", true)] + [InlineData(".git:stream", true)] + [InlineData("git~1::$INDEX_ALLOCATION", true)] + [InlineData(".gitx:stream", false)] + [InlineData(".", false)] + [InlineData(" ", false)] + [InlineData("git~", false)] + [InlineData("git~1a", false)] + [InlineData(".github", false)] + [InlineData(".gitignore", false)] + [InlineData("git", false)] + [InlineData("", false)] + public void IsGitDirectoryName_TheNameAndItsShortNameAlias_Only(string segment, bool expected) + { + Assert.Equal(expected, WorkingTree.IsGitDirectoryName(segment)); + } + + + + // ---------------------------------------------------------------- create + + [Fact] + public void Create_WritesANewFile_AndItsFolders() + { + using(FileStream file = WorkingTree.Create(_workdir, "a/b/c.txt")) + { + file.Write("hello"u8); + } + + Assert.Equal("hello", File.ReadAllText(Path.Combine(_workdir, "a", "b", "c.txt"))); + } + + + + [Fact] + public void Create_ReplacesAnExistingFile_ReadOnlyOrNot() + { + string path = Path.Combine(_workdir, "locked.txt"); + File.WriteAllText(path, "a much longer previous content"); + File.SetAttributes(path, FileAttributes.ReadOnly); + + using(FileStream file = WorkingTree.Create(_workdir, "locked.txt")) + { + file.Write("new"u8); + } + + Assert.Equal("new", File.ReadAllText(path)); + } + + + + [Fact] + public void Create_ThroughAFolderThatIsALink_IsRefused_AndNothingIsWrittenOutside() + { + LinkDirectory(Path.Combine(_workdir, "vendor"), _outside); + + InvalidOperationException ex = Assert.Throws( + () => WorkingTree.Create(_workdir, "vendor/planted.txt")); + + Assert.Contains("is a link to another location", ex.Message); + Assert.Empty(Directory.EnumerateFileSystemEntries(_outside)); + } + + + + [Fact] + public void Create_WhereAFileLinkSits_ReplacesTheLink_AndLeavesItsTargetAlone() + { + string victim = Path.Combine(_outside, "victim.txt"); + File.WriteAllText(victim, "untouched"); + string link = Path.Combine(_workdir, "report.txt"); + try + { + File.CreateSymbolicLink(link, victim); + } + catch(Exception ex) when(ex is IOException or UnauthorizedAccessException) + { + return; // creating a file symlink needs a privilege (or Developer Mode) on Windows + } + + using(FileStream file = WorkingTree.Create(_workdir, "report.txt")) + { + file.Write("payload"u8); + } + + Assert.Equal("untouched", File.ReadAllText(victim)); + Assert.Equal("payload", File.ReadAllText(link)); + Assert.Null(new FileInfo(link).LinkTarget); + } + + + + [Fact] + public void Create_OutsideOrInGit_IsRefused_BeforeAnythingIsTouched() + { + Directory.CreateDirectory(Path.Combine(_workdir, ".git")); + + Assert.Throws(() => WorkingTree.Create(_workdir, "../outside/planted.txt")); + Assert.Throws(() => WorkingTree.Create(_workdir, ".git/config")); + + Assert.Empty(Directory.EnumerateFileSystemEntries(_outside)); + Assert.Empty(Directory.EnumerateFileSystemEntries(Path.Combine(_workdir, ".git"))); + } + + + + // ---------------------------------------------------------------- delete + + [Fact] + public void Delete_RemovesTheFile_AndTheFoldersItLeavesEmpty_ButNotTheWorkingDirectory() + { + string nested = Path.Combine(_workdir, "a", "b", "c.txt"); + Directory.CreateDirectory(Path.GetDirectoryName(nested)!); + File.WriteAllText(nested, "x"); + File.SetAttributes(nested, FileAttributes.ReadOnly); + + WorkingTree.Delete(_workdir, "a/b/c.txt"); + + Assert.False(Directory.Exists(Path.Combine(_workdir, "a"))); + Assert.True(Directory.Exists(_workdir)); + } + + + + [Fact] + public void Delete_StopsPruningAtAFolderThatStillHoldsSomething() + { + Directory.CreateDirectory(Path.Combine(_workdir, "a", "b")); + File.WriteAllText(Path.Combine(_workdir, "a", "b", "c.txt"), "x"); + File.WriteAllText(Path.Combine(_workdir, "a", "keep.txt"), "keep"); + + WorkingTree.Delete(_workdir, "a/b/c.txt"); + + Assert.False(Directory.Exists(Path.Combine(_workdir, "a", "b"))); + Assert.True(File.Exists(Path.Combine(_workdir, "a", "keep.txt"))); + } + + + + [Fact] + public void Delete_MissingFile_IsFine() + { + WorkingTree.Delete(_workdir, "never/was/here.txt"); + WorkingTree.Delete(_workdir, "top-level.txt"); + + Assert.Empty(Directory.EnumerateFileSystemEntries(_workdir)); + } + + + + [Fact] + public void Delete_OutsideTheWorkingTree_IsRefused_AndTheFileSurvives() + { + string victim = Path.Combine(_outside, "victim.txt"); + File.WriteAllText(victim, "untouched"); + + Assert.Throws(() => WorkingTree.Delete(_workdir, victim)); + Assert.Throws(() => WorkingTree.Delete(_workdir, "../outside/victim.txt")); + + Assert.Equal("untouched", File.ReadAllText(victim)); + } + + + + [Fact] + public void Delete_ThroughAFolderThatIsALink_IsRefused_AndTheFileSurvives() + { + string victim = Path.Combine(_outside, "victim.txt"); + File.WriteAllText(victim, "untouched"); + LinkDirectory(Path.Combine(_workdir, "vendor"), _outside); + + Assert.Throws(() => WorkingTree.Delete(_workdir, "vendor/victim.txt")); + + Assert.Equal("untouched", File.ReadAllText(victim)); + } + + + + // ---------------------------------------------------------------- helpers + + /// + /// Makes a directory link to : a + /// junction on Windows (which, unlike a symbolic link, needs no privilege), a + /// symbolic link elsewhere. + /// + internal static void LinkDirectory(string link, string target) + { + if(!OperatingSystem.IsWindows()) + { + Directory.CreateSymbolicLink(link, target); + return; + } + + var startInfo = new ProcessStartInfo("cmd.exe") + { + UseShellExecute = false, + CreateNoWindow = true, + RedirectStandardOutput = true, + RedirectStandardError = true, + }; + startInfo.ArgumentList.Add("/c"); + startInfo.ArgumentList.Add("mklink"); + startInfo.ArgumentList.Add("/J"); + startInfo.ArgumentList.Add(link); + startInfo.ArgumentList.Add(target); + using Process process = Process.Start(startInfo)!; + process.WaitForExit(); + Assert.True(process.ExitCode == 0, "mklink /J failed: " + process.StandardError.ReadToEnd()); + Assert.True(new DirectoryInfo(link).Attributes.HasFlag(FileAttributes.ReparsePoint)); + } +} diff --git a/gclo.Engine/LibGit2GitClient.cs b/gclo.Engine/LibGit2GitClient.cs index 851b1b8..5087cbc 100644 --- a/gclo.Engine/LibGit2GitClient.cs +++ b/gclo.Engine/LibGit2GitClient.cs @@ -269,7 +269,7 @@ public sealed class LibGit2GitClient : IGitClient // tip through the stored mapping. Freshly-invalid paths the mapping does // not cover surface as a typed failure listing the effective paths. AdvanceHeadToTrackedTip(repo); - RecoveryDocument stored = LoadRecoveryDocument(recoveryFile); + RecoveryDocument stored = LoadRecoveryDocument(recoveryFile, repo.Info.WorkingDirectory); if(stored.MaterializedTip is not null && string.Equals(stored.MaterializedTip, repo.Head.Tip?.Sha, StringComparison.Ordinal)) { // The tree on disk already reflects this tip: nothing to rewrite. A @@ -677,7 +677,7 @@ public sealed class LibGit2GitClient : IGitClient } string recoveryFile = GetRecoveryFilePath(repo); - RecoveryDocument? previous = File.Exists(recoveryFile) ? LoadRecoveryDocument(recoveryFile) : null; + RecoveryDocument? previous = File.Exists(recoveryFile) ? LoadRecoveryDocument(recoveryFile, repo.Info.WorkingDirectory) : null; if(previous is not null) { recovery = MergeRecoveries(stored: ToRecovery(previous), incoming: recovery); @@ -707,7 +707,7 @@ public sealed class LibGit2GitClient : IGitClient { if(!effectiveNow.Contains(stale)) { - DeleteMaterializedFile(workdir, stale); + WorkingTree.Delete(workdir, stale); } } @@ -715,18 +715,11 @@ public sealed class LibGit2GitClient : IGitClient { ct.ThrowIfCancellationRequested(); - string fullPath = Path.Combine(workdir, effectivePath.Replace('/', Path.DirectorySeparatorChar)); - Directory.CreateDirectory(Path.GetDirectoryName(fullPath)!); - if(File.Exists(fullPath)) - { - // Force semantics: overwrite whatever is there, read-only or not. - File.SetAttributes(fullPath, FileAttributes.Normal); - } - // Filter through .gitattributes (CRLF etc.) under the ORIGINAL repo path, - // exactly as a checkout of that entry would. + // exactly as a checkout of that entry would. Force semantics: whatever is + // at the destination is replaced, read-only or not (see WorkingTree). using Stream content = blob.GetContentStream(new FilteringOptions(originalPath)); - using var file = new FileStream(fullPath, FileMode.Create, FileAccess.Write); + using FileStream file = WorkingTree.Create(workdir, effectivePath); content.CopyTo(file); } @@ -736,34 +729,6 @@ public sealed class LibGit2GitClient : IGitClient - /// - /// Removes one file a previous materialization wrote (a user-deleted file is - /// fine) and prunes the directories it leaves empty, stopping at the working - /// directory root. - /// - private static void DeleteMaterializedFile(string workdir, string effectivePath) - { - string fullPath = Path.Combine(workdir, effectivePath.Replace('/', Path.DirectorySeparatorChar)); - if(File.Exists(fullPath)) - { - File.SetAttributes(fullPath, FileAttributes.Normal); - File.Delete(fullPath); - } - - string root = Path.TrimEndingDirectorySeparator(Path.GetFullPath(workdir)); - string? directory = Path.GetDirectoryName(fullPath); - while(directory is not null - && !string.Equals(Path.TrimEndingDirectorySeparator(directory), root, StringComparison.OrdinalIgnoreCase) - && Directory.Exists(directory) - && !Directory.EnumerateFileSystemEntries(directory).Any()) - { - Directory.Delete(directory); - directory = Path.GetDirectoryName(directory); - } - } - - - /// /// Overlays onto : renames /// union with the incoming value winning on a shared original path; skips union. @@ -947,11 +912,19 @@ public sealed class LibGit2GitClient : IGitClient - private static RecoveryDocument LoadRecoveryDocument(string filePath) + /// + /// Reads .git\gclo-recovery.json. The file sits on disk between syncs, where + /// anything can edit it, and its list of materialized paths decides what the next + /// apply deletes: every entry must be a path inside the working directory, or the + /// whole file is refused (#141). Renames and skips need no check here; they only + /// take effect through the effective paths, which are validated before any write. + /// + private static RecoveryDocument LoadRecoveryDocument(string filePath, string workdir) { + RecoveryDocument document; try { - return JsonSerializer.Deserialize(File.ReadAllText(filePath), s_recoveryJsonOptions) + document = JsonSerializer.Deserialize(File.ReadAllText(filePath), s_recoveryJsonOptions) ?? throw new JsonException("the document is empty"); } catch(JsonException ex) @@ -960,6 +933,22 @@ public sealed class LibGit2GitClient : IGitClient $"The path-recovery file '{filePath}' is corrupt ({ex.Message}). " + "Delete it and use Resolve on this repository again to rebuild it.", ex); } + + foreach(string path in document.MaterializedPaths ?? []) + { + try + { + WorkingTree.Resolve(workdir, path); + } + catch(Exception ex) when(ex is InvalidOperationException or ArgumentException) + { + throw new InvalidOperationException( + $"The path-recovery file '{filePath}' lists a file that is not in this repository's working tree ('{path}'). " + + "Delete it and use Resolve on this repository again to rebuild it.", ex); + } + } + + return document; } @@ -1019,8 +1008,16 @@ public sealed class LibGit2GitClient : IGitClient { return; } - // Pack files under .git are written read-only; clear attributes so delete succeeds. - foreach(string file in Directory.EnumerateFiles(path, "*", SearchOption.AllDirectories)) + // Pack files under .git are written read-only; clear attributes so delete + // succeeds. Links are skipped, so the walk never leaves the folder (a checked + // out symlink to a directory would otherwise lead it elsewhere); the delete + // itself removes a link, not what it points at (#141). + var insideOnly = new EnumerationOptions + { + RecurseSubdirectories = true, + AttributesToSkip = FileAttributes.ReparsePoint, + }; + foreach(string file in Directory.EnumerateFiles(path, "*", insideOnly)) { File.SetAttributes(file, FileAttributes.Normal); } diff --git a/gclo.Engine/WindowsPathValidator.cs b/gclo.Engine/WindowsPathValidator.cs index 9dd9ee8..60c8b3c 100644 --- a/gclo.Engine/WindowsPathValidator.cs +++ b/gclo.Engine/WindowsPathValidator.cs @@ -265,6 +265,13 @@ public static class WindowsPathValidator return ("contains a character that is invalid on Windows", sanitized); } + if(WorkingTree.IsGitDirectoryName(segment)) + { + // git refuses such an entry at checkout; the recovery's own materialization + // must too, or a tree entry (or a rename) could write into .git (#141). + return ("'.git' is reserved for the repository's own data", segment.Replace('.', '_').Replace('~', '_')); + } + string baseName = segment.Split('.')[0]; if(s_reservedNames.Contains(baseName)) { diff --git a/gclo.Engine/WorkingTree.cs b/gclo.Engine/WorkingTree.cs new file mode 100644 index 0000000..4208f50 --- /dev/null +++ b/gclo.Engine/WorkingTree.cs @@ -0,0 +1,170 @@ +/* + * Copyright (c) 2026 James Maes (KofTwentyTwo) + * SPDX-License-Identifier: MIT + */ + +namespace gclo.Engine; + + +/// +/// The only way path recovery writes or deletes a file. A git checkout refuses to +/// leave the working directory, to write into .git, or to follow a link; the +/// recovery materializes the tree itself, so it has to keep the same three promises. +/// Every path it handles comes from somewhere that is not trusted: the repository's +/// tree, a rename the user typed, or a file on disk that anything could have edited +/// (#141). +/// +internal static class WorkingTree +{ + /// + /// The full path of (forward-slash separated, relative) + /// under . Throws when the path would leave the working + /// directory, or names the repository's own .git folder at any depth, in any + /// of the spellings Windows opens as that folder (see ). + /// The check is made on the resolved path, so .. cannot hide either. + /// + public static string Resolve(string workdir, string repoPath) + { + ArgumentException.ThrowIfNullOrWhiteSpace(workdir); + ArgumentException.ThrowIfNullOrWhiteSpace(repoPath); + + string root = Path.TrimEndingDirectorySeparator(Path.GetFullPath(workdir)); + string full = Path.GetFullPath(Path.Combine(root, repoPath.Replace('/', Path.DirectorySeparatorChar))); + + if(!full.StartsWith(root + Path.DirectorySeparatorChar, PathComparison)) + { + throw new InvalidOperationException($"'{repoPath}' is not a path inside the repository's working directory."); + } + + foreach(string segment in full[(root.Length + 1)..].Split(Path.DirectorySeparatorChar)) + { + if(IsGitDirectoryName(segment)) + { + throw new InvalidOperationException($"'{repoPath}' is inside a '.git' folder, which belongs to git, not to the working tree."); + } + } + + return full; + } + + + + /// + /// True for every name Windows opens as git's own folder: .git in any letter + /// case; the 8.3 short name NTFS may give it (GIT~1, GIT~2, ...); + /// either with trailing dots or spaces, which Windows drops when it opens a path; + /// and either followed by a stream name (.git::$INDEX_ALLOCATION), which + /// names the same folder. The rule is the same on every platform, so a path's + /// answer does not depend on where it is asked. + /// + public static bool IsGitDirectoryName(string segment) + { + ArgumentNullException.ThrowIfNull(segment); + + int stream = segment.IndexOf(':', StringComparison.Ordinal); + ReadOnlySpan name = (stream < 0 ? segment : segment[..stream]).AsSpan().TrimEnd(". "); + if(name.Equals(".git", StringComparison.OrdinalIgnoreCase)) + { + return true; + } + + return name.Length > 4 + && name.StartsWith("git~", StringComparison.OrdinalIgnoreCase) + && name[4..].IndexOfAnyExceptInRange('0', '9') < 0; + } + + + + /// + /// Opens for writing as a new, empty file, creating its + /// folders. A file that is already there is removed first, read-only or not, so the + /// content never goes through a link someone left in its place. + /// + public static FileStream Create(string workdir, string repoPath) + { + string full = Resolve(workdir, repoPath); + RefuseLinkOnTheWay(workdir, full); + + Directory.CreateDirectory(Path.GetDirectoryName(full)!); + RemoveFile(full); + return new FileStream(full, FileMode.CreateNew, FileAccess.Write); + } + + + + /// + /// Deletes (a missing file is fine) and then the folders + /// it leaves empty, stopping at the working directory itself. + /// + public static void Delete(string workdir, string repoPath) + { + string full = Resolve(workdir, repoPath); + RefuseLinkOnTheWay(workdir, full); + RemoveFile(full); + + string root = Path.TrimEndingDirectorySeparator(Path.GetFullPath(workdir)); + string? directory = Path.GetDirectoryName(full); + while(directory is not null + && !string.Equals(Path.TrimEndingDirectorySeparator(directory), root, PathComparison) + && Directory.Exists(directory) + && !Directory.EnumerateFileSystemEntries(directory).Any()) + { + Directory.Delete(directory); + directory = Path.GetDirectoryName(directory); + } + } + + + + private static StringComparison PathComparison + => OperatingSystem.IsWindows() ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal; + + + + /// + /// A folder between the working directory and that is a + /// link (a symbolic link or a junction) would carry the write or delete somewhere + /// else. Nothing in a tree gclo materializes is ever a link, so one on the way is + /// refused. + /// + private static void RefuseLinkOnTheWay(string workdir, string full) + { + string root = Path.TrimEndingDirectorySeparator(Path.GetFullPath(workdir)); + string current = root; + string[] segments = full[(root.Length + 1)..].Split(Path.DirectorySeparatorChar); + for(int i = 0; i < segments.Length - 1; i++) + { + current = Path.Combine(current, segments[i]); + var folder = new DirectoryInfo(current); + if(folder.Exists && folder.Attributes.HasFlag(FileAttributes.ReparsePoint)) + { + throw new InvalidOperationException( + $"'{current}' is a link to another location, so gclo will not write or delete through it. " + + "Remove the link, then try again."); + } + } + } + + + + /// + /// Removes one file, read-only or not, when it is there; removes a link itself (even + /// one whose target is gone), never what it points at. + /// + private static void RemoveFile(string full) + { + var file = new FileInfo(full); + bool isLink = file.LinkTarget is not null; + if(!file.Exists && !isLink) + { + return; + } + + if(!isLink) + { + file.Attributes = FileAttributes.Normal; + } + + File.Delete(full); + } +} -- 2.54.0 From a900c5d9ddd0e073bd1f062aedbb568265377a1b Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 10 Nov 2026 13:32:32 -0600 Subject: [PATCH 2/2] fix(recovery): names Windows opens differently, real links only, safe cleanup Follow-up to the independent review of this branch. - A folder whose name ends in a space was checked under one name and opened under another, so a link behind it could still carry a delete out of the working directory. WorkingTree.Resolve now refuses any part that Windows does not open literally (empty, trailing dot or space, a colon), and the recovery file's stored paths must also pass the path validator. - A link is recognized by having a target, not by the reparse-point attribute: folders under a cloud-sync root carry that attribute without being links, and recovery refused to write into them. - Cleanup of a failed clone moved to WorkingTree.DeleteDirectory, which removes a link inside the folder as a link and never enters it; it is tested, where the previous change to that cleanup was not. - The destination's old content is removed whatever it is: a file, a hard link, a file link, a folder link. Only the name in the working tree goes. - The git-folder rule is the one git has: .git and GIT~1 (with trailing dots, spaces or a stream suffix). git~2 and up are ordinary names again. The rule now runs before the trailing-dot rule, whose suggestion for ".git." was ".git" itself; every suggestion re-validates clean. - Shared parent folders are checked for links once per materialization. - A bare repository with a recovery file gets a clear message. - Tests remove the junctions they create, and a hard-link test replaces reliance on the symlink test that needs a privilege. Refs #141 Co-Authored-By: Claude Fable 5.2 Signed-off-by: James Maes --- docs/security/threat-model.md | 4 +- gclo.Engine.Tests/PathRecoveryTests.cs | 71 ++++- .../WindowsPathValidationTests.cs | 18 +- gclo.Engine.Tests/WorkingTreeTests.cs | 252 +++++++++++++++--- gclo.Engine/LibGit2GitClient.cs | 55 ++-- gclo.Engine/WindowsPathValidator.cs | 17 +- gclo.Engine/WorkingTree.cs | 139 +++++++--- 7 files changed, 449 insertions(+), 107 deletions(-) diff --git a/docs/security/threat-model.md b/docs/security/threat-model.md index 8dee00f..b45f715 100644 --- a/docs/security/threat-model.md +++ b/docs/security/threat-model.md @@ -85,8 +85,8 @@ 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, which also refuses a `.git` segment in any spelling Windows opens as that folder; `RepositoryPathResolver` containment; 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) | 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..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 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; 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..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. 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 | diff --git a/gclo.Engine.Tests/PathRecoveryTests.cs b/gclo.Engine.Tests/PathRecoveryTests.cs index 630b401..4b41e48 100644 --- a/gclo.Engine.Tests/PathRecoveryTests.cs +++ b/gclo.Engine.Tests/PathRecoveryTests.cs @@ -238,21 +238,32 @@ public sealed class PathRecoveryTests : IDisposable // ---------------------------------------------------------------- inside the working tree only (#141) [Theory] - [InlineData(false)] - [InlineData(true)] - public async Task RecoveryFileListingAFileOutsideTheRepository_IsRefused_AndThatFileSurvives(bool relative) + [InlineData("absolute")] + [InlineData("relative")] + [InlineData("trailing-space-folder")] + [InlineData("empty")] + public async Task RecoveryFileListingAFileOutsideTheRepository_IsRefused_AndThatFileSurvives(string kind) { string source = CreateForgedRepo(_root, ("bad:file.txt", "bad"), ("keep.txt", "keep")); - string target = await CloneExpectingInvalidPathsAsync(source, "outside-" + relative); + string target = await CloneExpectingInvalidPathsAsync(source, "outside-" + kind); await _client.ApplyRecoveryAsync(target, Recovery(renames: [("bad:file.txt", "bad_file.txt")]), CancellationToken.None); - string victim = NewPath("victim-" + relative + ".txt"); + string victim = NewPath("victim-" + kind + ".txt"); File.WriteAllText(victim, "untouched"); + string listed = kind switch + { + "absolute" => victim, + "relative" => "../" + Path.GetFileName(victim), + // Inside the tree as written, but "docs " is a name a link can hide behind: + // whoever checks "docs" and whoever opens "docs " would not mean the same folder. + "trailing-space-folder" => "docs /victim.txt", + _ => "", + }; // .git\gclo-recovery.json sits on disk between syncs; this is it after someone // (or something) added a path the recovery never wrote. The list of materialized // paths is what the next apply deletes from. string recoveryFile = RecoveryFilePath(target); JsonNode document = JsonNode.Parse(File.ReadAllText(recoveryFile))!; - document["MaterializedPaths"]!.AsArray().Add(relative ? "../" + Path.GetFileName(victim) : victim); + document["MaterializedPaths"]!.AsArray().Add(listed); File.WriteAllText(recoveryFile, document.ToJsonString()); ReplaceForgedEntry(source, remove: "keep.txt", add: ("other.txt", "o")); @@ -286,7 +297,7 @@ public sealed class PathRecoveryTests : IDisposable InvalidPathInfo info = Assert.Single(ex.Paths); Assert.Equal(gitFolderName, info.RepoPath); - Assert.Contains("reserved for the repository's own data", info.Reason); + Assert.Contains("git's own folder", info.Reason); Assert.False(File.Exists(Path.Combine(target, ".git", "poc.txt"))); Assert.False(File.Exists(Path.Combine(target, "keep.txt"))); // validated before any write } @@ -301,13 +312,49 @@ public sealed class PathRecoveryTests : IDisposable string elsewhere = NewPath("elsewhere"); Directory.CreateDirectory(elsewhere); // The folder the tree is about to be written into already exists, as a link. - WorkingTreeTests.LinkDirectory(Path.Combine(target, "vendor"), elsewhere); + string link = Path.Combine(target, "vendor"); + WorkingTreeTests.CreateDirectoryLink(link, elsewhere); + try + { + InvalidOperationException ex = await Assert.ThrowsAsync( + () => _client.ApplyRecoveryAsync(target, Recovery(renames: [("bad:file.txt", "bad_file.txt")]), CancellationToken.None)); - InvalidOperationException ex = await Assert.ThrowsAsync( - () => _client.ApplyRecoveryAsync(target, Recovery(renames: [("bad:file.txt", "bad_file.txt")]), CancellationToken.None)); + Assert.Contains("is a link to another location", ex.Message); + Assert.Empty(Directory.EnumerateFileSystemEntries(elsewhere)); + } + finally + { + WorkingTreeTests.RemoveLink(link); + } + } - Assert.Contains("is a link to another location", ex.Message); - Assert.Empty(Directory.EnumerateFileSystemEntries(elsewhere)); + + + [Fact] + public async Task FailedClone_CleanupNeverLeavesTheFolder_AndRemovesIt() + { + // A clone that fails after its fetch is removed again. What it leaves must be + // deleted without following a link out of the folder. + string source = CreateForgedRepo(_root, ("readme.txt", "hello")); + string target = NewPath("cleanup"); + string elsewhere = NewPath("cleanup-elsewhere"); + Directory.CreateDirectory(elsewhere); + string victim = Path.Combine(elsewhere, "victim.txt"); + File.WriteAllText(victim, "untouched"); + File.SetAttributes(victim, FileAttributes.ReadOnly); + _client.AfterFetchForTesting = () => + { + WorkingTreeTests.CreateDirectoryLink(Path.Combine(target, "linked"), elsewhere); + throw new InvalidOperationException("fails between fetch and checkout"); + }; + + await Assert.ThrowsAsync( + () => _client.CloneAsync(source, target, Token, null, CancellationToken.None)); + + Assert.False(Directory.Exists(target)); + Assert.Equal("untouched", File.ReadAllText(victim)); + Assert.True(File.GetAttributes(victim).HasFlag(FileAttributes.ReadOnly)); + File.SetAttributes(victim, FileAttributes.Normal); } diff --git a/gclo.Engine.Tests/WindowsPathValidationTests.cs b/gclo.Engine.Tests/WindowsPathValidationTests.cs index 99fec21..7e27944 100644 --- a/gclo.Engine.Tests/WindowsPathValidationTests.cs +++ b/gclo.Engine.Tests/WindowsPathValidationTests.cs @@ -65,7 +65,11 @@ public sealed class WindowsPathValidationTests : IDisposable [InlineData(".GIT", "_GIT")] [InlineData(".Git", "_Git")] [InlineData("git~1", "git_1")] - [InlineData("GIT~12", "GIT_12")] + [InlineData("GIT~1", "GIT_1")] + [InlineData(".git.", "_git")] + [InlineData(".git ", "_git")] + [InlineData("git~1.", "git_1")] + [InlineData(".git:stream", "_git_stream")] public void ValidatePaths_GitsOwnFolderName_IsInvalid_AsAnySegment(string segment, string expectedSuggestion) { // git refuses such a tree entry at checkout. The recovery materializes the tree @@ -75,18 +79,24 @@ public sealed class WindowsPathValidationTests : IDisposable InvalidPathInfo info = Assert.Single(WindowsPathValidator.ValidatePaths([path])); Assert.EndsWith(segment, info.RepoPath, StringComparison.Ordinal); - Assert.Contains("reserved for the repository's own data", info.Reason); + Assert.Contains("git's own folder", info.Reason); Assert.Equal(expectedSuggestion, info.SuggestedName); + // The suggestion is itself a valid name: accepting it settles the matter. + Assert.Empty(WindowsPathValidator.ValidatePaths([info.SuggestedName!])); } } [Fact] - public void ValidatePaths_NamesThatOnlyStartLikeGitsFolder_AreFine() + public void ValidatePaths_NamesThatOnlyResembleGitsFolder_AreFine() { + // git~2 and up are ordinary names: git accepts them and GitHub can host them. Assert.Empty(WindowsPathValidator.ValidatePaths( - [".github/workflows/ci.yml", ".gitignore", ".gitattributes", ".gitmodules", "git/readme.md", "git~x/readme.md", "docs/.git-notes.md"])); + [ + ".github/workflows/ci.yml", ".gitignore", ".gitattributes", ".gitmodules", "git/readme.md", + "git~2/readme.md", "GIT~10/readme.md", "git~x/readme.md", "docs/.git-notes.md", + ])); } diff --git a/gclo.Engine.Tests/WorkingTreeTests.cs b/gclo.Engine.Tests/WorkingTreeTests.cs index 7362d3c..fd141b7 100644 --- a/gclo.Engine.Tests/WorkingTreeTests.cs +++ b/gclo.Engine.Tests/WorkingTreeTests.cs @@ -23,6 +23,8 @@ public sealed class WorkingTreeTests : IDisposable private readonly string _outside; + private readonly List _links = []; + public WorkingTreeTests() @@ -35,7 +37,16 @@ public sealed class WorkingTreeTests : IDisposable - public void Dispose() => TryDeleteDirectory(_root); + public void Dispose() + { + // Links first, as links: a recursive delete that meets a junction stops there + // and leaves the folders above it behind. + foreach(string link in _links) + { + RemoveLink(link); + } + TryDeleteDirectory(_root); + } @@ -48,8 +59,10 @@ public sealed class WorkingTreeTests : IDisposable [InlineData(".gitignore")] [InlineData(".gitattributes")] [InlineData("docs/git~notes.md")] + [InlineData("git~2/file.txt")] + [InlineData("GIT~10/file.txt")] [InlineData("git~x/file.txt")] - [InlineData("git~/file.txt")] + [InlineData("name with spaces/and.dots.txt")] public void Resolve_PathInsideTheWorkingTree_IsItsFullPath(string repoPath) { string expected = Path.Combine(_workdir, repoPath.Replace('/', Path.DirectorySeparatorChar)); @@ -99,8 +112,13 @@ public sealed class WorkingTreeTests : IDisposable [InlineData(".git")] [InlineData("git~1/config")] [InlineData("GIT~1/config")] - [InlineData("sub/git~12/config")] + [InlineData("sub/Git~1/config")] [InlineData("src/../.git/config")] + [InlineData(".git./config")] + [InlineData(".git /config")] + [InlineData(".git.. ./config")] + [InlineData("GIT~1./config")] + [InlineData("sub/.GIT. /hooks/pre-commit")] public void Resolve_PathInsideAGitFolder_AtAnyDepthOrSpelling_IsRefused(string repoPath) { InvalidOperationException ex = Assert.Throws(() => WorkingTree.Resolve(_workdir, repoPath)); @@ -111,18 +129,18 @@ public sealed class WorkingTreeTests : IDisposable [Theory] - [InlineData(".git./config")] - [InlineData(".git /config")] - [InlineData(".git.. ./config")] - [InlineData("GIT~1./config")] - [InlineData("sub/.GIT. /hooks/pre-commit")] - public void Resolve_TrailingDotsAndSpaces_CannotDisguiseTheGitFolder(string repoPath) + [InlineData("docs /victim.txt")] + [InlineData("a/b. ./c.txt")] + [InlineData("docs/")] + [InlineData("notes.txt:stream")] + [InlineData("folder:stream/file.txt")] + public void Resolve_PartThatWindowsWouldOpenUnderAnotherName_IsRefused(string repoPath) { - // Windows drops trailing dots and spaces when it opens a path, so these all open - // the real .git folder there. + // "docs " and "docs" are two different folders to whoever checks the first and + // one folder to Windows when it opens the second: such a name is never trusted. InvalidOperationException ex = Assert.Throws(() => WorkingTree.Resolve(_workdir, repoPath)); - Assert.Contains("'.git' folder", ex.Message); + Assert.Contains("does not open under the name it is written with", ex.Message); } @@ -151,7 +169,7 @@ public sealed class WorkingTreeTests : IDisposable [InlineData(".git", true)] [InlineData(".GIT", true)] [InlineData("git~1", true)] - [InlineData("GIT~27", true)] + [InlineData("GIT~1", true)] [InlineData(".git.", true)] [InlineData(".git ", true)] [InlineData(".GIT. .", true)] @@ -159,16 +177,18 @@ public sealed class WorkingTreeTests : IDisposable [InlineData(".git::$INDEX_ALLOCATION", true)] [InlineData(".git:stream", true)] [InlineData("git~1::$INDEX_ALLOCATION", true)] - [InlineData(".gitx:stream", false)] - [InlineData(".", false)] - [InlineData(" ", false)] + [InlineData("git~2", false)] // a name git itself accepts, and GitHub can host + [InlineData("GIT~27", false)] [InlineData("git~", false)] [InlineData("git~1a", false)] + [InlineData(".gitx:stream", false)] [InlineData(".github", false)] [InlineData(".gitignore", false)] [InlineData("git", false)] + [InlineData(".", false)] + [InlineData(" ", false)] [InlineData("", false)] - public void IsGitDirectoryName_TheNameAndItsShortNameAlias_Only(string segment, bool expected) + public void IsGitDirectoryName_TheNamesGitItselfRefuses_Only(string segment, bool expected) { Assert.Equal(expected, WorkingTree.IsGitDirectoryName(segment)); } @@ -221,6 +241,49 @@ public sealed class WorkingTreeTests : IDisposable + [Fact] + public void Create_ALinkDeeperOnTheWay_IsRefusedToo_EvenWithFoldersAlreadyVerified() + { + var verified = new HashSet(StringComparer.Ordinal); + using(WorkingTree.Create(_workdir, "src/app/main.cs", verified)) + { + } + Assert.Contains(Path.Combine(_workdir, "src"), verified); + Assert.Contains(Path.Combine(_workdir, "src", "app"), verified); + LinkDirectory(Path.Combine(_workdir, "src", "app", "vendor"), _outside); + + // The shared parents are remembered; the new folder is still checked. + using(WorkingTree.Create(_workdir, "src/app/util.cs", verified)) + { + } + Assert.Throws(() => WorkingTree.Create(_workdir, "src/app/vendor/planted.txt", verified)); + + Assert.Empty(Directory.EnumerateFileSystemEntries(_outside)); + } + + + + [Fact] + public void Create_WhereAHardLinkSits_ReplacesIt_AndLeavesTheOtherNameOfThatFileAlone() + { + // A hard link needs no privilege, and writing "in place" would change the file + // under its other name too. + string victim = Path.Combine(_outside, "victim.txt"); + File.WriteAllText(victim, "untouched"); + string link = Path.Combine(_workdir, "report.txt"); + HardLink(link, victim); + + using(FileStream file = WorkingTree.Create(_workdir, "report.txt")) + { + file.Write("payload"u8); + } + + Assert.Equal("untouched", File.ReadAllText(victim)); + Assert.Equal("payload", File.ReadAllText(link)); + } + + + [Fact] public void Create_WhereAFileLinkSits_ReplacesTheLink_AndLeavesItsTargetAlone() { @@ -233,7 +296,7 @@ public sealed class WorkingTreeTests : IDisposable } catch(Exception ex) when(ex is IOException or UnauthorizedAccessException) { - return; // creating a file symlink needs a privilege (or Developer Mode) on Windows + return; // a file symlink needs a privilege (or Developer Mode) on Windows; the hard-link test above always runs } using(FileStream file = WorkingTree.Create(_workdir, "report.txt")) @@ -248,6 +311,24 @@ public sealed class WorkingTreeTests : IDisposable + [Fact] + public void Create_WhereAFolderLinkSits_ReplacesTheLinkWithTheFile_AndLeavesItsTargetAlone() + { + File.WriteAllText(Path.Combine(_outside, "kept.txt"), "kept"); + string link = Path.Combine(_workdir, "report.txt"); + LinkDirectory(link, _outside); + + using(FileStream file = WorkingTree.Create(_workdir, "report.txt")) + { + file.Write("payload"u8); + } + + Assert.Equal("payload", File.ReadAllText(link)); + Assert.Equal("kept", File.ReadAllText(Path.Combine(_outside, "kept.txt"))); + } + + + [Fact] public void Create_OutsideOrInGit_IsRefused_BeforeAnythingIsTouched() { @@ -255,9 +336,11 @@ public sealed class WorkingTreeTests : IDisposable Assert.Throws(() => WorkingTree.Create(_workdir, "../outside/planted.txt")); Assert.Throws(() => WorkingTree.Create(_workdir, ".git/config")); + Assert.Throws(() => WorkingTree.Create(_workdir, "docs /planted.txt")); Assert.Empty(Directory.EnumerateFileSystemEntries(_outside)); Assert.Empty(Directory.EnumerateFileSystemEntries(Path.Combine(_workdir, ".git"))); + Assert.Single(Directory.EnumerateFileSystemEntries(_workdir)); // only .git } @@ -334,36 +417,145 @@ public sealed class WorkingTreeTests : IDisposable + // ---------------------------------------------------------------- delete a directory + + [Fact] + public void DeleteDirectory_RemovesEverything_ReadOnlyFilesAndFoldersIncluded() + { + string folder = Path.Combine(_workdir, "half-clone"); + string pack = Path.Combine(folder, ".git", "objects", "pack", "pack-1.idx"); + Directory.CreateDirectory(Path.GetDirectoryName(pack)!); + File.WriteAllText(pack, "x"); + File.SetAttributes(pack, FileAttributes.ReadOnly | FileAttributes.Hidden); + File.WriteAllText(Path.Combine(folder, "readme.txt"), "x"); + new DirectoryInfo(Path.Combine(folder, ".git")).Attributes |= FileAttributes.Hidden | FileAttributes.ReadOnly; + + WorkingTree.DeleteDirectory(folder); + + Assert.False(Directory.Exists(folder)); + } + + + + [Fact] + public void DeleteDirectory_ALinkInside_IsRemovedAsALink_AndWhatItPointsAtIsNeitherEnteredNorChanged() + { + string victim = Path.Combine(_outside, "victim.txt"); + File.WriteAllText(victim, "untouched"); + File.SetAttributes(victim, FileAttributes.ReadOnly); + string folder = Path.Combine(_workdir, "half-clone"); + Directory.CreateDirectory(Path.Combine(folder, "src")); + LinkDirectory(Path.Combine(folder, "src", "vendor"), _outside); + + WorkingTree.DeleteDirectory(folder); + + Assert.False(Directory.Exists(folder)); + Assert.Equal("untouched", File.ReadAllText(victim)); + Assert.True(File.GetAttributes(victim).HasFlag(FileAttributes.ReadOnly), "the linked folder's files keep their attributes"); + File.SetAttributes(victim, FileAttributes.Normal); + } + + + + [Fact] + public void DeleteDirectory_OfALink_RemovesOnlyTheLink() + { + File.WriteAllText(Path.Combine(_outside, "kept.txt"), "kept"); + string link = Path.Combine(_workdir, "linked"); + LinkDirectory(link, _outside); + + WorkingTree.DeleteDirectory(link); + + Assert.False(Directory.Exists(link)); + Assert.Equal("kept", File.ReadAllText(Path.Combine(_outside, "kept.txt"))); + } + + + + [Fact] + public void DeleteDirectory_MissingFolder_IsFine_AndABlankPathIsACallerError() + { + WorkingTree.DeleteDirectory(Path.Combine(_workdir, "never-existed")); + + Assert.Throws(() => WorkingTree.DeleteDirectory(" ")); + } + + + // ---------------------------------------------------------------- helpers /// /// Makes a directory link to : a /// junction on Windows (which, unlike a symbolic link, needs no privilege), a - /// symbolic link elsewhere. + /// symbolic link elsewhere. The link is removed again when the test ends. /// - internal static void LinkDirectory(string link, string target) + private void LinkDirectory(string link, string target) { - if(!OperatingSystem.IsWindows()) + CreateDirectoryLink(link, target); + _links.Add(link); + } + + + + /// Creates a directory link without tracking it; the caller removes it with . + internal static void CreateDirectoryLink(string link, string target) + { + if(OperatingSystem.IsWindows()) + { + Run("cmd.exe", "/c", "mklink", "/J", link, target); + } + else { Directory.CreateSymbolicLink(link, target); - return; } + Assert.NotNull(new DirectoryInfo(link).LinkTarget); + } - var startInfo = new ProcessStartInfo("cmd.exe") + + + /// Removes a directory link itself (never its target); fine when it is already gone. + internal static void RemoveLink(string link) + { + var info = new DirectoryInfo(link); + if(info.LinkTarget is not null) + { + info.Delete(); + } + } + + + + private static void HardLink(string link, string target) + { + if(OperatingSystem.IsWindows()) + { + Run("cmd.exe", "/c", "mklink", "/H", link, target); + } + else + { + Run("ln", target, link); + } + } + + + + private static void Run(string fileName, params string[] arguments) + { + var startInfo = new ProcessStartInfo(fileName) { UseShellExecute = false, CreateNoWindow = true, RedirectStandardOutput = true, RedirectStandardError = true, }; - startInfo.ArgumentList.Add("/c"); - startInfo.ArgumentList.Add("mklink"); - startInfo.ArgumentList.Add("/J"); - startInfo.ArgumentList.Add(link); - startInfo.ArgumentList.Add(target); + foreach(string argument in arguments) + { + startInfo.ArgumentList.Add(argument); + } using Process process = Process.Start(startInfo)!; + string error = process.StandardError.ReadToEnd(); + process.StandardOutput.ReadToEnd(); process.WaitForExit(); - Assert.True(process.ExitCode == 0, "mklink /J failed: " + process.StandardError.ReadToEnd()); - Assert.True(new DirectoryInfo(link).Attributes.HasFlag(FileAttributes.ReparsePoint)); + Assert.True(process.ExitCode == 0, $"{fileName} {string.Join(' ', arguments)} failed: {error}"); } } diff --git a/gclo.Engine/LibGit2GitClient.cs b/gclo.Engine/LibGit2GitClient.cs index 5087cbc..da5e047 100644 --- a/gclo.Engine/LibGit2GitClient.cs +++ b/gclo.Engine/LibGit2GitClient.cs @@ -269,7 +269,7 @@ public sealed class LibGit2GitClient : IGitClient // tip through the stored mapping. Freshly-invalid paths the mapping does // not cover surface as a typed failure listing the effective paths. AdvanceHeadToTrackedTip(repo); - RecoveryDocument stored = LoadRecoveryDocument(recoveryFile, repo.Info.WorkingDirectory); + RecoveryDocument stored = LoadRecoveryDocument(recoveryFile, WorkingDirectoryOf(repo)); if(stored.MaterializedTip is not null && string.Equals(stored.MaterializedTip, repo.Head.Tip?.Sha, StringComparison.Ordinal)) { // The tree on disk already reflects this tip: nothing to rewrite. A @@ -676,8 +676,9 @@ public sealed class LibGit2GitClient : IGitClient repo.Config.Set("core.longpaths", true, ConfigurationLevel.Local); } + string workdir = WorkingDirectoryOf(repo); string recoveryFile = GetRecoveryFilePath(repo); - RecoveryDocument? previous = File.Exists(recoveryFile) ? LoadRecoveryDocument(recoveryFile, repo.Info.WorkingDirectory) : null; + RecoveryDocument? previous = File.Exists(recoveryFile) ? LoadRecoveryDocument(recoveryFile, workdir) : null; if(previous is not null) { recovery = MergeRecoveries(stored: ToRecovery(previous), incoming: recovery); @@ -697,7 +698,6 @@ public sealed class LibGit2GitClient : IGitClient throw new InvalidRepositoryPathsException(stillInvalid); } - string workdir = repo.Info.WorkingDirectory; // A real checkout removes what left the tree; this materialization must too, // or a recovery-managed working tree accumulates every file upstream ever @@ -711,6 +711,8 @@ public sealed class LibGit2GitClient : IGitClient } } + // Parent folders are shared by many files; each is checked for a link once. + var verifiedFolders = new HashSet(StringComparer.Ordinal); foreach((string? originalPath, string? effectivePath, Blob? blob) in entries) { ct.ThrowIfCancellationRequested(); @@ -719,7 +721,7 @@ public sealed class LibGit2GitClient : IGitClient // exactly as a checkout of that entry would. Force semantics: whatever is // at the destination is replaced, read-only or not (see WorkingTree). using Stream content = blob.GetContentStream(new FilteringOptions(originalPath)); - using FileStream file = WorkingTree.Create(workdir, effectivePath); + using FileStream file = WorkingTree.Create(workdir, effectivePath, verifiedFolders); content.CopyTo(file); } @@ -938,12 +940,18 @@ public sealed class LibGit2GitClient : IGitClient { try { + // Every path gclo ever recorded here had passed the validator and names a + // file in the tree; one that does not was not written by gclo. + if(string.IsNullOrEmpty(path) || WindowsPathValidator.ValidatePaths([path]).Count > 0) + { + throw new InvalidOperationException("not a path gclo could have written"); + } WorkingTree.Resolve(workdir, path); } - catch(Exception ex) when(ex is InvalidOperationException or ArgumentException) + catch(Exception ex) when(ex is InvalidOperationException or ArgumentException or IOException or NotSupportedException) { throw new InvalidOperationException( - $"The path-recovery file '{filePath}' lists a file that is not in this repository's working tree ('{path}'). " + $"The path-recovery file '{filePath}' lists a file that is not in this repository's working tree ('{DisplayPath(path)}'). " + "Delete it and use Resolve on this repository again to rebuild it.", ex); } } @@ -953,6 +961,23 @@ public sealed class LibGit2GitClient : IGitClient + /// A path from an untrusted file, cut to a length that fits a message. + private static string DisplayPath(string? path) + => path is null ? "" : path.Length <= 200 ? path : path[..200] + "..."; + + + + /// + /// The working directory path recovery writes into. A bare repository has none, + /// and nothing gclo clones is bare: one that carries a recovery file was put + /// together by hand. + /// + private static string WorkingDirectoryOf(Repository repo) + => repo.Info.WorkingDirectory + ?? throw new InvalidOperationException("This is a bare repository; it has no working tree to materialize."); + + + private static PathRecovery ToRecovery(RecoveryDocument document) => new( new Dictionary(document.SegmentRenames ?? new(StringComparer.Ordinal), StringComparer.Ordinal), @@ -1008,20 +1033,10 @@ public sealed class LibGit2GitClient : IGitClient { return; } - // Pack files under .git are written read-only; clear attributes so delete - // succeeds. Links are skipped, so the walk never leaves the folder (a checked - // out symlink to a directory would otherwise lead it elsewhere); the delete - // itself removes a link, not what it points at (#141). - var insideOnly = new EnumerationOptions - { - RecurseSubdirectories = true, - AttributesToSkip = FileAttributes.ReparsePoint, - }; - foreach(string file in Directory.EnumerateFiles(path, "*", insideOnly)) - { - File.SetAttributes(file, FileAttributes.Normal); - } - Directory.Delete(path, recursive: true); + // Read-only pack files and all; never through a link inside the folder (a + // checked-out symlink to a directory would otherwise lead the delete + // elsewhere), see WorkingTree (#141). + WorkingTree.DeleteDirectory(path); } catch { diff --git a/gclo.Engine/WindowsPathValidator.cs b/gclo.Engine/WindowsPathValidator.cs index 60c8b3c..a42e385 100644 --- a/gclo.Engine/WindowsPathValidator.cs +++ b/gclo.Engine/WindowsPathValidator.cs @@ -254,6 +254,16 @@ public static class WindowsPathValidator { return ($"segment longer than {MaxSegmentLength} characters", segment[..MaxSegmentLength]); } + if(WorkingTree.IsGitDirectoryName(segment)) + { + // git refuses such an entry at checkout; the recovery's own materialization + // must too, or a tree entry (or a rename) could write into .git (#141). + // Checked before the trailing-dot rule, whose suggestion for ".git." would + // be ".git" itself. + string plain = new string(segment.TrimEnd(' ', '.').Select(c => s_invalidChars.Contains(c) ? '_' : c).ToArray()); + return ("this name is git's own folder ('.git', or its short name 'GIT~1')", plain.Replace('.', '_').Replace('~', '_')); + } + if(segment.EndsWith(' ') || segment.EndsWith('.')) { string trimmed = segment.TrimEnd(' ', '.'); @@ -265,13 +275,6 @@ public static class WindowsPathValidator return ("contains a character that is invalid on Windows", sanitized); } - if(WorkingTree.IsGitDirectoryName(segment)) - { - // git refuses such an entry at checkout; the recovery's own materialization - // must too, or a tree entry (or a rename) could write into .git (#141). - return ("'.git' is reserved for the repository's own data", segment.Replace('.', '_').Replace('~', '_')); - } - string baseName = segment.Split('.')[0]; if(s_reservedNames.Contains(baseName)) { diff --git a/gclo.Engine/WorkingTree.cs b/gclo.Engine/WorkingTree.cs index 4208f50..f7b991c 100644 --- a/gclo.Engine/WorkingTree.cs +++ b/gclo.Engine/WorkingTree.cs @@ -14,21 +14,28 @@ namespace gclo.Engine; /// tree, a rename the user typed, or a file on disk that anything could have edited /// (#141). /// +/// +/// The link check and the write that follows it are two steps. Something that swaps +/// a folder for a link between them, on the user's own machine and with the user's +/// own rights, is outside what this guards against. +/// internal static class WorkingTree { /// /// The full path of (forward-slash separated, relative) - /// under . Throws when the path would leave the working - /// directory, or names the repository's own .git folder at any depth, in any - /// of the spellings Windows opens as that folder (see ). - /// The check is made on the resolved path, so .. cannot hide either. + /// under . Throws unless the path, once resolved, is + /// strictly inside the working directory and every segment of it is a name that + /// Windows opens literally: not empty, no trailing dot or space (Windows drops + /// those, so docs and docs would be two spellings of one check and + /// two different folders on disk), no colon (a stream of another file), and not + /// git's own folder (see ). /// public static string Resolve(string workdir, string repoPath) { ArgumentException.ThrowIfNullOrWhiteSpace(workdir); ArgumentException.ThrowIfNullOrWhiteSpace(repoPath); - string root = Path.TrimEndingDirectorySeparator(Path.GetFullPath(workdir)); + string root = Root(workdir); string full = Path.GetFullPath(Path.Combine(root, repoPath.Replace('/', Path.DirectorySeparatorChar))); if(!full.StartsWith(root + Path.DirectorySeparatorChar, PathComparison)) @@ -42,6 +49,13 @@ internal static class WorkingTree { throw new InvalidOperationException($"'{repoPath}' is inside a '.git' folder, which belongs to git, not to the working tree."); } + + if(segment.Length == 0 || segment[^1] is '.' or ' ' || segment.Contains(':', StringComparison.Ordinal)) + { + throw new InvalidOperationException( + $"'{repoPath}' has a part that Windows does not open under the name it is written with " + + "(empty, ending in a dot or a space, or containing a colon)."); + } } return full; @@ -50,12 +64,14 @@ internal static class WorkingTree /// - /// True for every name Windows opens as git's own folder: .git in any letter - /// case; the 8.3 short name NTFS may give it (GIT~1, GIT~2, ...); - /// either with trailing dots or spaces, which Windows drops when it opens a path; - /// and either followed by a stream name (.git::$INDEX_ALLOCATION), which + /// True for the names git itself refuses in a tree because Windows opens them as + /// the repository's own folder: .git in any letter case, and GIT~1, + /// the short name NTFS gives it; either with trailing dots or spaces, which Windows + /// drops; either followed by a stream name (.git::$INDEX_ALLOCATION), which /// names the same folder. The rule is the same on every platform, so a path's - /// answer does not depend on where it is asked. + /// answer does not depend on where it is asked. (Should NTFS have given + /// .git another short name, still refuses it: + /// resolving a path expands short names.) /// public static bool IsGitDirectoryName(string segment) { @@ -63,27 +79,27 @@ internal static class WorkingTree int stream = segment.IndexOf(':', StringComparison.Ordinal); ReadOnlySpan name = (stream < 0 ? segment : segment[..stream]).AsSpan().TrimEnd(". "); - if(name.Equals(".git", StringComparison.OrdinalIgnoreCase)) - { - return true; - } - - return name.Length > 4 - && name.StartsWith("git~", StringComparison.OrdinalIgnoreCase) - && name[4..].IndexOfAnyExceptInRange('0', '9') < 0; + return name.Equals(".git", StringComparison.OrdinalIgnoreCase) + || name.Equals("git~1", StringComparison.OrdinalIgnoreCase); } /// /// Opens for writing as a new, empty file, creating its - /// folders. A file that is already there is removed first, read-only or not, so the - /// content never goes through a link someone left in its place. + /// folders. Whatever file is already there is removed first, read-only or not, so + /// the content never goes through a link (symbolic or hard) left in its place. /// - public static FileStream Create(string workdir, string repoPath) + /// The repository's working directory. + /// Forward-slash separated path relative to it. + /// + /// Folders already found not to be links during the same operation; pass one set + /// to every call of one materialization so shared parent folders are checked once. + /// + public static FileStream Create(string workdir, string repoPath, ISet? verifiedFolders = null) { string full = Resolve(workdir, repoPath); - RefuseLinkOnTheWay(workdir, full); + RefuseLinkOnTheWay(workdir, full, verifiedFolders); Directory.CreateDirectory(Path.GetDirectoryName(full)!); RemoveFile(full); @@ -99,10 +115,10 @@ internal static class WorkingTree public static void Delete(string workdir, string repoPath) { string full = Resolve(workdir, repoPath); - RefuseLinkOnTheWay(workdir, full); + RefuseLinkOnTheWay(workdir, full, verifiedFolders: null); RemoveFile(full); - string root = Path.TrimEndingDirectorySeparator(Path.GetFullPath(workdir)); + string root = Root(workdir); string? directory = Path.GetDirectoryName(full); while(directory is not null && !string.Equals(Path.TrimEndingDirectorySeparator(directory), root, PathComparison) @@ -116,43 +132,100 @@ internal static class WorkingTree + /// + /// Removes and everything in it, read-only or not, + /// without ever leaving it: a link inside (symbolic link or junction) is removed + /// as a link, and what it points at is neither entered nor changed. A missing + /// directory is fine. + /// + public static void DeleteDirectory(string directory) + { + ArgumentException.ThrowIfNullOrWhiteSpace(directory); + + var folder = new DirectoryInfo(directory); + if(folder.LinkTarget is not null) + { + folder.Delete(); // the link itself + return; + } + + if(!folder.Exists) + { + return; + } + + foreach(DirectoryInfo child in folder.EnumerateDirectories()) + { + DeleteDirectory(child.FullName); + } + + foreach(FileInfo file in folder.EnumerateFiles()) + { + RemoveFile(file.FullName); + } + + folder.Attributes = FileAttributes.Directory; + folder.Delete(); + } + + + private static StringComparison PathComparison => OperatingSystem.IsWindows() ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal; + private static string Root(string workdir) => Path.TrimEndingDirectorySeparator(Path.GetFullPath(workdir)); + + + /// /// A folder between the working directory and that is a /// link (a symbolic link or a junction) would carry the write or delete somewhere /// else. Nothing in a tree gclo materializes is ever a link, so one on the way is - /// refused. + /// refused. A link is recognized by having a target, not by the reparse-point + /// attribute, which cloud-synced folders carry without leading anywhere. /// - private static void RefuseLinkOnTheWay(string workdir, string full) + private static void RefuseLinkOnTheWay(string workdir, string full, ISet? verifiedFolders) { - string root = Path.TrimEndingDirectorySeparator(Path.GetFullPath(workdir)); + string root = Root(workdir); string current = root; string[] segments = full[(root.Length + 1)..].Split(Path.DirectorySeparatorChar); for(int i = 0; i < segments.Length - 1; i++) { current = Path.Combine(current, segments[i]); - var folder = new DirectoryInfo(current); - if(folder.Exists && folder.Attributes.HasFlag(FileAttributes.ReparsePoint)) + if(verifiedFolders?.Contains(current) == true) + { + continue; + } + + if(new DirectoryInfo(current).LinkTarget is not null) { throw new InvalidOperationException( $"'{current}' is a link to another location, so gclo will not write or delete through it. " + "Remove the link, then try again."); } + + verifiedFolders?.Add(current); } } /// - /// Removes one file, read-only or not, when it is there; removes a link itself (even - /// one whose target is gone), never what it points at. + /// Removes what is at when it is a file or a link, read-only + /// or not: a link is removed itself (even one whose target is gone, even one to a + /// directory), never what it points at. /// private static void RemoveFile(string full) { + var directory = new DirectoryInfo(full); + if(directory.LinkTarget is not null && directory.Attributes.HasFlag(FileAttributes.Directory)) + { + directory.Delete(); // a junction or a symbolic link to a directory + return; + } + var file = new FileInfo(full); bool isLink = file.LinkTarget is not null; if(!file.Exists && !isLink) @@ -160,7 +233,9 @@ internal static class WorkingTree return; } - if(!isLink) + // Clearing read-only on a link acts on the link itself on Windows, but on what + // it points at elsewhere; there a link is deleted whatever its mode says. + if(!isLink || OperatingSystem.IsWindows()) { file.Attributes = FileAttributes.Normal; } -- 2.54.0