Fold-in fixes for #963 review findings

- Migration: auto-suffix duplicate Names (already-broken configs) instead
  of blocking startup; keep the friendly hard block for GatewayLocalIp and
  pathological suffix collisions. Tests updated + pathological case added.
- Program.cs: wrap the per-site migrate loop in MigrationSafety too.
- SaveOnly: tear down the old gateway config when gateway-affecting fields
  changed (mirrors SaveAndDeploy), so a later per-row Deploy doesn't hit a
  false-FOREIGN mark-range block from this row's own stale rules; the
  FOREIGN message now names the Remove-then-Deploy recovery.
- GenerateBootScript emits LF regardless of checkout line endings (fixes
  the CRLF-sensitive test on autocrlf machines).
- Shell-safety guard (shape-only) at the top of RemoveAsync/CheckStatusAsync.

Author: tvancott42

src/NetworkOptimizer.Storage/Migrations/20260706000000_AddMonitoringInterfaceAlias.cs

@@ -19,12 +19,27 @@ protected override void Up(MigrationBuilder migrationBuilder)
 
             // Tightening WanIfName+Name -> Name alone (nothing should share a boot-script
             // name across WANs, since the script filename and macvlan name are both keyed
-            // on Name only). If an existing database already has two rows with the same Name
-            // on different WANs, creating the unique index below fails with a raw SQLite
-            // constraint error. That error is caught at the migration call site
-            // (MigrationSafety.MigrateWithFriendlyErrors) and rethrown with an actionable
-            // message, so no SQL-level guard is needed (and RAISE() is illegal outside a
-            // trigger anyway).
+            // on Name only). Rows sharing a Name were legal under the old composite index
+            // but already broken on the gateway (both generate the same boot-script file and
+            // macvlan name), so auto-suffix every colliding row except the oldest with its
+            // own unique Id rather than failing the migration and blocking startup. The
+            // suffix keeps the name inside the validated shape (<= 15 chars, lowercase
+            // letter start, [a-z0-9-]): first chars of the old name + "-" + Id. A renamed
+            // row shows as not-deployed afterwards; redeploying it recreates its artifacts
+            // under the new name.
+            migrationBuilder.Sql(
+                "UPDATE MonitoringInterfaces " +
+                "SET Name = substr(Name, 1, 14 - length(CAST(Id AS TEXT))) || '-' || CAST(Id AS TEXT) " +
+                "WHERE Id NOT IN (SELECT MIN(Id) FROM MonitoringInterfaces GROUP BY Name) " +
+                "AND Name IN (SELECT Name FROM MonitoringInterfaces GROUP BY Name HAVING COUNT(*) > 1);");
+
+            // If a suffixed name still collides with a pre-existing row (pathological: the
+            // user already had a row named exactly "<truncated>-<thatId>"), the unique index
+            // below fails with a raw SQLite constraint error. That error is caught at the
+            // migration call site (MigrationSafety.MigrateWithFriendlyErrors) and rethrown
+            // with an actionable message, so no further SQL-level guard is needed (and
+            // RAISE() is illegal outside a trigger anyway). GatewayLocalIp collisions take
+            // the same friendly-error path - an address can't be auto-renumbered safely.
             migrationBuilder.DropIndex(
                 name: "IX_MonitoringInterfaces_WanIfName_Name",
                 table: "MonitoringInterfaces");

src/NetworkOptimizer.Web/Components/Shared/MonitoringInterfacesCard.razor

