diff --git a/backend/src/Taskdeck.Api/FirstRun/FirstRunBootstrapper.cs b/backend/src/Taskdeck.Api/FirstRun/FirstRunBootstrapper.cs
index 46524e6b5..8ec8b0f20 100644
--- a/backend/src/Taskdeck.Api/FirstRun/FirstRunBootstrapper.cs
+++ b/backend/src/Taskdeck.Api/FirstRun/FirstRunBootstrapper.cs
@@ -562,28 +562,45 @@ private static void PreserveCorruptConfig(string path, Exception parseError)
var backupPath = $"{path}.corrupt-{DateTime.UtcNow:yyyyMMddHHmmssfff}";
try
{
- File.Copy(path, backupPath, overwrite: false);
+ // The .corrupt-* backup holds the same secrets as the original (the connector key in
+ // particular), so create it ATOMICALLY with owner-only permissions (#1241/#1264) -- a
+ // copy-then-restrict sequence would briefly expose it via the directory's default ACL
+ // (File.Copy does not carry the source's security descriptor). Byte-faithful read: the file is
+ // unparsable, possibly from an interrupted write, so no text decoding is applied.
+ WriteRestrictedFile(backupPath, File.ReadAllBytes(path));
}
- catch (Exception copyEx)
+ catch (Exception ex)
{
- Console.Error.WriteLine(
- $"[FirstRun] WARNING: {path} contains invalid JSON ({parseError.Message}) and could NOT be " +
- $"backed up ({copyEx.Message}); it will be overwritten.");
- return;
- }
+ // Recovery-first fallback: a failed primary backup must not cost the ONLY copy of a
+ // possibly-recoverable key. Fall back to a plain copy + best-effort restriction, warning that
+ // the backup may be readable by other local users until fixed. Copy to a DISTINCT name: if the
+ // primary attempt's best-effort cleanup could not remove its partial file (e.g. an AV/indexer
+ // held a same-user handle), overwrite:false on the same name would always fail here even though
+ // the source is still perfectly readable.
+ backupPath = $"{backupPath}.fallback";
+ try
+ {
+ File.Copy(path, backupPath, overwrite: false);
+ }
+ catch (Exception copyEx)
+ {
+ Console.Error.WriteLine(
+ $"[FirstRun] WARNING: {path} contains invalid JSON ({parseError.Message}) and could NOT be " +
+ $"backed up (primary backup: {ex.Message}; fallback copy: {copyEx.Message}); it will be " +
+ "overwritten.");
+ return;
+ }
- // The .corrupt-* backup holds the same secrets as the original (the connector key in particular), so
- // lock it to the current user too -- otherwise recovery-preservation would re-expose it via the
- // directory's default ACL (#1241). Best-effort: a failed restriction warns but keeps the backup.
- try
- {
- RestrictFileToCurrentUser(backupPath);
- }
- catch (Exception aclEx)
- {
- Console.Error.WriteLine(
- $"[FirstRun] WARNING: preserved {backupPath} but could not restrict its permissions " +
- $"({aclEx.Message}); it may be readable by other local users until fixed.");
+ try
+ {
+ RestrictFileToCurrentUser(backupPath);
+ }
+ catch (Exception aclEx)
+ {
+ Console.Error.WriteLine(
+ $"[FirstRun] WARNING: preserved {backupPath} but could not restrict its permissions " +
+ $"({aclEx.Message}); it may be readable by other local users until fixed.");
+ }
}
Console.Error.WriteLine(
@@ -684,22 +701,20 @@ or WaitHandleCannotBeOpenedException
Directory.CreateDirectory(dir);
var tempPath = Path.Combine(dir, $".{Path.GetFileName(path)}.{Guid.NewGuid():N}.tmp");
- // Create the file empty and lock it to the current user BEFORE writing the secret payload,
- // closing the window where the JWT secret + connector key would sit on disk readable by other
- // local users: under the Unix default umask (022) the file would be 0644, and on Windows it would
- // inherit the directory's default ACL (e.g. BUILTIN\Users read). #1241. A narrow race remains:
- // the empty temp file briefly exists with default permissions, so a racer could pre-open a read
- // handle that survives the restriction (tightening ACLs does not revoke open handles) -- creating
- // the file atomically WITH the restricted ACL closes that too and is tracked in #1264. If the file
- // cannot be locked down, RestrictFileToCurrentUser throws (as IOException) and the secret is never
- // written to it -- the caller falls back to an in-memory value rather than leaving it world-readable.
- // The whole create -> restrict -> write -> move sequence is inside the try so any failure (incl. a
- // failed restrict) deletes the staged temp file rather than leaking it.
+ // Create the temp file ATOMICALLY with owner-only permissions and FileShare.None (#1241/#1264):
+ // under the Unix default umask (022) a plainly-created file would be 0644, and on Windows it
+ // would inherit the directory's default ACL (e.g. BUILTIN\Users read). Supplying the restrictive
+ // mode/DACL at creation leaves no instant where the file is openable by another local user --
+ // the previous create-then-restrict sequence had a window where a racer's pre-opened handle on
+ // the empty file survived the tightened ACL (ACL changes do not revoke open handles) and could
+ // read the later-written secret. FileMode.CreateNew also refuses to adopt a pre-created file.
+ // If the restricted create fails, WriteRestrictedFile throws (normalized to IOException) and the
+ // secret is never written -- the caller falls back to an in-memory value rather than leaving it
+ // world-readable. The create+write -> move sequence is inside the try so any failure deletes the
+ // staged temp file rather than leaking it.
try
{
- File.Create(tempPath).Dispose();
- RestrictFileToCurrentUser(tempPath);
- File.WriteAllText(tempPath, payload);
+ WriteRestrictedFile(tempPath, payload);
ReplaceFileWithRetry(tempPath, path);
}
catch
@@ -746,13 +761,150 @@ internal static void RestrictExistingLocalConfigFile()
}
///
- /// Restricts a freshly-created (still-empty) file to the current user only, BEFORE secrets are written
- /// into it, so appsettings.local.json (JWT secret + connector key) is never readable by other
- /// local users (#1241). On Unix this is 0600; on Windows it replaces the DACL with a single
- /// owner-only ACE and disables inheritance (so the directory's default ACEs -- e.g. BUILTIN\Users read --
- /// do not apply). Any failure is normalized to so the first-run callers'
- /// existing catch (IOException) falls back to an in-memory value -- the secret is never written to
- /// a file we could not lock down.
+ /// Creates ATOMICALLY with owner-only permissions and
+ /// , then writes through that same handle
+ /// (#1264). On Unix the file is born 0600 () and
+ /// the exact mode is then pinned through the open handle (umask-proof); on Windows the protected
+ /// owner-only DACL is supplied to CreateFile itself and read back through the open handle, so a
+ /// filesystem that silently ignores security descriptors (FAT32/exFAT, some SMB shares) fails closed
+ /// instead of persisting the secret unprotected. Unlike create-then-restrict there is no instant at
+ /// which another local user can open the file, and no pre-opened handle can survive into the written
+ /// secret; additionally refuses to adopt a file someone pre-created at
+ /// the path. Any failure is normalized to
+ /// (matching ) so first-run callers'
+ /// catch (IOException) falls back to an in-memory value; a partially-written file is
+ /// best-effort deleted so callers never observe a half-written secret file.
+ ///
+ internal static void WriteRestrictedFile(string path, string contents)
+ => WriteRestrictedFile(path, Encoding.UTF8.GetBytes(contents));
+
+ internal static void WriteRestrictedFile(string path, byte[] contents)
+ {
+ FileStream stream;
+ try
+ {
+ stream = CreateRestrictedNewFile(path);
+ }
+ catch (IOException)
+ {
+ // Creation failed -> nothing of ours remains at the path (CreateRestrictedNewFile removes its
+ // own file when the post-create lockdown pin/verification fails); never delete what we did not
+ // create (CreateNew fails precisely when the path is already occupied).
+ throw;
+ }
+ catch (Exception ex)
+ {
+ // Normalize (e.g. UnauthorizedAccessException, PlatformNotSupportedException) so the callers'
+ // catch(IOException) handles it uniformly -- the secret is never written to an unprotected file.
+ throw new IOException(
+ $"Could not create {path} restricted to the current user; refusing to write the secret to it.", ex);
+ }
+
+ try
+ {
+ using (stream)
+ {
+ stream.Write(contents, 0, contents.Length);
+ }
+ }
+ catch (Exception ex)
+ {
+ // We created the file, so a failed/partial write cleans it up (it was restricted from birth, so
+ // this is consistency hygiene, not exposure). Best-effort; the original failure propagates.
+ try { File.Delete(path); } catch { /* ignore */ }
+ if (ex is IOException)
+ {
+ throw;
+ }
+
+ throw new IOException(
+ $"Could not write the restricted file {path}; refusing to leave a partial secret file.", ex);
+ }
+ }
+
+ private static FileStream CreateRestrictedNewFile(string path)
+ {
+ if (OperatingSystem.IsWindows())
+ {
+ return CreateOwnerOnlyFileWindows(path);
+ }
+
+ var stream = new FileStream(path, new FileStreamOptions
+ {
+ Mode = FileMode.CreateNew,
+ Access = FileAccess.Write,
+ Share = FileShare.None,
+ UnixCreateMode = UnixFileMode.UserRead | UnixFileMode.UserWrite,
+ });
+ try
+ {
+ // open(2)'s mode argument is umask-masked (the mask can only STRIP bits from 0600, never widen
+ // it) and is silently ignored on non-POSIX filesystems (e.g. vfat). Pin exactly 0600 through the
+ // open handle (fchmod -- race-free): this restores the exact-mode guarantee regardless of umask,
+ // and it FAILS on filesystems that cannot store the mode exactly where the pre-#1264
+ // SetUnixFileMode(path) call failed -- keeping the fail-closed contract instead of silently
+ // persisting the secret with whatever mode the mount dictates.
+ File.SetUnixFileMode(stream.SafeFileHandle, UnixFileMode.UserRead | UnixFileMode.UserWrite);
+ return stream;
+ }
+ catch
+ {
+ // We created this file; a failed lockdown must not leave it behind. Best-effort; the original
+ // exception propagates (normalized to IOException by the caller).
+ stream.Dispose();
+ try { File.Delete(path); } catch { /* ignore */ }
+ throw;
+ }
+ }
+
+ [SupportedOSPlatform("windows")]
+ private static FileStream CreateOwnerOnlyFileWindows(string path)
+ {
+ // ReadPermissions (READ_CONTROL) is requested alongside Write so the DACL VERIFICATION below can
+ // read the security descriptor back through this same exclusive handle.
+ var stream = new FileInfo(path).Create(
+ FileMode.CreateNew,
+ FileSystemRights.Write | FileSystemRights.ReadPermissions,
+ FileShare.None,
+ bufferSize: 4096,
+ FileOptions.None,
+ BuildOwnerOnlyFileSecurity());
+ try
+ {
+ // CreateFileW silently IGNORES the supplied security descriptor on filesystems without ACL
+ // support (FAT32/exFAT, some SMB shares): the create succeeds and the file is world-readable.
+ // The pre-#1264 SetAccessControl path FAILED there (fail-closed). Read the DACL back through the
+ // open handle to restore that contract -- on NTFS this merely confirms the atomically-applied
+ // descriptor; on a non-ACL volume it throws or reports an unprotected DACL and we refuse.
+ var applied = stream.GetAccessControl();
+ if (!applied.AreAccessRulesProtected)
+ {
+ throw new IOException(
+ $"The filesystem hosting {path} did not honor the owner-only ACL (FAT32/exFAT and some " +
+ "network shares cannot store it); refusing to write the secret to an unprotected file.");
+ }
+
+ return stream;
+ }
+ catch
+ {
+ // We created this file; a failed lockdown verification must not leave it behind. Best-effort;
+ // the original exception propagates (normalized to IOException by the caller).
+ stream.Dispose();
+ try { File.Delete(path); } catch { /* ignore */ }
+ throw;
+ }
+ }
+
+ ///
+ /// Restricts an EXISTING file to the current user only (#1241). On Unix this is 0600; on Windows
+ /// it replaces the DACL with a single owner-only ACE and disables inheritance (so the directory's
+ /// default ACEs -- e.g. BUILTIN\Users read -- do not apply). Used for forward remediation of files that
+ /// already exist (, the corrupt-backup fallback); NEW
+ /// secret files are instead created atomically restricted via
+ /// (#1264).
+ /// Any failure is normalized to so callers'
+ /// catch (IOException) handle it uniformly.
///
internal static void RestrictFileToCurrentUser(string path)
{
@@ -764,7 +916,7 @@ internal static void RestrictFileToCurrentUser(string path)
return;
}
- RestrictFileToCurrentUserWindows(path);
+ new FileInfo(path).SetAccessControl(BuildOwnerOnlyFileSecurity());
}
catch (IOException)
{
@@ -780,7 +932,7 @@ internal static void RestrictFileToCurrentUser(string path)
}
[SupportedOSPlatform("windows")]
- private static void RestrictFileToCurrentUserWindows(string path)
+ private static FileSecurity BuildOwnerOnlyFileSecurity()
{
using var identity = WindowsIdentity.GetCurrent();
var owner = identity.User;
@@ -796,7 +948,7 @@ private static void RestrictFileToCurrentUserWindows(string path)
// Drop inherited ACEs and grant the current user full control -- the only ACE on the file.
security.SetAccessRuleProtection(isProtected: true, preserveInheritance: false);
security.AddAccessRule(new FileSystemAccessRule(owner, FileSystemRights.FullControl, AccessControlType.Allow));
- new FileInfo(path).SetAccessControl(security);
+ return security;
}
private static void ReplaceFileWithRetry(string tempPath, string path)
diff --git a/backend/tests/Taskdeck.Api.Tests/FirstRun/FirstRunBootstrapperTests.cs b/backend/tests/Taskdeck.Api.Tests/FirstRun/FirstRunBootstrapperTests.cs
index aaaeb4bbc..7ed208538 100644
--- a/backend/tests/Taskdeck.Api.Tests/FirstRun/FirstRunBootstrapperTests.cs
+++ b/backend/tests/Taskdeck.Api.Tests/FirstRun/FirstRunBootstrapperTests.cs
@@ -309,16 +309,38 @@ public void PersistValue_MutexConstructorIsGuarded_StructuralCheck()
}
[Fact]
- public void PersistValue_SetsUnixFileMode_StructuralCheck()
+ public void PersistValue_WritesSecretsViaAtomicRestrictedCreate_StructuralCheck()
{
- // Verify that the temp file gets 0600 permissions on Unix before being
- // moved into place, matching the CLI's CliFirstRunBootstrapper pattern.
+ // #1264 load-bearing wiring: the behavioral tests cannot distinguish atomic create-with-permissions
+ // from a regression back to create-then-restrict (the final file state is identical on NTFS), so pin
+ // the construction structurally. PersistValue's temp file AND the corrupt-config backup must go
+ // through WriteRestrictedFile; the helper must create with the permissions supplied at creation
+ // (UnixCreateMode / FileSecurity passed to Create) rather than restrict post-hoc.
var source = File.ReadAllText(
FindSourceFile("FirstRunBootstrapper.cs"));
- Assert.Contains("SetUnixFileMode", source);
- Assert.Contains("UnixFileMode.UserRead | UnixFileMode.UserWrite", source);
- Assert.Contains("!OperatingSystem.IsWindows()", source);
+ Assert.Contains("WriteRestrictedFile(tempPath, payload)", source);
+ Assert.Contains("WriteRestrictedFile(backupPath", source);
+ Assert.Contains("UnixCreateMode = UnixFileMode.UserRead | UnixFileMode.UserWrite", source);
+ Assert.Contains("BuildOwnerOnlyFileSecurity())", source);
+ // The pre-#1264 sequence must not come back.
+ Assert.DoesNotContain("File.Create(tempPath)", source);
+ Assert.DoesNotContain("RestrictFileToCurrentUser(tempPath)", source);
+ }
+
+ [Fact]
+ public void WriteRestrictedFile_FailsClosedOnNonAclFilesystems_StructuralCheck()
+ {
+ // On FAT32/exFAT/some SMB shares CreateFileW silently IGNORES the supplied security descriptor, and
+ // on non-POSIX mounts open(2)'s mode is ignored -- where the pre-#1264 restrict calls FAILED
+ // (fail-closed). Pin the two post-create guards that restore that contract: the Windows DACL
+ // read-back through the open handle and the Unix exact-mode pin (also umask-proof) through the
+ // open handle.
+ var source = File.ReadAllText(
+ FindSourceFile("FirstRunBootstrapper.cs"));
+
+ Assert.Contains("AreAccessRulesProtected", source);
+ Assert.Contains("File.SetUnixFileMode(stream.SafeFileHandle", source);
}
[Fact]
@@ -396,12 +418,13 @@ public void RestrictFileToCurrentUser_LocksFileToCurrentUserOnly()
}
[Fact]
- public void RestrictFileToCurrentUser_LockdownSurvivesAtomicMove()
+ public void WriteRestrictedFile_LockdownSurvivesAtomicMove()
{
- // #1241 load-bearing property: PersistValue restricts a temp file, then File.Move(overwrite)s it onto
- // the target. This asserts the owner-only lockdown survives that same-directory atomic rename. A
- // regression to copy-then-delete or File.Replace (which preserves the DESTINATION's ACL) would
- // silently defeat the fix; the in-isolation helper test would not catch it.
+ // #1241/#1264 load-bearing property: PersistValue atomically creates a restricted temp file, then
+ // File.Move(overwrite)s it onto the target. This asserts the owner-only lockdown survives that
+ // same-directory atomic rename. A regression to copy-then-delete or File.Replace (which preserves
+ // the DESTINATION's ACL) would silently defeat the fix; the in-isolation helper test would not
+ // catch it.
var dir = Path.Combine(Path.GetTempPath(), $"td-acl-move-{Guid.NewGuid():N}");
Directory.CreateDirectory(dir);
var tempPath = Path.Combine(dir, ".secrets.tmp");
@@ -411,9 +434,7 @@ public void RestrictFileToCurrentUser_LockdownSurvivesAtomicMove()
// Pre-seed an existing (unrestricted) target so the move is a real overwrite.
File.WriteAllText(targetPath, "{}");
- File.Create(tempPath).Dispose();
- FirstRunBootstrapper.RestrictFileToCurrentUser(tempPath);
- File.WriteAllText(tempPath, "{\"secret\":\"x\"}");
+ FirstRunBootstrapper.WriteRestrictedFile(tempPath, "{\"secret\":\"x\"}");
File.Move(tempPath, targetPath, overwrite: true);
if (OperatingSystem.IsWindows())
@@ -447,6 +468,118 @@ public void RestrictFileToCurrentUser_LockdownSurvivesAtomicMove()
}
}
+ [Fact]
+ public void WriteRestrictedFile_CreatesFileAtomicallyLockedToCurrentUser()
+ {
+ // #1264 behavioral test (each CI runner covers its own platform branch): the file must be BORN with
+ // owner-only permissions -- Unix 0600 via FileStreamOptions.UnixCreateMode, Windows via the
+ // protected owner-only DACL supplied to CreateFile -- and hold the exact written content.
+ var path = Path.Combine(Path.GetTempPath(), $"td-atomic-{Guid.NewGuid():N}.tmp");
+ try
+ {
+ FirstRunBootstrapper.WriteRestrictedFile(path, "{\"secret\":\"atomic\"}");
+
+ Assert.Equal("{\"secret\":\"atomic\"}", File.ReadAllText(path));
+ if (OperatingSystem.IsWindows())
+ {
+ var security = new FileInfo(path).GetAccessControl();
+ Assert.True(
+ security.AreAccessRulesProtected,
+ "inheritance should be disabled so the directory's default ACEs do not apply");
+
+ using var identity = WindowsIdentity.GetCurrent();
+ var currentSid = identity.User;
+ var rules = security.GetAccessRules(
+ includeExplicit: true, includeInherited: true, typeof(SecurityIdentifier));
+ Assert.True(rules.Count > 0, "the file should have at least one explicit access rule");
+ foreach (FileSystemAccessRule rule in rules)
+ {
+ Assert.Equal(AccessControlType.Allow, rule.AccessControlType);
+ Assert.Equal(currentSid, rule.IdentityReference);
+ }
+ }
+ else
+ {
+ Assert.Equal(
+ UnixFileMode.UserRead | UnixFileMode.UserWrite,
+ File.GetUnixFileMode(path));
+ }
+ }
+ finally
+ {
+ if (File.Exists(path)) File.Delete(path);
+ }
+ }
+
+ [Fact]
+ public void WriteRestrictedFile_RefusesToAdoptAPreExistingFile()
+ {
+ // #1264: FileMode.CreateNew must fail on an already-occupied path rather than write the secret into
+ // a file someone else created (whose handle/permissions we do not control) -- and it must not
+ // delete or overwrite that pre-existing file.
+ var path = Path.Combine(Path.GetTempPath(), $"td-preexist-{Guid.NewGuid():N}.tmp");
+ File.WriteAllText(path, "pre-existing");
+ try
+ {
+ Assert.Throws(() => FirstRunBootstrapper.WriteRestrictedFile(path, "secret"));
+
+ Assert.True(File.Exists(path), "the pre-existing file must not be deleted on a refused create");
+ Assert.Equal("pre-existing", File.ReadAllText(path));
+ }
+ finally
+ {
+ File.Delete(path);
+ }
+ }
+
+ [Fact]
+ public void QuarantineCorruptLocalConfigAt_BackupIsRestrictedToCurrentUser()
+ {
+ // #1241/#1264: the .corrupt-* backup holds the same secrets as the original, so it must be created
+ // with owner-only permissions (atomically, not copy-then-restrict) and stay byte-faithful for key
+ // recovery.
+ var dir = Path.Combine(Path.GetTempPath(), $"td-corrupt-acl-{Guid.NewGuid():N}");
+ Directory.CreateDirectory(dir);
+ var path = Path.Combine(dir, "appsettings.local.json");
+ File.WriteAllText(path, "{ corrupt but holds : the only key copy");
+ try
+ {
+ FirstRunBootstrapper.QuarantineCorruptLocalConfigAt(path);
+
+ var preserved = Directory.GetFiles(dir, "appsettings.local.json.corrupt-*");
+ Assert.Single(preserved);
+ Assert.Equal("{ corrupt but holds : the only key copy", File.ReadAllText(preserved[0]));
+ if (OperatingSystem.IsWindows())
+ {
+ var security = new FileInfo(preserved[0]).GetAccessControl();
+ Assert.True(
+ security.AreAccessRulesProtected,
+ "the backup must carry the protected (non-inherited) owner-only DACL");
+
+ using var identity = WindowsIdentity.GetCurrent();
+ var currentSid = identity.User;
+ var rules = security.GetAccessRules(
+ includeExplicit: true, includeInherited: true, typeof(SecurityIdentifier));
+ Assert.True(rules.Count > 0);
+ foreach (FileSystemAccessRule rule in rules)
+ {
+ Assert.Equal(AccessControlType.Allow, rule.AccessControlType);
+ Assert.Equal(currentSid, rule.IdentityReference);
+ }
+ }
+ else
+ {
+ Assert.Equal(
+ UnixFileMode.UserRead | UnixFileMode.UserWrite,
+ File.GetUnixFileMode(preserved[0]));
+ }
+ }
+ finally
+ {
+ try { Directory.Delete(dir, recursive: true); } catch { /* best-effort */ }
+ }
+ }
+
private static string FindSourceFile(string fileName)
{
// Walk up from the test output directory to find the repo source.