security: batch of fixes #95
@@ -214,7 +214,7 @@ impl OperatorSession {
|
||||
use crate::db::schema::evm_wallet_access;
|
||||
for entry in entries {
|
||||
diesel::delete(evm_wallet_access::table)
|
||||
.filter(evm_wallet_access::wallet_id.eq(entry))
|
||||
.filter(evm_wallet_access::id.eq(entry))
|
||||
.execute(&mut *conn)
|
||||
.await?;
|
||||
}
|
||||
@@ -289,3 +289,142 @@ impl OperatorSession {
|
||||
Ok(clients)
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use crate::db::{self, models::NewEvmWalletAccess, schema::evm_wallet_access};
|
||||
use diesel::{ExpressionMethods as _, QueryDsl as _, SelectableHelper};
|
||||
use diesel_async::{AsyncConnection, RunQueryDsl};
|
||||
|
||||
/// Regression test: revocation must delete by access-entry `id`, not by `wallet_id`.
|
||||
///
|
||||
/// Before the fix, revoking entry_id=1 would delete all rows where wallet_id=1,
|
||||
/// wiping out every client's access to wallet #1.
|
||||
#[tokio::test]
|
||||
async fn revoke_deletes_by_entry_id_not_wallet_id() {
|
||||
use crate::db::models::EvmWalletAccess;
|
||||
|
||||
let pool = db::create_test_pool().await;
|
||||
let mut conn = pool.get().await.expect("pool connection");
|
||||
|
||||
// Insert two access entries for the same wallet but different clients.
|
||||
// entry A: id will be 1, wallet_id=1, client_id=10
|
||||
// entry B: id will be 2, wallet_id=1, client_id=20
|
||||
let entry_a = diesel::insert_into(evm_wallet_access::table)
|
||||
.values(NewEvmWalletAccess {
|
||||
wallet_id: 1,
|
||||
client_id: 10,
|
||||
})
|
||||
.returning(EvmWalletAccess::as_select())
|
||||
.get_result(&mut *conn)
|
||||
.await
|
||||
.expect("insert entry A");
|
||||
|
||||
let entry_b = diesel::insert_into(evm_wallet_access::table)
|
||||
.values(NewEvmWalletAccess {
|
||||
wallet_id: 1,
|
||||
client_id: 20,
|
||||
})
|
||||
.returning(EvmWalletAccess::as_select())
|
||||
.get_result(&mut *conn)
|
||||
.await
|
||||
.expect("insert entry B");
|
||||
|
||||
// Revoke only entry A by its primary key id.
|
||||
conn.transaction(async |conn| {
|
||||
diesel::delete(evm_wallet_access::table)
|
||||
.filter(evm_wallet_access::id.eq(entry_a.id))
|
||||
.execute(&mut *conn)
|
||||
.await
|
||||
})
|
||||
.await
|
||||
.expect("revoke entry A");
|
||||
|
||||
// Entry A must be gone.
|
||||
let gone = evm_wallet_access::table
|
||||
.filter(evm_wallet_access::id.eq(entry_a.id))
|
||||
.count()
|
||||
.get_result::<i64>(&mut *conn)
|
||||
.await
|
||||
.expect("count entry A");
|
||||
assert_eq!(gone, 0, "revoked entry must be deleted");
|
||||
|
||||
// Entry B (same wallet, different client) must still exist.
|
||||
let still_there = evm_wallet_access::table
|
||||
.filter(evm_wallet_access::id.eq(entry_b.id))
|
||||
.count()
|
||||
.get_result::<i64>(&mut *conn)
|
||||
.await
|
||||
.expect("count entry B");
|
||||
assert_eq!(still_there, 1, "unrelated entry must not be deleted");
|
||||
}
|
||||
|
||||
/// Regression test: when entry_id and wallet_id differ, only the correct row is removed.
|
||||
///
|
||||
/// This specifically catches the case where entry.id=5 and wallet_id=1 are different values;
|
||||
/// the old bug would delete by wallet_id, potentially matching a completely different entry.
|
||||
#[tokio::test]
|
||||
async fn revoke_with_mismatched_wallet_and_entry_ids() {
|
||||
use crate::db::models::EvmWalletAccess;
|
||||
|
||||
let pool = db::create_test_pool().await;
|
||||
let mut conn = pool.get().await.expect("pool connection");
|
||||
|
||||
// Insert entries to force auto-increment IDs to diverge from wallet_ids.
|
||||
// We'll insert 5 placeholder entries first so that the real entry gets id=6.
|
||||
for i in 1_i32..=5 {
|
||||
diesel::insert_into(evm_wallet_access::table)
|
||||
.values(NewEvmWalletAccess {
|
||||
wallet_id: 99,
|
||||
client_id: i,
|
||||
})
|
||||
.execute(&mut *conn)
|
||||
.await
|
||||
.expect("insert placeholder");
|
||||
}
|
||||
|
||||
// Real target: wallet_id=1, will get id=6.
|
||||
let target = diesel::insert_into(evm_wallet_access::table)
|
||||
.values(NewEvmWalletAccess {
|
||||
wallet_id: 1,
|
||||
client_id: 1,
|
||||
})
|
||||
.returning(EvmWalletAccess::as_select())
|
||||
.get_result(&mut *conn)
|
||||
.await
|
||||
.expect("insert target");
|
||||
|
||||
// Sanity: target.id != target.wallet_id
|
||||
assert_ne!(
|
||||
target.id, target.wallet_id,
|
||||
"test prerequisite: id and wallet_id must differ"
|
||||
);
|
||||
|
||||
// Revoke by entry id.
|
||||
conn.transaction(async |conn| {
|
||||
diesel::delete(evm_wallet_access::table)
|
||||
.filter(evm_wallet_access::id.eq(target.id))
|
||||
.execute(&mut *conn)
|
||||
.await
|
||||
})
|
||||
.await
|
||||
.expect("revoke target");
|
||||
|
||||
let remaining = evm_wallet_access::table
|
||||
.filter(evm_wallet_access::id.eq(target.id))
|
||||
.count()
|
||||
.get_result::<i64>(&mut *conn)
|
||||
.await
|
||||
.expect("count target");
|
||||
assert_eq!(remaining, 0, "target must be deleted by its entry id");
|
||||
|
||||
// Placeholders for wallet_id=99 must be untouched.
|
||||
let placeholders = evm_wallet_access::table
|
||||
.filter(evm_wallet_access::wallet_id.eq(99))
|
||||
.count()
|
||||
.get_result::<i64>(&mut *conn)
|
||||
.await
|
||||
.expect("count placeholders");
|
||||
assert_eq!(placeholders, 5, "unrelated entries must survive");
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user