Skip to content

Commit

Permalink
Fix URI escaping of spaces in push.rb
Browse files Browse the repository at this point in the history
When `URI.escape` was deprecated, we switched to using `CGI.escape` to
construct the request path in push.rb.

The problem with that is that it mishandles spaces, encoding them as `+`
rather than `%20`. This leads to literal `+` characters in metric
labels, which is a bug.

This commit uses `ERB::Util.url_encode` instead, which gives us the
behaviour we want.

It's not the part of the standard library I expected to use for this,
but it's the only one that seems to have the behaviour we want and that
hasn't disappeared in Ruby 3.0. `WEBrick::HTTPUtils.escape` seems
similar on the surface, but doesn't encode `&`. Presumably it's intended
as a best-effort way to encode an entire URL, and has to assume that any
`&` character is a separator between query params.

Signed-off-by: Chris Sinjakli <chris@sinjakli.co.uk>
  • Loading branch information
Sinjo committed Jun 9, 2021
1 parent dfa5f90 commit ec5c5aa
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 4 deletions.
6 changes: 3 additions & 3 deletions lib/prometheus/client/push.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
require 'thread'
require 'net/http'
require 'uri'
require 'cgi'
require 'erb'

require 'prometheus/client'
require 'prometheus/client/formats/text'
Expand Down Expand Up @@ -72,9 +72,9 @@ def parse(url)

def build_path(job, instance)
if instance && !instance.empty?
format(INSTANCE_PATH, CGI::escape(job), CGI::escape(instance))
format(INSTANCE_PATH, ERB::Util::url_encode(job), ERB::Util::url_encode(instance))
else
format(PATH, CGI::escape(job))
format(PATH, ERB::Util::url_encode(job))
end
end

Expand Down
2 changes: 1 addition & 1 deletion spec/prometheus/client/push_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@
it 'escapes non-URL characters' do
push = Prometheus::Client::Push.new(job: 'bar job', instance: 'foo <my instance>')

expected = '/metrics/job/bar+job/instance/foo+%3Cmy+instance%3E'
expected = '/metrics/job/bar%20job/instance/foo%20%3Cmy%20instance%3E'
expect(push.path).to eql(expected)
end
end
Expand Down

0 comments on commit ec5c5aa

Please sign in to comment.