OC Feedback on my 1st Haskell program: Histcleaner - sanitize your files from your secrets

jaror, 9 months ago

The code looks very clean! Perhaps you could add version bounds in your cabal file.

I also saw this:

  if not exists
    then do
      createDirectory configFolder ownerModes
      pure ()
    else pure ()

Which could be written as:

  unless exists $ createDirectory configFolder ownerModes

I also see:

        contents <- C8.readFile filepath
        let (encSalt:_:encRest) = C8.lines contents
            rest = decodeSecrets encRest
        case decodeBase64 encSalt of
          Left _ -> error "Decoding error"
          Right salt -> pure $ Vault salt rest

Which I would rewrite for a bit more safety

        contents <- C8.readFile filepath
        case C8.lines contents of
          encSalt:_:encRest ->
            case decodeBase64 encSalt of
              Left _ -> error "Decoding error"
              Right salt -> pure $ Vault salt $ decodeSecrets encRest
          _ -> error "Decoding error"

Also, note that in this case laziness might do unexpected things. The contents of the vault will only be evaluated once you actually ask for the values. You might want to use strict fields for it like so:

data Vault =
  Vault
    { salt :: !ByteString
    , secrets :: ![ByteString]
    }

But that's not enough, because this will only force the first element (more precisely the first cons-cell). To truly force all values in the list of secrets you'd have to chose another data type. Unfortunately, there's not really any really popular strict vector type. The simplest fix is probably just to do the forcing of evaluation yourself like so:

              Right salt -> pure $! Vault salt $!! decodeSecrets encRest

Where $!! is from Control.DeepSeq from the deepseq package.

bionade24, 9 months ago

Thx a lot for your detailed feedback. I already heard about bang notation once, but forgot about it. I think I do understand how laziness works, but I can't understand why it'd be bad if Vault gets loaded into memory the 1st time it's needed. Does GHC split the parsing into multiple operations because salt is used a little earlier than secrets?

jaror, 9 months ago

It's not that the vault only gets loaded into memory the 1st time it is needed, what will happen is that the secrets will be read into memory in their raw form and only decoded on-demand. So if you only access the first secret, then it will only decode the first secret and not the rest. Haskell's laziness is very granular like that. In this case it indeed is not that big of a deal, because it probably won't use a lot more memory, but I'd still consider it a code smell.