Add security-sensitive change review checklist - #133
Conversation
Prompted by a Slack discussion about relying on reviewer intuition to catch high-blast-radius issues (auth, credentials, session state). PR #57 in this repo is the worked example: a cross-session credential-hijack bug and several resource-exhaustion gaps got past the original implementation and a full round of human review, caught only by later, independently-run adversarial passes. Adds a visible PR template section plus engineering-standards guidance covering: running /security-review (noting its explicit DoS/resource- exhaustion exclusions), writing down adversarial scenarios, adding a regression test proving the bug was real, and 2+ reviewer approval as a backstop. This is a prompt, not a CI gate.
|
|
||
| ## Security-sensitive change checklist | ||
|
|
||
| <!-- Only fill this in if this PR touches authentication, credentials, tokens, or session/connection state, AND an external user is involved. Otherwise, delete this section. --> |
There was a problem hiding this comment.
Why the distinction on external user?
There was a problem hiding this comment.
The risk this is guarding against is specifically multi-tenant: session/credential state leaking from one real external party to another. That only exists when the code path is actually reachable by more than one mutually-untrusted user at once — e.g. the hosted endpoint, where concurrent sessions from different customers can share server-side state. A single developer running this locally via stdio has no "other session" to leak into, so the same bug class isn't a security issue in that context — there's no second party to harm.
PR #57's bug is the concrete case: it mattered because the hosted deployment runs concurrent sessions from different customers on shared instances. The identical code running as one person's local process wouldn't have that blast radius.
Scoping it to "external user involved" is about keeping the overhead proportionate — apply the extra process where getting it wrong could expose one user's data to a different, unrelated user, not to every touch of session-shaped code regardless of whether anyone else is actually exposed to it.
2 participants
Summary
docs/engineering_standards.md.Test plan