Skip to content

[LG-7191] add configuration flipper to suppress content security policy (CSP)#6715

Closed
gangelo wants to merge 2 commits intomainfrom
lg-7191-add-configuration-flipper-to-suppress-content-security-policy-csp
Closed

[LG-7191] add configuration flipper to suppress content security policy (CSP)#6715
gangelo wants to merge 2 commits intomainfrom
lg-7191-add-configuration-flipper-to-suppress-content-security-policy-csp

Conversation

@gangelo
Copy link
Contributor

@gangelo gangelo commented Aug 10, 2022

https://cm-jira.usa.gov/browse/LG-7191

Suppress content security policy (CSP) if IdentityConfig.store.suppress_content_security_policy is true.

ONLY to be set to true on an AWS sandbox or during local development!

508 tools that rely on javascript execution (ANDI for example) are unable to execute in the development environment due to Content Security Policy (CSP) policies.

Creating/setting IdentityConfig.store.suppress_content_security_policy in an AWS sandbox will allow developers to turn CSP off, allowing these valuable tools to run.

@gangelo gangelo self-assigned this Aug 10, 2022
module DocumentCaptureConcern
def override_document_capture_step_csp
return if params[:step] != 'document_capture'
return if IdentityConfig.store.suppress_content_security_policy
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From @zachmargolis: this methods opens up the policy, does it still cause problems for the accessibiltiy tool on this step if we leave it in?

Copy link
Contributor Author

@gangelo gangelo Aug 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zachmargolis

ANDI (anyhow) cannot be loaded/used without line 5, despite the open policy. The reason the opening up the policy here doesn't work, is because of lines 8-10 where policy.xxx_src == :self. policy.xxx_src would need to allow all domains (i.e. policy.xxx_src == '*'). ANDI (for example) executes the following:

javascript:void((function(){andiScript=document.createElement('script');
andiScript.setAttribute('src','https://www.ssa.gov/accessibility/andi/andi.js');
document.body.appendChild(andiScript)})());

Screen Shot 2022-08-10 at 3 28 11 PM

@gangelo gangelo marked this pull request as ready for review August 11, 2022 01:43
@aduth
Copy link
Contributor

aduth commented Aug 11, 2022

Copying from #6705 (comment)

Would the existing disable_csp_unsafe_inline configuration help with the need? Could we consider other tools? It's a bit concerning to me that a tool wouldn't be functional without the ability to download third-party scripts or call out to a third-party API. Historically I've been able to use the Deque Axe DevTools and Accessibility Insights for Web extensions without any issues, and those have been pretty popular in the program.

If we did add this, would[/could] the idea be to only allow for it in local development, and not in any deployed environments?

@gangelo
Copy link
Contributor Author

gangelo commented Aug 11, 2022

Copying from #6705 (comment)

Would the existing disable_csp_unsafe_inline configuration help with the need? Could we consider other tools? It's a bit concerning to me that a tool wouldn't be functional without the ability to download third-party scripts or call out to a third-party API. Historically I've been able to use the Deque Axe DevTools and Accessibility Insights for Web extensions without any issues, and those have been pretty popular in the program.

Apologies for not addressing this, but unfortunately, disable_csp_unsafe_inline didn't do the trick.

If we did add this, would[/could] the idea be to only allow for it in local development, and not in any deployed environments?

The idea is to use it with an AWS sandbox that everyone can use and share; although, we could also use it locally of course.

@aduth
Copy link
Contributor

aduth commented Aug 12, 2022

I put a lot of value in the CSP, so I'd rather see it enforced in more places than in fewer (e.g. local development). One reason is that local development can currently give a false sense of confidence that a feature relying on a CSP bypass will work, only for it to break once it reaches a deployed environment (previously #5852, #4813, #4429, and likely soon again in #6694). As proposed, a fear I'd have is that we'd not know of these issues 'til they start affecting real users in production.

Personally, I'd rather see one of:

  • Open the CSP only as much as necessary to support what we need (i.e. add the directives and hosts necessary to support this tool as configuration)
  • Or, limit its usage to personal sandbox environments, not common ones like dev or int
  • Or (my preference), switch to using an accessibility tool that's compatible with the current CSP, like the ones mentioned in my previous comment

@gangelo
Copy link
Contributor Author

gangelo commented Aug 12, 2022

put a lot of value in the CSP, so I'd rather see it enforced in more places than in fewer (e.g. local development). One reason is that local development can currently give a false sense of confidence that a feature relying on a CSP bypass will work, only for it to break once it reaches a deployed environment (previously #5852, #4813, #4429, and likely soon again in #6694). As proposed, a fear I'd have is that we'd not know of these issues 'til they start affecting real users in production.

