Merge pull request #6448 from alphagov/attributes-double-escape

Author: romaricpascal

CHANGELOG.md

@@ -4,6 +4,12 @@ For advice on how to use these release notes, see [our guidance on staying up to
 
 ## Unreleased
 
+### Fixes
+
+We've made fixes to GOV.UK Frontend in the following pull requests:
+
+- [#6351: Preserve already escaped `attributes` values to prevent double escaping](https://github.com/alphagov/govuk-frontend/pull/6351) thanks to @colinrotherham for fixing this issue
+
 ## v6.0.0-beta.1 (Beta breaking release)
 
 ### Breaking changes
@@ -272,7 +278,6 @@ We've made fixes to GOV.UK Frontend in the following pull requests:
 
 - [#5311: Remove non-operational value parameter from file upload component](https://github.com/alphagov/govuk-frontend/pull/5311)
 - [#6434: Fix rebranded header background being visible when printed](https://github.com/alphagov/govuk-frontend/pull/6434) - thanks to @lewis-softwire for reporting this issue
-- [#6447: Fix pagination outputting empty links when provided a null or empty value](https://github.com/alphagov/govuk-frontend/pull/6447) – thanks to @NikhilNanjappa for reporting this issue
 
 ## v6.0.0-beta.0 (Beta breaking release)
 

packages/govuk-frontend/src/govuk/macros/attributes.njk

@@ -62,30 +62,46 @@
   @param {{ [attribute: string]: string | { value: string, optional?: boolean } } | string} attributes - Component attributes param
 #}
 {% macro govukAttributes(attributes) -%}
-  {#- Default attributes output -#}
+  {#- Default attributes output as string -#}
   {% set attributesHtml = attributes if attributes is string else "" %}
 
-  {#- Append attribute name/value pairs -#}
-  {%- if attributes is mapping %}
-    {% for name, value in attributes %}
-      {#- Detect if this is a `safe` filtered value. Just pass the value through if so. -#}
-      {#- https://github.com/alphagov/govuk-frontend/issues/4937 -#}
-      {% if value is mapping and not [undefined, null].includes(value.val) and value.length %}
-        {% set value = value.val %}
-      {% endif %}
+  {#- Default attributes output as `safe` filtered string -#}
+  {%- if attributes is escaped and attributes.val is string %}
+    {% set attributesHtml = attributes %}
 
+  {#- Append attribute name/value pairs -#}
+  {%- elif attributes is mapping and attributes is not escaped %}
+    {% for name, item in attributes %}
       {#- Set default attribute options -#}
-      {% set options = value if value is mapping else {
-        value: value,
+      {% set options = item if item is mapping and item is not escaped else {
+        value: item,
         optional: false
       } %}
 
+      {#- Remove values when not provided -#}
+      {%- if options.value in [undefined, null] %}
+        {% set value = null %}
+        {% set valueEscaped = "" %}
+
+      {#- Extract unescaped value when `safe` filtered -#}
+      {#- https://github.com/alphagov/govuk-frontend/issues/4937 -#}
+      {%- elif options.value is escaped %}
+        {% set value = options.value.val %}
+        {% set valueEscaped = options.value %}
+
+      {#- Otherwise assume escaping is necessary -#}
+      {#- https://github.com/alphagov/govuk-frontend/issues/4940 -#}
+      {%- else %}
+        {% set value = options.value %}
+        {% set valueEscaped = options.value | escape %}
+      {% endif %}
+
       {#- Output ` name` only (no value) for boolean attributes -#}
-      {% if options.optional === true and options.value === true %}
+      {% if options.optional === true and value === true %}
         {% set attributesHtml = attributesHtml + " " + name | escape %}
       {#- Skip optional empty attributes or output ` name="value"` pair by default -#}
-      {% elif (options.optional === true and not [undefined, null, false].includes(options.value)) or options.optional !== true %}
-        {% set attributesHtml = attributesHtml + " " + name | escape + '="' + options.value | escape + '"' %}
+      {% elif (options.optional === true and value not in [undefined, null, false]) or options.optional !== true %}
+        {% set attributesHtml = attributesHtml + " " + name | escape + '="' + valueEscaped + '"' %}
       {% endif %}
     {% endfor %}
   {% endif -%}

packages/govuk-frontend/src/govuk/macros/attributes.unit.test.mjs

@@ -1,5 +1,5 @@
-import { renderMacro } from '@govuk-frontend/lib/components'
-import nunjucks from 'nunjucks'
+import { renderMacro, renderString } from '@govuk-frontend/lib/components'
+import { outdent } from 'outdent'
 
 describe('attributes.njk', () => {
   describe('govukAttributes', () => {
@@ -245,11 +245,22 @@ describe('attributes.njk', () => {
         'govukAttributes',
         'govuk/macros/attributes.njk',
         {
-          context: ' data-attribute="value"'
+          context: ' data-attribute="Testing & more"'
         }
       )
 
-      expect(attributes).toBe(' data-attribute="value"')
+      expect(attributes).toBe(' data-attribute="Testing & more"')
+    })
+
+    it('outputs attributes from the `safe` filter if already stringified', () => {
+      // Render directly otherwise nunjucks `renderMacro()` will stringify
+      // safe `is escaped` instances into plain `is mapping` objects
+      const attributes = renderString(outdent`
+        {%- from "govuk/macros/attributes.njk" import govukAttributes -%}
+        {{- govukAttributes(' data-attribute="Testing & more"' | safe) -}}
+      `)
+
+      expect(attributes).toBe(' data-attribute="Testing & more"')
     })
 
     it('outputs nothing if there are no attributes', () => {
@@ -265,21 +276,53 @@ describe('attributes.njk', () => {
     })
 
     it('outputs values that are passed from the `safe` filter', () => {
-      // Set up a tiny Nunjucks environment, just so we can get the native `safe` filter
-      const nunjucksEnv = nunjucks.configure([])
+      // Render directly otherwise nunjucks `renderMacro()` will stringify
+      // safe `is escaped` instances into plain `is mapping` objects
+      const attributes = renderString(outdent`
+        {%- from "govuk/macros/attributes.njk" import govukAttributes -%}
 
-      const attributes = renderMacro(
-        'govukAttributes',
-        'govuk/macros/attributes.njk',
-        {
-          context: {
-            'data-text': 'Testing',
-            'data-safe-text': nunjucksEnv.getFilter('safe')('Testing')
-          }
-        }
+        {{- govukAttributes({
+          'data-text': 'Testing',
+          'data-unsafe-text': 'Testing & more',
+          'data-safe-text': 'Testing & more' | safe,
+          'data-escaped-text': 'Testing & more' | escape,
+          'data-double-escaped-text': 'Testing & more' | escape
+        }) -}}
+      `)
+
+      expect(attributes).toBe(
+        ' data-text="Testing" data-unsafe-text="Testing & more" data-safe-text="Testing & more" data-escaped-text="Testing & more" data-double-escaped-text="Testing & more"'
       )
+    })
+
+    it('outputs values from mappings that are passed from the `safe` filter', () => {
+      // Render directly otherwise nunjucks `renderMacro()` will stringify
+      // safe `is escaped` instances into plain `is mapping` objects
+      const attributes = renderString(outdent`
+        {%- from "govuk/macros/attributes.njk" import govukAttributes -%}
 
-      expect(attributes).toBe(' data-text="Testing" data-safe-text="Testing"')
+        {{- govukAttributes({
+          'data-text': {
+            value: 'Testing'
+          },
+          'data-unsafe-text': {
+            value: 'Testing & more'
+          },
+          'data-safe-text': {
+            value: 'Testing & more' | safe
+          },
+          'data-escaped-text': {
+            value: 'Testing & more' | escape
+          },
+          'data-double-escaped-text': {
+            value: 'Testing & more' | escape
+          }
+        }) -}}
+      `)
+
+      expect(attributes).toBe(
+        ' data-text="Testing" data-unsafe-text="Testing & more" data-safe-text="Testing & more" data-escaped-text="Testing & more" data-double-escaped-text="Testing & more"'
+      )
     })
   })
 })