Skip to content

Add security-sensitive change review checklist #133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mattpodwysocki wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add security-sensitive change review checklist #133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions .github/pull_request_template.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,21 @@

---

## 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. -->

@ctufts ctufts Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the distinction on external user?

@mattpodwysocki mattpodwysocki Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


- [ ] 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:**

<!-- 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" -->

---

## Additional Notes

<!-- Include any further details, follow-up items, or decisions relevant to the reviewer. -->
11 changes: 11 additions & 0 deletions docs/engineering_standards.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,17 @@ Observability is opt-in via OpenTelemetry. Enable only when debugging:
- Use Jaeger for local development: `npm run tracing:jaeger:start`
- See `docs/tracing.md` for detailed configuration

**Security-Sensitive Change Review:**

For changes that touch 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. PR #57 in this repo is 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.

**Docker:**

The project includes Docker support for containerized deployment:
Expand Down
Loading