Personally, I'd rather see one of:

  • Open the CSP only as much as necessary to support what we need (i.e. add the directives and hosts necessary to support this tool as configuration)
  • Or, limit its usage to personal sandbox environments, not common ones like dev or int
  • Or (my preference), switch to using an accessibility tool that's compatible with the current CSP, like the ones mentioned in my previous comment

The very idea behind this flipper is to explicitly turn CSP off to open up the possibility to use tools that are helpful, yet otherwise would be unavailable for use; tools that have nothing to do with CSP. 508 tools for example. No one should have any sense of security, false or otherwise, when utilizing this flipper. No one should be using a specific sandbox, unless they know what the sandbox is used for and anyone who would want to test anything to do with CSP on a box that has CSP turned off, probably shouldn't be there to begin with :).

The other tools you listed, are (from what I saw) mostly automated tools that require a lot of set up, and much more than a developer may need, as it is in our situation.

I'm okay with option 2 (Or, limit its usage to personal sandbox environments, not common ones like dev or int); however, it was Zach's suggestion to place this on a sandbox, and I think it's a good one. If we have to limit this to option 2, I'm okay with that.

@gangelo gangelo requested review from holytoastr, stevegsa and zachmargolis and removed request for zachmargolis August 12, 2022 13:49
@mitchellhenke
Copy link
Contributor

The very idea behind this flipper is to explicitly turn CSP off to open up the possibility to use tools that are helpful, yet otherwise would be unavailable for use; tools that have nothing to do with CSP. 508 tools for example. No one should have any sense of security, false or otherwise, when utilizing this flipper. No one should be using a specific sandbox, unless they know what the sandbox is used for and anyone who would want to test anything to do with CSP on a box that has CSP turned off, probably shouldn't be there to begin with :).

The other tools you listed, are (from what I saw) mostly automated tools that require a lot of set up, and much more than a developer may need, as it is in our situation.

I'm okay with option 2 (Or, limit its usage to personal sandbox environments, not common ones like dev or int); however, it was Zach's suggestion to place this on a sandbox, and I think it's a good one. If we have to limit this to option 2, I'm okay with that.

I dug in a bit, and I think the approach is a bit riskier than is necessary. The ANDI bookmarklet in question only tries to inject the https://www.ssa.gov/accessibility/andi/andi.js script into the page rather than as a browser extension, which requires the CSP hole-punching.

WAVE, Accessibility Insights, or Deque offer alternatives that should only take a few clicks to install and inspect elements or potential accessibility issues. With the complexity/risk present here, I think one of the aforementioned alternatives should be pursued.

If IdentityConfig.store.suppress_content_security_policy is true.

changelog: Accessibility, Internal, Add config to suppress content security policy in AWS sandbox (LG-7191)
Inside blocks/procs; use next instead.
@gangelo gangelo force-pushed the lg-7191-add-configuration-flipper-to-suppress-content-security-policy-csp branch from 8deccd4 to 7de9824 Compare August 15, 2022 11:43
@jmhooper
Copy link
Contributor

It looks like the ticket for this got cancelled? Is this PR still open for review?

@stevegsa
Copy link
Contributor

i agree there are times when i need it locally and do something similar in the code.

@zachmargolis
Copy link
Contributor

Looks like this PR hasn't been updated in a bit, I'm going to close it out to keep the list of open PRs small

@gangelo
Copy link
Contributor Author

gangelo commented Aug 24, 2022

It looks like the ticket for this got cancelled? Is this PR still open for review?

It was closed due to the aforementioned concerns. I personally didn't share the same concerns as this would be sandboxed, either locally or on a literal AWS sandbox. While this flipper was not meant to be tool-specific, other tools were offered in lieu of ANDI (which was what prompted this PR). A cursory of the tools offered weren't as intuitive or user-friendly IMHO; but, in all fairness, I didn't have time to dig too deeply into them, which may speak to the intuitiveness/user-friendliness of the tools offered, but like I mentioned, this may be an unfair assessment on my part.

@gangelo
Copy link
Contributor Author

gangelo commented Aug 24, 2022

i agree there are times when i need it locally and do something similar in the code.

I see no problem with having that flipper available, even if (albeit) simply for local dev use. We'll probably resort to passing a patch around since some members on our team are familiar with and like the ANDI 508 tool :(

@rnagilla-gsa rnagilla-gsa added the inherited proofing Pull Requests for the Inherited Proofing feature label Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

inherited proofing Pull Requests for the Inherited Proofing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants