-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle webchat CSP modifications in application #2643
Conversation
Webchat integrations on GOV.UK require modifications to the Content Security Policy (CSP), these previously have been handled in govup_app_config [1]. Overtime we have ended up with these falling out of date, with more CSP entries than used cases. It is also a pain to deploy these as you need to update the gem first. This PR moves the CSP configuration into this application, which resolves these pain points and follows the convention in our CSP documentation [2] to not apply config that is just for a single application. This applies an approach where the CSP modification is only applied on the webchat page, so it won't be broadcast across all other GOV.UK pages. I set this up to meet the needs of the one webchat integration we have which has a single entry to connect-src. In future we may well also need to add a script-src integration as well. I also considered setting up the ability that connect-src could contain an array, but didn't implement it as there wasn't precedence of a webchat with this. It shouldn't be hard to configure though if that situation occurs. A slight difference to the integration in the govuk_app_config CSP is the prefix of `https://` [3]. Which requires any connections to omni.eckoh.uk use TLS. I'm assuming we just didn't know we could specify this when we first set-up our CSP and the precedence remained. Setting up an integration test for this functionality was a little bit of a pain as the controller tests don't execute the CSP code and thus don't allow testing. I imagine this would be resolved by switching to Rails request testing [4]. I therefore set the testing up for this in a Capybara integration test. I noticed while developing this that the omni.eckoh.uk provider seems to have an integration problem (the available resource returns 404) which I deemed as out-of-scope for this change. [1]: https://github.com/alphagov/govuk_app_config/blob/7f060692720df50a27f6845f052b04eae2246226/lib/govuk_app_config/govuk_content_security_policy.rb#L77-L84 [2]: https://docs.publishing.service.gov.uk/manual/content-security-policy.html [3]: https://github.com/alphagov/govuk_app_config/blob/7f060692720df50a27f6845f052b04eae2246226/lib/govuk_app_config/govuk_content_security_policy.rb#L83 [4]: https://guides.rubyonrails.org/testing.html#integration-testing
This seems to be information specific to the reference-webchat-proxy [1]. Which has been removed from GOV.UK and archived. [1]: https://github.com/alphagov/reference-webchat-proxy
This method didn't need to be as complex as it was as it does presence checks in the `validate!` method. attrs.fetch("open_url_redirect", nil), is just a more complex way to write attrs["open_url_redirect"].
bc9fa80
to
76071c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Kevin,
Apologies for the delay, I spent some time understanding webchat and reading through the various docs linked on Trello (which were really helpful). I think it looks good. I tested it out locally:
Updating the YAML to https://www.my-amazing-webchat.com:
A non-webchat page:
@@ -13,6 +13,10 @@ class ContentItemsController < ApplicationController | |||
|
|||
attr_accessor :content_item, :taxonomy_navigation | |||
|
|||
content_security_policy do |p| | |||
p.connect_src(*p.connect_src, -> { csp_connect_src }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to check if I have understood correctly... in the past i've defined the CSP for the application in the initializers somewhere. In this case we want to change the CSP specific to different parts of the application (webchat), so this is instead setting the CSP for content items but it is overriden by this method in contact_presenter for webchat to allow the extra value? Otherwise connect_src
is set to nil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup exactly that.
We configure a global CSP for the app via the initializer and then we can refine it a controller or action level.
This allows us to have three layers of CSP:
- In govuk_app_config, directives that cover all GOV.UK apps. These are a pain to change and test because they require a new gem release, but are good to apply one rule consistently everywhere. Best used for a change that applies to multiple apps e.g. analytics or govspeak
- Across an app, via the initializer. This offers us a chance to apply configuration that applies to just one app and every route in it. Much faster to change than the gem. Best used for a case where an app uses a library that other apps don't. There are no cases of this used yet, but I'm expecting it to be used so that apps without jQuery can be configured more securely than the apps that still use jQuery (licence-finder and Whitehall).
- In a controller, which can be specified to an action level, this allows just a single resource to set up extra rules for the CSP which is best for when there's an exception to the rules for a couple of pages, i.e this scenario. We have precedence for doing this in Smart Answers already: https://github.com/alphagov/smart-answers/blob/45a463407b84ed3e58b268f4522d7493dbdd40ba/app/controllers/smart_answers_controller.rb#L8-L12
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok cool, just wanted to check I wasn't missing something. Thanks for the explanation, makes sense :)
@@ -5,13 +5,14 @@ class Webchat | |||
|
|||
validates :base_path, :open_url, :availability_url, presence: true | |||
|
|||
attr_reader :base_path, :open_url, :availability_url, :open_url_redirect | |||
attr_reader :base_path, :open_url, :availability_url, :open_url_redirect, :csp_connect_src |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@open_url = attrs["open_url"] | ||
@availability_url = attrs["availability_url"] | ||
@open_url_redirect = attrs["open_url_redirect"] | ||
@csp_connect_src = attrs["csp_connect_src"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def looks better!
|
||
```yaml | ||
csp_connect_src: https://webchat.host | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -1,4 +1,5 @@ | |||
- base_path: /government/organisations/hm-passport-office/contact/hm-passport-office-webchat | |||
open_url: https://omni.eckoh.uk/v03.5/launcherV3.php?p=HMPO&d=717&ch=CH&psk=chat_a1&iid=STC&srbp=0&fcl=0&r=Chat&s=https://omni.eckoh.uk/v03.5&u=&wo=&uh=&pid=2&iif=0 | |||
availability_url: https://omni.eckoh.uk/v03.5/providers/HMPO/api/availability.php | |||
csp_connect_src: https://omni.eckoh.uk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
# reset back to default driver | ||
Capybara.use_default_driver | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -14,6 +15,7 @@ class WebchatTest < ActiveSupport::TestCase | |||
assert_equal(instance.base_path, webchat_config["base_path"]) | |||
assert_equal(instance.open_url, webchat_config["open_url"]) | |||
assert_equal(instance.availability_url, webchat_config["availability_url"]) | |||
assert_equal(instance.csp_connect_src, webchat_config["csp_connect_src"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Add exemption for two sites to the Content Security Policy An exemption was added in [PR 2643] to allow CSP exemptions for Webchat. This exemption only tested single domain exemptions. The change being asked for by HMPO currently has the open and availability urls on separate domains. The [connect-src directive] does handle multiple domains, so the tests have been updated to make sure that we too handle them correctly. Test output appends the new domains: - https://d2dzxxvvhcianw.cloudfront.net - https://d33w03tp7zv79z.cloudfront.net Content-Security-Policy: default-src 'self'; base-uri 'none'; img-src 'self' *.publishing.service.gov.uk *.dev.gov.uk www.gov.uk www.google-analytics.com ssl.google-analytics.com stats.g.doubleclick.net www.googletagmanager.com www.region1.google-analytics.com region1.google-analytics.com lux.speedcurve.com assets.digital.cabinet-office.gov.uk https://img.youtube.com; script-src 'self' www.google-analytics.com ssl.google-analytics.com stats.g.doubleclick.net www.googletagmanager.com www.region1.google-analytics.com region1.google-analytics.com www.gstatic.com *.ytimg.com www.youtube.com www.youtube-nocookie.com 'nonce-Y9BKtfGEN3f1/X0HhSD2eg=='; style-src 'self' www.gstatic.com; font-src 'self'; object-src 'none'; frame-src 'self' *.publishing.service.gov.uk *.dev.gov.uk www.gov.uk www.youtube.com www.youtube-nocookie.com; connect-src 'self' *.publishing.service.gov.uk *.dev.gov.uk www.gov.uk www.google-analytics.com ssl.google-analytics.com stats.g.doubleclick.net www.googletagmanager.com www.region1.google-analytics.com region1.google-analytics.com lux.speedcurve.com https://d2dzxxvvhcianw.cloudfront.net https://d33w03tp7zv79z.cloudfront.net [PR 2643]: #2643 [connect-src directive]: https://stackoverflow.com/questions/17789426/whitelist-multiple-domains-in-content-security-policy
Trello: https://trello.com/c/lxxx5XLZ/178-govuk-has-a-half-implemented-content-security-policy-csp
Webchat integrations on GOV.UK require modifications to the Content
Security Policy (CSP), these previously have been handled in
govup_app_config 1. Overtime we have ended up with these falling out
of date, with more CSP entries than used cases. It is also a pain to
deploy these as you need to update the gem first.
This PR moves the CSP configuration into this application, which
resolves these pain points and follows the convention in our CSP
documentation 2 to not apply config that is just for a single
application.
This applies an approach where the CSP modification is only applied on
the webchat page, so it won't be broadcast across all other GOV.UK
pages. I set this up to meet the needs of the one webchat integration we
have which has a single entry to connect-src. In future we may well also
need to add a script-src integration as well. I also considered setting
up the ability that connect-src could contain an array, but didn't
implement it as there wasn't precedence of a webchat with this. It
shouldn't be hard to configure though if that situation occurs.
A slight difference to the integration in the govuk_app_config CSP is
the prefix of
https://
3. Which requires any connections toomni.eckoh.uk use TLS. I'm assuming we just didn't know we could specify
this when we first set-up our CSP and the precedence remained.
Setting up an integration test for this functionality was a little bit
of a pain as the controller tests don't execute the CSP code and thus
don't allow testing. I imagine this would be resolved by switching to
Rails request testing 4. I therefore set the testing up for this in a
Capybara integration test.
I noticed while developing this that the omni.eckoh.uk provider seems to
have an integration problem (the available resource returns 404) which I
deemed as out-of-scope for this change.
Follow these steps if you are doing a Rails upgrade.