[Bug] Search result descriptions are double-escaped

[stof]

Here is what happens when searching for an enum in the French version of the website (using French is relevant because the French description contains accents):

[pronskiy]

Search for ssh2-auth-none to reproduce in English doc.

[pronskiy]

@lhsazevedo, is this escape needed?

- ${escape(description)}
+ ${description}

[lhsazevedo]

Hmm, better to play safe when inserting dynamic content. We could decode the entities before escaping:

- ${escape(description)}
+ ${escape(decodeHtmlEntities(description))}

I'll open a PR if that's OK

[pronskiy]

@lhsazevedo, taking a look at implementation of escape(), wouldn't that be doing it forth and back?

[lhsazevedo]

wouldn't that be doing it forth and back?

Yup, but only for HTML entities. Decoding with an textarea ignores HTML tags, and they will be correctly escaped by escape(). See this demo and this answer.

[lhsazevedo]

By the way, we only need to be this safe because we use innerHTML to insert the entire results markup string into the document. An alternative would be to build the results using DOM nodes and use textContent for untrusted content:

const el = document.createElement('a');
el.href = url;
el.className = 'search-modal__result';
el.setAttribute('role', 'option');
el.setAttribute('aria-labelledby', `search-modal__result-name-${i}`);
el.setAttribute('aria-describedby', `search-modal__result-description-${i}`);
el.setAttribute('aria-selected', 'false');

const iconEl = document.createElement('div');
iconEl.className = 'search-modal__result-icon';
iconEl.innerHTML = icon; // icon is trusted and safe

const contentEl = document.createElement('div');
contentEl.className = 'search-modal__result-content';

const nameEl = document.createElement('div');
nameEl.className = 'search-modal__result-name';
nameEl.id = `search-modal__result-name-${i}`;
nameEl.textContent = item.name; // use textContent for untrusted content

const descEl = document.createElement('div');
descEl.className = 'search-modal__result-description';
descEl.id = `search-modal__result-description-${i}`;
descEl.textContent = description; // use textContent for untrusted content

contentEl.append(nameEl, descEl);
el.append(iconEl, contentEl);
resultsElement.appendChild(el);

This is quite imperative though, which makes it harder to interpret the markup structure.
It's quite common to solve this using a hyperscript-like h()/createElement() helper, and the results may look familiar if you’re used to JSX:

resultsElement.append(
  a(
    {
      href: url,
      className: "search-modal__result",
      role: "option",
      ariaLabelledby: `search-modal__result-name-${i}`,
      ariaDescribedby: `search-modal__result-description-${i}`,
      ariaSelected: "false",
    },
    [
      div({ className: "search-modal__result-icon", innerHTML: icon }),
      div({ className: "search-modal__result-content" }, [
        div(
          {
            id: `search-modal__result-name-${i}`,
            className: "search-modal__result-name",
          },
          item.name,
        ),
        div(
          {
            id: `search-modal__result-description-${i}`,
            className: "search-modal__result-description",
          },
          description,
        ),
      ]),
    ],
  ),
);

While I'm Ok with this approach, it might be a bit of over-engineering...

[sy-records]

Search content is generated by phd based on doc-*, should we consider it safe?

[lhsazevedo]

Even then, I'd still vote for escaping it, since we don't want to render unexpected markup.