Allow passing csp_nonce_assign_key into router #40

hoyon · 2024-04-04T16:04:37Z

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 oban-bg/oban#447

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

Thanks!

tompave · 2024-04-05T09:57:30Z

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.

hoyon · 2024-04-05T12:02:11Z

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!

tompave · 2024-04-05T12:47:18Z

test/fun_with_flags/ui/templates_test.exs

+      |> Plug.Conn.assign(:namespace, "/pear")
+      |> Plug.Conn.put_private(:csp_nonce_assign_key, %{style: :style_key, script: :script_key})
+      |> Plug.Conn.assign(:csrf_token, Plug.CSRFProtection.get_csrf_token())
+


This change has to be reverted (separate commit please).
The current tests need to pass even when no nonce is configured.
Then you can add extra template tests to assert that a nonce rendered into the HTML tags when configured.

EDIT: I see what you mean now.

I still suggest to make this default to nil, and assert that the rendered HTML makes sense.
Then you can add a different test group to see what happens when a nonce is present.

Updated the test cases and code to support the attribute not being private and added a test case to assert the csp nonce is rendered if provided.

tompave · 2024-04-05T12:49:13Z

lib/fun_with_flags/ui/templates/_head.html.eex

-  <link rel="stylesheet" href="<%= path(@conn, "/assets/bootstrap.min.css") %>">
-  <link rel="stylesheet" href="<%= path(@conn, "/assets/style.css") %>">
+  <link nonce="<%= csp_nonce(@conn, :style) %>" rel="stylesheet" href="<%= path(@conn, "/assets/bootstrap.min.css") %>">
+  <link nonce="<%= csp_nonce(@conn, :style) %>" rel="stylesheet" href="<%= path(@conn, "/assets/style.css") %>">


If no nonce is configured, this would result in:

<link nonce="" rel="stylesheet" href=".../assets/style.css">

Should the attribute be omitted instead?

tompave · 2024-04-12T07:57:25Z

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 · 2024-08-18T12:11:14Z

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

Ok, I've had time to do a deeper review. I've also had to brush up on some details of how CSP works.
Thank you for the PR. I've got a couple of minor comments about documentation, but the code looks good to me.

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.

It would help me understand which scenarios require this change.

[1]: is that what Oban used to do in 2021? It looks like today the web dashboard is extracted to a separate paid package, and that issue you linked doesn't link to the PR. (was it closed source back then already?)

tompave · 2024-08-18T12:40:13Z

README.md

@@ -97,6 +97,26 @@ defmodule MyPhoenixAppWeb.Router do
 end
 ```

+### Content Security Policy
+
+Content security policy nonces can be passed into the router to allow usage of strict content security policies throughout an application.


It would be good to link to some CSP docs here, for reference. For example:

https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP

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

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/style-src

https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/nonce

https://content-security-policy.com/nonce/

tompave · 2024-08-18T12:40:43Z

README.md

+
+``` elixir
+forward "/", FunWithFlags.UI.Router, namespace: "feature-flags", csp_nonce_assign_key: %{style: :my_style_nonce, script: :my_script_nonce}
+```


These two examples are for Phoenix. It would be good here to provide examples for Plug.

Also, I suggest to provide full examples, that also show how the nonce values can be generated and set in the assigns and response headers. In other words, it's important to show how to use this end-to-end, rather than just mention it without clear instructions.

I think the instructions from Oban are pretty clear, would you be open to basically lifting this into the docs?

The way I understand them, the instructions from Oban show how to configure the names of the assign keys for the nonces. They don't show where the nonces are supposed to come from. I suppose they would come from a different plug, but a full example would be helpful.

Good point! I already had a nonce stored in the conn (originally with put_private/3 and then I added it to the assigns as well for Oban). I can work on a full example.

Funny enough, this just popped up in my feed reader. Our solution is different than that.

Sorry for the delay, but I just wrote 2 examples, 1 simple with a single nonce and another more advanced with nonces specific to style and script tags.

tompave · 2024-08-18T13:00:59Z

test/fun_with_flags/ui/router_test.exs

+
+      conn =
+        conn(:get, "/flags/coconut")
+        |> Plug.Conn.assign(:pineapple, "mango")


Let's add a comment to explain what's happening here.

Why this can be confusing: manipulating conn here is basically preparing the request data, not the response, because this happens before calling Router.call(...).

Since this library's "server backend" is self-contained in its Router, I guess that here you expect the nonce "mango" to be generated and set in the assigns by a different plug, executed before this FWF router, in the host-application's router.

That's ok, but let's make it clear with a comment.

jc00ke · 2024-09-19T00:15:10Z

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

tompave · 2024-09-19T00:58:26Z

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.

jc00ke · 2024-09-20T20:14:17Z

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}';"

Allow passing csp_nonce_assign_key into router

6b9e4fb

hoyon force-pushed the csp-nonce branch from 8f20daa to 6b9e4fb Compare April 5, 2024 09:06

Add router tests for csp_nonce_assign_key option

5dabf27

Update README wording

00d1ea5

tompave reviewed Apr 5, 2024

View reviewed changes

hoyon added 2 commits April 5, 2024 14:38

only render nonce attribute if nonce provided and fix tests

b05dc84

make tests consistent with existing tests

ec06fae

tompave reviewed Aug 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow passing csp_nonce_assign_key into router #40

Allow passing csp_nonce_assign_key into router #40

hoyon commented Apr 4, 2024

tompave commented Apr 5, 2024 •

edited

Loading

hoyon commented Apr 5, 2024

tompave Apr 5, 2024 •

edited

Loading

hoyon Apr 5, 2024

tompave Apr 5, 2024 •

edited

Loading

hoyon Apr 5, 2024

tompave commented Apr 12, 2024

tompave commented Aug 18, 2024

tompave left a comment •

edited

Loading

tompave Aug 18, 2024

tompave Aug 18, 2024

tompave Aug 18, 2024

jc00ke Sep 19, 2024

tompave Sep 19, 2024

jc00ke Sep 20, 2024

jc00ke Oct 23, 2024

jc00ke Nov 12, 2024

tompave Aug 18, 2024 •

edited

Loading

jc00ke commented Sep 19, 2024

tompave commented Sep 19, 2024

jc00ke commented Sep 20, 2024

Allow passing csp_nonce_assign_key into router #40

Are you sure you want to change the base?

Allow passing csp_nonce_assign_key into router #40

Conversation

hoyon commented Apr 4, 2024

tompave commented Apr 5, 2024 • edited Loading

hoyon commented Apr 5, 2024

tompave Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tompave Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tompave commented Apr 12, 2024

tompave commented Aug 18, 2024

tompave left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tompave Aug 18, 2024 • edited Loading

Choose a reason for hiding this comment

jc00ke commented Sep 19, 2024

tompave commented Sep 19, 2024

jc00ke commented Sep 20, 2024

tompave commented Apr 5, 2024 •

edited

Loading

tompave Apr 5, 2024 •

edited

Loading

tompave Apr 5, 2024 •

edited

Loading

tompave left a comment •

edited

Loading

tompave Aug 18, 2024 •

edited

Loading