attributes_escape/1 does not escape ids completely

[ponychicken]

I have data from a legacy system. On a few entries in noticed a subtle breakage. I traced it down to attributes_escape/1 not escaping ids sufficiently-

The docs hint that passing a number will throw an error:

iex(8)> attributes_escape(%{id: 11})
** (ArgumentError) attempting to set id attribute to 11, but setting the DOM ID to a number can lead to unpredictable behaviour. Instead consider prefixing the id with a string, such as "user-11" or similar
    (phoenix_html 4.1.1) lib/phoenix_html.ex:258: Phoenix.HTML.id_value/1
    (phoenix_html 4.1.1) lib/phoenix_html.ex:208: Phoenix.HTML.build_attrs/1
    (phoenix_html 4.1.1) lib/phoenix_html.ex:195: Phoenix.HTML.attributes_escape/1
    iex:8: (file)

But passing the number a string will happily produce output:

iex(7)> attributes_escape(%{id: "11"})
{:safe, [" id=\"", "11", 34]}

Same for whitespace ("An ID attribute's value must not contain ASCII whitespace characters.")

iex(8)> attributes_escape(%{id: "white space"})
{:safe, [" id=\"", "white space", 34]}

Additionally also characters like "/" are not valid in ids, but the escape function will not escape them as well:

iex(4)> attributes_escape(%{id: "asdflkj//sdf"})
{:safe, [" id=\"", "asdflkj//sdf", 34]}

Technically, the value for an ID attribute may contain any other Unicode character. However, when used in CSS selectors, either from JavaScript using APIs like Document.querySelector() or in CSS stylesheets, ID attribute values must be valid CSS identifiers. This means that if an ID attribute value is not a valid CSS identifier (for example, my?id or 1234) then it must be escaped before being used in a selector, either using the CSS.escape() method or manually.

[SteffenDE]

I think we need to differentiate between what's allowed in attributes vs. what's allowed to be a valid CSS selector. The only official restriction on IDs in HTML5 is no whitespaces, so this is something we could warn about when we encounter one, but the additional overhead of checking strings for whitespaces might not be worth it.

Having a css_escape function would be useful though - I created an issue for this in Floki a while back: philss/floki#554

On a few entries in noticed a subtle breakage.

Can you provide some details? If this is on the JS side, adding a CSS.escape might already be enough.

[josevalim]

The goal of this library is to escape HTML to avoid HTML injection, that's it. Doing per attribute validation would be quite hard because some attributes also depend on the tag. In the case of ID, we do validate integers to avoid common pitfalls (and because it is very cheap to do so!), but it is not our goal or intention to validate all of it.

[ponychicken]

I think we need to differentiate between what's allowed in attributes vs. what's allowed to be a valid CSS selector. The only official restriction on IDs in HTML5 is no whitespaces, so this is something we could warn about when we encounter one, but the additional overhead of checking strings for whitespaces might not be worth it.

Having a css_escape function would be useful though - I created an issue for this in Floki a while back: philss/floki#554

On a few entries in noticed a subtle breakage.

Can you provide some details? If this is on the JS side, adding a CSS.escape might already be enough.

The problem is that if there is one element in the HEEX with an invalid id and this element has a phx-click attribute, the built in javascript will try to do document.querySelectorAll(invalidId) and fail, and since the error is not caught, no click handlers will be installed at all. eg one inavlid id ==> no javascript clicks at all.

[SteffenDE]

That's something we are able to fix in LV's js though, right? So more of a LV bug then.

[SteffenDE]

@ponychicken can you provide an example to reproduce the error in LV? I just tried a button with id "foo/bar" and it worked just fine:

<button phx-click="inc">+</button>
<button id="foo/bar" phx-click="dec">-</button>

[ponychicken]

The stack:

Uncaught SyntaxError: Failed to execute 'querySelectorAll' on 'Document': '#items-foo/bar-menu' is not a valid selector.
    at Object.all (dom.js:40:33)
    at Object.filterToEls (js.js:309:21)
    at js.js:18:12
    at Array.forEach (<anonymous>)
    at Object.exec (js.js:13:14)
    at live_socket.js:678:14
    at live_socket.js:467:33
    at LiveSocket.owner (live_socket.js:463:15)
    at LiveSocket.withinOwners (live_socket.js:467:10)
    at live_socket.js:677:14

The heex:

  attr :id, :string, required: true
  attr :class, :string, default: nil
  slot :inner_block, required: true
  slot :title, required: true
  attr :rest, :global
  slot :actions

  def card(assigns) do
    ~H"""
    <div
      class={[
        "flex flex-col justify-between h-full shadow rounded-lg p-3",
        @class
      ]}
      id={@id}
      {@rest}
    >
      <div class="row-1 text-gray-900 dark:text-gray-200 leading-snug text-base flex items-start gap-x-2">
        <strong class="text-base !leading-tight font-semibold">
          <%= render_slot(@title) %>
        </strong>
        <div
          phx-click-away={JS.hide(to: "##{@id}-menu")}
        >
          <button
            class="hero-ellipsis-vertical-solid text-zinc-700 dark:text-zinc-200 h-5 -mr-2"
            phx-click={
              JS.toggle(
                to: "##{@id}-menu",
                in: {"ease-out duration-100", "opacity-0 scale-95", "opacity-100 scale-100"},
                out: {"ease-out duration-75", "opacity-100 scale-100", "opacity-0 scale-95"}
              )
            }
          />
          <div
            id={@id <> "-menu"}
            class="dropdown absolute hidden right-0 z-10 shadow rounded-md bg-zinc-100"
          >
            <%= render_slot(@actions) %>
          </div>
        </div>
      </div>
      <div class="text-wrap break-words text-sm mb-1">
        <%= render_slot(@inner_block) %>
      </div>
    </div>
    """
  end

[josevalim]

@ponychicken what is your LV version? It may have been fixed in more recent ones.

[ponychicken]

I was on rc6 and just upgraded to rc7 with the same result

[SteffenDE]

phx-click-away={JS.hide(to: "##{@id}-menu")}

Ah well in that case it's not a LV bug as you are the one passing the selector. When you said

The problem is that if there is one element in the HEEX with an invalid id and this element has a phx-click attribute

I thought you were talking about the id="invalid/id" itself being a problem (that would be a bug), but it is indeed expected that things will break when you build an invalid selector.

The correct solution here would be to manually escape the strings before passing them to JS.hide, for example by using Floki's Floki.css_escape/1 (not yet released) or https://github.com/SteffenDE/css_escape (not released on Hex, but you can use {:css_escape, github: "SteffenDE/css_escape"} in your mix.exs). We cannot fix this problem in LV, as there's no way to escape this automatically. If you pass #invalid/id-123 and we'd simply escape that string, it would become \\#invalid\\/id-123, also escaping the #.

[josevalim]

@SteffenDE maybe we should make css_escape part of Phoenix.HTML? Going back to the starting point here. :)

[SteffenDE]

@ponychicken this will be part of the next :phoenix_html release, as well as the next Floki release. Together with the already mentioned library (https://github.com/SteffenDE/css_escape), you'll have plenty of options :)

I hope that this helps!

[ponychicken]

@SteffenDE Thank you <3333