fix(gateway): block internal HTTP session overrides#55789
Conversation
Greptile SummaryThis PR adds a security guard that blocks Confidence Score: 5/5Safe to merge; the guard logic is correct and consistent with existing session-key semantics, and both HTTP endpoints are protected. The only open finding is a P2 test-coverage gap (missing acp: and cron: cases alongside the existing subagent: test), which does not affect correctness of the shipped guard. No P0 or P1 issues found. src/gateway/http-utils.request-context.test.ts — missing acp: and cron: prefix test cases.
|
| Filename | Overview |
|---|---|
| src/gateway/http-utils.ts | Adds resolveReservedSessionKeyPrefix / normalizeReservedSessionKeyCandidate helpers; throws from resolveSessionKey when a reserved prefix is matched. |
| src/gateway/openai-http.ts | Wraps resolveGatewayRequestContext in try/catch; returns 400 invalid_request_error when the session key guard throws. |
| src/gateway/openresponses-http.ts | Same try/catch guard as openai-http.ts applied to the OpenResponses handler. |
| src/gateway/http-utils.request-context.test.ts | Adds a test for the subagent: prefix rejection path; acp: and cron: prefixes have no corresponding test cases. |
| src/gateway/openai-http.test.ts | Adds an e2e block verifying 400 is returned for an internal subagent: session key and no agent command is dispatched. |
| src/gateway/openresponses-http.test.ts | Adds equivalent e2e coverage for the OpenResponses endpoint. |
Comments Outside Diff (1)
-
src/gateway/http-utils.request-context.test.ts, line 46-57 (link)Only
subagent:prefix tested,acp:andcron:cases uncoveredThe new test exercises only the
subagent:reserved prefix.RESERVED_SESSION_KEY_PREFIXESalso includesacp:andcron:, but there are no cases that verify those are blocked. A future refactor that drops or misspells one of those entries would break silently.Consider adding additional
itblocks (or parametrizing the existing one) to cover at leastacp:andcron::// acp: prefix { "x-openclaw-session-key": "agent:main:acp:session" } → throws "...internal session namespace acp:" // cron: prefix { "x-openclaw-session-key": "agent:main:cron:daily" } → throws "...internal session namespace cron:" // also test bare (non-agent-prefixed) variants e.g. "cron:daily"Prompt To Fix With AI
This is a comment left during a code review. Path: src/gateway/http-utils.request-context.test.ts Line: 46-57 Comment: **Only `subagent:` prefix tested, `acp:` and `cron:` cases uncovered** The new test exercises only the `subagent:` reserved prefix. `RESERVED_SESSION_KEY_PREFIXES` also includes `acp:` and `cron:`, but there are no cases that verify those are blocked. A future refactor that drops or misspells one of those entries would break silently. Consider adding additional `it` blocks (or parametrizing the existing one) to cover at least `acp:` and `cron:`: ``` // acp: prefix { "x-openclaw-session-key": "agent:main:acp:session" } → throws "...internal session namespace acp:" // cron: prefix { "x-openclaw-session-key": "agent:main:cron:daily" } → throws "...internal session namespace cron:" // also test bare (non-agent-prefixed) variants e.g. "cron:daily" ``` How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/gateway/http-utils.request-context.test.ts
Line: 46-57
Comment:
**Only `subagent:` prefix tested, `acp:` and `cron:` cases uncovered**
The new test exercises only the `subagent:` reserved prefix. `RESERVED_SESSION_KEY_PREFIXES` also includes `acp:` and `cron:`, but there are no cases that verify those are blocked. A future refactor that drops or misspells one of those entries would break silently.
Consider adding additional `it` blocks (or parametrizing the existing one) to cover at least `acp:` and `cron:`:
```
// acp: prefix
{ "x-openclaw-session-key": "agent:main:acp:session" } → throws "...internal session namespace acp:"
// cron: prefix
{ "x-openclaw-session-key": "agent:main:cron:daily" } → throws "...internal session namespace cron:"
// also test bare (non-agent-prefixed) variants e.g. "cron:daily"
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(gateway): block internal HTTP sessio..." | Re-trigger Greptile
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14fd898273
ℹ️ 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".
| sendJson(res, 400, { | ||
| error: { | ||
| message: err instanceof Error ? err.message : "Invalid session context.", | ||
| type: "invalid_request_error", |
There was a problem hiding this comment.
Avoid exposing internal exceptions as client input errors
This catch now maps all resolveGatewayRequestContext failures to a 400 and returns err.message verbatim, but that context path calls resolveAgentIdForRequest() which reads config and can throw internal errors (for example loadConfig() throws detailed Invalid config at <path>... messages in src/config/io.ts). In that scenario, API callers receive server internals (filesystem path + validation details) as invalid_request_error, and a server-side failure is misclassified as a client error. Please only convert the reserved-session override failure to 400 and keep unexpected exceptions as generic 500s.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 825e21b. The reserved-prefix rejection now throws a dedicated GatewaySessionKeyOverrideError, and the OpenAI/OpenResponses handlers only translate that specific error to a 400 invalid_request_error. Unexpected context/config failures now fall through instead of leaking internal messages as client input errors.
|
Codex review: needs real behavior proof before merge. Reviewed June 1, 2026, 8:36 AM ET / 12:36 UTC. Summary PR surface: Source +67, Tests +70. Total +137 across 6 files. Reproducibility: unclear. The review failed before ClawSweeper could establish a reproduction path. Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Risk before merge
Maintainer options:
Next step before merge
Review detailsBest possible solution: Retry the Codex review after fixing the execution failure. Do we have a high-confidence way to reproduce the issue? Unclear. The review failed before ClawSweeper could establish a reproduction path. Is this the best way to solve the issue? Unclear. Retry the review first so ClawSweeper can evaluate the actual issue and fix direction. AGENTS.md: unclear because the file could not be read completely. Codex review notes: model gpt-5.5, reasoning high; reviewed against ec2455a842cf. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +67, Tests +70. Total +137 across 6 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
825e21b to
e58b389
Compare
|
Refreshed this PR onto latest New head: What changed:
Review:
Verification:
|
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
9eaa1fa to
868a0c4
Compare
Summary
x-openclaw-session-keyoverrides that target internal namespaces likesubagent:,acp:, orcron:invalid_request_errorfrom the OpenAI and OpenResponses HTTP endpoints instead of passing those keys into owner-trusted ingressTesting