🛡️ Sentinel: [MEDIUM] Fix TOCTOU vulnerability in file replacement

.jules/sentinel.md

@@ -83,3 +83,8 @@
 **Vulnerability:** 정적 HTML 생성 도구에서 매번 다른 Nonce를 동적으로 생성하여 CSP에 적용하는 것은, 캐싱 효율을 저하시킬 뿐만 아니라 정적 배포 환경(예: GitHub Pages 등)에서 올바른 보안 정책 수립을 방해할 수 있는 안티 패턴입니다.
 **Learning:** 정적으로 고정된 인라인 스타일이나 스크립트에는 난수화된 Nonce보다 콘텐츠 자체의 해시(SHA-256 등)를 사용하는 것이 안전하고 일관된 방식임을 배웠습니다.
 **Prevention:** 자동 생성되는 정적 HTML의 콘텐츠 보안 정책(CSP)에는 `style-src 'sha256-<HASH>'` 방식을 적용하고, `<style>` 태그에서 불필요한 `nonce` 속성을 제거하여 브라우저의 무결성 검증 기능을 적극 활용하십시오.
+
+## 2024-07-14 - [html4tree] 원자적 파일 교체를 통한 TOCTOU 완화
+**Vulnerability:** 임시 파일을 `index.html`로 교체할 때 원자적(Atomic) 이동을 사용하지 않으면, 파일 교체 중간에 외부에서 심볼릭 링크로 덮어쓰는 TOCTOU(Time-of-Check to Time-of-Use) 공격이 발생할 수 있습니다.
+**Learning:** `Files.move`를 사용할 때 `StandardCopyOption.ATOMIC_MOVE`를 시도하여 교체가 원자적으로 이루어지도록 보장해야 합니다. 파일 시스템이 이를 지원하지 않는 경우 예외를 잡아 `REPLACE_EXISTING`으로 우아하게 폴백(fallback)해야 합니다.
+**Prevention:** 파일 쓰기 작업 후 덮어쓰기를 할 때는 가능한 한 `StandardCopyOption.ATOMIC_MOVE` 옵션을 활용하여 경쟁 조건(Race condition)을 방지하십시오.

src/main/kotlin/html4tree/main.kt

@@ -5,6 +5,7 @@ import java.security.MessageDigest
 import java.nio.file.Files
 import java.nio.file.LinkOption
 import java.nio.file.StandardCopyOption
+import java.nio.file.AtomicMoveNotSupportedException
 import java.nio.file.attribute.BasicFileAttributes
 import java.util.Base64
 import com.github.ajalt.clikt.core.CliktCommand
