fix(gateway): block internal session history over HTTP#55928
fix(gateway): block internal session history over HTTP#55928RichardCao wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a175b6cd2f
ℹ️ 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".
|
|
||
| function isInternalSessionHistoryTarget(sessionKey: string): boolean { | ||
| return ( | ||
| isSubagentSessionKey(sessionKey) || isCronSessionKey(sessionKey) || isAcpSessionKey(sessionKey) |
There was a problem hiding this comment.
Block bare cron keys before serving HTTP history
isInternalSessionHistoryTarget relies on isCronSessionKey(sessionKey), but that helper only returns true for already agent-scoped keys; a request like GET /sessions/cron:daily/history passes this guard. The handler then canonicalizes via resolveGatewaySessionStoreTarget to agent:<default>:cron:daily and will return that internal transcript, so the new HTTP block can be bypassed for cron sessions. This affects any deployment where internal cron sessions exist and callers can hit the history endpoint with a bare cron:* key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bc5f5b6. The HTTP guard now rejects both raw and canonicalized internal session keys, so bare aliases like cron:daily are denied before history can be served. Added regression coverage for the raw cron:* path in src/gateway/sessions-history-http.test.ts, and re-ran pnpm exec vitest run src/gateway/sessions-history-http.test.ts.
Greptile SummaryThis PR adds a targeted HTTP-level guard that rejects session history requests whose session key belongs to an internal namespace (subagent, cron, or ACP). The check is placed correctly in the request pipeline — after bearer auth and scope authorization, and before any session store I/O — so internal transcript data is never read for blocked requests. The implementation delegates cleanly to the existing
Confidence Score: 5/5Safe to merge — the HTTP guard is correctly implemented with no logic errors or regressions. The only finding is a P2 observation about the WebSocket surface being outside the stated scope of this PR. The HTTP fix itself is correct, well-tested, and does not introduce any new issues. No files require special attention; the WebSocket gap exists in
|
| Filename | Overview |
|---|---|
| src/gateway/sessions-history-http.ts | Adds isInternalSessionHistoryTarget helper and a 403 guard placed correctly after auth/scope checks. Logic and error shape are consistent with the rest of the handler. |
| src/gateway/sessions-history-http.test.ts | Adds seedNamedSession helper and a test.each covering all three internal namespaces (subagent, cron, ACP). Tests correctly verify that sessions with real data still return 403, not 404, confirming the block fires before the store read. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/gateway/sessions-history-http.ts
Line: 196-205
Comment:
**WebSocket `chat.history` has no equivalent guard**
The HTTP path is now protected, but the WebSocket `chat.history` handler in `src/gateway/server-methods/chat.ts` (around line 1095) calls `loadSessionEntry(sessionKey)` directly — without any check against `isInternalSessionHistoryTarget`. An authenticated client with `operator.read` scope can still retrieve subagent, cron, or ACP session history over the WebSocket surface.
If the intent is to block all external access to internal session history, the same `isInternalSessionHistoryTarget` guard should be applied in the WS handler. If the WS surface is intentionally left unrestricted (e.g., internal-only callers), a comment explaining that distinction would help future readers.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(gateway): block internal session his..." | Re-trigger Greptile
| if (isInternalSessionHistoryTarget(sessionKey)) { | ||
| sendJson(res, 403, { | ||
| ok: false, | ||
| error: { | ||
| type: "forbidden", | ||
| message: "internal sessions are not available over HTTP history", | ||
| }, | ||
| }); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
chat.history has no equivalent guard
The HTTP path is now protected, but the WebSocket chat.history handler in src/gateway/server-methods/chat.ts (around line 1095) calls loadSessionEntry(sessionKey) directly — without any check against isInternalSessionHistoryTarget. An authenticated client with operator.read scope can still retrieve subagent, cron, or ACP session history over the WebSocket surface.
If the intent is to block all external access to internal session history, the same isInternalSessionHistoryTarget guard should be applied in the WS handler. If the WS surface is intentionally left unrestricted (e.g., internal-only callers), a comment explaining that distinction would help future readers.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/sessions-history-http.ts
Line: 196-205
Comment:
**WebSocket `chat.history` has no equivalent guard**
The HTTP path is now protected, but the WebSocket `chat.history` handler in `src/gateway/server-methods/chat.ts` (around line 1095) calls `loadSessionEntry(sessionKey)` directly — without any check against `isInternalSessionHistoryTarget`. An authenticated client with `operator.read` scope can still retrieve subagent, cron, or ACP session history over the WebSocket surface.
If the intent is to block all external access to internal session history, the same `isInternalSessionHistoryTarget` guard should be applied in the WS handler. If the WS surface is intentionally left unrestricted (e.g., internal-only callers), a comment explaining that distinction would help future readers.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Addressed in bc5f5b6. I reused the internal-session guard on the WebSocket chat.history path as well, so operator.read callers can no longer read subagent / cron / ACP transcripts over WS either. Added WS regression coverage in src/gateway/sessions-history-http.test.ts and re-ran pnpm exec vitest run src/gateway/sessions-history-http.test.ts src/gateway/server.chat.gateway-server-chat.test.ts src/gateway/server.chat.gateway-server-chat-b.test.ts.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc5f5b6e7e
ℹ️ 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".
| sessionKey: string; | ||
| limit?: number; | ||
| }; | ||
| if (isInternalGatewaySessionKey(sessionKey)) { |
There was a problem hiding this comment.
Preserve internal chat.history access for subagent flows
This new unconditional rejection blocks all internal namespace keys on WebSocket chat.history, but internal runtime features still rely on that method with subagent keys. For example, sendControlledSubagentMessage calls chat.history for targetSessionKey before and after a steer attempt (src/agents/subagent-control.ts:904 and src/agents/subagent-control.ts:945), and those keys are created as agent:<id>:subagent:<uuid> (src/agents/subagent-spawn.ts:411). With this guard, those calls now return INVALID_REQUEST, so subagent control/steer paths can fail even for authorized internal operators.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted. That WebSocket guard was too broad because internal subagent-control flows still depend on chat.history for subagent session keys. I reverted the WS-side rejection in c66e932 and narrowed this PR back to the HTTP surface only, while keeping the raw/canonical internal HTTP history block and its regressions. Re-ran pnpm exec vitest run src/gateway/sessions-history-http.test.ts src/gateway/server.chat.gateway-server-chat.test.ts src/gateway/server.chat.gateway-server-chat-b.test.ts.
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Keep open. Current main still has no internal-namespace denial on the HTTP session-history path, while the PR's potential merge commit adds the focused 403 guard and regression coverage; the remaining blockers are real gateway proof, documentation of the new exception, and maintainer acceptance of the compatibility break. Canonical path: Close this PR as superseded by #81815. So I’m closing this here and keeping the remaining discussion on #81815. Review detailsBest possible solution: Close this PR as superseded by #81815. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection on current main shows the HTTP history handler resolves and loads the target store without an internal-namespace denial, so seeding an internal session row and calling Is this the best way to solve the issue? Mostly yes. The runtime guard is narrow and placed before transcript reads while preserving WebSocket runtime flows; the patch should also document the new public HTTP exception and include real gateway proof. Security review: Security review cleared: The diff tightens an existing gateway HTTP access-control path and does not add dependency, workflow, lockfile, secret, download, publish, or code-execution surface changes. What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 483d7be6c40a. |
c66e932 to
b5d0a08
Compare
|
Refreshed this PR onto latest New head: What changed:
Review:
Verification:
|
|
Follow-up after the latest push: New head is now I added the remaining review-driven coverage for the access-control discussion:
Response-shape decision remains intentionally split from #55789:
Local targeted test still cannot execute assertions in this shell because |
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
ClawSweeper applied the proposed close for this PR.
|
Summary
Testing