From 68ca29fa8bc970ee516c27bff23591f10412edd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 21 Oct 2019 21:29:10 +0200 Subject: [PATCH] Convert markdown to HTML on demand We were converting markdown to HTML every time we saved a record, which has the same problems as sanitizing HTML before saving it to the database, particularly because the body of a legislation draft is stored in a translations table. Performance-wise this isn't a problem: converting a text with more than 200_000 characters takes about a milisecond on my machine. Note we need to modify a migration generated by globalize, since the method `create_translation_table!` would fail now that we don't define `translates :body_html` in the model. --- app/models/legislation/draft_version.rb | 23 ++++++------------- .../legislation/draft_versions/show.html.erb | 2 +- ..._collaborative_legislation_translations.rb | 8 ++++--- ...l_fields_form_legislation_draft_version.rb | 8 +++++++ db/schema.rb | 4 ---- spec/features/xss_spec.rb | 9 ++++++++ spec/models/legislation/draft_version_spec.rb | 12 +--------- 7 files changed, 31 insertions(+), 35 deletions(-) create mode 100644 db/migrate/20191006114603_remove_html_fields_form_legislation_draft_version.rb diff --git a/app/models/legislation/draft_version.rb b/app/models/legislation/draft_version.rb index 56a42fd07..e9da06546 100644 --- a/app/models/legislation/draft_version.rb +++ b/app/models/legislation/draft_version.rb @@ -7,8 +7,6 @@ class Legislation::DraftVersion < ApplicationRecord translates :title, touch: true translates :changelog, touch: true translates :body, touch: true - translates :body_html, touch: true - translates :toc_html, touch: true include Globalizable belongs_to :process, class_name: "Legislation::Process", foreign_key: "legislation_process_id" @@ -20,23 +18,16 @@ class Legislation::DraftVersion < ApplicationRecord scope :published, -> { where(status: "published").order("id DESC") } - before_save :render_html - - def render_html + def body_html renderer = Redcarpet::Render::HTML.new(with_toc_data: true) - toc_renderer = Redcarpet::Render::HTML_TOC.new(with_toc_data: true) - if body_changed? - self.body_html = Redcarpet::Markdown.new(renderer).render(body) - self.toc_html = Redcarpet::Markdown.new(toc_renderer).render(body) - end + Redcarpet::Markdown.new(renderer).render(body) + end - translations.each do |translation| - if translation.body_changed? - translation.body_html = Redcarpet::Markdown.new(renderer).render(translation.body) - translation.toc_html = Redcarpet::Markdown.new(toc_renderer).render(translation.body) - end - end + def toc_html + renderer = Redcarpet::Render::HTML_TOC.new(with_toc_data: true) + + Redcarpet::Markdown.new(renderer).render(body) end def display_title diff --git a/app/views/legislation/draft_versions/show.html.erb b/app/views/legislation/draft_versions/show.html.erb index 8259c74fa..3429c3e03 100644 --- a/app/views/legislation/draft_versions/show.html.erb +++ b/app/views/legislation/draft_versions/show.html.erb @@ -66,7 +66,7 @@ data-legislation-annotatable-base-url="<%= legislation_process_draft_version_path(@process, @draft_version) %>" data-legislation-open-phase="<%= @process.allegations_phase.open? %>"> <% end %> - <%= sanitize(@draft_version.body_html) %> + <%= sanitize(@draft_version.body_html, { attributes: ["id"] }) %> diff --git a/db/migrate/20180801140800_add_collaborative_legislation_translations.rb b/db/migrate/20180801140800_add_collaborative_legislation_translations.rb index 80d9d1f5b..607801492 100644 --- a/db/migrate/20180801140800_add_collaborative_legislation_translations.rb +++ b/db/migrate/20180801140800_add_collaborative_legislation_translations.rb @@ -20,12 +20,14 @@ class AddCollaborativeLegislationTranslations < ActiveRecord::Migration[4.2] { title: :string, changelog: :text, - body: :text, - body_html: :text, - toc_html: :text + body: :text }, { migrate_data: true } ) + + add_column :legislation_draft_version_translations, :body_html, :text + add_column :legislation_draft_version_translations, :toc_html, :text + Legislation::QuestionOption.create_translation_table!( { value: :string }, { migrate_data: true } diff --git a/db/migrate/20191006114603_remove_html_fields_form_legislation_draft_version.rb b/db/migrate/20191006114603_remove_html_fields_form_legislation_draft_version.rb new file mode 100644 index 000000000..4beaf22b9 --- /dev/null +++ b/db/migrate/20191006114603_remove_html_fields_form_legislation_draft_version.rb @@ -0,0 +1,8 @@ +class RemoveHtmlFieldsFormLegislationDraftVersion < ActiveRecord::Migration[5.0] + def change + remove_column :legislation_draft_versions, :body_html + remove_column :legislation_draft_version_translations, :body_html + remove_column :legislation_draft_versions, :toc_html + remove_column :legislation_draft_version_translations, :toc_html + end +end diff --git a/db/schema.rb b/db/schema.rb index 692ec3f82..7e762e9a8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -759,8 +759,6 @@ ActiveRecord::Schema.define(version: 20191020153455) do t.string "title" t.text "changelog" t.text "body" - t.text "body_html" - t.text "toc_html" t.datetime "hidden_at" t.index ["hidden_at"], name: "index_legislation_draft_version_translations_on_hidden_at", using: :btree t.index ["legislation_draft_version_id"], name: "index_900e5ba94457606e69e89193db426e8ddff809bc", using: :btree @@ -777,8 +775,6 @@ ActiveRecord::Schema.define(version: 20191020153455) do t.datetime "hidden_at" t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.text "body_html" - t.text "toc_html" t.index ["hidden_at"], name: "index_legislation_draft_versions_on_hidden_at", using: :btree t.index ["legislation_process_id"], name: "index_legislation_draft_versions_on_legislation_process_id", using: :btree t.index ["status"], name: "index_legislation_draft_versions_on_status", using: :btree diff --git a/spec/features/xss_spec.rb b/spec/features/xss_spec.rb index a545b7e78..8e9032fe5 100644 --- a/spec/features/xss_spec.rb +++ b/spec/features/xss_spec.rb @@ -153,4 +153,13 @@ describe "Cross-Site Scripting protection", :js do expect(page.text).not_to be_empty end + + scenario "legislation version body filters script tags but not header IDs" do + version = create(:legislation_draft_version, :published, body: "# Title 1\n#{attack_code}") + + visit legislation_process_draft_version_path(version.process, version) + + expect(page.text).not_to be_empty + expect(page).to have_css "h1#title-1", text: "Title 1" + end end diff --git a/spec/models/legislation/draft_version_spec.rb b/spec/models/legislation/draft_version_spec.rb index 4052b40b3..33eadb98d 100644 --- a/spec/models/legislation/draft_version_spec.rb +++ b/spec/models/legislation/draft_version_spec.rb @@ -10,7 +10,7 @@ describe Legislation::DraftVersion do expect(legislation_draft_version).to be_valid end - it "renders and saves the html from the markdown body field" do + it "renders the html from the markdown body field" do legislation_draft_version.body = body_markdown legislation_draft_version.save! @@ -19,16 +19,6 @@ describe Legislation::DraftVersion do expect(legislation_draft_version.toc_html).to eq(toc_html) end - it "renders and saves the html from the markdown body field with alternative translation" do - legislation_draft_version.title_fr = "Français" - legislation_draft_version.body_fr = body_markdown - - legislation_draft_version.save! - - expect(legislation_draft_version.body_html_fr).to eq(body_html) - expect(legislation_draft_version.toc_html_fr).to eq(toc_html) - end - def body_markdown <<-BODY_MARKDOWN # Title 1