@@ -592,11 +592,41 @@
 
     private async Task SaveOnly()
     {
+        // Same pre-edit capture as SaveAndDeploy: without a teardown here, an edit that
+        // changes gateway-affecting fields leaves the old artifacts behind, and for an
+        // aliased row the stale mangle/DNAT rules (same mark, old alias/target text) make
+        // the next per-row Deploy's mark-range preflight misclassify them as FOREIGN and
+        // block the deploy with no path forward short of Remove-then-Deploy.
+        var original = _editingOriginal;
+
         if (!await ValidateAndSaveAsync())
             return;
-        _message = "Saved. It won't take effect until you deploy.";
-        _messageSuccess = true;
+
+        var tornDown = false;
+        var teardownFailed = false;
+        if (original != null && NeedsOldTeardown(original, _editing))
+        {
+            tornDown = true;
+            try
+            {
+                var (removed, _) = await Deploy.RemoveAsync(original);
+                teardownFailed = !removed;
+            }
+            catch (Exception ex)
+            {
+                Logger.LogWarning(ex, "Failed to tear down previous monitoring interface config after save");
+                teardownFailed = true;
+            }
+        }
+
         CancelEdit();
+        // After CancelEdit - it clears _message.
+        _message = teardownFailed
+            ? "Saved, but removing the previous gateway config failed - use Remove, then Deploy, once the gateway is reachable."
+            : tornDown
+                ? "Saved and previous gateway config removed. The new config won't take effect until you deploy."
+                : "Saved. It won't take effect until you deploy.";
+        _messageSuccess = !teardownFailed;
         await LoadAsync();
     }
 

src/NetworkOptimizer.Web/Program.cs

@@ -865,7 +865,7 @@ ProductVersion TEXT NOT NULL
                 .UseSqlite($"Data Source={siteDbPath}")
                 .Options;
             using var siteDb = new NetworkOptimizerDbContext(siteOptions);
-            siteDb.Database.Migrate();
+            NetworkOptimizer.Storage.MigrationSafety.MigrateWithFriendlyErrors(siteDb);
 
             // Seed the Alerts & Schedule defaults into each site's DB too, so secondary
             // sites match the main site instead of showing blank lists. The main-DB seed

src/NetworkOptimizer.Web/Services/MonitoringInterfaceDeploymentService.cs

@@ -219,6 +219,31 @@ public static string MarkRangePreflightCommand(int id, string aliasIp, string ta
         return null;
     }
 
