Verified Commit e15e634d authored by Max Woolf's avatar Max Woolf Committed by GitLab
Browse files

Merge branch 'rate-limit/identifier-normalization' into 'master' 

fix: reject duplicate normalized rate-limit identifier keys

See merge request !337

Merged-by: Max Woolf's avatarMax Woolf <mwoolf@gitlab.com>
Approved-by: Max Woolf's avatarMax Woolf <mwoolf@gitlab.com>
Reviewed-by: default avatarGitLab Duo <gitlab-duo@gitlab.com>
Co-authored-by: Hercules Merscher's avatarHercules Merscher <hmerscher@gitlab.com>
parents ee6eb8ba 031b3e4b

lib/labkit/rate_limit/identifier.rb

+26 −1
Original line number Diff line number Diff line
@@ -6,6 +6,9 @@ module Labkit 
    # describe the caller (e.g. user, ip, endpoint).

    # Endpoint values are normalised at construction time (query string stripped).

    class Identifier

      InvalidKeyError = Class.new(ArgumentError)

      DuplicateNormalizedKeyError = Class.new(InvalidKeyError)



      # Normalize an endpoint value: strip query string.

      def self.normalize_endpoint(value)

        return value unless value.is_a?(String)
@@ -16,7 +19,29 @@ module Labkit 
      attr_reader :attributes



      def initialize(attributes = {})

        normalised = attributes.transform_keys(&:to_sym)

        normalised = {}

        original_keys = {}



        attributes.each do |key, value|

          unless key.respond_to?(:to_sym)

            # Reject keys such as nil, 42, and [] instead of leaking NoMethodError;

            # identifiers must have a canonical symbol key for matching and serialization.

            raise InvalidKeyError, "Identifier key #{key.inspect} must respond to #to_sym"

          end



          normalised_key = key.to_sym

          if normalised.key?(normalised_key)

            # Reject { user: 1, "user" => 2 } (and its reverse order): silently

            # choosing a value would make the rate-limit bucket insertion-order dependent.

            raise DuplicateNormalizedKeyError,

              "Identifier keys normalize to the same key #{normalised_key.inspect}: " \

                "#{original_keys[normalised_key].inspect} and #{key.inspect}"

          end



          original_keys[normalised_key] = key

          normalised[normalised_key] = value

        end



        normalised[:endpoint] = self.class.normalize_endpoint(normalised[:endpoint]) if normalised.key?(:endpoint)

        @attributes = normalised.freeze

      end

spec/labkit/rate_limit/identifier_spec.rb

+32 −0
Original line number Diff line number Diff line
@@ -15,6 +15,33 @@ RSpec.describe Labkit::RateLimit::Identifier do 
      expect(id[:user]).to eq(99)

    end



    it "accepts independent symbol and string keys" do

      id = described_class.new(user: 42, "ip" => "1.2.3.4")

      expect(id.attributes).to eq(user: 42, ip: "1.2.3.4")

    end



    it "rejects a symbol key followed by its string equivalent" do

      expect { described_class.new(user: 1, "user" => 2) }

        .to raise_error(described_class::DuplicateNormalizedKeyError, /:user and "user"/)

    end



    it "rejects a string key followed by its symbol equivalent" do

      expect { described_class.new("user" => 1, user: 2) }

        .to raise_error(described_class::DuplicateNormalizedKeyError, /"user" and :user/)

    end



    it "rejects duplicate normalized keys even when their values are equal" do

      expect { described_class.new(user: 1, "user" => 1) }

        .to raise_error(described_class::DuplicateNormalizedKeyError)

    end



    [nil, 42, []].each do |invalid_key|

      it "rejects #{invalid_key.inspect} as an attribute key" do

        expect { described_class.new(invalid_key => "value") }

          .to raise_error(described_class::InvalidKeyError, /must respond to #to_sym/)

      end

    end



    it "freezes attributes" do

      id = described_class.new(user: 1)

      expect(id.attributes).to be_frozen
@@ -58,6 +85,11 @@ RSpec.describe Labkit::RateLimit::Identifier do 
      id = described_class.new(user: 42, endpoint: "/api/foo?x=1")

      expect(id[:user]).to eq(42)

    end



    it "normalizes an endpoint provided with a string key" do

      id = described_class.new("endpoint" => "/api/foo?x=1")

      expect(id[:endpoint]).to eq("/api/foo")

    end

  end



  describe "round-trip serialize/deserialize" do