Preloaded Data Changes

[xfalcox]

!!! This is just a draft for discussion !!!

This draft moves the big preloaded data payload we send on the HTML from a data-attribute to a Javascript variable.

In doing this, it greatly reduces the size of the HTML (around ~50KB depending on site, before gzip) and reduces the number of necessary JSON.parse on the browser boot. To work with CSP it adds a nonce to secure the inline script, keeping this payload inline with the main document.

On the Ruby side, we are still generating/storing JSONs everywhere, so this PR hacks around by doing parsing each part back into a Hash so it can create a proper JSON only in the last step. If this approach is deemed worth to follow up we will need to fix this by adding Hash versions of each method so we don't do double the work on the backend.

I would like the review on this refactor to see if it is something worth pursuing.

One unexpected change from this PR is that it fix the "View Source" breaking on Firefox :D . I believe the 350KB single HTML tag highlight was the culprit.

[riking]

What happens if you use script type="text/discourse-preload" and JSON.parse the body instead? This avoids the HTML attribute escaping

[xfalcox]

That even bypasses the need for a nonce! Which also fixed the perf concern from @eviltrout and the nonce cache from @davidtaylorhq

[xfalcox]

On the other hand means I need to escape script close tags 🤔

[eviltrout]

In theory, I like it. It is much simpler. However, does it make us potential victims of this slow path?

https://v8.dev/blog/cost-of-javascript-2019#json

It might be faster to emit a variable that is a JSON.parse even though that seems counter intuitive.

[eviltrout]

Premature feedback, perhaps, but we should use a much more complex variable name here to avoid conflicts since it's a global namespace.

[davidtaylorhq]

The nonce-based CSP option is only secure if the nonce is unique on every request. I think that's mostly true with this implementation, except when the anon cache comes into play. When that happens, the same nonce will get sent to multiple clients. An attacker could then take that nonce and potentially use it to perform an XSS attack.

Maybe we could use the Hash method instead?

[davidtaylorhq]

I am curious why we need the sub thing... I think having < or > will never be included in json. For example:

pry(main)> puts ({test: "</script>h4cked"}.to_json)
{"test":"\u003c/script\u003eh4cked"}

But there must be a reason it was there before 🤔

[riking]

Check if the existing Rails html escaper takes care of the angle brackets, yeah.

[trs80]

There's also been some sort of performance regression since Firefox 77 relating to this data attribute https://bugzilla.mozilla.org/show_bug.cgi?id=1647942

[davidtaylorhq]

@xfalcox do you think we should explore this further? Or should we close this out?

[xfalcox]

I'm kinda with a full plate at the moment, but we should definitely follow this up when possible. Makes our HTML payload a lot smaller, which will use less cache, not break Firefox "View Source", etc.

…

On Fri, Feb 19, 2021 at 2:53 PM David Taylor ***@***.***> wrote: @xfalcox <https://github.com/xfalcox> do you think we should explore this further? Or should we close this out? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10129 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKSH7VWASUMKEYWKKHGDH3S72QRHANCNFSM4OIXLZDA> .

-- Rafael 'Falco' Dos Santos Silva

[SamSaffron]

overall I support a change here if we can get it past the line, I think it probably makes sense to have a dev todo for this.

will take lots of testing, maybe even we ship it behind an experimental setting for a while.