Security invulnerability with DataResolver#resolveFile

[kislball]

Please describe the problem you are having in as much detail as possible: Each time, message has an attachment, Discord.js fetches it. It is all ok as long as attachments are sent as files. But if we send image as an URL(https://someiplogger.com/coolimage.png), Discord.js will still fetch it and thus leak IP address of machine.

Further details:

• discord.js version: 13.1.0
• Node.js version: doesn't matter
• Operating system: doesn't matter
• Priority: high

Relevant client options:

• partials: none
• gateway intents: none
• other: none

P.S. originally found by @Mirdukkk

[kislball]

Possible workaround is to introduce something like message.attachments.first().fetch(), which will resolve attachment into a buffer or stream. Even tho it won't stop IP from leaking, users will know, that they might have their IP leaked

[SpaceEEC]

I don't think discord.js is automatically downloading attachments on received messages.

Can you create a Minimal, Reproducible Example for this?

[kislball]

I don't think discord.js is automatically downloading attachments on received messages.

Can you create a Minimal, Reproducible Example for this?

Misunderstood the problem at first. I thought, D.js fetches every attachment on messageCreate event, however, it is not true actually. However, the problem still exists.

client.on('messageCreate', async msg => {
  const args = msg.content.split(/ +/)

  if (args[0] === '!stealemoji') { // !stealemoji https://iplogger.com/ emojiname
    msg.guild.emojis.create(args[1], args[2]) 
  }
})

In this case, URL is passed, Discord.js fetches URL using DataResolver#resolveFile function and leaks IP of machine it is running on.

Unfortunately, the problem is pretty hard to fix without using proxy, since it is required to fetch attachment to send it back to Discord. So, I think, this behaviour should be documented to warn users about IP leak.

[monbrey]

Cant this be prevented by sanitising input? Per your example, stealing an emoji should always be a discord CDN URL?

[kislball]

It can be sanitized, e.g. only accepting a cdn.discordapp.com domain. Nevertheless, it can also be just a direct URL to the image, from imgur, for example. It can be easily IP logged. However, I still think, that fetching behaviour should be documented and users should be warned about accepting suspicous URLs.

[kyranet]

It's the bot developer's job to sanitise their arguments, not Discord.js's - just like it's your job to sanitise user input before inserting them in SQL code (unless you haven't heard of our friend "Bobby Tables").

Limiting where attachments can be uploaded from in a very specific endpoint leads to inconsistent behaviour, so if we're changing this, it'd be to remove the option to upload from URL, which is unlikely and doesn't solve your issue as you're just moving code from one place to another.

[Mirdukkk]

Developer just don't know that discord.js fetches provided link automatically. Is it so difficult for you to make several warnings in the documentation?

[kyranet]

If you didn't know discord.js can upload GIFs from links, what is the expected input users must sent to use that command? I can think of sending up to around a million characters in Base64, but that'd be a bit too much over the content length limit 😅

That being said, it's documented:

• https://discord.js.org/#/docs/main/stable/class/GuildEmojiManager?scrollTo=create takes BufferResolvable
• https://discord.js.org/#/docs/main/stable/typedef/BufferResolvable which takes a URL (third option)

There are many places where we accept BufferResolvable, not just GuildEmojiManager#create, would you suggest adding that same warning in all of them?

[Mirdukkk]

I suggest adding warnings where it is not obvious that the content from the link will be downloaded automatically. For example, GuildEmojiManager#create - in order to understand that the link will be fetched automatically, you either need to know that the Discord API does not accept links as emojis, or you need to read the library code. How many developers do you think are aware of this?