From d4130bcac83015564f0326457f2bb67d1b5ebab6 Mon Sep 17 00:00:00 2001 From: Tom Hughes Date: Wed, 22 Jul 2020 19:13:19 +0100 Subject: [PATCH] Fix the Redirect warnings from Brakeman Unfortunately I've had to leave the check disabed as Brakeman can't see inside the safe_referer method so doesn't realise that it is cleaning the referer. --- app/controllers/application_controller.rb | 17 +++++++++- app/controllers/friendships_controller.rb | 4 +-- app/controllers/messages_controller.rb | 2 +- app/controllers/site_controller.rb | 41 ++++++++++------------- app/controllers/users_controller.rb | 20 +++++++---- 5 files changed, 49 insertions(+), 35 deletions(-) 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] -- 2.43.2