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

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

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
 

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

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