Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: any remaining open redirects #61

Merged
merged 4 commits into from
Mar 24, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion app/controllers/auth/coauth_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,23 @@ def login_path

protected

def redirect_continue(path)
# checking for `.attacker.com` OR `//<external_domain>`
# no internal paths use `//`
if !path.start_with?("/") || path.include?("//")
authority = current_authority
uri = Addressable::URI.parse(path)

path = if uri.domain == authority.domain
"#{uri.request_uri}#{uri.fragment ? "##{uri.fragment}" : nil}"
else
yield
end
end

redirect_to path
end

def new_session(user)
@current_user = user
value = {
Expand Down Expand Up @@ -50,7 +67,7 @@ def store_social(uid, provider)
end

def set_continue(path)
if path.include?("://")
if !path.start_with?("/") || path.include?("//")
uri = Addressable::URI.parse(path)
path = "#{uri.request_uri}#{uri.fragment ? "##{uri.fragment}" : nil}"
end
Expand Down
28 changes: 9 additions & 19 deletions app/controllers/auth/sessions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ def signin

# If there is a path we are using an inline login form
if path
redirect_to path
redirect_continue(path) { "/" }
else
head :accepted
end
Expand Down Expand Up @@ -67,7 +67,7 @@ def create
user.save

Authentication.create_with_omniauth(authority.id, auth, user.id)
redirect_to path
redirect_continue(path) { success_path }
instance_exec user, auth["provider"], auth, &Authentication.after_login_block

# new auth and new user
Expand Down Expand Up @@ -104,7 +104,7 @@ def create

# redirect the user to the page they were trying to access and
# run any custom post-login actions
redirect_to path
redirect_continue(path) { success_path }
instance_exec user, auth["provider"], auth, &Authentication.after_login_block
else
info = "User creation failed with #{auth.inspect}"
Expand Down Expand Up @@ -138,7 +138,7 @@ def create
user.assign_attributes(args)
user.save
new_session(user)
redirect_to path
redirect_continue(path) { success_path }
instance_exec user, auth["provider"], auth, &Authentication.after_login_block
rescue => e
logger.error "Error with user account. Possibly due to a database failure:\nAuth model: #{auth_model.inspect}\n#{e.inspect}"
Expand All @@ -151,22 +151,12 @@ def create
def destroy
remove_session

# do we want to redirect externally?
path = params.permit(:continue)[:continue] || "/"

if !path.start_with?("/") || path.include?("//")
authority = current_authority
uri = Addressable::URI.parse(path)

if uri.domain == authority.domain
path = "#{uri.request_uri}#{uri.fragment ? "##{uri.fragment}" : nil}"
else
path = authority.logout_url
path = URI.decode_www_form_component(path.split("continue=", 2)[-1]) if path.include?("continue=")
end
# Only whitelisted URLs can redirect externally
redirect_continue(params.permit(:continue)[:continue] || "/") do
uri = authority.logout_url
uri = URI.decode_www_form_component(uri.split("continue=", 2)[-1]) if uri.include?("continue=")
uri
end

redirect_to path
end

protected
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/auth/signups_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ def create
new_session(user)

# we're in a pop-up so redirect to a page that can communicate to the main page
redirect_to path
redirect_continue(path) { success_path }
else
# Email address taken (all other validation can be checked on the client)
head :conflict
Expand Down