SAML-Toolkits / ruby-saml

SAML SSO for Ruby
https://developers.onelogin.com/v1.0/page/saml-toolkit-for-ruby-on-rails
MIT License
898 stars 561 forks source link

Replace strip! by strip on compute_digest method #650

Closed pitbulk closed 1 year ago

pitbulk commented 1 year ago

This PR replaces #647, Issue reported at #603

@bramleyjl This PR contains only 1 commit vs the one you created with 6 commits.

This PR is for a fix to the compute_digest method that was rendering a nil digest after Base64-encoding it. The reason for this bug is the usage of strip!, a method that returns nil if no whitespace is found on either end of the string to be stripped. By replacing it with strip, an identical method that will return the original string if no whitespace is found to be stripped, this bug can be prevented.

RDeckard commented 1 year ago

Indeed, as lots of Ruby bang methods, #strip! returns nil if it didn't make any change to its receiver. 🎯

But those methods are also known to be more efficient (especially with big objects), as they mutate their receivers instead of duplicating it.

So if you want the best of both worlds (knowing that here mutation didn't generate any problem by itself), you could also have done Base64.encode64(digest).tap(&:strip!). You probably already know that, but the short explanation is that the #tap method allows us to do whatever we want with its receiver in its block, then returns the receiver itself (i.e. not what the block returns). Therefore here, the receiver will be mutated by #strip! in the block, then #tap will return this mutated receiver itself instead of the #strip! method result (which could be nil indeed). 🙂

Nevertheless I am not familiar with this gem, so I have no idea if it would have had any impact here.