diff --git a/frontend/__snapshots__/scenes-app-recordings--recordings-play-list-no-pinned-recordings.png b/frontend/__snapshots__/scenes-app-recordings--recordings-play-list-no-pinned-recordings.png index a3d4147b50f..072bef08ef7 100644 Binary files a/frontend/__snapshots__/scenes-app-recordings--recordings-play-list-no-pinned-recordings.png and b/frontend/__snapshots__/scenes-app-recordings--recordings-play-list-no-pinned-recordings.png differ diff --git a/plugin-server/jest.setup.fetch-mock.js b/plugin-server/jest.setup.fetch-mock.js index 151debe7538..60e240a8982 100644 --- a/plugin-server/jest.setup.fetch-mock.js +++ b/plugin-server/jest.setup.fetch-mock.js @@ -6,7 +6,11 @@ import fetch from 'node-fetch' import { status } from './src/utils/status' -jest.mock('node-fetch') +jest.mock('node-fetch', () => ({ + __esModule: true, + ...jest.requireActual('node-fetch'), // Only mock fetch(), leave Request, Response, FetchError, etc. alone + default: jest.fn(), +})) beforeEach(() => { const responsesToUrls = { @@ -21,7 +25,7 @@ beforeEach(() => { ]), } - fetch.mockImplementation( + jest.mocked(fetch).mockImplementation( (url, options) => new Promise((resolve) => resolve({ diff --git a/plugin-server/package.json b/plugin-server/package.json index e2d766f344b..d13ed75c16f 100644 --- a/plugin-server/package.json +++ b/plugin-server/package.json @@ -127,6 +127,7 @@ "eslint-plugin-prettier": "^4.2.1", "eslint-plugin-promise": "^6.0.0", "eslint-plugin-simple-import-sort": "^7.0.0", + "ipaddr.js": "^2.1.0", "jest": "^28.1.1", "nodemon": "^2.0.22", "parse-prometheus-text-format": "^1.1.1", diff --git a/plugin-server/pnpm-lock.yaml b/plugin-server/pnpm-lock.yaml index ffb5724f5ec..9389350aa76 100644 --- a/plugin-server/pnpm-lock.yaml +++ b/plugin-server/pnpm-lock.yaml @@ -279,6 +279,9 @@ devDependencies: eslint-plugin-simple-import-sort: specifier: ^7.0.0 version: 7.0.0(eslint@8.39.0) + ipaddr.js: + specifier: ^2.1.0 + version: 2.1.0 jest: specifier: ^28.1.1 version: 28.1.3(@types/node@16.18.25)(ts-node@10.9.1) @@ -7510,6 +7513,11 @@ packages: resolution: {integrity: sha512-WKa+XuLG1A1R0UWhl2+1XQSi+fZWMsYKffMZTTYsiZaUD8k2yDAj5atimTUD2TZkyCkNEeYE5NhFZmupOGtjYQ==} dev: false + /ipaddr.js@2.1.0: + resolution: {integrity: sha512-LlbxQ7xKzfBusov6UMi4MFpEg0m+mAm9xyNGEduwXMEDuf4WfzB/RZwMVYEd7IKGvh4IUkEXYxtAVu9T3OelJQ==} + engines: {node: '>= 10'} + dev: true + /is-arguments@1.1.1: resolution: {integrity: sha512-8Q7EARjzEnKpt/PCD7e1cgUS0a6X8u5tdSiMqXhojOdoV9TsMsiO+9VLC5vAmO8N7/GmXn7yjR8qnA6bVAEzfA==} engines: {node: '>= 0.4'} diff --git a/plugin-server/src/config/config.ts b/plugin-server/src/config/config.ts index f3245ce6223..ed19e701fca 100644 --- a/plugin-server/src/config/config.ts +++ b/plugin-server/src/config/config.ts @@ -111,6 +111,7 @@ export function getDefaultConfig(): PluginsServerConfig { CONVERSION_BUFFER_ENABLED_TEAMS: '', CONVERSION_BUFFER_TOPIC_ENABLED_TEAMS: '', BUFFER_CONVERSION_SECONDS: isDevEnv() ? 2 : 60, // KEEP IN SYNC WITH posthog/settings/ingestion.py + FETCH_HOSTNAME_GUARD_TEAMS: '', PERSON_INFO_CACHE_TTL: 5 * 60, // 5 min KAFKA_HEALTHCHECK_SECONDS: 20, OBJECT_STORAGE_ENABLED: true, @@ -129,7 +130,7 @@ export function getDefaultConfig(): PluginsServerConfig { APP_METRICS_GATHERED_FOR_ALL: isDevEnv() ? true : false, MAX_TEAM_ID_TO_BUFFER_ANONYMOUS_EVENTS_FOR: 0, USE_KAFKA_FOR_SCHEDULED_TASKS: true, - CLOUD_DEPLOYMENT: 'default', // Used as a Sentry tag + CLOUD_DEPLOYMENT: null, STARTUP_PROFILE_DURATION_SECONDS: 300, // 5 minutes STARTUP_PROFILE_CPU: false, diff --git a/plugin-server/src/main/ingestion-queues/on-event-handler-consumer.ts b/plugin-server/src/main/ingestion-queues/on-event-handler-consumer.ts index 221c33b1381..31a0e425a40 100644 --- a/plugin-server/src/main/ingestion-queues/on-event-handler-consumer.ts +++ b/plugin-server/src/main/ingestion-queues/on-event-handler-consumer.ts @@ -85,7 +85,13 @@ export const startAsyncWebhooksHandlerConsumer = async ({ const actionManager = new ActionManager(postgres) await actionManager.prepare() const actionMatcher = new ActionMatcher(postgres, actionManager, statsd) - const hookCannon = new HookCommander(postgres, teamManager, organizationManager, statsd) + const hookCannon = new HookCommander( + postgres, + teamManager, + organizationManager, + new Set(serverConfig.FETCH_HOSTNAME_GUARD_TEAMS.split(',').filter(String).map(Number)), + statsd + ) const concurrency = serverConfig.TASKS_PER_WORKER || 20 const pubSub = new PubSub(serverConfig, { diff --git a/plugin-server/src/types.ts b/plugin-server/src/types.ts index 9cc7fbfa216..7434efb3635 100644 --- a/plugin-server/src/types.ts +++ b/plugin-server/src/types.ts @@ -181,6 +181,7 @@ export interface PluginsServerConfig { CONVERSION_BUFFER_ENABLED_TEAMS: string CONVERSION_BUFFER_TOPIC_ENABLED_TEAMS: string BUFFER_CONVERSION_SECONDS: number + FETCH_HOSTNAME_GUARD_TEAMS: string PERSON_INFO_CACHE_TTL: number KAFKA_HEALTHCHECK_SECONDS: number OBJECT_STORAGE_ENABLED: boolean // Disables or enables the use of object storage. It will become mandatory to use object storage @@ -201,7 +202,8 @@ export interface PluginsServerConfig { USE_KAFKA_FOR_SCHEDULED_TASKS: boolean // distribute scheduled tasks across the scheduler workers EVENT_OVERFLOW_BUCKET_CAPACITY: number EVENT_OVERFLOW_BUCKET_REPLENISH_RATE: number - CLOUD_DEPLOYMENT: string + /** Label of the PostHog Cloud environment. Null if not running PostHog Cloud. @example 'US' */ + CLOUD_DEPLOYMENT: string | null // dump profiles to disk, covering the first N seconds of runtime STARTUP_PROFILE_DURATION_SECONDS: number @@ -266,6 +268,7 @@ export interface Hub extends PluginsServerConfig { lastActivityType: string statelessVms: StatelessVmMap conversionBufferEnabledTeams: Set + fetchHostnameGuardTeams: Set // functions enqueuePluginJob: (job: EnqueuedPluginJob) => Promise // ValueMatchers used for various opt-in/out features diff --git a/plugin-server/src/utils/db/hub.ts b/plugin-server/src/utils/db/hub.ts index 4e37d8a5cd7..a3ee16667d2 100644 --- a/plugin-server/src/utils/db/hub.ts +++ b/plugin-server/src/utils/db/hub.ts @@ -70,6 +70,9 @@ export async function createHub( const conversionBufferEnabledTeams = new Set( serverConfig.CONVERSION_BUFFER_ENABLED_TEAMS.split(',').filter(String).map(Number) ) + const fetchHostnameGuardTeams = new Set( + serverConfig.FETCH_HOSTNAME_GUARD_TEAMS.split(',').filter(String).map(Number) + ) const statsd: StatsD | undefined = createStatsdClient(serverConfig, threadId) @@ -181,6 +184,7 @@ export async function createHub( rootAccessManager, promiseManager, conversionBufferEnabledTeams, + fetchHostnameGuardTeams, pluginConfigsToSkipElementsParsing: buildIntegerMatcher(process.env.SKIP_ELEMENTS_PARSING_PLUGINS, true), } diff --git a/plugin-server/src/utils/env-utils.ts b/plugin-server/src/utils/env-utils.ts index 0b343f09fc8..4c2ab7d1731 100644 --- a/plugin-server/src/utils/env-utils.ts +++ b/plugin-server/src/utils/env-utils.ts @@ -40,6 +40,8 @@ export const isTestEnv = (): boolean => determineNodeEnv() === NodeEnv.Test export const isDevEnv = (): boolean => determineNodeEnv() === NodeEnv.Development export const isProdEnv = (): boolean => determineNodeEnv() === NodeEnv.Production +export const isCloud = (): boolean => !!process.env.CLOUD_DEPLOYMENT + export function isIngestionOverflowEnabled(): boolean { const ingestionOverflowEnabled = process.env.INGESTION_OVERFLOW_ENABLED return stringToBoolean(ingestionOverflowEnabled) diff --git a/plugin-server/src/utils/fetch.ts b/plugin-server/src/utils/fetch.ts index c45166edd0d..298e9e70deb 100644 --- a/plugin-server/src/utils/fetch.ts +++ b/plugin-server/src/utils/fetch.ts @@ -1,21 +1,73 @@ // This module wraps node-fetch with a sentry tracing-aware extension -import fetch, { FetchError, Request, Response } from 'node-fetch' +import { LookupAddress } from 'dns' +import dns from 'dns/promises' +import * as ipaddr from 'ipaddr.js' +import fetch, { type RequestInfo, type RequestInit, type Response, FetchError, Request } from 'node-fetch' +import { URL } from 'url' import { runInSpan } from '../sentry' -function fetchWrapper(...args: Parameters): Promise { - const request = new Request(...args) - return runInSpan( +export async function trackedFetch(url: RequestInfo, init?: RequestInit): Promise { + const request = new Request(url, init) + return await runInSpan( { op: 'fetch', description: `${request.method} ${request.url}`, }, - () => fetch(...args) + async () => await fetch(url, init) ) } -fetchWrapper.isRedirect = fetch.isRedirect -fetchWrapper.FetchError = FetchError +trackedFetch.isRedirect = fetch.isRedirect +trackedFetch.FetchError = FetchError -export default fetchWrapper +export async function safeTrackedFetch(url: RequestInfo, init?: RequestInit): Promise { + const request = new Request(url, init) + return await runInSpan( + { + op: 'fetch', + description: `${request.method} ${request.url}`, + }, + async () => { + await raiseIfUserProvidedUrlUnsafe(request.url) + return await fetch(url, init) + } + ) +} + +safeTrackedFetch.isRedirect = fetch.isRedirect +safeTrackedFetch.FetchError = FetchError + +/** + * Raise if the provided URL seems unsafe, otherwise do nothing. + * + * Equivalent of Django raise_if_user_provided_url_unsafe. + */ +export async function raiseIfUserProvidedUrlUnsafe(url: string): Promise { + // Raise if the provided URL seems unsafe, otherwise do nothing. + let parsedUrl: URL + try { + parsedUrl = new URL(url) + } catch (err) { + throw new FetchError('Invalid URL', 'posthog-host-guard') + } + if (!parsedUrl.hostname) { + throw new FetchError('No hostname', 'posthog-host-guard') + } + if (parsedUrl.protocol !== 'http:' && parsedUrl.protocol !== 'https:') { + throw new FetchError('Scheme must be either HTTP or HTTPS', 'posthog-host-guard') + } + let addrinfo: LookupAddress[] + try { + addrinfo = await dns.lookup(parsedUrl.hostname, { all: true }) + } catch (err) { + throw new FetchError('Invalid hostname', 'posthog-host-guard') + } + for (const { address } of addrinfo) { + // Prevent addressing internal services + if (ipaddr.parse(address).range() !== 'unicast') { + throw new FetchError('Internal hostname', 'posthog-host-guard') + } + } +} diff --git a/plugin-server/src/worker/ingestion/hooks.ts b/plugin-server/src/worker/ingestion/hooks.ts index e3c15a93291..2cc8279c88d 100644 --- a/plugin-server/src/worker/ingestion/hooks.ts +++ b/plugin-server/src/worker/ingestion/hooks.ts @@ -5,7 +5,8 @@ import { format } from 'util' import { Action, Hook, PostIngestionEvent, Team } from '../../types' import { PostgresRouter, PostgresUse } from '../../utils/db/postgres' -import fetch from '../../utils/fetch' +import { isCloud } from '../../utils/env-utils' +import { safeTrackedFetch, trackedFetch } from '../../utils/fetch' import { status } from '../../utils/status' import { getPropertyValueByPath, stringify } from '../../utils/utils' import { OrganizationManager } from './organization-manager' @@ -256,6 +257,7 @@ export class HookCommander { organizationManager: OrganizationManager statsd: StatsD | undefined siteUrl: string + fetchHostnameGuardTeams: Set /** Hook request timeout in ms. */ EXTERNAL_REQUEST_TIMEOUT = 10 * 1000 @@ -264,11 +266,13 @@ export class HookCommander { postgres: PostgresRouter, teamManager: TeamManager, organizationManager: OrganizationManager, + fetchHostnameGuardTeams?: Set, statsd?: StatsD ) { this.postgres = postgres this.teamManager = teamManager this.organizationManager = organizationManager + this.fetchHostnameGuardTeams = fetchHostnameGuardTeams || new Set() if (process.env.SITE_URL) { this.siteUrl = process.env.SITE_URL } else { @@ -358,9 +362,10 @@ export class HookCommander { `⌛⌛⌛ Posting Webhook slow. Timeout warning after 5 sec! url=${webhookUrl} team_id=${team.id} event_id=${event.eventUuid}` ) }, 5000) + const relevantFetch = isCloud() && this.fetchHostnameGuardTeams.has(team.id) ? safeTrackedFetch : trackedFetch try { await instrumentWebhookStep('fetch', async () => { - const request = await fetch(webhookUrl, { + const request = await relevantFetch(webhookUrl, { method: 'POST', body: JSON.stringify(message, undefined, 4), headers: { 'Content-Type': 'application/json' }, @@ -399,8 +404,10 @@ export class HookCommander { `⌛⌛⌛ Posting RestHook slow. Timeout warning after 5 sec! url=${hook.target} team_id=${event.teamId} event_id=${event.eventUuid}` ) }, 5000) + const relevantFetch = + isCloud() && this.fetchHostnameGuardTeams.has(hook.team_id) ? safeTrackedFetch : trackedFetch try { - const request = await fetch(hook.target, { + const request = await relevantFetch(hook.target, { method: 'POST', body: JSON.stringify(payload, undefined, 4), headers: { 'Content-Type': 'application/json' }, diff --git a/plugin-server/src/worker/plugins/mmdb.ts b/plugin-server/src/worker/plugins/mmdb.ts index a825c931c4d..7321238b2ba 100644 --- a/plugin-server/src/worker/plugins/mmdb.ts +++ b/plugin-server/src/worker/plugins/mmdb.ts @@ -1,5 +1,6 @@ import { Reader, ReaderModel } from '@maxmind/geoip2-node' import { DateTime } from 'luxon' +import fetch from 'node-fetch' import * as schedule from 'node-schedule' import prettyBytes from 'pretty-bytes' import { brotliDecompress } from 'zlib' @@ -12,7 +13,6 @@ import { } from '../../config/mmdb-constants' import { Hub, PluginAttachmentDB } from '../../types' import { PostgresUse } from '../../utils/db/postgres' -import fetch from '../../utils/fetch' import { status } from '../../utils/status' import { delay } from '../../utils/utils' diff --git a/plugin-server/src/worker/vm/imports.ts b/plugin-server/src/worker/vm/imports.ts index bcb96489749..d7b02d87c1c 100644 --- a/plugin-server/src/worker/vm/imports.ts +++ b/plugin-server/src/worker/vm/imports.ts @@ -12,33 +12,37 @@ import * as jsonwebtoken from 'jsonwebtoken' import * as pg from 'pg' import snowflake from 'snowflake-sdk' import { PassThrough } from 'stream' +import { Hub } from 'types' import * as url from 'url' import * as zlib from 'zlib' -import fetch from '../../utils/fetch' +import { isCloud, isTestEnv } from '../../utils/env-utils' +import { safeTrackedFetch, trackedFetch } from '../../utils/fetch' import { writeToFile } from './extensions/test-utils' -export const imports = { - ...(process.env.NODE_ENV === 'test' - ? { - 'test-utils/write-to-file': writeToFile, - } - : {}), - '@google-cloud/bigquery': bigquery, - '@google-cloud/pubsub': pubsub, - '@google-cloud/storage': gcs, - '@posthog/plugin-contrib': contrib, - '@posthog/plugin-scaffold': scaffold, - 'aws-sdk': AWS, - ethers: ethers, - 'generic-pool': genericPool, - 'node-fetch': fetch, - 'snowflake-sdk': snowflake, - crypto: crypto, - jsonwebtoken: jsonwebtoken, - faker: faker, - pg: pg, - stream: { PassThrough }, - url: url, - zlib: zlib, +export function determineImports(hub: Hub, teamId: number) { + return { + ...(isTestEnv() + ? { + 'test-utils/write-to-file': writeToFile, + } + : {}), + '@google-cloud/bigquery': bigquery, + '@google-cloud/pubsub': pubsub, + '@google-cloud/storage': gcs, + '@posthog/plugin-contrib': contrib, + '@posthog/plugin-scaffold': scaffold, + 'aws-sdk': AWS, + ethers: ethers, + 'generic-pool': genericPool, + 'node-fetch': isCloud() && hub.fetchHostnameGuardTeams.has(teamId) ? safeTrackedFetch : trackedFetch, + 'snowflake-sdk': snowflake, + crypto: crypto, + jsonwebtoken: jsonwebtoken, + faker: faker, + pg: pg, + stream: { PassThrough }, + url: url, + zlib: zlib, + } } diff --git a/plugin-server/src/worker/vm/vm.ts b/plugin-server/src/worker/vm/vm.ts index 967701cdff8..95e40ca4a6d 100644 --- a/plugin-server/src/worker/vm/vm.ts +++ b/plugin-server/src/worker/vm/vm.ts @@ -11,7 +11,7 @@ import { createJobs } from './extensions/jobs' import { createPosthog } from './extensions/posthog' import { createStorage } from './extensions/storage' import { createUtils } from './extensions/utilities' -import { imports } from './imports' +import { determineImports } from './imports' import { transformCode } from './transforms' import { upgradeExportEvents } from './upgrades/export-events' import { addHistoricalEventsExportCapability } from './upgrades/historical-export/export-historical-events' @@ -34,6 +34,8 @@ export function createPluginConfigVM( pluginConfig: PluginConfig, // NB! might have team_id = 0 indexJs: string ): PluginConfigVMResponse { + const imports = determineImports(hub, pluginConfig.team_id) + const timer = new Date() const statsdTiming = (metric: string) => { diff --git a/plugin-server/tests/main/db.test.ts b/plugin-server/tests/main/db.test.ts index 06c23380448..a2a570ce0af 100644 --- a/plugin-server/tests/main/db.test.ts +++ b/plugin-server/tests/main/db.test.ts @@ -165,7 +165,7 @@ describe('DB', () => { user_id: 1001, resource_id: 69, event: 'action_performed', - target: 'https://rest-hooks.example.com/', + target: 'https://example.com/', created: new Date().toISOString(), updated: new Date().toISOString(), }) @@ -188,7 +188,7 @@ describe('DB', () => { team_id: 2, resource_id: 69, event: 'action_performed', - target: 'https://rest-hooks.example.com/', + target: 'https://example.com/', }, ], bytecode: null, @@ -226,7 +226,7 @@ describe('DB', () => { user_id: 1001, resource_id: 69, event: 'event_performed', - target: 'https://rest-hooks.example.com/', + target: 'https://example.com/', created: new Date().toISOString(), updated: new Date().toISOString(), }) @@ -236,7 +236,7 @@ describe('DB', () => { user_id: 1001, resource_id: 70, event: 'event_performed', - target: 'https://rest-hooks.example.com/', + target: 'https://example.com/', created: new Date().toISOString(), updated: new Date().toISOString(), }) diff --git a/plugin-server/tests/utils/fetch.test.ts b/plugin-server/tests/utils/fetch.test.ts new file mode 100644 index 00000000000..d6100232a71 --- /dev/null +++ b/plugin-server/tests/utils/fetch.test.ts @@ -0,0 +1,40 @@ +import { FetchError } from 'node-fetch' + +import { raiseIfUserProvidedUrlUnsafe } from '../../src/utils/fetch' + +test('raiseIfUserProvidedUrlUnsafe', async () => { + // Sync test cases with posthog/api/test/test_utils.py + await raiseIfUserProvidedUrlUnsafe('https://google.com?q=20') // Safe + await raiseIfUserProvidedUrlUnsafe('https://posthog.com') // Safe + await raiseIfUserProvidedUrlUnsafe('https://posthog.com/foo/bar') // Safe, with path + await raiseIfUserProvidedUrlUnsafe('https://posthog.com:443') // Safe, good port + await raiseIfUserProvidedUrlUnsafe('https://1.1.1.1') // Safe, public IP + await expect(raiseIfUserProvidedUrlUnsafe('')).rejects.toThrow(new FetchError('Invalid URL', 'posthog-host-guard')) + await expect(raiseIfUserProvidedUrlUnsafe('@@@')).rejects.toThrow( + new FetchError('Invalid URL', 'posthog-host-guard') + ) + await expect(raiseIfUserProvidedUrlUnsafe('posthog.com')).rejects.toThrow( + new FetchError('Invalid URL', 'posthog-host-guard') + ) + await expect(raiseIfUserProvidedUrlUnsafe('ftp://posthog.com')).rejects.toThrow( + new FetchError('Scheme must be either HTTP or HTTPS', 'posthog-host-guard') + ) + await expect(raiseIfUserProvidedUrlUnsafe('http://localhost')).rejects.toThrow( + new FetchError('Internal hostname', 'posthog-host-guard') + ) + await expect(raiseIfUserProvidedUrlUnsafe('http://192.168.0.5')).rejects.toThrow( + new FetchError('Internal hostname', 'posthog-host-guard') + ) + await expect(raiseIfUserProvidedUrlUnsafe('http://0.0.0.0')).rejects.toThrow( + new FetchError('Internal hostname', 'posthog-host-guard') + ) + await expect(raiseIfUserProvidedUrlUnsafe('http://10.0.0.24')).rejects.toThrow( + new FetchError('Internal hostname', 'posthog-host-guard') + ) + await expect(raiseIfUserProvidedUrlUnsafe('http://172.20.0.21')).rejects.toThrow( + new FetchError('Internal hostname', 'posthog-host-guard') + ) + await expect(raiseIfUserProvidedUrlUnsafe('http://fgtggggzzggggfd.com')).rejects.toThrow( + new FetchError('Invalid hostname', 'posthog-host-guard') + ) +}) diff --git a/plugin-server/tests/worker/ingestion/event-pipeline/event-pipeline-integration.test.ts b/plugin-server/tests/worker/ingestion/event-pipeline/event-pipeline-integration.test.ts index 343826d81a4..71643e2668b 100644 --- a/plugin-server/tests/worker/ingestion/event-pipeline/event-pipeline-integration.test.ts +++ b/plugin-server/tests/worker/ingestion/event-pipeline/event-pipeline-integration.test.ts @@ -176,7 +176,7 @@ describe('Event Pipeline integration test', () => { user_id: commonUserId, resource_id: 69, event: 'action_performed', - target: 'https://rest-hooks.example.com/', + target: 'https://example.com/', created: timestamp, updated: timestamp, } as Hook) @@ -200,7 +200,7 @@ describe('Event Pipeline integration test', () => { hook: { id: 'abc', event: 'action_performed', - target: 'https://rest-hooks.example.com/', + target: 'https://example.com/', }, data: { event: 'xyz', @@ -224,7 +224,7 @@ describe('Event Pipeline integration test', () => { // Using a more verbose way instead of toHaveBeenCalledWith because we need to parse request body // and use expect.any for a few payload properties, which wouldn't be possible in a simpler way - expect(jest.mocked(fetch).mock.calls[0][0]).toBe('https://rest-hooks.example.com/') + expect(jest.mocked(fetch).mock.calls[0][0]).toBe('https://example.com/') const secondArg = jest.mocked(fetch).mock.calls[0][1] expect(JSON.parse(secondArg!.body as unknown as string)).toStrictEqual(expectedPayload) expect(JSON.parse(secondArg!.body as unknown as string)).toStrictEqual(expectedPayload) diff --git a/plugin-server/tests/worker/ingestion/hooks.test.ts b/plugin-server/tests/worker/ingestion/hooks.test.ts index c319ba01c3b..19e1f0eb684 100644 --- a/plugin-server/tests/worker/ingestion/hooks.test.ts +++ b/plugin-server/tests/worker/ingestion/hooks.test.ts @@ -1,7 +1,8 @@ import { DateTime } from 'luxon' -import * as fetch from 'node-fetch' +import fetch, { FetchError } from 'node-fetch' import { Action, PostIngestionEvent, Team } from '../../../src/types' +import { isCloud } from '../../../src/utils/env-utils' import { UUIDT } from '../../../src/utils/utils' import { determineWebhookType, @@ -15,6 +16,8 @@ import { } from '../../../src/worker/ingestion/hooks' import { Hook } from './../../../src/types' +jest.mock('../../../src/utils/env-utils') + describe('hooks', () => { describe('determineWebhookType', () => { test('Slack', () => { @@ -471,29 +474,35 @@ describe('hooks', () => { let hook: Hook beforeEach(() => { - hookCommander = new HookCommander({} as any, {} as any, {} as any) + jest.mocked(isCloud).mockReturnValue(false) // Disable private IP guard hook = { id: 'id', - team_id: 2, + team_id: 1, user_id: 1, resource_id: 1, event: 'foo', - target: 'foo.bar', + target: 'https://example.com/', created: new Date().toISOString(), updated: new Date().toISOString(), } + hookCommander = new HookCommander( + {} as any, + {} as any, + {} as any, + new Set([hook.team_id]) // Hostname guard enabled + ) }) test('person = undefined', async () => { await hookCommander.postRestHook(hook, { event: 'foo' } as any) - expect(fetch).toHaveBeenCalledWith('foo.bar', { + expect(fetch).toHaveBeenCalledWith('https://example.com/', { body: JSON.stringify( { hook: { id: 'id', event: 'foo', - target: 'foo.bar', + target: 'https://example.com/', }, data: { event: 'foo', @@ -510,26 +519,28 @@ describe('hooks', () => { }) test('person data from the event', async () => { + jest.mocked(isCloud).mockReturnValue(true) // Enable private IP guard, which example.com should pass + const now = new Date().toISOString() const uuid = new UUIDT().toString() await hookCommander.postRestHook(hook, { event: 'foo', - teamId: 1, + teamId: hook.team_id, person_id: uuid, person_properties: { foo: 'bar' }, person_created_at: DateTime.fromISO(now).toUTC(), } as any) - expect(fetch).toHaveBeenCalledWith('foo.bar', { + expect(fetch).toHaveBeenCalledWith('https://example.com/', { body: JSON.stringify( { hook: { id: 'id', event: 'foo', - target: 'foo.bar', + target: 'https://example.com/', }, data: { event: 'foo', - teamId: 1, + teamId: hook.team_id, person: { uuid: uuid, properties: { foo: 'bar' }, @@ -545,5 +556,19 @@ describe('hooks', () => { timeout: 10000, }) }) + + test('private IP hook allowed on self-hosted', async () => { + await hookCommander.postRestHook({ ...hook, target: 'http://127.0.0.1' }, { event: 'foo' } as any) + + expect(fetch).toHaveBeenCalledWith('http://127.0.0.1', expect.anything()) + }) + + test('private IP hook forbidden on Cloud', async () => { + jest.mocked(isCloud).mockReturnValue(true) + + await expect( + hookCommander.postRestHook({ ...hook, target: 'http://127.0.0.1' }, { event: 'foo' } as any) + ).rejects.toThrow(new FetchError('Internal hostname', 'posthog-host-guard')) + }) }) }) diff --git a/plugin-server/tests/worker/plugins/mmdb.test.ts b/plugin-server/tests/worker/plugins/mmdb.test.ts index 9bd3769032b..8179191a276 100644 --- a/plugin-server/tests/worker/plugins/mmdb.test.ts +++ b/plugin-server/tests/worker/plugins/mmdb.test.ts @@ -1,7 +1,7 @@ import { ReaderModel } from '@maxmind/geoip2-node' import { readFileSync } from 'fs' import { DateTime } from 'luxon' -import * as fetch from 'node-fetch' +import fetch from 'node-fetch' import { join } from 'path' import { Hub, LogLevel } from '../../../src/types' diff --git a/plugin-server/tests/worker/vm.extra-lazy.test.ts b/plugin-server/tests/worker/vm.extra-lazy.test.ts index 6f971c2e38d..e571b2f809b 100644 --- a/plugin-server/tests/worker/vm.extra-lazy.test.ts +++ b/plugin-server/tests/worker/vm.extra-lazy.test.ts @@ -1,4 +1,4 @@ -import * as fetch from 'node-fetch' +import fetch from 'node-fetch' import { Hub, PluginTaskType } from '../../src/types' import { createHub } from '../../src/utils/db/hub' @@ -39,7 +39,7 @@ describe('VMs are extra lazy 💤', () => { expect(lazyVm.ready).toEqual(true) expect(lazyVm.setupPluginIfNeeded).not.toHaveBeenCalled() - expect(fetch).toHaveBeenCalledWith('https://onevent.com/') + expect(fetch).toHaveBeenCalledWith('https://onevent.com/', undefined) }) test('VM with jobs gets setup immediately', async () => { @@ -64,7 +64,7 @@ describe('VMs are extra lazy 💤', () => { expect(lazyVm.ready).toEqual(true) expect(lazyVm.setupPluginIfNeeded).not.toHaveBeenCalled() - expect(fetch).toHaveBeenCalledWith('https://onevent.com/') + expect(fetch).toHaveBeenCalledWith('https://onevent.com/', undefined) }) test('VM without tasks delays setup until necessary', async () => { @@ -91,7 +91,7 @@ describe('VMs are extra lazy 💤', () => { await lazyVm.getOnEvent() expect(lazyVm.ready).toEqual(true) expect(lazyVm.setupPluginIfNeeded).toHaveBeenCalled() - expect(fetch).toHaveBeenCalledWith('https://onevent.com/') + expect(fetch).toHaveBeenCalledWith('https://onevent.com/', undefined) }) test('getting methods and tasks returns null if plugin is in errored state', async () => { diff --git a/plugin-server/tests/worker/vm.test.ts b/plugin-server/tests/worker/vm.test.ts index 8496a94a5a2..138c813d5c7 100644 --- a/plugin-server/tests/worker/vm.test.ts +++ b/plugin-server/tests/worker/vm.test.ts @@ -1,5 +1,5 @@ import { PluginEvent, ProcessedPluginEvent } from '@posthog/plugin-scaffold' -import * as fetch from 'node-fetch' +import fetch from 'node-fetch' import { KAFKA_EVENTS_PLUGIN_INGESTION, KAFKA_PLUGIN_LOG_ENTRIES } from '../../src/config/kafka-topics' import { Hub, PluginLogEntrySource, PluginLogEntryType } from '../../src/types' @@ -122,7 +122,7 @@ describe('vm tests', () => { }) expect(fetch).not.toHaveBeenCalled() await vm.methods.teardownPlugin!() - expect(fetch).toHaveBeenCalledWith('https://google.com/results.json?query=hoho') + expect(fetch).toHaveBeenCalledWith('https://google.com/results.json?query=hoho', undefined) }) test('processEvent', async () => { @@ -376,7 +376,7 @@ describe('vm tests', () => { event: 'export', } await vm.methods.onEvent!(event) - expect(fetch).toHaveBeenCalledWith('https://google.com/results.json?query=export') + expect(fetch).toHaveBeenCalledWith('https://google.com/results.json?query=export', undefined) }) test('export default', async () => { @@ -395,7 +395,7 @@ describe('vm tests', () => { event: 'default export', } await vm.methods.onEvent!(event) - expect(fetch).toHaveBeenCalledWith('https://google.com/results.json?query=default export') + expect(fetch).toHaveBeenCalledWith('https://google.com/results.json?query=default export', undefined) }) }) @@ -723,7 +723,7 @@ describe('vm tests', () => { } await vm.methods.processEvent!(event) - expect(fetch).toHaveBeenCalledWith('https://google.com/results.json?query=fetched') + expect(fetch).toHaveBeenCalledWith('https://google.com/results.json?query=fetched', undefined) expect(event.properties).toEqual({ count: 2, query: 'bla', results: [true, true] }) }) @@ -745,7 +745,7 @@ describe('vm tests', () => { } await vm.methods.processEvent!(event) - expect(fetch).toHaveBeenCalledWith('https://google.com/results.json?query=fetched') + expect(fetch).toHaveBeenCalledWith('https://google.com/results.json?query=fetched', undefined) expect(event.properties).toEqual({ count: 2, query: 'bla', results: [true, true] }) }) @@ -766,7 +766,7 @@ describe('vm tests', () => { } await vm.methods.processEvent!(event) - expect(fetch).toHaveBeenCalledWith('https://google.com/results.json?query=fetched') + expect(fetch).toHaveBeenCalledWith('https://google.com/results.json?query=fetched', undefined) expect(event.properties).toEqual({ count: 2, query: 'bla', results: [true, true] }) }) @@ -1051,7 +1051,7 @@ describe('vm tests', () => { event: 'onEvent', } await vm.methods.onEvent!(event) - expect(fetch).toHaveBeenCalledWith('https://google.com/results.json?query=onEvent') + expect(fetch).toHaveBeenCalledWith('https://google.com/results.json?query=onEvent', undefined) }) describe('exportEvents', () => { @@ -1085,7 +1085,7 @@ describe('vm tests', () => { await vm.methods.onEvent!({ ...defaultEvent, event: 'otherEvent2' }) await vm.methods.onEvent!({ ...defaultEvent, event: 'otherEvent3' }) await delay(1010) - expect(fetch).toHaveBeenCalledWith('https://export.com/results.json?query=otherEvent2&events=2') + expect(fetch).toHaveBeenCalledWith('https://export.com/results.json?query=otherEvent2&events=2', undefined) expect(hub.appMetrics.queueMetric).toHaveBeenCalledWith({ teamId: pluginConfig39.team_id, pluginConfigId: pluginConfig39.id, @@ -1136,8 +1136,8 @@ describe('vm tests', () => { await vm.methods.onEvent!(event) await delay(1010) expect(fetch).toHaveBeenCalledTimes(4) - expect(fetch).toHaveBeenCalledWith('https://onevent.com/') - expect(fetch).toHaveBeenCalledWith('https://export.com/results.json?query=exported&events=2') + expect(fetch).toHaveBeenCalledWith('https://onevent.com/', undefined) + expect(fetch).toHaveBeenCalledWith('https://export.com/results.json?query=exported&events=2', undefined) }) test('buffers bytes with exportEventsBufferBytes', async () => { @@ -1264,10 +1264,16 @@ describe('vm tests', () => { indexJs ) await vm.methods.onEvent!(defaultEvent) - expect(fetch).not.toHaveBeenCalledWith('https://export.com/results.json?query=default event&events=1') + expect(fetch).not.toHaveBeenCalledWith( + 'https://export.com/results.json?query=default event&events=1', + undefined + ) await vm.methods.teardownPlugin!() - expect(fetch).toHaveBeenCalledWith('https://export.com/results.json?query=default event&events=1') + expect(fetch).toHaveBeenCalledWith( + 'https://export.com/results.json?query=default event&events=1', + undefined + ) }) }) diff --git a/posthog/api/test/test_utils.py b/posthog/api/test/test_utils.py index c34aa06dac9..84a3d7315c2 100644 --- a/posthog/api/test/test_utils.py +++ b/posthog/api/test/test_utils.py @@ -147,20 +147,35 @@ class TestUtils(BaseTest): self.assertEqual(safe_clickhouse_string("💜 \u1f49c\ 💜"), "💜 \u1f49c\ 💜") def test_raise_if_user_provided_url_unsafe(self): + # Sync test cases with plugin-server/src/utils/fetch.test.ts raise_if_user_provided_url_unsafe("https://google.com?q=20") # Safe raise_if_user_provided_url_unsafe("https://posthog.com") # Safe raise_if_user_provided_url_unsafe("https://posthog.com/foo/bar") # Safe, with path raise_if_user_provided_url_unsafe("https://posthog.com:443") # Safe, good port raise_if_user_provided_url_unsafe("https://1.1.1.1") # Safe, public IP - self.assertRaises(ValueError, lambda: raise_if_user_provided_url_unsafe("https://posthog.com:80")) # Bad port - self.assertRaises(ValueError, lambda: raise_if_user_provided_url_unsafe("ftp://posthog.com")) # Bad scheme - self.assertRaises(ValueError, lambda: raise_if_user_provided_url_unsafe("")) # Empty - self.assertRaises(ValueError, lambda: raise_if_user_provided_url_unsafe("posthog.com")) # No scheme - self.assertRaises(ValueError, lambda: raise_if_user_provided_url_unsafe("http://localhost")) # Internal - self.assertRaises(ValueError, lambda: raise_if_user_provided_url_unsafe("http://192.168.0.5")) # Internal - self.assertRaises(ValueError, lambda: raise_if_user_provided_url_unsafe("http://0.0.0.0")) # Internal - self.assertRaises(ValueError, lambda: raise_if_user_provided_url_unsafe("http://10.0.0.24")) # Internal - self.assertRaises(ValueError, lambda: raise_if_user_provided_url_unsafe("http://172.20.0.21")) # Internal - self.assertRaises( - ValueError, lambda: raise_if_user_provided_url_unsafe("http://fgtggggzzggggfd.com") + self.assertRaisesMessage(ValueError, "No hostname", lambda: raise_if_user_provided_url_unsafe("")) + self.assertRaisesMessage(ValueError, "No hostname", lambda: raise_if_user_provided_url_unsafe("@@@")) + self.assertRaisesMessage(ValueError, "No hostname", lambda: raise_if_user_provided_url_unsafe("posthog.com")) + self.assertRaisesMessage( + ValueError, + "Scheme must be either HTTP or HTTPS", + lambda: raise_if_user_provided_url_unsafe("ftp://posthog.com"), + ) + self.assertRaisesMessage( + ValueError, "Internal hostname", lambda: raise_if_user_provided_url_unsafe("http://localhost") + ) + self.assertRaisesMessage( + ValueError, "Internal hostname", lambda: raise_if_user_provided_url_unsafe("http://192.168.0.5") + ) + self.assertRaisesMessage( + ValueError, "Internal hostname", lambda: raise_if_user_provided_url_unsafe("http://0.0.0.0") + ) + self.assertRaisesMessage( + ValueError, "Internal hostname", lambda: raise_if_user_provided_url_unsafe("http://10.0.0.24") + ) + self.assertRaisesMessage( + ValueError, "Internal hostname", lambda: raise_if_user_provided_url_unsafe("http://172.20.0.21") + ) + self.assertRaisesMessage( + ValueError, "Invalid hostname", lambda: raise_if_user_provided_url_unsafe("http://fgtggggzzggggfd.com") ) # Non-existent diff --git a/posthog/api/user.py b/posthog/api/user.py index 05629437602..4fbb85e8ca1 100644 --- a/posthog/api/user.py +++ b/posthog/api/user.py @@ -33,6 +33,7 @@ from posthog.api.organization import OrganizationSerializer from posthog.api.shared import OrganizationBasicSerializer, TeamBasicSerializer from posthog.api.utils import raise_if_user_provided_url_unsafe from posthog.auth import authenticate_secondarily +from posthog.cloud_utils import is_cloud from posthog.email import is_email_available from posthog.event_usage import report_user_logged_in, report_user_updated, report_user_verified_email from posthog.models import Team, User, UserScenePersonalisation, Dashboard @@ -450,7 +451,8 @@ def test_slack_webhook(request): return JsonResponse({"error": "no webhook URL"}) message = {"text": "_Greetings_ from PostHog!"} try: - raise_if_user_provided_url_unsafe(webhook) + if is_cloud(): # Protect against SSRF + raise_if_user_provided_url_unsafe(webhook) response = requests.post(webhook, verify=False, json=message) if response.ok: diff --git a/posthog/api/utils.py b/posthog/api/utils.py index 2a9991ea37f..298908f1ccb 100644 --- a/posthog/api/utils.py +++ b/posthog/api/utils.py @@ -302,23 +302,20 @@ def parse_bool(value: Union[str, List[str]]) -> bool: def raise_if_user_provided_url_unsafe(url: str): - """Raise if the provided URL seems unsafe, otherwise do nothing.""" - parsed_url: urllib.parse.ParseResult = urllib.parse.urlparse(url) + """Raise if the provided URL seems unsafe, otherwise do nothing. + + Equivalent of plugin server raiseIfUserProvidedUrlUnsafe. + """ + parsed_url: urllib.parse.ParseResult = urllib.parse.urlparse(url) # urlparse never raises errors if not parsed_url.hostname: raise ValueError("No hostname") - if parsed_url.scheme == "http": - port = 80 - elif parsed_url.scheme == "https": - port = 443 - else: + if parsed_url.scheme not in ("http", "https"): raise ValueError("Scheme must be either HTTP or HTTPS") - if parsed_url.port is not None and parsed_url.port != port: - raise ValueError("Port does not match scheme") # Disallow if hostname resolves to a private (internal) IP address try: - addrinfo = socket.getaddrinfo(parsed_url.hostname, port) + addrinfo = socket.getaddrinfo(parsed_url.hostname, None) except socket.gaierror: raise ValueError("Invalid hostname") for _, _, _, _, sockaddr in addrinfo: if ip_address(sockaddr[0]).is_private: # Prevent addressing internal services - raise ValueError("Invalid hostname") + raise ValueError("Internal hostname")