From 424535c1aecc9ced51b21ca5186f0a797d883cea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Fuentes?= Date: Tue, 14 Aug 2018 14:20:09 +0200 Subject: [PATCH 01/18] Add models needed to include images on ckeditor4 --- app/models/ckeditor/asset.rb | 4 +++ app/models/ckeditor/attachment_file.rb | 13 +++++++++ app/models/ckeditor/picture.rb | 14 ++++++++++ .../20180813141443_create_ckeditor_assets.rb | 23 +++++++++++++++ db/schema.rb | 28 ++++++++++++++----- 5 files changed, 75 insertions(+), 7 deletions(-) create mode 100644 app/models/ckeditor/asset.rb create mode 100644 app/models/ckeditor/attachment_file.rb create mode 100644 app/models/ckeditor/picture.rb create mode 100644 db/migrate/20180813141443_create_ckeditor_assets.rb diff --git a/app/models/ckeditor/asset.rb b/app/models/ckeditor/asset.rb new file mode 100644 index 000000000..cf636ed19 --- /dev/null +++ b/app/models/ckeditor/asset.rb @@ -0,0 +1,4 @@ +class Ckeditor::Asset < ActiveRecord::Base + include Ckeditor::Orm::ActiveRecord::AssetBase + include Ckeditor::Backend::Paperclip +end diff --git a/app/models/ckeditor/attachment_file.rb b/app/models/ckeditor/attachment_file.rb new file mode 100644 index 000000000..8d0c2eec7 --- /dev/null +++ b/app/models/ckeditor/attachment_file.rb @@ -0,0 +1,13 @@ +class Ckeditor::AttachmentFile < Ckeditor::Asset + has_attached_file :data, + url: '/ckeditor_assets/attachments/:id/:filename', + path: ':rails_root/public/ckeditor_assets/attachments/:id/:filename' + + validates_attachment_presence :data + validates_attachment_size :data, less_than: 100.megabytes + do_not_validate_attachment_file_type :data + + def url_thumb + @url_thumb ||= Ckeditor::Utils.filethumb(filename) + end +end diff --git a/app/models/ckeditor/picture.rb b/app/models/ckeditor/picture.rb new file mode 100644 index 000000000..445c2bbd9 --- /dev/null +++ b/app/models/ckeditor/picture.rb @@ -0,0 +1,14 @@ +class Ckeditor::Picture < Ckeditor::Asset + has_attached_file :data, + url: '/ckeditor_assets/pictures/:id/:style_:basename.:extension', + path: ':rails_root/public/ckeditor_assets/pictures/:id/:style_:basename.:extension', + styles: { content: '800>', thumb: '118x100#' } + + validates_attachment_presence :data + validates_attachment_size :data, less_than: 2.megabytes + validates_attachment_content_type :data, content_type: /\Aimage/ + + def url_content + url(:content) + end +end diff --git a/db/migrate/20180813141443_create_ckeditor_assets.rb b/db/migrate/20180813141443_create_ckeditor_assets.rb new file mode 100644 index 000000000..df3df2862 --- /dev/null +++ b/db/migrate/20180813141443_create_ckeditor_assets.rb @@ -0,0 +1,23 @@ +class CreateCkeditorAssets < ActiveRecord::Migration + def self.up + create_table :ckeditor_assets do |t| + t.string :data_file_name, null: false + t.string :data_content_type + t.integer :data_file_size + t.string :data_fingerprint + t.string :type, limit: 30 + + # Uncomment it to save images dimensions, if your need it + t.integer :width + t.integer :height + + t.timestamps null: false + end + + add_index :ckeditor_assets, :type + end + + def self.down + drop_table :ckeditor_assets + end +end diff --git a/db/schema.rb b/db/schema.rb index 9c568058e..e03b22ac7 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180727140800) do +ActiveRecord::Schema.define(version: 20180813141443) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -294,6 +294,20 @@ ActiveRecord::Schema.define(version: 20180727140800) do t.datetime "updated_at", null: false end + create_table "ckeditor_assets", force: :cascade do |t| + t.string "data_file_name", null: false + t.string "data_content_type" + t.integer "data_file_size" + t.string "data_fingerprint" + t.string "type", limit: 30 + t.integer "width" + t.integer "height" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + + add_index "ckeditor_assets", ["type"], name: "index_ckeditor_assets_on_type", using: :btree + create_table "comments", force: :cascade do |t| t.integer "commentable_id" t.string "commentable_type" @@ -1291,6 +1305,12 @@ ActiveRecord::Schema.define(version: 20180727140800) do add_index "votes", ["votable_id", "votable_type", "vote_scope"], name: "index_votes_on_votable_id_and_votable_type_and_vote_scope", using: :btree add_index "votes", ["voter_id", "voter_type", "vote_scope"], name: "index_votes_on_voter_id_and_voter_type_and_vote_scope", using: :btree + create_table "web_sections", force: :cascade do |t| + t.text "name" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + create_table "widget_cards", force: :cascade do |t| t.string "title" t.text "description" @@ -1309,12 +1329,6 @@ ActiveRecord::Schema.define(version: 20180727140800) do t.datetime "updated_at", null: false end - create_table "web_sections", force: :cascade do |t| - t.text "name" - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - end - add_foreign_key "administrators", "users" add_foreign_key "annotations", "legacy_legislations" add_foreign_key "annotations", "users" From 8e68f4bbd062476f095534e37d850407e70ebd9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Fuentes?= Date: Tue, 14 Aug 2018 14:21:27 +0200 Subject: [PATCH 02/18] Update abilities for ckeditor4 images Add the image controller to use the obsolete load_and_authorize_resource and authorize_resource methods in the gem --- .../ckeditor/pictures_controller.rb | 36 +++++++++++++++++++ app/models/abilities/everyone.rb | 4 +++ config/routes.rb | 1 + 3 files changed, 41 insertions(+) create mode 100644 app/controllers/ckeditor/pictures_controller.rb diff --git a/app/controllers/ckeditor/pictures_controller.rb b/app/controllers/ckeditor/pictures_controller.rb new file mode 100644 index 000000000..1dded38f3 --- /dev/null +++ b/app/controllers/ckeditor/pictures_controller.rb @@ -0,0 +1,36 @@ +class Ckeditor::PicturesController < Ckeditor::ApplicationController + load_and_authorize_resource + def index + @pictures = Ckeditor.picture_adapter.find_all(ckeditor_pictures_scope) + @pictures = Ckeditor::Paginatable.new(@pictures).page(params[:page]) + + respond_to do |format| + format.html { render :layout => @pictures.first_page? } + end + end + + def create + @picture = Ckeditor.picture_model.new + respond_with_asset(@picture) + end + + def destroy + @picture.destroy + + respond_to do |format| + format.html { redirect_to pictures_path } + format.json { render :nothing => true, :status => 204 } + end + end + + protected + + def find_asset + @picture = Ckeditor.picture_adapter.get!(params[:id]) + end + + def authorize_resource + model = (@picture || Ckeditor.picture_model) + @authorization_adapter.try(:authorize, params[:action], model) + end +end \ No newline at end of file diff --git a/app/models/abilities/everyone.rb b/app/models/abilities/everyone.rb index 23cfdc971..dcc430cdd 100644 --- a/app/models/abilities/everyone.rb +++ b/app/models/abilities/everyone.rb @@ -3,6 +3,10 @@ module Abilities include CanCan::Ability def initialize(user) + can :access, :ckeditor # needed to access Ckeditor filebrowser + can [:access, :read, :create, :destroy], Ckeditor::Picture + can [:access, :read, :create, :destroy], Ckeditor::AttachmentFile + can [:read, :map], Debate can [:read, :map, :summary, :share], Proposal can :read, Comment diff --git a/config/routes.rb b/config/routes.rb index f3739c922..1eb129a21 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1,5 +1,6 @@ Rails.application.routes.draw do + mount Ckeditor::Engine => '/ckeditor' if Rails.env.development? || Rails.env.staging? get '/sandbox' => 'sandbox#index' get '/sandbox/*template' => 'sandbox#show' From 88f0f14eab8f87f0e0249456914c4f9d3871298d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Fuentes?= Date: Tue, 14 Aug 2018 14:25:05 +0200 Subject: [PATCH 03/18] Modify the configuration of ckeditor 4 add links, headers and images --- app/assets/javascripts/ckeditor/config.js | 24 ++-------- config/initializers/ckeditor.rb | 58 ++++++++++++++++++++++- 2 files changed, 59 insertions(+), 23 deletions(-) diff --git a/app/assets/javascripts/ckeditor/config.js b/app/assets/javascripts/ckeditor/config.js index 8d6edd348..445d98963 100644 --- a/app/assets/javascripts/ckeditor/config.js +++ b/app/assets/javascripts/ckeditor/config.js @@ -5,34 +5,13 @@ For licensing, see LICENSE.html or http://ckeditor.com/license CKEDITOR.editorConfig = function( config ) { - // Define changes to default configuration here. For example: - // config.language = 'fr'; - // config.uiColor = '#AADC6E'; - - /* Filebrowser routes */ - // The location of an external file browser, that should be launched when "Browse Server" button is pressed. config.filebrowserBrowseUrl = "/ckeditor/attachment_files"; - - // The location of an external file browser, that should be launched when "Browse Server" button is pressed in the Flash dialog. config.filebrowserFlashBrowseUrl = "/ckeditor/attachment_files"; - - // The location of a script that handles file uploads in the Flash dialog. config.filebrowserFlashUploadUrl = "/ckeditor/attachment_files"; - - // The location of an external file browser, that should be launched when "Browse Server" button is pressed in the Link tab of Image dialog. config.filebrowserImageBrowseLinkUrl = "/ckeditor/pictures"; - - // The location of an external file browser, that should be launched when "Browse Server" button is pressed in the Image dialog. config.filebrowserImageBrowseUrl = "/ckeditor/pictures"; - - // The location of a script that handles file uploads in the Image dialog. config.filebrowserImageUploadUrl = "/ckeditor/pictures"; - - // The location of a script that handles file uploads. config.filebrowserUploadUrl = "/ckeditor/attachment_files"; - - config.allowedContent = true; - // Rails CSRF token config.filebrowserParams = function(){ var csrf_token, csrf_param, meta, @@ -109,6 +88,9 @@ CKEDITOR.editorConfig = function( config ) config.toolbar_mini = [ { name: 'paragraph', groups: [ 'list' ], items: [ 'NumberedList', 'BulletedList' ] }, + { name: 'links', items: [ 'Link', 'Unlink' ] }, + { name: 'styles', items: [ 'Format' ] }, + { name: 'insert', items: [ 'Image' ] }, { name: 'basicstyles', groups: [ 'basicstyles', 'cleanup' ], items: [ 'Bold', 'Italic', 'Underline', 'Strike' ] } ]; config.toolbar = "mini"; diff --git a/config/initializers/ckeditor.rb b/config/initializers/ckeditor.rb index 829c3fa36..8f80a4308 100644 --- a/config/initializers/ckeditor.rb +++ b/config/initializers/ckeditor.rb @@ -1,4 +1,58 @@ +# Use this hook to configure ckeditor Ckeditor.setup do |config| - config.assets_languages = Rails.application.config.i18n.available_locales.map{|l| l.to_s.downcase} - config.assets_plugins = %w[copyformatting tableselection scayt wsc] + # ==> ORM configuration + # Load and configure the ORM. Supports :active_record (default), :mongo_mapper and + # :mongoid (bson_ext recommended) by default. Other ORMs may be + # available as additional gems. + require 'ckeditor/orm/active_record' + + # Allowed image file types for upload. + # Set to nil or [] (empty array) for all file types + # By default: %w(jpg jpeg png gif tiff) + # config.image_file_types = %w(jpg jpeg png gif tiff) + + # Allowed flash file types for upload. + # Set to nil or [] (empty array) for all file types + # By default: %w(jpg jpeg png gif tiff) + # config.flash_file_types = %w(swf) + + # Allowed attachment file types for upload. + # Set to nil or [] (empty array) for all file types + # By default: %w(doc docx xls odt ods pdf rar zip tar tar.gz swf) + # config.attachment_file_types = %w(doc docx xls odt ods pdf rar zip tar tar.gz swf) + + # Setup authorization to be run as a before filter + # By default: there is no authorization. + # config.authorize_with :cancan + + # Override parent controller CKEditor inherits from + # By default: 'ApplicationController' + # config.parent_controller = 'MyController' + + # Asset model classes + # config.picture_model { Ckeditor::Picture } + # config.attachment_file_model { Ckeditor::AttachmentFile } + + # Paginate assets + # By default: 24 + # config.default_per_page = 24 + + # Customize ckeditor assets path + # By default: nil + # config.asset_path = 'http://www.example.com/assets/ckeditor/' + + # To reduce the asset precompilation time, you can limit plugins and/or languages to those you need: + # By default: nil (no limit) + # config.assets_languages = ['en', 'uk'] + # config.assets_plugins = ['image', 'smiley'] + + # CKEditor CDN + # More info here http://cdn.ckeditor.com/ + # By default: nil (CDN disabled) + # config.cdn_url = '//cdn.ckeditor.com/4.7.1/standard/ckeditor.js' + + # JS config url + # Used when CKEditor CDN enabled + # By default: "ckeditor/config.js" + # config.js_config_url = 'ckeditor/config.js' end From d574657b779f2cce275b2788d49a4fb7c19d3258 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Fuentes?= Date: Fri, 24 Aug 2018 15:11:54 +0200 Subject: [PATCH 04/18] Fixes for the review made by @javierm this commit will be merged with the others when the chages are accepted --- app/controllers/application_controller.rb | 8 ++++++++ app/controllers/ckeditor/pictures_controller.rb | 8 +++++--- app/models/abilities/common.rb | 3 +++ app/models/abilities/everyone.rb | 4 +--- app/models/ckeditor/attachment_file.rb | 13 ------------- app/models/ckeditor/picture.rb | 6 +++--- config/initializers/ckeditor.rb | 2 ++ db/migrate/20180813141443_create_ckeditor_assets.rb | 4 +++- db/schema.rb | 1 + lib/wysiwyg_sanitizer.rb | 4 ++-- 10 files changed, 28 insertions(+), 25 deletions(-) delete mode 100644 app/models/ckeditor/attachment_file.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index e97b87cd3..06e0c79d8 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -27,8 +27,16 @@ class ApplicationController < ActionController::Base respond_to :html helper_method :current_budget + before_action :set_user_for_ckeditor_pictures + private + def set_user_for_ckeditor_pictures + if request.path == '/ckeditor/pictures' && request.request_method == 'POST' + params['user_id'] = current_user.id + end + end + def authenticate_http_basic authenticate_or_request_with_http_basic do |username, password| username == Rails.application.secrets.http_basic_username && password == Rails.application.secrets.http_basic_password diff --git a/app/controllers/ckeditor/pictures_controller.rb b/app/controllers/ckeditor/pictures_controller.rb index 1dded38f3..4cdb9f1ea 100644 --- a/app/controllers/ckeditor/pictures_controller.rb +++ b/app/controllers/ckeditor/pictures_controller.rb @@ -1,5 +1,7 @@ class Ckeditor::PicturesController < Ckeditor::ApplicationController + load_and_authorize_resource + def index @pictures = Ckeditor.picture_adapter.find_all(ckeditor_pictures_scope) @pictures = Ckeditor::Paginatable.new(@pictures).page(params[:page]) @@ -10,7 +12,7 @@ class Ckeditor::PicturesController < Ckeditor::ApplicationController end def create - @picture = Ckeditor.picture_model.new + @picture = Ckeditor.picture_model.new(user_id: current_user.id) respond_with_asset(@picture) end @@ -30,7 +32,7 @@ class Ckeditor::PicturesController < Ckeditor::ApplicationController end def authorize_resource - model = (@picture || Ckeditor.picture_model) + model = @picture || Ckeditor.picture_model @authorization_adapter.try(:authorize, params[:action], model) end -end \ No newline at end of file +end diff --git a/app/models/abilities/common.rb b/app/models/abilities/common.rb index 7c84089b4..d1b2cdf94 100644 --- a/app/models/abilities/common.rb +++ b/app/models/abilities/common.rb @@ -5,6 +5,9 @@ module Abilities def initialize(user) merge Abilities::Everyone.new(user) + can :access, :ckeditor # needed to access Ckeditor filebrowser + can [:access, :create, :destroy], Ckeditor::Picture, id: user.id + can [:read, :update], User, id: user.id can :read, Debate diff --git a/app/models/abilities/everyone.rb b/app/models/abilities/everyone.rb index dcc430cdd..090823e5c 100644 --- a/app/models/abilities/everyone.rb +++ b/app/models/abilities/everyone.rb @@ -3,10 +3,8 @@ module Abilities include CanCan::Ability def initialize(user) - can :access, :ckeditor # needed to access Ckeditor filebrowser - can [:access, :read, :create, :destroy], Ckeditor::Picture - can [:access, :read, :create, :destroy], Ckeditor::AttachmentFile + can :read, Ckeditor::Picture can [:read, :map], Debate can [:read, :map, :summary, :share], Proposal can :read, Comment diff --git a/app/models/ckeditor/attachment_file.rb b/app/models/ckeditor/attachment_file.rb deleted file mode 100644 index 8d0c2eec7..000000000 --- a/app/models/ckeditor/attachment_file.rb +++ /dev/null @@ -1,13 +0,0 @@ -class Ckeditor::AttachmentFile < Ckeditor::Asset - has_attached_file :data, - url: '/ckeditor_assets/attachments/:id/:filename', - path: ':rails_root/public/ckeditor_assets/attachments/:id/:filename' - - validates_attachment_presence :data - validates_attachment_size :data, less_than: 100.megabytes - do_not_validate_attachment_file_type :data - - def url_thumb - @url_thumb ||= Ckeditor::Utils.filethumb(filename) - end -end diff --git a/app/models/ckeditor/picture.rb b/app/models/ckeditor/picture.rb index 445c2bbd9..9c05daabf 100644 --- a/app/models/ckeditor/picture.rb +++ b/app/models/ckeditor/picture.rb @@ -4,9 +4,9 @@ class Ckeditor::Picture < Ckeditor::Asset path: ':rails_root/public/ckeditor_assets/pictures/:id/:style_:basename.:extension', styles: { content: '800>', thumb: '118x100#' } - validates_attachment_presence :data - validates_attachment_size :data, less_than: 2.megabytes - validates_attachment_content_type :data, content_type: /\Aimage/ + # validates_attachment_presence :data + # validates_attachment_size :data, less_than: 2.megabytes + # validates_attachment_content_type :data, content_type: /\Aimage/ def url_content url(:content) diff --git a/config/initializers/ckeditor.rb b/config/initializers/ckeditor.rb index 8f80a4308..c0ee18dba 100644 --- a/config/initializers/ckeditor.rb +++ b/config/initializers/ckeditor.rb @@ -56,3 +56,5 @@ Ckeditor.setup do |config| # By default: "ckeditor/config.js" # config.js_config_url = 'ckeditor/config.js' end + +Ckeditor::PicturesController.send(:load_and_authorize_resource) diff --git a/db/migrate/20180813141443_create_ckeditor_assets.rb b/db/migrate/20180813141443_create_ckeditor_assets.rb index df3df2862..098933bce 100644 --- a/db/migrate/20180813141443_create_ckeditor_assets.rb +++ b/db/migrate/20180813141443_create_ckeditor_assets.rb @@ -7,10 +7,12 @@ class CreateCkeditorAssets < ActiveRecord::Migration t.string :data_fingerprint t.string :type, limit: 30 - # Uncomment it to save images dimensions, if your need it + # Uncomment it to save images dimensions, if you need it t.integer :width t.integer :height + t.integer :user_id + t.timestamps null: false end diff --git a/db/schema.rb b/db/schema.rb index e03b22ac7..3f7771736 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -302,6 +302,7 @@ ActiveRecord::Schema.define(version: 20180813141443) do t.string "type", limit: 30 t.integer "width" t.integer "height" + t.integer "user_id" t.datetime "created_at", null: false t.datetime "updated_at", null: false end diff --git a/lib/wysiwyg_sanitizer.rb b/lib/wysiwyg_sanitizer.rb index 681c7b5fd..64c26c34f 100644 --- a/lib/wysiwyg_sanitizer.rb +++ b/lib/wysiwyg_sanitizer.rb @@ -1,7 +1,7 @@ class WYSIWYGSanitizer - ALLOWED_TAGS = %w(p ul ol li strong em u s) - ALLOWED_ATTRIBUTES = [] + ALLOWED_TAGS = %w(p ul ol li strong em u s img a h1 h2 h3 h4 h6 pre addres div) + ALLOWED_ATTRIBUTES = %w(href style src alt) def sanitize(html) ActionController::Base.helpers.sanitize(html, tags: ALLOWED_TAGS, attributes: ALLOWED_ATTRIBUTES) From bf0472fd5802771b7c255925897afd72f930081a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Fuentes?= Date: Tue, 28 Aug 2018 09:52:36 +0200 Subject: [PATCH 05/18] Fix test now the tag is permited, also the list of allowed tags is in the configuration of ckeditor4 --- spec/models/budget/phase_spec.rb | 7 ------- 1 file changed, 7 deletions(-) diff --git a/spec/models/budget/phase_spec.rb b/spec/models/budget/phase_spec.rb index 1fe39c20f..5afc57dfa 100644 --- a/spec/models/budget/phase_spec.rb +++ b/spec/models/budget/phase_spec.rb @@ -222,11 +222,4 @@ describe Budget::Phase do end end - describe "#sanitize_description" do - it "removes html entities from the description" do - expect{ - first_phase.update_attributes(description: "a

