enhancement: allowing editors to keep/move insecure content, but not …

…add/change it unless they are a site admin. Makes editing more flexible but secure.
kete · Feb 11, 2009 · 4fb50a6 · 4fb50a6
1 parent 62aec1f
commit 4fb50a6
Show file tree

Hide file tree

Showing 12 changed files with 94 additions and 23 deletions.
diff --git a/app/controllers/audio_controller.rb b/app/controllers/audio_controller.rb
@@ -50,7 +50,11 @@ def update
 
     version_after_update = @audio_recording.max_version + 1
 
-    if @audio_recording.update_attributes(params[:audio_recording])
+    @successful = ensure_no_new_insecure_elements_in('audio_recording')
+    @audio_recording.attributes = params[:audio_recording]
+    @successful = @audio_recording.save if @successful
+
+    if @successful
 
       after_successful_zoom_item_update(@audio_recording)
 

diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb
@@ -75,7 +75,11 @@ def update
 
     version_after_update = @comment.max_version + 1
 
-    if @comment.update_attributes(params[:comment])
+    @successful = ensure_no_new_insecure_elements_in('comment')
+    @comment.attributes = params[:comment]
+    @successful = @comment.save if @successful
+
+    if @successful
 
       @comment.add_as_contributor(current_user)
 

diff --git a/app/controllers/documents_controller.rb b/app/controllers/documents_controller.rb
@@ -49,7 +49,11 @@ def update
 
     version_after_update = @document.max_version + 1
 
-    if @document.update_attributes(params[:document])
+    @successful = ensure_no_new_insecure_elements_in('document')
+    @document.attributes = params[:document]
+    @successful = @document.save if @successful
+
+    if @successful
 
       after_successful_zoom_item_update(@document)
 

diff --git a/app/controllers/images_controller.rb b/app/controllers/images_controller.rb
@@ -90,7 +90,11 @@ def update
 
     version_after_update = @still_image.max_version + 1
 
-    if @still_image.update_attributes(params[:still_image])
+    @successful = ensure_no_new_insecure_elements_in('still_image')
+    @still_image.attributes = params[:still_image]
+    @successful = @still_image.save if @successful
+
+    if @successful
 
       if !params[:image_file][:uploaded_data].blank?
         # if they have uploaded something new, insert it

diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb
@@ -125,7 +125,9 @@ def update
 
         version_after_update = @topic.max_version + 1
 
-        @successful = @topic.update_attributes(params[:topic])
+        @successful = ensure_no_new_insecure_elements_in('topic')
+        @topic.attributes = params[:topic]
+        @successful = @topic.save if @successful
       else
         # they don't have permission
         # this will redirect them to edit

diff --git a/app/controllers/video_controller.rb b/app/controllers/video_controller.rb
@@ -49,7 +49,11 @@ def update
 
     version_after_update = @video.max_version + 1
 
-    if @video.update_attributes(params[:video])
+    @successful = ensure_no_new_insecure_elements_in('video')
+    @video.attributes = params[:video]
+    @successful = @video.save if @successful
+
+    if @successful
 
       after_successful_zoom_item_update(@video)
 

diff --git a/app/controllers/web_links_controller.rb b/app/controllers/web_links_controller.rb
@@ -48,7 +48,11 @@ def update
 
     version_after_update = @web_link.max_version + 1
 
-    if @web_link.update_attributes(params[:web_link])
+    @successful = ensure_no_new_insecure_elements_in('web_link')
+    @web_link.attributes = params[:web_link]
+    @successful = @web_link.save if @successful
+
+    if @successful
 
       after_successful_zoom_item_update(@web_link)
 

diff --git a/app/models/topic.rb b/app/models/topic.rb
@@ -101,7 +101,7 @@ class Topic < ActiveRecord::Base
   validates_xml :extended_content
   validates_presence_of :title
 
-  validates_as_sanitized_html :description, :extended_content
+  validates_as_sanitized_html :extended_content
 
   # TODO: add validation that prevents markup in short_summary
   # globalize stuff, uncomment later

