diff --git a/docs/security/threat-model.md b/docs/security/threat-model.md index c626c19..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; `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). 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 | @@ -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..4b41e48 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,130 @@ public sealed class PathRecoveryTests : IDisposable + // ---------------------------------------------------------------- inside the working tree only (#141) + + [Theory] + [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-" + kind); + await _client.ApplyRecoveryAsync(target, Recovery(renames: [("bad:file.txt", "bad_file.txt")]), CancellationToken.None); + 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(listed); + 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("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 + } + + + + [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. + 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)); + + Assert.Contains("is a link to another location", ex.Message); + Assert.Empty(Directory.EnumerateFileSystemEntries(elsewhere)); + } + finally + { + WorkingTreeTests.RemoveLink(link); + } + } + + + + [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); + } + + + // ---------------------------------------------------------------- persistence hardening (#31) [Fact] diff --git a/gclo.Engine.Tests/WindowsPathValidationTests.cs b/gclo.Engine.Tests/WindowsPathValidationTests.cs index f2584fa..7e27944 100644 --- a/gclo.Engine.Tests/WindowsPathValidationTests.cs +++ b/gclo.Engine.Tests/WindowsPathValidationTests.cs @@ -60,6 +60,47 @@ public sealed class WindowsPathValidationTests : IDisposable + [Theory] + [InlineData(".git", "_git")] + [InlineData(".GIT", "_GIT")] + [InlineData(".Git", "_Git")] + [InlineData("git~1", "git_1")] + [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 + // 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("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_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~2/readme.md", "GIT~10/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..fd141b7 --- /dev/null +++ b/gclo.Engine.Tests/WorkingTreeTests.cs @@ -0,0 +1,561 @@ +/* + * 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; + + private readonly List _links = []; + + + + public WorkingTreeTests() + { + _workdir = Path.Combine(_root, "work"); + _outside = Path.Combine(_root, "outside"); + Directory.CreateDirectory(_workdir); + Directory.CreateDirectory(_outside); + } + + + + 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); + } + + + + // ---------------------------------------------------------------- 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~2/file.txt")] + [InlineData("GIT~10/file.txt")] + [InlineData("git~x/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)); + + 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~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)); + + Assert.Contains("'.git' folder", ex.Message); + } + + + + [Theory] + [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) + { + // "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("does not open under the name it is written with", 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~1", 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("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_TheNamesGitItselfRefuses_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_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() + { + 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; // 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")) + { + 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_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() + { + Directory.CreateDirectory(Path.Combine(_workdir, ".git")); + + 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 + } + + + + // ---------------------------------------------------------------- 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)); + } + + + + // ---------------------------------------------------------------- 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. The link is removed again when the test ends. + /// + private void LinkDirectory(string link, string target) + { + 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); + } + Assert.NotNull(new DirectoryInfo(link).LinkTarget); + } + + + + /// 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, + }; + 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, $"{fileName} {string.Join(' ', arguments)} failed: {error}"); + } +} diff --git a/gclo.Engine/LibGit2GitClient.cs b/gclo.Engine/LibGit2GitClient.cs index 851b1b8..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); + 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) : 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 @@ -707,26 +707,21 @@ public sealed class LibGit2GitClient : IGitClient { if(!effectiveNow.Contains(stale)) { - DeleteMaterializedFile(workdir, stale); + WorkingTree.Delete(workdir, stale); } } + // 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(); - 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, verifiedFolders); content.CopyTo(file); } @@ -736,34 +731,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 +914,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,10 +935,49 @@ 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 + { + // 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 or IOException or NotSupportedException) + { + throw new InvalidOperationException( + $"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); + } + } + + return document; } + /// 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), @@ -1019,12 +1033,10 @@ 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)) - { - 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 9dd9ee8..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(' ', '.'); diff --git a/gclo.Engine/WorkingTree.cs b/gclo.Engine/WorkingTree.cs new file mode 100644 index 0000000..f7b991c --- /dev/null +++ b/gclo.Engine/WorkingTree.cs @@ -0,0 +1,245 @@ +/* + * 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). +/// +/// +/// 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 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 = Root(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."); + } + + 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; + } + + + + /// + /// 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. (Should NTFS have given + /// .git another short name, still refuses it: + /// resolving a path expands short names.) + /// + public static bool IsGitDirectoryName(string segment) + { + ArgumentNullException.ThrowIfNull(segment); + + int stream = segment.IndexOf(':', StringComparison.Ordinal); + ReadOnlySpan name = (stream < 0 ? segment : segment[..stream]).AsSpan().TrimEnd(". "); + return name.Equals(".git", StringComparison.OrdinalIgnoreCase) + || name.Equals("git~1", StringComparison.OrdinalIgnoreCase); + } + + + + /// + /// Opens for writing as a new, empty file, creating its + /// 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. + /// + /// 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, verifiedFolders); + + 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, verifiedFolders: null); + RemoveFile(full); + + string root = Root(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); + } + } + + + + /// + /// 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. 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, ISet? verifiedFolders) + { + 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]); + 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 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) + { + return; + } + + // 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; + } + + File.Delete(full); + } +}