Make it clear that the "html" attribute can be unsafe

[paroxp]

What

We had a discussion on a design system slack channel about the documentation not to encourage the use of html parameter in macros. We concluded, we shouldn't take away that functionality, but make it clear it's unsafe to use, causing people to rethink the strategy or apply necessary fixes before hand.

Why

It's more efficient to copy and paste a block of text without realising what it's doing. It could easily bring some XSS vulnerability into the application by accident, which we'd all like to avoid.

Proposal

Changing the name of the html attribute is a good start. This will need to be followed by some documentation changes on the Design System.

Before

{% from "panel/macro.njk" import govukPanel %}

{{ govukPanel({
  "titleText": "Application complete",
  "html": "Your reference number<br><strong>HDJ2123F</strong>"
})
}}

After

{% from "panel/macro.njk" import govukPanel %}

{{ govukPanel({
  "titleText": "Application complete",
  "unsafeHTML": "Your reference number<br><strong>HDJ2123F</strong>"
})
}}

Connection

alphagov/govuk-design-system#175

[edwardhorsford]

This suggested wording feels scary. I appreciate that's the point, but wonder if it's too strong. Used correctly, presumably it isn't unsafe.

If there aren't alternatives to providing html (such as the example given), do we really want such a scary key name?

[NickColley]

Another good example of this is React's dangerouslySetInnerHtml API.

[36degrees]

Valid concerns – we need to balance the two carefully. FWIW, we have spent some time trying to address this before, and our solution was to mimick the javascript API by consistently using text and html. But I agree that it could definitely be clearer.

[paroxp]

By all means, this only is a quick proposal. I imagine it will have to go through some user research before it's in stone.

[paroxp]

How does unprotectedHTML sound? :D

/cc @edwardhorsford

[NickColley]

I've put together some examples of different ways to pass html to macros and the outputs:

• Default has an XSS example input: https://nunjucks-passing-html-examples.glitch.me/
• Expected input: https://nunjucks-passing-html-examples.glitch.me/?input=Nick’s

I believe from the result of these that the only safe way to do this is to use call

Which would look something like:

{{ button({
  text: 'Inputs with text could still use a text parameter but would always escape <strong>HTML</strong>'
}) }}

Inputs with text could still use a text parameter but would always escape HTML

{% call button({}) %}
   Inputs that require unescaped <strong>HTML</strong>
{% endcall %}

Inputs that require unescaped HTML would use call, but this could also be used for plain text too if required. This means if a user wants to escape potentially dangerous HTML, this is something they have to explicitly do.

We could then consider renaming our current 'html' parameter to something that explains it's danger, if for some reason a user really wants to inject a variable directly.

[NickColley]

Related: what impact does this have with autoescaping turned off? https://github.com/alphagov/govuk-frontend/blob/master/app/app.js#L27

Could we use an environment global to make prototyping easier?

[joelanman]

Just to note that in prototyping, a 'scary' sounding name would be offputting to people already daunted by code, and using unescaped html in a prototype is probably fine anyway.

[NickColley]

In this case, we've not seen much adoption of macros in the research relating to prototyping yet.

Security should always come first in this regard. Cross site scripting is one of the most common forms of attack and can have disastrous consequences.

[joelanman]

Currently the agreed goal of the team is to get people mostly using macros in prototypes if we can.

Are there examples of other similar libraries using scary naming to tackle this issue?

[NickColley]

Real world evidence of how our approach will result in security vulnerabilities.

alphagov/govuk_publishing_components#305

[joelanman]

Not sure thats the same case? The old code appeared to use html_safe without the user explicitly calling it. Ours doesn't do that

 <% if info_text %>
   <span class="gem-c-button__info-text">
     <%= info_text.try(:html_safe) %>
   </span>
 <% end %>

[NickColley]

Only difference with ours is we have a 'html' property which will result in the same problem that they have had, since it's not clear what you're opting into.

This makes escaping and calling html_safe the exclusive responsibility of the application.

[NickColley]

I've put together an example that (I hope) meets the needs for macros.

1. Safe by default in production, with an explicit opt-in.
   • Typical input in production
   • Dangerous input in production
2. Simple for designers when protoyping
   • Typical input in a prototype
   • Dangerous input in a prototype

