Deprecate passing in unsafe HTML into Govspeak #356
Merged
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
andysellick
requested changes
Jun 5, 2018
| @@ -2,7 +2,7 @@ | |||
|
|
|||
| describe "All components" do | |||
| it "doesn't use `html_safe`" do | |||
| files_with_html_safe = `grep -rni "html_safe" app/views/govuk_publishing_components/components`.lines | |||
| files_with_html_safe = `grep -rni "html_safe " app/views/govuk_publishing_components/components`.lines | |||
There was a problem hiding this comment.
Lol I hoped you wouldn't notice. It's to avoid this test triggering on the deprecation message in the component.
| @@ -14,6 +14,21 @@ | |||
| <% if content.html_safe? %> | |||
| <%= content %> | |||
| <% else %> | |||
| <% puts " | |||
There was a problem hiding this comment.
I've not seen puts used like this in a template before. Will this render the text inside it as written, or will it all collapse into a single chunk?
There was a problem hiding this comment.
It looks like this in the output:
You've passed in unsanitised HTML into the govspeak component as the
`content` param.
Passing in unsafe HTML is deprecated and will be removed in a future
version. You need to pass in a block instead or use the `capture` helper.
See the component guide for examples.
If you're 100% sure there's no unsanitised user input in the string you
could also call `.html_safe` on the string or use the `raw` helper before
passing it in.
Called from /var/lib/jenkins/workspace/components_govspeak-content-LXCJ5KGV3W4GAGTNRFJXBGJGVEBXPWFEIRVZXHMIVRN7HGSF5OKA/app/views/govuk_publishing_components/component_guide/show.html.erb:16:in `__var_lib_jenkins_workspace_components_govspeak_content______________________________________________________app_views_govuk_publishing_components_component_guide_show_html_erb___1824013196706400841_47344039424500'
andysellick
approved these changes
Jun 5, 2018
vanitabarrett
approved these changes
Jun 5, 2018
The block will not have `raw` called on it, so it will be the safer option to pass in content.
When the HTML has been declared safe (either by constructing it properly, or using `html_safe`, or `raw) we won't mark it as safe again. This will prevent accidental marking as safe of unsafe content. Note that `raw foo` is the same as `foo.html_safe`.
When you pass in unsafe HTML into the component like this: ``` <%= render 'govuk_publishing_components/components/govspeak', content: "<p>Foo</p>" %> ``` You will now see deprecation message with instructions on how to better input content. I've chosen the message instead of just an error to avoid having to update all the apps at once.
Also adds tests for the blocks.
This silences the warnings.
We use `raw` because in some case the input is `nil` and will crash with `html_safe`.
This will be passed into the govspeak component, so it needs to be HTML safe.
This is the first example because it should be the main usage.
And skip govspeak for now.
|
Updated this PR to add tests, fix a problem with govspeak-htmlpub and fix the deprecated usage in the component guide. |
vanitabarrett
approved these changes
Jun 6, 2018
tijmenb
added a commit
to alphagov/email-alert-frontend
that referenced
this pull request
Jun 8, 2018
The Govspeak component has moved from Static to the gem. This switches this app to use the gem-based component. To make the transition easier I've added a `govspeak` helper that takes a block. This cleans up the views a lot and makes it easier to read the HTML. It also makes sure that we're using Rails' HTML escaping mechanism by default, which is needed because the component will in the future no longer call `.html_safe` on the input (alphagov/govuk_publishing_components#356). This fixes at least one (unexploitable) [XSS vulnerability in this app][1] (`@address` is add to the page unescaped). [1]: https://github.com/alphagov/email-alert-frontend/blob/73dc8d4db0174b215f c61c42fa8ad992f2d8f6d2/app/views/authentication/request_sign_in_token.ht ml.erb#L17 https://trello.com/c/uwcWXXNp
tijmenb
added a commit
to alphagov/government-frontend
that referenced
this pull request
Jun 8, 2018
This makes sure that we only pass in safe HTML to the govspeak component, because in the future the component won't call `html_safe` on it for us (alphagov/govuk_publishing_components#356). In some cases I've used `raw` because the input may be nil.
tijmenb
added a commit
to alphagov/government-frontend
that referenced
this pull request
Jun 8, 2018
This makes sure that we only pass in safe HTML to the govspeak component, because in the future the component won't call `html_safe` on it for us (alphagov/govuk_publishing_components#356). In some cases I've used `raw` because the input may be nil.
tijmenb
added a commit
to alphagov/government-frontend
that referenced
this pull request
Jun 8, 2018
This makes sure that we only pass in safe HTML to the govspeak component, because in the future the component won't call `html_safe` on it for us (alphagov/govuk_publishing_components#356). In some cases I've used `raw` because the input may be nil.
tijmenb
added a commit
to alphagov/email-alert-frontend
that referenced
this pull request
Jun 11, 2018
The Govspeak component has moved from Static to the gem. This switches this app to use the gem-based component. To make the transition easier I've added a `govspeak` helper that takes a block. This cleans up the views a lot and makes it easier to read the HTML. It also makes sure that we're using Rails' HTML escaping mechanism by default, which is needed because the component will in the future no longer call `.html_safe` on the input (alphagov/govuk_publishing_components#356). This fixes at least one (unexploitable) [XSS vulnerability in this app][1] (`@address` is add to the page unescaped). [1]: https://github.com/alphagov/email-alert-frontend/blob/73dc8d4db0174b215f c61c42fa8ad992f2d8f6d2/app/views/authentication/request_sign_in_token.ht ml.erb#L17 https://trello.com/c/uwcWXXNp
Merged
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.
Currently, when passing in HTML into the Govspeak component the component will mark the input as "HTML safe" (using
raw), meaning it will be rendered on the page without sanitisation.A simple situation like this, where
@baris untrusted user input, will result in an XSS vulnerability.We currently have 1 such vulnerability in email-alert-frontend, but it's not exploitable.
A better way to pass in input is to use a block, which will use sanitisation by default:
This PR deprecates (but doesn't remove) the functionality to pass in untrusted input. See commits for details.