security(#197): harden ApiKeyProvider persistence + add provider tests (review fixes)

Cold adversarial review of PR #292 = MERGEABLE-WITH-NITS (no BLOCKER/HIGH). Addresses:
- M1: ApiKeyProvider's never-empty invariant was untested. Add ApiKeyProviderTests
  covering WriteKey precedence, load-existing, empty-file regenerate, generate+persist,
  0600 mode, and still-usable-key-when-persist-fails. ResolveKey extracted to an
  internal seam taking the key path (InternalsVisibleTo ErsatzTV.Tests).
- L2: write-then-chmod race — the key was briefly world-readable. Persist now creates
  the file 0600 atomically via FileStreamOptions.UnixCreateMode (then re-asserts).
- L3: a transient read error on an EXISTING key file silently regenerated + clobbered
  it (invalidating every client key). ResolveKey now rethrows on an unreadable existing
  file (fail loud) and only regenerates when the file is absent or empty.

L4 (LocalhostOnly XFF-spoof under default trust-all) and N5 (length oracle on a
fixed-width key) accepted as documented/cosmetic.

Refs #197 #280

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

ErsatzTV.Tests/Services/ApiKeyProviderTests.cs

@@ -0,0 +1,115 @@
+using System.Collections.Generic;
+using System.IO;
+using ErsatzTV.Services;
+using Microsoft.Extensions.Configuration;
+using Microsoft.Extensions.Logging.Abstractions;
+using NUnit.Framework;
+using Shouldly;
+
+namespace ErsatzTV.Tests.Services;
+
+[TestFixture]
+public class ApiKeyProviderTests
+{
+    private DirectoryInfo _tempDir = null!;
+    private string _keyPath = null!;
+
+    [SetUp]
+    public void SetUp()
+    {
+        _tempDir = Directory.CreateTempSubdirectory("etv-apikey-tests");
+        _keyPath = Path.Combine(_tempDir.FullName, "api.key");
+    }
+
+    [TearDown]
+    public void TearDown()
+    {
+        try
+        {
+            _tempDir.Delete(recursive: true);
+        }
+        catch
+        {
+            // best-effort cleanup
+        }
+    }
+
+    private static IConfiguration Config(params (string Key, string Value)[] settings)
+    {
+        var dict = new Dictionary<string, string?>();
+        foreach ((string key, string value) in settings)
+        {
+            dict[key] = value;
+        }
+
+        return new ConfigurationBuilder().AddInMemoryCollection(dict).Build();
+    }
+
+    [Test]
+    public void Configured_WriteKey_Takes_Precedence_And_Does_Not_Write_A_File()
+    {
+        IConfiguration config = Config((ApiKeyProvider.WriteKeyConfigurationKey, "  from-config  "));
+
+        string key = ApiKeyProvider.ResolveKey(config, _keyPath, NullLogger.Instance);
+
+        key.ShouldBe("from-config"); // trimmed
+        File.Exists(_keyPath).ShouldBeFalse();
+    }
+
+    [Test]
+    public void Existing_Key_File_Is_Loaded()
+    {
+        File.WriteAllText(_keyPath, "  persisted-key  \n");
+
+        string key = ApiKeyProvider.ResolveKey(Config(), _keyPath, NullLogger.Instance);
+
+        key.ShouldBe("persisted-key");
+    }
+
+    [Test]
+    public void Empty_Key_File_Is_Regenerated()
+    {
+        File.WriteAllText(_keyPath, "   \n");
+
+        string key = ApiKeyProvider.ResolveKey(Config(), _keyPath, NullLogger.Instance);
+
+        key.ShouldMatch("^[0-9a-f]{64}$");
+        File.ReadAllText(_keyPath).Trim().ShouldBe(key); // the empty file was overwritten
+    }
+
+    [Test]
+    public void Generates_And_Persists_A_256_Bit_Key_When_None_Exists()
+    {
+        string key = ApiKeyProvider.ResolveKey(Config(), _keyPath, NullLogger.Instance);
+
+        key.ShouldMatch("^[0-9a-f]{64}$"); // 32 bytes hex
+        File.Exists(_keyPath).ShouldBeTrue();
+        File.ReadAllText(_keyPath).Trim().ShouldBe(key);
+    }
+
+    [Test]
+    [Platform(Exclude = "Win", Reason = "Unix file mode is not applicable on Windows")]
+    public void Persisted_Key_File_Is_Owner_Read_Write_Only()
+    {
+        ApiKeyProvider.ResolveKey(Config(), _keyPath, NullLogger.Instance);
+
+        UnixFileMode mode = File.GetUnixFileMode(_keyPath);
+        mode.ShouldBe(UnixFileMode.UserRead | UnixFileMode.UserWrite);
+    }
+
+    [Test]
+    public void Returns_A_Usable_Key_Even_When_Persist_Fails()
+    {
+        // Make persistence fail by giving the key a parent path that is a FILE, not a directory:
+        // Directory.CreateDirectory / file creation then throws, Persist swallows it, and the
+        // resolver must still return the in-memory generated key (the never-empty invariant, #280).
+        string fileAsParent = Path.Combine(_tempDir.FullName, "not-a-dir");
+        File.WriteAllText(fileAsParent, "x");
+        string unwritable = Path.Combine(fileAsParent, "api.key");
+
+        string key = ApiKeyProvider.ResolveKey(Config(), unwritable, NullLogger.Instance);
+
+        key.ShouldMatch("^[0-9a-f]{64}$");
+        File.Exists(unwritable).ShouldBeFalse();
+    }
+}

ErsatzTV/Services/ApiKeyProvider.cs

@@ -30,14 +30,14 @@ public sealed class ApiKeyProvider : IApiKeyProvider
         // Defense-in-depth default: gate reads too. Operators behind an authenticating proxy who
         // want anonymous catalog reads can opt out with Api:RequireKeyForReads=false.
         RequireKeyForReads = configuration.GetValue(RequireKeyForReadsConfigurationKey, true);
-        ApiKey = ResolveKey(configuration, logger);
+        ApiKey = ResolveKey(configuration, FileSystemLayout.ApiKeyPath, logger);
     }
 
     public string ApiKey { get; }
 
     public bool RequireKeyForReads { get; }
 
