Skip to content

Commit

Permalink
fix(webhooks): Always check for unsafe urls (#18360)
Browse files Browse the repository at this point in the history 
* fix(webhooks): Always check for unsafe urls

* Roll out the hostname guard fully

The mechanism is now only disabled in local dev.

* Update hooks.test.ts

* Disable URL check in functional tests

---------

Co-authored-by: Michael Matloka <michal@matloka.com>
  • Loading branch information
@timgl @Twixes
timgl and Twixes committed Nov 3, 2023
1 parent 948042d commit 22bd594
Show file tree
Hide file tree
Showing 12 changed files with 46 additions and 99 deletions.
1 change: 1 addition & 0 deletions plugin-server/bin/ci_functional_tests.sh
View file
Expand Up @@ -16,6 +16,7 @@ export CONVERSION_BUFFER_ENABLED=true
export BUFFER_CONVERSION_SECONDS=2 # Make sure we don't have to wait for the default 60 seconds
export KAFKA_MAX_MESSAGE_BATCH_SIZE=0
export APP_METRICS_GATHERED_FOR_ALL=true
export NODE_ENV=production-functional-tests

# Not important at all, but I like to see nice red/green for tests
export FORCE_COLOR=true
Expand Down
1 change: 0 additions & 1 deletion plugin-server/src/config/config.ts
View file
Expand Up @@ -110,7 +110,6 @@ 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,
Expand Down
1 change: 0 additions & 1 deletion plugin-server/src/main/ingestion-queues/on-event-handler-consumer.ts
View file
Expand Up @@ -89,7 +89,6 @@ export const startAsyncWebhooksHandlerConsumer = async ({
postgres,
teamManager,
organizationManager,
new Set(serverConfig.FETCH_HOSTNAME_GUARD_TEAMS.split(',').filter(String).map(Number)),
appMetrics,
statsd,
serverConfig.EXTERNAL_REQUEST_TIMEOUT_MS
Expand Down
3 changes: 0 additions & 3 deletions plugin-server/src/types.ts
View file
Expand Up @@ -178,7 +178,6 @@ 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
Expand Down Expand Up @@ -273,8 +272,6 @@ export interface Hub extends PluginsServerConfig {
lastActivityType: string
statelessVms: StatelessVmMap
conversionBufferEnabledTeams: Set<number>
/** null means that the hostname guard is enabled for everyone */
fetchHostnameGuardTeams: Set<number> | null
// functions
enqueuePluginJob: (job: EnqueuedPluginJob) => Promise<void>
// ValueMatchers used for various opt-in/out features
Expand Down
5 changes: 0 additions & 5 deletions plugin-server/src/utils/db/hub.ts
View file
Expand Up @@ -89,10 +89,6 @@ export async function createHub(
const conversionBufferEnabledTeams = new Set(
serverConfig.CONVERSION_BUFFER_ENABLED_TEAMS.split(',').filter(String).map(Number)
)
const fetchHostnameGuardTeams =
serverConfig.FETCH_HOSTNAME_GUARD_TEAMS === '*'
? null
: new Set(serverConfig.FETCH_HOSTNAME_GUARD_TEAMS.split(',').filter(String).map(Number))

const statsd: StatsD | undefined = createStatsdClient(serverConfig, threadId)

Expand Down Expand Up @@ -193,7 +189,6 @@ export async function createHub(
rootAccessManager,
promiseManager,
conversionBufferEnabledTeams,
fetchHostnameGuardTeams,
pluginConfigsToSkipElementsParsing: buildIntegerMatcher(process.env.SKIP_ELEMENTS_PARSING_PLUGINS, true),
poeEmbraceJoinForTeams: buildIntegerMatcher(process.env.POE_EMBRACE_JOIN_FOR_TEAMS, true),
eventsToDropByToken: createEventsToDropByToken(process.env.DROP_EVENTS_BY_TOKEN_DISTINCT_ID),
Expand Down
23 changes: 6 additions & 17 deletions plugin-server/src/utils/fetch.ts
View file
Expand Up @@ -7,37 +7,26 @@ import fetch, { type RequestInfo, type RequestInit, type Response, FetchError, R
import { URL } from 'url'

import { runInSpan } from '../sentry'
import { isProdEnv } from './env-utils'

export async function trackedFetch(url: RequestInfo, init?: RequestInit): Promise<Response> {
const request = new Request(url, init)
return await runInSpan(
{
op: 'fetch',
description: `${request.method} ${request.url}`,
},
async () => await fetch(url, init)
)
}

trackedFetch.isRedirect = fetch.isRedirect
trackedFetch.FetchError = FetchError

export async function safeTrackedFetch(url: RequestInfo, init?: RequestInit): Promise<Response> {
const request = new Request(url, init)
return await runInSpan(
{
op: 'fetch',
description: `${request.method} ${request.url}`,
},
async () => {
await raiseIfUserProvidedUrlUnsafe(request.url)
if (isProdEnv() && !process.env.NODE_ENV?.includes('functional-tests')) {
await raiseIfUserProvidedUrlUnsafe(request.url)
}
return await fetch(url, init)
}
)
}

safeTrackedFetch.isRedirect = fetch.isRedirect
safeTrackedFetch.FetchError = FetchError
trackedFetch.isRedirect = fetch.isRedirect
trackedFetch.FetchError = FetchError

/**
* Raise if the provided URL seems unsafe, otherwise do nothing.
Expand Down
20 changes: 3 additions & 17 deletions plugin-server/src/worker/ingestion/hooks.ts
View file
Expand Up @@ -5,8 +5,7 @@ import { format } from 'util'

import { Action, Hook, PostIngestionEvent, Team } from '../../types'
import { PostgresRouter, PostgresUse } from '../../utils/db/postgres'
import { isCloud } from '../../utils/env-utils'
import { safeTrackedFetch, trackedFetch } from '../../utils/fetch'
import { trackedFetch } from '../../utils/fetch'
import { status } from '../../utils/status'
import { getPropertyValueByPath, stringify } from '../../utils/utils'
import { AppMetrics } from './app-metrics'
Expand Down Expand Up @@ -259,25 +258,20 @@ export class HookCommander {
appMetrics: AppMetrics
statsd: StatsD | undefined
siteUrl: string
/** null means that the hostname guard is enabled for everyone */
fetchHostnameGuardTeams: Set<number> | null

/** Hook request timeout in ms. */
EXTERNAL_REQUEST_TIMEOUT: number

constructor(
postgres: PostgresRouter,
teamManager: TeamManager,
organizationManager: OrganizationManager,
fetchHostnameGuardTeams: Set<number> | null = new Set(),
appMetrics: AppMetrics,
statsd: StatsD | undefined,
timeout: number
) {
this.postgres = postgres
this.teamManager = teamManager
this.organizationManager = organizationManager
this.fetchHostnameGuardTeams = fetchHostnameGuardTeams
if (process.env.SITE_URL) {
this.siteUrl = process.env.SITE_URL
} else {
Expand Down Expand Up @@ -373,13 +367,9 @@ export class HookCommander {
} sec! url=${webhookUrl} team_id=${team.id} event_id=${event.eventUuid}`
)
}, slowWarningTimeout)
const relevantFetch =
isCloud() && (!this.fetchHostnameGuardTeams || this.fetchHostnameGuardTeams.has(team.id))
? safeTrackedFetch
: trackedFetch
try {
await instrumentWebhookStep('fetch', async () => {
const request = await relevantFetch(webhookUrl, {
const request = await trackedFetch(webhookUrl, {
method: 'POST',
body: JSON.stringify(message, undefined, 4),
headers: { 'Content-Type': 'application/json' },
Expand Down Expand Up @@ -455,12 +445,8 @@ export class HookCommander {
} team_id=${event.teamId} event_id=${event.eventUuid}`
)
}, slowWarningTimeout)
const relevantFetch =
isCloud() && (!this.fetchHostnameGuardTeams || this.fetchHostnameGuardTeams.has(hook.team_id))
? safeTrackedFetch
: trackedFetch
try {
const request = await relevantFetch(hook.target, {
const request = await trackedFetch(hook.target, {
method: 'POST',
body: JSON.stringify(payload, undefined, 4),
headers: { 'Content-Type': 'application/json' },
Expand Down
56 changes: 25 additions & 31 deletions plugin-server/src/worker/vm/imports.ts
View file
Expand Up @@ -12,40 +12,34 @@ 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 { isCloud, isTestEnv } from '../../utils/env-utils'
import { safeTrackedFetch, trackedFetch } from '../../utils/fetch'
import { isTestEnv } from '../../utils/env-utils'
import { trackedFetch } from '../../utils/fetch'
import { writeToFile } from './extensions/test-utils'

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 || hub.fetchHostnameGuardTeams.has(teamId))
? safeTrackedFetch
: trackedFetch,
'snowflake-sdk': snowflake,
crypto: crypto,
jsonwebtoken: jsonwebtoken,
faker: faker,
pg: pg,
stream: { PassThrough },
url: url,
zlib: zlib,
}
export const AVAILABLE_IMPORTS = {
...(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': trackedFetch,
'snowflake-sdk': snowflake,
crypto: crypto,
jsonwebtoken: jsonwebtoken,
faker: faker,
pg: pg,
stream: { PassThrough },
url: url,
zlib: zlib,
}
10 changes: 4 additions & 6 deletions plugin-server/src/worker/vm/vm.ts
View file
Expand Up @@ -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 { determineImports } from './imports'
import { AVAILABLE_IMPORTS } from './imports'
import { transformCode } from './transforms'
import { upgradeExportEvents } from './upgrades/export-events'
import { addHistoricalEventsExportCapability } from './upgrades/historical-export/export-historical-events'
Expand All @@ -34,8 +34,6 @@ 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) => {
Expand All @@ -46,7 +44,7 @@ export function createPluginConfigVM(
})
}

const transformedCode = transformCode(indexJs, hub, imports)
const transformedCode = transformCode(indexJs, hub, AVAILABLE_IMPORTS)

// Create virtual machine
const vm = new VM({
Expand All @@ -59,10 +57,10 @@ export function createPluginConfigVM(
vm.freeze(createPosthog(hub, pluginConfig), 'posthog')

// Add non-PostHog utilities to virtual machine
vm.freeze(imports['node-fetch'], 'fetch')
vm.freeze(AVAILABLE_IMPORTS['node-fetch'], 'fetch')
vm.freeze(createGoogle(), 'google')

vm.freeze(imports, '__pluginHostImports')
vm.freeze(AVAILABLE_IMPORTS, '__pluginHostImports')

if (process.env.NODE_ENV === 'test') {
vm.freeze(setTimeout, '__jestSetTimeout')
Expand Down
1 change: 0 additions & 1 deletion plugin-server/tests/worker/ingestion/event-pipeline/event-pipeline-integration.test.ts
View file
Expand Up @@ -52,7 +52,6 @@ describe('Event Pipeline integration test', () => {
hub.db.postgres,
hub.teamManager,
hub.organizationManager,
new Set(hub.FETCH_HOSTNAME_GUARD_TEAMS.split(',').filter(String).map(Number)),
hub.appMetrics,
undefined,
hub.EXTERNAL_REQUEST_TIMEOUT_MS
Expand Down
21 changes: 6 additions & 15 deletions plugin-server/tests/worker/ingestion/hooks.test.ts
View file
Expand Up @@ -2,7 +2,6 @@ import { DateTime } from 'luxon'
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 { AppMetrics } from '../../../src/worker/ingestion/app-metrics'
import {
Expand All @@ -17,9 +16,11 @@ import {
} from '../../../src/worker/ingestion/hooks'
import { Hook } from './../../../src/types'

jest.mock('../../../src/utils/env-utils')

describe('hooks', () => {
beforeEach(() => {
process.env.NODE_ENV = 'test'
})

describe('determineWebhookType', () => {
test('Slack', () => {
const webhookType = determineWebhookType('https://hooks.slack.com/services/')
Expand Down Expand Up @@ -475,7 +476,6 @@ describe('hooks', () => {
let hook: Hook

beforeEach(() => {
jest.mocked(isCloud).mockReturnValue(false) // Disable private IP guard
hook = {
id: 'id',
team_id: 1,
Expand All @@ -490,7 +490,6 @@ describe('hooks', () => {
{} as any,
{} as any,
{} as any,
new Set([hook.team_id]), // Hostname guard enabled
{ queueError: () => Promise.resolve(), queueMetric: () => Promise.resolve() } as unknown as AppMetrics,
undefined,
20000
Expand Down Expand Up @@ -523,8 +522,6 @@ 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, {
Expand Down Expand Up @@ -561,14 +558,8 @@ describe('hooks', () => {
})
})

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)
test('private IP hook forbidden in prod', async () => {
process.env.NODE_ENV = 'production'

await expect(
hookCommander.postRestHook({ ...hook, target: 'http://127.0.0.1' }, { event: 'foo' } as any)
Expand Down
3 changes: 1 addition & 2 deletions posthog/api/user.py
View file
Expand Up @@ -33,7 +33,6 @@
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,
Expand Down Expand Up @@ -470,7 +469,7 @@ def test_slack_webhook(request):
return JsonResponse({"error": "no webhook URL"})
message = {"text": "_Greetings_ from PostHog!"}
try:
if is_cloud(): # Protect against SSRF
if not settings.DEBUG:
raise_if_user_provided_url_unsafe(webhook)
response = requests.post(webhook, verify=False, json=message)

Expand Down

0 comments on commit 22bd594

Please sign in to comment.