fix(session): harden browser request checks#335
Conversation
Greptile SummaryThis PR adds layered browser-request hardening to the local session daemon: Fetch Metadata checks block cross-site/same-site browser traffic, a secondary check catches browser-looking requests (Sec-* headers) that omit an Origin, and a new
Confidence Score: 4/5Safe to merge; the request-validation logic is correct and the tests cover the new rejection paths end-to-end. The three new validator functions work together correctly — the interaction where sec-fetch-site: none passes validateFetchMetadata but is then caught by validateBrowserRequestOrigin is intentional and sound. The only gaps are two misleading JSDoc comment phrasings and a missing positive-case assertion for allowed Fetch-Metadata values, which could mask a future regression in the allow-list logic. brokerServer.test.ts would benefit from positive-case tests confirming that sec-fetch-site: none and sec-fetch-site: same-origin are permitted through validateFetchMetadata. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
REQ([Incoming Request]) --> HOST[validateHostHeader]
HOST -->|error| R400[400/403]
HOST -->|pass| ORIGIN[validateOriginHeader]
ORIGIN -->|error| R403A[403]
ORIGIN -->|pass| FM[validateFetchMetadata\nsec-fetch-site present?]
FM -->|none or same-origin| BRO[validateBrowserRequestOrigin\nSec-* headers w/o Origin?]
FM -->|cross-site / same-site / unknown| R403B[403]
FM -->|absent| BRO
BRO -->|Sec-* headers, no Origin| R403C[403]
BRO -->|no Sec-* headers, or Origin present| ROUTE{Route}
ROUTE -->|/health| HEALTH[200 Health JSON]
ROUTE -->|/session-api/capabilities| CAPS[200 Capabilities JSON]
ROUTE -->|/session-api POST| CLIHDR[validateSessionClientHeader\nx-hunk-session-client?]
ROUTE -->|WebSocket| WS[WebSocket Handler]
CLIHDR -->|missing/wrong| R403D[403]
CLIHDR -->|hunk-cli| CT[hasJsonContentType?]
CT -->|no| R415[415]
CT -->|yes| ACTION[Process Action]
ACTION --> RESP([Response])
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
src/session-broker/brokerServer.ts:101-102
The JSDoc comments on the two new validators use "Return whether..." phrasing, which implies a boolean return value. Both functions actually return `Response | null` (a pattern consistent with every other validator in this file). The existing validators (`validateHostHeader`, `validateOriginHeader`) use action-oriented "Block..." phrasing that makes the semantics clear.
```suggestion
/** Block requests whose Sec-Fetch-Site indicates cross-site or same-site browser traffic. */
function validateFetchMetadata(request: Request) {
```
### Issue 2 of 3
src/session-broker/brokerServer.ts:115-116
Same "Return whether..." phrasing issue as `validateFetchMetadata`. The function rejects the request (returns a 403 Response) rather than returning a boolean predicate.
```suggestion
/** Block browser-looking requests that carry Sec-* headers but omit an Origin. */
function validateBrowserRequestOrigin(request: Request) {
```
### Issue 3 of 3
src/session-broker/brokerServer.test.ts:382-420
**Missing positive-case coverage for allowed Fetch-Metadata values**
The test verifies that `cross-site` (HTTP) and `same-site` (WebSocket) are rejected, but there are no assertions that `sec-fetch-site: none` or `sec-fetch-site: same-origin` are explicitly allowed through `validateFetchMetadata`. Because `sec-fetch-site: none` is allowed by `validateFetchMetadata` but then caught by `validateBrowserRequestOrigin` (no Origin + Sec-* header present), the two functions interact subtly. A regression that accidentally tightens `validateFetchMetadata` to also block `none` would be invisible without a positive test exercising that path.
Reviews (1): Last reviewed commit: "fix(session): harden browser request che..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /** Return whether Fetch Metadata identifies this request as browser cross-site traffic. */ | ||
| function validateFetchMetadata(request: Request) { |
There was a problem hiding this comment.
Response | null (a pattern consistent with every other validator in this file). The existing validators (validateHostHeader, validateOriginHeader) use action-oriented "Block..." phrasing that makes the semantics clear.
| /** Return whether Fetch Metadata identifies this request as browser cross-site traffic. */ | |
| function validateFetchMetadata(request: Request) { | |
| /** Block requests whose Sec-Fetch-Site indicates cross-site or same-site browser traffic. */ | |
| function validateFetchMetadata(request: Request) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/session-broker/brokerServer.ts
Line: 101-102
Comment:
The JSDoc comments on the two new validators use "Return whether..." phrasing, which implies a boolean return value. Both functions actually return `Response | null` (a pattern consistent with every other validator in this file). The existing validators (`validateHostHeader`, `validateOriginHeader`) use action-oriented "Block..." phrasing that makes the semantics clear.
```suggestion
/** Block requests whose Sec-Fetch-Site indicates cross-site or same-site browser traffic. */
function validateFetchMetadata(request: Request) {
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Updated the comment to use action-oriented wording that matches the validator's Response/null behavior.
This comment was generated by Pi using GPT-5
| /** Return whether one request carries browser-only headers but omits an Origin. */ | ||
| function validateBrowserRequestOrigin(request: Request) { |
There was a problem hiding this comment.
validateFetchMetadata. The function rejects the request (returns a 403 Response) rather than returning a boolean predicate.
| /** Return whether one request carries browser-only headers but omits an Origin. */ | |
| function validateBrowserRequestOrigin(request: Request) { | |
| /** Block browser-looking requests that carry Sec-* headers but omit an Origin. */ | |
| function validateBrowserRequestOrigin(request: Request) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/session-broker/brokerServer.ts
Line: 115-116
Comment:
Same "Return whether..." phrasing issue as `validateFetchMetadata`. The function rejects the request (returns a 403 Response) rather than returning a boolean predicate.
```suggestion
/** Block browser-looking requests that carry Sec-* headers but omit an Origin. */
function validateBrowserRequestOrigin(request: Request) {
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Updated this comment as well so it describes the blocking behavior instead of implying a boolean predicate.
This comment was generated by Pi using GPT-5
| } | ||
| }); | ||
|
|
||
| test("rejects browser Fetch Metadata for HTTP and websocket requests", async () => { | ||
| const port = await reserveLoopbackPort(); | ||
| process.env.HUNK_MCP_HOST = "127.0.0.1"; | ||
| process.env.HUNK_MCP_PORT = String(port); | ||
|
|
||
| const server = serveSessionBrokerDaemon(); | ||
|
|
||
| try { | ||
| const response = await fetch(`http://127.0.0.1:${port}/health`, { | ||
| headers: { "sec-fetch-site": "cross-site" }, | ||
| }); | ||
| expect(response.status).toBe(403); | ||
| await expect(response.json()).resolves.toEqual({ | ||
| error: "Browser request metadata is not allowed for the local session broker.", | ||
| }); | ||
|
|
||
| const handshake = await readRawWebSocketHandshake(port, ["Sec-Fetch-Site: same-site"]); | ||
| expect(handshake).toStartWith("HTTP/1.1 403"); | ||
| } finally { | ||
| server.stop(true); | ||
| } | ||
| }); | ||
|
|
||
| test("rejects browser-looking requests that omit Origin", async () => { | ||
| const port = await reserveLoopbackPort(); | ||
| process.env.HUNK_MCP_HOST = "127.0.0.1"; | ||
| process.env.HUNK_MCP_PORT = String(port); | ||
|
|
||
| const server = serveSessionBrokerDaemon(); | ||
|
|
||
| try { | ||
| const response = await fetch(`http://127.0.0.1:${port}/health`, { | ||
| headers: { | ||
| "sec-fetch-mode": "navigate", | ||
| "sec-fetch-site": "none", | ||
| }, |
There was a problem hiding this comment.
The test verifies that cross-site (HTTP) and same-site (WebSocket) are rejected, but there are no assertions that sec-fetch-site: none or sec-fetch-site: same-origin are explicitly allowed through validateFetchMetadata. Because sec-fetch-site: none is allowed by validateFetchMetadata but then caught by validateBrowserRequestOrigin (no Origin + Sec-* header present), the two functions interact subtly. A regression that accidentally tightens validateFetchMetadata to also block none would be invisible without a positive test exercising that path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/session-broker/brokerServer.test.ts
Line: 382-420
Comment:
**Missing positive-case coverage for allowed Fetch-Metadata values**
The test verifies that `cross-site` (HTTP) and `same-site` (WebSocket) are rejected, but there are no assertions that `sec-fetch-site: none` or `sec-fetch-site: same-origin` are explicitly allowed through `validateFetchMetadata`. Because `sec-fetch-site: none` is allowed by `validateFetchMetadata` but then caught by `validateBrowserRequestOrigin` (no Origin + Sec-* header present), the two functions interact subtly. A regression that accidentally tightens `validateFetchMetadata` to also block `none` would be invisible without a positive test exercising that path.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Added positive coverage for both allowed Fetch Metadata values with a local Origin, so the test now verifies that same-origin and none pass through while cross-site/same-site remain blocked.
This comment was generated by Pi using GPT-5
benvinegar
force-pushed
the
fix/session-browser-csrf-hardening
branch
from
045b024 to
40e0ced
Compare
|
Eh, this might be overkill. To be revisited. |
1 participant
Summary
cross-site/same-site) before they reach HTTP or websocket handlers.Sec-*browser headers but omitOrigin.x-hunk-session-client: hunk-clion/session-apiposts, and send it from the Hunk session CLI.Validation
bun run typecheckbun run format:checkbun run lintbun testhunk session list/context/review/navigate/comment./session-apiPOST without marker header returns403; same request withx-hunk-session-client: hunk-clireturns200.This PR description was generated by Pi using GPT-5