diff --git a/app/views/topics/_form.html.erb b/app/views/topics/_form.html.erb
@@ -111,13 +111,13 @@ zoom_class = zoom_class_from_controller(controller)
 
     <%= form.text_area :description, { :class => "mceEditor", :cols => 120, :tabindex => '1',
                                        :label => 'Description', :label_class => 'cleared-right',
-                                       :example => "Note: forms and javascript are not allowed for security reasons
+                                       :example => "Note: new forms and javascript are not allowed for security reasons
                                                     #{", unless you check the box below" if @site_admin}." } -%>
 
     <% if @site_admin -%>
       <%= form.check_box :do_not_sanitize, { :tabindex => '1',
-                                             :label => 'Allow forms and other possible insecure elements
-                                                        and attributes in the HTML?' } -%>
+                                             :label => "Allow new forms and other possible insecure elements
+                                                        and attributes in the HTML?" } -%>
     <% end -%>
 
     <% unless [:topic, :web_link].include?(@item_type) -%>

diff --git a/config/initializers/tinymce.rb b/config/initializers/tinymce.rb
@@ -1,14 +1,15 @@
-fields = Array.new
-fields << 'code[class|dir<ltr?rtl|id|lang|onclick|ondblclick|onkeydown|onkeypress|onkeyup|onmousedown|onmousemove|onmouseout|onmouseover|onmouseup|style|title]'
-fields << 'form[accept|accept-charset|action|class|dir<ltr?rtl|enctype|id|lang|method<get?post|name|onclick|ondblclick|onkeydown|onkeypress|onkeyup|onmousedown|onmousemove|onmouseout|onmouseover|onmouseup|onreset|onsubmit|style|title|target]'
-fields << 'input[accept|accesskey|align<bottom?left?middle?right?top|alt|checked<checked|class|dir<ltr?rtl|disabled<disabled|id|ismap<ismap|lang|maxlength|name|onblur|onclick|ondblclick|onfocus|onkeydown|onkeypress|onkeyup|onmousedown|onmousemove|onmouseout|onmouseover|onmouseup|onselect|readonly<readonly|size|src|style|tabindex|title|type<button?checkbox?file?hidden?image?password?radio?reset?submit?text|usemap|value]'
-fields << 'select[class|dir<ltr?rtl|disabled<disabled|id|lang|multiple<multiple|name|onblur|onchange|onclick|ondblclick|onfocus|onkeydown|onkeypress|onkeyup|onmousedown|onmousemove|onmouseout|onmouseover|onmouseup|size|style|tabindex|title]'
-fields << 'option[class|dir<ltr?rtl|disabled<disabled|id|label|lang|onclick|ondblclick|onkeydown|onkeypress|onkeyup|onmousedown|onmousemove|onmouseout|onmouseover|onmouseup|selected<selected|style|title|value]'
-fields << 'label[accesskey|class|dir<ltr?rtl|for|id|lang|onblur|onclick|ondblclick|onfocus|onkeydown|onkeypress|onkeyup|onmousedown|onmousemove|onmouseout|onmouseover|onmouseup|style|title]'
-fields << 'object[classid|codebase|width|height|align|data]'
-fields << 'param[name|value]'
-fields << 'embed[quality|type|pluginspage|width|height|src|align|wmode|flashvars|allowfullscreen]'
-EXTENDED_VALID_ELEMENTS = fields.join(',')
+fields = Hash.new
+fields[:code]  = '[class|dir<ltr?rtl|id|lang|onclick|ondblclick|onkeydown|onkeypress|onkeyup|onmousedown|onmousemove|onmouseout|onmouseover|onmouseup|style|title]'
+fields[:form]  = '[accept|accept-charset|action|class|dir<ltr?rtl|enctype|id|lang|method<get?post|name|onclick|ondblclick|onkeydown|onkeypress|onkeyup|onmousedown|onmousemove|onmouseout|onmouseover|onmouseup|onreset|onsubmit|style|title|target]'
+fields[:input] = '[accept|accesskey|align<bottom?left?middle?right?top|alt|checked<checked|class|dir<ltr?rtl|disabled<disabled|id|ismap<ismap|lang|maxlength|name|onblur|onclick|ondblclick|onfocus|onkeydown|onkeypress|onkeyup|onmousedown|onmousemove|onmouseout|onmouseover|onmouseup|onselect|readonly<readonly|size|src|style|tabindex|title|type<button?checkbox?file?hidden?image?password?radio?reset?submit?text|usemap|value]'
+fields[:select] = '[class|dir<ltr?rtl|disabled<disabled|id|lang|multiple<multiple|name|onblur|onchange|onclick|ondblclick|onfocus|onkeydown|onkeypress|onkeyup|onmousedown|onmousemove|onmouseout|onmouseover|onmouseup|size|style|tabindex|title]'
+fields[:option] = '[class|dir<ltr?rtl|disabled<disabled|id|label|lang|onclick|ondblclick|onkeydown|onkeypress|onkeyup|onmousedown|onmousemove|onmouseout|onmouseover|onmouseup|selected<selected|style|title|value]'
+fields[:label] = '[accesskey|class|dir<ltr?rtl|for|id|lang|onblur|onclick|ondblclick|onfocus|onkeydown|onkeypress|onkeyup|onmousedown|onmousemove|onmouseout|onmouseover|onmouseup|style|title]'
+fields[:object] = '[classid|codebase|width|height|align|data]'
+fields[:param] = '[name|value]'
+fields[:embed] = '[quality|type|pluginspage|width|height|src|align|wmode|flashvars|allowfullscreen]'
+EXTENDED_VALID_ELEMENTS_HASH = fields
+EXTENDED_VALID_ELEMENTS = fields.collect { |k,v| "#{k}#{v}" }.join(',')
 
 VALID_TINYMCE_ACTIONS = ['new', 'create', 'edit', 'update', 'homepage_options', 'appearance', 'choose_type', 'render_item_form', 'new_related_set_from_archive_file', 'restore']
 

diff --git a/lib/configure_as_kete_content_item.rb b/lib/configure_as_kete_content_item.rb
@@ -62,7 +62,7 @@ def self.included(klass)
 
       klass.send :validates_presence_of, :title
 
-      klass.send :validates_as_sanitized_html, [:description, :extended_content]
+      klass.send :validates_as_sanitized_html, [:extended_content]
 
       # TODO: globalize stuff, uncomment later
       # translates :title, :description

diff --git a/lib/extended_content_controller.rb b/lib/extended_content_controller.rb
@@ -1,3 +1,5 @@
+require 'nokogiri'
+
 module ExtendedContentController
   unless included_modules.include? ExtendedContentController
     def self.included(klass)
@@ -50,6 +52,48 @@ def load_content_type
 
       private
 
+      def ensure_no_new_insecure_elements_in(item_type)
+        return true if @site_admin && params[item_type.to_sym][:do_not_sanitize] == '1'
+
+        @item = eval("@#{item_type}")
+
+        old_doc = Nokogiri.HTML(@item.description) unless @item.description.blank?
+        existing_elements = Array.new
+        new_doc = Nokogiri::HTML(params[item_type.to_sym][:description]) unless params[item_type.to_sym][:description].blank?
+        current_elements = Array.new
+
+        EXTENDED_VALID_ELEMENTS_HASH.keys.each do |field_key|
+          old_doc.search("//#{field_key.to_s}").each { |element| existing_elements << element.to_s.strip } unless @item.description.blank?
+          new_doc.search("//#{field_key.to_s}").each { |element| current_elements << element.to_s.strip } unless params[item_type.to_sym][:description].blank?
+        end
+
+        params[item_type.to_sym][:do_not_sanitize] = '1'
+        new_elements = Array.new
+        current_elements.each do |element|
+          if existing_elements.include?(element)
+            # delete it as we go so we can't use the same one again later
+            existing_elements.delete_at(existing_elements.index(element))
+          else
+            new_elements << element
+            params[item_type.to_sym][:do_not_sanitize] = '0'
+          end
+        end
+
+        if new_elements.size > 0
+          if @site_admin
+            @item.errors.add('Description', "contains #{new_elements.size} new insecure elements and you forgot to check the 'do not sanitize' checkbox.")
+            false
+          else
+            @item.errors.add('Description', "contains #{new_elements.size} new insecure elements but you're not authorized to add them.")
+            logger.warn "WARNING: #{current_user.login} tried to add the following new elements to #{item_type} #{@item.id}"
+            new_elements.each { |element| logger.warn element.inspect }
+            false
+          end
+        else
+          true
+        end
+      end
+
       def build_relations_from_topic_type_extended_field_choices(extended_values=nil)
         params_key = zoom_class_params_key_from(params[:controller])
         extended_values = (extended_values || params[params_key][:extended_content_values])