Skip to content

fix(session): harden browser request checks #335

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
benvinegar wants to merge 1 commit into from
Closed

fix(session): harden browser request checks #335

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ All notable user-visible changes to Hunk are documented in this file.

### Fixed

- Hardened the local session daemon against browser-originated requests by validating Host and Origin headers and requiring JSON content types for API posts.
- Hardened the local session daemon against browser-originated requests by validating Host, Origin, and Fetch Metadata headers, requiring JSON content types, and requiring the Hunk CLI marker header for API posts.
- Disabled the generic broker HTTP API by default so Hunk's supported session API is the only app-daemon command surface.
- Bounded session daemon memory by capping HTTP request body and websocket message sizes and rejecting session registrations with oversized file, hunk, patch, comment, or note payloads.

Expand Down
1 change: 1 addition & 0 deletions docs/agent-workflows.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ If `hunk session list` reports no sessions while Hunk is visibly running, the ag
```bash
curl -s -X POST http://127.0.0.1:47657/session-api \
-H 'content-type: application/json' \
-H 'x-hunk-session-client: hunk-cli' \
--data '{"action":"list"}'
```

Expand Down
5 changes: 5 additions & 0 deletions src/hunk-session/cli.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import type { SessionSelectorInput } from "../core/types";
import {
HUNK_SESSION_API_PATH,
HUNK_SESSION_API_VERSION,
HUNK_SESSION_CLIENT_HEADER,
HUNK_SESSION_CLIENT_HEADER_VALUE,
HUNK_SESSION_DAEMON_VERSION,
} from "../session/protocol";
import {
Expand Down Expand Up @@ -105,6 +107,9 @@ describe("HTTP Hunk session CLI client", () => {

expect(url).toEndWith(HUNK_SESSION_API_PATH);
expect(init?.method).toBe("POST");
expect(new Headers(init?.headers).get(HUNK_SESSION_CLIENT_HEADER)).toBe(
HUNK_SESSION_CLIENT_HEADER_VALUE,
);
const request = JSON.parse(String(init?.body));
requests.push(request);
return Response.json(responses[request.action as keyof typeof responses]);
Expand Down
3 changes: 3 additions & 0 deletions src/hunk-session/cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import type { SessionTerminalLocation, SessionTerminalMetadata } from "@hunk/ses
import { readHunkSessionDaemonCapabilities } from "../session/capabilities";
import {
HUNK_SESSION_API_PATH,
HUNK_SESSION_CLIENT_HEADER,
HUNK_SESSION_CLIENT_HEADER_VALUE,
type SessionDaemonCapabilities,
type SessionDaemonRequest,
} from "../session/protocol";
Expand Down Expand Up @@ -70,6 +72,7 @@ class HttpHunkSessionCliClient implements HunkSessionCliClient {
method: "POST",
headers: {
"content-type": "application/json",
[HUNK_SESSION_CLIENT_HEADER]: HUNK_SESSION_CLIENT_HEADER_VALUE,
},
body: JSON.stringify(input),
});
Expand Down
126 changes: 107 additions & 19 deletions src/session-broker/brokerServer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,18 @@ import {
createTestSessionSnapshot,
} from "../../test/helpers/session-daemon-fixtures";
import { SessionBrokerState } from "@hunk/session-broker-core";
import { HUNK_SESSION_CLIENT_HEADER, HUNK_SESSION_CLIENT_HEADER_VALUE } from "../session/protocol";
import { serveSessionBrokerDaemon } from "./brokerServer";

const originalHost = process.env.HUNK_MCP_HOST;
const originalPort = process.env.HUNK_MCP_PORT;
const originalUnsafeRemote = process.env.HUNK_MCP_UNSAFE_ALLOW_REMOTE;

const SESSION_API_HEADERS = {
"content-type": "application/json",
[HUNK_SESSION_CLIENT_HEADER]: HUNK_SESSION_CLIENT_HEADER_VALUE,
};

interface HealthResponse {
ok: boolean;
pid: number;
Expand Down Expand Up @@ -291,7 +297,7 @@ describe("Hunk session daemon server", () => {
expect(capabilities.status).toBe(200);
await expect(capabilities.json()).resolves.toMatchObject({
version: 1,
daemonVersion: 3,
daemonVersion: 4,
actions: [
"list",
"get",
Expand Down Expand Up @@ -376,6 +382,98 @@ describe("Hunk session daemon server", () => {
}
});

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("allows same-origin and none Fetch Metadata from a local 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 {
for (const site of ["same-origin", "none"]) {
const response = await fetch(`http://127.0.0.1:${port}/health`, {
headers: {
origin: `http://127.0.0.1:${port}`,
"sec-fetch-site": site,
},
});
expect(response.status).toBe(200);
await expect(response.json()).resolves.toMatchObject({ ok: true });
}
} 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",
},
Comment on lines 382 to +443
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

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.

Copy link
Copy Markdown
Member Author

@benvinegar benvinegar May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});
expect(response.status).toBe(403);
await expect(response.json()).resolves.toEqual({
error: "Browser requests to the local session broker must include an Origin.",
});
} finally {
server.stop(true);
}
});

test("requires the Hunk session client header for session API posts", 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}/session-api`, {
method: "POST",
headers: { "content-type": "application/json" },
body: JSON.stringify({ action: "list" }),
});

expect(response.status).toBe(403);
await expect(response.json()).resolves.toEqual({
error: "Expected Hunk session client header.",
});
} finally {
server.stop(true);
}
});

test("requires JSON content type for session API posts", async () => {
const port = await reserveLoopbackPort();
process.env.HUNK_MCP_HOST = "127.0.0.1";
Expand All @@ -386,7 +484,7 @@ describe("Hunk session daemon server", () => {
try {
const response = await fetch(`http://127.0.0.1:${port}/session-api`, {
method: "POST",
headers: { "content-type": "text/plain" },
headers: { ...SESSION_API_HEADERS, "content-type": "text/plain" },
body: JSON.stringify({ action: "list" }),
});

