Disallow usage of style element and style attribute #279
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ollietreend
reviewed
Jul 4, 2023
This forbids a user from inputting a style element as part of their govspeak markdown or using the style attribute in their HTML. Thus it prevents publishers from customising the style of a GOV.UK page. I am very confident we never intended to let users do this and consider this a bug fix - luckily, the evidence of usage is minimal. This bug provides publishers with an unprecedented ability to re-style a GOV.UK page in a way far outside of the GOV.UK brand guidelines. The reason I'm confident this was not intended is because we went to special effort to only style on table cell elements 7 years ago [1]. It appears that when we adopted Sanitize::Config::Relaxed in 2012 [2], style was not allowed as it was later added to Sanitize::Config::Relaxed in 2014 [3]. [1]: b5b337e [2]: c0fab20 [3]: rgrove/sanitize@86d53f9
I put the initial TODO comment as I was under the false impression that these special rules could be removed once any hardcoded HTML was removed. However these rules remain needed as they have to permit the HTML that Kramdown generates, including style attributes for table cells. These style attributes are then replaced in a post-processor method, but they're still needed to be permitted at this point as sanitisation happens before the post-processor.
There isn't any need to expand all attributes as these ones are allowed implicitly.
kevindew
force-pushed
the
remove-style-from-allowlist
branch
from
fdb5941 to
19a0d53
Compare
I intend to extend this test to cover tables outside of those with headers (admittedly I think this file is already testing beyond that).
These tests were mis-configured with the actual argument first before the expected. Minitest convention is to have expected as the first argument then the actual as the second one.
As suggested in the PR review for the changes we can simplify the use of a transformer class to instead use sanitize config for it [1]. This does have a minor behavior difference that it will allow text-align with invalid attributes, however since we replace style attributes on table cells in post-processing this has no user consequence. [1]: #279 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This forbids a user from inputting a style element as part of their
govspeak markdown or using the style attribute in their HTML.
Thus it prevents publishers from customising the style of a GOV.UK page.
I am very confident we never intended to let users do this and consider
this a bug fix - luckily, the evidence of usage is minimal. This bug
provides publishers with an unprecedented ability to re-style a GOV.UK
page in a way far outside of the GOV.UK brand guidelines.
The reason I'm confident this was not intended is because we went to
special effort to only style on table cell elements 7 years ago [1]. It
appears that when we adopted Sanitize::Config::Relaxed in 2012 [2],
style was not allowed as it was later added to Sanitize::Config::Relaxed
in 2014 [3].
The motivation for making this change is that, with the GOV.UK Content
Security Policy
style is blocked, so even if users could use this feature it wouldn't change
the frontend (but would generate reports).
I've been working through the apps to make sure publishers will only
have minimal effect from this change and know there's a couple of docs
in Whitehall that need fixing which will be co-ordinated with this gem release.