Harden API read parameter parsing

Author: jnunemaker

lib/flipper/api/action.rb

@@ -163,6 +163,42 @@ def existing_feature_names
         @existing_feature_names ||= flipper.features.map(&:key).to_set
       end
 
+      # Private: Returns request parameters, or an empty hash when Rack cannot
+      # parse client-controlled parameter syntax.
+      def safe_params
+        @safe_params ||= params
+      rescue *parameter_parser_errors
+        @params_parse_failed = true
+        @safe_params = {}
+      end
+
+      def params_parse_failed?
+        safe_params
+        @params_parse_failed == true
+      end
+
+      # Private: Returns a valid String parameter, ignoring other shapes and
+      # invalid encodings.
+      def string_param(name)
+        value = safe_params[name]
+        value if valid_param_string?(value)
+      end
+
+      def valid_param_string?(value)
+        value.is_a?(String) && value.valid_encoding?
+      end
+
+      def parameter_parser_errors
+        parsers = [Rack::Utils]
+        parsers << Rack.const_get(:QueryParser, false) if Rack.const_defined?(:QueryParser, false)
+        error_names = [:InvalidParameterError, :ParameterTypeError, :ParamsTooDeepError, :QueryLimitError]
+        parsers.each_with_object([]) do |parser, errors|
+          error_names.each do |name|
+            errors << parser.const_get(name, false) if parser.const_defined?(name, false)
+          end
+        end.uniq
+      end
+
       # Private: Returns the request method converted to an action method.
       # Converts head to get.
       def request_method_name

lib/flipper/api/v1/actions/actors.rb

@@ -9,7 +9,7 @@ class Actors < Api::Action
           route %r{\A/actors/(?<flipper_id>.*)/?\Z}
 
           def get
-            keys = params['keys']
+            keys = string_param('keys')
             features = if keys
                          names = keys.split(',')
                          if names.empty?

lib/flipper/api/v1/actions/feature.rb

@@ -12,7 +12,7 @@ class Feature < Api::Action
 
           def get
             return json_error_response(:feature_not_found) unless feature_exists?(feature_name)
-            exclude_gates = params['exclude_gates']&.downcase == "true"
+            exclude_gates = string_param('exclude_gates')&.downcase == "true"
             feature = Decorators::Feature.new(flipper[feature_name])
             json_response(feature.as_json(exclude_gates: exclude_gates))
           end

lib/flipper/api/v1/actions/features.rb

@@ -10,8 +10,8 @@ class Features < Api::Action
 
           def get
             names = requested_feature_names
-            exclude_gates = params['exclude_gates']&.downcase == "true"
-            exclude_gate_names = params['exclude_gate_names']&.downcase == "true"
+            exclude_gates = query_string_param('exclude_gates')&.downcase == "true"
+            exclude_gate_names = query_string_param('exclude_gate_names')&.downcase == "true"
 
             features = if names
               if names.empty?
@@ -51,10 +51,13 @@ def post
           private
 
           def requested_feature_names
-            keys = params['keys']
-            return keys.map(&:to_s) if keys.is_a?(Array)
+            keys = safe_params['keys']
+            return nil unless keys
+            return nil unless valid_feature_keys?(keys)
+            return keys if keys.is_a?(Array)
 
             raw_keys = raw_query_values("keys")
+            return nil unless raw_keys
             return decoded_feature_names if raw_keys.empty?
 
             raw_keys.flat_map do |keys|
@@ -68,19 +71,45 @@ def requested_feature_names
           end
 
           def decoded_feature_names
-            keys = params['keys']
-            return nil unless keys
-
-            Array(keys).flat_map { |key| key.to_s.split(',') }
+            safe_params['keys'].split(',')
           end
 
           def raw_query_values(name)
-            request.query_string.to_s.split(/[&;]/).each_with_object([]) do |part, values|
+            values = []
+            request.query_string.to_s.split(/[&;]/).each do |part|
               next if part.empty?
 
               key, value = part.split('=', 2)
-              values << value.to_s if Rack::Utils.unescape(key) == name
+              decoded_key = decoded_query_component(key)
+              return nil unless decoded_key
+              next unless decoded_key == name
+              return nil unless decoded_query_component(value.to_s)
+
+              values << value.to_s
             end
