fun_with_flags_ui icon indicating copy to clipboard operation
fun_with_flags_ui copied to clipboard

Allow passing csp_nonce_assign_key into router

Open hoyon opened this issue 1 year ago • 8 comments

Adds a new parameter to the Router module which allows Content Security Policy nonces to be used within the fun with flags admin pages.

This implementation is inspired by the implementation in live dashboard https://hexdocs.pm/phoenix_live_dashboard/Phoenix.LiveDashboard.Router.html#live_dashboard/2 and oban https://github.com/sorentwo/oban/issues/447

We have tested this in our own app with a strict CSP and it works well.

Thanks!

hoyon avatar Apr 04 '24 16:04 hoyon

Hey there, thank you for using the library and the PR.

Can you please not force-push? It makes it difficult to review and discuss the change.

With the initial diff, before the force-push, all tests were failing because the changes in the PR now expect a :csp_nonce_assign_key option to always be provided. After the force-push you've fixed it by setting that key for all tests. GitHub is also getting confused with the force-push and I don't seem able to retrieve the original commit that caused the original test failures (did you do several force-pushes perhaps? First to change the test, and then to clean up the formatting?).

Anyway, I think that this new option should be optional. The tests were failing because at the moment this is a breaking change in the library's configuration API.

You should revert the test changes (just recover the original commit if you can, and force-push one last time). Then update the implementation to make the new option optional. And then add some extra tests to verify that the option works, when provided.

tompave avatar Apr 05 '24 09:04 tompave

Thanks for the review and apologies for making a bit of a mess with the commits and opening the PR before the tests were passing.

I've added a new test at the router module level which asserts that the nonce is correctly rendered into the script and link tags.

The new option is indeed optional as you can see by the existing tests in the router module still passing even when the new option isn't passed in. At the template level the code is similar to the existing namespace option in that it is expected to be present with a valid value at that point as the router module will always insert a value, albeit it will be nil if none is specified by the user. I can add a test for this case if you would like.

Thanks again for the review!

hoyon avatar Apr 05 '24 12:04 hoyon

Hello, thank you for iterating on this.

I just wanted to align expectations on the timeline. This week I've been too busy to look at this, and I'll be travelling for the next few weeks. I'll look at this when I'm back, in May.

tompave avatar Apr 12 '24 07:04 tompave

Hi, apologies for the wait. I'm now reviewing this.

Can you please give me permission to edit the fork? I need to it bring the latest changes on master (CI setup, dependencies) into this PR's branch. Or you can do it yourself by merging master in.

tompave avatar Aug 18 '24 12:08 tompave

Hi, I'm running into this as well, would love to see this addressed. Happy to help in whatever way I can!

jc00ke avatar Sep 19 '24 00:09 jc00ke

Hi @jc00ke 👋

Hi, I'm running into this as well

Can you please describe what you're running into, exactly? See also my review message where I ask for specifics.

tompave avatar Sep 19 '24 00:09 tompave

Hi @tompave!

Can you please describe what you're running into, exactly?

I was noticing 403s on the UI js files and it led me here, and while I'm not experiencing a bug, I have recently gone through a security audit where adding CSP was required. I chose to do that with a nonce and appreciated how easy it was to add to Oban's Web.

I'm a bit unsure on whether this is strictly needed. I believe that CSP nonces are usually used for inline <script> and <style> [1], which this library doesn't use. In fact, the scripts and styles provided by this package should come from the same domain, and a simpler CSP header without nonces would also work well.

While they may usually be used for inline scripts, they can be used for <script src="https://example.com/foo.js" nonce="some-nonce"> and <link href="https://example.com/foo.css" nonce="some-nonce"> tags, which is how we chose to implement them in our application.

To appease scanners like ZAP, this was the simplest to implement (trimmed down):

nonce = conn.private[:my_app_nonce]

"default-src 'self'; \
base-uri 'none'; \
img-src 'self' 'nonce-#{nonce}' data:; \
script-src 'self' 'nonce-#{nonce}'; \
script-src-elem 'self' 'nonce-#{nonce}'; \
style-src 'self' 'nonce-#{nonce}';"

jc00ke avatar Sep 20 '24 20:09 jc00ke