From c43672ff1d7ee56e92415bceaffdc83b49c89cb2 Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Thu, 27 Aug 2026 03:26:17 +0300 Subject: [PATCH] Bind the plan to the conversion and the directory, not just the files A plan that records only file hashes answers "are these the same bytes?" but not "is this the same conversion?". Three ways it could still mean something other than what was approved: The conversion could change underneath it. The plan now records a semantics version alongside the schema version, and -Apply refuses a plan made under different conversion behaviour rather than carrying it out. That version is deliberately separate from the assembly version: tying plan validity to a release number would invalidate every plan on a release that changed nothing about conversion, which teaches people to work around the check instead of reading it. The plan also writes down the target, BOM policy, backup policy, detection mode and the guarantees the converting build provides, so the file is the whole approval and -Apply needs no ambient option state at all. The tree could change underneath it. Paths were absolute, so a plan copied alongside its tree still named the original tree - and every hash matched, because those were the files it was made from. Entries are now relative to a recorded root, applying a copy converts the tree that was approved, a missing root is refused cleanly, and an entry resolving outside the root is refused rather than followed. A plan is a file people can edit and pass around. The file could change between verification and write. The preflight proves every file matches when the run starts; a large tree leaves time after that. The converter's existing recheck compares length and timestamp against what that run itself saw on opening the file, so it cannot speak to a decision made earlier. A caller that committed to specific bytes can now supply the hash it approved, and the source is re-read and compared at the last point before installation. This narrows the window rather than closing it - a source rewritten between that check and File.Replace is still not caught, which would need every source held open against writers for the whole run - and the code and README both say so. Only -Apply pays for the extra read; an ordinary conversion has nothing to compare against and keeps the cheaper check. Also pins the reporting invariant: the displayed categories sum exactly to the selected population, with the two indented lines breaking down the one above them. The summary now describes the running build's guarantees rather than echoing the plan's own booleans, which an edited plan could otherwise use to claim something untrue about the conversion about to happen. 11 new tests, 383 passing. Co-Authored-By: Claude Opus 5 --- README.md | 55 +++-- .../ConversionPlanTests.cs | 214 +++++++++++++++++- .../ConversionSafetyInvariantTests.cs | 79 ++++++- sources/EncodingChecker/ConversionPlan.cs | 194 ++++++++++++++-- sources/EncodingChecker/ConversionReport.cs | 8 + sources/EncodingChecker/EncodingConverter.cs | 55 ++++- sources/EncodingChecker/Program.cs | 34 ++- sources/EncodingChecker/ScanEngine.cs | 4 + 8 files changed, 590 insertions(+), 53 deletions(-) diff --git a/README.md b/README.md index 3262bfb..d1318bb 100644 --- a/README.md +++ b/README.md @@ -112,23 +112,47 @@ Encoding not identified: 0 Refused, ambiguous encoding: 1 Refused, unreadable: 0 -Backups: enabled +Directory: C:\Source Target: utf-8 without BOM +Source encoding: detected per file +Backups: enabled +Guarantees: strict codecs, verified output, atomic install, ambiguity refusal No files modified. ``` +The two indented lines break down `Will convert`; the rest sum exactly to +`Selected`, so the totals can be checked rather than trusted. + `-Apply` carries that plan out. It does not detect anything a second time: the encodings, the target, and the backup setting all come from the plan, so `-BasePath`, `-Target`, `-From`, and `-Backup` are rejected rather than silently -ignored. - -The binding is the point of the feature, not the preview. Every scheduled file -carries the SHA-256 it had when the plan was made, and `-Apply` verifies each one -first. If any file has changed or been deleted in between, **nothing is -converted** — not even the files that still match. A plan reviewed as a whole -belongs to the directory it was reviewed against, and the files most likely to -have changed are the ones something else is actively writing. +ignored. `plan.json` is the whole approval. + +The binding is the point of the feature, not the preview: + +- **Bound to the files.** Every scheduled file carries the SHA-256 it had when + the plan was made, and `-Apply` verifies each one before writing anything. If + any file has changed or been deleted in between, **nothing is converted** — not + even the files that still match. A plan reviewed as a whole belongs to the + directory it was reviewed against, and the files most likely to have changed + are the ones something else is actively writing. Each file is checked once + more at the moment it is installed, which narrows the window between that + verification and the write. +- **Bound to the directory.** Paths are stored relative to a recorded root, so a + plan is a document about a directory rather than about one machine. Applying a + copy of a plan converts the tree it was approved for, not whichever tree it + happens to sit in, and an entry that resolves outside that root is refused. +- **Bound to the conversion.** The plan records the target encoding, BOM policy, + backup policy, whether the source encoding was detected or specified, and a + semantics version describing the conversion behaviour it was approved under. A + plan written under different behaviour is refused rather than carried out — + what was approved was a conversion, not a list of filenames. + +The semantics version is deliberately separate from EC's version number: it moves +only when conversion or classification behaviour changes, so a release that +changes nothing about conversion does not invalidate plans and teach people to +work around the check. ```bash EncodingChecker.exe -BasePath . -Include "*" -Target "utf-8" -Plan plan.json @@ -197,10 +221,15 @@ These are the guarantees the implementation actually provides. bytes is refused rather than guessed at, when the competing encodings would produce different text. `-From` overrides the detection, not the conversion safeguards. See [Ambiguous encodings](#ambiguous-encodings-and--from). -- A plan written by `-Plan` is bound to the SHA-256 of every file it schedules. - `-Apply` verifies all of them before writing anything and refuses the plan - whole if any file has changed, so a decision made about one set of bytes is - never applied to a different one. +- A plan written by `-Plan` is bound to the SHA-256 of every file it schedules, + to the directory those files are under, and to the conversion behaviour it was + approved under. `-Apply` verifies all of them before writing anything and + refuses the plan whole if any has changed, so a decision made about one set of + bytes is never applied to a different one. Under `-Apply`, each source is + re-hashed again immediately before installation. **This narrows the window + between verification and write; it does not close it** — a source rewritten + between that check and the replacement is still not detected, which would + require holding every source open against writers for the whole run. - `-BasePath` itself is rejected if it is a symbolic link, junction, or other reparse point. Reparse-point subdirectories are skipped during traversal, and a file that is (or becomes) a reparse point is rejected at diff --git a/sources/EncodingChecker.Tests/ConversionPlanTests.cs b/sources/EncodingChecker.Tests/ConversionPlanTests.cs index cb83447..6e77ad4 100644 --- a/sources/EncodingChecker.Tests/ConversionPlanTests.cs +++ b/sources/EncodingChecker.Tests/ConversionPlanTests.cs @@ -82,7 +82,20 @@ private ConversionPlan LoadPlan() private PlannedFile PlannedFor(string name) => Assert.Single( LoadPlan().Files, - f => string.Equals(Path.GetFileName(f.Path), name, StringComparison.Ordinal)); + f => string.Equals(f.RelativePath, name, StringComparison.Ordinal)); + + /// Edits the plan on disk, the way someone with a text editor would. + private void Rewrite(Action> edit) + { + using JsonDocument document = JsonDocument.Parse(File.ReadAllText(PlanPath)); + + Dictionary fields = document.RootElement + .EnumerateObject() + .ToDictionary(p => p.Name, p => p.Value.Clone()); + + edit(fields); + File.WriteAllText(PlanPath, JsonSerializer.Serialize(fields)); + } [Fact] public void PlanningWritesNothing() @@ -241,15 +254,7 @@ public void APlanFromAFutureVersionIsRefusedRatherThanGuessedAt() Write("jp.txt", "こんにちは世界。テキスト", "shift_jis"); Assert.Equal(0, Plan()); - using (JsonDocument document = JsonDocument.Parse(File.ReadAllText(PlanPath))) - { - Dictionary fields = document.RootElement - .EnumerateObject() - .ToDictionary(p => p.Name, p => p.Value.Clone()); - - fields["PlanVersion"] = JsonSerializer.SerializeToElement(99); - File.WriteAllText(PlanPath, JsonSerializer.Serialize(fields)); - } + Rewrite(fields => fields["PlanVersion"] = JsonSerializer.SerializeToElement(99)); Assert.Equal(1, Run("-Apply", PlanPath)); } @@ -295,6 +300,195 @@ public void PlanIsRejectedInModesThatConvertNothing(params string[] mode) ["-BasePath", _root, "-Target", "utf-8", "-Plan", PlanPath, .. mode])); } + [Fact] + public void ThePlanDescribesTheConversionAndNotOnlyTheFiles() + { + // "-Apply plan.json" must need no ambient option state to mean something exact, + // so everything that shapes the conversion has to be in the file. + Write("jp.txt", "こんにちは世界。テキスト", "shift_jis"); + + Assert.Equal(0, Plan("-Backup")); + + ConversionPlan plan = LoadPlan(); + + Assert.Equal(ConversionPlan.CurrentPlanVersion, plan.PlanVersion); + Assert.Equal(ConversionSemantics.Current, plan.SemanticsVersion); + Assert.Equal("utf-8", plan.TargetEncoding); + Assert.False(plan.TargetHasBom); + Assert.True(plan.BackupEnabled); + Assert.Equal("Detected", plan.DetectionMode); + Assert.Equal(Path.GetFullPath(_root), plan.BaseDirectory); + Assert.NotEmpty(plan.ECVersion); + + Assert.True(plan.Semantics.StrictDecoding); + Assert.True(plan.Semantics.StrictEncoding); + Assert.True(plan.Semantics.OutputVerification); + Assert.True(plan.Semantics.AtomicInstall); + Assert.True(plan.Semantics.AmbiguityRefusal); + } + + [Fact] + public void ExplicitSourceSelectionIsRecordedAsSuch() + { + Write("jp.txt", "こんにちは世界。テキスト", "shift_jis"); + + Assert.Equal(0, Plan("-From", "shift_jis")); + + ConversionPlan plan = LoadPlan(); + + Assert.Equal("Explicit", plan.DetectionMode); + Assert.Equal("shift_jis", plan.ExplicitSourceEncoding); + Assert.True(Assert.Single(plan.Files).SourceWasSpecified); + Assert.Contains("detection bypassed", plan.Summarize()); + } + + [Fact] + public void APlanMadeUnderDifferentConversionBehaviourIsRefused() + { + // The schema can be identical while the conversion it describes is not. What the + // user approved was a conversion, not a file listing. + string path = Write("jp.txt", "こんにちは世界。テキスト", "shift_jis"); + byte[] original = File.ReadAllBytes(path); + + Assert.Equal(0, Plan()); + Rewrite(fields => + fields["SemanticsVersion"] = + JsonSerializer.SerializeToElement(ConversionSemantics.Current + 1)); + + Assert.Equal(1, Run("-Apply", PlanPath)); + Assert.Equal(original, File.ReadAllBytes(path)); + + Assert.Null(ConversionPlan.Load(PlanPath, out string? error)); + Assert.Contains("different conversion behaviour", error); + } + + [Fact] + public void APlanAppliedFromACopyStillConvertsTheTreeItWasApprovedFor() + { + // A plan carrying absolute paths that is copied alongside its tree still names + // the original tree, and every hash matches, because those are the files it was + // made from. Resolving against the recorded root makes the intent explicit + // instead of incidental: the plan is about one directory, and says which. + const string text = "こんにちは世界。テキスト"; + string original = Write("jp.txt", text, "shift_jis"); + + Assert.Equal(0, Plan()); + + string copy = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); + Directory.CreateDirectory(copy); + + try + { + string copiedPlan = Path.Combine(copy, "plan.json"); + File.Copy(original, Path.Combine(copy, "jp.txt")); + File.Copy(PlanPath, copiedPlan); + + Assert.Equal(0, Run("-Apply", copiedPlan)); + + // The tree the plan named was converted; the copy was not touched. + Assert.Equal(text, Encoding.UTF8.GetString(File.ReadAllBytes(original))); + Assert.Equal( + Encoding.GetEncoding("shift_jis").GetBytes(text), + File.ReadAllBytes(Path.Combine(copy, "jp.txt"))); + } + finally + { + Directory.Delete(copy, recursive: true); + } + } + + [Fact] + public void APlanWhoseDirectoryIsGoneIsRefusedRatherThanResolvedElsewhere() + { + Write("jp.txt", "こんにちは世界。テキスト", "shift_jis"); + Assert.Equal(0, Plan()); + + Rewrite(fields => fields["BaseDirectory"] = JsonSerializer.SerializeToElement( + Path.Combine(_root, "no-such-directory"))); + + Assert.Equal(3, Run("-Apply", PlanPath)); + } + + [Fact] + public void AnEntryReachingOutsideThePlansDirectoryIsRefused() + { + // A plan is an ordinary file that anyone can edit or receive from someone else. + // Its paths must stay inside the directory it claims to be about. + string outside = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); + byte[] content = Encoding.GetEncoding("shift_jis").GetBytes("こんにちは世界。テキスト"); + File.WriteAllBytes(outside, content); + + try + { + Write("jp.txt", "こんにちは世界。テキスト", "shift_jis"); + Assert.Equal(0, Plan()); + + Rewrite(fields => + { + List files = [.. fields["Files"].EnumerateArray()]; + + Dictionary entry = files[0] + .EnumerateObject() + .ToDictionary(p => p.Name, p => p.Value.Clone()); + + entry["RelativePath"] = JsonSerializer.SerializeToElement( + Path.GetRelativePath(_root, outside)); + entry["Sha256"] = JsonSerializer.SerializeToElement( + ConversionMetadataStore.ComputeSha256(outside)); + + fields["Files"] = JsonSerializer.SerializeToElement(new[] { entry }); + }); + + Assert.Equal(3, Run("-Apply", PlanPath)); + + // Untouched, and still Shift_JIS rather than the UTF-8 it would have become. + Assert.Equal(content, File.ReadAllBytes(outside)); + } + finally + { + File.Delete(outside); + } + } + + [Fact] + public void TheDisplayedCategoriesSumToTheSelectedPopulation() + { + // A category total that does not add up is how a mechanism that is actually safe + // loses the confidence it earned. + Write("jp.txt", "こんにちは世界。日本語のテキストです。", "shift_jis"); + Write("ambiguous.txt", "Le café était déjà prêt", "windows-1252"); + Write("plain.txt", "just ascii here", "ascii"); + + Assert.Equal(0, Plan()); + + ConversionPlan plan = LoadPlan(); + string summary = plan.Summarize(); + + int Line(string label) + { + string line = Assert.Single( + summary.Split(Environment.NewLine), + l => l.StartsWith(label, StringComparison.Ordinal)); + + return int.Parse(line[label.Length..].Trim()); + } + + // The indented pair breaks down "Will convert" and is not part of the sum. + Assert.Equal( + Line("Selected:"), + Line("Will convert:") + + Line("Already in target encoding:") + + Line("Encoding not identified:") + + Line("Refused, ambiguous encoding:") + + Line("Refused, unreadable:")); + + Assert.Equal( + Line("Will convert:"), + Line(" encoding determined:") + Line(" same text either way:")); + + Assert.Equal(plan.Files.Count, Line("Selected:")); + } + [Fact] public void TheSummaryAccountsForEverySelectedFile() { diff --git a/sources/EncodingChecker.Tests/ConversionSafetyInvariantTests.cs b/sources/EncodingChecker.Tests/ConversionSafetyInvariantTests.cs index 8cba834..417d0de 100644 --- a/sources/EncodingChecker.Tests/ConversionSafetyInvariantTests.cs +++ b/sources/EncodingChecker.Tests/ConversionSafetyInvariantTests.cs @@ -1,4 +1,4 @@ -using System.Text; +using System.Text; namespace EncodingChecker.Tests; @@ -190,6 +190,83 @@ public void ASuccessfulConversionStillProvesPreservation() Assert.Equal(text, Encoding.UTF8.GetString(File.ReadAllBytes(path))); } + [Fact] + public void SourceThatNoLongerMatchesWhatWasApproved_RefusesAtInstallation() + { + // The preflight in -Apply proves every file matched when the run started. A + // large tree can take a while after that, and the length-and-timestamp recheck + // inside the converter only compares against what this run itself saw on + // opening the file - it cannot speak to a decision made before that. + string path = Path.Combine(_root, "approved.txt"); + byte[] original = Encoding.GetEncoding("shift_jis").GetBytes("こんにちは世界"); + File.WriteAllBytes(path, original); + + ConversionResult result = EncodingConverter.Convert( + path, + path, + Encoding.GetEncoding("shift_jis"), + Encoding.UTF8, + new ConversionOptions + { + // The hash of something else entirely: what an earlier plan would have + // recorded for a file that has since been rewritten. + ExpectedSourceSha256 = new string('0', 64), + }, + progress: null, + CancellationToken.None); + + Assert.False(result.Success); + Assert.Equal(ConversionErrorCode.SourceChangedDuringConversion, result.ErrorCode); + Assert.False(result.ReplacementCommitted); + Assert.Equal(original, File.ReadAllBytes(path)); + } + + [Fact] + public void SourceThatStillMatchesWhatWasApproved_Converts() + { + // The other direction, which matters just as much: a check that refuses + // everything is not a safety feature, it is a broken one. + const string text = "こんにちは世界"; + string path = Path.Combine(_root, "unchanged.txt"); + File.WriteAllBytes(path, Encoding.GetEncoding("shift_jis").GetBytes(text)); + + ConversionResult result = EncodingConverter.Convert( + path, + path, + Encoding.GetEncoding("shift_jis"), + Encoding.UTF8, + new ConversionOptions + { + ExpectedSourceSha256 = ConversionMetadataStore.ComputeSha256(path), + }, + progress: null, + CancellationToken.None); + + Assert.True(result.Success); + Assert.Equal(text, Encoding.UTF8.GetString(File.ReadAllBytes(path))); + } + + [Fact] + public void AnOrdinaryConversionDoesNotPayForTheApprovalCheck() + { + // The stronger check costs a second full read of every file. Conversions that + // nothing committed to in advance have nothing to compare against, so they keep + // the length-and-timestamp recheck and skip the extra pass. + const string text = "こんにちは世界"; + string path = Path.Combine(_root, "ordinary.txt"); + File.WriteAllBytes(path, Encoding.GetEncoding("shift_jis").GetBytes(text)); + + Assert.Null(ConversionOptions.Default.ExpectedSourceSha256); + + ConversionResult result = EncodingConverter.Convert( + path, path, + Encoding.GetEncoding("shift_jis"), Encoding.UTF8, + ConversionOptions.Default, progress: null, CancellationToken.None); + + Assert.True(result.Success); + Assert.Equal(text, Encoding.UTF8.GetString(File.ReadAllBytes(path))); + } + private void AssertRefusedAndUnchanged( string name, byte[] content, string source, string target) { diff --git a/sources/EncodingChecker/ConversionPlan.cs b/sources/EncodingChecker/ConversionPlan.cs index d5a02e3..5188f82 100644 --- a/sources/EncodingChecker/ConversionPlan.cs +++ b/sources/EncodingChecker/ConversionPlan.cs @@ -26,10 +26,76 @@ internal enum PlannedAction Refuse, } +/// +/// What a conversion carried out by this build of EC guarantees. +/// +/// +/// Recorded in every plan so that -Apply is not merely repeating a list of files +/// but re-asserting the conversion those files were approved for. None of these are +/// user-settable today; they are written down because a plan approved under them must +/// not be carried out by a build that no longer provides them. +/// +internal sealed record ConversionSemantics +{ + /// + /// Bumped whenever conversion or classification behaviour changes in a way that + /// makes an older plan's decisions no longer the ones this build would make. + /// + /// + /// Deliberately separate from the assembly version. Tying plan validity to the + /// version number would invalidate every plan on a release that changed nothing + /// about conversion, which teaches people to work around the check rather than read + /// it. This moves only when the meaning of a plan moves. + /// + internal const int Current = 1; + + /// + /// The guarantees of , in words, for a person reading a summary. + /// + /// + /// Comes from the build rather than from a loaded plan's booleans on purpose. A plan + /// is an editable file; describing what it claims about itself would let an edited + /// one state something untrue about the conversion that is actually going to happen. + /// A plan only reaches a summary once its semantics version has been accepted, so + /// describing this build is describing that plan. + /// + internal const string Describes = + "strict codecs, verified output, atomic install, ambiguity refusal"; + + /// Malformed input is rejected rather than replaced. + public bool StrictDecoding { get; init; } = true; + + /// Content the target cannot represent is rejected, not substituted. + public bool StrictEncoding { get; init; } = true; + + /// The output is re-decoded and compared before it is installed. + public bool OutputVerification { get; init; } = true; + + /// The source is never rewritten in place. + public bool AtomicInstall { get; init; } = true; + + /// + /// Files whose bytes do not identify their encoding, where the rival readings + /// disagree about the text, are refused rather than converted on a guess. + /// + public bool AmbiguityRefusal { get; init; } = true; +} + /// One file's entry in a conversion plan. internal sealed record PlannedFile { - public required string Path { get; init; } + /// + /// The file's path relative to the plan's . + /// + /// + /// The identity, in place of an absolute path. A plan carrying absolute paths that is + /// copied alongside its tree still names the original tree, so applying it from the + /// copy would convert the files somewhere else - and every hash would match, because + /// those are the files the plan was made from. Resolving against the recorded root + /// makes that impossible to do by accident, and makes the plan legible as a document + /// about a directory rather than about one machine. + /// + public required string RelativePath { get; init; } public required long Size { get; init; } @@ -76,15 +142,34 @@ internal sealed record PlannedFile /// had when the plan was made, and applying the plan verifies each one. A file that /// changed in between invalidates the plan rather than being converted on the strength of /// a decision made about different bytes. +/// +/// The plan also describes the conversion itself - root, target, BOM policy, source +/// encoding and how it was arrived at, backup policy, and the guarantees the converting +/// build provides - so that applying it needs no ambient option state at all. The file is +/// the whole approval. +/// /// internal sealed record ConversionPlan { - public int PlanVersion { get; init; } = 1; + /// The plan file's schema. Changed only when this shape changes. + internal const int CurrentPlanVersion = 2; + + public int PlanVersion { get; init; } = CurrentPlanVersion; + + /// The conversion behaviour this plan was made under. Checked on apply. + public int SemanticsVersion { get; init; } = ConversionSemantics.Current; public required string CreatedUtc { get; init; } public required string ECVersion { get; init; } + /// + /// What the conversion guaranteed when the plan was approved. Recorded for the + /// reader; is what the tool enforces. + /// + public ConversionSemantics Semantics { get; init; } = new(); + + /// The directory every is under. public required string BaseDirectory { get; init; } public required string TargetEncoding { get; init; } @@ -96,6 +181,12 @@ internal sealed record ConversionPlan /// The source encoding named by the caller, if any. public string? ExplicitSourceEncoding { get; init; } + /// + /// Whether the source encoding was chosen by the caller or worked out from the bytes. + /// + public string DetectionMode => + string.IsNullOrEmpty(ExplicitSourceEncoding) ? "Detected" : "Explicit"; + public required IReadOnlyList Files { get; init; } private static readonly JsonSerializerOptions Options = new() @@ -111,6 +202,7 @@ internal static ConversionPlan FromEntries( bool backupEnabled, string? explicitSource) { + string root = Path.TrimEndingDirectorySeparator(Path.GetFullPath(baseDirectory)); var files = new List(); foreach (ConversionReportEntry entry in entries) @@ -153,7 +245,7 @@ internal static ConversionPlan FromEntries( files.Add(new PlannedFile { - Path = entry.FilePath, + RelativePath = Path.GetRelativePath(root, entry.FilePath), Size = size, Sha256 = hash, Action = action, @@ -173,7 +265,7 @@ internal static ConversionPlan FromEntries( CreatedUtc = DateTime.UtcNow.ToString("O", CultureInfo.InvariantCulture), ECVersion = typeof(ConversionPlan).Assembly.GetName().Version?.ToString() ?? "unknown", - BaseDirectory = baseDirectory, + BaseDirectory = root, TargetEncoding = targetEncoding, TargetHasBom = targetHasBom, BackupEnabled = backupEnabled, @@ -210,9 +302,30 @@ internal static ConversionPlan FromEntries( return null; } - if (plan.PlanVersion != 1) + if (plan.PlanVersion != CurrentPlanVersion) + { + error = $"This plan uses schema version {plan.PlanVersion}; this build " + + $"writes and reads version {CurrentPlanVersion}. Re-run -Plan " + + "to produce one it can carry out."; + return null; + } + + // The schema can be identical while the conversion it describes is not. A + // plan approved under different behaviour is not this build's plan, whatever + // its file format says. + if (plan.SemanticsVersion != ConversionSemantics.Current) + { + error = "This plan was made under different conversion behaviour " + + $"(semantics version {plan.SemanticsVersion}; this build uses " + + $"{ConversionSemantics.Current}, and the plan was written by " + + $"EC {plan.ECVersion}). What it approved is not what this " + + "build would do. Re-run -Plan and review the result."; + return null; + } + + if (plan.Files is null) { - error = $"Plan version {plan.PlanVersion} is not supported."; + error = $"'{path}' does not list any files."; return null; } @@ -227,6 +340,36 @@ internal static ConversionPlan FromEntries( } } + /// + /// The absolute path of a planned file, resolved against the plan's own root. + /// + /// + /// The full path, or if the entry resolves outside that root - + /// a plan is an ordinary file that anyone can edit, and one naming + /// ..\..\Windows\System32 must not reach outside the directory it claims to + /// be about. + /// + internal string? ResolvePath(PlannedFile file) + { + string root = Path.TrimEndingDirectorySeparator(Path.GetFullPath(BaseDirectory)); + string full; + + try + { + full = Path.GetFullPath(Path.Combine(root, file.RelativePath)); + } + catch (Exception ex) when (ex is ArgumentException or NotSupportedException) + { + return null; + } + + return full.StartsWith( + root + Path.DirectorySeparatorChar, + StringComparison.OrdinalIgnoreCase) + ? full + : null; + } + /// /// Confirms every file is still exactly what it was when the plan was made. /// @@ -248,20 +391,28 @@ internal IReadOnlyList FindStaleFiles() if (file.Action != PlannedAction.Convert) continue; + string? path = ResolvePath(file); + + if (path is null) + { + stale.Add($"{file.RelativePath} (resolves outside the plan's directory)"); + continue; + } + try { - if (!File.Exists(file.Path)) + if (!File.Exists(path)) { - stale.Add($"{file.Path} (no longer exists)"); + stale.Add($"{path} (no longer exists)"); continue; } - if (ConversionMetadataStore.ComputeSha256(file.Path) != file.Sha256) - stale.Add($"{file.Path} (contents changed since the plan was made)"); + if (ConversionMetadataStore.ComputeSha256(path) != file.Sha256) + stale.Add($"{path} (contents changed since the plan was made)"); } catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { - stale.Add($"{file.Path} ({ex.Message})"); + stale.Add($"{path} ({ex.Message})"); } } @@ -271,8 +422,9 @@ internal IReadOnlyList FindStaleFiles() /// The summary a person reads before deciding. /// /// Written so the counts add up on the page. A reader who cannot see that the - /// sub-totals sum to the whole has to trust the numbers instead of checking them, - /// which is the opposite of what a preflight is for. + /// categories sum to the whole has to trust the numbers instead of checking them, + /// which is the opposite of what a preflight is for. The two indented lines break + /// down the one above them and are not part of that sum. /// internal string Summarize() { @@ -297,17 +449,19 @@ internal string Summarize() $"Refused, ambiguous encoding: {changing}", $"Refused, unreadable: {otherRefusals}", string.Empty, - $"Backups: {(BackupEnabled ? "enabled" : "DISABLED")}", + $"Directory: {BaseDirectory}", $"Target: {TargetEncoding}" + (TargetHasBom ? " with BOM" : " without BOM"), + "Source encoding: " + + (string.IsNullOrEmpty(ExplicitSourceEncoding) + ? "detected per file" + : $"{ExplicitSourceEncoding} (specified; detection bypassed)"), + $"Backups: {(BackupEnabled ? "enabled" : "DISABLED")}", + $"Guarantees: {ConversionSemantics.Describes}", + string.Empty, + "No files modified.", }; - if (!string.IsNullOrEmpty(ExplicitSourceEncoding)) - lines.Add($"Source encoding: {ExplicitSourceEncoding} (specified)"); - - lines.Add(string.Empty); - lines.Add("No files modified."); - return string.Join(Environment.NewLine, lines); } } diff --git a/sources/EncodingChecker/ConversionReport.cs b/sources/EncodingChecker/ConversionReport.cs index b35d3cb..47ac39b 100644 --- a/sources/EncodingChecker/ConversionReport.cs +++ b/sources/EncodingChecker/ConversionReport.cs @@ -79,6 +79,14 @@ internal sealed class ConversionReportEntry /// internal bool SourceEncodingWasSpecified { get; set; } + /// + /// The SHA-256 this file is required to still have when it is installed, or + /// when nothing earlier committed to its contents. Set when + /// the conversion was approved in advance by a plan. + /// Internal state; not included in CSV output. + /// + internal string? ExpectedSourceSha256 { get; set; } + /// Additional error detail; not included in CSV output. internal string? Diagnostic { get; set; } } diff --git a/sources/EncodingChecker/EncodingConverter.cs b/sources/EncodingChecker/EncodingConverter.cs index 9aa82dd..41b4fd6 100644 --- a/sources/EncodingChecker/EncodingConverter.cs +++ b/sources/EncodingChecker/EncodingConverter.cs @@ -73,6 +73,25 @@ internal sealed record ConversionOptions /// A failure here therefore aborts the conversion with the original intact. /// internal Func? RecordConversion { get; init; } + + /// + /// The SHA-256 the source is required to still have at the moment of installation, + /// or to skip the check. + /// + /// + /// Set when the decision to convert this file was made earlier, against bytes that + /// were read then - which is what a conversion plan is. The length-and-timestamp + /// recheck above catches a source rewritten during conversion, but it compares + /// against what this run itself observed on opening the file, so it cannot speak to + /// anything that happened before that. A plan can. + /// + /// This narrows the window rather than closing it: a source rewritten between this + /// check and File.Replace is still not detected. Eliminating that entirely + /// needs the source held open against writers for the whole conversion, which would + /// fail conversions of files legitimately open elsewhere. + /// + /// + internal string? ExpectedSourceSha256 { get; init; } } /// @@ -405,11 +424,13 @@ internal static ConversionResult Convert( // purpose: the backup already exists, the conversion is verified, and the // original is still in place, so a failure to record leaves nothing to // recover from and nothing needing recovery. - if (options.RecordConversion is not null) + // Hashed only when something asks for it, so the extra read is not paid for + // by conversions that need neither the check nor the record. + string sourceFileSha = string.Empty; + + if (options.ExpectedSourceSha256 is not null || + options.RecordConversion is not null) { - // Hashed only when a record is being written, so the extra pass is not - // paid for by conversions that do not need it. - string sourceFileSha; try { sourceFileSha = ComputeFileSha256(sourcePath); @@ -419,7 +440,33 @@ internal static ConversionResult Convert( { sourceFileSha = string.Empty; } + } + // The last point at which nothing has been installed. A caller that decided + // to convert these bytes earlier gets to insist they are still those bytes. + if (options.ExpectedSourceSha256 is not null && + !string.Equals( + sourceFileSha, + options.ExpectedSourceSha256, + StringComparison.OrdinalIgnoreCase)) + { + return Failure( + ConversionErrorCode.SourceChangedDuringConversion, + sourceFileSha.Length == 0 + ? "The source file could not be re-read to confirm it still " + + "matches the one this conversion was approved for." + : "The source file no longer matches the one this conversion " + + "was approved for; it changed before installation.", + sourceEncoding, + targetEncoding) with + { + SourceBytes = sourceBytesProcessed, + TargetBytes = targetBytesWritten, + }; + } + + if (options.RecordConversion is not null) + { string? recordError = options.RecordConversion(new ConversionRecord { SourcePath = sourcePath, diff --git a/sources/EncodingChecker/Program.cs b/sources/EncodingChecker/Program.cs index cb52d11..c319e5f 100644 --- a/sources/EncodingChecker/Program.cs +++ b/sources/EncodingChecker/Program.cs @@ -157,17 +157,27 @@ Write a conversion plan and change nothing. The plan whether the bytes identify it uniquely, and which files could come out with different text. + It also records the conversion itself - directory, + target encoding, BOM policy, backup policy, and the + guarantees this build provides - so the file is the + whole approval and needs no other options to mean + something exact. + [-Apply ] Carry out a plan written by -Plan. Every file is checked against the hash it had when the plan was - made; if any has changed, nothing is converted. A - plan approved for one set of files is not applied to - a different one. + made; if any has changed, nothing is converted at + all. A plan approved for one set of files is not + applied to a different one. Each file is checked + once more at the moment it is installed. Nothing is detected a second time: the encodings, the target, and the backup setting all come from the plan, so -BasePath, -Target, -From, and -Backup are - rejected here rather than silently ignored. + rejected here rather than silently ignored. A plan + written under different conversion behaviour, or by + an incompatible schema, is refused rather than + guessed at. Modes: Conversion is the default mode. @@ -260,6 +270,15 @@ private static int ApplyPlan(CliOptions options) return 1; } + // Every path in the plan is relative to this, so if it is gone there is nothing + // to resolve them against and no way to tell which tree was meant. + if (!Directory.Exists(plan.BaseDirectory)) + { + Console.Error.WriteLine( + $"The plan's directory no longer exists: {plan.BaseDirectory}"); + return 3; + } + // The whole reason a plan exists. Re-detecting here would make the preview a // demonstration rather than a promise: a second pass can reach different // conclusions, and it was the first that the user approved. @@ -288,7 +307,7 @@ .. plan.Files .Where(f => f.Action == PlannedAction.Convert) .Select(f => new ConversionReportEntry { - FilePath = f.Path, + FilePath = plan.ResolvePath(f)!, SourceEncoding = f.SourceEncoding, SourceHasBom = f.SourceHasBom, TargetEncoding = plan.TargetEncoding, @@ -299,6 +318,11 @@ .. plan.Files AmbiguityReason = f.AmbiguityReason, CompetingEncodings = f.CompetingEncodings, SourceEncodingWasSpecified = f.SourceWasSpecified, + + // Checked again at the moment of installation. FindStaleFiles above + // proved the file matched when this run started; a long conversion + // leaves room for that to stop being true. + ExpectedSourceSha256 = f.Sha256, }) ]; diff --git a/sources/EncodingChecker/ScanEngine.cs b/sources/EncodingChecker/ScanEngine.cs index 8f1468b..9fb188e 100644 --- a/sources/EncodingChecker/ScanEngine.cs +++ b/sources/EncodingChecker/ScanEngine.cs @@ -504,6 +504,10 @@ private static void ApplyConversion( RecordConversion = backup ? record => WriteConversionMetadata(path, record) : null, + + // Non-null only under -Apply, where an earlier run committed to these exact + // bytes. An ordinary conversion has nothing to compare against. + ExpectedSourceSha256 = entry.ExpectedSourceSha256, }; // Without the token, Parallel.ForEach could only observe cancellation between