Added methods to strip those non-XML control characters from tags in AMF controller...

diff --git a/app/controllers/amf_controller.rb b/app/controllers/amf_controller.rb
index 2aee84d0cf0bf475735d8564bf18ddfc43324c9f..20053644d8635177a32ffd3c175328bb496ee29c 100644 (file)
--- a/app/controllers/amf_controller.rb
+++ b/app/controllers/amf_controller.rb
@@ -526,6 +526,8 @@ class AmfController < ApplicationController
     amf_handle_error("'putrelation' #{relid}")  do
       user = getuser(usertoken)
       if !user then return -1,"You are not logged in, so the relation could not be saved." end
+      if !tags_ok(tags) then return -1,"One of the tags is invalid. Please pester Adobe to fix Flash on Linux." end
+      tags = strip_non_xml_chars tags
 
       relid = relid.to_i
       visible = (visible.to_i != 0)
@@ -612,6 +614,8 @@ class AmfController < ApplicationController
       user = getuser(usertoken)
       if !user then return -1,"You are not logged in, so the way could not be saved." end
       if pointlist.length < 2 then return -2,"Server error - way is only #{points.length} points long." end
+      if !tags_ok(tags) then return -1,"One of the tags is invalid. Please pester Adobe to fix Flash on Linux." end
+      tags = strip_non_xml_chars tags
 
       originalway = originalway.to_i
       pointlist.collect! {|a| a.to_i }
@@ -708,6 +712,8 @@ class AmfController < ApplicationController
     amf_handle_error("'putpoi' #{id}") do
       user = getuser(usertoken)
       if !user then return -1,"You are not logged in, so the point could not be saved." end
+      if !tags_ok(tags) then return -1,"One of the tags is invalid. Please pester Adobe to fix Flash on Linux." end
+      tags = strip_non_xml_chars tags
 
       id = id.to_i
       visible = (visible.to_i == 1)
@@ -862,6 +868,31 @@ class AmfController < ApplicationController
   def getlocales
     Dir.glob("#{RAILS_ROOT}/config/potlatch/localised/*").collect { |f| File.basename(f) }
   end
+  
+  ##
+  # check that all key-value pairs are valid UTF-8.
+  def tags_ok(tags)
+    tags.each do |k, v|
+      return false unless UTF8.valid? k
+      return false unless UTF8.valid? v
+    end
+    return true
+  end
+
+  ##
+  # strip characters which are invalid in XML documents from the strings
+  # in the +tags+ hash.
+  def strip_non_xml_chars(tags)
+    new_tags = Hash.new
+    unless tags.nil?
+      tags.each do |k, v|
+        new_k = k.delete "\000-\037", "^\011\012\015"
+        new_v = v.delete "\000-\037", "^\011\012\015"
+        new_tags[new_k] = new_v
+      end
+    end
+    return new_tags
+  end
 
   # ====================================================================
   # Alternative SQL queries for getway/whichways

diff --git a/lib/utf8.rb b/lib/utf8.rb
new file mode 100644 (file)
index 0000000..5f0d219
--- /dev/null
+++ b/lib/utf8.rb
@@ -0,0 +1,14 @@
+module UTF8
+  ##
+  # Checks that a string is valid UTF-8 by trying to convert it to UTF-8
+  # using the iconv library, which is in the standard library.
+  def self.valid?(str)
+    return true if str.nil?
+    Iconv.conv("UTF-8", "UTF-8", str)
+    return true
+    
+  rescue
+    return false
+  end  
+end
+

diff --git a/lib/validators.rb b/lib/validators.rb
index 095fb7af9ac3dc1bedea4f231963e1835f5dac2b..640a495649fc82a32b4db508a5a02a74323c89d2 100644 (file)
--- a/lib/validators.rb
+++ b/lib/validators.rb
@@ -11,22 +11,10 @@ module ActiveRecord
       # is a valid UTF-8 format string.
       def validates_as_utf8(*attrs)
         validates_each(attrs) do |record, attr, value|
-          record.errors.add(attr, @@invalid_utf8_message) unless valid_utf8? value
+          record.errors.add(attr, @@invalid_utf8_message) unless UTF8.valid? value
         end
       end    
-      
-      ##
-      # Checks that a string is valid UTF-8 by trying to convert it to UTF-8
-      # using the iconv library, which is in the standard library.
-      def valid_utf8?(str)
-        return true if str.nil?
-        Iconv.conv("UTF-8", "UTF-8", str)
-        return true
 
-      rescue
-        return false
-      end  
-      
     end
   end
 end

diff --git a/test/functional/amf_controller_test.rb b/test/functional/amf_controller_test.rb
index ab0f5d5ad35cd6010c2a5590c35bf5493d472de8..9193cb4d2d3bd4f305dad45724c10b2fec5234b2 100644 (file)
--- a/test/functional/amf_controller_test.rb
+++ b/test/functional/amf_controller_test.rb
@@ -454,6 +454,65 @@ class AmfControllerTest < ActionController::TestCase
     assert_equal result[4], first_historic_node.version, "The version returned, is different to the one returned by the amf"
   end
   
+  # try creating a POI with rubbish in the tags
+  def test_putpoi_create_with_control_chars
+    # This node has no tags
+    nd = Node.new
+    # create a node with random lat/lon
+    lat = rand(100)-50 + rand
+    lon = rand(100)-50 + rand
+    # normal user has a changeset open
+    changeset = changesets(:public_user_first_change)
+    
+    mostly_invalid = 32.times.to_a.map {|i| i.chr}.join
+    tags = { "something" => "foo#{mostly_invalid}bar" }
+      
+    amf_content "putpoi", "/1", ["test@example.com:test", changeset.id, nil, nil, lon, lat, tags, nil]
+    post :amf_write
+    assert_response :success
+    amf_parse_response
+    result = amf_result("/1")
+      
+    # check the array returned by the amf
+    assert_equal 5, result.size
+    assert_equal 0, result[0], "Expected to get the status ok in the amf"
+    assert_equal 0, result[2], "The old id should be 0"
+    assert result[3] > 0, "The new id should be greater than 0"
+    assert_equal 1, result[4], "The new version should be 1"
+    
+    # Finally check that the node that was saved has saved the data correctly 
+    # in both the current and history tables
+    # First check the current table
+    current_node = Node.find(result[3])
+    assert_equal 1, current_node.tags.size, "There seems to be a tag that has been added to the node"
+    assert_equal({ "something" => "foo\t\n\rbar" }, current_node.tags, "tags were not fixed correctly")
+    assert_equal result[4], current_node.version, "The version returned, is different to the one returned by the amf"
+  end
+
+  # try creating a POI with rubbish in the tags
+  def test_putpoi_create_with_invalid_utf8
+    # This node has no tags
+    nd = Node.new
+    # create a node with random lat/lon
+    lat = rand(100)-50 + rand
+    lon = rand(100)-50 + rand
+    # normal user has a changeset open
+    changeset = changesets(:public_user_first_change)
+    
+    invalid = "\xc0\xc0"
+    tags = { "something" => "foo#{invalid}bar" }
+      
+    amf_content "putpoi", "/1", ["test@example.com:test", changeset.id, nil, nil, lon, lat, tags, nil]
+    post :amf_write
+    assert_response :success
+    amf_parse_response
+    result = amf_result("/1")
+
+    assert_equal 2, result.size
+    assert_equal -1, result[0], "Expected to get the status FAIL in the amf"
+    assert_equal "One of the tags is invalid. Please pester Adobe to fix Flash on Linux.", result[1] 
+  end
+      
   def test_putpoi_delete_valid
     
   end