Skip to content

Mark Builder methods as pure#1076

Merged
lcobucci merged 2 commits into
lcobucci:5.4.xfrom
b1rdex:pure-builder
Nov 6, 2024
Merged

Mark Builder methods as pure#1076
lcobucci merged 2 commits into
lcobucci:5.4.xfrom
b1rdex:pure-builder

Conversation

@b1rdex

@b1rdex b1rdex commented Nov 4, 2024

Copy link
Copy Markdown
Contributor

Fix #1075

@Slamdunk Slamdunk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Ocramius commented Nov 4, 2024

Copy link
Copy Markdown
Collaborator

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

@Slamdunk

Slamdunk commented Nov 4, 2024

Copy link
Copy Markdown
Collaborator

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

@b1rdex

b1rdex commented Nov 4, 2024

Copy link
Copy Markdown
Contributor Author

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 commented Nov 4, 2024

Copy link
Copy Markdown
Collaborator

@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

Slamdunk commented Nov 4, 2024

Copy link
Copy Markdown
Collaborator

getToken() interacts with the Signer

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

@Ocramius

Ocramius commented Nov 4, 2024

Copy link
Copy Markdown
Collaborator

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

@Slamdunk

Slamdunk commented Nov 4, 2024

Copy link
Copy Markdown
Collaborator

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 💪

@b1rdex
b1rdex requested a review from Slamdunk November 4, 2024 14:51
@Slamdunk Slamdunk added the Bug label Nov 4, 2024
@Slamdunk Slamdunk added this to the 5.4.1 milestone Nov 4, 2024
@lcobucci
lcobucci changed the base branch from 5.5.x to 5.4.x November 5, 2024 23:11
@lcobucci

lcobucci commented Nov 5, 2024

Copy link
Copy Markdown
Owner

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

b1rdex commented Nov 6, 2024

Copy link
Copy Markdown
Contributor Author

@lcobucci done

@lcobucci
lcobucci merged commit 848815d into lcobucci:5.4.x Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Please mark Builder methods @pure to ease understanding that it's immutable

4 participants