Merge branch...

Merge branch '32995-issue-contents-dynamically-replaced-with-stale-version-after-saving-or-refreshing-relative-external_url-only' into 'master'

Fix incorrect ETag cache key when relative instance URL is used

Closes #32995

See merge request !11964

changelogs/unreleased/32995-issue-contents-dynamically-replaced-with-stale-version-after-saving-or-refreshing-relative-external_url-only.yml

+---
+title: Fix incorrect ETag cache key when relative instance URL is used
+merge_request: 11964
+author:

lib/gitlab/etag_caching/middleware.rb

@@ -6,12 +6,13 @@ module Gitlab
       end
 
       def call(env)
-        route = Gitlab::EtagCaching::Router.match(env)
+        request = Rack::Request.new(env)
+        route = Gitlab::EtagCaching::Router.match(request)
         return @app.call(env) unless route
 
         track_event(:etag_caching_middleware_used, route)
 
-        etag, cached_value_present = get_etag(env)
+        etag, cached_value_present = get_etag(request)
         if_none_match = env['HTTP_IF_NONE_MATCH']
 
         if if_none_match == etag
@@ -27,8 +28,8 @@ module Gitlab
 
       private
 
-      def get_etag(env)
-        cache_key = env['PATH_INFO']
+      def get_etag(request)
+        cache_key = request.path
         store = Gitlab::EtagCaching::Store.new
         current_value = store.get(cache_key)
         cached_value_present = current_value.present?

lib/gitlab/etag_caching/router.rb

@@ -53,8 +53,8 @@ module Gitlab
         )
       ].freeze
 
-      def self.match(env)
-        ROUTES.find { |route| route.regexp.match(env['PATH_INFO']) }
+      def self.match(request)
+        ROUTES.find { |route| route.regexp.match(request.path_info) }
       end
     end
   end

spec/lib/gitlab/etag_caching/middleware_spec.rb

@@ -164,6 +164,25 @@ describe Gitlab::EtagCaching::Middleware do
     end
   end
 
+  context 'when GitLab instance is using a relative URL' do
+    before do
+      mock_app_response
+    end
+
+    it 'uses full path as cache key' do
+      env = {
+        'PATH_INFO' => enabled_path,
+        'SCRIPT_NAME' => '/relative-gitlab'
+      }
+
+      expect_any_instance_of(Gitlab::EtagCaching::Store)
+        .to receive(:get).with("/relative-gitlab#{enabled_path}")
+        .and_return(nil)
+
+      middleware.call(env)
+    end
+  end
+
   def mock_app_response
     allow(app).to receive(:call).and_return([app_status_code, {}, ['body']])
   end

spec/lib/gitlab/etag_caching/router_spec.rb

@@ -2,115 +2,115 @@ require 'spec_helper'
 
 describe Gitlab::EtagCaching::Router do
   it 'matches issue notes endpoint' do
-    env = build_env(
+    request = build_request(
       '/my-group/and-subgroup/here-comes-the-project/noteable/issue/1/notes'
     )
 
-    result = described_class.match(env)
+    result = described_class.match(request)
 
     expect(result).to be_present
     expect(result.name).to eq 'issue_notes'
   end
 
   it 'matches issue title endpoint' do
-    env = build_env(
+    request = build_request(
       '/my-group/my-project/issues/123/realtime_changes'
     )
 
-    result = described_class.match(env)
+    result = described_class.match(request)
 
     expect(result).to be_present
     expect(result.name).to eq 'issue_title'
   end
 
   it 'matches project pipelines endpoint' do
-    env = build_env(
+    request = build_request(
       '/my-group/my-project/pipelines.json'
     )
 
-    result = described_class.match(env)
+    result = described_class.match(request)
 
     expect(result).to be_present
     expect(result.name).to eq 'project_pipelines'
   end
 
   it 'matches commit pipelines endpoint' do
-    env = build_env(
+    request = build_request(
       '/my-group/my-project/commit/aa8260d253a53f73f6c26c734c72fdd600f6e6d4/pipelines.json'
     )
 
-    result = described_class.match(env)
+    result = described_class.match(request)
 
     expect(result).to be_present
     expect(result.name).to eq 'commit_pipelines'
   end
 
   it 'matches new merge request pipelines endpoint' do
-    env = build_env(
+    request = build_request(
       '/my-group/my-project/merge_requests/new.json'
     )
 
-    result = described_class.match(env)
+    result = described_class.match(request)
 
     expect(result).to be_present
     expect(result.name).to eq 'new_merge_request_pipelines'
   end
 
   it 'matches merge request pipelines endpoint' do
-    env = build_env(
+    request = build_request(
       '/my-group/my-project/merge_requests/234/pipelines.json'
     )
 
-    result = described_class.match(env)
+    result = described_class.match(request)
 
     expect(result).to be_present
     expect(result.name).to eq 'merge_request_pipelines'
   end
 
   it 'matches build endpoint' do
-    env = build_env(
+    request = build_request(
       '/my-group/my-project/builds/234.json'
     )
 
-    result = described_class.match(env)
+    result = described_class.match(request)
 
     expect(result).to be_present
     expect(result.name).to eq 'project_build'
   end
 
   it 'does not match blob with confusing name' do
-    env = build_env(
+    request = build_request(
       '/my-group/my-project/blob/master/pipelines.json'
     )
 
-    result = described_class.match(env)
+    result = described_class.match(request)
 
     expect(result).to be_blank
   end
 
   it 'matches the environments path' do
-    env = build_env(
+    request = build_request(
       '/my-group/my-project/environments.json'
     )
 
-    result = described_class.match(env)
+    result = described_class.match(request)
     expect(result).to be_present
 
     expect(result.name).to eq 'environments'
   end
 
   it 'matches pipeline#show endpoint' do
-    env = build_env(
+    request = build_request(
       '/my-group/my-project/pipelines/2.json'
     )
 
-    result = described_class.match(env)
+    result = described_class.match(request)
 
     expect(result).to be_present
     expect(result.name).to eq 'project_pipeline'
   end
 
-  def build_env(path)
-    { 'PATH_INFO' => path }
+  def build_request(path)
+    double(path_info: path)
   end
 end