diff --git a/lib/policyEvaluator/requestUtils.ts b/lib/policyEvaluator/requestUtils.ts index cf1c15c03..5b07f9e54 100644 --- a/lib/policyEvaluator/requestUtils.ts +++ b/lib/policyEvaluator/requestUtils.ts @@ -8,9 +8,6 @@ export interface S3Config { } } -// TODO -// I'm not sure about this behavior. -// Should it returns string | string[] | undefined or string ? /** * getClientIp - Gets the client IP from the request * @param request - http request object @@ -20,8 +17,7 @@ export interface S3Config { export function getClientIp(request: IncomingMessage, s3config?: S3Config): string { const requestConfig = s3config?.requests; const remoteAddress = request.socket.remoteAddress; - // TODO What to do if clientIp === undefined ? - const clientIp = (requestConfig ? remoteAddress : request.headers['x-forwarded-for'] || remoteAddress)?.toString() ?? ''; + const clientIp = remoteAddress?.toString() ?? ''; if (requestConfig) { const { trustedProxyCIDRs, extractClientIPFromHeader } = requestConfig; /** @@ -30,11 +26,14 @@ export function getClientIp(request: IncomingMessage, s3config?: S3Config): stri * which header to be used to extract client IP */ if (ipCheck.ipMatchCidrList(trustedProxyCIDRs, clientIp)) { - const ipFromHeader = request.headers[extractClientIPFromHeader]?.toString(); + // Request headers in nodejs are lower-cased, so we should not + // be case-sentive when looking for the header, as http headers + // are case-insensitive. + const ipFromHeader = request.headers[extractClientIPFromHeader.toLowerCase()]?.toString(); if (ipFromHeader && ipFromHeader.trim().length) { return ipFromHeader.split(',')[0].trim(); } } } - return clientIp?.toString() ?? ''; + return clientIp; } diff --git a/package.json b/package.json index 47dbab66e..c20879fe0 100644 --- a/package.json +++ b/package.json @@ -3,7 +3,7 @@ "engines": { "node": ">=16" }, - "version": "7.10.63", + "version": "7.10.64", "description": "Common utilities for the S3 project components", "main": "build/index.js", "repository": { diff --git a/tests/unit/policyEvaluator/requestUtils.spec.js b/tests/unit/policyEvaluator/requestUtils.spec.js index 78df1cd5f..ffa77df99 100644 --- a/tests/unit/policyEvaluator/requestUtils.spec.js +++ b/tests/unit/policyEvaluator/requestUtils.spec.js @@ -27,6 +27,27 @@ describe('requestUtils.getClientIp', () => { assert.strictEqual(result, testClientIp1); }); + it('should return client Ip address in the proxy case when the header has uppercases', () => { + const request = new DummyRequest({ + headers: { + 'x-forwarded-for': [testClientIp1, testProxyIp].join(','), + }, + url: '/', + parsedHost: 'localhost', + socket: { + remoteAddress: testProxyIp, + }, + }); + const result = requestUtils.getClientIp(request, { + requests: { + viaProxy: true, + trustedProxyCIDRs: ['192.168.100.0/22'], + extractClientIPFromHeader: 'X-Forwarded-For', + }, + }); + assert.strictEqual(result, testClientIp1); + }); + it('should return client Ip address from socket info if the request is not forwarded from proxies', () => { const request = new DummyRequest({ headers: {}, @@ -56,8 +77,8 @@ describe('requestUtils.getClientIp', () => { assert.strictEqual(result, testClientIp2); }); - it('should return client Ip address from header if the request comes via proxies and ' + - 'no request config is available', () => { + it('should not return client Ip address from header if the request comes via proxies and ' + + 'no request config is available as the proxy is not trusted', () => { const request = new DummyRequest({ headers: { 'x-forwarded-for': testClientIp1, @@ -69,7 +90,7 @@ describe('requestUtils.getClientIp', () => { }, }); const result = requestUtils.getClientIp(request, configWithoutProxy); - assert.strictEqual(result, testClientIp1); + assert.strictEqual(result, testProxyIp); }); it('should return client Ip address from socket info if the request comes via proxies and ' +