From: Tom Hughes Date: Wed, 22 Jul 2020 18:29:05 +0000 (+0100) Subject: Merge remote-tracking branch 'upstream/pull/2725' X-Git-Tag: live~3445 X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/a792b74bea261834791e9f75c64161436d37053f?hp=47d11763fbe02ce1aa9470e33aabb151bef68297 Merge remote-tracking branch 'upstream/pull/2725' --- 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/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