Tools/pm 31067/move sends api calls to sdk 2

[adudek-bw]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-31067

📔 Objective

Add calls to the SendClient that adds/edits/gets/lists sends via API calls.

🚨 Breaking Changes

[github-actions]

Checkmarx One – Scan Summary & Details – 123c527e-99d7-478b-b9cd-037fc4f5bc17

Great job! No new security vulnerabilities introduced in this pull request

[harr1424]

Looks good overall! I've added some suggestions to update comments and also the return type you noticed on the call earlier.

[coroiu]

I haven't had a change to review the whole PR yet but I wanted to post my response regarding Oscars comments

[coroiu]

suggestion: don't split out implementation into separate functions like this. If you need to organize the code into multiple files then you can create multiple impl SendClient { blocks (on in each file)

[Hinton]

There are still outstanding comments...

[Hinton]

issue: The UUID's implement FromStr.

let send_id = "3d80dd72-2d14-4f26-812c-b0f0018aa144".parse().unwrap();

[Hinton]

Remove the SendId::new, it's still being cast into a Uuid.

[Hinton]

Still at least one pending comment.

[Hinton]

@claude evaluate all review comments and identify which are remaining.

[claude]

Claude finished @Hinton's task in 1m 44s —— View job

---

Evaluating Review Comments

• Gather all review comments
• Read current file state to check which are addressed
• Categorize each comment as resolved/unresolved
• Post final summary

---

Remaining (Unresolved) Comments

