From d28339a34fdca0affa1b5a8458cf8ee4e8d45038 Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Fri, 14 Aug 2026 00:01:14 -0400 Subject: [PATCH] Add security-sensitive change review checklist Prompted by a Slack discussion about relying on reviewer intuition to catch high-blast-radius issues (auth, credentials, session state). mcp-devkit-server PR #57 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. --- .github/pull_request_template.md | 15 +++++++++++++++ docs/engineering_standards.md | 7 +++++++ 2 files changed, 22 insertions(+) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 1156cb68..400e0721 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -21,6 +21,21 @@ --- +## Security-sensitive change checklist + + + +- [ ] Ran `/security-review` (note: by design it does not check for denial-of-service or resource-exhaustion issues — those need the next steps) +- [ ] Considered adversarial/misuse scenarios and checked the code against each (list them below) +- [ ] Added a regression test that fails against the pre-fix code for anything found this way +- [ ] Checked resource bounds: timeouts, cache eviction/max size, request/entry limits + +**Scenarios considered:** + + + +--- + ## Additional Notes diff --git a/docs/engineering_standards.md b/docs/engineering_standards.md index 8c89617c..bc07c44a 100644 --- a/docs/engineering_standards.md +++ b/docs/engineering_standards.md @@ -56,6 +56,13 @@ global.fetch = myCustomFetch; // ❌ Don't do this - **No Secrets in Code:** Never commit API keys, tokens, or secrets. Use environment variables and `.env` files (excluded from git). - **Dependency Updates:** Keep dependencies up to date and monitor for vulnerabilities. +- **Security-Sensitive Change Review:** For changes touching authentication, credentials, tokens, or session/connection state where an external user is involved, review needs more than a diff read: + 1. Run the `/security-review` skill. It's good at authentication, authorization, injection, and crypto issues, but its own instructions explicitly exclude denial-of-service and resource-exhaustion findings, so a clean result doesn't mean those are covered. + 2. Write down 2-3 concrete adversarial or misuse scenarios for the change (e.g. "what if this instance is shared across two concurrent sessions", "what if a client returns a value far larger than expected", "what if this token isn't verified the way we assume") and check the code against each one. + 3. Add a regression test that fails against the pre-fix code for anything found this way — proof the issue was real, not just "looks fixed." + 4. Get 2+ reviewer approvals as a floor, treated as a backstop rather than the primary defense. + + This doesn't apply to every PR, only ones where getting it wrong could expose one user's data or credentials to another, or let a shared resource be exhausted by external input. See `mcp-devkit-server` PR #57 for a 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, and were only caught by later, independently-run adversarial passes. ## 7. Automation