CVE-2023-28846: Uncontrolled Resource Consumption (CWE-400)
value: Unpoly is a JavaScript framework for server-side web applications. There is a possible Denial of Service (DoS) vulnerability in the `unpoly-rails` gem that implements the Unpoly server protocol for Rails applications. This issues affects Rails applications that operate as an upstream of a load balancer's that uses passive health checks. The `unpoly-rails` gem echoes the request URL as an `X-Up-Location` response header. By making a request with exceedingly long URLs (paths or query string), an attacker can cause unpoly-rails to write a exceedingly large response header.  If the response header is too large to be parsed by a load balancer downstream of the Rails application, it may cause the load balancer to remove the upstream from a load balancing group. This causes that application instance to become unavailable until a configured timeout is reached or until an active healthcheck succeeds. This issue has been fixed and released as version 2.7.2.2 which is available via RubyGems and GitHub. Users unable to upgrade may: Configure your load balancer to use active health checks, e.g. by periodically requesting a route with a known response that indicates healthiness; Configure your load balancer so the maximum size of response headers is at least twice the maximum size of a URL; or instead of changing your server configuration you may also configure your Rails application to delete redundant `X-Up-Location` headers set by unpoly-rails.

CWE description: The product does not properly control the allocation and maintenance of a limited resource, thereby enabling an actor to influence the amount of resources consumed, eventually leading to the exhaustion of available resources.
CVSS v3 base score: 7.5
Published: 2023-03-30T20:15Z
Repository: https://github.com/unpoly/unpoly-rails
Language: Ruby
Files changed: lib/unpoly/rails/request_echo_headers.rb, spec/unpoly/rails/controller_spec.rb
Commit date: 2023-03-30 13:00:06 +0200
Commit message: Only echo the request URL as X-Up-Location if it contains query params prefixed with "_up"

Diff: diff --git a/lib/unpoly/rails/request_echo_headers.rb b/lib/unpoly/rails/request_echo_headers.rb
index 44958c2..b682469 100644
--- a/lib/unpoly/rails/request_echo_headers.rb
+++ b/lib/unpoly/rails/request_echo_headers.rb
@@ -16,13 +16,17 @@ module Unpoly
           base.before_filter :set_up_request_echo_headers
         end
       end
 
       private
-      
+
       def set_up_request_echo_headers
-        response.headers['X-Up-Location'] = up.request_url_without_up_params
+        request_url_without_up_params = up.request_url_without_up_params
+        unless request_url_without_up_params == request.original_url
+          response.headers['X-Up-Location'] = up.request_url_without_up_params
+        end
+
         response.headers['X-Up-Method'] = request.method
       end
 
       ::ActionController::Base.send(:include, self)
 
diff --git a/spec/unpoly/rails/controller_spec.rb b/spec/unpoly/rails/controller_spec.rb
index 47dfa46..9e9591e 100644
--- a/spec/unpoly/rails/controller_spec.rb
+++ b/spec/unpoly/rails/controller_spec.rb
@@ -993,35 +993,32 @@ describe Unpoly::Rails::Controller, type: :request do
 
   end
 
   describe 'echoing of the request location' do
 
-    it 'echoes the current path in an X-Up-Location response header' do
+    it 'does not echo the current path in an X-Up-Location response header to prevent the user-controlled request URL from exceeding the maximum response header size' do
       get '/binding_test/text'
-      expect(response.headers['X-Up-Location']).to end_with('/binding_test/text')
+      expect(response.headers['X-Up-Location']).to be_nil
     end
 
-    it 'echoes the current path after a redirect' do
-      get '/binding_test/redirect1'
-      expect(response).to be_redirect
-      follow_redirect!
-      expect(response.headers['X-Up-Location']).to end_with('/binding_test/redirect2')
-    end
+    describe 'when the request URL contains query params prefixed with "_up-"' do
 
