How to spot injection (untrusted input into a sink) in code review
Injection is untrusted input reaching something that interprets it: a query, a shell, a file path, a URL, a page of HTML. The input looks like data to the developer and like code or a location to the thing that receives it.
What it looks like in a diff
A request value built into a string by concatenation or formatting, next to an interpreter. The value may pass through two or three functions before it gets there, so the line that builds the string can look harmless on its own.
Trace each request value to where it ends up. The fixes depend on the sink: bound parameters for queries, an allowlist for names that cannot be bound, a resolved path checked against its base directory, an argument list instead of a shell string, a same-site check on redirects, escaping by the template rather than by hand.
Questions to ask in review
- Which values in this diff come from the request, a file or another service?
- Where does each one end up, and does that place interpret it?
- Is it passed as data (a bound parameter, an argument list) or spliced into a string?
- If it is checked, is the check on the final value, after decoding and joining?
How to word the comment
Name the input, the path it takes and what goes wrong, then the fix. For example (on an imagined PR, not one from the library):
The uploaded file name goes into the
ffmpegcommand string, and the command runs through a shell, so a name with;in it runs a second command on the server. Pass the arguments as a list without a shell, and generate the output file name yourself.
Practise on real PRs
As of October 2026, the DiffDojo library has 7 live PRs that plant injection, across 5 stacks. They are not listed here: a PR you open knowing its bug teaches less. Each review is graded on whether you find the bug and explain it.