diff --git a/CHANGELOG.md b/CHANGELOG.md index d804f492c..50465b62c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) ### Deprecated ### Removed ### Fixed +- Fixed `request_signer` receiving the raw body object instead of the serialized string, causing signature mismatches ([#311](https://github.com/opensearch-project/opensearch-ruby/issues/311)) +- Fixed double-encoding of wildcard and special characters in index/path parameters (e.g. `index: "test-*"` was being sent as `test-%252A`) ([#319](https://github.com/opensearch-project/opensearch-ruby/issues/319)) ### Security ## [4.0.0] diff --git a/lib/opensearch/api/utils.rb b/lib/opensearch/api/utils.rb index ad45d7aa9..934f8a484 100644 --- a/lib/opensearch/api/utils.rb +++ b/lib/opensearch/api/utils.rb @@ -9,7 +9,7 @@ # frozen_string_literal: true -require 'cgi/escape' +require 'uri' module OpenSearch module API @@ -38,11 +38,14 @@ def self.validate_query_params!(params, valid_param_names = nil) end # @return the value escaped for URL unless it is a Hash + # Uses URI encoding rather than CGI.escape so that characters meaningful in + # OpenSearch index patterns (e.g. '*') are preserved and not percent-encoded, + # which would cause double-encoding when the HTTP layer encodes the URL. def self.normalize_value(value) return value.clone if value.is_a? Hash value = value.to_s.strip unless value.is_a? Enumerable value = value.split(',') if value.is_a? String - value.map { |v| CGI.escape(v.to_s) }.join(',') + value.map { |v| URI::DEFAULT_PARSER.escape(v.to_s, /[^A-Za-z0-9\-._~*]/) }.join(',') end def self.build_url(*parts) diff --git a/lib/opensearch/transport/client.rb b/lib/opensearch/transport/client.rb index 374d35bf3..4ac65d016 100644 --- a/lib/opensearch/transport/client.rb +++ b/lib/opensearch/transport/client.rb @@ -190,8 +190,13 @@ def perform_request(method, path, params = {}, body = nil, headers = {}) method = @send_get_body_as if method == 'GET' && body if @options[:request_signer] connection = transport.get_connection + # Serialize body to the same string that will be sent on the wire so + # the signer computes a signature over the exact bytes transmitted. + # The transport uses __convert_to_json (which returns a String as-is + # and serializes everything else), so we mirror that here. + serialized_body = body.is_a?(String) ? body : (body && transport.serializer.dump(body)) headers = @options[:request_signer].sign_request( - method: method, path: path, params: params, body: body, headers: headers, + method: method, path: path, params: params, body: serialized_body, headers: headers, host: connection.host[:host], port: connection.host[:port], url: connection.full_url(path, params), diff --git a/spec/opensearch/api/utils_spec.rb b/spec/opensearch/api/utils_spec.rb index dbe0d29eb..492059d3b 100644 --- a/spec/opensearch/api/utils_spec.rb +++ b/spec/opensearch/api/utils_spec.rb @@ -44,11 +44,11 @@ it 'returns a new hash with keys as strings and values normalized except those of NON_URL_ARGS' do is_expected.to eq({ - 'hello' => 'world+2', + 'hello' => 'world%202', 'hash' => { bar: 'baz' }, 'number' => '42', 'bool' => 'true', - 'string' => 'amazing+spider-man,+big+%26+small', + 'string' => 'amazing%20spider-man,%20big%20%26%20small', 'nil' => '', 'array' => '1,2,3', 'headers' => [100] @@ -56,6 +56,32 @@ end end + describe '#normalize_value' do + it 'preserves wildcard * so index patterns are not double-encoded by the HTTP layer' do + expect(described_class.normalize_value('test-*')).to eq('test-*') + end + + it 'preserves * in comma-separated multi-index patterns' do + expect(described_class.normalize_value('logs-*,metrics-*')).to eq('logs-*,metrics-*') + end + + it 'percent-encodes spaces as %20 (not + which would be double-encoded)' do + expect(described_class.normalize_value('my index')).to eq('my%20index') + end + + it 'percent-encodes & and other special chars' do + expect(described_class.normalize_value('a&b')).to eq('a%26b') + end + + it 'returns plain strings unchanged' do + expect(described_class.normalize_value('my-index_name')).to eq('my-index_name') + end + + it 'converts an array of values joining with comma' do + expect(described_class.normalize_value(['idx-one', 'idx-*'])).to eq('idx-one,idx-*') + end + end + describe '#validate_query_params!' do subject(:validate) { described_class.validate_query_params!(params, Set.new(%w[foo bar])) } diff --git a/spec/opensearch/client/unit/request_signer_spec.rb b/spec/opensearch/client/unit/request_signer_spec.rb index 2f7403dba..21dbca3c6 100644 --- a/spec/opensearch/client/unit/request_signer_spec.rb +++ b/spec/opensearch/client/unit/request_signer_spec.rb @@ -40,9 +40,48 @@ def self.sign_request(args) { query: 'string' }, { request: :body }, { header1: 'value1', - dummy_header1: { request: :body }, + dummy_header1: '{"request":"body"}', dummy_header2: { query: 'string' }, dummy_header3: 'localhost' } ) end + + context 'when a body is provided' do + let(:captured_bodies) { [] } + let(:capturing_signer) do + bodies = captured_bodies + Class.new do + define_singleton_method(:sign_request) do |args| + bodies << args[:body] + args[:headers] + end + end + end + let(:capturing_client) do + described_class.new( + host: 'http://localhost:9200', + request_signer: capturing_signer + ).tap do |cli| + allow(cli.transport.transport).to receive(:perform_request).and_return({ body: 'ok' }) + end + end + + it 'passes a serialized String to sign_request, not the raw Hash' do + body_hash = { query: { match_all: {} } } + capturing_client.transport.perform_request('POST', '_search', {}, body_hash, {}) + expect(captured_bodies.last).to be_a(String) + expect(captured_bodies.last).to eq(OpenSearch::API.serializer.dump(body_hash)) + end + + it 'passes nil to sign_request when body is nil' do + capturing_client.transport.perform_request('GET', '_cat/health', {}, nil, {}) + expect(captured_bodies.last).to be_nil + end + + it 'passes a pre-serialized String body through unchanged' do + raw = '{"already":"serialized"}' + capturing_client.transport.perform_request('POST', '_search', {}, raw, {}) + expect(captured_bodies.last).to eq(raw) + end + end end