Guard against non-numeric lat and lons in nodes and notes

diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb
index 1b28cae6e64bbed2aa6c81fb12007a75c4ef4e35..9c6eb94572488e75e5a99e6ddeb3b8a3c2617491 100644 (file)
--- a/app/controllers/notes_controller.rb
+++ b/app/controllers/notes_controller.rb
@@ -59,8 +59,16 @@ class NotesController < ApplicationController
     raise OSM::APIBadUserInput.new("No text was given") if params[:text].blank?
 
     # Extract the arguments
-    lon = params[:lon].to_f
-    lat = params[:lat].to_f
+    begin
+      lon = Float(params[:lon])
+    rescue
+      raise OSM::APIBadUserInput.new("lon was not a number")
+    end
+    begin
+      lat = Float(params[:lat])
+    rescue
+      raise OSM::APIBadUserInput.new("lat was not a number")
+    end
     comment = params[:text]
 
     # Include in a transaction to ensure that there is always a note_comment for every note

diff --git a/app/models/node.rb b/app/models/node.rb
index 2f528076eba632140ac85df651960617c78c5883..775f1fd3b22b6ba86d282c72ae5741339b96db6f 100644 (file)
--- a/app/models/node.rb
+++ b/app/models/node.rb
@@ -83,8 +83,16 @@ class Node < ActiveRecord::Base
     
     raise OSM::APIBadXMLError.new("node", pt, "lat missing") if pt['lat'].nil?
     raise OSM::APIBadXMLError.new("node", pt, "lon missing") if pt['lon'].nil?
-    node.lat = pt['lat'].to_f
-    node.lon = pt['lon'].to_f
+    begin
+      node.lat = Float(pt['lat'])
+    rescue
+      raise OSM::APIBadXMLError.new("node", pt, "lat not a number")
+    end
+    begin
+      node.lon = Float(pt['lon'])
+    rescue
+      raise OSM::APIBadXMLError.new("node", pt, "lon not a number")
+    end
     raise OSM::APIBadXMLError.new("node", pt, "Changeset id is missing") if pt['changeset'].nil?
     node.changeset_id = pt['changeset'].to_i
 

diff --git a/test/functional/node_controller_test.rb b/test/functional/node_controller_test.rb
index 6903dd60b425e7a68e5db5f33ccb135db9d1cfc4..32667d9c904950a44cf04584b4c390b1ad79cc7e 100644 (file)
--- a/test/functional/node_controller_test.rb
+++ b/test/functional/node_controller_test.rb
@@ -122,6 +122,22 @@ class NodeControllerTest < ActionController::TestCase
     assert_response :bad_request, "node upload did not return bad_request status"
     assert_equal "Cannot parse valid node from xml string <node lat=\"3.434\" changeset=\"#{changeset.id}\"/>. lon missing", @response.body
 
+    # test that the upload is rejected when lat is non-numeric
+    # create a minimal xml file
+    content("<osm><node lat='abc' lon='#{lon}' changeset='#{changeset.id}'/></osm>")
+    put :create
+    # hope for success
+    assert_response :bad_request, "node upload did not return bad_request status"
+    assert_equal "Cannot parse valid node from xml string <node lat=\"abc\" lon=\"#{lon}\" changeset=\"#{changeset.id}\"/>. lat not a number", @response.body
+
+    # test that the upload is rejected when lon is non-numeric
+    # create a minimal xml file
+    content("<osm><node lat='#{lat}' lon='abc' changeset='#{changeset.id}'/></osm>")
+    put :create
+    # hope for success
+    assert_response :bad_request, "node upload did not return bad_request status"
+    assert_equal "Cannot parse valid node from xml string <node lat=\"#{lat}\" lon=\"abc\" changeset=\"#{changeset.id}\"/>. lon not a number", @response.body
+
     # test that the upload is rejected when we have a tag which is too long
     content("<osm><node lat='#{lat}' lon='#{lon}' changeset='#{changeset.id}'><tag k='foo' v='#{'x'*256}'/></node></osm>")
     put :create

diff --git a/test/functional/notes_controller_test.rb b/test/functional/notes_controller_test.rb
index 0b52d035e682aeb7980a19222b0b82d93ca0976c..a4720eb06c5437754fa7e29ae76fedbce8cde9be 100644 (file)
--- a/test/functional/notes_controller_test.rb
+++ b/test/functional/notes_controller_test.rb
@@ -197,6 +197,20 @@ class NotesControllerTest < ActionController::TestCase
       end
     end
     assert_response :bad_request
+
+    assert_no_difference('Note.count') do
+      assert_no_difference('NoteComment.count') do
+        post :create, {:lat => 'abc', :lon => -1.0, :text => "This is a comment"}
+      end
+    end
+    assert_response :bad_request
+
+    assert_no_difference('Note.count') do
+      assert_no_difference('NoteComment.count') do
+        post :create, {:lat => -1.0, :lon => 'abc', :text => "This is a comment"}
+      end
+    end
+    assert_response :bad_request
   end
 
   def test_comment_success