Special characters in tags field saved escaped when selected using autocomplete

[silllli]

Description

I have a tags field that gets its options from a query:

fields:
  bikeManufacturer:
    type: tags
    max: 1
    options: query
    query: page.parent.childrenAndDrafts.pluck('bikeManufacturer', ',', true)
    required: true

When I save a tag featuring an ampersand (e. g. “Riese & Müller“) it gets saved correctly. But when I then go to a sibling page and select that same value from the autocomplete options, it’s getting saved and output as Riese & Müller.

Expected behavior
The value being saved just like the original.

To reproduce

1. Create a blueprint using the YAML provided above.
2. Add a page and save a value featuring an ampersand in the tags field.
3. Create a second page and select the previously saved value from the autocomplete options.
4. Create a third page and look at the provided autocomplete options or the value in the just saved txt file of the second page.

Your setup

Kirby Version

3.6.1.1

Console output

Your system (please complete the following information)

• Device: MacBook Pro
• OS: macOS 12.0.1
• Browser: Google Chrome
• Version: 96.0.4664.110

Maybe related to #4041.

[silllli]

It also happens with select fields that access values of a structure field in their query.

[silllli]

Do you know when there will be a fix available? My client is filling their website with content and saving dozens of entries with wrong data. 🥴

[silllli]

This bug was introduced in version 3.6.0.

[lukasbestle]

Issue summary

Status quo

This issue occurs because we use Str::safeTemplate() for both the display text and the value: https://github.com/getkirby/kirby/blob/351b6348348cf78aed9a65920358caf218473102/src/Form/OptionsQuery.php#L128-L129 https://github.com/getkirby/kirby/blob/351b6348348cf78aed9a65920358caf218473102/src/Form/OptionsQuery.php#L93-L106

Because the value is escaped, the stored value will also be escaped when the option is selected.

Rough solution draft

The issue can be solved by only using safeTemplate() for the text and template() for the value. Diff:

diff --git a/src/Form/OptionsQuery.php b/src/Form/OptionsQuery.php
index ba4660f7..be71cfb5 100644
--- a/src/Form/OptionsQuery.php
+++ b/src/Form/OptionsQuery.php
@@ -86,11 +86,10 @@ class OptionsQuery
     /**
      * @param string $object
      * @param string $field
-     * @param array $data
      * @return string
      * @throws \Kirby\Exception\NotFoundException
      */
-    protected function template(string $object, string $field, array $data)
+    protected function templateValue(string $object, string $field)
     {
         $value = $this->$field();

@@ -102,7 +101,7 @@ class OptionsQuery
             $value = $value[$object];
         }

-        return Str::safeTemplate($value, $data);
+        return $value;
     }

     /**
@@ -125,8 +124,8 @@ class OptionsQuery
             $data  = array_merge($data, [$alias => $item]);

             $options[] = [
-                'text'  => $this->template($alias, 'text', $data),
-                'value' => $this->template($alias, 'value', $data)
+                'text'  => Str::safeTemplate($this->templateValue($alias, 'text'), $data),
+                'value' => Str::template($this->templateValue($alias, 'value'), $data)
             ];
         }

This can be a possible temporary fix for you @silllli if you are not affected by the side effects below.

Side effects

• This seems to break the select field for some reason. If an option with an escaped text is selected (= when text differs from value), the field is not considered valid and cannot be saved. That said, the select field display is currently broken as well because it does not use v-html for displaying the options, so the text is escaped twice.
• The multiselect field displays both the text and the value in the dropdown (the value in parentheses). If we didn't escape the value in the backend, the field displays it unescaped (XSS attack vector!). So we also need to update the multiselect field to escape the value on display.

Summary

• We need a refactoring of the Options classes that makes sure that all combinations of options fields and option settings (query, API, fixed list) work. It is not possible to fix this issue with an isolated change without breaking other option fields.
• We need to ensure that all frontend components for all option fields consistently handle the text and value props, i.e. they should all escape the value on output for display but not the text.

[distantnative]

@lukasbestle

the select field display is currently broken as well because it does not use v-html for displaying the options

We use the standard <option> tags which don't allow HTML inside - I don't really see a good forward that doesn't require to totally refactor the select input to something custom :/

Current state of refactoring the backend: https://github.com/getkirby/kirby/compare/develop...lab/nico/4043-options-escape

[lukasbestle]

@distantnative

We use the standard <option> tags which don't allow HTML inside

Shouldn't the <option> just ignore the HTML though and render the text without formatting? Otherwise a simple solution could be to strip the HTML tags in the select field component before the text is printed to the <option>. In any case we must not double-escape here.

[iamwebrocker]

Can confirm that this also exists in a 3.7 (3.7.0.1). Just ran into this unexpected behavior on my site, which obviously if this exists since 3.6.x, I don't update too often, content-wise :-) Since the content txt files may be used for other outlets, I think there shouldn't be an escaped char like "&" for "&" in there, even if in my case I could fix my templates to work with the "&"s instead.

[bastianallgeier]

✅