@@ -62,14 +63,20 @@ internal fun crawl_directories(
     processDirectory: (File, Set<String>, Array<File>?) -> Unit = { file, exclude, files -> process_dir(file, exclude, files) },
     processIgnoreFile: (File, Array<String>?) -> Set<String> = { file, names -> process_ignore_file(file, names) },
     listFiles: (File) -> Array<File>? = { it.listFiles() },
-    isDirectory: (File) -> Boolean = { Files.isDirectory(it.toPath(), LinkOption.NOFOLLOW_LINKS) },
-    isSymbolicLink: (File) -> Boolean = { Files.isSymbolicLink(it.toPath()) },
+    readAttributes: (File) -> BasicFileAttributes? = { file ->
+        try {
+            Files.readAttributes(file.toPath(), BasicFileAttributes::class.java, LinkOption.NOFOLLOW_LINKS)
+        } catch (e: Exception) {
+            null
+        }
+    },
     readIdentity: (File) -> FileIdentity = ::read_file_identity
 ) {
     var lle: LinkedListEntry? = ll.pull()
 
     while(lle != null){
-        if (!isDirectory(lle.file)) {
+        val rootAttrs = readAttributes(lle.file)
+        if (rootAttrs == null || !rootAttrs.isDirectory) {
             lle = ll.pull()
             continue
         }
@@ -94,9 +101,12 @@ internal fun crawl_directories(
             dirFiles?.forEach {
                 // ⚡ Bolt Performance Optimization: Short-circuit OS stat calls (isDirectory/isSymbolicLink)
                 // by checking cheap in-memory string exclusion rules first
-                if(!it.name.startsWith(".") && it.name !in exclude && isDirectory(it) && !isSymbolicLink(it)) {
-                    val childEntry = LinkedListEntry(it, currentLevel+1, readIdentity(it).key)
-                    ll.push(childEntry)
+                if(!it.name.startsWith(".") && it.name !in exclude) {
+                    val attrs = readAttributes(it)
+                    if (attrs != null && attrs.isDirectory && !attrs.isSymbolicLink) {
+                        val childEntry = LinkedListEntry(it, currentLevel+1, readIdentity(it).key)
+                        ll.push(childEntry)
+                    }
                 }
             }
         }
@@ -229,12 +239,25 @@ fun process_ignore_file(curr_dir: File, dirFilesNames: Array<String>? = null): S
     return files_to_exclude
 }
 
-fun write_index_file(curr_dir: File, content: String) {
+fun write_index_file(
+    curr_dir: File,
+    content: String,
+    moveAtomic: (java.nio.file.Path, java.nio.file.Path) -> Unit = { src, dst ->
+        Files.move(src, dst, StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE)
+    },
+    moveFallback: (java.nio.file.Path, java.nio.file.Path) -> Unit = { src, dst ->
+        Files.move(src, dst, StandardCopyOption.REPLACE_EXISTING)
+    }
+) {
     val indexPath = curr_dir.toPath().resolve("index.html")
     val tempPath = Files.createTempFile(curr_dir.toPath(), ".index-", ".html")
     try {
         Files.write(tempPath, content.toByteArray(Charsets.UTF_8))
-        Files.move(tempPath, indexPath, StandardCopyOption.REPLACE_EXISTING)
+        try {
+            moveAtomic(tempPath, indexPath)
+        } catch (e: AtomicMoveNotSupportedException) {
+            moveFallback(tempPath, indexPath)
+        }
     } finally {
         Files.deleteIfExists(tempPath)
     }

src/test/kotlin/html4tree/MainTest.kt

@@ -154,8 +154,7 @@ class MainTest {
             processDirectory = { file, _, _ -> processed.add(file) },
             processIgnoreFile = { _, _ -> emptySet() },
             listFiles = { emptyArray() },
-            isDirectory = { true },
-            isSymbolicLink = { false },
+            readAttributes = { mockAttributes(isDir = true) },
             readIdentity = { FileIdentity("after-swap", true) }
         )
 
@@ -176,8 +175,7 @@ class MainTest {
             processDirectory = { file, _, _ -> processed.add(file) },
             processIgnoreFile = { _, _ -> emptySet() },
             listFiles = { emptyArray() },
-            isDirectory = { true },
-            isSymbolicLink = { false },
+            readAttributes = { mockAttributes(isDir = true) },
             readIdentity = { FileIdentity(null, false) }
         )
 
@@ -200,8 +198,7 @@ class MainTest {
             processDirectory = { file, _, _ -> processed.add(file) },
             processIgnoreFile = { _, _ -> emptySet() },
             listFiles = { file -> if (file == root) arrayOf(child) else emptyArray() },
-            isDirectory = { true },
-            isSymbolicLink = { false },
+            readAttributes = { mockAttributes(isDir = true) },
             readIdentity = { file ->
                 val key = file.absolutePath
                 val callCount = callsByPath.getOrDefault(key, 0)
@@ -239,8 +236,7 @@ class MainTest {
             processDirectory = { file, _, _ -> processed.add(file) },
             processIgnoreFile = { _, _ -> emptySet() },
             listFiles = { emptyArray() },
-            isDirectory = { it == directoryEntry },
-            isSymbolicLink = { false },
+            readAttributes = { file -> mockAttributes(isDir = file == directoryEntry) },
             readIdentity = { FileIdentity("directory-key", true) }
         )
 
@@ -444,6 +440,56 @@ class MainTest {
         assertFalse(File(gitDir, "index.html").exists())
     }
 
+    @Test
+    fun testWriteIndexFileAtomicMoveFallback() {
+        val dir = File(tempDir, "fallback")
+        dir.mkdir()
+        var fallbackCalled = false
+        write_index_file(
+            dir,
+            "content",
+            moveAtomic = { _, _ -> throw java.nio.file.AtomicMoveNotSupportedException("src", "dst", "not supported") },
+            moveFallback = { src, dst ->
+                fallbackCalled = true
+                java.nio.file.Files.move(src, dst, java.nio.file.StandardCopyOption.REPLACE_EXISTING)
+            }
+        )
+        assertTrue(fallbackCalled)
+        assertEquals("content", File(dir, "index.html").readText())
+    }
+
+    @Test
+    fun testWriteIndexFileAtomicMoveDefaultFallback() {
+        val dir = File(tempDir, "default-fallback")
+        dir.mkdir()
+        write_index_file(
+            dir,
+            "content",
+            moveAtomic = { _, _ -> throw java.nio.file.AtomicMoveNotSupportedException("src", "dst", "not supported") }
+            // Omitting moveFallback to trigger the default implementation
+        )
+        assertEquals("content", File(dir, "index.html").readText())
+    }
+
+    @Test
+    fun testWriteIndexFileAtomicMoveSuccess() {
+        val dir = File(tempDir, "success")
+        dir.mkdir()
+        var fallbackCalled = false
+        write_index_file(
+            dir,
+            "content",
+            moveAtomic = { src, dst ->
+                java.nio.file.Files.move(src, dst, java.nio.file.StandardCopyOption.REPLACE_EXISTING)
+            },
+            moveFallback = { _, _ ->
+                fallbackCalled = true
+            }
+        )
+        assertFalse(fallbackCalled)
+        assertEquals("content", File(dir, "index.html").readText())
+    }
+
     @Test
     fun testProcessDirHandlesNonDirectoryWithoutThrowing() {
         val notADirectory = File(tempDir, "not-a-directory")
@@ -476,6 +522,16 @@ class MainTest {
         }
     }
 
+    @Test
+    fun testCrawlDirectoriesReadAttributesExceptionCoverage() {
+        val ll = LinkedList()
+        val entry = LinkedListEntry(File(tempDir, "does-not-exist"), 0)
+        ll.push(entry)
+        crawl_directories(ll, -1)
+        // Ensure it doesn't crash and pulls the entry
+        assertNull(ll.pull())
+    }
+
     @Test
     fun testCliParsing() {
         val cli = Html4tree()
@@ -698,12 +754,25 @@ class MainTest {
                 listed = true
                 emptyArray()
             },
-            isDirectory = { true },
-            isSymbolicLink = { false },
+            readAttributes = { mockAttributes(isDir = true) },
             readIdentity = { FileIdentity("current-key", true) }
         )
 
         assertFalse(processed, "fileKey mismatch should skip directory processing")
         assertFalse(listed, "fileKey mismatch should skip child listing")
     }
+
+    private fun mockAttributes(isDir: Boolean): java.nio.file.attribute.BasicFileAttributes {
+        return object : java.nio.file.attribute.BasicFileAttributes {
+            override fun lastModifiedTime() = java.nio.file.attribute.FileTime.fromMillis(0)
+            override fun lastAccessTime() = java.nio.file.attribute.FileTime.fromMillis(0)
+            override fun creationTime() = java.nio.file.attribute.FileTime.fromMillis(0)
+            override fun isRegularFile() = !isDir
+            override fun isDirectory() = isDir
+            override fun isSymbolicLink() = false
+            override fun isOther() = false
+            override fun size() = 0L
+            override fun fileKey() = "mock-key"
+        }
+    }
 }