Send JSON payload within a <script> tag

[flawedworld]

Changes proposed in this pull request:

Instead of dynamically changing this script every time with the JSON payload, send the JSON payload within a <script> tag. This will allow site admins to specify a hash for the 2 inline scripts in this file within their CSP, so they should be able to remove the need for unsafe-inline.

Reviewers should focus on:

I don't really have any concerns, at some point I would like to move the 2 inline scripts in framework/core/views/frontend/app.blade.php to their own dedicated files, how would the project prefer to deal with that?

Screenshot

N/A

Necessity

• Has the problem that is being solved here been clearly explained?
• If applicable, have various options for solving this problem been considered?
• For core PRs, does this need to be in core, or could it be in an extension?
• Are we willing to maintain this for years / potentially forever?

Confirmed

• Frontend changes: tested on a local Flarum installation.
• Backend changes: tests are green (run composer test).
• Core developer confirmed locally this works as intended.
• Tests have been added, or are not appropriate here.

[thestinger]

Here's where we're using this improvement:

GrapheneOS/discuss.grapheneos.org@0dfc779

'unsafe-inline' for script-src can be replaced with 'sha256-/EpI8KnybjL+Nr74PZCKr6RjFdz9ZTYI9k7FmBUHsL0=' 'sha256-LPH3sskAOz2Arvw8RMnxs/omWH+RFNl4CGea/pvCq4g='.

[thestinger]

This is a more readable way to view our Content-Security-Policy setup since GitHub doesn't wrap lines:

https://raw.githubusercontent.com/GrapheneOS/discuss.grapheneos.org/main/nginx/snippets/security-headers.conf

The remaining improvements would be replacing all the inline CSS and the more difficult step of integrating Trusted Types, ideally by avoiding APIs like innerHTML so that the Trusted Types policy can be enforced as none. Most issues are likely to occur on the server side rather than the client side but it would still be nice to enforce this on the client side since it's widely deployed due to being available in Chromium-based browsers. Enforcing something similar server side would be much harder, although I'm not familiar with what's available for making PHP safer in that regard.

[SychO9]

Thanks for the PR!

I would like to move the 2 inline scripts in framework/core/views/frontend/app.blade.php to their own dedicated files, how would the project prefer to deal with that?

That would be better in the long term, but with the way we handle assets with the SPA it won't be straightforward. We might need to add a new frontend definition on the backend to register non-SPA scripts.

'unsafe-inline' for script-src can be replaced with..

You might not be able to get rid of unsafe-inline just yet depending on the extensions you use and if you allow post formatting.

We have CSP on the roadmap to investigate and come up with a plan, the problem is supporting it with third-party extensions and making it possible with the textformatter library we use which adds inline scripts and styles depending on post content.

But this is a good step forward imo.

[thestinger]

BBCode extension tries to use inline / dynamically evaluated JavaScript for both BBCode and Markdown code blocks which will end up breaking. It appears that Markdown doesn't support syntax highlighting when the BBCode extension is disabled and we have it disabled.

It looks like this could be resolved by statically including the highlight.js scripts/styles with Flarum with global JavaScript handling applying it to the code blocks instead of code being included inline with each one. This would also avoid the need to depend on fetching highlight.js content from an external CDN.

It breaks a bit differently for viewing existing posts and the preview feature because it counts as dynamic code evaluation with how the preview works vs. unsafe inline styles/scripts for viewing existing content. It looks like it would be possible to whitelist it in CSP with how it works for existing content by adding the CDN URLs and the relevant hash for the inline script along with unsafe-hashes to make it allow it for an event handler. I don't see a way to add CSP to make the current preview approach work though.

For now, we're fine with how things work since we don't really want BBCode and that happens to disable the syntax highlighting support right now which would break with our Content-Security-Policy.

[JoshyPHP]

For future reference, the dynamically evaluated code is from the live preview events onrender/onupdate. They are declared in data-* attributes directly in the markup, similarly to an inline event. The attribute's content is used to create a Function object that evaluates it. The instance is cached for performance. Recently, I have added support for prefilling the function cache with actual function declarations. That means the functions are declared directly within the code, and it doesn't rely on dynamic evaluation for static code. By static, I mean anything that's not an XSLT attribute value template. As a result, the live preview can function without unsafe-eval.

The same strategy can be used to avoid unsafe-inline, converting inline events into onrender/onupdate live preview events. I don't have any immediate plans for it because CSP doesn't seem like a priority, but it can be done.

I have a live demo online that usually runs the latest version of the code. The current version has a minimal CSP directive that only allows the live preview code and the CDN. Do note that while the hljs-loader script is loaded with SRI, the JavaScript files from Highlight.js CDN are not.