-    private static string ResolveKey(IConfiguration configuration, ILogger logger)
+    internal static string ResolveKey(IConfiguration configuration, string keyFilePath, ILogger logger)
     {
         string configured = configuration[WriteKeyConfigurationKey];
         if (!string.IsNullOrWhiteSpace(configured))
@@ -46,31 +46,41 @@ public sealed class ApiKeyProvider : IApiKeyProvider
             return configured.Trim();
         }
 
-        string path = FileSystemLayout.ApiKeyPath;
-
-        try
+        if (File.Exists(keyFilePath))
         {
-            if (File.Exists(path))
+            string existing;
+            try
             {
-                string existing = File.ReadAllText(path).Trim();
-                if (!string.IsNullOrWhiteSpace(existing))
-                {
-                    logger.LogInformation("Loaded persisted API key from {Path}", path);
-                    return existing;
-                }
+                existing = File.ReadAllText(keyFilePath).Trim();
             }
-        }
-        catch (Exception ex)
-        {
-            logger.LogWarning(ex, "Failed to read persisted API key from {Path}; generating a new one", path);
+            catch (Exception ex)
+            {
+                // The file is present but unreadable (e.g. wrong permissions). Fail loud rather than
+                // silently generate + overwrite it, which would invalidate every client's stored key.
+                logger.LogError(
+                    ex,
+                    "API key file {Path} exists but could not be read; refusing to overwrite it. Fix its "
+                    + "permissions or set {ConfigurationKey}.",
+                    keyFilePath,
+                    WriteKeyConfigurationKey);
+                throw;
+            }
+
+            if (!string.IsNullOrWhiteSpace(existing))
+            {
+                logger.LogInformation("Loaded persisted API key from {Path}", keyFilePath);
+                return existing;
+            }
+
+            logger.LogWarning("Persisted API key file {Path} was empty; generating a new key", keyFilePath);
         }
 
         string generated = GenerateKey();
-        Persist(path, generated, logger);
+        Persist(keyFilePath, generated, logger);
         return generated;
     }
 
-    private static string GenerateKey() =>
+    internal static string GenerateKey() =>
         // 256 bits of entropy, rendered as lowercase hex so it is trivial to copy/paste with no
         // URL-/header-unsafe characters.
         Convert.ToHexString(RandomNumberGenerator.GetBytes(32)).ToLowerInvariant();
@@ -85,12 +95,30 @@ public sealed class ApiKeyProvider : IApiKeyProvider
                 Directory.CreateDirectory(directory);
             }
 
-            File.WriteAllText(path, key);
-
-            // Owner read/write only (0600); no-op / unsupported on Windows.
-            if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
+            if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
             {
-                File.SetUnixFileMode(path, UnixFileMode.UserRead | UnixFileMode.UserWrite);
+                File.WriteAllText(path, key);
+            }
+            else
+            {
+                // Create the file owner-only (0600) up front so the key is never briefly world-readable
+                // between write and chmod. SetUnixFileMode afterwards re-asserts it if the file pre-existed
+                // (UnixCreateMode only applies to a newly-created file, not a truncated one).
+                const UnixFileMode ownerReadWrite = UnixFileMode.UserRead | UnixFileMode.UserWrite;
+                var options = new FileStreamOptions
+                {
+                    Mode = FileMode.Create,
+                    Access = FileAccess.Write,
+                    UnixCreateMode = ownerReadWrite
+                };
+
+                using (var stream = new FileStream(path, options))
+                using (var writer = new StreamWriter(stream))
+                {
+                    writer.Write(key);
+                }
+
+                File.SetUnixFileMode(path, ownerReadWrite);
             }
 
             logger.LogWarning(