Skip to content

Commit

Permalink
Fix the Redirect warnings from Brakeman
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
tomhughes committed Jul 22, 2020
1 parent 8642820 commit d4130bc
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 35 deletions.
17 changes: 16 additions & 1 deletion app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ def require_cookies
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"
Expand Down Expand Up @@ -372,4 +372,19 @@ def get_auth_data

# 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
4 changes: 2 additions & 2 deletions app/controllers/friendships_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ def make_friend
end

if params[:referer]
redirect_to params[:referer]
redirect_to safe_referer(params[:referer])
else
redirect_to user_path
end
Expand All @@ -50,7 +50,7 @@ def remove_friend
end

if params[:referer]
redirect_to params[:referer]
redirect_to safe_referer(params[:referer])
else
redirect_to user_path
end
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/messages_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ def destroy
flash[:notice] = t ".destroyed"

if params[:referer]
redirect_to params[:referer]
redirect_to safe_referer(params[:referer])
else
redirect_to :action => :inbox
end
Expand Down
41 changes: 17 additions & 24 deletions app/controllers/site_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,39 +17,32 @@ def index

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
new_params[:mlat] = lat
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
Expand Down Expand Up @@ -166,6 +159,6 @@ def redirect_map_params
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
20 changes: 13 additions & 7 deletions app/controllers/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ def save
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
Expand All @@ -63,7 +63,7 @@ def save
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
Expand Down Expand Up @@ -198,7 +198,11 @@ def reset_password

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]
Expand Down Expand Up @@ -231,7 +235,9 @@ def create
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"

Expand All @@ -258,7 +264,7 @@ def create
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]
Expand All @@ -278,7 +284,7 @@ def logout
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
Expand All @@ -300,7 +306,7 @@ def confirm
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]
Expand Down

0 comments on commit d4130bc

Please sign in to comment.