FIX: Don't leak unhashed user API keys to redis (#14682)

FIX: Don’t leak unhashed user API keys to redis (#14682)

User API keys (not the same thing as admin API keys) are currently leaked to redis when rate limits are applied to them since redis is the backend for rate limits in Discourse and the API keys are included in the redis keys that are used to track usage of user API keys in the last 24 hours.

This commit stops the leak by using a SHA-256 representation of the user API key instead of the key itself to form the redis key.

We don’t need to manually delete the existing redis keys that contain unhashed user API keys because they’re not long-lived and will be automatically deleted within 48 hours after this commit is deployed to your Discourse instance.

diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb
index 8c16392..571bad9 100644
--- a/lib/auth/default_current_user_provider.rb
+++ b/lib/auth/default_current_user_provider.rb
@@ -137,8 +137,9 @@ class Auth::DefaultCurrentUserProvider
     # user api key handling
     if user_api_key
 
-      limiter_min = RateLimiter.new(nil, "user_api_min_#{user_api_key}", GlobalSetting.max_user_api_reqs_per_minute, 60)
-      limiter_day = RateLimiter.new(nil, "user_api_day_#{user_api_key}", GlobalSetting.max_user_api_reqs_per_day, 86400)
+      hashed_user_api_key = ApiKey.hash_key(user_api_key)
+      limiter_min = RateLimiter.new(nil, "user_api_min_#{hashed_user_api_key}", GlobalSetting.max_user_api_reqs_per_minute, 60)
+      limiter_day = RateLimiter.new(nil, "user_api_day_#{hashed_user_api_key}", GlobalSetting.max_user_api_reqs_per_day, 86400)
 
       unless limiter_day.can_perform?
         limiter_day.performed!
diff --git a/spec/components/auth/default_current_user_provider_spec.rb b/spec/components/auth/default_current_user_provider_spec.rb
index b8da06f..502e289 100644
--- a/spec/components/auth/default_current_user_provider_spec.rb
+++ b/spec/components/auth/default_current_user_provider_spec.rb
@@ -621,8 +621,8 @@ describe Auth::DefaultCurrentUserProvider do
       end
 
       it "rate limits api usage" do
-        limiter1 = RateLimiter.new(nil, "user_api_day_#{api_key.key}", 10, 60)
-        limiter2 = RateLimiter.new(nil, "user_api_min_#{api_key.key}", 10, 60)
+        limiter1 = RateLimiter.new(nil, "user_api_day_#{ApiKey.hash_key(api_key.key)}", 10, 60)
+        limiter2 = RateLimiter.new(nil, "user_api_min_#{ApiKey.hash_key(api_key.key)}", 10, 60)
         limiter1.clear!
         limiter2.clear!
 

GitHub sha: 70fa67a9e11b1e8d6390f0e86805b8c3ec23a55e

This commit appears in #14682 which was approved by danielwaterworth. It was merged by OsamaSayegh.