Uploading `.glb` files leaves `UploadConfig` with empty `:client_type`

APB9785 commented 2 months ago

Environment

Elixir version (elixir -v): 1.16.0
Phoenix version (mix deps): 1.7.11
Phoenix LiveView version (mix deps): 0.20.3
Operating system: OS X 14.4.1
Browsers you attempted to reproduce this bug on (the more the merrier): Firefox
Does the problem persist after removing "assets/node_modules" and trying again? Yes/no: Yes

Actual behavior

I'm seeing some odd behavior when uploading .glb files in LiveView - the %Phoenix.LiveView.UploadEntry{} always has client_type: "", which causes the upload's :progress_event to never be called, and it just hangs forever. All other acceptable file types seem to work fine, it's only .glb
The UploadEntry's :acceptable_types does properly contain "model/gltf-binary" and the :acceptable_exts has ".glb"
I can see all the above info by inspecting socket.assigns.uploads in the form's validation function
If I manually overwrite the data in the UploadEntry, to replace the missing client_type, then it works OK. But it doesn't seem right to do that, because it throws error:
```
:uploads is a reserved assign by LiveView and it cannot be set directly
```
and the workaround for this (manually manipulating the socket) is ugly 🙁

Expected behavior

After uploading the .glb file, the UploadEntry contains client_type: "model/gltf-binary" and calls the :progress_event function.

mcrumm commented 2 months ago

Extension and file type checking occurs via the MIME library. Have you added your custom type to your app's config?

APB9785 commented 2 months ago

@mcrumm that was my first idea, but then I saw

The UploadEntry's :acceptable_types does properly contain "model/gltf-binary" and the :acceptable_exts has ".glb"

which seemed to imply that the type is already included with that library. was I wrong about how that works?

mcrumm commented 2 months ago

Can you share your allow_upload() code?

I would add the MIME config and see if it changes anything :) The code paths differ depending on whether you're accepting extensions or file types and that might be playing a role in the behavior you're seeing.

APB9785 commented 2 months ago

socket
|> ...
|> allow_upload(:files,
  accept: ~w(... .glb),
  max_entries: 1000,
  auto_upload: true,
  progress: &handle_progress/3,
  max_file_size: 600_000_000,
  external: fn entry, socket ->
    MyApp.Asset.presign_upload(...)
  end
)

We upload externally to Google Cloud Storage, and then in handle_progress/3 we parse the response into our own Asset model and save it. In this case, however, handle_progress/3 is never getting called because it sits at 0% progress forever.

I will add the MIME config and see if that resolves the issue. Will report back either way 🙂

APB9785 commented 2 months ago

@mcrumm It seems like the MIME config doesn't make a difference here. Adding

config :mime, :types, %{
  "model/gltf-binary" => ["glb"]
}

still yields the same results as before. Doesn't seem to be anything wrong with the config either:

iex(1)> MIME.compiled_custom_types()
%{"model/gltf-binary" => ["glb"]}
iex(2)> MIME.from_path("test.glb")
"model/gltf-binary"

You mentioned that there is an alternate path we could take on the Phoenix side - what would that look like?

mcrumm commented 2 months ago

@APB9785 Sorry for the delayed response– also I owe you further apology because I partially misunderstood what was reported. :)

To clarify where I misunderstood what was reported, the :client_type value, and all values for UploadEntry fields starting with :client_ come directly from the browser and LiveView does not modify them in any way. I can confirm that (at least) Chrome does not send a content-type value for .glb files. Fwiw I downloaded the following file for testing:

https://github.com/KhronosGroup/glTF-Sample-Models/blob/main/2.0/2CylinderEngine/glTF-Binary/2CylinderEngine.glb

So your best bet if you're going to rely on untrusted client data is to assume :client_type can be blank and use the MIME helpers to infer the type from the :client_name.

As far as the progress callback goes, something else must be causing it not to be invoked. I used the following single-file script for testing in Livebook and I cannot reproduce the problem:

# Upload GLB

Mix.install(
  [
    {:phoenix_playground, "~> 0.1.0"}
  ],
  config: [
    mime: [
      types: %{"model/gltf-binary" => ["glb"]}
    ]
  ]
)

