fix xss vulnerability issue (#170)

Gemfile

@@ -40,7 +40,7 @@ gem 'rubyzip'
 gem 'locomotive_jammit-s3', :require => 'jammit-s3'
 gem 'SystemTimer', :platforms => :ruby_18
 gem 'cells'
-
+gem 'sanitize'
 gem 'highline'
 
 # The rest of the dependencies are for use when in the locomotive dev environment

Gemfile.lock

@@ -255,6 +255,8 @@ GEM
     rubyzip (0.9.4)
     s3 (0.3.8)
       proxies (~> 0.2.0)
+    sanitize (2.0.3)
+      nokogiri (>= 1.4.4, < 1.6)
     sass (3.1.2)
     selenium-webdriver (2.4.0)
       childprocess (>= 0.2.1)
@@ -331,6 +333,7 @@ DEPENDENCIES
   ruby-debug
   ruby-debug19
   rubyzip
+  sanitize
   sass (= 3.1.2)
   spork (~> 0.9.0.rc)
   unicorn

app/controllers/admin/api_contents_controller.rb

@@ -7,6 +7,10 @@ module Admin
 
     before_filter :set_content_type
 
+    before_filter :block_content_type_with_disabled_api
+
+    before_filter :sanitize_content_params, :only => :create
+
     def create
       @content = @content_type.contents.build(params[:content])
 
@@ -32,7 +36,22 @@ module Admin
 
     def set_content_type
       @content_type = current_site.content_types.where(:slug => params[:slug]).first
-      render :json => { :error => 'Api not enabled' } and return false unless @content_type.api_enabled
+    end
+
+    def block_content_type_with_disabled_api
+      unless @content_type.api_enabled?
+        respond_to do |format|
+          format.json { render :json => { :error => 'Api not enabled' }, :status => :forbidden  }
+          format.html { render :text => 'Api not enabled', :status => :forbidden }
+        end
+        return false
+      end
+    end
+
+    def sanitize_content_params
+      (params[:content] || {}).each do |key, value|
+        params[:content][key] = Sanitize.clean(value, Sanitize::Config::BASIC)
+      end
     end
 
   end

doc/changelogs/20110812.txt

@@ -6,4 +6,7 @@
 - Adding indexes for pages to avoid "Mongo::OperationFailure: too much data" errors (#159)
 - better spanish translations (#161, #162)
 - fix carrierwave version to 0.5.6 (#163)
-- has_many target is not exported correctly (issue #165) + fix the import module as well
+- has_many target is not exported correctly (issue #165) + fix the import module as well
+- XSS vulnerability when adding new content from the api_contents_controller (#170)
+- small bugs: #169
+-

locomotive_cms.gemspec

@@ -32,6 +32,8 @@ Gem::Specification.new do |s|
   s.add_dependency 'formtastic', '~> 1.2.3'
   s.add_dependency 'inherited_resources', '~> 1.1.2'
   s.add_dependency 'cells'
+  s.add_dependency 'highline'
+  s.add_dependency 'sanitize'
 
   s.add_dependency 'json_pure', '1.5.1'
   s.add_dependency 'bushido'

spec/controllers/admin/api_contents_controller_spec.rb

@@ -0,0 +1,80 @@
+require 'spec_helper'
+
+describe Admin::ApiContentsController do
+
+  before(:each) do
+    @site = Factory('existing site')
+    @site.content_types.first.tap do |content_type|
+      content_type.content_custom_fields.build :label => 'Name', :kind => 'string', :required => true
+      content_type.content_custom_fields.build :label => 'Description', :kind => 'text'
+    end.save
+
+    controller.stubs(:require_site).returns(true)
+    controller.stubs(:current_site).returns(@site)
+  end
+
+  describe 'API disabled' do
+
+    it 'blocks the creation of a new instance' do
+      post 'create', default_post_params
+
+      response.code.should eq('403')
+      response.body.should == 'Api not enabled'
+    end
+
+  end
+
+  describe 'API enabled' do
+
+    before(:each) do
+      ContentType.any_instance.stubs(:api_enabled?).returns(true)
+    end
+
+    it 'saves a content' do
+      post 'create', default_post_params
+
+      response.should redirect_to('http://www.locomotivecms.com/success')
+
+      @site.reload.content_types.first.contents.size.should == 1
+    end
+
+    it 'does not save a content if required parameters are missing' do
+      post 'create', default_post_params(:content => { :name => '' })
+
+      response.should redirect_to('http://www.locomotivecms.com/failure')
+
+      @site.reload.content_types.first.contents.size.should == 0
+    end
+
+    describe 'XSS vulnerability' do
+
+      it 'sanitizes the params (simple example)' do
+        post 'create', default_post_params(:content => { :name => %(Hacking <script type="text/javascript">alert("You have been hacked")</script>) })
+
+        content = @site.reload.content_types.first.contents.first
+
+        content.name.should == "Hacking alert(\"You have been hacked\")"
+      end
+
+      it 'sanitizes the params (more complex example)' do
+        post 'create', default_post_params(:content => { :name => %(<img src=javascript:alert('Hello')><table background="javascript:alert('Hello')">Hacked) })
+
+        content = @site.reload.content_types.first.contents.first
+
+        content.name.should == "Hacked"
+      end
+
+    end
+
+  end
+
+  def default_post_params(options = {})
+    {
+      :slug => 'projects',
+      :content => { :name => 'LocomotiveCMS', :description => 'Lorem ipsum' }.merge(options.delete(:content) || {}),
+      :success_callback => 'http://www.locomotivecms.com/success',
+      :error_callback => 'http://www.locomotivecms.com/failure'
+    }.merge(options)
+  end
+
+end