Add new methods to MessageVerifier

[lleger]

This commit adds two boolean methods to ActiveSupport::MessageVerifier: #verified, to return false on invalid messages, and #valid_message?, a convenience boolean method to check the validity of a message's format.

#verified method
The first commit adds a #verified method to return false when an error occurs decoding the message; it continues to return the message on success. (The behavior of #verify remains unchanged.) Previously it was not possible to attempt to decode a message without possibly encountering an exception.

Example

@verifier = ActiveSupport::MessageVerifier.new("c0mplexs3cret")
@data = @verifier.generate("a very important message")
@verifier.verified(@data) # => "a very important message"
@verifier.verify(@data) # => "a very important message"
@verifier.verified("foo") # => false
@verifier.verify("foo") # => ActiveSupport::MessageVerifier::InvalidSignature

#valid_message? method
Sometimes it may be beneficial to verify the format of the message without performing the decoding. The second commit adds a #valid_message? boolean method which performs this validation. It has the added benefit of cleaning up a conditional in the #verified method. I also took some liberty restructuring the tests in this commit.

Example

@verifier = ActiveSupport::MessageVerifier.new("c0mplexs3cret")
@data = @verifier.generate("a very important message") # => "BAhJIh1hIHZlcnkgaW1wb3J0YW50IG1lc3NhZ2UGOgZFVA==--4cf27b268ccb74307099b03d2b023f68b9e9763c"
@data.valid_message?(@data) # => true
@data.valid_message?("foo") # => false
@data.valid_message?("foo--bar") # => false
first, second = @data.split("--")
@data.valid_message?("#{first.reverse}--#{second.reverse}") # => false
@data.valid_message?("#{first}--#{second.reverse}") # => false
@data.valid_message?("#{first.reverse}--#{second}") # => false
@data.valid_message?("#{second}--#{first}") # => false
@data.valid_message?("#{first}--#{first}") # => false

[egilburg]

Thanks for your contribution. Some questions:

1. It's confusing that a message that fails verification can still pass valid_message? Is there a better word than valid for what seems to be "can be parsed"?
2. The API changes between verify and verify! impact existing apps using Message Verifier. If someone wrote code depending on unverified message to raise error as it did before this patch, such as in a before_filter or middleware, this change may silently introduce a security vulnerability. You mentioned you understand this change, but given the impact, I think at a minimum there'd need to be a deprecation cycle.
3. #find vs #find!: find actually does raise error, find_by_id doesn't. That's by design as the most common use case. Perhaps a similar approach should be here, with a new non-raising method introduced and verify becoming non_raising_method || raise(...)

Overall, perhaps an API like this:

def valid_format?(message)
  # true or false
end

def verified?(message)
  # true or false
end

def verify(message)
  verified?(message) || raise(...)
end

The above will address the ambiguous meaning of valid and avoid changing existing API.

[lleger]

Good questions.

1. Perhaps valid_message_format?, but that seems a tad wordy to me. I'm open to alternative names, though. For what it's worth, I think the only way it could pass in valid_message? and fail verification is if there's an error in the serializer.
2. Yes, as I noted above, this is definitely a breaking change. It may not be appropriate until a major version, but I wanted to submit the change to gather feedback.
3. Good point. I think that would be amenable. It would certainly change it from a breaking change to a non-breaking change as well.

[egilburg]

@lleger thanks, I update my first comment with proposed example.

[lleger]

I'm not sure about verified? in that example. The goal of this PR is to provide a way to get the message without encountering an exception. In your example, that's still not possible. Maybe we just need another name?

[sgrif]

@lleger You mention this in response to the breaking change:

Yes, as I noted above, this is definitely a breaking change. It may not be appropriate until a major version, but I wanted to submit the change to gather feedback.

We do not allow breaking API changes, even in major version bumps. We follow a "shifted" semver model. Major and minor versions are treated as the same, with major version bumps reserved for features deemed "major enough". In either case, all breaking API changes must go through a deprecation cycle. Even if we had a deprecation warning emitted with the exception for a version, I don't think we would accept a change that could potentially silently introduce a vulnerability like that.

[lleger]

@sgrif Gotcha. So would the appropriate direction here be to leave #verify as-is and introduce a new method?

[sgrif]

Yes, that would be the safer course of action in this case.

[lleger]

👍 I'll work on updating the PR, thanks for the suggestion.

[lleger]

@egilburg @sgrif I updated the PR according to your feedback. It looks like Travis choked for some reason, but the tests are passing as far as I can tell. Ready for re-review.

[sgrif]

You can merge these into a single entry

[lleger]

Alright, I'll update this.

[lleger]

