Format errors should not crash MessageVerifier #7348

wants to merge 1 commit into


None yet
4 participants

bgipsy commented Aug 14, 2012

It seems logical to treat incompatibly marshaled data the same way as unsigned. This would fix issues like #2509 when class signature changes or serializer gets replaced. Motivation for changing session secret hash should not be connected to changes in implementation details of flash or session serialization.


NZKoz commented Aug 14, 2012

Raising an 'unverified' exception isn't really the right thing to do here, the signature was correct, it just couldn't be unmarshalled.

I think the correct fix for the annoying errors is to make the session store transparently ignore both incorrectly signed, and invalid marshal data. The Verifier itself should continue to function as it is.

bgipsy commented Aug 16, 2012

In its current form MessageVerifier encapsulates marshaling along with verification, so it does two things, not one. I'd have chosen the path you've mentioned if I saw verifier accepting and returning strings, not arbitrary objects.

In such case session store could use it something like this:

  if data = @verifier.verified_data(message)
    object = @serializer.load(data)

This form gives session store nice reason for catching marshaling exceptions: it actiually knows that object is being marshaled. Such signature could also avoid use of exceptions for flow control.

Marshaling errors are serializer specific (and my struggle with this fact can be seen from the changeset), so catching them too far from the the point where serializer.load is called may become harmful, and it would also break encapsulation of verifier. My concern is that session store can transparently ignore too much if these rescues go in it and verifier keeps its marshaling responsibility.

Otherwise, if marshaling responsibility is kept in MessageVerifier, adding new exception like MessageVerifier::SerializationError to verifier and catching it in session store along with InvalidSignature may be a better way of encapsulating it.


NZKoz commented Aug 16, 2012

Yeah, you could add MessageVerifier::DeserializationError and make it a subclass of the error that Marshal raises natively. That would give us something specific, but also not break people who already have rescue statements for handling the marshal errors.

bgipsy commented Aug 17, 2012

It's not possible to subclass native exception because there's no such thing. Even Marshal throws two different exceptions: TypeError and ArgumentError depending on input. And Marshal is just one implementation of many which can be injected through constructor. For example the default json thing would give MultiJson::DecodeError. In ideal world exceptions would be part of serializer's contract, but that's what we have in practice.

This gets us back to my point with reusing InvalidSignature: it won't break existing clients. I agree with you in that it's a bit of a stretch to call successfully verified string rejected by serializer as tampered. But alternative is mush weaker encapsulation: users of verifier class should now know that there's such thing as deserialization and that it happens when signature is verified. On more practical side, what value can such exposure provide? I can hardly think of any useful fallback behavior other than ignoring such messages. Verifier doesn't give access to verified data string and the only way to get something into it is to discard message and generate a new one.

bgipsy commented Sep 4, 2012

Do we have decision on this? Can I help with any additional information?


NZKoz commented Sep 11, 2012

I'm still uncomfortable raising a failed signature error when the signature has succeeded, those are two seperate error conditions and conflating the two is dangerous.

I'd prefer we addressed these issues by ensuring that the rails-default stack (cookies.signed and session) handle these errors gracefully, and leave MessageVerifier as a lower level building block.

It's not ideal that the interface serializes using Marshal, and that Marshal provides no useful errors / exceptions, but that ship has sailed.

Barring some huge thing that I'm missing, I think we're better off with the current behaviour.


steveklabnik commented Nov 17, 2012

It's been two months with no further discussion, and @NZKoz says we're better off the way we are, so I'm giving this a close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment