extract inline js when baking theme fields (PR #6447)

What are extracted?

  • Handlebar template overrides (type="text/x-handlebars")
  • scripts with type="text/discourse-plugin", and transpilation errors are also extracted as console.errors
  • other inline scripts are extracted as-is

I will ship a background job to trigger a rebake of all HTML theme fields in a future PR, so it gives us a bit of time to play with the feature.

GitHub

You’ve signed the CLA, xrav3nz. Thank you! This pull request is ready for review.

1 Warning
:warning: This pull request is big! We prefer smaller PRs whenever possible, as they are easier to review. Can this be split into a few smaller PRs?

Generated by :no_entry_sign: Danger

Since this is a potential security vector we should have a test. Otherwise someone could remove it from routes.rb accidentally and any file on the server could be served which is nasty.

I’m surprised you don’t have to escape values in the bracketed area. Usually I would do \- and \_ but this seems to work?

There are no indexes on this table. It looks like it’s mainly queried by digest so it should have an index on that, probably a unique index?

Ah thanks, will double check by adding a test, and also test for the security aspect.

There is one index on theme_field_id (added by t.references helper), and another on digest (added by index: true).

digest is calculated entirely based on content, so it may not be unique (one common case: snippets copied from a Meta post). We could calculate it with "#{id}_#{content}" to make it “unique”, do you think it’s necessary?

tightened this significantly by only allowing 40 hexdigit chars, and added tests.

Excellent thanks!

Oh my bad, I did not see the index: true - In fact I didn’t know you could add one that way. It’s fine if it’s not unique, I was wrong about that.

can’t content be nil here, then you will try to write nil to a file which in theory will explode? it should 404 in this case

How big is the cache? how do we guarantee this table does not keep growing and growing?

Good point. In theory content at this point cannot be nil because:

  1. we check to make sure the database entry exist at the start of the request
  2. DB constraint to make sure content cannot be nil (it is at least an empty string)

But I agree to be safe I will add in another 404 check here.

EDIT: added the check here.

Each theme field only has one javascript cache entry, and the entry should (will add a test to verify) be deleted when the corresponding theme field is deleted.

The size of the extracted content is not bounded by anything though, it could be as large as the theme field permits.

Thanks @xrav3nz !!! Merging