Merge pull request #242 from prometheus/sinjo-fix-basic-auth

Move to explicit method call for Basic Auth credentials in push client

Author: Sinjo

lib/prometheus/client/push.rb

@@ -39,14 +39,21 @@ 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')
         @http.open_timeout = kwargs[:open_timeout] if kwargs[:open_timeout]
         @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,39 @@ 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. Instead of passing
+            Basic Auth credentials like this:
+
+            ```
+            push = Prometheus::Client::Push.new(job: "my-job", gateway: "http://user:password@localhost:9091")
+            ```
+
+            please pass them like this instead:
+
+            ```
+            push = Prometheus::Client::Push.new(job: "my-job", gateway: "http://localhost:9091")
+            push.basic_auth("user", "password")
+            ```
+
+            While URLs do support passing Basic Auth credentials using the
+            `http://user:[email protected]/` 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?

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