Skip to content
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

WIP restore CSP #2651

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

WIP restore CSP #2651

wants to merge 1 commit into from

Conversation

colinxfleming
Copy link
Member

I rule and have completed some work on Case Manager that's ready for review!

(brief, plain english overview of your changes here)

This pull request makes the following changes:

  • (your change here)
  • (another change here)
  • (etc)

(If there are changes to the views, please include a screenshot so we know what to look for!)

It relates to the following issue #s:

  • Fixes #X
  • Bumps #Y

For reviewer:

  • Adjust the title to explain what it does for the notification email to the listserv.
  • Tag this PR:
    • feature if it contains a feature, fix, or similar. This is anything that contains a user-facing fix in some way, such as frontend changes, alterations to backend behavior, or bug fixes.
    • dependencies if it contains library upgrades or similar. This is anything that upgrades any dependency, such as a Gemfile update or npm package upgrade.
  • If it contains neither, no need to tag this PR.

@colinxfleming
Copy link
Member Author

Awful news for us: internet consensus seems to be 'js.erb is getting replaced by hotwire because it fundamentally doesn't play nice with CSPs'. bluh

@xmunoz
Copy link
Member

xmunoz commented Aug 8, 2022

Do you have a reference for that quote?

@xmunoz
Copy link
Member

xmunoz commented Sep 5, 2022

I'm going to pick this up

@xmunoz
Copy link
Member

xmunoz commented Sep 5, 2022

Huh, our current CSP is not great:

  policy.default_src :self
  policy.font_src    :self, :https, :data
  policy.img_src     :self, :https, :data
  policy.object_src  :none
  policy.font_src    :self, 'fonts.gstatic.com'
  policy.connect_src :self
  policy.script_src  :self, :unsafe_eval
  policy.style_src   :self, :unsafe_inline

We should not allow unsafe_eval in script_src or unsafe_inline in the style_src.

Ref: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/script-src#unsafe_inline_script

@colinxfleming
Copy link
Member Author

Do you have a reference for that quote?

read em and weep I think: https://twitter.com/dhh/status/1252751363219419136?lang=en

image

@colinxfleming
Copy link
Member Author

I'll make a larger issue for this since I think it's going to be more involved, but the state of play as I understand it is basically:

  • Best CSP practices (such as, for example, not doing unsafe_eval) have rendered js.erb (rails unobtrusive javascript, or UJS, I think is what it's called more general) obsolete; it's no longer the rails 7 happy path
  • There are a few alt approaches; one seems to be turbo stream, another swapping to hotwire (though these might really be the same thing?). Both are fairly recent developments in planet rails. I have no idea how involved they're going to be but I bet the answer is somewhat.
  • Fortunately we don't have that much js.erb (~20 files) and what we do have isn't that involved save one or two things like patient edit.

So I think the action item as it stands is to figure out what alternative to js.erb/rails UJS is least chaotic for us, and then implement that, and THEN we'll be able to CSP properly again. Though if there's a way to target the CSPs so that it hits everything EXCEPT js.erb payloads that would be sick.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants