Skip to content

Commit

Permalink
Replace inline style attributes with classes
Browse files Browse the repository at this point in the history
We aspire to disallow inline style elements on GOV.UK as these present a
XSS security risk [1]. However this change was scuppered [2] because
Govspeak has one scenario (text alignment in Kramdown) which uses inline
styles [3].

The approach I've taken to resolve this is to use the post-processor as
a way to replace the style elements on td and th elements with class
parameters. This change won't be safe to be merged until
govuk_publishing_components is updated to apply the styles of the class.

This handles a couple of edge cases, that may not be that necessary. It
can cope if the style attribute is given multiple text-align rules and
will use the last one (as CSS would). It also can handle the scenario
where this already a class on the td/th.

Once the GOV.UK stack migrates existing content to use it can remove the
leniency that allows style elements in the sanitizer rules. I've left a
comment as a reminder that this could be removed (hopefully I'll
remember).

[1]: https://github.com/alphagov/govuk_app_config/blob/13083f8f8563c4703c14fe8175ca35646ec64442/lib/govuk_app_config/govuk_content_security_policy.rb#L64-L67
[2]: alphagov/govuk_app_config#279 (comment)
[3]: https://github.com/gettalong/kramdown/blob/0b0a9e072f9a76e59fe2bbafdf343118fb27c3fa/test/testcases/block/14_table/header.html#L37-L56
  • Loading branch information
kevindew committed Mar 17, 2023
1 parent 4061981 commit f50d51f
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 10 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## Unreleased

* Replace inline style attributes in td/th elements with classes [#268](https://github.com/alphagov/govspeak/pull/268)

## 7.0.2

* Fix for abbreviations nested in button. [#267](https://github.com/alphagov/govspeak/pull/267)
Expand Down
2 changes: 2 additions & 0 deletions lib/govspeak/html_sanitizer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ def sanitize_config(allowed_elements: [])
"svg" => Sanitize::Config::RELAXED[:attributes][:all] + %w[xmlns width height viewbox focusable],
"path" => Sanitize::Config::RELAXED[:attributes][:all] + %w[fill d],
"div" => [:data],
# @TODO These style attributes can be removed once we've checked there
# isn't hardcoded HTML in documents that uses them
"th" => Sanitize::Config::RELAXED[:attributes]["th"] + %w[style],
"td" => Sanitize::Config::RELAXED[:attributes]["td"] + %w[style],
"govspeak-embed-attachment" => %w[content-id],
Expand Down
17 changes: 17 additions & 0 deletions lib/govspeak/post_processor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,23 @@ def self.extension(title, &block)
end
end

extension("convert table cell inline styles to classes") do |document|
document.css("th[style], td[style]").each do |el|
style = el.remove_attribute("style")
matches = style.value.scan(/text-align:\s*(left|center|right)/).flatten

next unless matches.any?

class_name = "cell-text-#{matches.last}"

if el[:class]
el[:class] += " #{class_name}"
else
el[:class] = class_name
end
end
end

extension("use gem component for buttons") do |document|
document.css(".govuk-button").map do |el|
button_html = GovukPublishingComponents.render(
Expand Down
20 changes: 10 additions & 10 deletions test/govspeak_table_with_headers_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -58,21 +58,21 @@ def expected_outcome_for_table_with_alignments
<table>
<thead>
<tr>
<td style="text-align: left"></td>
<th style="text-align: center" scope="col">Second Column</th>
<th style="text-align: right" scope="col">Third Column</th>
<td class="cell-text-left"></td>
<th scope="col" class="cell-text-center">Second Column</th>
<th scope="col" class="cell-text-right">Third Column</th>
</tr>
</thead>
<tbody>
<tr>
<th style="text-align: left" scope="row">First row</th>
<td style="text-align: center">Cell</td>
<td style="text-align: right">Cell</td>
<th scope="row" class="cell-text-left">First row</th>
<td class="cell-text-center">Cell</td>
<td class="cell-text-right">Cell</td>
</tr>
<tr>
<th style="text-align: left" scope="row">Second row</th>
<td style="text-align: center">Cell</td>
<td style="text-align: right">Cell</td>
<th scope="row" class="cell-text-left">Second row</th>
<td class="cell-text-center">Cell</td>
<td class="cell-text-right">Cell</td>
</tr>
</tbody>
</table>
Expand Down Expand Up @@ -255,7 +255,7 @@ def document_body_with_table_headers_containing_links
assert_equal document_body_with_hashes_for_row_headers.to_html, expected_outcome
end

test "Cells are aligned correctly" do
test "Cells are given classes to indicate alignment" do
assert_equal document_body_with_alignments.to_html, expected_outcome_for_table_with_alignments
end

Expand Down

0 comments on commit f50d51f

Please sign in to comment.