Skip to content

fix(api): reject NUL bytes in agent_card_metadata instead of 500ing - #479

Merged
scale-ballen merged 4 commits into
mainfrom
sec/agent-card-metadata-nul-byte-500
Nov 5, 2026
Merged

scale-ballen merged 4 commits into
mainfrom
sec/agent-card-metadata-nul-byte-500

Conversation

@scale-ballen

@scale-ballen scale-ballen commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

What

GET /agents?agent_card_metadata={"skill":"x\u0000"} returns 500. This makes it a 400, like every other malformed value of that parameter.

Why

\u0000 is valid JSON. json.loads decodes it to an ordinary str, so it sails through _parse_agent_card_metadata — but a PostgreSQL text value cannot hold a NUL. The driver only discovers that while encoding the JSONB bind parameter, and raises asyncpg.exceptions.UntranslatableCharacterError from inside the query, past every specific exception handler and into the catch-all.

_parse_agent_card_metadata already exists to catch exactly this class of input — values Python's json accepts but PostgreSQL will not store: NaN, Infinity, floats that overflow to infinity, integers past CPython's conversion limit. NUL was the one remaining hole in that guard.

Caller error reaching the catch-all handler is bad on its own terms: it burns error budget, pages on the wrong signal, and writes a driver-level stack trace to the logs for what is just a bad query string.

How

_reject_nul_bytes walks the decoded object — keys, values, nested dicts, nested lists — and the parser turns a hit into a 400 with the same shape as the existing rejections.

Tests

test_list_agents_agent_card_metadata_nul_byte_returns_400, parametrized over the NUL in a value, in a key, inside a list and inside a nested object.

🤖 Generated with Claude Code

RetriggerView in GreptileConfidence Score: 5/5

The PR appears safe to merge.

Summary

Rejects decoded NUL characters in agent_card_metadata before the database query, returning HTTP 400 instead of 500.

  • GET /agents returns 400 for NUL characters in metadata filters.

Reviews (4) · Last reviewed commit: "Merge branch 'main' into sec/agent-card-..." · Reviewed by Greptile

`GET /agents?agent_card_metadata={"skill":"x\u0000"}` returned 500. The escape
is valid JSON and `json.loads` decodes it to an ordinary `str`, so it passed
`_parse_agent_card_metadata` untouched, but a PostgreSQL `text` value cannot
hold a NUL. The driver only finds out while encoding the JSONB bind parameter
and raises `asyncpg.exceptions.UntranslatableCharacterError` from inside the
query -- past every specific exception handler, into the catch-all.

That function already rejects the other values Python's `json` accepts but
PostgreSQL will not store (`NaN`, `Infinity`, overflowing floats, oversized
integers) for exactly this reason. NUL was the remaining hole. Walk the decoded
object -- keys, values, nested dicts and lists -- and reject it as a 400 like
the rest.

Caller error should not be a server error: a 500 here burns an error budget,
pages on the wrong signal, and writes a driver-level stack trace to the logs
for what is simply a bad query string.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@scale-ballen
scale-ballen requested a review from a team as a code owner November 4, 2026 10:59
`_detected_resource()` used `get_aggregated_resources([OTELResourceDetector()])`,
which merges the SDK's default resource. Since opentelemetry-sdk 1.3x that
default generates a random UUID for `service.instance.id` when nothing else
supplies one, and the dependency floor here is `opentelemetry-sdk>=1.28.0`, so
an ordinary resolver bump turned it on.

`_unique_instance_id` then always took its "existing value" branch, and both
call sites -- the auto-instrumentation bootstrap and the standalone
MeterProvider resource -- started publishing `<random-uuid>.<pid>`. Still
unique per worker, which is what the function guarantees, but it drops the
service and pod the series belongs to and changes on every process start, so
metric identity no longer survives a restart.

`_detected_resource` exists to answer "what did the operator configure", so it
now uses the detector alone and reports `service.instance.id` as absent when
the operator did not set one. An operator-supplied value, via
OTEL_RESOURCE_ATTRIBUTES, still wins and still gets the pid suffix.

The two `_unique_instance_id` tests rebuilt the aggregated resource by hand
instead of calling `_detected_resource()`, which is why they drifted from the
code path they name; `test_unique_instance_id_builds_when_missing` had been
failing on main. They now go through the real function, and a new test pins
the absence of the SDK-generated id directly.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment thread agentex/src/utils/otel_metrics.py Resolved
Review catch on the previous commit. Dropping the SDK's generated
`service.instance.id` left `<service>.unknown.<pid>` as the fallback whenever
neither an operator value nor `k8s.pod.name` is present -- off Kubernetes, in
local runs, in plain containers. Pids repeat freely across containers and
hosts, so two replicas could report the same instance id and merge their
counter streams into one wrong series at a shared collector. That is the exact
uniqueness guarantee the function exists to provide.

The SDK's random id is still the wrong thing to *prefer* -- opaque, and new on
every restart -- but it is a sound tiebreaker, so it now stands in for the
missing pod name instead of the literal "unknown". A configured identity, and
a real pod name, both still win.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@scale-ballen
scale-ballen merged commit b1ecd31 into main Nov 5, 2026
46 checks passed
@scale-ballen
scale-ballen deleted the sec/agent-card-metadata-nul-byte-500 branch November 5, 2026 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants