diff --git a/app/controllers/user_api_keys_controller.rb b/app/controllers/user_api_keys_controller.rb index 175623751..3875e982b 100644 --- a/app/controllers/user_api_keys_controller.rb +++ b/app/controllers/user_api_keys_controller.rb @@ -90,7 +90,16 @@ class UserApiKeysController < ApplicationController end def revoke - find_key.update_columns(revoked_at: Time.zone.now) + revoke_key = find_key + if current_key = request.env['HTTP_USER_API_KEY'] + request_key = UserApiKey.find_by(key: current_key) + if request_key && request_key.id != revoke_key.id && !request_key.write + raise Discourse::InvalidAccess + end + end + + revoke_key.update_columns(revoked_at: Time.zone.now) + render json: success_json end diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb index a4224d823..a840b2770 100644 --- a/lib/auth/default_current_user_provider.rb +++ b/lib/auth/default_current_user_provider.rb @@ -176,10 +176,16 @@ class Auth::DefaultCurrentUserProvider protected + WHITELISTED_WRITE_PATHS ||= [/^\/message-bus\/.*\/poll/, /^\/user-api-key\/revoke$/] def lookup_user_api_user(user_api_key) if api_key = UserApiKey.where(key: user_api_key, revoked_at: nil).includes(:user).first - if !api_key.write && (@env["REQUEST_METHOD"] != "GET" && @env["PATH_INFO"] !~ /^\/message-bus\/.*\/poll/) - raise Discourse::InvalidAccess + unless api_key.write + if @env["REQUEST_METHOD"] != "GET" + path = @env["PATH_INFO"] + unless WHITELISTED_WRITE_PATHS.any?{|whitelisted| path =~ whitelisted} + raise Discourse::InvalidAccess + end + end end api_key.user diff --git a/spec/controllers/user_api_keys_controller_spec.rb b/spec/controllers/user_api_keys_controller_spec.rb index 9884eff07..9d0324c9a 100644 --- a/spec/controllers/user_api_keys_controller_spec.rb +++ b/spec/controllers/user_api_keys_controller_spec.rb @@ -94,6 +94,27 @@ TXT end + it "will not allow readonly api keys to revoke others" do + key1 = Fabricate(:readonly_user_api_key) + key2 = Fabricate(:readonly_user_api_key) + + request.env['HTTP_USER_API_KEY'] = key1.key + post :revoke, id: key2.id + + expect(response.status).to eq(403) + end + + it "will allow readonly api keys to revoke self" do + key = Fabricate(:readonly_user_api_key) + request.env['HTTP_USER_API_KEY'] = key.key + post :revoke, id: key.id + + expect(response.status).to eq(200) + + key.reload + expect(key.revoked_at).not_to eq(nil) + end + it "will not return p access if not yet configured" do SiteSetting.min_trust_level_for_user_api_key = 0 SiteSetting.allowed_user_api_auth_redirects = args[:auth_redirect] diff --git a/spec/fabricators/user_api_key_fabricator.rb b/spec/fabricators/user_api_key_fabricator.rb new file mode 100644 index 000000000..5658a4981 --- /dev/null +++ b/spec/fabricators/user_api_key_fabricator.rb @@ -0,0 +1,9 @@ +Fabricator(:readonly_user_api_key, from: :user_api_key) do + user + read true + write false + push false + client_id { SecureRandom.hex } + key { SecureRandom.hex } + application_name 'some app' +end