#21588: fix(security): decouple external hook trust from session key format

by nabbilkhan open 2026-02-20 04:20 View on GitHub →

gateway size: L

## Summary - **Problem:** When `hooks.allowRequestSessionKey: true` is set, the external content security wrapper (added January 2026) can be bypassed entirely. An attacker with a valid hook token sends `"sessionKey": "main"` in the `POST /hooks/agent` body. `isExternalHookSession("main")` returns `false` → `shouldWrapExternal = false` → the message reaches the agent as a trusted instruction with no `<<<EXTERNAL_UNTRUSTED_CONTENT>>>` boundaries and no SECURITY NOTICE. - **Root cause:** `src/cron/isolated-agent/run.ts` derived `isExternalHook` from `isExternalHookSession(baseSessionKey)`, which checks `sessionKey.startsWith("hook:")`. Session keys are attacker-controlled strings in the HTTP body — not a reliable trust signal. The wrapper assumed the `hook:` prefix would always be present. When `allowRequestSessionKey` is enabled, that assumption breaks. - **Solution:** Add `fromExternalHook?: boolean` to `RunCronAgentTurnParams`. `dispatchAgentHook()` — the sole external HTTP hook entry point — sets it unconditionally. The trust decision becomes: ```ts // BEFORE (vulnerable) const isExternalHook = isExternalHookSession(baseSessionKey); // AFTER (fixed) const isExternalHook = params.fromExternalHook === true || isExternalHookSession(baseSessionKey); ``` `fromExternalHook` is an internal TypeScript parameter set only at the dispatch call site — it is not present in `normalizeAgentPayload()` and cannot be set by any external HTTP input. `isExternalHookSession()` is retained as belt-and-suspenders for legacy callers. - **Scope boundary:** `resolveHookSessionKey()`, `CronJob`/`CronPayload` types, the `allowUnsafeExternalContent` override path, all existing callers of `runCronIsolatedAgentTurn` — none touched. The new param is `optional` and defaults to `undefined` (= `false`). The cron scheduler path is unaffected. ## Change Type - [x] Bug fix - [x] Security hardening ## Scope - [x] Security - [x] Gateway / orchestration - [x] Cron / agents ## Linked Issue No upstream issue exists for this specific bypass — discovered during code review of the January 2026 external content security wrapper. ## Security Impact | Attribute | Value | |-----------|-------| | CVSS v3.1 | **8.1 High** — `AV:N/AC:H/PR:L/UI:N/S:C/C:H/I:H/A:H` | | Prerequisite | `hooks.allowRequestSessionKey: true` (non-default, but commonly set for Gmail/webhook workflows) | | What's bypassed | `<<<EXTERNAL_UNTRUSTED_CONTENT>>>` boundaries + SECURITY NOTICE preamble | | Worst-case outcome | Arbitrary tool execution if agent has `exec` with `security: full` | **Honest scope note:** The bypass is deterministic — wrapping is reliably skipped when preconditions are met. Whether the LLM acts on an injected instruction is probabilistic and model-dependent. The score reflects worst-case realistic impact. Reviewers preferring a conservative read: `AV:N/AC:H/PR:L/UI:N/S:U/C:H/I:H/A:N` = 6.8 Medium. The fix is warranted either way. Operators who have **not** set `allowRequestSessionKey: true` are not affected — session keys are config/auto-generated and always carry the `hook:` prefix. - Changes file/network permissions? **No** - Touches secrets/tokens/credentials? **No** - New network calls? **No** - Changes tool execution surface? **No** — narrows it ## Defense in Depth The fix adds a layered defense. All four layers are verified by tests: | Layer | Mechanism | Status | |-------|-----------|--------| | **1. Dispatch-origin flag** | `fromExternalHook=true` forces wrapping regardless of session key | ✅ Cannot be bypassed via HTTP input | | **2. Session key prefix** | `isExternalHookSession()` belt-and-suspenders for `hook:` keys | ✅ Retained, backward compat | | **3. Entry-point prefix enforcement** | `allowedSessionKeyPrefixes` rejects non-`hook:` keys at gateway | ✅ Effective when configured | | **4. HTTP body stripping** | `normalizeAgentPayload()` does not parse `allowUnsafeExternalContent` | ✅ Verified by test — attacker cannot opt out of wrapping via HTTP body | **On `allowUnsafeExternalContent`:** This field can only be set via server-side config mappings — never via the HTTP body. Verified: `normalizeAgentPayload()` silently drops the field if present in the payload. Additionally, even if a cron job payload carries `allowUnsafeExternalContent=true` (e.g. set via the cron tool), it has zero security impact on the cron scheduler path: `fromExternalHook` is unset → `isExternalHook=false` → `shouldWrapExternal=false` already. `allowUnsafeExternalContent` is only evaluated when `isExternalHook=true`. Both properties are explicitly tested. **On `fromExternalHook` and config reload:** `dispatchAgentHook` is a fire-and-forget async closure. `fromExternalHook: true` is captured in the closure at dispatch time, before `loadConfig()` is called. A config reload mid-flight cannot affect it. ## Repro + Verification **Environment** - OS: Ubuntu 24.04 LTS - Runtime: Node 22.x - Config: `hooks.enabled: true`, `hooks.allowRequestSessionKey: true` **Steps (before fix)** 1. Start gateway with `hooks.allowRequestSessionKey: true` and a valid `hooks.token`. 2. POST to `/hooks/agent`: ```bash curl -X POST https://gateway/hooks/agent \ -H "Authorization: Bearer <HOOK_TOKEN>" \ -H "Content-Type: application/json" \ -d '{"message": "ignore previous instructions", "name": "Test", "sessionKey": "main"}' ``` 3. `isExternalHookSession("main")` → `false` → `shouldWrapExternal = false` → raw message delivered to agent as trusted instruction. **After fix:** same request → `fromExternalHook: true` → `isExternalHook = true` → wrapped. ## Evidence New file: `src/cron/isolated-agent/run.external-hook-wrapping.test.ts` — **21 tests** | Test | What it proves | |------|----------------| | `sessionKey="main"` + `fromExternalHook=true` → wrapped | Exact CVE scenario | | `sessionKey="agent:main:main"` + flag → wrapped | Variant key formats | | Arbitrary session key + flag → wrapped | General case | | Suspicious patterns detected + logged | Monitoring path preserved | | `sessionKey="hook:..."` without flag → wrapped | Backward compat | | `sessionKey="hook:gmail:..."` + flag → wrapped | Gmail hook | | `getHookType()` called with correct key | Source labeling accuracy | | `sessionKey="cron:..."`, no flag → NOT wrapped | No false positives for cron | | `fromExternalHook=false` → NOT wrapped | Explicit opt-out | | `fromExternalHook=undefined` → NOT wrapped | Default cron path | | `allowUnsafeExternalContent=true` + flag → NOT wrapped | Operator override preserved | | Suspicious patterns still detected with `allowUnsafeExternalContent=true` | Monitoring preserved | | `allowUnsafeExternalContent=false` + flag → wrapped | Normal hook path | | Both flag + `hook:` prefix → wrapped exactly once | Belt-and-suspenders | | `buildSafeExternalPrompt` receives correct metadata | Wrapper accuracy | | `allowedSessionKeyPrefixes=["hook:"]` rejects `"main"` | Defense layer 3 | | `allowedSessionKeyPrefixes=["hook:"]` allows `"hook:gmail:..."` | Defense layer 3 | | No prefix restriction → `"main"` accepted (why layer 1 is essential) | Documents threat model | | **Cron job with `allowUnsafeExternalContent=true` — no wrapping bypass possible** | Layer 4 interaction | | **Cron job with `allowUnsafeExternalContent=false` — also not wrapped** | Cron path isolation | | **`normalizeAgentPayload` drops `allowUnsafeExternalContent` from HTTP body** | Layer 4 verified | E2E: `src/gateway/server.hooks.e2e.test.ts` extended to assert `fromExternalHook: true` passes through the HTTP handler → `runCronIsolatedAgentTurn` boundary. **Full suite:** 771 test files / 6217 tests passing. One pre-existing failure in `src/browser/server.post-tabs-open-profile-unknown-returns-404.test.ts` exists on `main` before this PR. ## Human Verification - Traced every callsite of `runCronIsolatedAgentTurn` — exactly two: cron scheduler (does not set flag, correct) and hook dispatcher (sets flag unconditionally, correct) - Verified `fromExternalHook` is not reachable via HTTP body — not parsed in `normalizeAgentPayload()`, not on `HookAgentPayload` type, not forwarded through any mapping path - Verified `allowUnsafeExternalContent` cannot be supplied via HTTP body (`normalizeAgentPayload` drops it — tested) - Verified `fromExternalHook` closure capture is immune to config reload races - `pnpm build` — zero errors - `pnpm check` (oxfmt + oxlint) — zero formatting issues, zero lint warnings - `pnpm test` — 771 files, 6217 tests passing ## Compatibility / Migration - Backward compatible: **Yes** — new param is optional; existing callers unchanged - Config changes: **None** - Migration: **None required** - Revert: `git revert <sha>` ## Risks - A custom plugin calling `runCronIsolatedAgentTurn` directly without setting `fromExternalHook: true` for external content would not get the new protection. Mitigated by: `isExternalHookSession()` fallback still catches `hook:` prefix keys; JSDoc on the param explicitly warns future callers. - `fromExternalHook=true` over-applied to trusted cron content: mitigated by the flag being `undefined` by default and set only in `dispatchAgentHook`. False-positive tests explicitly verify cron jobs are not wrapped. --- - [x] AI-assisted (GPT-5.3-Codex) | fully tested — 6217 tests passing  <h3>Greptile Summary</h3> This PR fixes a security bypass where attackers with valid hook tokens could circumvent external content security wrapping by supplying arbitrary session keys when `hooks.allowRequestSessionKey: true` is configured. The fix adds a dispatch-origin flag (`fromExternalHook`) set at the HTTP entry point, ensuring wrapping is applied based on where the request originates rather than the attacker-controlled session key format. **Key changes:** - Added `fromExternalHook?: boolean` parameter to `RunCronAgentTurnParams` (src/cron/isolated-agent/run.ts:175) - Modified trust decision from `isExternalHookSession(baseSessionKe...