serenity-rs / serenity

A Rust library for the Discord API.
https://discord.gg/serenity-rs
ISC License
4.77k stars 581 forks source link

Attachment names string treatment is not consistent between different functions #2635

Closed asibahi closed 2 days ago

asibahi commented 11 months ago

Serenity version: I am using poise 0.6.0

Rust version (rustc -V): 1.74

Minimal test case if possible:

    // inside a command function
    let attachment_name = format!("{}'s_{}.png", "some", "one"); // mind the apostrophe

    let attachment = serenity::CreateAttachment::bytes(image_data, &attachment_name); // ๐Ÿ‘ˆ first use here

    let embed = serenity::CreateEmbed::new()
         // other things here
        .attachment(attachment_name); // ๐Ÿ‘ˆ second use here

    let reply = poise::CreateReply::default()
        .attachment(attachment)
        .embed(embed);
}

the apostrophe in attachment_name causes the two functions to give different output somehow. The attachment is no longer in the embed.

attachment out of the embed

Removing the apostrophe resolved that problem.

attachment inside the embed
ivinjabraham commented 2 months ago

I'll try looking into this

ivinjabraham commented 2 months ago

This is caused by a URL sanitation issue. The url field in CreateEmbed(Embed) shouldn't contain invalid characters. Perhaps serde replaces the invalid characters, but somehow that messes up where the attachment should go.

This is a temporary fix:

    pub fn attachment(self, filename: impl Into<String>) -> Self {
        let mut filename = filename.into();
        filename.retain(|c| c != '\'');
        filename.insert_str(0, "attachment://");
        self.image(filename)
    }

Will try and check if there are better ways to ensure url is valid and also why it causes the attachment to go out of the embed.

UPDATE: Found that when there are invalid characters, the Message struct has attachments, and the embed does not have an image. As expected, when it does not have invalid characters, there are no attachments and the embed as an EmbedImage.

asibahi commented 2 months ago

Thanks for looking into this. I know of the work around (just not add invalid characters .. duh!). Yes what you're describing is the same behavior I found.

jamesbt365 commented 2 months ago

Discord converts the names when they include characters it doesn't like, which means that when supplying the attachment name to the embed, it doesn't point to the real attachment anymore.

This is more of a quirk than a bug, and would require filtering the name by the lib, which is currently out of scope incase discord changes in the future.

ivinjabraham commented 2 months ago

Maybe we should add a warning in the docs about it? And if there's nothing more to do about this, then perhaps this issue can be closed?

asibahi commented 2 months ago

A โ€œfixโ€ would have serenity do Discordโ€™s treatment internally to all strings passed through it.

ivinjabraham commented 2 months ago

Is that not what was discussed here:

would require filtering the name by the lib, which is currently out of scope incase discord changes in the future.

jamesbt365 commented 2 days ago

Fixed by #2959.