javascript") - }.to change{ first_phase.description }.to('a javascript') - end - end end From 116bdebc96e18b624e97526028e4376c5c21ae09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 7 Sep 2018 13:23:47 +0200 Subject: [PATCH 06/18] Don't add CKEditor attachments to version control --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 7761dba2a..0413aaa36 100644 --- a/.gitignore +++ b/.gitignore @@ -33,3 +33,4 @@ public/sitemap.xml public/system/ +/public/ckeditor_assets/ From 471061f475e4b3cd4294941564f6033c9f49758f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 7 Sep 2018 13:26:47 +0200 Subject: [PATCH 07/18] Allow only admins to attach CKEditor images Right now allowing regular users to attach images would make us vulnerable to CSRF attacks. --- app/models/abilities/administrator.rb | 3 +++ app/models/abilities/common.rb | 3 --- app/models/abilities/everyone.rb | 2 -- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/app/models/abilities/administrator.rb b/app/models/abilities/administrator.rb index 40e951326..a761f7abc 100644 --- a/app/models/abilities/administrator.rb +++ b/app/models/abilities/administrator.rb @@ -79,6 +79,9 @@ module Abilities can :manage, SiteCustomization::Image can :manage, SiteCustomization::ContentBlock + can :access, :ckeditor + can :manage, Ckeditor::Picture + can [:manage], ::Legislation::Process can [:manage], ::Legislation::DraftVersion can [:manage], ::Legislation::Question diff --git a/app/models/abilities/common.rb b/app/models/abilities/common.rb index d1b2cdf94..7c84089b4 100644 --- a/app/models/abilities/common.rb +++ b/app/models/abilities/common.rb @@ -5,9 +5,6 @@ module Abilities def initialize(user) merge Abilities::Everyone.new(user) - can :access, :ckeditor # needed to access Ckeditor filebrowser - can [:access, :create, :destroy], Ckeditor::Picture, id: user.id - can [:read, :update], User, id: user.id can :read, Debate diff --git a/app/models/abilities/everyone.rb b/app/models/abilities/everyone.rb index 090823e5c..23cfdc971 100644 --- a/app/models/abilities/everyone.rb +++ b/app/models/abilities/everyone.rb @@ -3,8 +3,6 @@ module Abilities include CanCan::Ability def initialize(user) - - can :read, Ckeditor::Picture can [:read, :map], Debate can [:read, :map, :summary, :share], Proposal can :read, Comment From b6855b7140d70e926e88ff6af19d203a4ba4ab40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 7 Sep 2018 13:35:38 +0200 Subject: [PATCH 08/18] Restore allowedContent line We're not sure why it was removed, and we're using server-side sanitizers to remove dangerous HTML. --- app/assets/javascripts/ckeditor/config.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/assets/javascripts/ckeditor/config.js b/app/assets/javascripts/ckeditor/config.js index 445d98963..c6dbd10da 100644 --- a/app/assets/javascripts/ckeditor/config.js +++ b/app/assets/javascripts/ckeditor/config.js @@ -12,6 +12,9 @@ CKEDITOR.editorConfig = function( config ) config.filebrowserImageBrowseUrl = "/ckeditor/pictures"; config.filebrowserImageUploadUrl = "/ckeditor/pictures"; config.filebrowserUploadUrl = "/ckeditor/attachment_files"; + + config.allowedContent = true; + // Rails CSRF token config.filebrowserParams = function(){ var csrf_token, csrf_param, meta, From 9705360d9880000098c5830cfbdc05ca372dc6ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 7 Sep 2018 13:37:07 +0200 Subject: [PATCH 09/18] Enable only a few headings in CKEditor Allowing every format is way more than what we initially intended. I've only added h2 and h3 because h1 is set somewhere else in the page (like the title), and h4, h5 and h6 are usually not necessary. --- app/assets/javascripts/ckeditor/config.js | 1 + lib/wysiwyg_sanitizer.rb | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/ckeditor/config.js b/app/assets/javascripts/ckeditor/config.js index c6dbd10da..305d0faf9 100644 --- a/app/assets/javascripts/ckeditor/config.js +++ b/app/assets/javascripts/ckeditor/config.js @@ -14,6 +14,7 @@ CKEDITOR.editorConfig = function( config ) config.filebrowserUploadUrl = "/ckeditor/attachment_files"; config.allowedContent = true; + config.format_tags = "p;h2;h3"; // Rails CSRF token config.filebrowserParams = function(){ diff --git a/lib/wysiwyg_sanitizer.rb b/lib/wysiwyg_sanitizer.rb index 64c26c34f..26792b21c 100644 --- a/lib/wysiwyg_sanitizer.rb +++ b/lib/wysiwyg_sanitizer.rb @@ -1,6 +1,6 @@ class WYSIWYGSanitizer - ALLOWED_TAGS = %w(p ul ol li strong em u s img a h1 h2 h3 h4 h6 pre addres div) + ALLOWED_TAGS = %w(p ul ol li strong em u s img a h2 h3) ALLOWED_ATTRIBUTES = %w(href style src alt) def sanitize(html) From 27155dd7d7f3cc012d788ea8defa87f71c46337a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 7 Sep 2018 13:45:23 +0200 Subject: [PATCH 10/18] Fix typo --- config/routes.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/config/routes.rb b/config/routes.rb index 1eb129a21..6f65956f3 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1,6 +1,7 @@ Rails.application.routes.draw do mount Ckeditor::Engine => '/ckeditor' + if Rails.env.development? || Rails.env.staging? get '/sandbox' => 'sandbox#index' get '/sandbox/*template' => 'sandbox#show' From 71ce7acc1021f4633a103a5ec1ff39430f89a854 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 7 Sep 2018 13:52:35 +0200 Subject: [PATCH 11/18] Restore deleted CKEditor config Just so we can navigate throught git history faster: * Introduced in 345e34d to avoid precompiling all CKEditor assets. * Modified in 54c82a5 to avoid compiling assets during tests. * Overwritten by `rails g ckeditor:install` in c0d6c0b. --- config/initializers/ckeditor.rb | 52 ++------------------------------- 1 file changed, 2 insertions(+), 50 deletions(-) diff --git a/config/initializers/ckeditor.rb b/config/initializers/ckeditor.rb index c0ee18dba..5b6589f95 100644 --- a/config/initializers/ckeditor.rb +++ b/config/initializers/ckeditor.rb @@ -1,4 +1,3 @@ -# Use this hook to configure ckeditor Ckeditor.setup do |config| # ==> ORM configuration # Load and configure the ORM. Supports :active_record (default), :mongo_mapper and @@ -6,55 +5,8 @@ Ckeditor.setup do |config| # available as additional gems. require 'ckeditor/orm/active_record' - # Allowed image file types for upload. - # Set to nil or [] (empty array) for all file types - # By default: %w(jpg jpeg png gif tiff) - # config.image_file_types = %w(jpg jpeg png gif tiff) - - # Allowed flash file types for upload. - # Set to nil or [] (empty array) for all file types - # By default: %w(jpg jpeg png gif tiff) - # config.flash_file_types = %w(swf) - - # Allowed attachment file types for upload. - # Set to nil or [] (empty array) for all file types - # By default: %w(doc docx xls odt ods pdf rar zip tar tar.gz swf) - # config.attachment_file_types = %w(doc docx xls odt ods pdf rar zip tar tar.gz swf) - - # Setup authorization to be run as a before filter - # By default: there is no authorization. - # config.authorize_with :cancan - - # Override parent controller CKEditor inherits from - # By default: 'ApplicationController' - # config.parent_controller = 'MyController' - - # Asset model classes - # config.picture_model { Ckeditor::Picture } - # config.attachment_file_model { Ckeditor::AttachmentFile } - - # Paginate assets - # By default: 24 - # config.default_per_page = 24 - - # Customize ckeditor assets path - # By default: nil - # config.asset_path = 'http://www.example.com/assets/ckeditor/' - - # To reduce the asset precompilation time, you can limit plugins and/or languages to those you need: - # By default: nil (no limit) - # config.assets_languages = ['en', 'uk'] - # config.assets_plugins = ['image', 'smiley'] - - # CKEditor CDN - # More info here http://cdn.ckeditor.com/ - # By default: nil (CDN disabled) - # config.cdn_url = '//cdn.ckeditor.com/4.7.1/standard/ckeditor.js' - - # JS config url - # Used when CKEditor CDN enabled - # By default: "ckeditor/config.js" - # config.js_config_url = 'ckeditor/config.js' + config.assets_languages = Rails.application.config.i18n.available_locales.map{|l| l.to_s.downcase} + config.assets_plugins = %w[copyformatting tableselection scayt wsc] end Ckeditor::PicturesController.send(:load_and_authorize_resource) From 531750cf6fa1f50778b4f44d57ce4c9aff2aa31f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 7 Sep 2018 13:54:30 +0200 Subject: [PATCH 12/18] Restore commented lines Not sure if they were supposed to be removed. For now I'll assume they were commented accidentally. --- app/models/ckeditor/picture.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/ckeditor/picture.rb b/app/models/ckeditor/picture.rb index 9c05daabf..445c2bbd9 100644 --- a/app/models/ckeditor/picture.rb +++ b/app/models/ckeditor/picture.rb @@ -4,9 +4,9 @@ class Ckeditor::Picture < Ckeditor::Asset path: ':rails_root/public/ckeditor_assets/pictures/:id/:style_:basename.:extension', styles: { content: '800>', thumb: '118x100#' } - # validates_attachment_presence :data - # validates_attachment_size :data, less_than: 2.megabytes - # validates_attachment_content_type :data, content_type: /\Aimage/ + validates_attachment_presence :data + validates_attachment_size :data, less_than: 2.megabytes + validates_attachment_content_type :data, content_type: /\Aimage/ def url_content url(:content) From 7347874f4b7a9a910314141822a51688ccc7f62f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 7 Sep 2018 14:00:29 +0200 Subject: [PATCH 13/18] Remove inaccurate comment It was automatically added by `rails g ckeditor:install`. --- db/migrate/20180813141443_create_ckeditor_assets.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/db/migrate/20180813141443_create_ckeditor_assets.rb b/db/migrate/20180813141443_create_ckeditor_assets.rb index 098933bce..ee857cfd0 100644 --- a/db/migrate/20180813141443_create_ckeditor_assets.rb +++ b/db/migrate/20180813141443_create_ckeditor_assets.rb @@ -7,7 +7,6 @@ class CreateCkeditorAssets < ActiveRecord::Migration t.string :data_fingerprint t.string :type, limit: 30 - # Uncomment it to save images dimensions, if you need it t.integer :width t.integer :height From 43e83889ff59e6bd4f754aacadbf0f2ba29eb24a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 7 Sep 2018 14:03:01 +0200 Subject: [PATCH 14/18] Simplify CKEditor authorization We can use the `config.authorize_with` option, so we don't need to copy the controller in order to load and authorize resource. Besides, only administrators can upload images, so we don't need to track the image's user id. --- app/controllers/application_controller.rb | 8 ---- .../ckeditor/pictures_controller.rb | 38 ------------------- config/initializers/ckeditor.rb | 4 +- .../20180813141443_create_ckeditor_assets.rb | 2 - db/schema.rb | 1 - 5 files changed, 2 insertions(+), 51 deletions(-) delete mode 100644 app/controllers/ckeditor/pictures_controller.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 06e0c79d8..e97b87cd3 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -27,16 +27,8 @@ class ApplicationController < ActionController::Base respond_to :html helper_method :current_budget - before_action :set_user_for_ckeditor_pictures - private - def set_user_for_ckeditor_pictures - if request.path == '/ckeditor/pictures' && request.request_method == 'POST' - params['user_id'] = current_user.id - end - end - def authenticate_http_basic authenticate_or_request_with_http_basic do |username, password| username == Rails.application.secrets.http_basic_username && password == Rails.application.secrets.http_basic_password diff --git a/app/controllers/ckeditor/pictures_controller.rb b/app/controllers/ckeditor/pictures_controller.rb deleted file mode 100644 index 4cdb9f1ea..000000000 --- a/app/controllers/ckeditor/pictures_controller.rb +++ /dev/null @@ -1,38 +0,0 @@ -class Ckeditor::PicturesController < Ckeditor::ApplicationController - - load_and_authorize_resource - - def index - @pictures = Ckeditor.picture_adapter.find_all(ckeditor_pictures_scope) - @pictures = Ckeditor::Paginatable.new(@pictures).page(params[:page]) - - respond_to do |format| - format.html { render :layout => @pictures.first_page? } - end - end - - def create - @picture = Ckeditor.picture_model.new(user_id: current_user.id) - respond_with_asset(@picture) - end - - def destroy - @picture.destroy - - respond_to do |format| - format.html { redirect_to pictures_path } - format.json { render :nothing => true, :status => 204 } - end - end - - protected - - def find_asset - @picture = Ckeditor.picture_adapter.get!(params[:id]) - end - - def authorize_resource - model = @picture || Ckeditor.picture_model - @authorization_adapter.try(:authorize, params[:action], model) - end -end diff --git a/config/initializers/ckeditor.rb b/config/initializers/ckeditor.rb index 5b6589f95..1e173cca8 100644 --- a/config/initializers/ckeditor.rb +++ b/config/initializers/ckeditor.rb @@ -5,8 +5,8 @@ Ckeditor.setup do |config| # available as additional gems. require 'ckeditor/orm/active_record' + config.authorize_with :cancan + config.assets_languages = Rails.application.config.i18n.available_locales.map{|l| l.to_s.downcase} config.assets_plugins = %w[copyformatting tableselection scayt wsc] end - -Ckeditor::PicturesController.send(:load_and_authorize_resource) diff --git a/db/migrate/20180813141443_create_ckeditor_assets.rb b/db/migrate/20180813141443_create_ckeditor_assets.rb index ee857cfd0..f7b5b85f4 100644 --- a/db/migrate/20180813141443_create_ckeditor_assets.rb +++ b/db/migrate/20180813141443_create_ckeditor_assets.rb @@ -10,8 +10,6 @@ class CreateCkeditorAssets < ActiveRecord::Migration t.integer :width t.integer :height - t.integer :user_id - t.timestamps null: false end diff --git a/db/schema.rb b/db/schema.rb index 3f7771736..e03b22ac7 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -302,7 +302,6 @@ ActiveRecord::Schema.define(version: 20180813141443) do t.string "type", limit: 30 t.integer "width" t.integer "height" - t.integer "user_id" t.datetime "created_at", null: false t.datetime "updated_at", null: false end From 5faeefab2cb32ef2ec73239506b2fe8638ef0ab5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 7 Sep 2018 14:11:29 +0200 Subject: [PATCH 15/18] Show the image button only if editing admin pages That's the only place where we need to attach images so far. --- app/assets/javascripts/ckeditor/config.js | 6 +++++- app/views/admin/site_customization/pages/_form.html.erb | 3 ++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/ckeditor/config.js b/app/assets/javascripts/ckeditor/config.js index 305d0faf9..1ec099fc0 100644 --- a/app/assets/javascripts/ckeditor/config.js +++ b/app/assets/javascripts/ckeditor/config.js @@ -94,8 +94,12 @@ CKEDITOR.editorConfig = function( config ) { name: 'paragraph', groups: [ 'list' ], items: [ 'NumberedList', 'BulletedList' ] }, { name: 'links', items: [ 'Link', 'Unlink' ] }, { name: 'styles', items: [ 'Format' ] }, - { name: 'insert', items: [ 'Image' ] }, { name: 'basicstyles', groups: [ 'basicstyles', 'cleanup' ], items: [ 'Bold', 'Italic', 'Underline', 'Strike' ] } ]; + + config.toolbar_admin = config.toolbar_mini.concat([ + { name: 'insert', items: [ 'Image' ] } + ]); + config.toolbar = "mini"; }; diff --git a/app/views/admin/site_customization/pages/_form.html.erb b/app/views/admin/site_customization/pages/_form.html.erb index 88c2288b1..6d0416b6e 100644 --- a/app/views/admin/site_customization/pages/_form.html.erb +++ b/app/views/admin/site_customization/pages/_form.html.erb @@ -50,7 +50,8 @@
<%= f.label :content %> - <%= f.cktext_area :content, label: false, cols: 80, rows: 10, ckeditor: { language: I18n.locale } %> + <%= f.cktext_area :content, label: false, cols: 80, rows: 10, + ckeditor: { language: I18n.locale, toolbar: "admin" } %>
From f917f5eed94d66944adeb0de25b705a9011b123d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 7 Sep 2018 14:13:48 +0200 Subject: [PATCH 16/18] Filter image tags everywhere except in custom pages Allowing image tags everywhere makes us vulnerable to CSRF attacks. --- app/views/pages/custom_page.html.erb | 2 +- lib/admin_wysiwyg_sanitizer.rb | 9 +++++++++ lib/wysiwyg_sanitizer.rb | 10 +++++++--- spec/lib/admin_wysiwyg_sanitizer_spec.rb | 12 ++++++++++++ spec/lib/wysiwyg_sanitizer_spec.rb | 15 +++++++++++++++ 5 files changed, 44 insertions(+), 4 deletions(-) create mode 100644 lib/admin_wysiwyg_sanitizer.rb create mode 100644 spec/lib/admin_wysiwyg_sanitizer_spec.rb diff --git a/app/views/pages/custom_page.html.erb b/app/views/pages/custom_page.html.erb index bdd85ab30..61e167549 100644 --- a/app/views/pages/custom_page.html.erb +++ b/app/views/pages/custom_page.html.erb @@ -8,7 +8,7 @@

