From 7ac394f51f147a7dd2db1467ef42bff0d41ee6fa Mon Sep 17 00:00:00 2001 From: Blake Erickson Date: Tue, 12 Mar 2019 17:16:42 -0600 Subject: [PATCH] FIX: prevent mixed api auth headers & query params When using the api and you provide an http header based api key any other auth based information (username, external_id, or user_id) passed in as query params will not be used and vice versa. Followup to f03b293e6a5f12f12ba2a61ab2bc2cfb8a7f1a63 --- lib/auth/default_current_user_provider.rb | 12 ++-- .../default_current_user_provider_spec.rb | 56 +++++++++++++++++++ 2 files changed, 64 insertions(+), 4 deletions(-) diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb index 8404ee6694b..18e23848752 100644 --- a/lib/auth/default_current_user_provider.rb +++ b/lib/auth/default_current_user_provider.rb @@ -45,7 +45,7 @@ class Auth::DefaultCurrentUserProvider request = @request user_api_key = @env[USER_API_KEY] - api_key = @env.blank? ? nil : request[API_KEY] || @env[HEADER_API_KEY] + api_key = @env.blank? ? nil : @env[HEADER_API_KEY] || request[API_KEY] auth_token = request.cookies[TOKEN_COOKIE] unless user_api_key || api_key @@ -284,7 +284,7 @@ class Auth::DefaultCurrentUserProvider def lookup_api_user(api_key_value, request) if api_key = ApiKey.where(key: api_key_value).includes(:user).first - api_username = request[API_USERNAME] || @env[HEADER_API_USERNAME] + api_username = header_api_key? ? @env[HEADER_API_USERNAME] : request[API_USERNAME] if api_key.allowed_ips.present? && !api_key.allowed_ips.any? { |ip| ip.include?(request.ip) } Rails.logger.warn("[Unauthorized API Access] username: #{api_username}, IP address: #{request.ip}") @@ -295,9 +295,9 @@ class Auth::DefaultCurrentUserProvider api_key.user if !api_username || (api_key.user.username_lower == api_username.downcase) elsif api_username User.find_by(username_lower: api_username.downcase) - elsif user_id = request["api_user_id"] || @env[HEADER_API_USER_ID] + elsif user_id = header_api_key? ? @env[HEADER_API_USER_ID] : request["api_user_id"] User.find_by(id: user_id.to_i) - elsif external_id = request["api_user_external_id"] || @env[HEADER_API_USER_EXTERNAL_ID] + elsif external_id = header_api_key? ? @env[HEADER_API_USER_EXTERNAL_ID] : request["api_user_external_id"] SingleSignOnRecord.find_by(external_id: external_id.to_s).try(:user) end end @@ -305,6 +305,10 @@ class Auth::DefaultCurrentUserProvider private + def header_api_key? + !!@env[HEADER_API_KEY] + end + def rate_limit_admin_api_requests(api_key) return if Rails.env == "profile" diff --git a/spec/components/auth/default_current_user_provider_spec.rb b/spec/components/auth/default_current_user_provider_spec.rb index ed582333d82..5d188b66e3c 100644 --- a/spec/components/auth/default_current_user_provider_spec.rb +++ b/spec/components/auth/default_current_user_provider_spec.rb @@ -92,6 +92,15 @@ describe Auth::DefaultCurrentUserProvider do expect(provider("/?api_key=hello&api_username=#{user.username.downcase}").current_user.id).to eq(user.id) end + it "raises for a mismatched api_key param and header username" do + user = Fabricate(:user) + ApiKey.create!(key: "hello", created_by_id: -1) + params = { "HTTP_API_USERNAME" => user.username.downcase } + expect { + provider("/?api_key=hello", params).current_user + }.to raise_error(Discourse::InvalidAccess) + end + it "finds a user for a correct system api key with external id" do user = Fabricate(:user) ApiKey.create!(key: "hello", created_by_id: -1) @@ -99,12 +108,31 @@ describe Auth::DefaultCurrentUserProvider do expect(provider("/?api_key=hello&api_user_external_id=abc").current_user.id).to eq(user.id) end + it "raises for a mismatched api_key param and header external id" do + user = Fabricate(:user) + ApiKey.create!(key: "hello", created_by_id: -1) + SingleSignOnRecord.create(user_id: user.id, external_id: "abc", last_payload: '') + params = { "HTTP_API_USER_EXTERNAL_ID" => "abc" } + expect { + provider("/?api_key=hello", params).current_user + }.to raise_error(Discourse::InvalidAccess) + end + it "finds a user for a correct system api key with id" do user = Fabricate(:user) ApiKey.create!(key: "hello", created_by_id: -1) expect(provider("/?api_key=hello&api_user_id=#{user.id}").current_user.id).to eq(user.id) end + it "raises for a mismatched api_key param and header user id" do + user = Fabricate(:user) + ApiKey.create!(key: "hello", created_by_id: -1) + params = { "HTTP_API_USER_ID" => user.id } + expect { + provider("/?api_key=hello", params).current_user + }.to raise_error(Discourse::InvalidAccess) + end + context "rate limiting" do before do RateLimiter.enable @@ -243,6 +271,15 @@ describe Auth::DefaultCurrentUserProvider do expect(provider("/", params).current_user.id).to eq(user.id) end + it "raises for a mismatched api_key header and param username" do + user = Fabricate(:user) + ApiKey.create!(key: "hello", created_by_id: -1) + params = { "HTTP_API_KEY" => "hello" } + expect { + provider("/?api_username=#{user.username.downcase}", params).current_user + }.to raise_error(Discourse::InvalidAccess) + end + it "finds a user for a correct system api key with external id" do user = Fabricate(:user) ApiKey.create!(key: "hello", created_by_id: -1) @@ -251,6 +288,16 @@ describe Auth::DefaultCurrentUserProvider do expect(provider("/", params).current_user.id).to eq(user.id) end + it "raises for a mismatched api_key header and param external id" do + user = Fabricate(:user) + ApiKey.create!(key: "hello", created_by_id: -1) + SingleSignOnRecord.create(user_id: user.id, external_id: "abc", last_payload: '') + params = { "HTTP_API_KEY" => "hello" } + expect { + provider("/?api_user_external_id=abc", params).current_user + }.to raise_error(Discourse::InvalidAccess) + end + it "finds a user for a correct system api key with id" do user = Fabricate(:user) ApiKey.create!(key: "hello", created_by_id: -1) @@ -258,6 +305,15 @@ describe Auth::DefaultCurrentUserProvider do expect(provider("/", params).current_user.id).to eq(user.id) end + it "raises for a mismatched api_key header and param user id" do + user = Fabricate(:user) + ApiKey.create!(key: "hello", created_by_id: -1) + params = { "HTTP_API_KEY" => "hello" } + expect { + provider("/?api_user_id=#{user.id}", params).current_user + }.to raise_error(Discourse::InvalidAccess) + end + context "rate limiting" do before do RateLimiter.enable