feat(stored-key): add atomic file storage method using temporary files (#4756)

* feat(stored-key): add atomic file storage method using temporary files

* test(stored-key): add tests for storing keys with temporary files

* feat(stored-key): add storageFailed error case and handle in save method

Author: sergei-boiko-trustwallet

android/app/src/androidTest/java/com/trustwallet/core/app/utils/TestKeyStore.kt

@@ -7,6 +7,7 @@ import wallet.core.jni.StoredKey
 import wallet.core.jni.CoinType
 import wallet.core.jni.StoredKeyEncryption
 import java.io.File
+import java.util.UUID
 
 class TestKeyStore {
 
@@ -121,6 +122,30 @@ class TestKeyStore {
     }
 
     @Test
+    fun testStoreWithTemporaryFile() {
+        val password = "password".toByteArray()
+        val keyStore = StoredKey("Test Wallet", password)
+
+        // Use UUID-generated names in the system temp directory — no file is
+        // created upfront, avoiding the TOCTOU race of create-then-delete.
+        // Both paths share the same directory so rename(2) is atomic.
+        val tmpDir = File(System.getProperty("java.io.tmpdir")!!)
+        val destFile = File(tmpDir, UUID.randomUUID().toString() + ".json")
+        val tempFile = File(tmpDir, UUID.randomUUID().toString() + ".json.tmp")
+
+        try {
+            assertTrue(keyStore.storeWithTemporaryFile(destFile.absolutePath, tempFile.absolutePath))
+            assertTrue(destFile.exists())
+            assertFalse(tempFile.exists()) // rename consumed the temp file
+
+            val loaded = StoredKey.load(destFile.absolutePath)
+            assertNotNull(loaded)
+        } finally {
+            destFile.delete()
+            tempFile.delete()
+        }
+    }
+
     fun testFixScryptWithEmptySalt() {
         val gMnemonic = "team engine square letter hero song dizzy scrub tornado fabric divert saddle"
         val password = "password".toByteArray()

include/TrustWalletCore/TWStoredKey.h

@@ -285,12 +285,34 @@ void TWStoredKeyRemoveAccountForCoinDerivationPath(struct TWStoredKey* _Nonnull
 
 /// Saves the key to a file.
 ///
+/// \note Prefer `TWStoredKeyStoreWithTemporaryFile` over this function. It writes to a
+/// temporary file first and then renames it atomically, which prevents data loss if the
+/// process is interrupted mid-write. This function writes directly to `path` and will
+/// truncate the existing file before writing, so an interrupted write leaves a corrupt file.
+///
 /// \param key Non-null pointer to a stored key
 /// \param path Non-null string filepath where the key will be saved
 /// \return true if the key was successfully stored in the given filepath file, false otherwise
 TW_EXPORT_METHOD
 bool TWStoredKeyStore(struct TWStoredKey* _Nonnull key, TWString* _Nonnull path);
 
+/// Saves the key to a file atomically using a temporary file and rename.
+///
+/// Writes the key JSON to `temporaryPath` first, then renames it to `path` in a single
+/// atomic operation. The original file at `path` is never truncated until the new content
+/// is fully written and flushed, so a crash or I/O error mid-write leaves the original
+/// file intact. Prefer this over `TWStoredKeyStore` whenever the caller can supply a
+/// suitable temporary path (typically the same directory as `path` with a unique suffix
+/// to guarantee the rename stays on the same filesystem volume).
+///
+/// \param key Non-null pointer to a stored key
+/// \param path Non-null string filepath where the key will be saved
+/// \param temporaryPath Non-null string filepath used for the intermediate write; must be
+///        on the same filesystem volume as `path`
+/// \return true if the key was successfully stored in the given filepath file, false otherwise
+TW_EXPORT_METHOD
+bool TWStoredKeyStoreWithTemporaryFile(struct TWStoredKey* _Nonnull key, TWString* _Nonnull path, TWString* _Nonnull temporaryPath);
+
 /// Decrypts the private key.
 ///
 /// \param key Non-null pointer to a stored key

src/Keystore/StoredKey.cpp

@@ -506,9 +506,32 @@ nlohmann::json StoredKey::json() const {
 
 // File operations
 
-void StoredKey::store(const std::string& path) {
+void StoredKey::store(const std::string& path) const {
+    const std::string jsonData = json().dump();
+
     auto stream = std::ofstream(path);
-    stream << json();
+    if (!stream) {
+        throw std::invalid_argument("Can't open file for writing: " + path);
+    }
+    stream << jsonData;
+    stream.flush();
+    if (!stream) {
+        throw std::runtime_error("Failed to write key file: " + path);
+    }
+}
+
+void StoredKey::storeWithTemporaryFile(const std::string& path, const std::string& tempFilePath) const {
+    try {
+        store(tempFilePath);
+    } catch (...) {
+        std::remove(tempFilePath.c_str());
+        throw;
+    }
+
+    if (std::rename(tempFilePath.c_str(), path.c_str()) != 0) {
+        std::remove(tempFilePath.c_str());
+        throw std::runtime_error("Failed to rename key file: temp=" + tempFilePath + ", target=" + path);
+    }
 }
 
 StoredKey StoredKey::load(const std::string& path) {

src/Keystore/StoredKey.h

@@ -151,7 +151,13 @@ class StoredKey {
     /// Stores the key into an encrypted file.
     ///
     /// \param path file path to store in.
-    void store(const std::string& path);
+    void store(const std::string& path) const;
+
+    /// Stores the key into an encrypted file, using a temporary file to ensure atomicity of the operation.
+    ///
+    /// \param path file path to store in.
+    /// \param tempFilePath file path to use for temporary file during the store operation.
+    void storeWithTemporaryFile(const std::string& path, const std::string& tempFilePath) const;
 
     /// Initializes `StoredKey` with a JSON object.
     void loadJson(const nlohmann::json& json);

src/interface/TWStoredKey.cpp

@@ -221,6 +221,17 @@ bool TWStoredKeyStore(struct TWStoredKey* _Nonnull key, TWString* _Nonnull path)
     }
 }
 
+bool TWStoredKeyStoreWithTemporaryFile(struct TWStoredKey* _Nonnull key, TWString* _Nonnull path, TWString* _Nonnull temporaryPath) {
+    try {
+        const auto& pathString = *reinterpret_cast<const std::string*>(path);
+        const auto& temporaryPathString = *reinterpret_cast<const std::string*>(temporaryPath);
+        key->impl.storeWithTemporaryFile(pathString, temporaryPathString);
+        return true;
+    } catch (...) {
+        return false;
+    }
+}
+
 TWData* _Nullable TWStoredKeyDecryptPrivateKey(struct TWStoredKey* _Nonnull key, TWData* _Nonnull password) {
     try {
         const auto passwordData = TW::data(TWDataBytes(password), TWDataSize(password));

swift/Sources/KeyStore.Error.swift

@@ -13,6 +13,7 @@ extension KeyStore {
         case invalidJSON
         case invalidKey
         case invalidPassword
+        case storageFailed
 
         public var errorDescription: String? {
             switch self {
@@ -26,6 +27,8 @@ extension KeyStore {
                 return NSLocalizedString("Invalid private key", comment: "Error message when trying to import an invalid private key")
             case .invalidPassword:
                 return NSLocalizedString("Invalid password", comment: "Error message when trying to export a private key")
+            case .storageFailed:
+                return NSLocalizedString("Failed to save keystore file", comment: "Error message when the keystore file cannot be written to disk. Please check available disk space and directory permissions")
             }
         }
     }

swift/Sources/KeyStore.swift

@@ -395,8 +395,16 @@ public final class KeyStore {
         return keyDirectory.appendingPathComponent(generateFileName(identifier: UUID().uuidString))
     }
 
+    private func generateTempFileURL(accountURL: URL) -> URL {
+        return accountURL.deletingLastPathComponent()
+            .appendingPathComponent(accountURL.lastPathComponent + "." + UUID().uuidString + ".tmp")
+    }
+
     private func save(wallet: Wallet) throws {
-        _ = wallet.key.store(path: wallet.keyURL.path)
+        let tempFilePath = generateTempFileURL(accountURL: wallet.keyURL).path
+        guard wallet.key.storeWithTemporaryFile(path: wallet.keyURL.path, temporaryPath: tempFilePath) else {
+            throw Error.storageFailed
+        }
     }
 
     /// Generates a unique file name for an address.

swift/Tests/Keystore/KeyStoreTests.swift

@@ -574,6 +574,27 @@ class KeyStoreTests: XCTestCase {
         XCTAssertEqual(decryptedMnemonic, mnemonic)
     }
 
+    func testCreateWalletThrowsStorageFailedOnUnwritableDirectory() throws {
+        let dir = try createTempDirURL()
+        let keyStore = try KeyStore(keyDirectory: dir)
+
+        // Make the directory read-only so storeWithTemporaryFile cannot create any file in it.
+        try fileManager.setAttributes([.posixPermissions: 0o444], ofItemAtPath: dir.path)
+        defer {
+            // Restore permissions so teardown can clean up the directory.
+            try? fileManager.setAttributes([.posixPermissions: 0o755], ofItemAtPath: dir.path)
+        }
+
+        do {
+            _ = try keyStore.createWallet(name: "test", password: "password", coins: [.ethereum])
+            XCTFail("Expected storageFailed error but no error was thrown")
+        } catch KeyStore.Error.storageFailed {
+            // expected
+        } catch {
+            XCTFail("Expected storageFailed but got: \(error)")
+        }
+    }
+
     func createTempDirURL() throws -> URL {
         let dir = URL(fileURLWithPath: NSTemporaryDirectory()).appendingPathComponent("keystore")
         try? fileManager.removeItem(at: dir)

swift/Tests/Keystore/KeystoreKeyTests.swift

@@ -130,6 +130,29 @@ class KeystoreKeyTests: XCTestCase {
         let kdfparams: KdfParams
     }
 
+    func testStoreWithTemporaryFile() {
+        let key = StoredKey.importPrivateKey(
+            privateKey: Data(hexString: "3a1076bf45ab87712ad64ccb3b10217737f7faacbf2872e88fdd9a537d8fe266")!,
+            name: "name",
+            password: Data("password".utf8),
+            coin: .ethereum
+        )!
+
+        let dir = URL(fileURLWithPath: NSTemporaryDirectory())
+        let destURL = dir.appendingPathComponent(UUID().uuidString + ".json")
+        let tempURL = dir.appendingPathComponent(UUID().uuidString + ".json.tmp")
+        defer {
+            try? FileManager.default.removeItem(at: destURL)
+            try? FileManager.default.removeItem(at: tempURL)
+        }
+
+        XCTAssertTrue(key.storeWithTemporaryFile(path: destURL.path, temporaryPath: tempURL.path))
+        XCTAssertTrue(FileManager.default.fileExists(atPath: destURL.path))
+        XCTAssertFalse(FileManager.default.fileExists(atPath: tempURL.path)) // consumed by rename
+
+        XCTAssertNotNil(StoredKey.load(path: destURL.path))
+    }
+
     func testEncryptionParameters() {
         let url = Bundle(for: type(of: self)).url(forResource: "key", withExtension: "json")!
         let key = StoredKey.load(path: url.path)!

tests/interface/TWStoredKeyTests.cpp

@@ -34,10 +34,25 @@ struct std::shared_ptr<TWStoredKey> createAStoredKey(TWCoinType coin, TWData* pa
 struct std::shared_ptr<TWStoredKey> createDefaultStoredKey(TWStoredKeyEncryption encryption = TWStoredKeyEncryptionAes128Ctr) {
     const auto passwordString = WRAPS(TWStringCreateWithUTF8Bytes("password"));
     const auto password = WRAPD(TWDataCreateWithBytes(reinterpret_cast<const uint8_t *>(TWStringUTF8Bytes(passwordString.get())), TWStringSize(passwordString.get())));
-    
+
     return createAStoredKey(TWCoinTypeBitcoin, password.get(), encryption);
 }
 
+/// Reads the entire contents of the file at `path` into a Data buffer.
+/// Throws std::invalid_argument if the file cannot be opened.
+/// Note: uses a byte-by-byte loop instead of ifs.read() to avoid static-analysis warnings.
+static Data readFileData(const string& path) {
+    ifstream ifs(path);
+    if (!ifs.is_open()) throw std::invalid_argument("Cannot open file: " + path);
+    ifs.seekg(0, ifs.end);
+    const auto length = ifs.tellg();
+    ifs.seekg(0, ifs.beg);
+    Data data(length);
+    size_t idx = 0;
+    while (!ifs.eof() && idx < static_cast<size_t>(length)) { char c = ifs.get(); data[idx++] = static_cast<uint8_t>(c); }
+    return data;
+}
+
 TEST(TWStoredKey, loadPBKDF2Key) {
     const auto filename = WRAPS(TWStringCreateWithUTF8Bytes((TESTS_ROOT + "/common/Keystore/Data/pbkdf2.json").c_str()));
     const auto key = WRAP(TWStoredKey, TWStoredKeyLoad(filename.get()));
@@ -439,19 +454,8 @@ TEST(TWStoredKey, storeAndImportJSONAES192Ctr) {
     const auto outFileNameStr = WRAPS(TWStringCreateWithUTF8Bytes(outFileName.c_str()));
     EXPECT_TRUE(TWStoredKeyStore(key.get(), outFileNameStr.get()));
 
-    // read contents of file
-    ifstream ifs(outFileName);
-    // get length of file:
-    ifs.seekg (0, ifs.end);
-    auto length = ifs.tellg();
-    ifs.seekg (0, ifs.beg);
-    EXPECT_TRUE(length > 20);
-
-    Data json(length);
-    size_t idx = 0;
-    // read the slow way, ifs.read gave some false warnings with codacy
-    while (!ifs.eof() && idx < static_cast<std::size_t>(length)) { char c = ifs.get(); json[idx++] = (uint8_t)c; }
-
+    auto json = readFileData(outFileName);
+    EXPECT_GT(json.size(), 20ul);
     const auto key2 = WRAP(TWStoredKey, TWStoredKeyImportJSON(WRAPD(TWDataCreateWithData(&json)).get()));
     const auto name2 = WRAPS(TWStoredKeyName(key2.get()));
     EXPECT_EQ(string(TWStringUTF8Bytes(name2.get())), "name");
@@ -463,19 +467,8 @@ TEST(TWStoredKey, storeAndImportJSONAES256Ctr) {
     const auto outFileNameStr = WRAPS(TWStringCreateWithUTF8Bytes(outFileName.c_str()));
     EXPECT_TRUE(TWStoredKeyStore(key.get(), outFileNameStr.get()));
 
-    // read contents of file
-    ifstream ifs(outFileName);
-    // get length of file:
-    ifs.seekg (0, ifs.end);
-    auto length = ifs.tellg();
-    ifs.seekg (0, ifs.beg);
-    EXPECT_TRUE(length > 20);
-
-    Data json(length);
-    size_t idx = 0;
-    // read the slow way, ifs.read gave some false warnings with codacy
-    while (!ifs.eof() && idx < static_cast<std::size_t>(length)) { char c = ifs.get(); json[idx++] = (uint8_t)c; }
-
+    auto json = readFileData(outFileName);
+    EXPECT_GT(json.size(), 20ul);
     const auto key2 = WRAP(TWStoredKey, TWStoredKeyImportJSON(WRAPD(TWDataCreateWithData(&json)).get()));
     const auto name2 = WRAPS(TWStoredKeyName(key2.get()));
     EXPECT_EQ(string(TWStringUTF8Bytes(name2.get())), "name");
@@ -486,21 +479,9 @@ TEST(TWStoredKey, storeAndImportJSON) {
     const auto outFileName = string(getTestTempDir() + "/TWStoredKey_store.json");
     const auto outFileNameStr = WRAPS(TWStringCreateWithUTF8Bytes(outFileName.c_str()));
     EXPECT_TRUE(TWStoredKeyStore(key.get(), outFileNameStr.get()));
-    //EXPECT_TRUE(filesystem::exists(outFileName));  // some linker issues with filesystem
-    
-    // read contents of file
-    ifstream ifs(outFileName);
-    // get length of file:
-    ifs.seekg (0, ifs.end);
-    auto length = ifs.tellg();
-    ifs.seekg (0, ifs.beg);
-    EXPECT_TRUE(length > 20);
-
-    Data json(length);
-    size_t idx = 0;
-    // read the slow way, ifs.read gave some false warnings with codacy 
-    while (!ifs.eof() && idx < static_cast<std::size_t>(length)) { char c = ifs.get(); json[idx++] = (uint8_t)c; }
 
+    auto json = readFileData(outFileName);
+    EXPECT_GT(json.size(), 20ul);
     const auto key2 = WRAP(TWStoredKey, TWStoredKeyImportJSON(WRAPD(TWDataCreateWithData(&json)).get()));
     const auto name2 = WRAPS(TWStoredKeyName(key2.get()));
     EXPECT_EQ(string(TWStringUTF8Bytes(name2.get())), "name");
@@ -678,6 +659,61 @@ TEST(TWStoredKey, encryptionParameters) {
     )");
 }
 
+TEST(TWStoredKey, storeInvalidPath) {
+    const auto key = createDefaultStoredKey();
+    const auto invalidPath = WRAPS(TWStringCreateWithUTF8Bytes("/non-existing/file/path.json"));
+    EXPECT_FALSE(TWStoredKeyStore(key.get(), invalidPath.get()));
+}
+
+TEST(TWStoredKey, storeWithTemporaryFileSuccess) {
+    const auto key = createDefaultStoredKey();
+    const auto outFileName = string(getTestTempDir() + "/TWStoredKey_storeWithTmp.json");
+    const auto tmpFileName = string(getTestTempDir() + "/TWStoredKey_storeWithTmp.json.tmp");
+    const auto outFileNameStr = WRAPS(TWStringCreateWithUTF8Bytes(outFileName.c_str()));
+    const auto tmpFileNameStr = WRAPS(TWStringCreateWithUTF8Bytes(tmpFileName.c_str()));
+
+    EXPECT_TRUE(TWStoredKeyStoreWithTemporaryFile(key.get(), outFileNameStr.get(), tmpFileNameStr.get()));
+
+    // The temp file must have been renamed away — only the final file should exist.
+    EXPECT_FALSE(ifstream(tmpFileName).is_open());
+
+    // The final file must be readable and parse as a valid StoredKey.
+    auto json = readFileData(outFileName);
+    EXPECT_GT(json.size(), 20ul);
+    const auto reloaded = WRAP(TWStoredKey, TWStoredKeyImportJSON(WRAPD(TWDataCreateWithData(&json)).get()));
+    ASSERT_NE(reloaded.get(), nullptr);
+    const auto name = WRAPS(TWStoredKeyName(reloaded.get()));
+    EXPECT_EQ(string(TWStringUTF8Bytes(name.get())), "name");
+}
+
+TEST(TWStoredKey, storeWithTemporaryFileInvalidPath) {
+    const auto key = createDefaultStoredKey();
+    const auto invalidPath = WRAPS(TWStringCreateWithUTF8Bytes("/non-existing/file/path.json"));
+    const auto tmpFileName = string(getTestTempDir() + "/TWStoredKey_storeWithTmpInvalidPath.json.tmp");
+    const auto tmpFileNameStr = WRAPS(TWStringCreateWithUTF8Bytes(tmpFileName.c_str()));
+
+    EXPECT_FALSE(TWStoredKeyStoreWithTemporaryFile(key.get(), invalidPath.get(), tmpFileNameStr.get()));
+}
+
+TEST(TWStoredKey, storeWithTemporaryFileInvalidTempPath) {
+    const auto key = createDefaultStoredKey();
+    const auto outFileName = string(getTestTempDir() + "/TWStoredKey_storeWithTmpInvalidTmp.json");
+    const auto outFileNameStr = WRAPS(TWStringCreateWithUTF8Bytes(outFileName.c_str()));
+    const auto invalidTmpPath = WRAPS(TWStringCreateWithUTF8Bytes("/non-existing/file/path.tmp"));
+
+    // Write known sentinel content to the destination file so we can verify it is untouched.
+    const string sentinel = "{\"sentinel\":true}";
+    { ofstream f(outFileName); f << sentinel; }
+
+    EXPECT_FALSE(TWStoredKeyStoreWithTemporaryFile(key.get(), outFileNameStr.get(), invalidTmpPath.get()));
+
+    // The destination file must be unchanged.
+    ifstream ifs(outFileName);
+    ASSERT_TRUE(ifs.is_open());
+    const string actual((istreambuf_iterator<char>(ifs)), istreambuf_iterator<char>());
+    EXPECT_EQ(actual, sentinel);
+}
+
 TEST(TWStoredKey, fixScryptWithEmptySaltAes256Ctr) {
     const auto filename = WRAPS(TWStringCreateWithUTF8Bytes((TESTS_ROOT + "/common/Keystore/Data/scrypt-empty-salt-aes-256-ctr.json").c_str()));
     const auto key = WRAP(TWStoredKey, TWStoredKeyLoad(filename.get()));