Skip to content
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.

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

[INTER-726] Fix invalid host header #225

Merged
merged 15 commits into from
Jun 18, 2024
Merged
Show file tree
Hide file tree
Changes from 7 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
2 changes: 1 addition & 1 deletion e2e/scripts/mockTests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ async function main() {
for (const info of testInfo.tests) {
const agentPath = `${info.routePrefix}/${info.agentDownloadPath}`
const resultPath = `${info.routePrefix}/${info.getResultPath}`
const host = info.functionAppUrl
const host = info.frontdoorUrl

const agentUrl = new URL(host)
agentUrl.pathname = agentPath
Expand Down
9,236 changes: 5,107 additions & 4,129 deletions pnpm-lock.yaml

Large diffs are not rendered by default.

20 changes: 15 additions & 5 deletions proxy/utils/headers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ const mockReq = {
headers: {
'content-type': 'application/json',
'content-length': '24354',
host: 'fpjs.sh',
host: 'example.org',
'transfer-encoding': 'br',
via: 'azure.com',
cookie: '_iidt=7A03Gwg; _vid_t=gEFRuIQlzYmv692/UL4GLA==',
Expand All @@ -27,7 +27,9 @@ const mockReq = {
'strict-transport-security': 'max-age=600',
'x-azure-requestchain': 'hops=1',
'x-azure-socketip': '46.204.4.119',
'x-forwarded-for': '127.0.0.1',
'x-forwarded-for': '127.0.0.1:12345',
'x-azure-socketip': '127.0.0.1:12345',
'x-forwarded-host': 'fpjs.sh',
},
user: null,
params: {},
Expand Down Expand Up @@ -161,15 +163,23 @@ describe('prepareHeadersForIngressAPI', () => {
it('should set client ip and proxy secret', () => {
const result = prepareHeadersForIngressAPI(mockReq, 'secret')

expect(result['fpjs-proxy-client-ip']).toBe(mockReq.headers['x-forwarded-for'])
expect(result['fpjs-proxy-client-ip']).toBe('127.0.0.1')
expect(result['fpjs-proxy-secret']).toBe('secret')
expect(result['fpjs-proxy-forwarded-host']).toBe(new URL(mockReq.url).hostname)
expect(result['fpjs-proxy-forwarded-host']).toBe('fpjs.sh')
})

it('should set correct host', () => {
const result = prepareHeadersForIngressAPI(mockReq, 'secret')

expect(result['fpjs-proxy-client-ip']).toBe('127.0.0.1')
expect(result['fpjs-proxy-secret']).toBe('secret')
expect(result['fpjs-proxy-forwarded-host']).toBe('fpjs.sh')
})

it('should not set secret if it is undefined', () => {
const result = prepareHeadersForIngressAPI(mockReq, undefined)

expect(result['fpjs-proxy-client-ip']).toBe(mockReq.headers['x-forwarded-for'])
expect(result['fpjs-proxy-client-ip']).toBe('127.0.0.1')
expect(result['fpjs-proxy-secret']).toBe(undefined)
expect(result['fpjs-proxy-forwarded-host']).toBe(undefined)
})
Expand Down
27 changes: 10 additions & 17 deletions proxy/utils/headers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import * as http from 'http'
import { HttpRequest, HttpRequestHeaders, HttpResponseHeaders, Logger } from '@azure/functions'
import { updateCacheControlHeader } from './cacheControl'
import { filterCookie } from './cookies'
import { stripPort } from './ip'

const CACHE_CONTROL_HEADER_NAME = 'cache-control'

Expand Down Expand Up @@ -70,26 +71,18 @@ export function updateResponseHeaders(
}

function resolveClientIp(request: HttpRequest, logger?: Logger) {
const forwardedFor = request.headers['x-forwarded-for']
const clientIp =
request.headers['x-azure-socketip'] || request.headers['x-client-ip'] || request.headers['x-real-ip'] || ''

if (forwardedFor) {
const [clientIp] = forwardedFor.split(',')

logger?.verbose('Client IP resolved from x-forwarded-for', {
clientIp,
forwardedFor,
})

return clientIp
}

const clientIp = request.headers['x-client-ip'] || request.headers['x-real-ip']

logger?.verbose('Client IP resolved from x-client-ip or x-real-ip', {
logger?.verbose('Client IP resolved', {
clientIp,
})

return clientIp
return stripPort(clientIp)
}

export function getHost(request: Pick<HttpRequest, 'headers' | 'url'>) {
return request.headers['x-forwarded-host'] || request.headers.host || new URL(request.url).hostname
}

export function prepareHeadersForIngressAPI(request: HttpRequest, preSharedSecret?: string, logger?: Logger) {
Expand All @@ -99,7 +92,7 @@ export function prepareHeadersForIngressAPI(request: HttpRequest, preSharedSecre

if (preSharedSecret) {
headers['fpjs-proxy-secret'] = preSharedSecret
headers['fpjs-proxy-forwarded-host'] = new URL(request.url).hostname
headers['fpjs-proxy-forwarded-host'] = getHost(request)
}

return headers
Expand Down
23 changes: 23 additions & 0 deletions proxy/utils/ip.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { stripPort } from './ip'

describe('Strip port', () => {
it('strip port from ipv4 address', () => {
expect(stripPort('237.84.2.178:80')).toBe('237.84.2.178')
})

it('ipv6 without port', () => {
expect(stripPort('5be8:dde9:7f0b:d5a7:bd01:b3be:9c69:573b')).toBe('5be8:dde9:7f0b:d5a7:bd01:b3be:9c69:573b')
})

it('ipv4 without port', () => {
expect(stripPort('237.84.2.178')).toBe('237.84.2.178')
})

it('strip port from ipv6 address', () => {
expect(stripPort('[2001:0db8:85a3:0000:0000:8a2e:0370:7334]:443')).toBe('2001:0db8:85a3:0000:0000:8a2e:0370:7334')
})

it.each(['127.0', 'invalid', 'localhost', '2001:0db8:85a3:0000:0000'])('invalid ip: %s', (data) => {
expect(stripPort(data)).toBe(data)
})
})
28 changes: 28 additions & 0 deletions proxy/utils/ip.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { isIPv4, isIPv6 } from 'net'

export function stripPort(ip: string) {
necipallef marked this conversation as resolved.
Show resolved Hide resolved
// Check if it's an IPv6 address with a port
if (ip.startsWith('[')) {
// IPv6 address with port
const closingBracketIndex = ip.indexOf(']')
if (closingBracketIndex !== -1) {
return ip.substring(1, closingBracketIndex)
}
} else {
// IPv4 address with port or IPv6 without brackets
const colonIndex = ip.lastIndexOf(':')
if (colonIndex !== -1) {
const ipWithoutPort = ip.substring(0, colonIndex)
// Validate if the part before the colon is a valid IPv4 or IPv6 address
if (isValidIp(ipWithoutPort)) {
return ipWithoutPort
}
}
}
// If no port is found, return the original IP
return ip
}

function isValidIp(ip: string) {
return isIPv4(ip) || isIPv6(ip)
}
4 changes: 4 additions & 0 deletions shared/test/azure.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ export const mockRequestGet = (url: string, uri: string, query: HttpRequestQuery
'x-azure-requestchain': 'hops=1',
'x-azure-socketip': '46.204.4.119',
'x-forwarded-for': '127.0.0.1',
'x-client-ip': '128.0.0.1',
'x-azure-socketip': '127.0.0.1',
},
query,
params: {
Expand Down Expand Up @@ -66,6 +68,8 @@ export const mockRequestPost = (url: string, uri: string) => {
'x-azure-requestchain': 'hops=1',
'x-azure-socketip': '46.204.4.119',
'x-forwarded-for': '127.0.0.1',
'x-client-ip': '128.0.0.1',
'x-azure-socketip': '127.0.0.1',
},
query: {},
params: {
Expand Down
Loading