@sgrif I updated the PR to reduce the CHANGELOG entries and I also changed the #verified method to no longer be a predicate. (I think, in hindsight, #verified? read better in my mind, but I agree that it makes more sense as #verified.) I can change the behavior of #verify/#verified to reduce the diff noise as you mentioned if that's necessary, but I honestly think it's a pretty small diff to begin with. I'm not sure that makes a practical difference, but I'm open to having my mind changed.

[egilburg]

I think that having a raising method implemented in terms of a non raising method is logically simpler than the non raising method rescuing from a raising method.

Eugene

On Nov 26, 2014, at 9:35 AM, Logan Leger notifications@github.com wrote:

@sgrif I updated the PR to reduce the CHANGELOG entries and I also changed the #verified method to no longer be a predicate. (I think, in hindsight, #verified? read better in my mind, but I agree that it makes more sense as #verified.) I can change the behavior of #verify/#verified to reduce the diff noise as you mentioned if that's necessary, but I honestly think it's a pretty small diff to begin with. I'm not sure that makes a practical difference, but I'm open to having my mind changed.

—
Reply to this email directly or view it on GitHub.

[sgrif]

I agree that the raising implemented in terms of the non raising makes more sense. The complexity of the fit history, and being able to quickly find the commit which introduced a line of code is important to us, though. I really don't feel strongly on it though so you should go with whatever you think is best.

[lleger]

I think that having a raising method implemented in terms of a non raising method is logically simpler than the non raising method rescuing from a raising method.

👍 My thoughts exactly.

The complexity of the git history, and being able to quickly find the commit which introduced a line of code is important to us…

Understandable. I could, perhaps, reorder the methods and reduce the diff noise a bit, but that seems gratuitous to me. It seems logical how it's structured. The overall diff is pretty small, too.

I really don't feel strongly on it though so you should go with whatever you think is best.

In that case, I think this is ready to be merged, yes? Travis is still choking, but these tests run fine locally (the failure looks like a Ruby interpreter bug and not related to these changes).

[egilburg]

IMO a side-effect-free pattern which intends to whitelist an object against a condition, and which returns an object on success condition, should return nil rather than false on failure condition. This is similar to the presence method in Active Support.

You can just not have the else clause here and it'll return nil implicitly.

[lleger]

Hm, yeah, I see what you're saying here. I think it works better, in my mind, returning false. I imagined this method working more like #save et al. Besides removing a branch in the conditional, is there any other benefit to returning nil implicitly over returning false explicitly? I guess, practically, it shouldn't largely affect the functionality.

[rafaelfranca]

It is the project guideline to not return explicit booleans. We prefer to always return nil unless it is documented to return false

[lleger]

Sorry, I wasn't aware of that guideline. (Are those guidelines written down anywhere? Perhaps I just missed it.) I prefer the explicit false, but if it's the guideline, I'm happy to change it, since it makes no practical difference. Would you like me to open another PR with that change?

[sgrif]

It was already changed. :)

On Tue Dec 02 2014 at 9:32:27 AM Logan Leger notifications@github.com
wrote:

In activesupport/lib/active_support/message_verifier.rb:

       raise
     end
   else

•    raise InvalidSignature
  

•    false
  

Sorry, I wasn't aware of that guideline. (Are those guidelines written
down anywhere? Perhaps I just missed it.) I prefer the explicit false,
but if it's the guideline, I'm happy to change it, since it makes no
practical difference. Would you like me to open another PR with that change?

—
Reply to this email directly or view it on GitHub
https://github.com/rails/rails/pull/17727/files#r21170473.

[rafaelfranca]

Yes. It is written at http://guides.rubyonrails.org/api_documentation_guidelines.html#booleans

[lleger]

It was already changed. :)

Oh, I see. @rafaelfranca changed it in bc8cc56. Great.

Yes. It is written at…

Oh, I see now. Thanks for pointing that out, I must've missed it. I'll know to do it next time!

[sgrif]

In that case, I think this is ready to be merged, yes?

Yeah, we'll just need the commits squashed and rebased. I also can't merge until we ship 4.2.0.rc1, which is when we'll create the 4-2-stable branch and master will be tracking Rails 5. It's far too late for this to make it into 4.2

[lleger]

Yeah, we'll just need the commits squashed and rebased.

I've been keeping it rebased against master. I'll go ahead and squash it down to one commit and push that up.

It's far too late for this to make it into 4.2…

Indeed, I suppose it is. I'll make sure to follow up here when the RC drops.

Thanks for all your help!

[lleger]

@sgrif I saw that master was tracking Rails 5 as of f25ad07, so I went ahead and rebased and squashed. Let me know if you need anything else from me.

[sgrif]

Great thanks. I need to let ci run I'll check on this in the morning

