Fix a validation bug

[ollietreend]

What

This PR fixes a couple of bugs in the way generated HTML markup was being sanitised and validated.

1. Validation before post-processing

Validation is now performed before post-processors have acted upon the generated HTML. This means that disallowed HTML tags and attributes can now be inserted into a document by post-processors – since they are trusted code – but still cannot be inserted into the source Markdown by users.

In other words, the order of processing is now:

[ User input ] --> [ Validation ] --> [ Post-processing ]

This seems to be the most logical way for validation to be performed, although it wasn't actually being done that way in practice.

Previously, post-processing would happen before the validation step:

[ User input ] --> [ Post-processing ] --> [ Validation ]

Resulting in changes in post-processed elements – such as button elements – to be run through the validator.

2. Apply allowed_image_hosts consistently

The first change resulted in tests covering the allowed_image_hosts sanitiser option to fail.

It turns out the allowed_image_hosts wasn't being carried through to the Govspeak::Document object which performed the final HTML conversion & sanitisation, meaning this rule wasn't being applied consistently between the validation and final markup produced.

Why is this needed?

This was brought to our attention by users of Publishing apps who have recently become unable to save documents containing Start Button elements.

Whenever a document contained the following snippet of Govspeak-flavoured Markdown:

{button start}[Start Now](https://gov.uk/example-service){/button}

a validation error would be displayed, and the user's changes wouldn't save.

Therefore, users have been unable to create/update any pages that happen to contain a start button.

Why did this happen?

This bug only appeared recently, and it wasn't caused by a change in this gem.

This gem is dependent on the govuk_publishing_components gem to render button elements. A recent change to that gem, which tweaked the HTML markup used for buttons, had the side effect of exposing a flaw in the way validation was being performed in this gem.

The new HTML markup for buttons was triggering a validation error due to it using disallowed HTML attributes – even though this wasn't HTML entered by the user, but was instead generated by the govuk_publishing_components gem.

Visual Changes

None.

Trello: https://trello.com/c/HQ85E1ux/1315-publishers-unable-to-save-drafts-when-there-is-a-start-button

[ollietreend]

Additional thoughts about this code

The interfaces of some of these classes – particularly the Govspeak::HtmlSanitizer class – feel quite messy to me.

HtmlSanitizer options

For example, some sanitisation options need to be passed in to the HtmlSanitizer constructor, where others are passed in to the HtmlSanitizer#sanitize instance method.

This leads to code such as:

HtmlSanitizer.new(kramdown_doc.to_html, allowed_image_hosts: @allowed_image_hosts)
             .sanitize(allowed_elements: @allowed_elements)

which needs to know that :allowed_image_hosts has to be given to the constructor, but :allowed_elements is given to the #sanitize method.

Whilst I'd be keen to refactor this, there's a risk that consumers of this gem have come to rely upon this interface. After all, it's been this way for 8 years at the time of writing.

sanitize_config

It'd also be nice to refactor the way configuration is passed into the third-party Sanitize gem.

Currently the configuration is defined in two different methods (#sanitize and #sanitize_config) and is then merged. It'd be nicer to have a single place for this config to live.

However, since both methods are public, it's another case where consumers could depend on these methods behaving as they currently do. In fact, this is exactly what alphagov/hmrc-manuals-api does – see here.

[brucebolt]

Great investigation work @ollietreend! Code changes all look good to me.

There appears to be a lack of testing around the post processing. I wonder if increasing the test coverage there would have caught this problem and blocked the PR that bumped us to the newer version of govuk_publishing_components?