Should lib check the dictionary message fields?

[twinsant]

In this code:

class SiweMessage:
...
    def __init__(self, message: Union[str, dict] = None, abnf: bool = True):
...
        for k, v in message_dict.items():
            if k == "expiration_time" and v is not None:
                self.expiration_time_parsed = isoparse(v)
            elif k == "not_before" and v is not None:
                self.not_before_parsed = isoparse(v)
            setattr(self, k, v)
...

the library didn't check the fields name, so this may be a exploit here when the caller didn't verify user's input.

My question is should the lib check the fields or just leave this to the application developers?

I did it in my app code:

        FIELDS_CHECK = ['ens', 'domain', 'address', 'chainId', 'chain_id', 'uri', 'version', 'statement', 'type', 'nonce', 'issuedAt', 'issued_at', 'signature']
        for k, v in message.items():
            if k not in FIELDS_CHECK:
                app_log.warn(f'Message filed invalid: {k}!')
                return False

[payton]

Thanks for raising this issue, @twinsant!

I think adding a VALID_FIELDS list to check against would be nice to have here. Instead of logging and returning false, we should instead just raise a ValueError with a short description of the invalid field. This will then match how we fail parsing of string inputs: https://github.com/spruceid/siwe-py/blob/main/siwe/parsed.py#L14 and https://github.com/spruceid/siwe-py/blob/main/siwe/parsed.py#L39.

The intent I had with adding this dictionary input option was for a scenario where the dev had well-defined input. However, you raise a valid point because this may not always be the case depending upon the user. Adding some light guard rails like this sound good to me!

Feel free to open a PR for this! Otherwise, I can get one up at some point tomorrow. Thanks again for all of your questions and interest in the project.