From e7111cd8e8f5780133660fbd932a053bfbb6ba2b Mon Sep 17 00:00:00 2001
From: Chris Sinjakli <chris@sinjakli.co.uk>
Date: Sun, 9 Jan 2022 16:51:15 +0000
Subject: [PATCH] Move to explicit method call for Basic Auth credentials in
 push client

While URLs do support passing Basic Auth credentials using the
`http://user:password@example.com/` syntax, the username and password in
that syntax have to follow the usual rules for URL encoding of
characters per RFC 3986
(https://datatracker.ietf.org/doc/html/rfc3986#section-2.1).

Rather than place the burden of correctly performing that encoding on
users of this gem, we decided to have a separate method for supplying
Basic Auth credentials, with no requirement to URL encode the characters
in them.

Signed-off-by: Chris Sinjakli <chris@sinjakli.co.uk>
---
 lib/prometheus/client/push.rb       | 30 +++++++++++++++++++++-
 spec/prometheus/client/push_spec.rb | 40 +++++++++++++++++++----------
 2 files changed, 55 insertions(+), 15 deletions(-)

diff --git a/lib/prometheus/client/push.rb b/lib/prometheus/client/push.rb
index 04f4d7ae..5230b6b4 100644
--- a/lib/prometheus/client/push.rb
+++ b/lib/prometheus/client/push.rb
@@ -39,7 +39,9 @@ def initialize(job:, gateway: DEFAULT_GATEWAY, grouping_key: {}, **kwargs)
         @gateway = gateway || DEFAULT_GATEWAY
         @grouping_key = grouping_key
         @path = build_path(job, grouping_key)
+
         @uri = parse("#{@gateway}#{@path}")
+        validate_no_basic_auth!(@uri)
 
         @http = Net::HTTP.new(@uri.host, @uri.port)
         @http.use_ssl = (@uri.scheme == 'https')
@@ -47,6 +49,11 @@ def initialize(job:, gateway: DEFAULT_GATEWAY, grouping_key: {}, **kwargs)
         @http.read_timeout = kwargs[:read_timeout] if kwargs[:read_timeout]
       end
 
+      def basic_auth(user, password)
+        @user = user
+        @password = password
+      end
+
       def add(registry)
         synchronize do
           request(Net::HTTP::Post, registry)
@@ -113,7 +120,7 @@ def request(req_class, registry = nil)
 
         req = req_class.new(@uri)
         req.content_type = Formats::Text::CONTENT_TYPE
-        req.basic_auth(@uri.user, @uri.password) if @uri.user
+        req.basic_auth(@user, @password) if @user
         req.body = Formats::Text.marshal(registry) if registry
 
         response = @http.request(req)
@@ -126,6 +133,27 @@ def synchronize
         @mutex.synchronize { yield }
       end
 
+      def validate_no_basic_auth!(uri)
+        if uri.user || uri.password
+          raise ArgumentError, <<~EOF
+            Setting Basic Auth credentials in the gateway URL is not supported, please call the `basic_auth` method
+
+            Received username `#{uri.user}` in gateway URL
+
+            While URLs do support passing Basic Auth credentials using the
+            `http://user:password@example.com/` syntax, the username and
+            password in that syntax have to follow the usual rules for URL
+            encoding of characters per RFC 3986
+            (https://datatracker.ietf.org/doc/html/rfc3986#section-2.1).
+
+            Rather than place the burden of correctly performing that encoding
+            on users of this gem, we decided to have a separate method for
+            supplying Basic Auth credentials, with no requirement to URL encode
+            the characters in them.
+          EOF
+        end
+      end
+
       def validate_no_label_clashes!(registry)
         # There's nothing to check if we don't have a grouping key
         return if @grouping_key.empty?
diff --git a/spec/prometheus/client/push_spec.rb b/spec/prometheus/client/push_spec.rb
index 372094ca..e4fede99 100644
--- a/spec/prometheus/client/push_spec.rb
+++ b/spec/prometheus/client/push_spec.rb
@@ -271,23 +271,35 @@
     end
 
     context 'Basic Auth support' do
-      let(:gateway) { 'https://super:secret@localhost:9091' }
+      context 'when credentials are passed in the gateway URL' do
+        let(:gateway) { 'https://super:secret@localhost:9091' }
 
-      it 'sets Basic Auth header when requested' do
-        request = double(:request)
-        expect(request).to receive(:content_type=).with(content_type)
-        expect(request).to receive(:basic_auth).with('super', 'secret')
-        expect(request).to receive(:body=).with(data)
-        expect(Net::HTTP::Put).to receive(:new).with(uri).and_return(request)
+        it "raises an ArgumentError explaining why we don't support that mechanism" do
+          expect { push }.to raise_error ArgumentError, /in the gateway URL.*username `super`/m
+        end
+      end
 
-        http = double(:http)
-        expect(http).to receive(:use_ssl=).with(true)
-        expect(http).to receive(:open_timeout=).with(5)
-        expect(http).to receive(:read_timeout=).with(30)
-        expect(http).to receive(:request).with(request).and_return(response)
-        expect(Net::HTTP).to receive(:new).with('localhost', 9091).and_return(http)
+      context 'when credentials are passed to the separate `basic_auth` method' do
+        let(:gateway) { 'https://localhost:9091' }
+
+        it 'passes the credentials on to the HTTP client' do
+          request = double(:request)
+          expect(request).to receive(:content_type=).with(content_type)
+          expect(request).to receive(:basic_auth).with('super', 'secret')
+          expect(request).to receive(:body=).with(data)
+          expect(Net::HTTP::Put).to receive(:new).with(uri).and_return(request)
+
+          http = double(:http)
+          expect(http).to receive(:use_ssl=).with(true)
+          expect(http).to receive(:open_timeout=).with(5)
+          expect(http).to receive(:read_timeout=).with(30)
+          expect(http).to receive(:request).with(request).and_return(response)
+          expect(Net::HTTP).to receive(:new).with('localhost', 9091).and_return(http)
+
+          push.basic_auth("super", "secret")
 
-        push.send(:request, Net::HTTP::Put, registry)
+          push.send(:request, Net::HTTP::Put, registry)
+        end
       end
     end