Skip to content

Commit 5052afd

Browse files
authored
[PM-28026] Lint when holding KeyStoreContext across await points (#551)
## 🎟️ Tracking https://bitwarden.atlassian.net/browse/PM-28026 ## 📔 Objective Clippy by default lints when trying to hold an RwLock/Mutex guard across an await point, but that doesn't apply to custom types by default. This PR configures clippy to also apply that lint to our own types that wrap lock guards, which at the moment is only `KeyStoreContext`. We only have two instances that needed fixing and both were in tests, but some of the SM code I wrote already had to deal with it by creating short lived contexts, so I assume it might be a problem in the future as well: https://github.com/bitwarden/sdk-internal/blob/main/bitwarden_license/bitwarden-sm/src/secrets/create.rs#L41-L64 The lint docs: https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_invalid_type This is what the lint looks like: <img width="1011" height="262" alt="image" src="https://github.com/user-attachments/assets/77f9eaa2-f46b-460a-81b7-627e440e3f7a" /> ## ⏰ Reminders before review - Contributor guidelines followed - All formatters and local linters executed and passed - Written new unit and / or integration tests where applicable - Protected functional changes with optionality (feature flags) - Used internationalization (i18n) for all UI strings - CI builds passed - Communicated to DevOps any deployment requirements - Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team ## 🦮 Reviewer guidelines <!-- Suggested interactions but feel free to use (or not) as you desire! --> - 👍 (`:+1:`) or similar for great changes - 📝 (`:memo:`) or ℹ️ (`:information_source:`) for notes or general info - ❓ (`:question:`) for questions - 🤔 (`:thinking:`) or 💭 (`:thought_balloon:`) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion - 🎨 (`:art:`) for suggestions / improvements - ❌ (`:x:`) or ⚠️ (`:warning:`) for more significant problems or concerns needing attention - 🌱 (`:seedling:`) or ♻️ (`:recycle:`) for future improvements or indications of technical debt - ⛏ (`:pick:`) for minor or nitpick changes
1 parent b033edd commit 5052afd

File tree

3 files changed

+68
-67
lines changed

3 files changed

+68
-67
lines changed

clippy.toml

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,10 @@
1-
allow-unwrap-in-tests=true
2-
allow-expect-in-tests=true
1+
allow-unwrap-in-tests = true
2+
allow-expect-in-tests = true
3+
4+
await-holding-invalid-types = [
5+
{ path = "bitwarden_crypto::KeyStoreContext", reason = """
6+
Contains a RwLock guard that will limit concurrency and even deadlock when held across await points.
7+
Instead, prefer to hold a reference to bitwarden_core::KeyStore and use its encrypt/decrypt methods directly.
8+
If you require access to KeyStoreContext, create short-lived KeyStoreContext instances as needed.
9+
""" },
10+
]

crates/bitwarden-vault/src/cipher/cipher_client/edit.rs

Lines changed: 50 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -462,58 +462,56 @@ mod tests {
462462
cipher_id: CipherId,
463463
name: &str,
464464
) {
465-
let mut ctx = store.context();
466-
467-
repository
468-
.set(
469-
cipher_id.to_string(),
470-
Cipher {
471-
id: Some(cipher_id),
472-
organization_id: None,
473-
folder_id: None,
474-
collection_ids: vec![],
475-
key: None,
476-
name: name.encrypt(&mut ctx, SymmetricKeyId::User).unwrap(),
477-
notes: None,
478-
r#type: CipherType::Login,
479-
login: Some(Login {
480-
username: Some("test@example.com")
481-
.map(|u| u.encrypt(&mut ctx, SymmetricKeyId::User))
482-
.transpose()
483-
.unwrap(),
484-
password: Some("password123")
485-
.map(|p| p.encrypt(&mut ctx, SymmetricKeyId::User))
486-
.transpose()
487-
.unwrap(),
488-
password_revision_date: None,
489-
uris: None,
490-
totp: None,
491-
autofill_on_page_load: None,
492-
fido2_credentials: None,
493-
}),
494-
identity: None,
495-
card: None,
496-
secure_note: None,
497-
ssh_key: None,
498-
favorite: false,
499-
reprompt: CipherRepromptType::None,
500-
organization_use_totp: true,
501-
edit: true,
502-
permissions: None,
503-
view_password: true,
504-
local_data: None,
505-
attachments: None,
506-
fields: None,
507-
password_history: None,
508-
creation_date: "2024-01-01T00:00:00Z".parse().unwrap(),
509-
deleted_date: None,
510-
revision_date: "2024-01-01T00:00:00Z".parse().unwrap(),
511-
archived_date: None,
512-
data: None,
513-
},
514-
)
515-
.await
516-
.unwrap();
465+
let cipher = {
466+
let mut ctx = store.context();
467+
468+
Cipher {
469+
id: Some(cipher_id),
470+
organization_id: None,
471+
folder_id: None,
472+
collection_ids: vec![],
473+
key: None,
474+
name: name.encrypt(&mut ctx, SymmetricKeyId::User).unwrap(),
475+
notes: None,
476+
r#type: CipherType::Login,
477+
login: Some(Login {
478+
username: Some("test@example.com")
479+
.map(|u| u.encrypt(&mut ctx, SymmetricKeyId::User))
480+
.transpose()
481+
.unwrap(),
482+
password: Some("password123")
483+
.map(|p| p.encrypt(&mut ctx, SymmetricKeyId::User))
484+
.transpose()
485+
.unwrap(),
486+
password_revision_date: None,
487+
uris: None,
488+
totp: None,
489+
autofill_on_page_load: None,
490+
fido2_credentials: None,
491+
}),
492+
identity: None,
493+
card: None,
494+
secure_note: None,
495+
ssh_key: None,
496+
favorite: false,
497+
reprompt: CipherRepromptType::None,
498+
organization_use_totp: true,
499+
edit: true,
500+
permissions: None,
501+
view_password: true,
502+
local_data: None,
503+
attachments: None,
504+
fields: None,
505+
password_history: None,
506+
creation_date: "2024-01-01T00:00:00Z".parse().unwrap(),
507+
deleted_date: None,
508+
revision_date: "2024-01-01T00:00:00Z".parse().unwrap(),
509+
archived_date: None,
510+
data: None,
511+
}
512+
};
513+
514+
repository.set(cipher_id.to_string(), cipher).await.unwrap();
517515
}
518516

519517
#[tokio::test]

crates/bitwarden-vault/src/folder/edit.rs

Lines changed: 8 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -76,19 +76,14 @@ mod tests {
7676
folder_id: FolderId,
7777
name: &str,
7878
) {
79-
repository
80-
.set(
81-
folder_id.to_string(),
82-
Folder {
83-
id: Some(folder_id),
84-
name: name
85-
.encrypt(&mut store.context(), SymmetricKeyId::User)
86-
.unwrap(),
87-
revision_date: "2024-01-01T00:00:00Z".parse().unwrap(),
88-
},
89-
)
90-
.await
91-
.unwrap();
79+
let folder = Folder {
80+
id: Some(folder_id),
81+
name: name
82+
.encrypt(&mut store.context(), SymmetricKeyId::User)
83+
.unwrap(),
84+
revision_date: "2024-01-01T00:00:00Z".parse().unwrap(),
85+
};
86+
repository.set(folder_id.to_string(), folder).await.unwrap();
9287
}
9388

9489
#[tokio::test]

0 commit comments

Comments
 (0)