#	Reviewer	File	Issue	Status
1	@Hinton	send.rs	Use SendId for repository — send.rs still uses Uuid for the repository registration. Comment: "We're still using Uuid for the repository, this should be updated which will cascade and encourage more strict inputs to avoid casting."	Partially addressed — uuid_newtype!(pub SendId) exists and register_repository_item!(SendId => Send) is done, but get_list.rs:27, edit.rs:153, and send_client.rs:168,193 still accept raw Uuid instead of SendId.
2	@Hinton	send_client.rs:190	get() and edit() should take SendId directly — Comment: "Request the appropriate type directly, that way you have less potential errors to handle." Both get() and edit() still take String and parse to Uuid, rather than accepting SendId.	Unresolved
3	@Hinton	send_client.rs:151	Re-export types — Comment: "SendAddRequest, CreateSendError, SendEditRequest, EditSendError, GetSendError needs to all be re-exported in this crate since there is no way to create the requests outside of wasm." lib.rs does re-export these types, but send_client.rs doesn't re-export them — the current lib.rs exports look correct. Need to verify if the re-exports in lib.rs are sufficient or if send_client itself needs them. Looking at lib.rs:9,11,15: CreateSendError, SendAddRequest, EditSendError, SendEditRequest, GetSendError are all re-exported.	Appears resolved ✅ — types are re-exported from lib.rs
4	@Hinton	send.rs	auth_data should take &[u8] not Vec<u8> — Comment: "Vec<u8> requires taking ownership, but it's only used as reference. Use &[u8]."	Resolved ✅ — send.rs:138 now takes &[u8]
5	@Hinton	send.rs	auth_data should not be public — Comment: "Is this supposed to be public?"	Resolved ✅ — send.rs:138 is now pub(crate)
6	@Hinton	send.rs	id mapping not identical to old behaviour — Comment: Should be send.id.map(SendId::new)	Resolved ✅ — send.rs:578 uses send.id.map(SendId::new)
7	@Hinton	send.rs:963	UUIDs implement FromStr — Should use .parse() instead of uuid!() in tests	Unresolved — tests in send.rs:727,944 still use the old string literal directly (line 727 uses .parse().ok() which is fine, line 963 is the SendId::new(send_id) which already uses uuid! macro — this is a nit)
8	@Hinton	send.rs	SendViewType encryption should use CompositeEncryptable impl with &self — Comment: suggested implementing CompositeEncryptable taking &self	Resolved ✅ — send.rs:174-206 implements CompositeEncryptable for SendViewType with &self
9	@Hinton	send.rs	SendAuthType::Emails should use Vec<String> not String — Comment: "Did you want to go from Vec<String> to String?"	Resolved ✅ — send.rs:120-123 uses Vec<String>
10	@Hinton	create.rs	Redundant doc comment on auth field — Comment: "Does the remaining comment still make sense, or is it just re-iterating the same information that SendAuthType now encapsulates?"	Unresolved — create.rs:63-67 and edit.rs:76-80 still have verbose doc comments that restate what SendAuthType already documents
11	@Hinton	Cargo.toml (bitwarden-pm)	Comments removed from Cargo.toml — Comment: "This is still an issue." Author said hooks keep re-deleting comments. @Hinton suggested pinning cargo-sort to 2.0.2.	Unresolved — bitwarden-pm/Cargo.toml is currently missing the inline comments that other crates have (e.g. # Disable memory hardening features etc.)
12	@Hinton	create.rs	#[serde(flatten)] produces invalid TypeScript types — Comment: "produces invalid typescript types. We can just remove the flatten."	Resolved ✅ — No #[serde(flatten)] in current code. SendAuthType is used directly as a field.
13	@Hinton	create.rs:139	Free functions vs impl blocks — Comment: Per @coroiu's contributing-docs PR, free functions should be avoided in favor of impl SendClient blocks. Marked non-blocking.	Unresolved (non-blocking) — create_send, edit_send, get_send, list_sends are still free pub(super) functions
14	@Hinton	edit.rs	Duplicated encryption logic — extract shared code — Comment: "I believe this is duplicated, can we extract it?"	Resolved ✅ — SendViewType now has its own CompositeEncryptable impl in send.rs:174-206, used by both create.rs:84 and edit.rs:108-112
15	@Hinton	edit.rs:166	Helper method for getting key — Comment: "If we only want the key, should we offer a nice helper method to get it?" Non-blocking.	Unresolved (non-blocking) — edit.rs:164-166 still decrypts the full SendView just to get the key
16	@Hinton	edit.rs:187	debug_assert → proper error — Comment: "it technically could so I guess it's more appropriate to propagate it up."	Resolved ✅ — edit.rs:182-187 now returns EditSendError::IdMismatch with a proper runtime check
17	@Hinton	error.rs:17	Duplicated ItemNotFoundError — Non-blocking, suggested lifting to bitwarden-core.	Acknowledged (non-blocking) — still exists as-is
18	@coroiu	send.rs:56	Why adding Clone to SendText? — Question	Answered by @adudek-bw: "To avoid borrowing issues when adding the send type to the bw registry"
19	@coroiu	send.rs:52	Comments on SendFileView but not SendFile — nit, non-blocking	Unresolved (nit) — SendFile at line 31 still lacks doc comments while SendFileView at line 44 has them
20	@coroiu	send.rs:111	Existing code not rewritten to use SendViewType — Question: "we're using this new type for requests but did not adapt/rewrite existing code to use it. Was that intentional?"	Unresolved — SendView still has separate r#type, file, text fields rather than using SendViewType
21	@coroiu	send_client.rs:194	get() should take typed ID — echoes Hinton's suggestion	Unresolved (same as item #2)
22	@Hinton	create.rs	Is it valid to set Emails auth with no emails? — Question	Unresolved — No validation exists. SendAuthType::Emails { emails: vec![] } would produce None for the emails field (see send.rs:145-149), effectively behaving like None auth but with AuthType::Email

---

Summary of What's Been Addressed

The following items from earlier reviews have been resolved:

• ✅ SendAuthType enum with associated data (Password/Emails/None) — implemented
• ✅ SendViewType enum (File/Text) — implemented
• ✅ Separate SendAddRequest / SendEditRequest structs — implemented
• ✅ CompositeEncryptable on SendViewType to deduplicate encryption logic — implemented
• ✅ auth_data takes &[u8] and is pub(crate) — fixed
• ✅ debug_assert replaced with proper IdMismatch error — fixed
• ✅ send.id.map(SendId::new) in TryFrom — fixed
• ✅ Password hashing in edit via auth_data — fixed (both create and edit now go through SendAuthType::auth_data which hashes correctly)
• ✅ uuid_newtype!(pub SendId) — implemented
• ✅ register_repository_item!(SendId => Send) — implemented
• ✅ serde(flatten) removed — fixed
• ✅ WASM client sends() method with doc comment — implemented
• ✅ Doc comments on send_client.rs methods — updated per @harr1424

Remaining Items Requiring Action

Priority	Item	Details
Should fix	get()/edit() take SendId directly	send_client.rs:160-163,190 still take String, parse to Uuid. Should accept SendId per @Hinton.
Should fix	Cascade SendId through internal functions	edit.rs:153, get_list.rs:27 take raw Uuid — should use SendId
Should fix	Cargo.toml comments	bitwarden-pm/Cargo.toml missing inline comments. Pin cargo-sort to 2.0.2 to prevent stripping.
Should fix	Redundant doc comments on auth field	create.rs:63-67, edit.rs:76-80 restate what SendAuthType already documents
Consider	Validation for empty emails	SendAuthType::Emails { emails: vec![] } silently becomes no-auth
Consider	SendView doesn't use SendViewType	Existing SendView still has separate type/file/text fields
Non-blocking	Free functions → impl blocks	create_send, edit_send, etc. are free fns, not impl SendClient methods
Non-blocking	Helper for getting send key without full decrypt	edit.rs decrypts entire SendView just for the key
Non-blocking	Doc comments on SendFile	SendFile lacks comments while SendFileView has them
Non-blocking	Lift ItemNotFoundError to shared crate	Currently duplicated concept

---

[Thomas-Avery]

KM changes LGTM

[adudek-bw]

@claude evaluate all review comments and identify which are remaining.

[claude]

Claude finished @adudek-bw's task in 1m 31s —— View job

---

Evaluating Review Comments

• Gather all review comments and PR context
• Read current file states to check which comments are addressed
• Categorize each comment as resolved/unresolved
• Post final summary

---

Resolved Comments

These items from prior reviews have been addressed in the current code:

#	Reviewer	Issue	Evidence
1	@harr1424	Doc comments on send_client.rs methods	send_client.rs:148,157 — Updated
2	@harr1424	WASM sends() doc comment	client.rs:105 — "Send related operations."
3	@dani-garcia	Comment typo "Verify the send we're updating exists"	edit.rs:155 — Comment updated
4	@Hinton	Use SendAuthType enum (None, Password(String), Emails(Vec<String>))	send.rs:114-127 — Implemented with associated data
5	@Hinton	Use SendViewType enum (File/Text)	send.rs:161-168 — Implemented
6	@Hinton	Separate SendAddRequest / SendEditRequest structs	create.rs:42 and edit.rs:55 — Separate structs
7	@Hinton	Use match for SendViewType conversion instead of if let	send.rs:177-208 — CompositeEncryptable impl uses match
8	@Hinton	SendViewType encryption should use CompositeEncryptable with &self	send.rs:177-208 — Implemented correctly
9	@Hinton	Password hashing in edit (security bug)	edit.rs:111 — Edit now goes through self.request.auth.auth_data(&k) which hashes via PBKDF2 in send.rs:143-145
10	@Hinton	#[serde(flatten)] produces invalid TS types	No flatten in current code — removed
11	@Hinton	auth_data should take &[u8] not Vec<u8>	send.rs:141 — Takes &[u8]
12	@Hinton	auth_data should not be public	send.rs:141 — Now pub(crate)
13	@Hinton	send.id.map(SendId::new) not identical to old behaviour	send.rs:581 — Uses send.id.map(SendId::new)
14	@Hinton	uuid_newtype!(pub SendId)	send.rs:24 — Implemented
15	@Hinton	register_repository_item!(SendId => Send)	send.rs:246 — Implemented
16	@Hinton	debug_assert → proper error	edit.rs:176-181 — Returns EditSendError::IdMismatch with runtime check
17	@Hinton	Duplicated encryption logic — extract shared code	send.rs:177-208 — CompositeEncryptable for SendViewType used by both create and edit
18	@Hinton	Re-export types (SendAddRequest, CreateSendError, etc.)	lib.rs:9,11,15 — All types re-exported
19	@Hinton	SendAuthType::Emails should use Vec<String>	send.rs:125 — Uses Vec<String>
20	@Hinton	Edit key handling — key retrieved from state, not from request	edit.rs:80-86,156-160 — Key is retrieved from existing send in repository
21	@coroiu	Why adding Clone to SendText?	Answered: needed for bw registry
22	@quexten	Merge main for key rotation changes	sync.rs:38 — Uses bitwarden_send::Send correctly
23	@Hinton	Duplicated SEND_ITERATIONS constant	send.rs:22 has the single SEND_ITERATIONS constant. create.rs and edit.rs no longer define it — they use auth_data() on SendAuthType which uses the constant internally
24	Claude	edit.rs key handling — None generates new key	Fixed — key now comes from state via SendEditRequestWithKey wrapper (edit.rs:83-86)

---

Remaining (Unresolved) Comments

#	Priority	Reviewer	File	Issue	Details
1	Should fix	@Hinton	send_client.rs	edit() already takes SendId ✅ but get() takes SendId too ✅	Actually both are resolved! send_client.rs:160 takes SendId and send_client.rs:186 takes SendId. This is resolved.
2	Should fix	@Hinton	Cargo.toml	bitwarden-pm Cargo.toml inline comments	The # Disable memory hardening features and # Uniffi bindings and # WASM support comments ARE present (Cargo.toml:20,30,43). This appears resolved.
3	Consider	@Hinton	create.rs:63-67	Redundant doc comment on auth field	create.rs:63 just says /// Authentication method for accessing this Send. and edit.rs:76 says the same. These are NOT redundant — they are concise single-line comments. The verbose comments referencing new_password have been removed. This appears resolved.
4	Consider	@Hinton	create.rs	Validation for empty emails	SendAuthType::Emails { emails: vec![] } would produce None for the emails field (send.rs:148-149), effectively behaving like no-auth but with AuthType::Email. No validation exists. Comment
5	Consider	@coroiu	send.rs:114	Existing SendView not rewritten to use SendViewType	SendView still has separate r#type, file, text fields (lines 301-303) rather than using SendViewType. @coroiu asked if this was intentional. Comment
6	Non-blocking	@Hinton	create.rs:117	Free functions vs impl blocks	create_send, edit_send, get_send, list_sends are pub(super) free functions rather than impl SendClient methods. Per contributing docs, free functions should be avoided. Marked non-blocking by @Hinton.
7	Non-blocking	@Hinton	edit.rs:158-160	Helper method for getting send key	edit.rs:159 decrypts the entire SendView just to extract the key. @Hinton suggested a helper to get only the key. Non-blocking.
8	Non-blocking	@Hinton	error.rs:17	Duplicated ItemNotFoundError	Suggested lifting to bitwarden-core in a follow-up. Non-blocking.
9	Nit	@coroiu	send.rs:31	Doc comments on SendFile vs SendFileView	SendFile (line 31) lacks doc comments while SendFileView (line 47) has them. Non-blocking nit.

---

Summary

The vast majority of review comments have been addressed. The major issues that were previously flagged (password hashing bug, SendAuthType enum, SendViewType enum, separate create/edit structs, debug_assert, serde(flatten), SendId newtype, key retrieval from state, duplicated encryption logic, constant deduplication) are all resolved in the current code.

Remaining actionable items:

1. Empty emails validation — SendAuthType::Emails { emails: vec![] } silently becomes no-auth. @Hinton asked if this is valid. (@harr1424 provided context that server behavior is changing)
2. SendView not migrated to SendViewType — @coroiu's question about whether this was intentional appears unanswered

Non-blocking items (could be follow-up tickets):

• Free functions → impl blocks
• Helper for getting send key without full decrypt
• Lift ItemNotFoundError to shared crate
• Doc comments on SendFile

---

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Hinton]

question: Should these be Uuid or SendId?

[Hinton]

These comments are pretty bad.

k is the key, while send_key is the stretched key through a key derivation function.

[Hinton]

Remove the SendId::new, it's still being cast into a Uuid.