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

Cloak.Ecto.SHA256.hash_string/1 break when the value is nil #53

Open
SimonLab opened this issue Apr 9, 2024 · 3 comments · May be fixed by #54
Open

Cloak.Ecto.SHA256.hash_string/1 break when the value is nil #53

SimonLab opened this issue Apr 9, 2024 · 3 comments · May be fixed by #54

Comments

@SimonLab
Copy link

SimonLab commented Apr 9, 2024

First of all thank you for the recent release!

I've noticed some of my tests are failing due to:

@doc false
@impl Ecto.Type
def equal?(value1, value2) do
hash_string(value1) == hash_string(value2)
end
defp hash_string(string) do
if String.valid?(string), do: hash(string), else: string
end

I have a schema similar to

  schema "tokens" do
    field(:token_hash, Cloak.Ecto.SHA256)
   ...
   end

I can create a new changeset from this schema, however the initial value will be nil for :token_hash.
If I then call the following: put_change(changeset, :token_hash, "my_token") the String.valid?(nil) in hash_string(string) will fail.

I'm not entirely sure if a check needs to be added in the hash_string function to make sure first the value are not nil, or if I need to make sure first the changeset contains a valid string for the hash first.

One way I can resolve this is to add a default empty string value to field(:token_hash, Cloak.Ecto.SHA256, default: "").
Let me know if this is a non issue and feel free to close this.
Thanks

@vitalis
Copy link

vitalis commented Apr 10, 2024

This is my solution for the issue:

    changeset
    |> get_field(:contact_email)
    |> case do
      nil ->
        changeset

      email ->
        put_change(changeset, :contact_email_hash, email)
    end

@SimonLab
Copy link
Author

Thanks @vitalis this resolves the issue when the contact_email is nil but I don't think it does when the current contact_email_hash is nil.

If I'm not mistaken put_change(changeset, :contact_email_hash, email) will call the equal?/2 which then calls the private function has_string/1 which contains the call to String.valid?, breaking with the current nil value

SimonLab added a commit to SimonLab/cloak_ecto that referenced this issue Apr 12, 2024
- See danielberkompas#53
- Check for nil values in `equal?/2` to avoid call `String.valid/1` on
  nil
@SimonLab SimonLab linked a pull request Apr 12, 2024 that will close this issue
@vitalis
Copy link

vitalis commented Apr 12, 2024

@SimonLab
It did resolve the issue for me, together with your suggestion: "Cloak.Ecto.SHA256, default: "")"
I hope the PR will solve the issue without "workarounds" :)

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 a pull request may close this issue.

2 participants