Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

georgeboot
Copy link
Contributor

This PR does the following things:

  1. It adds space (back) in front of the nonce="" property.
  2. It adds the nonce to the <script src=""> tag as well (useful when using strict-dynamic
  3. It allows setting a nonce for the style tag

@georgeboot georgeboot marked this pull request as ready for review February 10, 2021 12:43
@joshhanley
Copy link
Member

@georgeboot not sure if you have seen, there was a closed PR #1364 that tried to add nonce to style tags.

Can you have a look at the questions raised there by Caleb and see if you can answer them here? That would be a great help for supporting why this PR is needed.

Hope this helps!

@georgeboot
Copy link
Contributor Author

georgeboot commented Feb 10, 2021

@joshhanley sure! If primarily useful when using strict-dynamic (available in CSP 3).

When you use that, all white listed urls etc get ignored, and you basically have to add nonce (or hash) to all (script) tags. Benefit of this approach is that this will automatically whitelist all scripts called by the script you added the nonce for.

See strict-dynamic for more info.

Regarding the style tag specific, it's less of a concern. CSP allows you to lock down style sources as well, and using a nonce is the easiest way for this.

Hope this helps!

@joshhanley
Copy link
Member

joshhanley commented Feb 10, 2021

@georgeboot thanks! 🙂 Sorry to keep pushing, but can you comment on these questions specifically: #1364 (comment)

Curious about his. I understand wanting this for your site's CSP, but is this a real issue you've faced? Or just a theoretical improvement? I believe Livewire's JS uses "eval" which would violate CSP anyways.

I'd love more info on your situation. I don't want to just pull this in because it's a good idea, you know?

@georgeboot
Copy link
Contributor Author

Sure, no problem!

This is a real issue I've faced. Livewire indeed uses eval, but you can allow livewire to use it using unsafe-eval.

The more specific you can do this, the better. This PR helps with that.

Agreed, still granting livewire permission to use eval is still a potential hole, so it's debatable how much more secure your site actually gets. However, I believe Livewire should offer a working integration, or none. "Half-baked" as is, doesn't rally help either cases.

@calebporzio
Copy link
Collaborator

Thanks @georgeboot! You're right, something is better than nothing here.

I think there is a bigger conversation to be had here about Livewire and CSP.

Here are the current violations in the codebase:
A) Using an inline script tag (from @livewireScripts) - which we're talking about here
B) Using "eval()" to evaluate inline scripts from the server on subsequent updates
C) Using "eval()" to get method name and parameters from something like wire:click="foo('bar' + 'baz')"

Here are some thoughts on the difficulty of overcoming each above point:
A) In a future version of Livewire, we could do away with the inline script entirely I think. The only real hurdle there is providing a way to automatically make the csrf token available to Livewire's JS. (we could just put it in a meta tag in the body or something)
B) This one would be tough to do unless we remove this functionality entirely. Which I suppose I would be open to in a future version or at least a "CSP-safe" version.
C) This is the toughest, we would be dropping down to a more primitive parser that doesn't support full JS expressions like are currently supported.

I think the path forward here is a "CSP-safe" toggle for the next major version of Livewire.

Going to close this one now as it's a partial solution.

Thank you!

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.

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.