Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

serialise: add deserialiseFullyOrFail #251

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

infinity0
Copy link
Contributor

This is useful in protocol-related logic where you want to be strict as possible and don't allow an attacker to send junk after a valid piece of data.

Copy link
Member

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

What about being more radical and changing the behaviour of the existing deserialise functions? That is, perhaps we should just make this the default behaviour. Any reason not to other than possible worries about backwards compat?

-- or get back a @'Either' 'DeserialiseFailure' 'ByteString'@.
--
-- @since 0.2.4.0
deserialiseFullyOrFail :: Serialise a => BS.ByteString -> Either (Either CBOR.Read.DeserialiseFailure BS.ByteString) a
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we really need the trailing data as a result? Why not just include it into the failure?

If someone really needs to decode a prefix, there are lower level interfaces they can use.

Copy link
Contributor Author

@infinity0 infinity0 Feb 23, 2021

Choose a reason for hiding this comment

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

Including the trailing data is nice as then you can generate a custom error message with some extra details such as the length of the junk or a prefix of it.

As you pointed out, I just realised that Codec.CBOR.Read.deserialiseFromBytes decode will have nearly the same signature. However using this requires adding a direct dependency on cborg which may be surprising/inconvenient for users. The API in serialise otherwise provides no access to the leftover input, so it's nice to have this additional method. For example, the corresponding API in binary has it (decodeOrFail) although it is slightly less clean.

So, I could change the type signature (and definition) to be the same as Codec.CBOR.Read.deserialiseFromBytes decode if you prefer?

I deleted a previous comment agreeing with the "more radical" option, since as per what I said above, I now think it's good to have an API in serialise that exposes this information - and I suppose changing the existing type signature on deserialiseOrFail to express this, would not be acceptable as this would break compilation. Another option is to change DeserialiseFailure to include a variant for junk leftovers, which would allow us to preserve the type signature on deserialiseOrFail whilst changing its behaviour, but this would break people that are pattern-matching on DeserialiseFailure (with -Werror=incomplete-patterns or similar)

@infinity0
Copy link
Contributor Author

infinity0 commented Apr 21, 2021

ping again on this :)

In case it wasn't clear, my last comment boils down to modifying this PR to instead have

tryDeserialise :: Serialise a => ByteString -> Either DeserialiseFailure (ByteString, a)
tryDeserialise = deserialiseFromBytes decode

the main purpose being to allow the user to avoid a direct dependency on cborg whilst also having full control over the decode process -- analogous to Data.Binary.decodeOrFail

@infinity0 infinity0 force-pushed the deserialiseFullyOrFail branch 2 times, most recently from f20931b to 623ac1e Compare April 21, 2021 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants