Closed benjdevries closed 6 months ago
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?
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?
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.
pyotp.random_base32
andpyotp.random_hex
uselist("ABCDEFGHIJKLMNOPQRSTUVWXYZ234567")
andlist("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: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 asSequence[str]
which would be satisfied by simply using the strings"ABCDEFGHIJKLMNOPQRSTUVWXYZ234567"
and"ABCDEF0123456789"
directly as default arguments. And if they do need to belist
objects, they should have a default value ofNone
with a conditional check that assigns the variable within the function body, e.g.