From 798b4bb6049fe59ef3e67c738cd730d3912cccf3 Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Thu, 26 Jan 2023 13:26:29 +0000
Subject: [PATCH] FIX: Ensure anon-cached values are never returned for API
requests (#20021)
Under some situations, we would inadvertently return a public (unauthenticated) result to an authenticated API request. This commit adds the `Api-Key` header to our anonymous cache bypass logic.
---
lib/middleware/anonymous_cache.rb | 3 +++
spec/lib/middleware/anonymous_cache_spec.rb | 17 ++++++++++++++++-
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/lib/middleware/anonymous_cache.rb b/lib/middleware/anonymous_cache.rb
index a62f4e53cfc..7f1fbec9a4d 100644
--- a/lib/middleware/anonymous_cache.rb
+++ b/lib/middleware/anonymous_cache.rb
@@ -66,6 +66,7 @@ module Middleware
!@request.path.ends_with?("srv/status") &&
@request[Auth::DefaultCurrentUserProvider::API_KEY].nil? &&
@env[Auth::DefaultCurrentUserProvider::USER_API_KEY].nil? &&
+ @env[Auth::DefaultCurrentUserProvider::HEADER_API_KEY].nil? &&
CrawlerDetection.is_blocked_crawler?(@env[USER_AGENT])
end
@@ -184,11 +185,13 @@ module Middleware
request = Rack::Request.new(@env)
request.cookies["_bypass_cache"].nil? && (request.path != "/srv/status") &&
request[Auth::DefaultCurrentUserProvider::API_KEY].nil? &&
+ @env[Auth::DefaultCurrentUserProvider::HEADER_API_KEY].nil? &&
@env[Auth::DefaultCurrentUserProvider::USER_API_KEY].nil?
end
def force_anonymous!
@env[Auth::DefaultCurrentUserProvider::USER_API_KEY] = nil
+ @env[Auth::DefaultCurrentUserProvider::HEADER_API_KEY] = nil
@env["HTTP_COOKIE"] = nil
@env["HTTP_DISCOURSE_LOGGED_IN"] = nil
@env["rack.request.cookie.hash"] = {}
diff --git a/spec/lib/middleware/anonymous_cache_spec.rb b/spec/lib/middleware/anonymous_cache_spec.rb
index 05a496c8cb1..f5c35713257 100644
--- a/spec/lib/middleware/anonymous_cache_spec.rb
+++ b/spec/lib/middleware/anonymous_cache_spec.rb
@@ -4,7 +4,7 @@ RSpec.describe Middleware::AnonymousCache do
let(:middleware) { Middleware::AnonymousCache.new(lambda { |_| [200, {}, []] }) }
def env(opts = {})
- create_request_env(path: "http://test.com/path?bla=1").merge(opts)
+ create_request_env(path: opts.delete(:path) || "http://test.com/path?bla=1").merge(opts)
end
describe Middleware::AnonymousCache::Helper do
@@ -38,6 +38,18 @@ RSpec.describe Middleware::AnonymousCache do
it "is false for srv/status routes" do
expect(new_helper("PATH_INFO" => "/srv/status").cacheable?).to eq(false)
end
+
+ it "is false for API requests using header" do
+ expect(new_helper("HTTP_API_KEY" => "abcde").cacheable?).to eq(false)
+ end
+
+ it "is false for API requests using parameter" do
+ expect(new_helper(path: "/path?api_key=abc").cacheable?).to eq(false)
+ end
+
+ it "is false for User API requests using header" do
+ expect(new_helper("HTTP_USER_API_KEY" => "abcde").cacheable?).to eq(false)
+ end
end
describe "per theme cache" do
@@ -322,6 +334,9 @@ RSpec.describe Middleware::AnonymousCache do
"QUERY_STRING" => "api_key=#{api_key.key}&api_username=system",
}
expect(@status).to eq(200)
+
+ get "/latest", headers: { "HTTP_API_KEY" => api_key.key, "HTTP_API_USERNAME" => "system" }
+ expect(@status).to eq(200)
end
it "applies blocked_crawler_user_agents correctly" do