fix(session): harden daemon HTTP requests#329
Conversation
Greptile SummaryThis PR hardens the local session daemon against cross-origin browser requests by adding Host and Origin header validation to all HTTP and WebSocket upgrade paths, and enforces
Confidence Score: 4/5The security hardening is correctly layered and well-tested; the two observations are minor edge cases that do not affect normal clients or browser-based DNS-rebinding attacks. The Host/Origin/content-type guards work correctly for their intended threat model. The duplicate content-type check inside parseJsonRequest is dead code that could mislead future maintainers, and the unconditional pass for port-less Host headers slightly weakens the port guard for crafted non-browser clients — but neither affects real-world browser-based attack scenarios. The parseHostAndPort + isAllowedHostPort logic in brokerServer.ts and the redundant guard in daemon.ts parseJsonRequest are the two spots worth a second look before merge. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant BunServe as Bun Serve Layer
participant Handler as handleRequest (brokerServer)
participant Daemon as SessionBrokerDaemon
Client->>BunServe: HTTP / WebSocket request
BunServe->>Handler: handleRequest(request)
Handler->>Handler: validateHostHeader(request, port, allowRemote)
alt Host missing or non-loopback
Handler-->>Client: 400 / 403 JSON error
end
Handler->>Handler: validateOriginHeader(request, port, allowRemote)
alt Origin present and non-local/wrong-port
Handler-->>Client: 403 JSON error
end
alt "pathname == /health or /session-api/capabilities"
Handler-->>Client: 200 JSON response
else "pathname == /session-api POST"
Handler->>Handler: hasJsonContentType?
alt "Content-Type != application/json"
Handler-->>Client: 415 JSON error
else
Handler->>Handler: parseJsonRequest + dispatch action
Handler-->>Client: 200 JSON response
end
else "pathname == /session WebSocket upgrade"
Handler-->>BunServe: undefined (defer to daemon)
BunServe->>Daemon: upgrade + handleConnectionMessage
Daemon-->>Client: WebSocket connection
end
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/session-broker/src/daemon.ts:66-68
**Dead code — content-type guard inside `parseJsonRequest` is never reached**
`handleApiRequest` already returns a 415 response before calling `parseJsonRequest`, so the `hasJsonContentType` throw on line 67 is unreachable from its only call site. If a future caller invokes `parseJsonRequest` without the pre-check, the error would be caught by the outer `try/catch` and surfaced as a generic 400 (not 415), silently misrepresenting the failure.
### Issue 2 of 2
src/session-broker/brokerServer.ts:144-151
**Port validation is skipped when the Host header omits the port**
`isAllowedHostPort` returns `true` whenever `hostPort.port === undefined`, regardless of the daemon's actual listening port. A crafted HTTP client that sends `Host: localhost` or `Host: 127.0.0.1` (without a port number) will pass this check even when the daemon is running on a non-standard port like 47657. DNS-rebinding attacks from browsers always include the port in the Host header, so real-world risk is low, but the port guard provides no defence against non-browser clients that omit the port.
Reviews (1): Last reviewed commit: "fix(session): harden daemon HTTP request..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!hasJsonContentType(request)) { | ||
| throw new Error("Expected Content-Type application/json."); | ||
| } |
There was a problem hiding this comment.
parseJsonRequest is never reached
handleApiRequest already returns a 415 response before calling parseJsonRequest, so the hasJsonContentType throw on line 67 is unreachable from its only call site. If a future caller invokes parseJsonRequest without the pre-check, the error would be caught by the outer try/catch and surfaced as a generic 400 (not 415), silently misrepresenting the failure.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/session-broker/src/daemon.ts
Line: 66-68
Comment:
**Dead code — content-type guard inside `parseJsonRequest` is never reached**
`handleApiRequest` already returns a 415 response before calling `parseJsonRequest`, so the `hasJsonContentType` throw on line 67 is unreachable from its only call site. If a future caller invokes `parseJsonRequest` without the pre-check, the error would be caught by the outer `try/catch` and surfaced as a generic 400 (not 415), silently misrepresenting the failure.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Removed the redundant content-type check from parseJsonRequest; the API handler remains responsible for returning the 415 before parsing.
Responded by Pi using OpenAI GPT-5.
| function isAllowedHostPort( | ||
| hostPort: { host: string; port?: number }, | ||
| expectedPort: number, | ||
| options: { allowRemote: boolean }, | ||
| ) { | ||
| const hostAllowed = options.allowRemote || isLoopbackHost(hostPort.host); | ||
| return hostAllowed && (hostPort.port === undefined || hostPort.port === expectedPort); | ||
| } |
There was a problem hiding this comment.
isAllowedHostPort returns true whenever hostPort.port === undefined, regardless of the daemon's actual listening port. A crafted HTTP client that sends Host: localhost or Host: 127.0.0.1 (without a port number) will pass this check even when the daemon is running on a non-standard port like 47657. DNS-rebinding attacks from browsers always include the port in the Host header, so real-world risk is low, but the port guard provides no defence against non-browser clients that omit the port.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/session-broker/brokerServer.ts
Line: 144-151
Comment:
**Port validation is skipped when the Host header omits the port**
`isAllowedHostPort` returns `true` whenever `hostPort.port === undefined`, regardless of the daemon's actual listening port. A crafted HTTP client that sends `Host: localhost` or `Host: 127.0.0.1` (without a port number) will pass this check even when the daemon is running on a non-standard port like 47657. DNS-rebinding attacks from browsers always include the port in the Host header, so real-world risk is low, but the port guard provides no defence against non-browser clients that omit the port.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Tightened host validation so an omitted Host port is treated as the HTTP default port and rejected unless it matches the daemon port. Added regression coverage for Host: 127.0.0.1 without a port.
Responded by Pi using OpenAI GPT-5.
1 participant
Summary
application/jsonfor session and raw broker API POST bodies.Tests
bun run format:checkbun run typecheckbun run lintbun test src/session-broker/brokerServer.test.ts packages/session-broker/src/daemon.test.ts packages/session-broker-bun/src/serve.test.ts packages/session-broker-node/src/serve.test.tsbun test ./src ./packages ./scripts ./test/cli ./test/sessionThis PR description was generated by Pi using OpenAI GPT-5