diff --git a/src/Simplex/Chat/Library/Commands.hs b/src/Simplex/Chat/Library/Commands.hs index d5c6178ff6..2941802e47 100644 --- a/src/Simplex/Chat/Library/Commands.hs +++ b/src/Simplex/Chat/Library/Commands.hs @@ -1503,8 +1503,7 @@ processChatCommand cxt nm = \case pure $ CRWallet user True paths profiles APIWalletBind acct_ -> withUser $ \user -> do seed <- deviceSeed - bound <- withFastStore' $ \db -> bindAccountIndex db user (wsId seed) acct_ - unless bound $ throwCmdError "another profile uses this account" + withFastStore' (\db -> bindAccountIndex db user (wsId seed) acct_) >>= either throwCmdError pure processChatCommand cxt nm APIWallet APIWalletCreate -> withUser $ \_ -> do g <- asks random @@ -6143,10 +6142,12 @@ chatCommandP = quotedP = safeDecodeUtf8 <$> (A.char '"' *> A.takeTill (== '"') <* A.char '"') text1P = safeDecodeUtf8 <$> A.takeTill (== ' ') char_ = optional . A.char - -- BIP-32 hardens at 2^31, and Word32 would wrap + -- BIP-32 hardens at 2^31, and Word32 would wrap. Digits are counted before + -- they are read, as reading a very long number is not free. keyIndexP = do - i <- A.decimal :: Parser Integer - if i < 0x80000000 then pure (fromIntegral i) else fail "key index too large" + ds <- A.takeWhile1 isDigit + let i = read (B.unpack ds) :: Integer + if B.length ds <= 10 && i < 0x80000000 then pure (fromIntegral i) else fail "key index too large" displayNameP :: Parser Text displayNameP = safeDecodeUtf8 <$> displayNameP_ diff --git a/src/Simplex/Chat/Store/Wallets.hs b/src/Simplex/Chat/Store/Wallets.hs index 6934921504..99eb4b226e 100644 --- a/src/Simplex/Chat/Store/Wallets.hs +++ b/src/Simplex/Chat/Store/Wallets.hs @@ -73,13 +73,21 @@ createSeed :: DB.Connection -> ByteString -> IO Bool createSeed db entropy = getDeviceSeed db >>= \case Just _ -> pure False - Nothing -> True <$ DB.execute db "INSERT INTO wallet_seeds (seed) VALUES (?)" (Only entropy) + Nothing -> True <$ DB.execute db "INSERT INTO wallet_seeds (seed) VALUES (?)" (Only $ DB.Binary entropy) --- | Without an account the next free one is taken. False if another profile --- holds the account asked for. -bindAccountIndex :: DB.Connection -> User -> SeedId -> Maybe AccountIndex -> IO Bool -bindAccountIndex db User {userId} sId = \case - Nothing -> True <$ (takeAccountIndex db sId >>= bindUser db userId sId) +-- | Without an account the next free one is taken, which a profile that has +-- one does not need: moving to another account is asked for by number. +bindAccountIndex :: DB.Connection -> User -> SeedId -> Maybe AccountIndex -> IO (Either String ()) +bindAccountIndex db user@User {userId} sId = \case + Nothing -> + getAccountIndex db user >>= \case + Just _ -> pure $ Left "this profile already has an account" + Nothing -> do + acct <- takeAccountIndex db sId + -- BIP-32 hardens at 2^31, and every index above it is the same key again + if acct >= 0x80000000 + then pure $ Left "no free account on this key" + else Right () <$ bindUser db userId sId acct Just acct -> do taken <- maybeFirstRow fromOnly $ @@ -88,12 +96,12 @@ bindAccountIndex db User {userId} sId = \case "SELECT 1 FROM users WHERE wallet_seed_id = ? AND wallet_account_index = ? AND user_id != ?" (sId, fromIntegral acct :: Int64, userId) case (taken :: Maybe Int64) of - Just _ -> pure False + Just _ -> pure $ Left "another profile uses this account" Nothing -> do bindUser db userId sId (fromIntegral acct) -- the counter moves past it, so the next profile is not handed the same one setNextAccountIndex db sId (fromIntegral acct + 1) - pure True + pure $ Right () -- | So two profiles cannot be handed the same account. takeAccountIndex :: DB.Connection -> SeedId -> IO Int64 diff --git a/tests/WalletTests.hs b/tests/WalletTests.hs index 9434d9ca63..cd509e5473 100644 --- a/tests/WalletTests.hs +++ b/tests/WalletTests.hs @@ -56,6 +56,7 @@ walletTests = do it "exports the secret of any name key" testWalletExportDerivedSecret it "deletes the key, and a key can be imported again" testWalletDelete it "binds a profile to the account it had" testWalletBind + it "binds a profile once, and only to an account BIP-32 can harden" testWalletBindLimits it "needs no import when the database was backed up after the key" testWalletBackupAfterKey it "rebinds by index when the database was backed up before the key" testWalletBackupBeforeKey it "discards a key imported before the database is restored" testWalletImportThenRestore @@ -245,3 +246,24 @@ testWalletImportThenRestore ps = withNewTestChat ps "alice" aliceProfile $ \alic alice ##> "/_wallet bind" rows <- nameRows alice map fst rows `shouldBe` ["m/44'/60'/0'/0/0", "m/44'/60'/0'/0/1"] + +testWalletBindLimits :: HasCallStack => TestParams -> IO () +testWalletBindLimits ps = withNewTestChat ps "alice" aliceProfile $ \alice -> do + alice ##> ("/_wallet import " <> B.unpack testPhrase) + alice <## "no account for this profile" + alice ##> "/_wallet bind" + _ <- nameRows alice + -- a profile that has an account asks for another one by number + alice ##> "/_wallet bind" + alice <## "bad chat command: this profile already has an account" + alice ##> "/create user alisa" + showActiveUser alice "alisa" + alice ##> "/_wallet bind 2147483647" + rows <- nameRows alice + alice <## "also on same seed: alice" + map fst rows `shouldBe` ["m/44'/60'/2147483647'/0/0", "m/44'/60'/2147483647'/0/1"] + -- the counter is past what BIP-32 can harden, where it would repeat account 0 + alice ##> "/create user carol" + showActiveUser alice "carol" + alice ##> "/_wallet bind" + alice <## "bad chat command: no free account on this key"