defmodule DemoLive do
  use Phoenix.LiveView
  require Logger

  def render(assigns) do
    ~H"""
    <h2>Upload</h2>

    <p>You may add up to <%= @uploads.files.max_entries %> files at a time.</p>

    <p :for={error <- upload_errors(@uploads.files)}>
      <%= upload_error_to_string(error) %>
    </p>

    <form id="auto-form" phx-change="validate">
      <.live_file_input upload={@uploads.files} />
    </form>

    <section
      id="uploads:pending"
      phx-drop-target={@uploads.files.ref}
      style="min-height: 100%;"
    >
      <h3>Pending Uploads (<%= length(@uploads.files.entries) %>)</h3>

        <div :for={entry <- @uploads[:files].entries}>
          <progress
            value={entry.progress}
            id={"#{entry.ref}-progress"}
            max="100">
            <%= entry.progress %>%
          </progress>

          <p :for={error <- upload_errors(@uploads.files, entry)}>
            <%= upload_error_to_string(error) %>
          </p>

          <div>
            <%= entry.uuid %><br />
            <a
              href="#"
              phx-click="cancel-upload"
              phx-value-ref={entry.ref}
            >
              Cancel Upload
            </a>
          </div>
        </div>
    </section>

    <section id="uploads:completed">
      <h2>Uploaded Files (<%= length(@uploaded_files) %>)</h2>
      <p :for={file <- @uploaded_files}><%= file %></p>
    </section>
    """
  end

  def handle_event("validate", _, socket) do
    {:noreply, socket}
  end

  def handle_event("cancel-upload", %{"ref" => ref}, socket) do
    {:noreply, cancel_upload(socket, :files, ref)}
  end

  defp handle_progress(:files, entry, socket) do
    if entry.done? do
      uuid =
        consume_uploaded_entry(socket, entry, fn _meta ->
          Logger.debug([upload: :consuming, entry: entry])
          {:ok, entry.uuid}
        end)

      {:noreply, update(socket, :uploaded_files, &[uuid | &1])}
    else
      {:noreply, socket}
    end
  end

  defp upload_error_to_string(:too_large), do: "The file is too large"
  defp upload_error_to_string(:too_many_files), do: "You have selected too many files"
  defp upload_error_to_string(:not_accepted), do: "You have selected an unacceptable file type"
  defp upload_error_to_string(:external_client_failure), do: "Something went terribly wrong"

  def mount(_params, _session, socket) do
    socket = 
      socket
      |> assign(:uploaded_files, [])
      |> allow_upload(:files,
           accept: ~w(.jpg .png .glb),
           max_entries: 5,
           chunk_size: 256,
           auto_upload: true,
           progress: &handle_progress/3
         )

    {:ok, socket}
  end
end

PhoenixPlayground.start(live: DemoLive)

APB9785 commented 2 months ago

@mcrumm thanks for your response!

So your best bet if you're going to rely on untrusted client data is to assume :client_type can be blank and use the MIME helpers to infer the type from the :client_name.

Assuming client_type is blank, how can we "do the work" to fix it manually, without running into @uploads is a reserved assign by LiveView and it cannot be set directly?

The workaround we are currently using feels... not great

def handle_event("validate", %{"_target" => ["model"]} = _params, socket) do
  model = socket.assigns.uploads.model
  entries = Enum.map(model.entries, &fix_entry_if_needed/1)
  model = Map.put(model, :entries, entries)
  uploads = Map.put(socket.assigns.uploads, :model, model)
  assigns = Map.put(socket.assigns, :uploads, uploads)
  socket = Map.put(socket, :assigns, assigns)

  {:noreply, socket}
end

defp fix_entry_if_needed(%{client_type: ""} = entry) do
  if String.ends_with?(entry.client_name, ".glb") do
    Map.put(entry, :client_type, "model/gltf-binary")
  else
    entry
  end
end

defp fix_entry_if_needed(entry), do: entry

If this is always the case for .glb (and maybe other) files due to browser issues, maybe a public API could be exposed in LiveView to allow the necessary updates to your :uploads ?

I used the following single-file script for testing in Livebook and I cannot reproduce the problem

Perhaps the difference is that in my case the upload is handled by a LiveComponent as opposed to the parent LV? In our case, the "validate" event (handler shown in the snippet above) is called first, and then it passes off to handle_progress/3 as expected. But if we remove the fix_entry logic and allow empty "content-type" to be passed through, handle_progress/3 is never called.

P.S. I've been testing this on Firefox so far

mcrumm commented 2 months ago

how can we "do the work" to fix it manually

I would avoid attempting to modify LiveView's UploadEntry struct and I would compute the missing client_type when consuming the upload.

Perhaps the difference is that in my case the upload is handled by a LiveComponent as opposed to the parent LV?

If you can create a single-file example that we can use to test I would be happy to look further. :)

APB9785 commented 2 months ago

Ah, while attempting to create a minimal example, I think I found the ideal solution you've been getting at. With a config like:

allow_upload(socket, :model,
  accept: ~w(.glb),
  max_entries: 1,
  auto_upload: true,
  progress: &handle_progress/3,
  max_file_size: 100_000_000,
  external: fn entry, socket ->
    MyApp.Asset.presign_upload(...)
  end
)

The :external option is called before handle_progress/3, and in our case that function presign_upload is what's halting the upload when content-type is missing. By moving the fix_entry_if_needed/1 helper (shown in previous comment) into the presign_upload site, we can add the appropriate type without all the manual-overwrite messiness, and then handle_progress/3 fires as expected! :tada:

@mcrumm thanks for all your help working through this, it's been a great help 🙂

For anyone reading this in the future with a similar issue - investigate all the code in your :external function, and use that stage for any last minute fixes/overwrites to your entry data.

phoenixframework / phoenix_live_view