<%= @custom_page.subtitle%>

<% end %> - <%= text_with_links @custom_page.content %> + <%= safe_html_with_links AdminWYSIWYGSanitizer.new.sanitize(@custom_page.content) %>
<% if @custom_page.print_content_flag %> diff --git a/lib/admin_wysiwyg_sanitizer.rb b/lib/admin_wysiwyg_sanitizer.rb new file mode 100644 index 000000000..6e219f1e8 --- /dev/null +++ b/lib/admin_wysiwyg_sanitizer.rb @@ -0,0 +1,9 @@ +class AdminWYSIWYGSanitizer < WYSIWYGSanitizer + def allowed_tags + super + %w[img] + end + + def allowed_attributes + super + %w[alt src style] + end +end diff --git a/lib/wysiwyg_sanitizer.rb b/lib/wysiwyg_sanitizer.rb index 26792b21c..ee056383c 100644 --- a/lib/wysiwyg_sanitizer.rb +++ b/lib/wysiwyg_sanitizer.rb @@ -1,10 +1,14 @@ class WYSIWYGSanitizer + def allowed_tags + %w[p ul ol li strong em u s a h2 h3] + end - ALLOWED_TAGS = %w(p ul ol li strong em u s img a h2 h3) - ALLOWED_ATTRIBUTES = %w(href style src alt) + def allowed_attributes + %w[href] + end def sanitize(html) - ActionController::Base.helpers.sanitize(html, tags: ALLOWED_TAGS, attributes: ALLOWED_ATTRIBUTES) + ActionController::Base.helpers.sanitize(html, tags: allowed_tags, attributes: allowed_attributes) end end \ No newline at end of file diff --git a/spec/lib/admin_wysiwyg_sanitizer_spec.rb b/spec/lib/admin_wysiwyg_sanitizer_spec.rb new file mode 100644 index 000000000..4593b8699 --- /dev/null +++ b/spec/lib/admin_wysiwyg_sanitizer_spec.rb @@ -0,0 +1,12 @@ +require 'rails_helper' + +describe AdminWYSIWYGSanitizer do + let(:sanitizer) { AdminWYSIWYGSanitizer.new } + + describe '#sanitize' do + it 'allows images' do + html = 'DangerousSmile image' + expect(sanitizer.sanitize(html)).to eq(html) + end + end +end diff --git a/spec/lib/wysiwyg_sanitizer_spec.rb b/spec/lib/wysiwyg_sanitizer_spec.rb index 17236aa03..e86c351ef 100644 --- a/spec/lib/wysiwyg_sanitizer_spec.rb +++ b/spec/lib/wysiwyg_sanitizer_spec.rb @@ -15,10 +15,25 @@ describe WYSIWYGSanitizer do expect(subject.sanitize(html)).to eq(html) end + it 'allows links' do + html = '