+            values
+          end
+
+          def query_string_param(name)
+            value = string_param(name)
+            return value if value || safe_params.key?(name)
+            return nil if params_parse_failed?
+
+            raw_values = raw_query_values(name)
+            Rack::Utils.unescape(raw_values.last) if raw_values && !raw_values.empty?
+          end
+
+          def valid_feature_keys?(keys)
+            return valid_param_string?(keys) unless keys.is_a?(Array)
+
+            keys.all? { |key| valid_param_string?(key) }
+          end
+
+          def decoded_query_component(value)
+            decoded = Rack::Utils.unescape(value)
+            decoded if decoded.valid_encoding?
+          rescue ArgumentError
+            nil
           end
         end
       end

spec/flipper/api/action_spec.rb

@@ -107,4 +107,14 @@ def delete
       end
     end
   end
+
+  describe 'safe parameters' do
+    it 'does not classify unrelated application errors as parameter errors' do
+      request = double('Request', request_method: 'GET')
+      allow(request).to receive(:params).and_raise(ArgumentError, 'application failure')
+      action = action_subclass.new(flipper, request)
+
+      expect { action.send(:safe_params) }.to raise_error(ArgumentError, 'application failure')
+    end
+  end
 end

spec/flipper/api/v1/actions/actors_spec.rb

@@ -9,6 +9,27 @@
       flipper[:my_feature_3].enable_actor(actor)
     end
 
+    malformed_queries = {
+      'array-shaped keys' => 'keys[]=my_feature_1',
+      'hash-shaped keys' => 'keys[a]=my_feature_1',
+      'a bare percent escape' => 'keys=%',
+      'an invalid percent escape' => 'keys=%GG',
+      'invalid UTF-8' => 'keys=%FF',
+      'conflicting parameter shapes' => 'a=1&a[]=2',
+      'excessive nesting' => "a#{'[a]' * 150}=1",
+    }
+
+    malformed_queries.each do |description, query|
+      it "ignores #{description}" do
+        expected_response = raw_api_get("/actors/#{actor.flipper_id}", '')
+
+        response = raw_api_get("/actors/#{actor.flipper_id}", query)
+
+        expect(response.first).to eq(200)
+        expect(response).to eq(expected_response)
+      end
+    end
+
     context 'when no feature is specified' do
       before do
         get "/actors/#{actor.flipper_id}"

spec/flipper/api/v1/actions/feature_spec.rb

@@ -4,6 +4,40 @@
   let(:gate) { feature.gate(:boolean) }
 
   describe 'get' do
+    malformed_queries = {
+      'array-shaped exclude_gates' => 'exclude_gates[]=true',
+      'hash-shaped exclude_gates' => 'exclude_gates[a]=true',
+      'a bare percent escape' => 'exclude_gates=%',
+      'an invalid percent escape' => 'exclude_gates=%GG',
+      'invalid UTF-8' => 'exclude_gates=%FF',
+      'conflicting parameter shapes' => 'a=1&a[]=2',
+      'excessive nesting' => "a#{'[a]' * 150}=1",
+    }
+
+    malformed_queries.each do |description, query|
+      it "ignores #{description}" do
+        flipper[:my_feature].enable
+        expected_response = raw_api_get('/features/my_feature', '')
+
+        response = raw_api_get('/features/my_feature', query)
+
+        expect(response.first).to eq(200)
+        expect(response).to eq(expected_response)
+      end
+    end
+
+    it 'responds without gates when instructed by param' do
+      flipper[:my_feature].enable
+
+      status, _, body = raw_api_get('/features/my_feature', 'exclude_gates=true')
+
+      expect(status).to eq(200)
+      expect(body).to eq(
+        'key' => 'my_feature',
+        'state' => 'on'
+      )
+    end
+
     context 'enabled feature' do
       before do
         flipper[:my_feature].enable

spec/flipper/api/v1/actions/features_spec.rb

@@ -4,6 +4,100 @@
   let(:admin) { double 'Fake Fliper Thing', flipper_id: 10 }
 
   describe 'get' do
