fix(hooks): reject internal session namespaces#55767
Conversation
Greptile SummaryThis PR closes a session-key namespace injection path by rejecting hook-provided session keys that target internal control namespaces ( Confidence Score: 5/5Safe to merge — the security boundary is fully enforced; all reserved-namespace injection paths are blocked at or before dispatch. All remaining findings are P2 style/consistency suggestions. The core security fix is correct, case-insensitive, and covered by unit + integration tests. No P0 or P1 issues found. No files require special attention; the single P2 note on mapping session key validation in hooks.ts is a hardening suggestion, not a blocking issue.
|
| Filename | Overview |
|---|---|
| src/gateway/hooks.ts | Adds HOOK_RESERVED_SESSION_KEY_PREFIXES and two helpers to strip the agent prefix before checking reserved namespaces. Validation applied to defaultSessionKey at config-load time and to request/mapping keys at dispatch time. Logic is correct and case-insensitive. |
| src/gateway/hooks.test.ts | Adds two unit tests: one for resolveHookSessionKey rejecting internal namespaces across request and mapping sources, one for resolveHooksConfig rejecting a reserved defaultSessionKey. Coverage is thorough for the new paths. |
| src/gateway/server.hooks.test.ts | Adds an integration test covering both the direct /hooks/agent request path and a mapped path with a reserved session key. Verifies HTTP 400 responses with the correct error message and that cronIsolatedRun is never called. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/gateway/hooks.ts
Line: 73-78
Comment:
**Config-load gap: mapping `sessionKey` values are not validated against reserved prefixes**
`defaultSessionKey` is now validated at config-load time (throws on startup), but static mapping `sessionKey` values with reserved namespaces (e.g. `"cron:daily"` or `"agent:main:subagent:worker"`) pass `resolveHookMappings` silently and are only caught by `resolveHookSessionKey` at request dispatch time. The security boundary is still intact — no reserved-namespace injection is possible — but a misconfigured mapping will silently load and then return HTTP 400 on every hit to that endpoint, which is harder to diagnose than a startup error.
Consider calling `resolveReservedHookSessionKeyPrefix` for each static mapping `sessionKey` inside `resolveHookMappings` and throwing early (the same pattern used for `defaultSessionKey`). Template-rendered keys (those containing `{{`) could be skipped since they can only be checked at runtime.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(hooks): reject internal session name..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const reservedDefaultSessionPrefix = resolveReservedHookSessionKeyPrefix(defaultSessionKey); | ||
| if (reservedDefaultSessionPrefix) { | ||
| throw new Error( | ||
| `hooks.defaultSessionKey may not target internal session namespace ${reservedDefaultSessionPrefix}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
sessionKey values are not validated against reserved prefixes
defaultSessionKey is now validated at config-load time (throws on startup), but static mapping sessionKey values with reserved namespaces (e.g. "cron:daily" or "agent:main:subagent:worker") pass resolveHookMappings silently and are only caught by resolveHookSessionKey at request dispatch time. The security boundary is still intact — no reserved-namespace injection is possible — but a misconfigured mapping will silently load and then return HTTP 400 on every hit to that endpoint, which is harder to diagnose than a startup error.
Consider calling resolveReservedHookSessionKeyPrefix for each static mapping sessionKey inside resolveHookMappings and throwing early (the same pattern used for defaultSessionKey). Template-rendered keys (those containing {{) could be skipped since they can only be checked at runtime.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/hooks.ts
Line: 73-78
Comment:
**Config-load gap: mapping `sessionKey` values are not validated against reserved prefixes**
`defaultSessionKey` is now validated at config-load time (throws on startup), but static mapping `sessionKey` values with reserved namespaces (e.g. `"cron:daily"` or `"agent:main:subagent:worker"`) pass `resolveHookMappings` silently and are only caught by `resolveHookSessionKey` at request dispatch time. The security boundary is still intact — no reserved-namespace injection is possible — but a misconfigured mapping will silently load and then return HTTP 400 on every hit to that endpoint, which is harder to diagnose than a startup error.
Consider calling `resolveReservedHookSessionKeyPrefix` for each static mapping `sessionKey` inside `resolveHookMappings` and throwing early (the same pattern used for `defaultSessionKey`). Template-rendered keys (those containing `{{`) could be skipped since they can only be checked at runtime.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Addressed in 37c84c6. Static hook mapping session keys now fail fast during config load when they target reserved internal namespaces, while templated keys still defer to runtime validation. I also added coverage for both the startup rejection path and the templated runtime-only path.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84f099fe81
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return value; | ||
| } | ||
| value = parsed.rest; |
There was a problem hiding this comment.
The new reserved-namespace guard can be bypassed by adding whitespace after an agent: unwrap boundary (for example agent:main: subagent:worker). parseAgentSessionKey() preserves that leading space in rest, and this function returns it unchanged, so startsWith("subagent:")/acp:/cron: does not match here even though downstream session classifiers trim and still treat it as an internal namespace. That means external hooks can still target internal control sessions despite this fix.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 783cc48. I now trim each unwrapped segment before checking reserved prefixes, so values like agent:main: subagent:worker are rejected the same way as the canonical form. Re-ran pnpm exec vitest run src/gateway/hooks.test.ts src/gateway/server.hooks.test.ts.
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Keep this PR open for review rather than cleanup: the diff is still useful security hardening for Gateway hook session routing, and I did not find a current-main implementation that already rejects the reserved namespaces. I found no blocking code-review defect, but the PR still needs real behavior proof and maintainer acceptance of the intentional compatibility/message-delivery break for existing hook configs that route into internal namespaces. Canonical path: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review. So I’m closing this here because the remaining work is already tracked in the canonical issue. Review detailsBest possible solution: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review. Do we have a high-confidence way to reproduce the issue? Yes. Current main source shows an allowlisted hook sessionKey is accepted by resolveHookSessionKey and then dispatched by the hook request handler; I did not run a live server repro in this read-only review. Is this the best way to solve the issue? Yes, with maintainer acceptance of the compatibility break. The PR centralizes the reserved-prefix check before config/runtime acceptance and preserves wake-mapping compatibility, but it still needs real behavior proof and upgrade context. Security review: Security review cleared: The diff is security-sensitive but improves the hook session-key boundary and does not add dependencies, secrets handling, downloads, workflow permissions, or new code-execution surfaces. AGENTS.md: found and applied where relevant. What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 421ea1f45806. |
783cc48 to
7fe2ae9
Compare
|
Refreshed this PR onto latest New head: What changed:
Review:
Verification:
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
openclaw-barnacle
Bot
added
the
triage: needs-real-behavior-proof
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Heads up: this PR needs to be updated against current |
7fe2ae9 to
83725fd
Compare
83725fd to
61467d8
Compare
|
ClawSweeper applied the proposed close for this PR.
|
2 participants
Summary
subagent:,acp:, orcron:namespaceshooks.defaultSessionKeyagainst the same reserved namespaces at config load timeWhy
Hook agent runs execute through the isolated cron path. If external hooks can choose an existing internal session namespace, the resulting run inherits session-key scoped control state intended only for internal subagent / ACP / cron flows.
Testing