Expand All @@ -409,7 +507,7 @@ describe("Hunk session daemon server", () => {
try {
const response = await fetch(`http://127.0.0.1:${port}/session-api`, {
method: "POST",
headers: { "content-type": "application/json" },
headers: SESSION_API_HEADERS,
body: JSON.stringify({ action: "list", filler: "x".repeat(5 * 1024 * 1024) }),
});

Expand Down Expand Up @@ -497,9 +595,7 @@ describe("Hunk session daemon server", () => {

const emptyList = await fetch(`http://127.0.0.1:${port}/session-api`, {
method: "POST",
headers: {
"content-type": "application/json",
},
headers: SESSION_API_HEADERS,
body: JSON.stringify({ action: "list" }),
});
expect(emptyList.status).toBe(200);
Expand All @@ -509,9 +605,7 @@ describe("Hunk session daemon server", () => {
try {
const response = await fetch(`http://127.0.0.1:${port}/session-api`, {
method: "POST",
headers: {
"content-type": "application/json",
},
headers: SESSION_API_HEADERS,
body: JSON.stringify({ action: "list" }),
});

Expand Down Expand Up @@ -660,9 +754,7 @@ describe("Hunk session daemon server", () => {
try {
const response = await fetch(`http://127.0.0.1:${port}/session-api`, {
method: "POST",
headers: {
"content-type": "application/json",
},
headers: SESSION_API_HEADERS,
body: JSON.stringify({
action: "review",
selector: { sessionId: "session-1" },
Expand Down Expand Up @@ -721,9 +813,7 @@ describe("Hunk session daemon server", () => {
try {
const response = await fetch(`http://127.0.0.1:${port}/session-api`, {
method: "POST",
headers: {
"content-type": "application/json",
},
headers: SESSION_API_HEADERS,
body: JSON.stringify({
action: "reload",
selector: { sessionPath: "/tmp/live-session" },
Expand Down Expand Up @@ -782,7 +872,7 @@ describe("Hunk session daemon server", () => {
try {
const listResponse = await fetch(`http://127.0.0.1:${port}/session-api`, {
method: "POST",
headers: { "content-type": "application/json" },
headers: SESSION_API_HEADERS,
body: JSON.stringify({
action: "comment-list",
selector: { sessionId: "session-1" },
Expand Down Expand Up @@ -855,9 +945,7 @@ describe("Hunk session daemon server", () => {
try {
const response = await fetch(`http://127.0.0.1:${port}/session-api`, {
method: "POST",
headers: {
"content-type": "application/json",
},
headers: SESSION_API_HEADERS,
body: JSON.stringify({
action: "comment-apply",
selector: { sessionId: "session-1" },
Expand Down
62 changes: 62 additions & 0 deletions src/session-broker/brokerServer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ import {
HUNK_SESSION_API_PATH,
HUNK_SESSION_API_VERSION,
HUNK_SESSION_CAPABILITIES_PATH,
HUNK_SESSION_CLIENT_HEADER,
HUNK_SESSION_CLIENT_HEADER_VALUE,
HUNK_SESSION_DAEMON_VERSION,
type SessionDaemonAction,
type SessionDaemonCapabilities,
Expand Down Expand Up @@ -96,6 +98,51 @@ function jsonError(message: string, status = 400) {
return Response.json({ error: message }, { status });
}

/** Block requests whose Sec-Fetch-Site indicates cross-site or same-site browser traffic. */
function validateFetchMetadata(request: Request) {
const site = request.headers.get("sec-fetch-site")?.trim().toLowerCase();
if (!site) {
return null;
}

if (site === "none" || site === "same-origin") {
return null;
}

return jsonError("Browser request metadata is not allowed for the local session broker.", 403);
}

/** Block browser-looking requests that carry Sec-* headers but omit an Origin. */
function validateBrowserRequestOrigin(request: Request) {
if (request.headers.has("origin")) {
return null;
}

const browserHeaderNames = [
"sec-fetch-dest",
"sec-fetch-mode",
"sec-fetch-site",
"sec-fetch-user",
"sec-ch-ua",
"sec-ch-ua-mobile",
"sec-ch-ua-platform",
];
if (!browserHeaderNames.some((header) => request.headers.has(header))) {
return null;
}

return jsonError("Browser requests to the local session broker must include an Origin.", 403);
}

/** Require an intentional native-client header for the JSON command endpoint. */
function validateSessionClientHeader(request: Request) {
if (request.headers.get(HUNK_SESSION_CLIENT_HEADER) === HUNK_SESSION_CLIENT_HEADER_VALUE) {
return null;
}

return jsonError("Expected Hunk session client header.", 403);
}

/** Return whether one request body was explicitly sent as JSON. */
function hasJsonContentType(request: Request) {
const contentType = request.headers.get("content-type");
Expand Down Expand Up @@ -213,6 +260,11 @@ async function handleSessionApiRequest(state: HunkSessionBrokerState, request: R
return jsonError("Session API requests must use POST.", 405);
}

const clientHeaderError = validateSessionClientHeader(request);
if (clientHeaderError) {
return clientHeaderError;
}

if (!hasJsonContentType(request)) {
return jsonError("Expected Content-Type application/json.", 415);
}
Expand Down Expand Up @@ -447,6 +499,16 @@ export function serveSessionBrokerDaemon(
return originError;
}

const fetchMetadataError = validateFetchMetadata(request);
if (fetchMetadataError) {
return fetchMetadataError;
}

const browserOriginError = validateBrowserRequestOrigin(request);
if (browserOriginError) {
return browserOriginError;
}

const url = new URL(request.url);

if (url.pathname === "/health") {
Expand Down
4 changes: 3 additions & 1 deletion src/session/protocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,15 @@ import type {

export const HUNK_SESSION_API_PATH = "/session-api";
export const HUNK_SESSION_CAPABILITIES_PATH = `${HUNK_SESSION_API_PATH}/capabilities`;
export const HUNK_SESSION_CLIENT_HEADER = "x-hunk-session-client";
export const HUNK_SESSION_CLIENT_HEADER_VALUE = "hunk-cli";
export const HUNK_SESSION_API_VERSION = 1;

/**
* Version daemon/session compatibility separately from the HTTP action surface so newer Hunk
* builds can refresh an older daemon even when it still exposes the same API endpoints.
*/
export const HUNK_SESSION_DAEMON_VERSION = 3;
export const HUNK_SESSION_DAEMON_VERSION = 4;

export type SessionDaemonAction =
| "list"
Expand Down
Loading