Home

' + expect(subject.sanitize(html)).to eq(html) + end + + it 'allows headings' do + html = '

Objectives

Fix flaky specs

Explain why the test is flaky

' + expect(subject.sanitize(html)).to eq(html) + end + it 'filters out dangerous tags' do html = '

This is

' expect(subject.sanitize(html)).to eq('

This is alert("dangerous");

') end + + it 'filters images' do + html = 'DangerousSmile image' + expect(subject.sanitize(html)).to eq('Dangerous image') + end end end From 9cfa07ff1defb10f3b5051de2b2472114223b1d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 7 Sep 2018 14:24:45 +0200 Subject: [PATCH 17/18] Remove unrelated schema changes --- db/schema.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/db/schema.rb b/db/schema.rb index e03b22ac7..867737358 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1305,12 +1305,6 @@ ActiveRecord::Schema.define(version: 20180813141443) do add_index "votes", ["votable_id", "votable_type", "vote_scope"], name: "index_votes_on_votable_id_and_votable_type_and_vote_scope", using: :btree add_index "votes", ["voter_id", "voter_type", "vote_scope"], name: "index_votes_on_voter_id_and_voter_type_and_vote_scope", using: :btree - create_table "web_sections", force: :cascade do |t| - t.text "name" - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - end - create_table "widget_cards", force: :cascade do |t| t.string "title" t.text "description" @@ -1329,6 +1323,12 @@ ActiveRecord::Schema.define(version: 20180813141443) do t.datetime "updated_at", null: false end + create_table "web_sections", force: :cascade do |t| + t.text "name" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + add_foreign_key "administrators", "users" add_foreign_key "annotations", "legacy_legislations" add_foreign_key "annotations", "users" From f2bebca6bec84b0d5c1acdda42bac99151fae414 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 7 Sep 2018 14:27:05 +0200 Subject: [PATCH 18/18] Bring back and fix deleted test It was removed in 755be96 because some tags were allowed, but we can just update it to check it doesn't remove those tags. --- spec/models/budget/phase_spec.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/spec/models/budget/phase_spec.rb b/spec/models/budget/phase_spec.rb index 5afc57dfa..34b99e26f 100644 --- a/spec/models/budget/phase_spec.rb +++ b/spec/models/budget/phase_spec.rb @@ -222,4 +222,11 @@ describe Budget::Phase do end end + describe "#sanitize_description" do + it "removes not allowed html entities from the description" do + expect{ + first_phase.update_attributes(description: '

a

') + }.to change{ first_phase.description }.to('

a

javascript') + end + end end