Don't use mutable objects as default arguments

[benjdevries]

pyotp.random_base32 and pyotp.random_hex use list("ABCDEFGHIJKLMNOPQRSTUVWXYZ234567") and list("ABCDEF0123456789") respectively as default arguments. While it does appear that this specific usage is safe, using mutable objects as default arguments is a recipe for disaster since the objects will be instantiated at module load and the reference will be shared between any calls to that module.

Consider the following dangerous usage where calling foo prints a different result every time:

def foo(bar=list("1234")):
    print(bar)
    bar.pop()

foo()  # ['1', '2', '3', '4'] 
foo()  # ['1', '2', '3'] 
foo()  # ['1', '2'] 
foo()  # ['1']
foo()  # []
foo()  # IndexError  

From the looks of it, there is no reason these arguments need to be lists. They exist only to be passed to random.choice and are typed as Sequence[str] which would be satisfied by simply using the strings "ABCDEFGHIJKLMNOPQRSTUVWXYZ234567" and "ABCDEF0123456789" directly as default arguments. And if they do need to be list objects, they should have a default value of None with a conditional check that assigns the variable within the function body, e.g.

def random_base32(length: int = 32, chars: Sequence[str] = None -> str:
    if chars is None:
        chars = list("ABCDEFGHIJKLMNOPQRSTUVWXYZ234567")
    ...

[kislyuk]

As you point out, this usage is safe because the functions in question don't mutate, return, or expose the value of the argument. However, for the avoidance of confusion I agree that it would be great to make this argument immutable with frozenset(). Would you like to do this in a PR?

[benjdevries]

Yes, I can add a PR. Is there a reason you suggested frozenset() over just using the strings directly, which are already immutable sequences of characters?

[kislyuk]

Good point, we can use strings directly as the argument values. For legibility, I would like to keep the value directly in the signature instead of setting it to None.