+    /// <summary>
+    /// Defense-in-depth guard for the methods that interpolate row values into root SSH
+    /// commands without running the full <see cref="Validate"/> (remove/status). Shape-only -
+    /// none of Validate's business rules (id range, SNAT requirement, subnet relations) - so
+    /// rows that can no longer pass full validation (e.g. an aliased row whose id outgrew the
+    /// mark range) can still be cleaned up and inspected, while a row whose strings wouldn't
+    /// survive shell interpolation (a hand-edited database) is rejected before reaching SSH.
+    /// The only current write path (the card's ValidateAndSaveAsync) always runs Validate
+    /// first, so this should never fire in normal operation.
+    /// </summary>
+    private static void EnsureShellSafeStrings(MonitoringInterface mi)
+    {
+        static bool IsIpv4(string? s) => (s ?? "").Split('.').Length == 4 &&
+            IPAddress.TryParse(s, out var a) && a.AddressFamily == System.Net.Sockets.AddressFamily.InterNetwork;
+
+        if (!System.Text.RegularExpressions.Regex.IsMatch(mi.Name ?? "", @"\A[a-z][a-z0-9-]{0,14}\z") ||
+            !System.Text.RegularExpressions.Regex.IsMatch(mi.WanIfName ?? "", @"\A[a-zA-Z0-9._-]{1,20}\z") ||
+            !IsIpv4(mi.TargetIp) || !IsIpv4(mi.GatewayLocalIp) ||
+            (mi.AliasIp != null && !IsIpv4(mi.AliasIp)))
+        {
+            throw new InvalidOperationException(
+                $"Monitoring interface {mi.Id} contains values that fail shell-safety validation; refusing to build SSH commands from it.");
+        }
+    }
+
     /// <summary>Reason a deploy preflight blocked, or <see cref="None"/> when clear to deploy.</summary>
     public enum PreflightBlock
     {
@@ -392,7 +417,8 @@ public async Task<DeployResult> DeployAsync(MonitoringInterface mi, Cancellation
             {
                 return new DeployResult(false, PreflightBlock.MarkRangeConflict,
                     $"Table {table} (derived from this interface's own id) is already used by something else on the gateway. " +
-                    "This should not happen under normal operation - report it rather than deploying.", steps);
+                    "If you recently edited this interface's alias or target IP, these are likely its own stale rules - " +
+                    "click Remove, then Deploy, to clear and recreate them. Otherwise report it rather than deploying.", steps);
             }
             // Honest about the residual risk: unlike the boot script's other idempotent
             // checks, its cleanup_marked_rules() sweep for THIS range would happily flush or
@@ -436,6 +462,7 @@ public async Task<DeployResult> DeployAsync(MonitoringInterface mi, Cancellation
     /// </summary>
     public async Task<(bool success, List<string> steps)> RemoveAsync(MonitoringInterface mi)
     {
+        EnsureShellSafeStrings(mi);
         var steps = new List<string>();
         var path = ScriptPath(mi);
         var success = true;
@@ -565,6 +592,7 @@ public bool IsFullyApplied(MonitoringInterface mi)
     /// </summary>
     public async Task<InterfaceStatus> CheckStatusAsync(MonitoringInterface mi)
     {
+        EnsureShellSafeStrings(mi);
         var status = new InterfaceStatus();
         var path = ScriptPath(mi);
 
@@ -679,7 +707,11 @@ private async Task<bool> IsReachableFromServerAsync(string ip)
     public static string GenerateBootScript(MonitoringInterface mi)
     {
         var aliased = mi.AliasIp != null;
+        // The verbatim template inherits this source file's checked-out line endings
+        // (CRLF on Windows with autocrlf) - normalize here so the generated script is
+        // LF-only everywhere it's consumed, not just after DeployAsync's upload scrub.
         return BootScriptTemplate
+            .Replace("\r\n", "\n")
             .Replace("__IFACE__", mi.Name)
             .Replace("__WAN_IF__", mi.WanIfName)
             .Replace("__VLAN_ID__", mi.WanVlanId?.ToString() ?? "")

tests/NetworkOptimizer.Storage.Tests/MonitoringInterfaceMigrationTests.cs

@@ -11,8 +11,9 @@ namespace NetworkOptimizer.Storage.Tests;
 /// <summary>
 /// Exercises the real EF Core migration pipeline against a file-backed SQLite database (no
 /// EnsureCreated, no EF InMemory provider). Covers the AddMonitoringInterfaceAlias migration that
-/// tightens the WanIfName+Name unique index down to Name alone, and the MigrationSafety wrapper that
-/// turns the resulting raw SQLite constraint error into an actionable message.
+/// tightens the WanIfName+Name unique index down to Name alone (auto-suffixing colliding names so
+/// existing installs keep starting), and the MigrationSafety wrapper that turns the remaining raw
+/// SQLite constraint errors into actionable messages.
 /// </summary>
 public class MonitoringInterfaceMigrationTests : IDisposable
 {
@@ -61,11 +62,15 @@ public void MigrateWithFriendlyErrors_OnCleanDatabase_AppliesAllMigrationsWithou
     }
 
     [Fact]
-    public void MigrateWithFriendlyErrors_WhenExistingRowsShareNameAcrossWans_ThrowsHelpfulError()
+    public void MigrateWithFriendlyErrors_WhenExistingRowsShareNameAcrossWans_AutoSuffixesInsteadOfBlocking()
     {
         // Simulate an existing database from before the tightening: migrate up to (but not
         // including) AddMonitoringInterfaceAlias, then seed two rows that share a Name on
         // different WANs - legal under the old WanIfName+Name index but not the new Name index.
+        // Such rows were already broken on the gateway (same boot-script file, same macvlan
+        // name), so the migration renames every colliding row except the oldest to
+        // "<name>-<id>" rather than blocking startup. The 15-char pair proves the truncation
+        // keeps the suffixed name inside the validated shape.
         using (var context = CreateContext())
         {
             var migrator = context.GetService<IMigrator>();
@@ -74,10 +79,45 @@ public void MigrateWithFriendlyErrors_WhenExistingRowsShareNameAcrossWans_Throws
             context.Database.ExecuteSqlRaw(
                 """
                 INSERT INTO MonitoringInterfaces
-                    (Name, WanIfName, TargetIp, SubnetPrefix, GatewayLocalIp, SnatEnabled, WatchdogIntervalMinutes, IsManuallyDeployed, CreatedAt, UpdatedAt)
+                    (Id, Name, WanIfName, TargetIp, SubnetPrefix, GatewayLocalIp, SnatEnabled, WatchdogIntervalMinutes, IsManuallyDeployed, CreatedAt, UpdatedAt)
                 VALUES
-                    ('modem0', 'eth4', '192.168.100.1', 24, '192.168.100.2', 1, 5, 0, '2026-01-01T00:00:00', '2026-01-01T00:00:00'),
-                    ('modem0', 'eth2', '192.168.200.1', 24, '192.168.200.2', 1, 5, 0, '2026-01-01T00:00:00', '2026-01-01T00:00:00');
+                    (1, 'modem0', 'eth4', '192.168.100.1', 24, '192.168.100.2', 1, 5, 0, '2026-01-01T00:00:00', '2026-01-01T00:00:00'),
+                    (2, 'modem0', 'eth2', '192.168.200.1', 24, '192.168.200.2', 1, 5, 0, '2026-01-01T00:00:00', '2026-01-01T00:00:00'),
+                    (3, 'abcdefghijklmno', 'eth5', '192.168.150.1', 24, '192.168.150.2', 1, 5, 0, '2026-01-01T00:00:00', '2026-01-01T00:00:00'),
+                    (4, 'abcdefghijklmno', 'eth6', '192.168.160.1', 24, '192.168.160.2', 1, 5, 0, '2026-01-01T00:00:00', '2026-01-01T00:00:00');
+                """);
+        }
+
+        using (var context = CreateContext())
+        {
+            var act = () => MigrationSafety.MigrateWithFriendlyErrors(context);
+
+            act.Should().NotThrow();
+            var names = context.MonitoringInterfaces.OrderBy(m => m.Id).Select(m => m.Name).ToList();
+            names.Should().Equal("modem0", "modem0-2", "abcdefghijklmno", "abcdefghijklm-4");
+            names.Should().OnlyContain(n => n.Length <= 15);
+        }
+    }
+
+    [Fact]
+    public void MigrateWithFriendlyErrors_WhenAutoSuffixedNameCollidesWithExistingRow_ThrowsHelpfulError()
+    {
+        // Pathological: a pre-existing row already holds exactly the name the fixup would
+        // assign ("modem0-2"). The rename then collides, the unique index fails, and the
+        // friendly Name error surfaces instead of a raw SQLite exception.
+        using (var context = CreateContext())
+        {
+            var migrator = context.GetService<IMigrator>();
+            migrator.Migrate(PreAliasMigration);
+
+            context.Database.ExecuteSqlRaw(
+                """
+                INSERT INTO MonitoringInterfaces
+                    (Id, Name, WanIfName, TargetIp, SubnetPrefix, GatewayLocalIp, SnatEnabled, WatchdogIntervalMinutes, IsManuallyDeployed, CreatedAt, UpdatedAt)
+                VALUES
+                    (1, 'modem0', 'eth4', '192.168.100.1', 24, '192.168.100.2', 1, 5, 0, '2026-01-01T00:00:00', '2026-01-01T00:00:00'),
+                    (2, 'modem0', 'eth2', '192.168.200.1', 24, '192.168.200.2', 1, 5, 0, '2026-01-01T00:00:00', '2026-01-01T00:00:00'),
+                    (3, 'modem0-2', 'eth5', '192.168.150.1', 24, '192.168.150.2', 1, 5, 0, '2026-01-01T00:00:00', '2026-01-01T00:00:00');
                 """);
         }