Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
21 changes: 20 additions & 1 deletion src/http/extractHttpUserFromHeaders.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

import { HttpRequestUser } from '@azure/functions';
import { nonNullValue } from '../utils/nonNull';
import { workerSystemLog } from '../utils/workerSystemLog';

/* grandfathered in. Should fix when possible */
/* eslint-disable @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-argument, @typescript-eslint/no-unsafe-member-access */
Expand All @@ -12,7 +13,25 @@ export function extractHttpUserFromHeaders(headers: Headers): HttpRequestUser |

const clientPrincipal = headers.get('x-ms-client-principal');
if (clientPrincipal) {
const claimsPrincipalData = JSON.parse(Buffer.from(clientPrincipal, 'base64').toString('utf-8'));
let claimsPrincipalData: any;
try {
Comment thread
TsuyoshiUshio marked this conversation as resolved.
claimsPrincipalData = JSON.parse(Buffer.from(clientPrincipal, 'base64').toString('utf-8'));
} catch (err) {
workerSystemLog(
'warning',
`Failed to parse x-ms-client-principal header: ${err instanceof Error ? err.message : String(err)}`
);
return null;
}

if (
claimsPrincipalData === null ||
typeof claimsPrincipalData !== 'object' ||
Array.isArray(claimsPrincipalData)
) {
workerSystemLog('warning', 'Parsed x-ms-client-principal header was not a JSON object.');
return null;
}

if (claimsPrincipalData['identityProvider']) {
user = {
Expand Down
34 changes: 33 additions & 1 deletion src/http/httpProxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,18 @@ const responses: Record<string, http.ServerResponse> = {};
const minPort = 55000;
const maxPort = 55025;

const blockedProxyResponseHeaders = new Set([
'connection',
'keep-alive',
'proxy-authenticate',
'proxy-authorization',
'te',
'trailer',
'trailers',
'transfer-encoding',
'upgrade',
]);

const invocRequestEmitter = new EventEmitter();

export async function waitForProxyRequest(invocationId: string): Promise<http.IncomingMessage> {
Expand All @@ -39,8 +51,11 @@ const invocationIdHeader = 'x-ms-invocation-id';
export async function sendProxyResponse(invocationId: string, userRes: HttpResponse): Promise<void> {
const proxyRes = nonNullProp(responses, invocationId);
delete responses[invocationId];
const connectionHeader = userRes.headers.get('connection');
for (const [key, val] of userRes.headers.entries()) {
proxyRes.setHeader(key, val);
if (isAllowedProxyResponseHeader(key, connectionHeader)) {
proxyRes.setHeader(key, val);
}
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.

Content-Length is an end-to-end header, and customers may set it intentionally for HEAD/304/206 responses, downloads, or other scenarios where clients rely on it. Dropping it could change behavior for existing apps.

Can you check RFC Compliant docs if we can preseves 'content-length'.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. \Content-Length\ is an end-to-end header, and dropping it may have compatibility impact for HEAD/304/206/download scenarios. I am leaving this thread open for discussion rather than changing it blindly. My original concern was mismatched framing when proxying streamed bodies; we should decide whether preserving it is preferred or whether only conflicting cases should be filtered.

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.

So what's your recommendation?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My recommendation is to preserve Content-Length and remove it from the blocked header list.

Content-Length is end-to-end response metadata, not a hop-by-hop header, and removing other hop-by-hop headers does not change the response body bytes. Dropping it could break existing apps that intentionally set it, especially for HEAD/304/206/download scenarios.

The original concern was about forwarding connection/framing controls through this proxy. This code copies user response headers to Node.js ServerResponse, then writes the response body through proxyRes.write(...). If we forward headers like Connection, TE, Trailer, Transfer-Encoding, or Upgrade, those headers can describe connection-specific behavior or wire framing that should be owned by the proxy/Node response layer instead.

To address that without treating Content-Length as hop-by-hop, I updated the implementation to:

  • allow content-length;
  • keep blocking the standard hop-by-hop headers;
  • also strip any header names listed by the Connection header, because Connection: x-private-hop marks x-private-hop as connection-specific and a proxy should not forward it.

I am not validating Content-Length against the streamed body here, because that would require buffering/counting the stream and would weaken the streaming path. If a user sets an incorrect Content-Length, that is a response correctness issue separate from this hop-by-hop proxy hardening.

}
proxyRes.setHeader(invocationIdHeader, invocationId);
proxyRes.statusCode = userRes.status;
Expand All @@ -57,6 +72,23 @@ export async function sendProxyResponse(invocationId: string, userRes: HttpRespo
proxyRes.end();
}

export function isAllowedProxyResponseHeader(headerName: string, connectionHeader?: string | null): boolean {
const normalizedHeaderName = headerName.toLowerCase();
return (
!blockedProxyResponseHeaders.has(normalizedHeaderName) &&
!getConnectionHeaderNames(connectionHeader).has(normalizedHeaderName)
);
}

function getConnectionHeaderNames(connectionHeader?: string | null): Set<string> {
return new Set(
connectionHeader
?.split(',')
.map((headerName) => headerName.trim().toLowerCase())
.filter((headerName) => headerName.length > 0)
);
}

function setCookies(userRes: HttpResponse, proxyRes: http.ServerResponse): void {
const serializedCookies: string[] = userRes.cookies.map((c) => {
let sameSite: true | false | 'lax' | 'strict' | 'none' | undefined;
Expand Down
30 changes: 30 additions & 0 deletions test/http/extractHttpUserFromHeaders.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,4 +82,34 @@ describe('Extract Http User Claims Principal from Headers', () => {

expect(user).to.be.null;
});

it('Returns null when client principal header is not valid JSON', () => {
const headers: Headers = new Headers({
'x-ms-client-principal': Buffer.from('not json').toString('base64'),
});

const user: HttpRequestUser | null = extractHttpUserFromHeaders(headers);

expect(user).to.be.null;
});

it('Returns null when client principal header parses to null', () => {
const headers: Headers = new Headers({
'x-ms-client-principal': Buffer.from(JSON.stringify(null)).toString('base64'),
});

const user: HttpRequestUser | null = extractHttpUserFromHeaders(headers);

expect(user).to.be.null;
});

it('Returns null when client principal header parses to a string', () => {
const headers: Headers = new Headers({
'x-ms-client-principal': Buffer.from(JSON.stringify('not an object')).toString('base64'),
});

const user: HttpRequestUser | null = extractHttpUserFromHeaders(headers);

expect(user).to.be.null;
});
});
43 changes: 43 additions & 0 deletions test/http/httpProxy.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the MIT License.

import 'mocha';
import { expect } from 'chai';
import { isAllowedProxyResponseHeader } from '../../src/http/httpProxy';

describe('Http proxy', () => {
it('Blocks hop-by-hop response headers', () => {
const blockedHeaders = [
'connection',
'keep-alive',
'proxy-authenticate',
'proxy-authorization',
'te',
'trailer',
'trailers',
'transfer-encoding',
'upgrade',
];

for (const header of blockedHeaders) {
expect(isAllowedProxyResponseHeader(header), header).to.be.false;
expect(isAllowedProxyResponseHeader(header.toUpperCase()), header).to.be.false;
}
});

it('Allows end-to-end response headers', () => {
const allowedHeaders = ['content-type', 'cache-control', 'set-cookie', 'x-frame-options', 'content-length'];

for (const header of allowedHeaders) {
expect(isAllowedProxyResponseHeader(header), header).to.be.true;
}
});

it('Blocks headers named by the Connection response header', () => {
const connectionHeader = 'keep-alive, x-private-hop, X-Another-Hop';

expect(isAllowedProxyResponseHeader('x-private-hop', connectionHeader)).to.be.false;
expect(isAllowedProxyResponseHeader('x-another-hop', connectionHeader)).to.be.false;
expect(isAllowedProxyResponseHeader('content-type', connectionHeader)).to.be.true;
});
});
Loading