DEV: extract inline js when baking theme fields (#6447)

* extract inline js when baking theme fields
* destroy javascript cache when destroying theme fields

This work is needed to support CSP work

app/controllers/javascripts_controller.rb

@@ -0,0 +1,65 @@
+# frozen_string_literal: true
+class JavascriptsController < ApplicationController
+  DISK_CACHE_PATH = "#{Rails.root}/tmp/javascript-cache"
+
+  skip_before_action(
+    :check_xhr,
+    :handle_theme,
+    :preload_json,
+    :redirect_to_login_if_required,
+    :verify_authenticity_token,
+    only: [:show]
+  )
+
+  before_action :is_asset_path, :no_cookies, only: [:show]
+
+  def show
+    raise Discourse::NotFound unless last_modified.present?
+    return render body: nil, status: 304 if not_modified?
+
+    # Security: safe due to route constraint
+    cache_file = "#{DISK_CACHE_PATH}/#{params[:digest]}.js"
+
+    unless File.exist?(cache_file)
+      content = query.pluck(:content).first
+      raise Discourse::NotFound if content.nil?
+
+      FileUtils.mkdir_p(DISK_CACHE_PATH)
+      File.write(cache_file, content)
+    end
+
+    set_cache_control_headers
+    send_file(cache_file, disposition: :inline)
+  end
+
+  private
+
+  def query
+    @query ||= JavascriptCache.where(digest: params[:digest]).limit(1)
+  end
+
+  def last_modified
+    @last_modified ||= query.pluck(:updated_at).first
+  end
+
+  def not_modified?
+    cache_time =
+      begin
+        Time.rfc2822(request.env["HTTP_IF_MODIFIED_SINCE"])
+      rescue ArgumentError
+        nil
+      end
+
+    cache_time && last_modified && last_modified <= cache_time
+  end
+
+  def set_cache_control_headers
+    if Rails.env.development?
+      response.headers['Last-Modified'] = Time.zone.now.httpdate
+      immutable_for(1.second)
+    else
+      response.headers['Last-Modified'] = last_modified.httpdate if last_modified
+      immutable_for(1.year)
+    end
+  end
+end

app/models/javascript_cache.rb

@@ -0,0 +1,39 @@
+# frozen_string_literal: true
+class JavascriptCache < ActiveRecord::Base
+  belongs_to :theme_field
+
+  validate :content_cannot_be_nil
+
+  before_save :update_digest
+
+  def url
+    "#{GlobalSetting.cdn_url}#{GlobalSetting.relative_url_root}/javascripts/#{digest}.js"
+  end
+
+  private
+
+  def update_digest
+    self.digest = Digest::SHA1.hexdigest(content) if content_changed?
+  end
+
+  def content_cannot_be_nil
+    errors.add(:content, :empty) if content.nil?
+  end
+end
+
+# == Schema Information
+#
+# Table name: javascript_caches
+#
+#  id             :bigint(8)        not null, primary key
+#  theme_field_id :bigint(8)        not null
+#  digest         :string
+#  content        :text             not null
+#  created_at     :datetime         not null
+#  updated_at     :datetime         not null
+#
+# Indexes
+#
+#  index_javascript_caches_on_digest          (digest)
+#  index_javascript_caches_on_theme_field_id  (theme_field_id)
+#

app/models/theme_field.rb

@@ -3,6 +3,7 @@ require_dependency 'theme_settings_parser'
 class ThemeField < ActiveRecord::Base
 
   belongs_to :upload
+  has_one :javascript_cache, dependent: :destroy
 
   scope :find_by_theme_ids, ->(theme_ids) {
     return none unless theme_ids.present?
@@ -68,6 +69,8 @@ PLUGIN_API_JS
 
   def process_html(html)
     errors = nil
+    javascript_cache || build_javascript_cache
+    javascript_cache.content = ''
 
     doc = Nokogiri::HTML.fragment(html)
     doc.css('script[type="text/x-handlebars"]').each do |node|
@@ -83,43 +86,52 @@ PLUGIN_API_JS
 
       if is_raw
         template = "requirejs('discourse-common/lib/raw-handlebars').template(#{Barber::Precompiler.compile(hbs_template)})"
-        node.replace <<COMPILED
+        javascript_cache.content << <<COMPILED
-          <script>
+          (function() {
-            (function() {
+            if ('Discourse' in window) {
-              if ('Discourse' in window) {
               Discourse.RAW_TEMPLATES[#{name.sub(/\.raw$/, '').inspect}] = #{template};
-              }
+            }
-            })();
+          })();
-          </script>
 COMPILED
       else
         template = "Ember.HTMLBars.template(#{Barber::Ember::Precompiler.compile(hbs_template)})"
-        node.replace <<COMPILED
+        javascript_cache.content << <<COMPILED
-          <script>
+          (function() {
-            (function() {
+            if ('Em' in window) {
-              if ('Em' in window) {
               Ember.TEMPLATES[#{name.inspect}] = #{template};
-              }
+            }
-            })();
+          })();
-          </script>
 COMPILED
       end
 
+      node.remove
     end
 
     doc.css('script[type="text/discourse-plugin"]').each do |node|
       if node['version'].present?
         begin
-          code = transpile(node.inner_html, node['version'])
+          javascript_cache.content << transpile(node.inner_html, node['version'])
-          node.replace("<script>#{code}</script>")
         rescue MiniRacer::RuntimeError => ex
-          node.replace("<script type='text/discourse-js-error'>#{ex.message}</script>")
+          javascript_cache.content << "console.error('Theme Transpilation Error:', #{ex.message.inspect});"
+
           errors ||= []
           errors << ex.message
         end
+
+        node.remove
       end
     end
 
+    doc.css('script').each do |node|
+      next if node['src'].present?
+
+      javascript_cache.content << "(function() { #{node.inner_html} })();"
+      node.remove
+    end
+
+    javascript_cache.save!
+
+    doc.add_child("<script src='#{javascript_cache.url}'></script>") if javascript_cache.content.present?
     [doc.to_s, errors&.join("\n")]
   end
 

config/routes.rb

@@ -450,6 +450,7 @@ Discourse::Application.routes.draw do
 
   get "stylesheets/:name.css.map" => "stylesheets#show_source_map", constraints: { name: /[-a-z0-9_]+/ }
   get "stylesheets/:name.css" => "stylesheets#show", constraints: { name: /[-a-z0-9_]+/ }
+  get "javascripts/:digest.js" => "javascripts#show", constraints: { digest: /\h{40}/ }
 
   post "uploads" => "uploads#create"
   post "uploads/lookup-urls" => "uploads#lookup_urls"

db/migrate/20180927135248_create_javascript_caches.rb

@@ -0,0 +1,10 @@
+class CreateJavascriptCaches < ActiveRecord::Migration[5.2]
+  def change
+    create_table :javascript_caches do |t|
+      t.references :theme_field, null: false
+      t.string :digest, null: true, index: true
+      t.text :content, null: false
+      t.timestamps
+    end
+  end
+end

spec/models/javascript_cache_spec.rb

@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+require 'rails_helper'
+
+RSpec.describe JavascriptCache, type: :model do
+  let(:theme) { Fabricate(:theme) }
+  let(:theme_field) { ThemeField.create!(theme: theme, target_id: 0, name: "header", value: "<a>html</a>") }
+
+  describe '#save' do
+    it 'updates the digest only if the content has changed' do
+      javascript_cache = JavascriptCache.create!(content: 'console.log("hello");', theme_field: theme_field)
+      expect(javascript_cache.digest).to_not be_empty
+
+      expect { javascript_cache.save! }.to_not change { javascript_cache.reload.digest }
+
+      expect do
+        javascript_cache.content = 'console.log("world");'
+        javascript_cache.save!
+      end.to change { javascript_cache.reload.digest }
+    end
+
+    it 'allows content to be empty, but not nil' do
+      javascript_cache = JavascriptCache.create!(content: 'console.log("hello");', theme_field: theme_field)
+
+      javascript_cache.content = ''
+      expect(javascript_cache.valid?).to eq(true)
+
+      javascript_cache.content = nil
+      expect(javascript_cache.valid?).to eq(false)
+      expect(javascript_cache.errors.details[:content]).to include(error: :empty)
+    end
+  end
+end

spec/models/theme_field_spec.rb

@@ -27,7 +27,31 @@ describe ThemeField do
     end
   end
 
-  it "correctly generates errors for transpiled js" do
+  it 'does not insert a script tag when there are no inline script' do
+    theme_field = ThemeField.create!(theme_id: 1, target_id: 0, name: "body_tag", value: '<div>new div</div>')
+    expect(theme_field.value_baked).to_not include('<script')
+  end
+
+  it 'only extracts inline javascript to an external file' do
+    html = <<HTML
+<script type="text/discourse-plugin" version="0.8">
+  var a = "inline discourse plugin";
+</script>
+<script>
+  var b = "inline raw script";
+</script>
+<script src="/external-script.js"></script>
+HTML
+
+    theme_field = ThemeField.create!(theme_id: 1, target_id: 0, name: "header", value: html)
+
+    expect(theme_field.value_baked).to include("<script src=\"#{theme_field.javascript_cache.url}\"></script>")
+    expect(theme_field.value_baked).to include("external-script.js")
+    expect(theme_field.javascript_cache.content).to include('inline discourse plugin')
+    expect(theme_field.javascript_cache.content).to include('inline raw script')
+  end
+
+  it "correctly extracts and generates errors for transpiled js" do
     html = <<HTML
 <script type="text/discourse-plugin" version="0.8">
    badJavaScript(;
@@ -36,6 +60,8 @@ HTML
 
     field = ThemeField.create!(theme_id: 1, target_id: 0, name: "header", value: html)
     expect(field.error).not_to eq(nil)
+    expect(field.value_baked).to include("<script src=\"#{field.javascript_cache.url}\"></script>")
+    expect(field.javascript_cache.content).to include("Theme Transpilation Error:")
 
     field.update!(value: '')
     expect(field.error).to eq(nil)
@@ -49,12 +75,14 @@ HTML
 HTML
 
     ThemeField.create!(theme_id: 1, target_id: 3, name: "yaml", value: "string_setting: \"test text \\\" 123!\"")
-    baked_value = ThemeField.create!(theme_id: 1, target_id: 0, name: "head_tag", value: html).value_baked
+    theme_field = ThemeField.create!(theme_id: 1, target_id: 0, name: "head_tag", value: html)
+    javascript_cache = theme_field.javascript_cache
 
-    expect(baked_value).to include("testing-div")
+    expect(theme_field.value_baked).to include("<script src=\"#{javascript_cache.url}\"></script>")
-    expect(baked_value).to include("theme-setting-injector")
+    expect(javascript_cache.content).to include("testing-div")
-    expect(baked_value).to include("string_setting")
+    expect(javascript_cache.content).to include("theme-setting-injector")
-    expect(baked_value).to include("test text \\\\\\\\u0022 123!")
+    expect(javascript_cache.content).to include("string_setting")
+    expect(javascript_cache.content).to include("test text \\\\\\\\u0022 123!")
   end
 
   it "correctly generates errors for transpiled css" do

spec/models/theme_spec.rb

@@ -120,10 +120,12 @@ HTML
     theme.set_field(target: :common, name: "header", value: with_template)
     theme.save!
 
+    field = theme.theme_fields.find_by(target_id: Theme.targets[:common], name: 'header')
     baked = Theme.lookup_field(theme.id, :mobile, "header")
 
-    expect(baked).to match(/HTMLBars/)
+    expect(baked).to include(field.javascript_cache.url)
-    expect(baked).to match(/raw-handlebars/)
+    expect(field.javascript_cache.content).to include('HTMLBars')
+    expect(field.javascript_cache.content).to include('raw-handlebars')
   end
 
   it 'should create body_tag_baked on demand if needed' do
@@ -214,7 +216,7 @@ HTML
   context "plugin api" do
     def transpile(html)
       f = ThemeField.create!(target_id: Theme.targets[:mobile], theme_id: 1, name: "after_header", value: html)
-      f.value_baked
+      return f.value_baked, f.javascript_cache
     end
 
     it "transpiles ES6 code" do
@@ -224,10 +226,10 @@ HTML
         </script>
 HTML
 
-      transpiled = transpile(html)
+      baked, javascript_cache = transpile(html)
-      expect(transpiled).to match(/\<script\>/)
+      expect(baked).to include(javascript_cache.url)
-      expect(transpiled).to match(/var x = 1;/)
+      expect(javascript_cache.content).to include('var x = 1;')
-      expect(transpiled).to match(/_registerPluginCode\('0.1'/)
+      expect(javascript_cache.content).to include("_registerPluginCode('0.1'")
     end
 
     it "converts errors to a script type that is not evaluated" do
@@ -238,9 +240,10 @@ HTML
         </script>
 HTML
 
-      transpiled = transpile(html)
+      baked, javascript_cache = transpile(html)
-      expect(transpiled).to match(/text\/discourse-js-error/)
+      expect(baked).to include(javascript_cache.url)
-      expect(transpiled).to match(/read-only/)
+      expect(javascript_cache.content).to include('Theme Transpilation Error')
+      expect(javascript_cache.content).to include('read-only')
     end
   end
 
@@ -319,33 +322,37 @@ HTML
 
     it "allows values to be used in JS" do
       theme.set_field(target: :settings, name: :yaml, value: "name: bob")
-      theme.set_field(target: :common, name: :after_header, value: '<script type="text/discourse-plugin" version="1.0">alert(settings.name); let a = ()=>{};</script>')
+      theme_field = theme.set_field(target: :common, name: :after_header, value: '<script type="text/discourse-plugin" version="1.0">alert(settings.name); let a = ()=>{};</script>')
       theme.save!
 
       transpiled = <<~HTML
-      <script>if ('Discourse' in window) {
+      if ('Discourse' in window) {
         Discourse._registerPluginCode('1.0', function (api) {
           var settings = { "name": "bob" };
           alert(settings.name);var a = function a() {};
         });
-      }</script>
+      }
       HTML
 
-      expect(Theme.lookup_field(theme.id, :desktop, :after_header)).to eq(transpiled.strip)
+      theme_field.reload
+      expect(Theme.lookup_field(theme.id, :desktop, :after_header)).to include(theme_field.javascript_cache.url)
+      expect(theme_field.javascript_cache.content).to eq(transpiled.strip)
 
       setting = theme.settings.find { |s| s.name == :name }
       setting.value = 'bill'
 
       transpiled = <<~HTML
-      <script>if ('Discourse' in window) {
+      if ('Discourse' in window) {
         Discourse._registerPluginCode('1.0', function (api) {
           var settings = { "name": "bill" };
           alert(settings.name);var a = function a() {};
         });
-      }</script>
+      }
       HTML
-      expect(Theme.lookup_field(theme.id, :desktop, :after_header)).to eq(transpiled.strip)
 
+      theme_field.reload
+      expect(Theme.lookup_field(theme.id, :desktop, :after_header)).to include(theme_field.javascript_cache.url)
+      expect(theme_field.javascript_cache.content).to eq(transpiled.strip)
     end
 
   end

spec/requests/admin/themes_controller_spec.rb

@@ -210,4 +210,22 @@ describe Admin::ThemesController do
       expect(JSON.parse(response.body)["errors"].first).to include(I18n.t("themes.errors.component_no_default"))
     end
   end
+
+  describe '#destroy' do
+    let(:theme) { Fabricate(:theme) }
+
+    it "deletes the field's javascript cache" do
+      theme.set_field(target: :common, name: :header, value: '<script>console.log("test")</script>')
+      theme.save!
+
+      javascript_cache = theme.theme_fields.find_by(target_id: Theme.targets[:common], name: :header).javascript_cache
+      expect(javascript_cache).to_not eq(nil)
+
+      delete "/admin/themes/#{theme.id}.json"
+
+      expect(response.status).to eq(204)
+      expect { theme.reload }.to raise_error(ActiveRecord::RecordNotFound)
+      expect { javascript_cache.reload }.to raise_error(ActiveRecord::RecordNotFound)
+    end
+  end
 end

spec/requests/javascripts_controller_spec.rb

@@ -0,0 +1,54 @@
+# frozen_string_literal: true
+require 'rails_helper'
+
+describe JavascriptsController do
+  let(:theme) { Fabricate(:theme) }
+  let(:theme_field) { ThemeField.create!(theme: theme, target_id: 0, name: "header", value: "<a>html</a>") }
+  let(:javascript_cache) { JavascriptCache.create!(content: 'console.log("hello");', theme_field: theme_field) }
+
+  describe '#show' do
+    def update_digest_and_get(digest)
+      # actually set digest to make sure 404 is raised by router
+      javascript_cache.update_attributes(digest: digest)
+
+      get "/javascripts/#{digest}.js"
+    end
+
+    it 'only accepts 40-char hexdecimal digest name' do
+      update_digest_and_get('0123456789abcdefabcd0123456789abcdefabcd')
+      expect(response.status).to eq(200)
+
+      update_digest_and_get('0123456789abcdefabcd0123456789abcdefabc')
+      expect(response.status).to eq(404)
+
+      update_digest_and_get('gggggggggggggggggggggggggggggggggggggggg')
+      expect(response.status).to eq(404)
+
+      update_digest_and_get('0123456789abcdefabc_0123456789abcdefabcd')
+      expect(response.status).to eq(404)
+
+      update_digest_and_get('0123456789abcdefabc-0123456789abcdefabcd')
+      expect(response.status).to eq(404)
+
+      update_digest_and_get('../../Gemfile')
+      expect(response.status).to eq(404)
+    end
+
+    it 'considers the database record as the source of truth' do
+      clear_disk_cache
+
+      get "/javascripts/#{javascript_cache.digest}.js"
+      expect(response.status).to eq(200)
+      expect(response.body).to eq(javascript_cache.content)
+
+      javascript_cache.destroy!
+
+      get "/javascripts/#{javascript_cache.digest}.js"
+      expect(response.status).to eq(404)
+    end
+
+    def clear_disk_cache
+      `rm #{JavascriptsController::DISK_CACHE_PATH}/*`
+    end
+  end
+end