How does it work?

Instead of calling

{{ params.html | safe if params.html else params.text }}

we would instead call

{{ text(params) }}

Where text is a new utility macro

{% macro text(params) %}
  {% set text = params.text if params.text %}

  {# Set the default to the global variable, if it does not exist this will be falsy #}
  {% set dangerouslySetText = defaultDangerouslySetText %}

  {# If the developer has opted-in or opted-out to set the text dangerously, override the default #}
  {% if params.dangerouslySetText === true or params.dangerouslySetText === false %}
    {% set dangerouslySetText = params.dangerouslySetText %}
  {% endif %}

  {% if dangerouslySetText %}
    {% set text = text | safe %}
  {% endif %}

  {{ text }}
{% endmacro %}

This new macro checks for a global variable defaultDangerouslySetText to be set, which we would enable in the GOV.UK Prototype Kit, that allows text to be input without worrying about how secure it is. (You can see this in the prototyping glitch example above)

This global variable would require a user of GOV.UK Frontend to add it themselves explicitly using

environment.addGlobal('defaultDangerouslySetText', true)

Params such as dangerouslySetText are up for debate, we could mirror Nunjucks and call this safe instead. (Which is in this proposal: #752)

See the glitch app source code for a complete example of how this works: https://glitch.com/edit/#!/nunjucks-text-safe?path=views/index.html:49:119

[NickColley]

Another example of GOV.UK avoiding unsafe HTML: alphagov/govuk_publishing_components#356

Note they choose to use the caller nested style api, which would also work fine in Nunjucks.

[hannalaakso]

Discussed with team and decided we'd like to learn more about whether users realise this is an issue and how we could do more to help users to ensure their application is not exposed to any related vulnerabilities. Created a research issue: https://trello.com/c/T3tLnQWf

[joelanman]

I think one of the main things we can do is avoid the use of html as much as possible in our examples. If common patterns have to use html maybe thats a sign the macro needs to change somehow, for example by using call instead, or by adding some more parameters.

[36degrees]

I think we definitely can and should do more to support call, but it's only going to be helpful for components that have one obvious 'area' that accepts HTML, and it won't help where components call other components (such as passing HTML to the label that's inside a text input).

[joelanman]

I guess the main point I'm trying to make is, html param should be an exception - if we're having to use it commonly that seems like we could investigate that macro to meet the common need a different way. The idea of macros is to avoid people writing their own html.

[NickColley]

@joelanman @36degrees I have put a spike (private trello sorry: https://trello.com/c/jn54p5Cz/1357-spike-explore-more-flexible-use-of-macros-what-if-we-only-used-nested-components) in the backlog that would explore what components would look like if they used caller in this fashion.

[hannalaakso]

Hi @paroxp

We have added the following to the options tables of component pages in the Design System:

If you're using Nunjucks macros in production with "html" options, or ones ending with "html", you must sanitise the HTML to protect against [cross-site scripting exploits](https://developer.mozilla.org/en-US/docs/Glossary/Cross-site_scripting).

Does this address your concerns? We also plan to include a description of the issue in our own words in Using Nunjucks Macros in the Get Started for Production page.

[paroxp]

Although, that's a good direction you took there, are you comfortable with the fact that people may not read notes but blindly copy and paste the macro (not saying from your docs, but perhaps different code snippet).

In fairness, there is only so much you can do to assist your users and they will run under the bus anyway :D

I may consider adding an extra test to our codebase which will attempt to grep for presumably unsecure use of html tag and fail if any found - but that may not be a solution for everyone.

(\"|\')?html(\"|\')?:\s?(\w+)?(\'|\")?(\w+|(.+)?(\{+\w+\}+)(.+)?)(\'|\")?

[hannalaakso]

Thanks a lot for your comments @paroxp, they're all being taken aboard 🙌

@NickColley has done some great work above at looking at the options for making this safer. We use set for creating the HTML in our examples and we've got a card in our backlog to look at making this better. We're also looking to improve the guidance around this, and to run a spike to use caller more in our macros.

Let us know how you get on with the tests, we'd be really interested in hearing more about it.

[timpaul]

Closing this for now, unless the issue is raised again