api::core::ciphers::put_cipher_share_selected does not use cipher_ids #636

New Issue

OVERLORD · 2025-10-09T16:43:04+03:00

OVERLORD commented

2025-10-09 16:43:04 +03:00

Originally created by @zacknewman on GitHub.

This may not be a bug, but it could be if cipher_ids is actually important. Anyway, cipher_ids in api::core::ciphers::put_cipher_share_selected is never read from. Is this accidental? What's the point of pushing IDs into it without ever reading the IDs?

Possible fix if that variable is unimportant:

#[put("/ciphers/share", data = "<data>")]
async fn put_cipher_share_selected(
    data: JsonUpcase<ShareSelectedCipherData>,
    headers: Headers,
    mut conn: DbConn,
    nt: Notify<'_>,
) -> EmptyResult {
    let mut data: ShareSelectedCipherData = data.into_inner().data;

    if data.Ciphers.is_empty() {
        err!("You must select at least one cipher.")
    }

    if data.CollectionIds.is_empty() {
        err!("You must select at least one collection.")
    }

    for cipher in data.Ciphers.iter() {
        if cipher.Id.is_none() {
            err!("Request missing ids field");
        }
    }

    while let Some(cipher) = data.Ciphers.pop() {
        let mut shared_cipher_data = ShareCipherData {
            Cipher: cipher,
            CollectionIds: data.CollectionIds.clone(),
        };

        match shared_cipher_data.Cipher.Id.take() {
            Some(id) => share_cipher_by_uuid(&id, shared_cipher_data, &headers, &mut conn, &nt).await?,
            None => err!("Request missing ids field"),
        };
    }
    Ok(())
}

Originally created by @zacknewman on GitHub. This may not be a bug, but it could be if `cipher_ids` is actually important. Anyway, `cipher_ids` in [`api::core::ciphers::put_cipher_share_selected`](https://github.com/dani-garcia/vaultwarden/blob/main/src/api/core/ciphers.rs#L852) is never read from. Is this accidental? What's the point of `push`ing IDs into it without ever reading the IDs? Possible fix if that variable is unimportant: ```rust #[put("/ciphers/share", data = "<data>")] async fn put_cipher_share_selected( data: JsonUpcase<ShareSelectedCipherData>, headers: Headers, mut conn: DbConn, nt: Notify<'_>, ) -> EmptyResult { let mut data: ShareSelectedCipherData = data.into_inner().data; if data.Ciphers.is_empty() { err!("You must select at least one cipher.") } if data.CollectionIds.is_empty() { err!("You must select at least one collection.") } for cipher in data.Ciphers.iter() { if cipher.Id.is_none() { err!("Request missing ids field"); } } while let Some(cipher) = data.Ciphers.pop() { let mut shared_cipher_data = ShareCipherData { Cipher: cipher, CollectionIds: data.CollectionIds.clone(), }; match shared_cipher_data.Cipher.Id.take() { Some(id) => share_cipher_by_uuid(&id, shared_cipher_data, &headers, &mut conn, &nt).await?, None => err!("Request missing ids field"), }; } Ok(()) } ```

OVERLORD added the enhancement label 2025-10-09 16:43:04 +03:00

OVERLORD closed this issue

2025-10-09 16:43:05 +03:00

OVERLORD commented

2025-10-09 16:43:06 +03:00

@BlackDex commented on GitHub:

Looks like a leftover indeed. I'll check it out further but i think you are right.

@BlackDex commented on GitHub: Looks like a leftover indeed. I'll check it out further but i think you are right.

OVERLORD commented

2025-10-09 16:43:07 +03:00

@BlackDex commented on GitHub:

btw strange that clippy doesn't find this. It should see that it's never read after written.

@BlackDex commented on GitHub: btw strange that clippy doesn't find this. It should see that it's never read after written.

OVERLORD commented

2025-10-09 16:43:07 +03:00

@zacknewman commented on GitHub:

clippy::nursery (https://rust-lang.github.io/rust-clippy/master/index.html#/collection_is_never_read?groups=nursery) contains the necessary lint. On my personal fork of Vaultwarden, I deny all lints and allow specific lints (e.g., implicit_return), and it has helped me clean up some stuff including detecting the &mut DbConn I mentioned earlier.

#![deny(unsafe_code, unused, warnings, clippy::all, clippy::cargo, clippy::complexity, clippy::correctness, clippy::nursery, clippy::pedantic, clippy::perf, clippy::restriction, clippy::style, clippy::suspicious)]

@zacknewman commented on GitHub: clippy::nursery (https://rust-lang.github.io/rust-clippy/master/index.html#/collection_is_never_read?groups=nursery) contains the necessary lint. On my personal fork of Vaultwarden, I deny all lints and allow specific lints (e.g., implicit_return), and it has helped me clean up some stuff including detecting the &mut DbConn I mentioned earlier. #![deny(unsafe_code, unused, warnings, clippy::all, clippy::cargo, clippy::complexity, clippy::correctness, clippy::nursery, clippy::pedantic, clippy::perf, clippy::restriction, clippy::style, clippy::suspicious)]

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: starred/vaultwarden#636