From: Tom Hughes Date: Wed, 22 Jul 2020 18:29:08 +0000 (+0100) Subject: Merge remote-tracking branch 'upstream/pull/2727' X-Git-Tag: live~3096 X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/ca92fe3359ce2a751763b1bb0bfe824a89b20853?hp=56fdf73b659e7722a9a4860b3708cbb1ab885897 Merge remote-tracking branch 'upstream/pull/2727' --- diff --git a/.travis.yml b/.travis.yml index ce0a625ec..6acf89e48 100644 --- a/.travis.yml +++ b/.travis.yml @@ -31,6 +31,7 @@ script: - bundle exec rubocop -f fuubar - bundle exec rake eslint - bundle exec erblint . + - bundle exec brakeman -q - bundle exec rake db:structure:dump - sed -e "/idle_in_transaction_session_timeout/d" -e 's/ IMMUTABLE / /' -e "/^--/d" db/structure.sql > db/structure.actual - diff -uw db/structure.expected db/structure.actual diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 439e2e95f..568566c17 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -41,6 +41,14 @@ You can view test coverage statistics by browsing the `coverage` directory. The tests are automatically run on Pull Requests and other commits with the results shown on [Travis CI](https://travis-ci.org/openstreetmap/openstreetmap-website). +## Static Analysis + +We also perform static analysis of our code. You can run the analysis yourself with: + +``` +bundle exec brakeman -q +``` + ## Comments Sometimes it's not apparent from the code itself what it does, or, diff --git a/Gemfile b/Gemfile index 1cd0edca7..540f61634 100644 --- a/Gemfile +++ b/Gemfile @@ -138,6 +138,7 @@ end # Gems needed for running tests group :test do + gem "brakeman" gem "capybara", ">= 2.15" gem "coveralls", :require => false gem "erb_lint", :require => false diff --git a/Gemfile.lock b/Gemfile.lock index cc8dcaefb..0843b96a0 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -74,8 +74,8 @@ GEM autoprefixer-rails (9.8.5) execjs aws-eventstream (1.1.0) - aws-partitions (1.340.0) - aws-sdk-core (3.103.0) + aws-partitions (1.343.0) + aws-sdk-core (3.104.1) aws-eventstream (~> 1, >= 1.0.2) aws-partitions (~> 1, >= 1.239.0) aws-sigv4 (~> 1.1) @@ -83,8 +83,8 @@ GEM aws-sdk-kms (1.36.0) aws-sdk-core (~> 3, >= 3.99.0) aws-sigv4 (~> 1.1) - aws-sdk-s3 (1.74.0) - aws-sdk-core (~> 3, >= 3.102.1) + aws-sdk-s3 (1.75.0) + aws-sdk-core (~> 3, >= 3.104.1) aws-sdk-kms (~> 1) aws-sigv4 (~> 1.1) aws-sigv4 (1.2.1) @@ -110,6 +110,7 @@ GEM autoprefixer-rails (>= 9.1.0) popper_js (>= 1.14.3, < 2) sassc-rails (>= 2.0.0) + brakeman (4.8.2) browser (4.2.0) builder (3.2.4) bzip2-ffi (1.0.0) @@ -484,6 +485,7 @@ DEPENDENCIES binding_of_caller bootsnap (>= 1.4.2) bootstrap (~> 4.5.0) + brakeman browser bzip2-ffi cancancan diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index a2211ea69..616223a06 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -65,7 +65,7 @@ class ApplicationController < ActionController::Base if request.cookies["_osm_session"].to_s == "" if params[:cookie_test].nil? session[:cookie_test] = true - redirect_to params.to_unsafe_h.merge(:cookie_test => "true") + redirect_to params.to_unsafe_h.merge(:only_path => true, :cookie_test => "true") false else flash.now[:warning] = t "application.require_cookies.cookies_needed" @@ -372,4 +372,19 @@ class ApplicationController < ActionController::Base # override to stop oauth plugin sending errors def invalid_oauth_response; end + + # clean any referer parameter + def safe_referer(referer) + referer = URI.parse(referer) + + if referer.scheme == "http" || referer.scheme == "https" + referer.scheme = nil + referer.host = nil + referer.port = nil + elsif referer.scheme || referer.host || referer.port + referer = nil + end + + referer.to_s + end end diff --git a/app/controllers/friendships_controller.rb b/app/controllers/friendships_controller.rb index a983bec75..75e53368d 100644 --- a/app/controllers/friendships_controller.rb +++ b/app/controllers/friendships_controller.rb @@ -27,7 +27,7 @@ class FriendshipsController < ApplicationController end if params[:referer] - redirect_to params[:referer] + redirect_to safe_referer(params[:referer]) else redirect_to user_path end @@ -50,7 +50,7 @@ class FriendshipsController < ApplicationController end if params[:referer] - redirect_to params[:referer] + redirect_to safe_referer(params[:referer]) else redirect_to user_path end diff --git a/app/controllers/messages_controller.rb b/app/controllers/messages_controller.rb index a55690951..4d054b5f2 100644 --- a/app/controllers/messages_controller.rb +++ b/app/controllers/messages_controller.rb @@ -120,7 +120,7 @@ class MessagesController < ApplicationController flash[:notice] = t ".destroyed" if params[:referer] - redirect_to params[:referer] + redirect_to safe_referer(params[:referer]) else redirect_to :action => :inbox end diff --git a/app/controllers/site_controller.rb b/app/controllers/site_controller.rb index 52fea6133..1cb848ea6 100644 --- a/app/controllers/site_controller.rb +++ b/app/controllers/site_controller.rb @@ -17,7 +17,7 @@ class SiteController < ApplicationController def permalink lon, lat, zoom = ShortLink.decode(params[:code]) - new_params = params.except(:code, :lon, :lat, :zoom, :layers, :node, :way, :relation, :changeset) + new_params = params.except(:host, :controller, :action, :code, :lon, :lat, :zoom, :layers, :node, :way, :relation, :changeset) if new_params.key? :m new_params.delete :m @@ -25,31 +25,24 @@ class SiteController < ApplicationController new_params[:mlon] = lon end - if params.key? :node - new_params[:controller] = "browse" - new_params[:action] = "node" - new_params[:id] = params[:node] - elsif params.key? :way - new_params[:controller] = "browse" - new_params[:action] = "way" - new_params[:id] = params[:way] - elsif params.key? :relation - new_params[:controller] = "browse" - new_params[:action] = "relation" - new_params[:id] = params[:relation] - elsif params.key? :changeset - new_params[:controller] = "browse" - new_params[:action] = "changeset" - new_params[:id] = params[:changeset] - else - new_params[:controller] = "site" - new_params[:action] = "index" - end - new_params[:anchor] = "map=#{zoom}/#{lat}/#{lon}" new_params[:anchor] += "&layers=#{params[:layers]}" if params.key? :layers - redirect_to new_params.to_unsafe_h + options = new_params.to_unsafe_h.to_options + + path = if params.key? :node + node_path(params[:node], options) + elsif params.key? :way + way_path(params[:way], options) + elsif params.key? :relation + relation_path(params[:relation], options) + elsif params.key? :changeset + changeset_path(params[:changeset], options) + else + root_url(options) + end + + redirect_to path end def key @@ -166,6 +159,6 @@ class SiteController < ApplicationController anchor << "layers=N" end - redirect_to params.to_unsafe_h.merge(:anchor => anchor.join("&")) if anchor.present? + redirect_to params.to_unsafe_h.merge(:only_path => true, :anchor => anchor.join("&")) if anchor.present? end end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 676f8d8f9..aa9a4f02a 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -43,7 +43,7 @@ class UsersController < ApplicationController flash[:notice] = t("users.new.terms declined", :url => t("users.new.terms declined url")).html_safe if current_user.save if params[:referer] - redirect_to params[:referer] + redirect_to safe_referer(params[:referer]) else redirect_to :action => :account, :display_name => current_user.display_name end @@ -63,7 +63,7 @@ class UsersController < ApplicationController end if params[:referer] - redirect_to params[:referer] + redirect_to safe_referer(params[:referer]) else redirect_to :action => :account, :display_name => current_user.display_name end @@ -198,7 +198,11 @@ class UsersController < ApplicationController def new @title = t "users.new.title" - @referer = params[:referer] || session[:referer] + @referer = if params[:referer] + safe_referer(params[:referer]) + else + session[:referer] + end append_content_security_policy_directives( :form_action => %w[accounts.google.com *.facebook.com login.live.com github.com meta.wikimedia.org] @@ -231,7 +235,9 @@ class UsersController < ApplicationController self.current_user = User.new(user_params) if check_signup_allowed(current_user.email) - session[:referer] = params[:referer] + session[:referer] = safe_referer(params[:referer]) if params[:referer] + + Rails.logger.info "create: #{session[:referer]}" current_user.status = "pending" @@ -258,7 +264,7 @@ class UsersController < ApplicationController end def login - session[:referer] = params[:referer] if params[:referer] + session[:referer] = safe_referer(params[:referer]) if params[:referer] if params[:username].present? && params[:password].present? session[:remember_me] ||= params[:remember_me] @@ -278,7 +284,7 @@ class UsersController < ApplicationController session.delete(:user) session_expires_automatically if params[:referer] - redirect_to params[:referer] + redirect_to safe_referer(params[:referer]) else redirect_to :controller => "site", :action => "index" end @@ -300,7 +306,7 @@ class UsersController < ApplicationController user.email_valid = true flash[:notice] = gravatar_status_message(user) if gravatar_enable(user) user.save! - referer = token.referer + referer = safe_referer(token.referer) if token.referer token.destroy if session[:token] diff --git a/app/views/browse/_common_details.html.erb b/app/views/browse/_common_details.html.erb index 1100fe97b..683cf6d70 100644 --- a/app/views/browse/_common_details.html.erb +++ b/app/views/browse/_common_details.html.erb @@ -24,7 +24,7 @@ <% if @type == "node" and common_details.visible? %>
<%= t "browse.location" %> - <%= link_to(tag.span(number_with_delimiter(common_details.lat), :class => "latitude") + ", " + tag.span(number_with_delimiter(common_details.lon), :class => "longitude"), :controller => "site", :action => "index", :anchor => "map=18/#{common_details.lat}/#{common_details.lon}") %> + <%= link_to(tag.span(number_with_delimiter(common_details.lat), :class => "latitude") + ", " + tag.span(number_with_delimiter(common_details.lon), :class => "longitude"), root_path(:anchor => "map=18/#{common_details.lat}/#{common_details.lon}")) %>
<% end %> diff --git a/app/views/browse/note.html.erb b/app/views/browse/note.html.erb index 74d1223c7..c72730a76 100644 --- a/app/views/browse/note.html.erb +++ b/app/views/browse/note.html.erb @@ -12,10 +12,16 @@
-

<%= note_event("opened", @note.created_at, @note.author) %>

- <% if @note.status == "closed" %> -

<%= note_event(@note.status, @note.closed_at, @note.all_comments.last.author) %>

- <% end %> +
<% if @note_comments.find { |comment| comment.author.nil? } -%> diff --git a/config/brakeman.yml b/config/brakeman.yml new file mode 100644 index 000000000..f8fab871e --- /dev/null +++ b/config/brakeman.yml @@ -0,0 +1,9 @@ +:skip_checks: +# These checks are skipped, but should be considered TODO +- CheckCrossSiteScripting +- CheckExecute +- CheckFileAccess +- CheckRedirect +- CheckRender +- CheckSendFile +- CheckSQL diff --git a/test/controllers/api/ways_controller_test.rb b/test/controllers/api/ways_controller_test.rb index fa0c47792..a6f8fae82 100644 --- a/test/controllers/api/ways_controller_test.rb +++ b/test/controllers/api/ways_controller_test.rb @@ -64,31 +64,31 @@ module Api ## # check the "full" mode def test_full - Way.all.each do |way| - get way_full_path(way) - - # full call should say "gone" for non-visible ways... - unless way.visible - assert_response :gone - next - end - - # otherwise it should say success - assert_response :success - - # Check the way is correctly returned - assert_select "osm way[id='#{way.id}'][version='#{way.version}'][visible='#{way.visible}']", 1 - - # check that each node in the way appears once in the output as a - # reference and as the node element. - way.nodes.each do |n| - count = (way.nodes - (way.nodes - [n])).length - assert_select "osm way nd[ref='#{n.id}']", count - assert_select "osm node[id='#{n.id}'][version='#{n.version}'][lat='#{format('%.7f', n.lat)}'][lon='#{format('%.7f', n.lon)}']", 1 - end + way = create(:way_with_nodes, :nodes_count => 3) + + get way_full_path(way) + + assert_response :success + + # Check the way is correctly returned + assert_select "osm way[id='#{way.id}'][version='1'][visible='true']", 1 + + # check that each node in the way appears once in the output as a + # reference and as the node element. + way.nodes.each do |n| + assert_select "osm way nd[ref='#{n.id}']", 1 + assert_select "osm node[id='#{n.id}'][version='1'][lat='#{format('%.7f', n.lat)}'][lon='#{format('%.7f', n.lon)}']", 1 end end + def test_full_deleted + way = create(:way, :deleted) + + get way_full_path(way) + + assert_response :gone + end + ## # test fetching multiple ways def test_index diff --git a/test/controllers/changesets_controller_test.rb b/test/controllers/changesets_controller_test.rb index d1fc5c65c..a5ce2aa62 100644 --- a/test/controllers/changesets_controller_test.rb +++ b/test/controllers/changesets_controller_test.rb @@ -33,6 +33,8 @@ class ChangesetsControllerTest < ActionDispatch::IntegrationTest ## # This should display the last 20 changesets closed def test_index + changesets = create_list(:changeset, 30, :num_changes => 1) + get history_path(:format => "html") assert_response :success assert_template "history" @@ -43,12 +45,14 @@ class ChangesetsControllerTest < ActionDispatch::IntegrationTest assert_response :success assert_template "index" - check_index_result(Changeset.all) + check_index_result(changesets.last(20)) end ## # This should display the last 20 changesets closed def test_index_xhr + changesets = create_list(:changeset, 30, :num_changes => 1) + get history_path(:format => "html"), :xhr => true assert_response :success assert_template "history" @@ -59,12 +63,22 @@ class ChangesetsControllerTest < ActionDispatch::IntegrationTest assert_response :success assert_template "index" - check_index_result(Changeset.all) + check_index_result(changesets.last(20)) end ## # This should display the last 20 changesets closed in a specific area def test_index_bbox + changesets = create_list(:changeset, 10, :num_changes => 1, :min_lat => 50000000, :max_lat => 50000001, :min_lon => 50000000, :max_lon => 50000001) + other_changesets = create_list(:changeset, 10, :num_changes => 1, :min_lat => 0, :max_lat => 1, :min_lon => 0, :max_lon => 1) + + # First check they all show up without a bbox parameter + get history_path(:format => "html", :list => "1"), :xhr => true + assert_response :success + assert_template "index" + check_index_result(changesets + other_changesets) + + # Then check with bbox parameter get history_path(:format => "html", :bbox => "4.5,4.5,5.5,5.5") assert_response :success assert_template "history" @@ -75,7 +89,7 @@ class ChangesetsControllerTest < ActionDispatch::IntegrationTest assert_response :success assert_template "index" - check_index_result(Changeset.where("min_lon < 55000000 and max_lon > 45000000 and min_lat < 55000000 and max_lat > 45000000")) + check_index_result(changesets) end ## @@ -111,7 +125,7 @@ class ChangesetsControllerTest < ActionDispatch::IntegrationTest assert_response :success assert_template "index" - check_index_result(Changeset.none) + check_index_result([]) end ## @@ -131,7 +145,8 @@ class ChangesetsControllerTest < ActionDispatch::IntegrationTest def test_index_friends private_user = create(:user, :data_public => true) friendship = create(:friendship, :befriender => private_user) - create(:changeset, :user => friendship.befriendee) + changeset = create(:changeset, :user => friendship.befriendee, :num_changes => 1) + _changeset2 = create(:changeset, :user => create(:user), :num_changes => 1) get friend_changesets_path assert_response :redirect @@ -147,7 +162,7 @@ class ChangesetsControllerTest < ActionDispatch::IntegrationTest assert_response :success assert_template "index" - check_index_result(Changeset.where(:user => private_user.friends.identifiable)) + check_index_result([changeset]) end ## @@ -155,7 +170,9 @@ class ChangesetsControllerTest < ActionDispatch::IntegrationTest def test_index_nearby private_user = create(:user, :data_public => false, :home_lat => 51.1, :home_lon => 1.0) user = create(:user, :home_lat => 51.0, :home_lon => 1.0) - create(:changeset, :user => user) + far_away_user = create(:user, :home_lat => 51.0, :home_lon => 130) + changeset = create(:changeset, :user => user, :num_changes => 1) + _changeset2 = create(:changeset, :user => far_away_user, :num_changes => 1) get nearby_changesets_path assert_response :redirect @@ -171,23 +188,26 @@ class ChangesetsControllerTest < ActionDispatch::IntegrationTest assert_response :success assert_template "index" - check_index_result(Changeset.where(:user => user.nearby)) + check_index_result([changeset]) end ## # Check that we can't request later pages of the changesets index def test_index_max_id - get history_path(:format => "html", :max_id => 4), :xhr => true + changeset = create(:changeset, :num_changes => 1) + _changeset2 = create(:changeset, :num_changes => 1) + + get history_path(:format => "html", :max_id => changeset.id), :xhr => true assert_response :success assert_template "history" assert_template :layout => "xhr" assert_select "h2", :text => "Changesets", :count => 1 - get history_path(:format => "html", :list => "1", :max_id => 4), :xhr => true + get history_path(:format => "html", :list => "1", :max_id => changeset.id), :xhr => true assert_response :success assert_template "index" - check_index_result(Changeset.where("id <= 4")) + check_index_result([changeset]) end ## @@ -275,11 +295,6 @@ class ChangesetsControllerTest < ActionDispatch::IntegrationTest ## # check the result of a index def check_index_result(changesets) - changesets = changesets.where("num_changes > 0") - .order(:created_at => :desc) - .limit(20) - assert changesets.size <= 20 - assert_select "ol.changesets", :count => [changesets.size, 1].min do assert_select "li", :count => changesets.size diff --git a/vendor/assets/iD/iD.js b/vendor/assets/iD/iD.js index 9e36a035e..6cc200808 100644 --- a/vendor/assets/iD/iD.js +++ b/vendor/assets/iD/iD.js @@ -54819,6 +54819,8 @@ var loc = extent.center(); context.map().centerEase(loc); + // we could enter the mode multiple times, so reset follow for next time + _follow = false; } @@ -89867,7 +89869,7 @@ _base = context.graph(); _coalesceChanges = false; - loadActivePresets(); + loadActivePresets(true); return entityEditor .modified(false); @@ -89881,7 +89883,7 @@ }; - function loadActivePresets() { + function loadActivePresets(isForNewSelection) { var graph = context.graph(); @@ -89903,11 +89905,14 @@ return _mainPresetIndex.item(pID); }); - // A "weak" preset doesn't set any tags. (e.g. "Address") - var weakPreset = _activePresets.length === 1 && - Object.keys(_activePresets[0].addTags || {}).length === 0; - // Don't replace a weak preset with a fallback preset (e.g. "Point") - if (weakPreset && matches.length === 1 && matches[0].isFallback()) { return; } + if (!isForNewSelection) { + // A "weak" preset doesn't set any tags. (e.g. "Address") + var weakPreset = _activePresets.length === 1 && + !_activePresets[0].isFallback() && + Object.keys(_activePresets[0].addTags || {}).length === 0; + // Don't replace a weak preset with a fallback preset (e.g. "Point") + if (weakPreset && matches.length === 1 && matches[0].isFallback()) { return; } + } entityEditor.presets(matches); } @@ -117386,10 +117391,14 @@ // some targets have default click events we don't want to override var isOkayTarget = event.composedPath().some(function(node) { - // clicking