[claudiob]

@lleger I am trying to add documentation to the methods that you added, as suggested by @rafaelfranca in ee73d9f.

However, I have a doubt regarding verified. In the CHANGELOG you wrote:

Previously, the only way to decode a message with ActiveSupport::MessageVerifier was to use #verify, which would raise an exception on invalid messages. Now, #verified will return either false when it encounters an error or the message.

However, that does not seem to be true: verified does not return false when the message is invalid, but raises an exception. There is even a test that checks when verified raises an exception.

Am I right? And if that's the case… what is the correct purpose of verified?

[lleger]

@claudiob Actually, it no longer returns false explicitly (bc8cc56), so I guess the CHANGELOG isn't totally accurate. It returns nil instead (but the functionality isn't practically different). But verified also does raise if it has an unspecified decoding error that doesn't relate to this code (it still returns if it's a badly formatted message, though). If it's a bad Base64 decode, for example, it'll still raise. The test you link to is actually testing that the serializer class is available, which is also necessary. If you look at the tests, verified does return if there's a decoding error. Does that answer your questions?

Anyway, I was going to write the documentation, but hadn't gotten around to it yet. Thanks for tackling it!

[claudiob]

@lleger Thanks for your reply.

Let me clarify what I was asking for with an example.
Imagine we have two verifiers with the same secret but different serializer:

require 'yaml'
verifier1 = ActiveSupport::MessageVerifier.new 's3Krit'
verifier2 = ActiveSupport::MessageVerifier.new 's3Krit', serializer: YAML

If you sign a message with the first verifier, and then you ask if the message is valid for the second verifier, you will get true:

signed_message = verifier1.generate 'private-message' #=> "BAhJI..."
verifier2.valid_message?(signed_message) #=> true

This is my question for you: don't you think this is misleading?

As I developer, I see that verifier2 consider signed_message to be a "valid message".
So my next thought is extract the signed value from the message but alas I wouldn't be able to:

verifier2.verified(signed_message) # Psych::SyntaxError: (<unknown>): control characters are not allowed at line 1 column 1
verifier2.verify(signed_message) # Psych::SyntaxError: (<unknown>): control characters are not allowed at line 1 column 1

What do you think? Did you envision valid_message? to actually be a public method, or was it just meant to be a private method only to be used internally by verified?

[lleger]

@claudiob Ah, I see what you're saying now. I don't consider this misleading. I intended for valid_message? to be a test of the format so that you could determine if a given string is a valid digest without actually decoding it. I definitely intended for it to be a public method; there are places in several apps where I have used such a method to avoid decoding invalid messages. Does that make sense?

[claudiob]

@lleger what I don't understand is a use case of what you just described:

there are places in several apps where I have used such a method to avoid decoding invalid messages

As I tried to explain with the code above, you can call valid_message?, it can return true, and yet you can still get an error when you decode the message.

Then… why not call verified directly? What's the benefit of first calling valid_message? and getting a result that does not tell you whether you can actually extract the value from the message?

[lleger]

@claudiob For example, if you encode a token to send in an email, and you want to check if that token is valid before showing the page; if not, you want to redirect. At that point I don't care about the message itself, I just care that it's valid digest. I don't actually care if the message itself is valid until I go through the steps of validating the message (maybe from the database, maybe in a different action or controller, even). If you wanted to, you could easily call verified in this situation and just throw away the result, but since it also had the added convenience of cleaning up a conditional, and because it technically saves a code path that I don't need, it seemed logical to also expose it as a public API so that it can be used in exactly this situation. I wouldn't think of valid_message? as testing the validity of the message itself, but rather testing that it could be valid. It's just checking the format; eventually, you need to do an actual check, with either verified or verify. But, still, I think that's useful.

[claudiob]

@lleger Is this the case you are describing?

1. a user has forgotten his password, so goes on the "Forget your password" page
2. the user enters "user@example.com" and then clicks on "Forgot my password".
3. the user receives an email with a link to reset the password, something like http://example.com/edit_password?token=t1f-qyCuYNxGC – the token indicates which user is trying to reset the password (by signing the e-mail address)
4. the user clicks on the link and is directed to the page
5. the corresponding Rails controller/action checks with valid_message?:
   • if valid_message?('t1f-qyCuYNxGC') is truthy, the form to enter a new password is shown
   • otherwise, an "Unrecognized token" message is displayed
6. the user enters the new password and clicks on "Change password"
7. the corresponding Rails controller/action invokes verify:
   • if verify('t1f-qyCuYNxGC') returns an e-mail address which matches a user in the database, then that user's password is updated
   • otherwise, an "Unrecognized token" message is displayed

[lleger]

@claudiob More or less, yes.