serokell / coffer

Multi-backend password store with multiple frontends
4 stars 2 forks source link

Fix unsafe FomJSON/FromJSONKey/FromHttpApiData instances #118

Closed dcastro closed 2 years ago

dcastro commented 2 years ago

Clarification and motivation

Consider the FieldName data type:

newtype FieldName = UnsafeFieldName Text
  deriving stock (Show, Eq)
  deriving newtype (A.ToJSON, A.ToJSONKey, A.FromJSON, A.FromJSONKey, Hashable, Buildable, ToHttpApiData, FromHttpApiData)

allowedCharSet :: [Char]
allowedCharSet = ['a'..'z'] ++ ['A'..'Z'] ++ ['0'..'9'] ++ "-_;"

newFieldName :: Text -> Either BadFieldName FieldName
newFieldName t
  | T.null t =
      Left $ BadFieldName "Tags must contain at least 1 character"
  | T.any (`notElem` allowedCharSet) t =
      Left $ BadFieldName ("Tags can only contain the following characters: '" <> T.pack allowedCharSet <> "'")
  | otherwise = Right $ UnsafeFieldName t

This data type is a newtype wrapper around Text with the following invariants: the text is not empty and only contains the allowed characters. The constructor is prefixed with Unsafe to discourage its use - we should use the newFieldName smart constructor instead. The smart constructor enforces the invariants.

However, the newtype-derived FromJSON and FromHttpApiData instances don't use the smart constructor, they coerce through the Unsafe constructor.

> A.decode' @FieldName "\"\""
Just (UnsafeFieldName "")

We should add hand-written instances that use the smart constructor. Let's do this for every newtype with a "smart constructor".

NOTE: the newFieldName error messages say "Tags must ..." instead of saying "Field names must ...", let's fix that too.

Acceptance criteria