Rapptz / discord.py

An API wrapper for Discord written in Python.
http://discordpy.rtfd.org/en/latest
MIT License
14.84k stars 3.76k forks source link

discord.colour has poor data validation #1241

Closed NegativeZero000 closed 6 years ago

NegativeZero000 commented 6 years ago

I don't know if this is a bug or status by design but I was trying to randomize embed colours and was passing string based hex values to discord.colour

listing_embed = discord.Embed(title=self.title, description=self.description,
                                                    color=discord.Colour(hex(random.randint(0, 16777215))),
                                                    url=self.url)

I had assumed that this was working fine since there was not error thrown at declaration. However using that code netted me a HTTP 400 error on submission. I have since changed it to

 listing_embed = discord.Embed(title=self.title, description=self.description,
                                                    color=discord.Colour(random.randint(0, 16777215)),
                                                    url=self.url)

Which works exactly how I wanted it to. I would have done that in the first place if the data I passed it was rejected. I was reading http://discordpy.readthedocs.io/en/latest/api.html?highlight=colour#discord.Embed.colour which is why I thought about using .Colour(). FYI I am not great at Python but looking at colour.py I noticed this for the constructor

def __init__(self, value):
    self.value = value

Yes, the docs for Colour support that it should be int. Should there not be some data validation there or no? I am told, in #help, that this is not an issue in the rewrite branch.

xNinjaKittyx commented 6 years ago

In rewrite, it's written like this:

    def __init__(self, value):
        if not isinstance(value, int):
            raise TypeError('Expected int parameter, received %s instead.' % value.__class__.__name__)

        self.value = value

So it does do a type check before setting itself.

Only issue that would happen is if someone tried to manually change .value itself directly, which I don't think is intended as far as i know?

Rapptz commented 6 years ago

Right this is fixed already.

Manually setting is unsupported however.