Skip to content
Permalink
Browse files Browse the repository at this point in the history
[fix][cms] open redirect vulnerability on member login (#3646)
* [fix] open redirect vulnerability

* [add] reproducible spec
  • Loading branch information
sunny4381 committed Jul 1, 2020
1 parent 04675a5 commit 040a02c
Show file tree
Hide file tree
Showing 5 changed files with 163 additions and 8 deletions.
5 changes: 0 additions & 5 deletions app/controllers/concerns/member/login_filter.rb
Expand Up @@ -68,11 +68,6 @@ def member_login_path
"#{member_login_node.url}login.html"
end

def redirect_url
return "/" unless member_login_node
member_login_node.redirect_url || "/"
end

def translate_redirect_option(opts)
case opts[:redirect]
when true
Expand Down
27 changes: 25 additions & 2 deletions app/controllers/member/agents/nodes/login_controller.rb
Expand Up @@ -21,13 +21,36 @@ def set_member_and_redirect(member)
remote_addr: remote_addr,
user_agent: request.user_agent)

ref = URI::decode(params[:ref] || flash[:ref] || "")
ref = redirect_url if ref.blank?
ref = @cur_node.make_trusted_full_url(params[:ref] || flash[:ref])
ref = @cur_node.redirect_full_url if ref.blank?
ref = @cur_site.full_url if ref.blank?
flash.discard(:ref)

redirect_to ref
end

def make_url_if_trusted(ref)
return if ref.blank?

ref = URI::decode(ref)

site_url = URI.parse(@cur_site.full_url)
full_url = URI.join(site_url, ref) rescue nil
return if full_url.blank?
return unless %w(http https).include?(full_url.scheme)

# normalize full url
full_url.fragment = nil
full_url.query = nil

# trusted?
return if full_url.scheme != site_url.scheme
return if full_url.host != site_url.host
return if full_url.port != site_url.port

full_url.to_s
end

public

def login
Expand Down
39 changes: 39 additions & 0 deletions app/models/member/node.rb
Expand Up @@ -22,6 +22,45 @@ class Login
include History::Addon::Backup

default_scope ->{ where(route: "member/login") }

def redirect_full_url
return if redirect_url.blank?

ret = make_full_url(redirect_url)
return if ret.blank?
return unless trusted?(ret)

ret.to_s
end

def make_trusted_full_url(ref)
return if ref.blank?

full_url = make_full_url(URI::decode(ref))
return if full_url.blank?

# normalize full url
full_url.fragment = nil
full_url.query = nil

# trusted?
return unless trusted?(full_url)

full_url.to_s
end

private

def make_full_url(path)
site_root_url = URI.parse(site.full_root_url)
URI.join(site_root_url, path) rescue nil
end

def trusted?(full_url)
return false if full_url.blank?

%w(http https).include?(full_url.scheme) && full_url.to_s.start_with?(site.full_url)
end
end

class Mypage
Expand Down
27 changes: 26 additions & 1 deletion spec/features/members/agents/nodes/login_spec.rb
@@ -1,6 +1,6 @@
require 'spec_helper'

describe 'members/agents/nodes/login', type: :feature, dbscope: :example do
describe 'members/agents/nodes/login', type: :feature, dbscope: :example, js: true do
describe "ss-2724" do
let(:root_site) { cms_site }
let(:site1) { create(:cms_site_subdir, domains: root_site.domains) }
Expand Down Expand Up @@ -46,4 +46,29 @@
expect(page).to have_css(".form-login")
end
end

describe "open redirect" do
let(:site) { cms_site }
let(:layout) { create_cms_layout(cur_site: site) }
let(:index_page) { create :cms_page, cur_site: site, layout: layout, filename: "index.html", html: "you've been logged in" }
let!(:login_node) do
create(
:member_node_login, cur_site: site, layout: layout, redirect_url: index_page.url, form_auth: "enabled"
)
end
let!(:member) { create :cms_member, cur_site: site }

it do
visit "#{login_node.full_url}login.html" + "?" + { ref: URI.encode("https://www.ss-proj.org/") }.to_query

within ".form-login" do
fill_in "item[email]", with: member.email
fill_in "item[password]", with: member.in_password

click_on I18n.t("ss.login")
end

expect(page).to have_css(".body", text: "you've been logged in")
end
end
end
73 changes: 73 additions & 0 deletions spec/models/member/node/login_spec.rb
@@ -0,0 +1,73 @@
require 'spec_helper'

describe Member::Node::Login, type: :model, dbscope: :example do
describe "#redirect_full_url" do
let(:site) { cms_site }

context "when redirect_url is missing" do
let(:node) { create :member_node_login, cur_site: site, redirect_url: nil }
it do
expect(node.redirect_full_url).to be_blank
end
end

context "when redirect_url is full path" do
let(:node) { create :member_node_login, cur_site: site, redirect_url: "/#{unique_id}" }
it do
expect(node.redirect_full_url).to eq URI.join(node.site.full_url, node.redirect_url).to_s
end
end

context "when redirect_url is full url" do
let(:node) { create :member_node_login, cur_site: site, redirect_url: "#{site.full_url}#{unique_id}" }
it do
expect(node.redirect_full_url).to eq URI.join(node.site.full_url, node.redirect_url).to_s
end
end

context "when redirect_url is other site url" do
let(:node) { create :member_node_login, cur_site: site, redirect_url: unique_url }
it do
expect(node.redirect_full_url).to be_blank
end
end
end

describe "#make_trusted_full_url" do
let(:site) { cms_site }
let(:node) { create :member_node_login, cur_site: site }

context "when nil is given" do
it do
expect(node.make_trusted_full_url(nil)).to be_blank
end
end

context "when empty string is given" do
it do
expect(node.make_trusted_full_url("")).to be_blank
end
end

context "when full path is given" do
let(:path) { "/#{unique_id}" }
it do
expect(node.make_trusted_full_url(path)).to eq URI.join(node.site.full_url, path[1..-1]).to_s
end
end

context "when full url is given" do
let(:url) { "#{site.full_url}#{unique_id}" }
it do
expect(node.make_trusted_full_url(url)).to eq url
end
end

context "when other site url is given" do
let(:url) { unique_url }
it do
expect(node.make_trusted_full_url(url)).to be_blank
end
end
end
end

0 comments on commit 040a02c

Please sign in to comment.