Mark Builder methods as pure

[b1rdex]

Fix #1075

[Slamdunk]

We can't change the interface as it would be a BC-break

Please only mark as @pure the implementations.

Moreover I think we can mark so the whole class, IIRC

[Ocramius]

Possibly worth making a major release for such an interface addition - any PRNG stuff can't be pure though, obviously

[Slamdunk]

@Ocramius where is the PRNG here? As far as I know, only PS256 algorithm involves PRNG, the other ones are all deterministic 🤔

[b1rdex]

Moreover I think we can mark so the whole class, IIRC

This is not possible, because @pure is only for functions/methods.

We can't change the interface as it would be a BC-break

I'm not sure why could this be considered as a BC break, because this only makes obvious for SCA tools of what was intended to be introduced in the original PR (#979). The behavior is not changed.

Possibly worth making a major release for such an interface addition - any PRNG stuff can't be pure though, obviously

There is nothing related to PRNG, the methods are just plain "setters".

[Ocramius]

@Ocramius where is the PRNG here? As far as I know, only PS256 algorithm involves PRNG, the other ones are all deterministic 🤔

getToken() interacts with the Signer, which uses CSPRNG under the hood

[Slamdunk]

getToken() interacts with the Signer

I know, and I'll reiterate: there are no random number generation in any currently supported Signer 🤔

[Ocramius]

All signatures are deterministic? They don't even factor in any random offset/salt to make brute forcing harder? 😱

[Slamdunk]

All signatures are deterministic? They don't even factor in any random offset/salt to make brute forcing harder? 😱

All the doc I read state that this is by design: getting secure randomness is tricky, so it's better to only do it while generating the key.

BTW, back to the topic: @b1rdex please update the Token\Builder functions signature, excluding the getToken just to be sure, and we can 🚢 it 💪

[lcobucci]

Given this is marked as a bug fix, I've changed the target branch to 5.4.x.

@b1rdex would please rebase the branch and remove the commits that aren't related to your changes?

Once we do that, we're good for merge & release.

Thanks everyone!

[b1rdex]

@lcobucci done