From 6acdea37c4e77723c774152e9076dfedda99493a Mon Sep 17 00:00:00 2001 From: Kyle Zhao Date: Mon, 15 Oct 2018 12:55:23 +0800 Subject: [PATCH] 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 | 65 +++++++++++++++++++ app/models/javascript_cache.rb | 39 +++++++++++ app/models/theme_field.rb | 46 ++++++++----- config/routes.rb | 1 + ...20180927135248_create_javascript_caches.rb | 10 +++ spec/models/javascript_cache_spec.rb | 32 +++++++++ spec/models/theme_field_spec.rb | 40 ++++++++++-- spec/models/theme_spec.rb | 41 +++++++----- spec/requests/admin/themes_controller_spec.rb | 18 +++++ spec/requests/javascripts_controller_spec.rb | 54 +++++++++++++++ 10 files changed, 306 insertions(+), 40 deletions(-) create mode 100644 app/controllers/javascripts_controller.rb create mode 100644 app/models/javascript_cache.rb create mode 100644 db/migrate/20180927135248_create_javascript_caches.rb create mode 100644 spec/models/javascript_cache_spec.rb create mode 100644 spec/requests/javascripts_controller_spec.rb diff --git a/app/controllers/javascripts_controller.rb b/app/controllers/javascripts_controller.rb new file mode 100644 index 00000000000..5e861a3e651 --- /dev/null +++ b/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 diff --git a/app/models/javascript_cache.rb b/app/models/javascript_cache.rb new file mode 100644 index 00000000000..fd0bbfa735c --- /dev/null +++ b/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) +# diff --git a/app/models/theme_field.rb b/app/models/theme_field.rb index c4d6ab4e942..84a6b9b561e 100644 --- a/app/models/theme_field.rb +++ b/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 < - (function() { - if ('Discourse' in window) { + javascript_cache.content << < + } + })(); COMPILED else template = "Ember.HTMLBars.template(#{Barber::Ember::Precompiler.compile(hbs_template)})" - node.replace < - (function() { - if ('Em' in window) { + javascript_cache.content << < + } + })(); 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']) - node.replace("") + javascript_cache.content << transpile(node.inner_html, node['version']) rescue MiniRacer::RuntimeError => ex - node.replace("") + 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("") if javascript_cache.content.present? [doc.to_s, errors&.join("\n")] end diff --git a/config/routes.rb b/config/routes.rb index 1d6caab2b07..f4769f67a20 100644 --- a/config/routes.rb +++ b/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" diff --git a/db/migrate/20180927135248_create_javascript_caches.rb b/db/migrate/20180927135248_create_javascript_caches.rb new file mode 100644 index 00000000000..c2b3e83d12b --- /dev/null +++ b/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 diff --git a/spec/models/javascript_cache_spec.rb b/spec/models/javascript_cache_spec.rb new file mode 100644 index 00000000000..1599fbe92e6 --- /dev/null +++ b/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: "html") } + + 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 diff --git a/spec/models/theme_field_spec.rb b/spec/models/theme_field_spec.rb index 152531d8d10..89aa19ebfb8 100644 --- a/spec/models/theme_field_spec.rb +++ b/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: '
new div
') + expect(theme_field.value_baked).to_not include(' + var a = "inline discourse plugin"; + + + +HTML + + theme_field = ThemeField.create!(theme_id: 1, target_id: 0, name: "header", value: html) + + expect(theme_field.value_baked).to include("") + 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 = < 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("") + 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(baked_value).to include("theme-setting-injector") - expect(baked_value).to include("string_setting") - expect(baked_value).to include("test text \\\\\\\\u0022 123!") + expect(theme_field.value_baked).to include("") + expect(javascript_cache.content).to include("testing-div") + expect(javascript_cache.content).to include("theme-setting-injector") + 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 diff --git a/spec/models/theme_spec.rb b/spec/models/theme_spec.rb index 15ad9318ca6..9b7211de7ad 100644 --- a/spec/models/theme_spec.rb +++ b/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 match(/raw-handlebars/) + expect(baked).to include(field.javascript_cache.url) + 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 HTML - transpiled = transpile(html) - expect(transpiled).to match(/\/) - expect(transpiled).to match(/var x = 1;/) - expect(transpiled).to match(/_registerPluginCode\('0.1'/) + baked, javascript_cache = transpile(html) + expect(baked).to include(javascript_cache.url) + expect(javascript_cache.content).to include('var x = 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 HTML - transpiled = transpile(html) - expect(transpiled).to match(/text\/discourse-js-error/) - expect(transpiled).to match(/read-only/) + baked, javascript_cache = transpile(html) + expect(baked).to include(javascript_cache.url) + 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: '') + theme_field = theme.set_field(target: :common, name: :after_header, value: '') theme.save! transpiled = <<~HTML - + } 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 - + } 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 diff --git a/spec/requests/admin/themes_controller_spec.rb b/spec/requests/admin/themes_controller_spec.rb index cff93367358..2d9d34d54d8 100644 --- a/spec/requests/admin/themes_controller_spec.rb +++ b/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: '') + 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 diff --git a/spec/requests/javascripts_controller_spec.rb b/spec/requests/javascripts_controller_spec.rb new file mode 100644 index 00000000000..a0a985a63be --- /dev/null +++ b/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: "html") } + 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