Non-string label values bypass escaping and can produce malformed exposition

[milcho0604]

Summary

When a label value is not a string, escapeLabelValue() returns it untouched, and it is converted to a string later during interpolation — after escaping would have happened. If that string form contains " or a newline, the rendered exposition is malformed and promtool check metrics rejects the whole response.

Reproduction

const client = require('prom-client');

const registry = new client.Registry();
const g = new client.Gauge({ name: 'g', help: 'h', labelNames: ['x'], registers: [registry] });

// Array label value containing a quote
g.set({ x: ['say "hi"'] }, 1);
console.log(JSON.stringify(await registry.metrics()));

Rendered output (whole body, JSON.stringify'd so the escaping is unambiguous):

"# HELP g h\n# TYPE g gauge\ng{x=\"say \"hi\"\"} 1\n"

The quotes inside the label value are not escaped. With a newline instead, the value breaks the line in two:

g.set({ x: ['a\nb'] }, 1);
// "# HELP g h\n# TYPE g gauge\ng{x=\"a\nb\"} 1\n"

A plain string with the same content is escaped correctly, which is the contrast:

g.set({ x: 'say "hi"' }, 1);
// "# HELP g h\n# TYPE g gauge\ng{x=\"say \\\"hi\\\"\"} 1\n"

Effect on scraping

Rendering a registry that holds one such series plus two ordinary ones and piping it to promtool check metrics:

healthy_metric 1
bad_metric{x="oops"quote"} 1
another_healthy 2

error while linting: text format parsing error in line 7: unexpected end of label value "oops"

promtool exits rc=1. The same body with a plain string label value (bad_metric{x="oops\"quote"} 1) exits rc=0. The whole response is rejected, not just the offending series. (I checked the exposition with promtool; I did not point a running Prometheus server at it.)

Where it comes from

lib/registry.js:

function escapeLabelValue(str) {
	if (typeof str !== 'string') {
		return str;
	}
	return escapeString(str).replace(/"/g, '\\"');
}

The value is interpolated afterwards as `${n}="${escapeLabelValue(v)}"`, so a non-string is stringified after the escaping step has already been skipped. Removing the early return locally (coercing before escaping) changes the first example's output to g{x="say \"hi\""} 1, which isolates the behaviour to those lines.

Note

#194 described the same scraping failure for string label values and was closed as fixed. This is the remaining path for values that are not strings.

Whether non-string label values should be coerced and escaped, or rejected when recorded, seems like a policy call rather than something to decide in a patch — arrays in particular are already used in the wild (#582), so rejecting them would be a behaviour change. Happy to open a PR for whichever direction you prefer.

Environment

• prom-client: main @ 21c8c85
• Node.js: v25.9.0
• promtool 3.11.3 (prom/prometheus@sha256:e4254400b856)
• Scope of what I tested: Gauge#set() with a label value, rendered via registry.metrics().

[jdmarshall]

Let me preface this by saying I think we should probably fix this, but also state what my boundaries are, since there's a bunch of issues in the backlog with a similar theme that I will probably be closing as won't fix.

Telemetry gathering is a very low-level, and time sensitive operation. We cannot afford to run sanity checks on every single call, and I intend to be more assertive about saying no to any checks that cannot be implemented cheaply. If I had my way I'd make all the stats collection another 30% cheaper, but I've done what I can do and am waiting for some new epiphany to show me a better way to collect the data cheaper, or for someone to file a PR that does.

That said, this is a once per run call, and so the cost is to bootstrap and possibly first response time, which I'm okay with. Now that interpolation is a thing that's the simplest solution and the one I'd look for here.

[jdmarshall]

After seeing a couple PRs offering to fix this problem, I think we now have a clearer illustration of the problem. escapeValue is a very hot path, and so much earlier versions of this function skipped a few important steps. Some of those have been fixed. Non-string values have not.

The problem is that escapeLabelValue() runs during metrics(), and the cost of `metrics()' is linearly proportional to the total tag cardinality of the process, which only ever goes up.

So I suspect that the real solution is to move some check either to:

1. validateLabels(), which may be just as expensive for apps with a high enough rate of stats
2. LabelMap and LabelGrouper, so the values are only escaped and type converted the first time each combination is encountered

As the author of LabelMap, I'm rather fond of option 2. The whole point of that object was to abstract complexity scattered around the library to one spot where different improvements could be tried without triggering a major version number release due to changes in the storage strategy. And this is a great example of one.

My request would be that lines like

https://github.com/prometheus/client_js/blob/main/lib/util.js#L198C1-L198C42 be extracted to a helper function, so that the label escaping can happen consistently before every this.#map.set call because there will only be one.

I don't know if we would want to go as far as relocating more of the ecape code here. If so that would be a substantial performance improvement. Hence the perf label I added a few minutes ago.

[jdmarshall]

On the wider topic of label values:

If you guys don't know what your label values are at compile time, you are fundamentally misusing your telemetry tools, in a manner similar to SQL injection attacks.

First, any vendor who charges based on label cardinality will make an absolute mint off of their fee schedules that punish open ended label-value pairing. Worse, there are attack vectors there for DOS attacks by exhausting memory or your monthly Cloud spend.

You should never, ever, have a situation in production where a label value needs to be sanitized. Except for punctuation escaping, and one could argue that even that should be coded out with a PR to reduce telemetry overhead.

There are a few exceptions to values that aren't known at compile time but are still bounded, such as version numbers and deployment numbers. And of course Exemplars may have some fuzzier data like correlation-ids, but those are not technically labels. And none of those exceptions apply to this issue.

I've seen enough comments now misunderstanding this situation, both here and in the OTEL projects, that I think I need to write this up, but this is the Cliff's Notes.

[milcho0604]

Thanks for clarifying the performance concern. I agree the fix should avoid repeating coercion for every sample on every metrics() call. The issue itself is narrower: whenever a value reaches exposition formatting, the library must correctly escape the string representation it emits.

Option 2 makes sense for labels that pass through the built-in metric stores: pay the coercion cost once when a new label combination is created rather than on every metrics() call. I'll put together a PR along those lines:

• Introduce a shared label-normalization helper and apply it consistently when LabelMap entries and LabelGrouper values are stored — including update paths such as LabelMap.merge() that can replace stored labels without going through the insertion point, and values that keep their own copy of the labels (Summary does: its exported labels come from the stored value, not the map entry).
• Coerce with the same semantics the exposition itself applies (template interpolation), and add lookup tests for nullish and equivalent string/non-string inputs — keyFrom() already coerces ordinary non-nullish values while building keys, so lookups stay consistent.
• Keep the existing-combination update path unchanged (the first insertion of a combination pays one extra pass over its labels), and add focused benchmarks for first insertion and repeated metrics() calls, with before/after numbers in the PR.

Two scope points I'll call out explicitly in the PR rather than paper over:

• Custom metric results, registry default labels, and exemplar labels do not all pass through LabelMap, so the storage-boundary change doesn't cover them by itself. I'll document what is and isn't covered.
• Coercing at storage changes the observable label values returned by getMetricsAsJSON() (3 becomes "3"). I'll note and test that behaviour.

I'll keep full escape relocation out of the initial PR: rendering currently escapes backslashes, so moving the escaping has to cover every label source atomically or it either misses paths or double-escapes. I can scope and benchmark that separately if the first numbers look promising.

[jdmarshall]

Exemplar labels are often unique and would never get conflated with the metric labels anyway. So they won’t ever settle the way telemetry labels should. I’d leave them out of the scope of this work. Thanks for volunteering to take on the larger task.

On the topic of label values being converted to strings, the Prometheus docs are oddly quiet on this but my understanding was the proposed change is generally the expected behavior.

[jdmarshall]

I'm on the fence about LabelGrouper. On the one hand, any extra aggregate() costs will be amortized across the number of worker threads being aggregated, and so the costs are more controlled. On the other hand I worked by butt off for a 2.4x improvement there and I'm loathe to give it back right away. Although maybe before it ships is the time to do so. We shall have to see about the timing.

Can you consider the possibility of the aggregate workflow expecting the data to have already been sanitized (at least the string coercion aspect if not the regexp) by the metrics() call? I think #789 doesn't make that situation worse, but it might.