-    it 'echoes the current path with query params' do
-      get '/binding_test/text?foo=bar'
-      expect(response.headers['X-Up-Location']).to end_with('/binding_test/text?foo=bar')
-    end
+      it 'removes params prefixed with "_up-"' do
+        get '/binding_test/text?_up_1&_up_2=y'
+        expect(response.headers['X-Up-Location']).to end_with('/binding_test/text')
+      end
 
-    it 'removes _up-* params' do
-      get '/binding_test/text?_up_1=x&foo=bar&_up_2=y'
-      expect(response.headers['X-Up-Location']).to end_with('/binding_test/text?foo=bar')
-    end
+      it 'keeps params not prefixed with "_up-"' do
+        get '/binding_test/text?_up_1=x&foo=bar&_up_2=y'
+        expect(response.headers['X-Up-Location']).to end_with('/binding_test/text?foo=bar')
+      end
+
+      it 'does not mangle array params (BUGFIX)' do
+        get '/binding_test/text?_up_1=x&foo%5B%5D=bar&foo%5B%5D=qux&_up_location=up_location'
+        expect(response.headers['X-Up-Location']).to end_with('/binding_test/text?foo%5B%5D=bar&foo%5B%5D=qux')
+      end
 
-    it 'does not mangle array params (BUGFIX)' do
-      get '/binding_test/text?foo%5B%5D=bar&foo%5B%5D=qux&_up_location=up_location'
-      expect(response.headers['X-Up-Location']).to end_with('/binding_test/text?foo%5B%5D=bar&foo%5B%5D=qux')
     end
 
   end
 
   describe 'echoing of the request method' do

Vulnerable code:       
        response.headers['X-Up-Location'] = up.request_url_without_up_params
    it 'echoes the current path in an X-Up-Location response header' do
      expect(response.headers['X-Up-Location']).to end_with('/binding_test/text')
    it 'echoes the current path after a redirect' do
      get '/binding_test/redirect1'
      expect(response).to be_redirect
      follow_redirect!
      expect(response.headers['X-Up-Location']).to end_with('/binding_test/redirect2')
    end
    it 'echoes the current path with query params' do
      get '/binding_test/text?foo=bar'
      expect(response.headers['X-Up-Location']).to end_with('/binding_test/text?foo=bar')
    end
    it 'removes _up-* params' do
      get '/binding_test/text?_up_1=x&foo=bar&_up_2=y'
      expect(response.headers['X-Up-Location']).to end_with('/binding_test/text?foo=bar')
    end
    it 'does not mangle array params (BUGFIX)' do
      get '/binding_test/text?foo%5B%5D=bar&foo%5B%5D=qux&_up_location=up_location'
      expect(response.headers['X-Up-Location']).to end_with('/binding_test/text?foo%5B%5D=bar&foo%5B%5D=qux')
Fixed code: 
        request_url_without_up_params = up.request_url_without_up_params
        unless request_url_without_up_params == request.original_url
          response.headers['X-Up-Location'] = up.request_url_without_up_params
        end

    it 'does not echo the current path in an X-Up-Location response header to prevent the user-controlled request URL from exceeding the maximum response header size' do
      expect(response.headers['X-Up-Location']).to be_nil
    describe 'when the request URL contains query params prefixed with "_up-"' do
      it 'removes params prefixed with "_up-"' do
        get '/binding_test/text?_up_1&_up_2=y'
        expect(response.headers['X-Up-Location']).to end_with('/binding_test/text')
      end
      it 'keeps params not prefixed with "_up-"' do
        get '/binding_test/text?_up_1=x&foo=bar&_up_2=y'
        expect(response.headers['X-Up-Location']).to end_with('/binding_test/text?foo=bar')
      end

      it 'does not mangle array params (BUGFIX)' do
        get '/binding_test/text?_up_1=x&foo%5B%5D=bar&foo%5B%5D=qux&_up_location=up_location'
        expect(response.headers['X-Up-Location']).to end_with('/binding_test/text?foo%5B%5D=bar&foo%5B%5D=qux')
      end