Possible bug in interaction between Ecto empty_values, LiveView and dead view

tomekowal commented 1 year ago

Elixir version

Elixir 1.14.1 (compiled with Erlang/OTP 25

Database and Version

PostgreSQL 15.3

Ecto Versions

3.10.1

Database Adapter and Versions (postgrex, myxql, etc)

Postgrex

Current behavior

After upgrading to 3.10.2 from 3.7 (yes, significant upgrade). I am getting:

ArgumentError: cannot deserialize &Ecto.Type.empty_trimmed_string?/1, the term is not safe for deserialization
  File "lib/plug/crypto.ex", line 80, in Plug.Crypto.non_executable_terms/1
  File "lib/plug/crypto.ex", line 87, in Plug.Crypto.non_executable_list/1
  File "lib/plug/crypto.ex", line 66, in anonymous fn/3 in Plug.Crypto.non_executable_terms/1
  File "maps.erl", line 233, in :maps.fold_1/3
  File "lib/plug/crypto.ex", line 66, in anonymous fn/3 in Plug.Crypto.non_executable_terms/1
  File "maps.erl", line 233, in :maps.fold_1/3
  File "lib/plug/crypto.ex", line 66, in anonymous fn/3 in Plug.Crypto.non_executable_terms/1
  File "maps.erl", line 233, in :maps.fold_1/3

Expected behavior

The application continues to work without throwing exceptions.

Preliminary research

The problem is that I have a dead view; inside it, there is a LiveView rendered with live_render function.

There was a similar issue reported here: https://github.com/phoenixframework/phoenix_live_view/issues/180#issuecomment-482976498 so I understand I should not put the changeset in the LiveView session.

However, I am not trying to do that. My problem is that the changeset is in the session because of the parent dead view, so it is "by accident".

According to the docs https://hexdocs.pm/phoenix_live_view/Phoenix.Component.html#live_render/3 the session option can only add to the session, so the error persists even if I put an empty map there.

I thought putting a changeset in the dead view session was an OK practice. If that is the case, there is this interplay between:

Ecto putting the function in the changeset
dead view with changeset in the session
live_view session deserializer tripping on functions in deserialized data (since it is unsafe)

I am putting the issue here since I am still determining where the problem is and upgrading Ecto surface it.

Feel free to close the issue if I am not supposed to put changesets even in a dead view session.

Otherwise, I am not sure if the problem lies in Ecto or Phoenix/LiveView

josevalim commented 1 year ago

This is expected. There is no guarantee changesets are serializable and my recommendation is to not store changesets in sessions. Instead, store the data that allows you to build them.

Another reason to not store changesets in session is because we can add new fields, then when you update Ecto, it is broken. Generally, you should only store data that you on (and even then, with care).

tomekowal commented 1 year ago

Thank you for your quick answer! Much appreciated.

I have a followup question.

Would it make sense to fail a little bit earlier and disallow serializing unsafe data in the first place?

I like symmetry :)

josevalim commented 1 year ago

We could at the cost of also making serialization more expensive, which I am not sure if worth it?

exlee commented 1 year ago

More context on the issue (I'm working on this alongside @tomekowal):

So far I found that problem surfaces iff:

Changset include nil values
Ecto being at 3.10 (based on the code read-through, I wasn't able to downgrade to lower version)

I won't argue with the rationale - because I agree - yet we are where we are 😉

I find it a ninja issue - not very visible (no indication what is causing it) with the same code working before. It's failure during channel join/token verification (it creates working channel fine) the visible manifestation is a form reloading every 30s.

I hadn't found even single &Ecto.Type.empty_trimmed_string?/1 reference anywhere during data inspections on multiple levels, and I believe Phoenix's token seems to be the only element affected.

Just to note - I have exact same issue with different app than mentioned above (we have dependency sync). See slightly longer stack trace (see below). Same root cause - nil within Changeset passed as a parameter to live_render.


** (ArgumentError) cannot deserialize &Ecto.Type.empty_trimmed_string?/1, the term is not safe for deserialization
    (plug_crypto 1.2.3) lib/plug/crypto.ex:80: Plug.Crypto.non_executable_terms/1
    (plug_crypto 1.2.3) lib/plug/crypto.ex:87: Plug.Crypto.non_executable_list/1
    (plug_crypto 1.2.3) lib/plug/crypto.ex:66: anonymous fn/3 in Plug.Crypto.non_executable_terms/1
    (stdlib 4.3.1.1) maps.erl:411: :maps.fold_1/3
    (plug_crypto 1.2.3) lib/plug/crypto.ex:66: anonymous fn/3 in Plug.Crypto.non_executable_terms/1
    (stdlib 4.3.1.1) maps.erl:411: :maps.fold_1/3
    (plug_crypto 1.2.3) lib/plug/crypto.ex:66: anonymous fn/3 in Plug.Crypto.non_executable_terms/1
    (stdlib 4.3.1.1) maps.erl:411: :maps.fold_1/3
    (plug_crypto 1.2.3) lib/plug/crypto.ex:66: anonymous fn/3 in Plug.Crypto.non_executable_terms/1
    (stdlib 4.3.1.1) maps.erl:411: :maps.fold_1/3
    (plug_crypto 1.2.3) lib/plug/crypto.ex:99: Plug.Crypto.non_executable_tuple/2
    (plug_crypto 1.2.3) lib/plug/crypto.ex:51: Plug.Crypto.non_executable_binary_to_term/2
    (plug_crypto 1.2.3) lib/plug/crypto.ex:330: Plug.Crypto.decode/2
    (phoenix_live_view 0.17.7) lib/phoenix_live_view/static.ex:29: Phoenix.LiveView.Static.verify_token/2
    (phoenix_live_view 0.17.7) lib/phoenix_live_view/session.ex:60: Phoenix.LiveView.Session.verify_session/4
    (phoenix_live_view 0.17.7) lib/phoenix_live_view/channel.ex:805: Phoenix.LiveView.Channel.mount/3
    (phoenix_live_view 0.17.7) lib/phoenix_live_view/channel.ex:58: Phoenix.LiveView.Channel.handle_info/2```

tomekowal commented 1 year ago

We could at the cost of also making serialization more expensive, which I am not sure if worth it?

Probably not worth it.

It could be worth it if we could make the check on serialization but then skip it on deserialization. However, AFAIU, that is impossible because somebody could tamper with it.

elixir-ecto / ecto