From daf6479294061da8c90843334bdcf0540bca0dad Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Wed, 4 Nov 2026 13:58:04 -0800 Subject: [PATCH 1/2] docs(security): an OAuth callback with no session trusts only its state The pattern behind #3436: sign the state server-side with the user and resource, refuse anything else before the code exchange, bind what the callback hands back to that user, and still check authority over the resource at the write. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01MDs9DT6tbhKm6UhsmFhLk9 --- docs/security-patterns.md | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/docs/security-patterns.md b/docs/security-patterns.md index bd41fd314..66d84ae7c 100644 --- a/docs/security-patterns.md +++ b/docs/security-patterns.md @@ -18,6 +18,7 @@ shape. Sources at the bottom. | Regex capture from message content | Mongoose filter scalar OR `$in` | [Avoid putting it in the query at all](#dont-put-user-tainted-strings-in-mongoose-filters-period) | | `req.params.id` already validated by middleware| Mongoose `_id` filter | [Still cast](#objectidisvalid--cast-the-canonical-objectid-sanitizer) — CodeQL won't trust the middleware | | A URL from a request, or one a user stored | Server-side `fetch` / axios | [Named hosts, or public addresses only](#outbound-fetch-of-a-url-from-a-request) | +| An OAuth callback with no session | Anything it writes or mints for a resource | [Signed state that names the user](#an-oauth-callback-with-no-session-trusts-only-its-state) | --- @@ -140,6 +141,37 @@ Perf is acceptable as long as the scan-side dataset is bounded — `.limit(N)` is the lever. For pod-scoped reads, N=500 covers typical pod-file counts well above the 99th percentile. +## An OAuth callback with no session trusts only its state + +A provider redirect like `/api/discord/callback` arrives with no session. The +browser that lands there may not be the person who started the install. Its +query string (`code`, `state`, any ids) is whatever the link said, and anyone +can build that link. They authorize your app with their own account, then send +the resulting `code` together with *your* resource id. + +So the `state` must say who asked and for what, in a form only the server could +have produced: + +- **Mint it server-side** at the endpoint that already checked the caller's + authority over the resource (for Discord, `install-link`, creator or admin). + Sign it with a purpose claim, the resource id, the user id and a short expiry. +- **Refuse any other state before the code exchange.** A bare `pod_` is not + a state; it is a resource id anyone can type. +- **Carry the user into whatever the callback hands back** (an install proof, a + one-time token), and have every consumer accept it only from that user. +- **The final write still checks authority over the resource.** Guild or + account authority proves the far side, not the pod. A pod-level setting such + as a channel bridge takes the pod's own authority (creator or admin). +- Give each signed token its own `pur` claim. Readers check it, so a state can't + be read as a proof, and neither can be read as a session. + +Reference implementation: `backend/utils/discordInstallProof.ts` +(`mintDiscordInstallState` / `readDiscordInstallState`, and the user-bound +install proof), with the walk-through test in +`backend/__tests__/service/discordInstallBind.test.js`. Until #3436 the Discord +state was `pod_`. One click from a signed-in member on a crafted link could +bind their pod to an outside server, which could then read the pod's summary. + ## Anti-patterns ### Don't: `$in` with a user-tainted array @@ -173,6 +205,9 @@ through `String()` and bound length when logging user content. ## Sources +- PR #3436 — a signed, user-bound OAuth state for the Discord install, and + pod authority on the bind. + - PR #408 — first `String + replace + roundtrip` shape in the install endpoint, set the pattern. - PR #413 — the 5-iteration CodeQL battle that drove the From dd9b583d2db2061992ca1edf902fbb434f953b7b Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Wed, 4 Nov 2026 13:58:58 -0800 Subject: [PATCH 2/2] docs(security): review nits: sources order, cite the summary path, widen the header Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01MDs9DT6tbhKm6UhsmFhLk9 --- docs/security-patterns.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/docs/security-patterns.md b/docs/security-patterns.md index 66d84ae7c..711e6bed2 100644 --- a/docs/security-patterns.md +++ b/docs/security-patterns.md @@ -5,7 +5,8 @@ Reference doc for backend authors. Captures the patterns CodeQL's sanitizers, plus the anti-patterns that look safe but fail review. Updated when a CodeQL battle on a real PR uncovers a non-obvious -shape. Sources at the bottom. +shape, or a review finds an authorization shape worth reusing. Sources at +the bottom. --- @@ -170,7 +171,9 @@ Reference implementation: `backend/utils/discordInstallProof.ts` install proof), with the walk-through test in `backend/__tests__/service/discordInstallBind.test.js`. Until #3436 the Discord state was `pod_`. One click from a signed-in member on a crafted link could -bind their pod to an outside server, which could then read the pod's summary. +bind their pod to an outside server. That server's `/commonly summary` command +then answered with the pod's latest chat summary +(`discordCommandService.handleSummaryCommand`). ## Anti-patterns @@ -205,13 +208,12 @@ through `String()` and bound length when logging user content. ## Sources -- PR #3436 — a signed, user-bound OAuth state for the Discord install, and - pod authority on the bind. - - PR #408 — first `String + replace + roundtrip` shape in the install endpoint, set the pattern. - PR #413 — the 5-iteration CodeQL battle that drove the in-memory-Set refactor (and revealed regex captures are untrackable in our setup). +- PR #3436 — a signed, user-bound OAuth state for the Discord install, and + pod authority on the bind. - CodeQL JS query docs: https://codeql.github.com/codeql-query-help/javascript/js-nosql-injection/