+    malformed_queries = {
+      'array-shaped exclude_gate_names' => 'exclude_gate_names[]=true',
+      'hash-shaped exclude_gates' => 'exclude_gates[a]=true',
+      'hash-shaped keys' => 'keys[a]=my_feature',
+      'nested array-shaped keys' => 'keys[][]=my_feature',
+      'a bare percent escape' => 'keys=%',
+      'a truncated percent escape' => 'keys=%2',
+      'an invalid percent escape in a value' => 'keys=%GG',
+      'an invalid percent escape in a key' => '%GG=value',
+      'invalid UTF-8 in a key' => '%FF=value&keys=my_feature',
+      'invalid UTF-8' => 'keys=%FF',
+      'conflicting parameter shapes' => 'a=1&a[]=2',
+      'excessive nesting' => "a#{'[a]' * 150}=1",
+      'excessive parameter count' => (1..5000).map { |index| "ignored#{index}=1" }.join('&'),
+    }
+
+    malformed_queries.each do |description, query|
+      it "fails open for #{description}" do
+        flipper[:my_feature].enable
+        expected_response = raw_api_get('/features', '')
+
+        response = raw_api_get('/features', query)
+
+        expect(response.first).to eq(200)
+        expect(response).to eq(expected_response)
+      end
+    end
+
+    it 'fully fails open when malformed syntax is combined with a valid option' do
+      flipper[:my_feature].enable
+      expected_response = raw_api_get('/features', '')
+
+      response = raw_api_get('/features', 'a=1&a[]=2&exclude_gate_names=true')
+
+      expect(response.first).to eq(200)
+      expect(response).to eq(expected_response)
+    end
+
+    separator_queries = [
+      '&exclude_gate_names=true',
+      '&&exclude_gate_names=true&&',
+      ';exclude_gate_names=true',
+      ';;exclude_gate_names=true;;',
+      'exclude_gate_names=true&&;;',
+    ]
+
+    separator_queries.each do |query|
+      it "preserves optional parameter semantics with empty query segments in #{query.inspect}" do
+        flipper[:my_feature].enable
+        expected_response = raw_api_get('/features', 'exclude_gate_names=true')
+
+        response = raw_api_get('/features', query)
+
+        expect(response.first).to eq(200)
+        expect(response).to eq(expected_response)
+      end
+    end
+
+    it 'fails open when decoding a raw query component raises ArgumentError' do
+      request = instance_double(Rack::Request, query_string: 'keys=%')
+      action = described_class.new(flipper, request)
+      allow(action).to receive(:safe_params).and_return('keys' => 'my_feature')
+      allow(Rack::Utils).to receive(:unescape).with('keys').and_return('keys')
+      allow(Rack::Utils).to receive(:unescape).with('%').and_raise(ArgumentError)
+
+      expect(action.send(:requested_feature_names)).to be_nil
+    end
+
+    it 'uses parsed keys when no raw keys parameter is present' do
+      request = instance_double(Rack::Request, query_string: 'other=value')
+      action = described_class.new(flipper, request)
+      allow(action).to receive(:safe_params).and_return('keys' => 'my_feature,other_feature')
+
+      expect(action.send(:requested_feature_names)).to eq(%w[my_feature other_feature])
+    end
+
+    {
+      'keys=my_feature&keys=other_feature' => %w[my_feature other_feature],
+      'keys=&keys=my_feature' => %w[my_feature],
+      '=&keys=my_feature&=value' => %w[my_feature],
+      'keys=' => [],
+    }.each do |query, expected_keys|
+      it "preserves duplicate and blank parameter semantics for #{query.inspect}" do
+        flipper[:my_feature].enable
+        flipper[:other_feature].disable
+
+        status, _, body = raw_api_get('/features', query)
+        keys = body.fetch('features').map { |feature| feature.fetch('key') }.sort
+
+        expect(status).to eq(200)
+        expect(keys).to eq(expected_keys.sort)
+      end
+    end
+
     context 'with flipper features' do
       before do
         flipper[:my_feature].enable
@@ -80,6 +174,20 @@
         expect(json_response).to eq(expected_response)
       end
 
+      it 'responds without gates when instructed by param' do
+        get '/features', 'exclude_gates' => 'true'
+
+        expect(last_response.status).to eq(200)
+        expect(json_response).to eq(
+          'features' => [
+            {
+              'key' => 'my_feature',
+              'state' => 'on',
+            },
+          ]
+        )
+      end
+
       it 'ignores a leading empty query segment' do
         get '/features?exclude_gate_names=true'
         expected_response = json_response

spec/support/spec_helpers.rb

@@ -37,6 +37,15 @@ def json_response
     JSON.parse(body)
   end
 
+  def raw_api_get(path, query_string)
+    env = Rack::MockRequest.env_for(path, 'QUERY_STRING' => query_string)
+    status, headers, body = app.call(env)
+    response_body = body.each_with_object(+'') { |part, buffer| buffer << part }
+    [status, headers, JSON.parse(response_body)]
+  ensure
+    body.close if body.respond_to?(:close)
+  end
+
   def api_error_code_reference_url
     'https://flippercloud.io/docs/api#error-code-reference'
   end