Last week I shipped a lot: eleven new exercise types on Monday and Tuesday, a SQL exercise type running a throwaway Postgres per attempt on Wednesday. On Thursday I stopped adding things and spent the day attacking what I'd just built.
Six bugs. Every one of them belongs to a class that generalizes, so here they are with names.
1. Gating the answer, but not the field that contains the answer
Our auto-graded exercises have an author-supplied explanation field. The tests for it contain strings like "6*7=42". It is, in practice, the answer written out in prose.
The raw answer keys — expectedAnswer, correctCells, correctOrder — were all correctly gated behind the author's showCorrectAnswerOnFailure setting. In the same services. Written at the same time.
explanation was returned unconditionally. In eight step types. One wrong attempt, and the response handed over the answer — no probing, no cleverness, and the author's explicit "don't reveal the answer on failure" setting silently ignored.
The class: secondary fields that contain the secret. You will remember to protect the thing that is obviously the secret. The risk is the human-readable field sitting next to it — the explanation, the hint, the error message, the debug payload, the log line — that contains the same information in a different shape. When you gate something, grep for every field that could restate it.
2. Correctness overlays are answer keys
Same audit, a related one. Our matching exercise returned pairResults: a map of pairId → boolean telling the UI which pairs the student got right, so it can draw green and red.
Returned unconditionally, on every attempt.
That's not feedback, that's an oracle. Submit anything, read which pairs are true, flip the ones that are false, submit again. Two attempts to a perfect score without understanding the material.
The class: per-item correctness is equivalent to the key when items are independent. A single "you scored 3/8" is fine. "Which 3" is the answer key delivered in instalments. The same bug had already been fixed for the cell/position/indent variants in other step types — one shape was missed, and one is enough.
3. @Body('field') silently skips validation
This one is NestJS-specific and I'd bet real money it's in your codebase too.
// The DTO exists. @MaxLength(8000) is right there on it.
async submit(@Body('studentSql') studentSql: string) { ... }
ValidationPipe skips validation when the metatype is a primitive. String is a primitive. So the DTO class sits in the repo, fully annotated, imported nowhere that matters, and not one of its constraints runs on this path. Everything looks validated. Nothing is.
// Fix: take the whole body, typed as the DTO.
async submit(@Body() body: SubmitSqlChallengeDto) { ... }
Once I knew the shape, I grepped for it — and found six more controllers doing the same thing, some via @Body('field'), some via data: any, some with an inline object literal as the parameter type.
The class: validation that is present but not reachable. This is worse than no validation, because no validation is visible in review. A @MaxLength sitting on an unreachable DTO reads as a control to everyone who looks at it.
4. A session default is not a limit
Our SQL sandbox runs student SQL as a restricted role, created per attempt:
ALTER ROLE sql_challenge_student SET statement_timeout = '5s';
That is a session default. The student's own SQL can begin with SET statement_timeout = 0; and it is gone. The mechanism I was relying on to stop runaway queries could be switched off by the queries it was supposed to stop.
And the author-supplied setup.sql and verify.sql, which run as superuser, had no timeout at all. So a Pro-tier author could put pg_sleep() in a setup script and quietly DoS the shared sandbox container pool — on every verify and every submit, for every student in the course.
The fix moves the limit somewhere the workload can't reach it: every psql invocation wrapped in timeout N at the process level. (Detail for anyone copying this: BusyBox timeout, which is what you get in postgres:16-alpine, exits 143 on kill, not GNU's 124. If you only check for 124, your timeouts look like crashes.)
The class: limits enforced inside the thing being limited. If the constrained party can execute code in the same context as the constraint, it isn't a constraint. Put the ceiling in the layer above — a process timeout, a cgroup, a supervisor.
5. Your CSV export executes code on someone else's machine
Our gradebook export escaped CSV correctly by the usual definition: quotes doubled, fields with ", ; or newlines wrapped. Textbook.
It did nothing about a leading =, +, - or @, which Excel and Google Sheets interpret as the start of a formula.
Students control their own names. A student sets their display name to a HYPERLINK formula. The teacher exports the gradebook and opens it. The formula runs — on the teacher's machine, in the teacher's spreadsheet, with the teacher's data.
The fix is a one-liner (prefix such fields with an apostrophe), and the interesting part is how the bug survived: the identical issue had already been fixed in our attendance export, independently, by someone solving the same problem in a different file. Two implementations of "escape a CSV field", one hardened and one not.
The class: escaping for the wrong consumer. We escaped for the CSV parser. The attacker's target was the spreadsheet application that opens the CSV afterwards. Ask who ultimately interprets your output, not what format you're emitting. (Same audit, same class: instructor-controlled strings interpolated unescaped into reminder emails sent to every enrolled student. HTML has a consumer too.)
6. The endpoint that forgot it was the paid tier
The one I'd rather not write up, which is exactly why it's here.
Our SQL exercise editor has a preview endpoint so authors can test their setup script while writing an exercise. create() and update() both enforce the Pro-tier plan gate. preview() checked that you were logged in, and nothing else.
What preview does is run the submitted setupSql — as full Postgres superuser, before the restricted student role exists, because that's what setting up an exercise requires. Postgres superuser includes COPY ... TO/FROM PROGRAM: command execution inside the container.
So: any logged-in account, including a free one, could reach superuser SQL execution in the sandbox. The plan gate on the two obvious endpoints created a completely convincing illusion of a guarded feature.
The class: auxiliary endpoints inherit the feature's privileges but not its checks. Preview, dry-run, validate, test-connection, export-sample — the endpoints that exist so the main flow feels good, get written last, and get reviewed least. They usually reach exactly as deep as the endpoint they're previewing. Enumerate every route that touches a privileged capability, not every route that looks important.
What I actually take from this
Four of the six exist because a control was applied somewhere and not everywhere: the answer key gated but not the explanation, the cell overlay gated but not the pair overlay, the plan gate on create/update but not preview, CSV formulas neutralized in one export but not the other. The failure mode isn't ignorance of the rule. It's incomplete application of a rule everyone involved already knew.
Which suggests the audit question worth asking isn't "did we handle this?" It's "where else does this shape appear, and did we handle it there?" Every one of these was found by taking a known control and grepping for every place it should exist.
The other one: I audited this code the day after writing it, while I still remembered every decision — and still found six. The version of me that wrote it was certain it was fine.
Top comments (0)