From: Ian Dees <ian.dees@gmail.com>
Date: Thu, 20 Jun 2013 19:11:59 +0000 (+0000)
Subject: Guard against non-numeric lat and lons in nodes and notes
X-Git-Tag: live~4915
X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/9d2fed811fd37577bf5cce8e8fe03d2097852428?hp=378869b77dd488b10eabda7f213550b0b815923a;ds=sidebyside

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 1b28cae6e..9c6eb9457 100644
--- 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 2f528076e..775f1fd3b 100644
--- 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 6903dd60b..32667d9c9 100644
--- 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 0b52d035e..a4720eb06 100644
--- 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