mirror of
https://github.com/simstudioai/sim.git
synced 2026-09-24 15:45:35 +08:00
fix(security): bound response bodies in secureFetchWithPinnedIP by default (#6169)
secureFetchWithPinnedIP only capped a response body when the caller passed maxResponseBytes; with the option absent the body streamed into an unbounded Buffer.concat. Several tool proxies whose target host is user-supplied (Jupyter, ClickHouse, Grafana, 1Password Connect) and the RSS poller called it without that option, so an attacker-controlled server answering with an endless chunked body could grow the shared process heap until it was OOM-killed. Make the cap fail-safe: default to 100MB (and treat a non-positive value as the default) so there is no unlimited mode, then pass tighter explicit caps at the user-supplied-host sites. Responses that carry no body (HEAD, 204, 304) are exempted from the content-length pre-check — they advertise the resource size as metadata, which would otherwise spuriously fail a HEAD probe of a large file or an RSS conditional-GET 304.
This commit is contained in:
@@ -12,6 +12,7 @@ const { mockValidateDatabaseHost, mockSecureFetchWithPinnedIP, mockValidateSqlWh
|
||||
}))
|
||||
|
||||
vi.mock('@/lib/core/security/input-validation.server', () => ({
|
||||
MAX_JSON_API_RESPONSE_BYTES: 10 * 1024 * 1024,
|
||||
validateDatabaseHost: mockValidateDatabaseHost,
|
||||
secureFetchWithPinnedIP: mockSecureFetchWithPinnedIP,
|
||||
validateSqlWhereClause: mockValidateSqlWhereClause,
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
import {
|
||||
MAX_JSON_API_RESPONSE_BYTES,
|
||||
secureFetchWithPinnedIP,
|
||||
validateDatabaseHost,
|
||||
validateSqlWhereClause,
|
||||
@@ -96,6 +97,7 @@ async function clickhouseRequest(
|
||||
body: statement,
|
||||
timeout: REQUEST_TIMEOUT_MS,
|
||||
allowHttp: !config.secure,
|
||||
maxResponseBytes: MAX_JSON_API_RESPONSE_BYTES,
|
||||
})
|
||||
|
||||
const text = await response.text()
|
||||
|
||||
@@ -5,6 +5,7 @@ import { grafanaUpdateAlertRuleContract } from '@/lib/api/contracts/tools/grafan
|
||||
import { getValidationErrorMessage, parseRequest } from '@/lib/api/server'
|
||||
import { checkInternalAuth } from '@/lib/auth/hybrid'
|
||||
import {
|
||||
MAX_JSON_API_RESPONSE_BYTES,
|
||||
secureFetchWithPinnedIP,
|
||||
validateUrlWithDNS,
|
||||
} from '@/lib/core/security/input-validation.server'
|
||||
@@ -76,6 +77,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
|
||||
const getResponse = await secureFetchWithPinnedIP(getUrl, getValidation.resolvedIP, {
|
||||
method: 'GET',
|
||||
headers: getHeaders,
|
||||
maxResponseBytes: MAX_JSON_API_RESPONSE_BYTES,
|
||||
})
|
||||
|
||||
if (!getResponse.ok) {
|
||||
@@ -205,6 +207,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
|
||||
method: 'PUT',
|
||||
headers,
|
||||
body: JSON.stringify(updatedRule),
|
||||
maxResponseBytes: MAX_JSON_API_RESPONSE_BYTES,
|
||||
})
|
||||
|
||||
if (!updateResponse.ok) {
|
||||
|
||||
@@ -5,6 +5,7 @@ import { grafanaUpdateDashboardContract } from '@/lib/api/contracts/tools/grafan
|
||||
import { getValidationErrorMessage, parseRequest } from '@/lib/api/server'
|
||||
import { checkInternalAuth } from '@/lib/auth/hybrid'
|
||||
import {
|
||||
MAX_JSON_API_RESPONSE_BYTES,
|
||||
secureFetchWithPinnedIP,
|
||||
validateUrlWithDNS,
|
||||
} from '@/lib/core/security/input-validation.server'
|
||||
@@ -75,6 +76,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
|
||||
const getResponse = await secureFetchWithPinnedIP(getUrl, getValidation.resolvedIP, {
|
||||
method: 'GET',
|
||||
headers: getHeaders,
|
||||
maxResponseBytes: MAX_JSON_API_RESPONSE_BYTES,
|
||||
})
|
||||
|
||||
if (!getResponse.ok) {
|
||||
@@ -166,6 +168,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
|
||||
method: 'POST',
|
||||
headers,
|
||||
body: JSON.stringify(body),
|
||||
maxResponseBytes: MAX_JSON_API_RESPONSE_BYTES,
|
||||
})
|
||||
|
||||
if (!updateResponse.ok) {
|
||||
|
||||
@@ -5,6 +5,7 @@ import { grafanaUpdateFolderContract } from '@/lib/api/contracts/tools/grafana'
|
||||
import { getValidationErrorMessage, parseRequest } from '@/lib/api/server'
|
||||
import { checkInternalAuth } from '@/lib/auth/hybrid'
|
||||
import {
|
||||
MAX_JSON_API_RESPONSE_BYTES,
|
||||
secureFetchWithPinnedIP,
|
||||
validateUrlWithDNS,
|
||||
} from '@/lib/core/security/input-validation.server'
|
||||
@@ -73,6 +74,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
|
||||
const getResponse = await secureFetchWithPinnedIP(folderUrl, urlValidation.resolvedIP, {
|
||||
method: 'GET',
|
||||
headers,
|
||||
maxResponseBytes: MAX_JSON_API_RESPONSE_BYTES,
|
||||
})
|
||||
|
||||
if (!getResponse.ok) {
|
||||
@@ -104,6 +106,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
|
||||
method: 'PUT',
|
||||
headers,
|
||||
body: JSON.stringify(body),
|
||||
maxResponseBytes: MAX_JSON_API_RESPONSE_BYTES,
|
||||
})
|
||||
|
||||
if (!updateResponse.ok) {
|
||||
|
||||
@@ -5,6 +5,7 @@ import { jupyterProxyContract } from '@/lib/api/contracts/tools/jupyter'
|
||||
import { parseRequest } from '@/lib/api/server'
|
||||
import { checkInternalAuth } from '@/lib/auth/hybrid'
|
||||
import {
|
||||
MAX_JSON_API_RESPONSE_BYTES,
|
||||
secureFetchWithPinnedIP,
|
||||
validateUrlWithDNS,
|
||||
} from '@/lib/core/security/input-validation.server'
|
||||
@@ -77,6 +78,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
|
||||
body: hasBody ? JSON.stringify(data.body) : undefined,
|
||||
allowHttp: true,
|
||||
maxRedirects: 0,
|
||||
maxResponseBytes: MAX_JSON_API_RESPONSE_BYTES,
|
||||
})
|
||||
|
||||
const text = await upstream.text()
|
||||
|
||||
@@ -5,6 +5,7 @@ import { jupyterUploadContract } from '@/lib/api/contracts/storage-transfer'
|
||||
import { parseRequest } from '@/lib/api/server'
|
||||
import { checkInternalAuth } from '@/lib/auth/hybrid'
|
||||
import {
|
||||
MAX_JSON_API_RESPONSE_BYTES,
|
||||
secureFetchWithPinnedIP,
|
||||
validateUrlWithDNS,
|
||||
} from '@/lib/core/security/input-validation.server'
|
||||
@@ -118,6 +119,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
|
||||
}),
|
||||
allowHttp: true,
|
||||
maxRedirects: 0,
|
||||
maxResponseBytes: MAX_JSON_API_RESPONSE_BYTES,
|
||||
})
|
||||
|
||||
if (!response.ok) {
|
||||
|
||||
@@ -6,6 +6,7 @@ import { onePasswordGetItemFileContract } from '@/lib/api/contracts/tools/onepas
|
||||
import { parseRequest, validationErrorResponse } from '@/lib/api/server'
|
||||
import { checkInternalAuth } from '@/lib/auth/hybrid'
|
||||
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
|
||||
import { MAX_FILE_SIZE } from '@/lib/uploads/utils/validation'
|
||||
import {
|
||||
connectRequest,
|
||||
createOnePasswordClient,
|
||||
@@ -80,6 +81,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
|
||||
apiKey: creds.apiKey!,
|
||||
path: `/v1/vaults/${params.vaultId}/items/${params.itemId}/files/${params.fileId}/content`,
|
||||
method: 'GET',
|
||||
maxResponseBytes: MAX_FILE_SIZE,
|
||||
})
|
||||
if (!contentResponse.ok) {
|
||||
const errorData = await contentResponse.json().catch(() => ({}))
|
||||
|
||||
@@ -17,7 +17,10 @@ import { toError } from '@sim/utils/errors'
|
||||
import { generateId } from '@sim/utils/id'
|
||||
import * as ipaddr from 'ipaddr.js'
|
||||
import { isHosted } from '@/lib/core/config/env-flags'
|
||||
import { secureFetchWithPinnedIP } from '@/lib/core/security/input-validation.server'
|
||||
import {
|
||||
MAX_JSON_API_RESPONSE_BYTES,
|
||||
secureFetchWithPinnedIP,
|
||||
} from '@/lib/core/security/input-validation.server'
|
||||
|
||||
/** Connect-format field type strings returned by normalization. */
|
||||
type ConnectFieldType =
|
||||
@@ -344,7 +347,13 @@ export interface ConnectResponse {
|
||||
arrayBuffer: () => Promise<ArrayBuffer>
|
||||
}
|
||||
|
||||
/** Proxy a request to the 1Password Connect Server. */
|
||||
/**
|
||||
* Proxy a request to the 1Password Connect Server.
|
||||
*
|
||||
* The Connect server is self-hosted at a user-supplied `serverUrl`, so the response body
|
||||
* is always capped. JSON endpoints use {@link MAX_JSON_API_RESPONSE_BYTES}; callers
|
||||
* downloading file content pass a larger `maxResponseBytes` explicitly.
|
||||
*/
|
||||
export async function connectRequest(options: {
|
||||
serverUrl: string
|
||||
apiKey: string
|
||||
@@ -352,6 +361,7 @@ export async function connectRequest(options: {
|
||||
method: string
|
||||
body?: unknown
|
||||
query?: string
|
||||
maxResponseBytes?: number
|
||||
}): Promise<ConnectResponse> {
|
||||
const resolvedIP = await validateConnectServerUrl(options.serverUrl)
|
||||
|
||||
@@ -372,6 +382,7 @@ export async function connectRequest(options: {
|
||||
headers,
|
||||
body: options.body ? JSON.stringify(options.body) : undefined,
|
||||
allowHttp: true,
|
||||
maxResponseBytes: options.maxResponseBytes ?? MAX_JSON_API_RESPONSE_BYTES,
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
@@ -358,6 +358,11 @@ export interface SecureFetchOptions {
|
||||
body?: string | Buffer | Uint8Array
|
||||
timeout?: number
|
||||
maxRedirects?: number
|
||||
/**
|
||||
* Maximum bytes read from the response body. Defaults to
|
||||
* {@link DEFAULT_MAX_RESPONSE_BYTES} — there is deliberately no "unlimited" mode, since
|
||||
* many callers target a user-supplied host that can stream an endless body.
|
||||
*/
|
||||
maxResponseBytes?: number
|
||||
signal?: AbortSignal
|
||||
/** Drop the Authorization header when following a redirect, so it is not sent to the redirect target's origin. */
|
||||
@@ -414,6 +419,19 @@ export interface SecureFetchResponse {
|
||||
|
||||
const DEFAULT_MAX_REDIRECTS = 5
|
||||
|
||||
/**
|
||||
* Fail-safe ceiling applied by {@link secureFetchWithPinnedIP} when the caller does not
|
||||
* pass `maxResponseBytes`. Many callers fetch a user-supplied host, so an omitted cap
|
||||
* would let a malicious upstream stream an endless chunked body into memory until the
|
||||
* process is OOM-killed. Set to the platform's largest legitimate payload (100MB, matching
|
||||
* the upload limit); callers that need more must opt in explicitly, and callers handling
|
||||
* small JSON should pass a much tighter cap.
|
||||
*/
|
||||
export const DEFAULT_MAX_RESPONSE_BYTES = 100 * 1024 * 1024
|
||||
|
||||
/** Response cap for JSON/control-plane proxies to user-supplied hosts. */
|
||||
export const MAX_JSON_API_RESPONSE_BYTES = 10 * 1024 * 1024
|
||||
|
||||
function isRedirectStatus(status: number): boolean {
|
||||
return status >= 300 && status < 400 && status !== 304
|
||||
}
|
||||
@@ -937,6 +955,10 @@ export function createPinnedFetchWithDispatcher(
|
||||
* Performs a fetch with IP pinning to prevent DNS rebinding attacks.
|
||||
* Uses the pre-resolved IP address while preserving the original hostname for TLS SNI.
|
||||
* Follows redirects securely by validating each redirect target.
|
||||
*
|
||||
* The response body is always bounded — `options.maxResponseBytes` when supplied (and
|
||||
* positive), otherwise {@link DEFAULT_MAX_RESPONSE_BYTES}. Exceeding the cap rejects with
|
||||
* a {@link PayloadSizeLimitError} and destroys the socket.
|
||||
*/
|
||||
export async function secureFetchWithPinnedIP(
|
||||
url: string,
|
||||
@@ -945,7 +967,11 @@ export async function secureFetchWithPinnedIP(
|
||||
redirectCount = 0
|
||||
): Promise<SecureFetchResponse> {
|
||||
const maxRedirects = options.maxRedirects ?? DEFAULT_MAX_REDIRECTS
|
||||
const maxResponseBytes = options.maxResponseBytes
|
||||
const requestedMaxResponseBytes = options.maxResponseBytes
|
||||
const maxResponseBytes =
|
||||
typeof requestedMaxResponseBytes === 'number' && requestedMaxResponseBytes > 0
|
||||
? requestedMaxResponseBytes
|
||||
: DEFAULT_MAX_RESPONSE_BYTES
|
||||
|
||||
return new Promise((resolve, reject) => {
|
||||
const parsed = new URL(url)
|
||||
@@ -1037,8 +1063,15 @@ export async function secureFetchWithPinnedIP(
|
||||
}
|
||||
}
|
||||
|
||||
// Responses that carry no body (HEAD, 204, 304) may still advertise the resource's full
|
||||
// size in content-length. That is metadata, not a payload, so it must not trip the cap —
|
||||
// otherwise a HEAD probe of a large file, or a conditional-GET 304, would fail spuriously.
|
||||
const isBodylessResponse =
|
||||
(requestOptions.method || 'GET').toUpperCase() === 'HEAD' ||
|
||||
statusCode === 204 ||
|
||||
statusCode === 304
|
||||
const contentLength = headersRecord['content-length']
|
||||
if (typeof maxResponseBytes === 'number' && maxResponseBytes > 0 && contentLength) {
|
||||
if (contentLength && !isBodylessResponse) {
|
||||
const parsedLength = Number.parseInt(contentLength, 10)
|
||||
if (Number.isFinite(parsedLength) && parsedLength > maxResponseBytes) {
|
||||
cleanupAbort()
|
||||
@@ -1074,11 +1107,7 @@ export async function secureFetchWithPinnedIP(
|
||||
start(controller) {
|
||||
nodeRes.on('data', (chunk: Buffer) => {
|
||||
totalBytes += chunk.length
|
||||
if (
|
||||
typeof maxResponseBytes === 'number' &&
|
||||
maxResponseBytes > 0 &&
|
||||
totalBytes > maxResponseBytes
|
||||
) {
|
||||
if (totalBytes > maxResponseBytes) {
|
||||
cleanupAbort()
|
||||
controller.error(
|
||||
new PayloadSizeLimitError({
|
||||
|
||||
@@ -0,0 +1,102 @@
|
||||
/**
|
||||
* @vitest-environment node
|
||||
*/
|
||||
import http from 'node:http'
|
||||
import type { AddressInfo } from 'node:net'
|
||||
import { afterEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
vi.mock('@sim/security/dns', () => ({
|
||||
resolveHostAddresses: vi.fn(),
|
||||
preferIpv4: (addresses: string[]) => addresses[0],
|
||||
}))
|
||||
|
||||
vi.mock('@/lib/core/config/env-flags', () => ({
|
||||
isHosted: false,
|
||||
isPrivateDatabaseHostsAllowed: false,
|
||||
getProxyUrl: () => undefined,
|
||||
}))
|
||||
|
||||
import {
|
||||
DEFAULT_MAX_RESPONSE_BYTES,
|
||||
secureFetchWithPinnedIP,
|
||||
} from '@/lib/core/security/input-validation.server'
|
||||
|
||||
const servers: http.Server[] = []
|
||||
|
||||
afterEach(() => {
|
||||
for (const server of servers.splice(0)) server.close()
|
||||
})
|
||||
|
||||
/** Starts a throwaway loopback server and returns its origin. */
|
||||
async function startServer(handler: http.RequestListener): Promise<string> {
|
||||
const server = http.createServer(handler)
|
||||
servers.push(server)
|
||||
await new Promise<void>((resolve) => server.listen(0, '127.0.0.1', resolve))
|
||||
return `http://127.0.0.1:${(server.address() as AddressInfo).port}`
|
||||
}
|
||||
|
||||
describe('secureFetchWithPinnedIP response cap', () => {
|
||||
it('rejects a body that exceeds an explicit cap instead of buffering it', async () => {
|
||||
const origin = await startServer((_req, res) => {
|
||||
res.writeHead(200, { 'Content-Type': 'application/octet-stream' })
|
||||
// Chunked (no content-length), so only the streaming counter can catch it.
|
||||
for (let i = 0; i < 8; i++) res.write(Buffer.alloc(64 * 1024, 0x41))
|
||||
res.end()
|
||||
})
|
||||
|
||||
const response = await secureFetchWithPinnedIP(origin, '127.0.0.1', {
|
||||
maxResponseBytes: 64 * 1024,
|
||||
})
|
||||
|
||||
await expect(response.text()).rejects.toThrow(/response body/i)
|
||||
})
|
||||
|
||||
it('rejects a content-length above the default cap when the caller passes no cap', async () => {
|
||||
const origin = await startServer((_req, res) => {
|
||||
res.writeHead(200, {
|
||||
'Content-Type': 'application/json',
|
||||
'Content-Length': String(DEFAULT_MAX_RESPONSE_BYTES + 1),
|
||||
})
|
||||
res.end('{}')
|
||||
})
|
||||
|
||||
await expect(secureFetchWithPinnedIP(origin, '127.0.0.1', {})).rejects.toThrow(/response body/i)
|
||||
})
|
||||
|
||||
it('reads a body that fits under the default cap', async () => {
|
||||
const origin = await startServer((_req, res) => {
|
||||
res.writeHead(200, { 'Content-Type': 'application/json' })
|
||||
res.end('{"ok":true}')
|
||||
})
|
||||
|
||||
const response = await secureFetchWithPinnedIP(origin, '127.0.0.1', {})
|
||||
|
||||
expect(await response.text()).toBe('{"ok":true}')
|
||||
})
|
||||
|
||||
it('does not trip on a HEAD response advertising a size above the cap', async () => {
|
||||
const origin = await startServer((_req, res) => {
|
||||
res.writeHead(200, {
|
||||
'Content-Type': 'video/mp4',
|
||||
'Content-Length': String(DEFAULT_MAX_RESPONSE_BYTES + 1),
|
||||
})
|
||||
res.end()
|
||||
})
|
||||
|
||||
const response = await secureFetchWithPinnedIP(origin, '127.0.0.1', { method: 'HEAD' })
|
||||
|
||||
expect(response.status).toBe(200)
|
||||
expect(response.headers.get('content-type')).toBe('video/mp4')
|
||||
})
|
||||
|
||||
it('does not trip on a 304 advertising a size above the cap', async () => {
|
||||
const origin = await startServer((_req, res) => {
|
||||
res.writeHead(304, { 'Content-Length': String(DEFAULT_MAX_RESPONSE_BYTES + 1) })
|
||||
res.end()
|
||||
})
|
||||
|
||||
const response = await secureFetchWithPinnedIP(origin, '127.0.0.1', {})
|
||||
|
||||
expect(response.status).toBe(304)
|
||||
})
|
||||
})
|
||||
@@ -20,6 +20,17 @@ import { processPolledWebhookEvent } from '@/lib/webhooks/processor'
|
||||
|
||||
const MAX_GUIDS_TO_TRACK = 500
|
||||
|
||||
/**
|
||||
* Cap on the feed body read from `feedUrl`. The URL is attacker-choosable (anyone can save
|
||||
* an RSS trigger pointing at their own host) and the poller runs unattended, so an
|
||||
* unbounded body would let a malicious feed grow the polling worker's heap until it is
|
||||
* OOM-killed. This also bounds the XML handed to `rss-parser`, which has no input-size
|
||||
* limit of its own. Set well above any real feed (they are typically well under 1MB) so the
|
||||
* bound never trips a legitimate poll. The request strips `accept-encoding`, so this counts
|
||||
* decoded bytes.
|
||||
*/
|
||||
const MAX_RSS_FEED_BYTES = 10 * 1024 * 1024
|
||||
|
||||
interface RssWebhookConfig {
|
||||
feedUrl: string
|
||||
lastCheckedTimestamp?: string
|
||||
@@ -208,6 +219,7 @@ async function fetchNewRssItems(
|
||||
const response = await secureFetchWithPinnedIP(config.feedUrl, urlValidation.resolvedIP!, {
|
||||
headers,
|
||||
timeout: 30000,
|
||||
maxResponseBytes: MAX_RSS_FEED_BYTES,
|
||||
})
|
||||
|
||||
if (response.status === 304) {
|
||||
|
||||
@@ -29,6 +29,8 @@ export const inputValidationMockFns = {
|
||||
* ```
|
||||
*/
|
||||
export const inputValidationMock = {
|
||||
DEFAULT_MAX_RESPONSE_BYTES: 100 * 1024 * 1024,
|
||||
MAX_JSON_API_RESPONSE_BYTES: 10 * 1024 * 1024,
|
||||
validateUrlWithDNS: inputValidationMockFns.mockValidateUrlWithDNS,
|
||||
validateAndPinProxyUrl: inputValidationMockFns.mockValidateAndPinProxyUrl,
|
||||
validateDatabaseHost: inputValidationMockFns.mockValidateDatabaseHost,
|
||||
|
||||
Reference in New Issue
Block a user