Flow
Three walls around one list
1Fixed query text
The query text never changes. The list goes in as one array parameter.
2Strict shape
Every argument is checked against a strict shape. Anything off is rejected, not cleaned.
3One-tenant account
Tools use a database account that can only see one tenant.
The new usage tool checks the tenant correctly. Then the next line lets one optional list walk right around that check, and a model is the one filling in the list.
The tool is part of my observability service, which tracks what my coding agents spend. A recent commit let the agents ask the service questions. One tool shows usage per session and takes an optional list of session IDs.
The token part holds
The caller sends a signed token. The server checks it, finds the user's organization, and keeps it for the length of the request. The tool reads the tenant from there, never from its arguments. The tenant goes to the database as a bound parameter, so it can't be turned into code.
The list goes into the query text
The session IDs don't get that treatment. Each one is wrapped in quotes and pasted straight into the query text. So an ID can contain a quote and a closing bracket, end the list early, and add its own condition. Something like "or 1 = 1", plus a comment mark to hide the rest.
AND is checked before OR. So the query becomes "this tenant and this session, or true", and true always wins. The caller gets usage totals from every tenant. It's only a read, but it's someone else's data.
I found this in the diff. I haven't tried it on the real database, and I'm not sure if the database account would block it. The app wouldn't. Failed calls also send their error text back, which helps anyone poking at it.
The model writes the list
No person types those IDs. A model picks them from what it has read: files, issues, web pages, tool output. The user is real, but the string came from somewhere else. I treat tool arguments from a model like form fields from a stranger.
The same commit told my agents, in their rule files, that tenant safety is critical and that queries should use the safe helpers. Another file in that commit binds session IDs the right way. Probably an agent wrote the bad line anyway.
The same habit, broken differently
A duplicate check hand-quotes session IDs too, but puts them in a bound parameter. The quotes end up inside the value, so it likely never matches a real ID and never catches a duplicate. The commit's tests don't cover either spot.
There are smaller problems on the same path too, like a limit with no cap and a token check that skips the audience.
Three walls I'd put up
- Keep the query text fixed and pass the list as one array parameter.
- Check every argument against a strict shape and reject anything off, rather than cleaning it.
- Give tools a database account that can only see one tenant.
const ids = z.array(z.string().regex(/^[a-z0-9-]+$/)).max(50).parse(args.ids ?? []);
Any one of those three would have stopped this. Right now the tool has neither of the first two.