From 36e230e71a98525f00ca90ca5eafb84259d8ac03 Mon Sep 17 00:00:00 2001 From: Ian Dees Date: Thu, 20 Jun 2013 19:11:59 +0000 Subject: [PATCH 1/2] Guard against non-numeric lat/lons in nodes and notes. --- app/controllers/notes_controller.rb | 12 ++++++++++-- app/models/node.rb | 12 ++++++++++-- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb index f59e9974e1..7c1c3f2573 100644 --- a/app/controllers/notes_controller.rb +++ b/app/controllers/notes_controller.rb @@ -59,8 +59,16 @@ def create 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 2f528076eb..775f1fd3b2 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -83,8 +83,16 @@ def self.from_xml_node(pt, create=false) 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 From b6cb662c435eb164430e2e5e0c6a4ef06a93d887 Mon Sep 17 00:00:00 2001 From: Ian Dees Date: Thu, 20 Jun 2013 22:45:09 +0000 Subject: [PATCH 2/2] Add tests. --- test/functional/node_controller_test.rb | 16 ++++++++++++++++ test/functional/notes_controller_test.rb | 14 ++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/test/functional/node_controller_test.rb b/test/functional/node_controller_test.rb index 6903dd60b4..32667d9c90 100644 --- a/test/functional/node_controller_test.rb +++ b/test/functional/node_controller_test.rb @@ -122,6 +122,22 @@ def test_create_invalid_xml assert_response :bad_request, "node upload did not return bad_request status" assert_equal "Cannot parse valid node from xml string . lon missing", @response.body + # test that the upload is rejected when lat is non-numeric + # create a minimal xml file + content("") + 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 . lat not a number", @response.body + + # test that the upload is rejected when lon is non-numeric + # create a minimal xml file + content("") + 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 . lon not a number", @response.body + # test that the upload is rejected when we have a tag which is too long content("") put :create diff --git a/test/functional/notes_controller_test.rb b/test/functional/notes_controller_test.rb index 0b52d035e6..a4720eb06c 100644 --- a/test/functional/notes_controller_test.rb +++ b/test/functional/notes_controller_test.rb @@ -197,6 +197,20 @@ def test_create_fail 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