From 11eb7b9e90ad034491e8863c5fafacacdc50c2be Mon Sep 17 00:00:00 2001 From: Jon Kazama Date: Tue, 26 May 2026 13:16:57 +0200 Subject: [PATCH] fix(themes): tighten editing-buffer save path (tmp cleanup, log-PII, line-refs) Three review-pass fixes on SaveEditingBuffer: - Wrap File.Move in try/catch that deletes the .tmp sibling on failure (AV-scanner lock, EXDEV, share-violation) then rethrows so the outer IOException catch still owns the error path. Avoids accumulating '.json.tmp' litter in the themes dir on retry storms. - Reduce PII in the five new LogWarning calls that previously included full paths containing the user's home directory. Filename-only via Path.GetFileName is sufficient for triage; the two forensics-critical path-escape log calls keep full paths because diagnosing the escape needs the resolved target. WHY-comment anchors the v1.8.0 PII re-audit roadmap. - Replace seven hardcoded 'ThemeRegistry.cs:' references in comments with method-name + symbol descriptions so future Switch/ RefreshCustomCache refactors do not bit-rot the comments. Build 0/0, csharpier clean. --- HellionChat/Themes/ThemeRegistry.cs | 113 ++++++++++++++++++---------- 1 file changed, 75 insertions(+), 38 deletions(-) diff --git a/HellionChat/Themes/ThemeRegistry.cs b/HellionChat/Themes/ThemeRegistry.cs index f734102..3b2f3ee 100644 --- a/HellionChat/Themes/ThemeRegistry.cs +++ b/HellionChat/Themes/ThemeRegistry.cs @@ -116,8 +116,8 @@ public sealed class ThemeRegistry // True try-pattern lookup: returns false when neither built-in nor custom // cache holds the slug, no fallback to default. M3 ThemePicker uses this // for card-rendering, M6 ThemeImportExportRow for fork-slug collisions. - // Cold-cache fallback: LoadCustomBySlug only reverse-iterates the - // pre-populated _customCache (see ThemeRegistry.cs:263-280). If a freshly + // Cold-cache fallback: see `LoadCustomBySlug` lookup-by-slug reverse + // iteration — it only walks the pre-populated _customCache. If a freshly // imported file has not been enumerated yet (or no warm-up ran), the first // lookup would miss silently. Drain RefreshCustomCache once on miss so the // custom file gets picked up before the second lookup. @@ -279,12 +279,12 @@ public sealed class ThemeRegistry // CALLER CONTRACT: the buffer slug must NOT collide with a built-in slug. // Switch() prefers built-ins over custom themes with the same slug - // (ThemeRegistry.cs:107-112), so saving a custom file under a built-in - // slug persists the file but leaves the built-in active — looks green, - // behaves broken. M4 ColorPicker DrawIdleState forks built-in themes - // into a custom slug before BeginEditing, M6 ImportFromPath renames - // built-in-colliding imports to _imported. New call-sites must - // either fork first or rename to a non-built-in slug. + // (see `Switch` built-in-first lookup), so saving a custom file under + // a built-in slug persists the file but leaves the built-in active — + // looks green, behaves broken. M4 ColorPicker DrawIdleState forks + // built-in themes into a custom slug before BeginEditing, M6 + // ImportFromPath renames built-in-colliding imports to _imported. + // New call-sites must either fork first or rename to a non-built-in slug. public bool SaveEditingBuffer(out string targetPath) { targetPath = string.Empty; @@ -310,12 +310,12 @@ public sealed class ThemeRegistry // Safe-by-construction: refuse any slug that collides with a built-in // BEFORE we touch the disk. Switch() prefers built-ins over custom files - // with the same slug (ThemeRegistry.cs:107-112). Without this reject a - // mis-routed caller (or a future bug in ImportFromPath) could persist a - // custom file under a built-in slug — the file lands on disk, Switch - // keeps the built-in active, and the post-save active-slug check below - // returns false. The caller then sees "save failed" while a garbage file - // accumulates in the themes dir on every retry. M4 ColorPicker forks + // with the same slug (see `Switch` built-in-first lookup). Without this + // reject a mis-routed caller (or a future bug in ImportFromPath) could + // persist a custom file under a built-in slug — the file lands on disk, + // Switch keeps the built-in active, and the post-save active-slug check + // below returns false. The caller then sees "save failed" while a garbage + // file accumulates in the themes dir on every retry. M4 ColorPicker forks // built-in themes into a custom slug before BeginEditing, M6 ImportFromPath // renames built-in-colliding imports to _imported, so production // paths already steer clear; this guard catches everything else. @@ -361,20 +361,41 @@ public sealed class ThemeRegistry // mid-write crash (power loss, Wine kill, OOM) leaves either the // previous content or the new content on disk, never a partial JSON // that would silently disappear at next Plugin-Start through the - // ThemeJsonLoader catch-and-continue path (ThemeRegistry.cs:319-322). + // ThemeJsonLoader catch-and-continue path inside RefreshCustomCache. var tmpPath = targetPath + ".tmp"; File.WriteAllText(tmpPath, json); - File.Move(tmpPath, targetPath, overwrite: true); + try + { + File.Move(tmpPath, targetPath, overwrite: true); + } + catch + { + // Avoid `.tmp` litter when Move fails (target locked by AV + // scanner, EXDEV cross-device, share-violation). Best-effort + // delete, then rethrow so the outer IOException catch still + // reports the failure. + try + { + File.Delete(tmpPath); + } + catch + { + // best-effort cleanup + } + throw; + } // Note: the redundant `_lastActiveStamp = DateTime.MinValue` reset from // the earlier plan-draft was removed — Switch() itself already resets - // _lastActiveStamp on the custom-theme path (ThemeRegistry.cs:124) as - // part of the active-switch, so a pre-Switch reset is overwritten anyway. + // _lastActiveStamp on the custom-theme path (see `Switch` + // custom-theme branch resets `_lastActiveStamp`) as part of the + // active-switch, so a pre-Switch reset is overwritten anyway. - // RefreshCustomCache is a yield-iterator (ThemeRegistry.cs:282) — a bare - // call would build the iterator but never enumerate it, so the cache - // side-effect (_customCache[key] = (theme, stamp)) would never run. - // Force-enumerate so the subsequent Switch() finds the freshly saved file. + // `RefreshCustomCache` is a yield-iterator (see its `yield return` + // body) — a bare call would build the iterator but never enumerate + // it, so the cache side-effect (_customCache[key] = (theme, stamp)) + // would never run. Force-enumerate so the subsequent Switch() finds + // the freshly saved file. foreach (var _ in RefreshCustomCache()) { } // Use the sanitised slug for Switch() too — the buffer's raw Slug @@ -391,12 +412,13 @@ public sealed class ThemeRegistry Switch(targetSlug); - // Same-slug in-place edit: Switch() hits the Same-Slug-Noop-Return - // (ThemeRegistry.cs:102-103) and leaves _active pointing at the - // PRE-edit Theme reference. The newly saved colours would only - // surface on the next RefreshActiveIfStale tick (1Hz-throttled, - // up to ~1s lag). Force-pull the freshly-cached Theme directly so - // the post-Save UI sees the edit in the next frame. + // Same-slug in-place edit: Switch() hits its same-slug noop + // early-return (see `Switch` same-slug noop early-return) and + // leaves _active pointing at the PRE-edit Theme reference. The + // newly saved colours would only surface on the next + // RefreshActiveIfStale tick (1Hz-throttled, up to ~1s lag). + // Force-pull the freshly-cached Theme directly so the post-Save + // UI sees the edit in the next frame. if (string.Equals(_active.Slug, targetSlug, StringComparison.OrdinalIgnoreCase)) { var reloaded = LoadCustomBySlug(targetSlug, out _); @@ -408,15 +430,22 @@ public sealed class ThemeRegistry } // Switch() falls back to DefaultSlug when neither built-in nor custom - // matches (ThemeRegistry.cs:128-132). Verify we actually landed on the - // intended theme before reporting success — a silent fallback to the - // default would otherwise mask a save that did persist the file but - // failed to become active (e.g. cache race on slow disks). + // matches (see `Switch` default-slug fallback at the end of the + // method). Verify we actually landed on the intended theme before + // reporting success — a silent fallback to the default would + // otherwise mask a save that did persist the file but failed to + // become active (e.g. cache race on slow disks). if (!string.Equals(_active.Slug, targetSlug, StringComparison.OrdinalIgnoreCase)) { + // Log filename-only (not the full path) here — the path includes + // the user's home directory which counts as PII. Forensics-critical + // log calls above (path-escape detection) keep the full paths + // because diagnosing the escape needs the resolved target. Memory + // anchor: feedback_hellion_chat_changelog (v1.8.0 PII re-audit + // roadmap). _logger?.LogWarning( - "SaveEditingBuffer persisted {Path} but Switch landed on {Active} instead of {Target}", - targetPath, + "SaveEditingBuffer persisted {File} but Switch landed on {Active} instead of {Target}", + Path.GetFileName(targetPath), _active.Slug, targetSlug ); @@ -427,12 +456,20 @@ public sealed class ThemeRegistry } catch (IOException ex) { - _logger?.LogWarning(ex, "I/O error saving editing buffer to {Path}", targetPath); + _logger?.LogWarning( + ex, + "I/O error saving editing buffer to {File}", + Path.GetFileName(targetPath) + ); return false; } catch (UnauthorizedAccessException ex) { - _logger?.LogWarning(ex, "Access denied saving editing buffer to {Path}", targetPath); + _logger?.LogWarning( + ex, + "Access denied saving editing buffer to {File}", + Path.GetFileName(targetPath) + ); return false; } catch (JsonException ex) @@ -444,8 +481,8 @@ public sealed class ThemeRegistry // — verify before saving and add the import if it's not yet present. _logger?.LogWarning( ex, - "JSON serialisation failed for editing buffer at {Path}", - targetPath + "JSON serialisation failed for editing buffer at {File}", + Path.GetFileName(targetPath) ); return false; }