Skip to content

server: avoid forwarding auth headers in CORS proxy#24373

Open
ItsMatti4 wants to merge 1 commit into
ggml-org:masterfrom
ItsMatti4:codex/server-cors-proxy-strip-auth
Open

server: avoid forwarding auth headers in CORS proxy#24373
ItsMatti4 wants to merge 1 commit into
ggml-org:masterfrom
ItsMatti4:codex/server-cors-proxy-strip-auth

Conversation

@ItsMatti4

@ItsMatti4 ItsMatti4 commented Jun 9, 2026

Copy link
Copy Markdown

Overview

The CORS proxy now ignores incoming request headers by default and only forwards headers that are explicitly provided with the x-proxy-header-* prefix. This prevents llama-server auth and cookie headers from being forwarded to the proxied target, while preserving explicit target-header passthrough for MCP servers that need authentication.

Additional information

Frontend check: configured MCP headers already go through buildProxiedHeaders(...). I also updated the MCP diagnostic fetch path so headers added dynamically by the MCP SDK during requests are wrapped as x-proxy-header-* when useProxy is enabled. Diagnostic header redaction now treats proxied headers as their underlying target headers, so proxied auth/session/custom header values are not logged in clear text.

Tests / verification:

  • Added server regression coverage for not forwarding direct auth/cookie headers and still forwarding explicit x-proxy-header-authorization.
  • Added UI unit coverage for dynamic proxied request headers and proxied diagnostic redaction.
  • git diff --check passes locally.
  • Local server pytest did not run fully because build/bin/Release/llama-server.exe is not present in this checkout.
  • Local UI vitest did not run because npm ci failed on this machine with ENOSPC while installing dependencies.

Requirements

  • I have read and agree with the contributing guidelines
  • AI usage disclosure: YES - I used OpenAI Codex to inspect the proxy/frontend flow, implement the patch, and draft this PR description. I reviewed the changes and am responsible for the submitted code.

@ItsMatti4 ItsMatti4 requested a review from a team as a code owner June 9, 2026 18:18
@github-actions github-actions Bot added examples python python script changes server labels Jun 9, 2026
@ServeurpersoCom

Copy link
Copy Markdown
Contributor

Real footgun worth studying since the proxy silently forwards the server API key and cookies to the target, but this breaks the explicit x-proxy-header-* passthrough for authenticated MCP servers, I think an allowlist forwarding only prefixed headers would solve it by construction, also please fill the PR template.

@ngxson WDYT (the idea is there, not the current implementation) ?

@ngxson

ngxson commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Real footgun worth studying since the proxy silently forwards the server API key and cookies to the target

hmm that's unexpected, I think that worth fixing than the current PR (do not forward these headers to server, maybe only whitelist safe headers like user-agent). can you push a fix @ServeurpersoCom ?

I'm not quite comfortable with the blacklist instead of whitelist approach in this PR, and on top of that, breaking x-proxy-header- is a no-go

edit: I can't think of any headers that need to be whitelisted; the user-agent can already be set explicitly via web ui, so probably just don't whitelist anything and only accept x-proxy-header-*

Comment on lines -37 to -39
auto new_key = key;
if (string_starts_with(new_key, "x-proxy-header-")) {
string_replace_all(new_key, "x-proxy-header-", "");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

should simply ignore all headers not starting with x-proxy-header-

frontend may need to be adapted too

@ServeurpersoCom

Copy link
Copy Markdown
Contributor

Yes, if needed I will make a targeted PR that supersedes, the most important thing is to generate traces with a server (like my mcp.js) to really see what is leaking and what the PR solves.

@ItsMatti4 ItsMatti4 force-pushed the codex/server-cors-proxy-strip-auth branch from af76f7d to e044c61 Compare June 10, 2026 15:18
@ItsMatti4

Copy link
Copy Markdown
Author

Updated, thanks for the review.

The proxy now ignores all incoming headers that do not start with x-proxy-header-, as suggested. Explicit proxy headers are still forwarded after stripping the prefix, so authenticated MCP servers can still use x-proxy-header-authorization.

I also updated the regression test to cover both cases:

  • implicit server/client auth headers are not forwarded
  • explicit x-proxy-header-* headers are forwarded

AI usage disclosure: I used OpenAI Codex to help identify the issue, prepare the patch, and update it after review.

@ngxson

ngxson commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator
  1. please update the PR's description
  2. check frontend code to verify if it needs to be updated

@ItsMatti4 ItsMatti4 force-pushed the codex/server-cors-proxy-strip-auth branch from e044c61 to ff26794 Compare June 11, 2026 09:49
@ItsMatti4 ItsMatti4 requested a review from a team as a code owner June 11, 2026 09:49
@ItsMatti4

Copy link
Copy Markdown
Author

Thanks, updated. I changed the backend to only forward explicit x-proxy-header-* headers and checked the frontend path. Configured MCP headers were already wrapped, and dynamic SDK request headers are now wrapped when useProxy is enabled. I also added diagnostic redaction coverage for proxied headers.

for (const [key, value] of new Headers(headers).entries()) {
const proxiedKey =
useProxy && !key.toLowerCase().startsWith('x-proxy-header-')
? `x-proxy-header-${key}`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's use a constant instead of magic string for the header namespace

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done, I added a shared CORS_PROXY_HEADER_PREFIX constant and reused it in the proxy header builder, diagnostic fetch path, and header redaction logic.

@ItsMatti4 ItsMatti4 force-pushed the codex/server-cors-proxy-strip-auth branch from ff26794 to 47e8892 Compare June 11, 2026 12:30
Comment thread tools/ui/src/lib/utils/cors-proxy.ts Outdated
import { base } from '$app/paths';
import { CORS_PROXY_ENDPOINT, CORS_PROXY_URL_PARAM } from '$lib/constants';

export const CORS_PROXY_HEADER_PREFIX = 'x-proxy-header-';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe let's rename it

Suggested change
export const CORS_PROXY_HEADER_PREFIX = 'x-proxy-header-';
export const CORS_PROXY_HEADER_PREFIX = 'x-llama-server-proxy-header-';

Comment on lines +58 to +67
'x-proxy-header-authorization': 'Bearer secret',
'x-proxy-header-mcp-session-id': 'session-12345',
'x-proxy-header-x-vendor-key': 'vendor-secret'
});
const partial = new Map([['mcp-session-id', 5]]);
const result = sanitizeHeaders(headers, ['x-vendor-key'], partial);

expect(result['x-proxy-header-authorization']).toBe('[redacted]');
expect(result['x-proxy-header-mcp-session-id']).toBe('....12345');
expect(result['x-proxy-header-x-vendor-key']).toBe('[redacted]');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and maybe let's use the constant here for this test, might be easier to update in the future

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done, renamed the proxy header namespace to x-llama-server-proxy-header- across the backend and frontend, and updated the UI tests to build proxied header names from CORS_PROXY_HEADER_PREFIX.

@ItsMatti4 ItsMatti4 force-pushed the codex/server-cors-proxy-strip-auth branch from 47e8892 to 4ae0646 Compare June 11, 2026 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants