DynoInvoice · Reviewing like an attacker
Every DynoInvoice query must carry its tenant, so one customer can never read another’s data. A shared library adds that condition to every query and refuses anything that could leave it out. Before its first release was merged, the reviewer spent three rounds trying to break it, and broke it five times.
--Raw SQL fragments made only of operator characters were allowed, and -- is made of operator characters. The reviewer planted where(DSL.condition("--")) and the library prepared ... where ((--) and "orders"."tenant_id" = cast(? as uuid)): the comment swallows the tenant condition. It failed only by luck, when parameters were bound. The fix accepts no free text at all, recognises the library’s own operators by their place in the query tree, and checks the rendered statement for comments, semicolons and other escape characters outside literals.
The guard classified tables by their name in the model, but SQL is rendered after a table-mapping setting is applied. Mapping a platform table onto a tenant table returned three rows from two tenants instead of two from one, with no tenant condition, and a settings file on the classpath could switch it on without touching code. Now the library owns its settings, denies by default, and checks them on every statement.
The tests for the Spring integration were never wired into CI. They are now, and the report shows them running.
Fixes: 79ef79a6
Round 2. The first fix refused every window function, a regression no test covered. The library now lets a custom part through only where the database library itself places it (b7996c29).
Round 3. The check “does this part come from the database library’s own jar?” broke inside a Spring Boot executable jar, which is how services ship: every application class would have counted as trusted. The fix compares against the exact classes the library’s own factories produce, and a new test builds and runs three real jars with impostor classes planted in the library’s package. Run against the old check, the impostor passed the guard, and only the statement check caught its -- (d483b4de).
The merge was passed only after the reviewer’s closing check: planted joins, subqueries, common table expressions, unions, OR conditions, wrong-tenant and alias cases were all refused before any SQL was sent, and the mutation test was real. The pipeline ran 1,220 tests with none failing.
Security issues were the largest share of the findings that blocked DynoInvoice merges between 12 and 26 October, about a quarter of them. This one shows why review is a process, not a gate: the reviewer thinks like an attacker, plants the attack, and each finding becomes a test that stays in the build, so the same hole can’t quietly reopen. Each fix was reviewed again, and twice the fix itself was the next problem.
The library’s first release: e8353420. The rule it enforces is a requirement: a query that can omit the tenant condition is a defect.
More: when green isn’t enough · why does this line exist? · when the spec runs out · when a decision changes · one requirement and everything recorded about it · eight traces