Skip to content

Commit 7cdea0d

Browse files
fix: validate deployment content-server url against a catalyst allowlist (#122)
fix: validate deployment content-server url against a content-server allowlist the deployment event's contentServerUrls is attacker-influenced and the registry fetches the entity from it (catalyst / worlds), so: - the message processor drops any deployment carrying an off-allowlist content-server host before dispatching to ANY handler, so a poisoned message can't leave an orphaned queue-status entry via the status handler. the deployment handler keeps the same guard as defense in depth. - content fetches (catalyst content client + worlds) no longer follow redirects, so an allowlisted host can't 30x to an internal resource. the allowlist is sourced from the required ALLOWED_CONTENT_SERVER_HOSTS env var (set per-env in the definitions repo, with known defaults in .env.default). entityId is not gated here: in the registry it only reaches parameterized sql / cache keys, not a filesystem path or s3 key. related to decentraland/asset-bundle-converter#306
1 parent 1c5a484 commit 7cdea0d

10 files changed

Lines changed: 314 additions & 16 deletions

File tree

.env.default

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,10 @@ REGISTRY_URL=<URL>
1818
ASSET_BUNDLE_ADMIN_URL=<URL>
1919
PROFILE_IMAGES_URL=<URL>
2020

21+
# SSRF allowlist for the deployment event's content-server URL (issue #306).
22+
# Required — comma-separated content-server hosts.
23+
ALLOWED_CONTENT_SERVER_HOSTS=peer.decentraland.org,peer-ec1.decentraland.org,peer-ec2.decentraland.org,peer-wc1.decentraland.org,peer-eu1.decentraland.org,peer-ap1.decentraland.org,interconnected.online,peer.melonwave.com,peer.uadevops.com,peer.dclnodes.io,worlds-content-server.decentraland.org,peer-testing.decentraland.org,peer-testing-2.decentraland.org,peer.decentraland.zone,peer-ap1.decentraland.zone,worlds-content-server.decentraland.zone
24+
2125
# Sync
2226
DISABLE_PROFILE_SYNC=true
2327

.env.test

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,3 +3,4 @@ PROFILE_IMAGES_URL=https://profile-images.decentraland.zone
33
DISABLE_PROFILE_SYNC=true
44
CATALYST_WITH_HISTORICAL_DATA=https://peer.decentraland.zone
55
SERVICE_URL=https://asset-bundle-registry.decentraland.zone
6+
ALLOWED_CONTENT_SERVER_HOSTS=peer.decentraland.org,peer.decentraland.zone,worlds-content-server.decentraland.org,worlds-content-server.decentraland.zone

src/adapters/catalyst.ts

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { Entity, EntityType } from '@dcl/schemas'
2+
import { IFetchComponent, RequestOptions } from '@well-known-components/interfaces'
23
import { ContentClient, createContentClient, createLambdasClient } from 'dcl-catalyst-client'
34
import { Profile } from 'dcl-catalyst-client/dist/client/specs/lambdas-client'
45

@@ -13,8 +14,18 @@ export async function createCatalystAdapter({
1314
}: Pick<AppComponents, 'config' | 'logs' | 'fetch'>): Promise<ICatalystComponent> {
1415
const logger = logs.getLogger('catalyst-client')
1516

17+
// Defense-in-depth (issue #306): content fetches must NOT follow redirects — an
18+
// allowlisted host could otherwise 30x to an internal resource. Treat a redirect
19+
// as a hard error instead of silently following it.
20+
const contentFetcher: IFetchComponent = {
21+
fetch: (url, init?: RequestOptions) => fetch.fetch(url, { ...init, redirect: 'error' })
22+
}
23+
1624
const catalystLoadBalancer = await config.requireString('CATALYST_LOADBALANCER_HOST')
17-
const defaultContentClient = createContentClient({ fetcher: fetch, url: ensureContentUrl(catalystLoadBalancer) })
25+
const defaultContentClient = createContentClient({
26+
fetcher: contentFetcher,
27+
url: ensureContentUrl(catalystLoadBalancer)
28+
})
1829

1930
// We use a historical catalyst (instead of the load balancer) because some official nodes
2031
// have garbage-collected old profiles. The historical catalyst retains all profile data
@@ -90,7 +101,7 @@ export async function createCatalystAdapter({
90101
function getContentClientOrDefault(contentServerUrl?: string): ContentClient {
91102
const contentClientToReturn = contentServerUrl
92103
? createContentClient({
93-
fetcher: fetch,
104+
fetcher: contentFetcher,
94105
url: ensureContentUrl(contentServerUrl)
95106
})
96107
: defaultContentClient

src/adapters/worlds.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,9 @@ export async function createWorldsAdapter({
2727
): Promise<Entity | null> {
2828
try {
2929
const url = `${contentServerUrl}/contents/${entityId}`
30-
const response = await fetch.fetch(url)
30+
// Defense-in-depth (issue #306): do not follow redirects — an allowlisted
31+
// world content server could otherwise 30x to an internal resource.
32+
const response = await fetch.fetch(url, { redirect: 'error' })
3133

3234
if (!response.ok) {
3335
return null

src/logic/handlers/deployment-handler.ts

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,24 +2,36 @@ import { Entity } from '@dcl/schemas'
22
import { AppComponents, IEventHandlerComponent, EventHandlerName, EventHandlerResult, Registry } from '../../types'
33
import { DeploymentToSqs } from '@dcl/schemas/dist/misc/deployments-to-sqs'
44
import { Authenticator } from '@dcl/crypto'
5+
import { isAllowedContentServerUrl } from '../validation'
56

6-
export const createDeploymentEventHandler = ({
7-
registry,
8-
catalyst,
9-
worlds,
10-
db,
11-
logs
12-
}: Pick<
13-
AppComponents,
14-
'catalyst' | 'worlds' | 'db' | 'logs' | 'registry'
15-
>): IEventHandlerComponent<DeploymentToSqs> => {
7+
export const createDeploymentEventHandler = (
8+
{ registry, catalyst, worlds, db, logs }: Pick<AppComponents, 'catalyst' | 'worlds' | 'db' | 'logs' | 'registry'>,
9+
allowedContentServerHosts: Set<string>
10+
): IEventHandlerComponent<DeploymentToSqs> => {
1611
const HANDLER_NAME = EventHandlerName.DEPLOYMENT
1712
const logger = logs.getLogger('deployment-handler')
1813

1914
return {
2015
handle: async (event: DeploymentToSqs): Promise<EventHandlerResult> => {
2116
let entity: Entity | null
2217
try {
18+
// SSRF guard (issue #306): contentServerUrls rides in the SQS payload and
19+
// the entity is fetched from it (catalyst / worlds), so reject any
20+
// off-allowlist host and skip the event (ok: true — deterministic, so
21+
// retrying wouldn't help). Validate EVERY entry, not just [0]: the whole
22+
// array is preserved in the message. entityId isn't gated here: it only
23+
// reaches parameterized SQL / cache keys, not a filesystem path or S3 key.
24+
const disallowedContentServerUrl = event.contentServerUrls?.find(
25+
(url) => !isAllowedContentServerUrl(url, allowedContentServerHosts)
26+
)
27+
if (disallowedContentServerUrl) {
28+
logger.warn('Skipping deployment: a contentServerUrl is not an allowed content server (SSRF guard)', {
29+
entityId: event.entity.entityId,
30+
contentServerUrl: String(disallowedContentServerUrl).slice(0, 120)
31+
})
32+
return { ok: true, handlerName: HANDLER_NAME }
33+
}
34+
2335
const registryAlreadyExists = await db.getRegistryById(event.entity.entityId)
2436

2537
if (registryAlreadyExists) {

src/logic/message-processor.ts

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import { createUndeploymentEventHandler } from './handlers/undeployment-handler'
2121
import { createWorldUndeploymentEventHandler } from './handlers/world-undeployment-handler'
2222
import { createSpawnCoordinateEventHandler } from './handlers/spawn-coordinate-handler'
2323
import { DeploymentToSqs } from '@dcl/schemas/dist/misc/deployments-to-sqs'
24+
import { isAllowedContentServerUrl, parseAllowedContentServerHosts } from './validation'
2425

2526
export async function createMessageProcessorComponent({
2627
catalyst,
@@ -37,6 +38,16 @@ export async function createMessageProcessorComponent({
3738
>): Promise<IMessageProcessorComponent> {
3839
const MAX_RETRIES: number = (await config.getNumber('MAX_RETRIES')) || 3
3940
const log = logs.getLogger('message-processor')
41+
42+
// Strict host allowlist for the (attacker-influenced) deployment content-server
43+
// URL (issue #306). Sourced entirely from ALLOWED_CONTENT_SERVER_HOSTS (set per-env in the
44+
// definitions repo) — required, with no built-in fallback list.
45+
const allowedContentServerHosts = parseAllowedContentServerHosts(
46+
await config.requireString('ALLOWED_CONTENT_SERVER_HOSTS')
47+
)
48+
if (allowedContentServerHosts.size === 0) {
49+
throw new Error('ALLOWED_CONTENT_SERVER_HOSTS is set but contains no valid catalyst hosts')
50+
}
4051
const processors: IEventHandlerComponent<
4152
| DeploymentToSqs
4253
| AssetBundleConversionManuallyQueuedEvent
@@ -45,7 +56,7 @@ export async function createMessageProcessorComponent({
4556
| WorldUndeploymentEvent
4657
| WorldSpawnCoordinateSetEvent
4758
>[] = [
48-
createDeploymentEventHandler({ catalyst, worlds, registry, db, logs }),
59+
createDeploymentEventHandler({ catalyst, worlds, registry, db, logs }, allowedContentServerHosts),
4960
createTexturesEventHandler({
5061
db,
5162
logs,
@@ -75,6 +86,25 @@ export async function createMessageProcessorComponent({
7586
}
7687
}
7788

89+
// SSRF / poisoned-message guard at the dispatch level (issue #306): a
90+
// deployment carrying an off-allowlist content-server host is dropped before
91+
// ANY handler runs. The deployment handler already skips it, but other
92+
// handlers (e.g. status) would otherwise still act on it and leave an
93+
// orphaned queue-status entry. Drop it (ok, no retry) — it's deterministic.
94+
const disallowedContentServerUrl = Array.isArray(message?.contentServerUrls)
95+
? message.contentServerUrls.find((url: string) => !isAllowedContentServerUrl(url, allowedContentServerHosts))
96+
: undefined
97+
if (disallowedContentServerUrl) {
98+
log.warn('Dropping message: a contentServerUrl is not an allowed content server (SSRF/allowlist guard)', {
99+
entityId: message?.entity?.entityId,
100+
contentServerUrl: String(disallowedContentServerUrl).slice(0, 120)
101+
})
102+
return {
103+
ok: true,
104+
failedHandlers: []
105+
}
106+
}
107+
78108
log.debug('Processing', { message })
79109

80110
const handlers:

src/logic/validation.ts

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
/**
2+
* SSRF guard for deployment events the registry consumes. Mirrors the
3+
* asset-bundle-converter (issue #306): the deployment event's
4+
* `contentServerUrls[0]` is attacker-influenced and the registry fetches the
5+
* entity from it (catalyst / worlds), so it is pinned to a strict HTTPS +
6+
* exact-host allowlist. (entityId is not gated here — in the registry it only
7+
* reaches parameterized SQL / cache keys, not a filesystem path or S3 key.)
8+
*
9+
* The allowlist is sourced entirely from the `ALLOWED_CONTENT_SERVER_HOSTS` env var (set
10+
* per-environment in the `definitions` repo) — there is no built-in default.
11+
*/
12+
13+
/**
14+
* Normalize one allowlist entry to a bare lowercase hostname. Accepts a bare
15+
* host or a full URL; returns undefined for blank/unparseable entries.
16+
*/
17+
function normalizeContentServerHost(entry: string): string | undefined {
18+
const trimmed = entry.trim().toLowerCase()
19+
if (trimmed.length === 0) return undefined
20+
if (trimmed.includes('/')) {
21+
try {
22+
const withScheme = /^[a-z][a-z0-9+.-]*:\/\//.test(trimmed) ? trimmed : `https://${trimmed}`
23+
const host = new URL(withScheme).hostname.replace(/^\[|\]$/g, '').toLowerCase()
24+
return host.length > 0 ? host : undefined
25+
} catch {
26+
return undefined
27+
}
28+
}
29+
return trimmed
30+
}
31+
32+
/**
33+
* Parse the `ALLOWED_CONTENT_SERVER_HOSTS` env var (comma-separated catalyst hosts) into a
34+
* Set of normalized hostnames. No built-in fallback list — the caller requires
35+
* the var and rejects an empty result.
36+
*/
37+
export function parseAllowedContentServerHosts(raw: string | undefined): Set<string> {
38+
const hosts = (raw ?? '')
39+
.split(',')
40+
.map(normalizeContentServerHost)
41+
.filter((h): h is string => h !== undefined)
42+
return new Set(hosts)
43+
}
44+
45+
/** True when `raw` is an HTTPS URL whose host is exactly on the allowlist. */
46+
export function isAllowedContentServerUrl(raw: string, allowedHosts: Set<string>): boolean {
47+
let u: URL
48+
try {
49+
u = new URL(raw)
50+
} catch {
51+
return false
52+
}
53+
if (u.protocol !== 'https:') return false
54+
const host = u.hostname.replace(/^\[|\]$/g, '').toLowerCase()
55+
return allowedHosts.has(host)
56+
}

test/unit/logic/handlers/deployment-handler.spec.ts

Lines changed: 70 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { Entity, EntityType } from '@dcl/schemas'
22
import { AuthLinkType } from '@dcl/crypto'
33
import { DeploymentToSqs } from '@dcl/schemas/dist/misc/deployments-to-sqs'
44
import { createDeploymentEventHandler } from '../../../../src/logic/handlers/deployment-handler'
5+
import { parseAllowedContentServerHosts } from '../../../../src/logic/validation'
56
import { EventHandlerName, Registry } from '../../../../src/types'
67
import { createDbMockComponent } from '../../mocks/db'
78
import { createLogMockComponent } from '../../mocks/logs'
@@ -10,6 +11,10 @@ import { createWorldsMockComponent } from '../../mocks/worlds'
1011
import { createRegistryMockComponent } from '../../mocks/registry'
1112

1213
describe('when handling deployment events', () => {
14+
const allowedContentServerHosts = parseAllowedContentServerHosts(
15+
'peer.decentraland.org, worlds-content-server.decentraland.org'
16+
)
17+
1318
const createDeploymentEvent = (entityId: string, contentServerUrls?: string[]): DeploymentToSqs => ({
1419
entity: {
1520
entityId,
@@ -49,7 +54,7 @@ describe('when handling deployment events', () => {
4954
catalyst = createCatalystMockComponent()
5055
worlds = createWorldsMockComponent()
5156
registry = createRegistryMockComponent()
52-
handler = createDeploymentEventHandler({ logs, db, catalyst, worlds, registry })
57+
handler = createDeploymentEventHandler({ logs, db, catalyst, worlds, registry }, allowedContentServerHosts)
5358
})
5459

5560
afterEach(() => {
@@ -95,7 +100,7 @@ describe('when handling deployment events', () => {
95100
catalyst = createCatalystMockComponent()
96101
worlds = createWorldsMockComponent()
97102
registry = createRegistryMockComponent()
98-
handler = createDeploymentEventHandler({ logs, db, catalyst, worlds, registry })
103+
handler = createDeploymentEventHandler({ logs, db, catalyst, worlds, registry }, allowedContentServerHosts)
99104
})
100105

101106
afterEach(() => {
@@ -208,6 +213,28 @@ describe('when handling deployment events', () => {
208213
})
209214
})
210215

216+
describe('and the deployment has an empty contentServerUrls array', () => {
217+
let event: DeploymentToSqs
218+
let entity: Entity
219+
220+
beforeEach(() => {
221+
event = createDeploymentEvent('genesis-entity-id', [])
222+
entity = createEntity({ id: 'genesis-entity-id', pointers: ['0,0'] })
223+
db.getRegistryById.mockResolvedValue(null)
224+
worlds.isWorldDeployment.mockReturnValue(false)
225+
catalyst.getEntityById.mockResolvedValue(entity)
226+
registry.persistAndRotateStates.mockResolvedValue(undefined as any)
227+
})
228+
229+
it('should not reject it and should fetch from the default catalyst with no override', async () => {
230+
await handler.handle(event)
231+
232+
expect(catalyst.getEntityById).toHaveBeenCalledWith('genesis-entity-id', {
233+
overrideContentServerUrl: undefined
234+
})
235+
})
236+
})
237+
211238
describe('and the deployment is a Genesis City scene with a content server URL', () => {
212239
let event: DeploymentToSqs
213240
let entity: Entity
@@ -283,6 +310,47 @@ describe('when handling deployment events', () => {
283310
})
284311
})
285312

313+
describe('and the deployment points at a content server that is not an allowed catalyst', () => {
314+
let event: DeploymentToSqs
315+
316+
beforeEach(() => {
317+
event = createDeploymentEvent('genesis-entity-id', ['https://169.254.169.254/latest/meta-data/'])
318+
db.getRegistryById.mockResolvedValue(null)
319+
worlds.isWorldDeployment.mockReturnValue(false)
320+
})
321+
322+
it('should skip the event without fetching the entity (SSRF guard)', async () => {
323+
const result = await handler.handle(event)
324+
325+
expect(result.ok).toBe(true)
326+
expect(result.handlerName).toBe(EventHandlerName.DEPLOYMENT)
327+
expect(catalyst.getEntityById).not.toHaveBeenCalled()
328+
expect(registry.persistAndRotateStates).not.toHaveBeenCalled()
329+
})
330+
})
331+
332+
describe('and a later contentServerUrl entry is not an allowed catalyst', () => {
333+
let event: DeploymentToSqs
334+
335+
beforeEach(() => {
336+
// First entry is allowlisted, second is not — every entry must be checked.
337+
event = createDeploymentEvent('genesis-entity-id', [
338+
'https://peer.decentraland.org/content',
339+
'https://evil.internal/ssrf'
340+
])
341+
db.getRegistryById.mockResolvedValue(null)
342+
worlds.isWorldDeployment.mockReturnValue(false)
343+
})
344+
345+
it('should skip the event even though the first URL is allowlisted', async () => {
346+
const result = await handler.handle(event)
347+
348+
expect(result.ok).toBe(true)
349+
expect(catalyst.getEntityById).not.toHaveBeenCalled()
350+
expect(registry.persistAndRotateStates).not.toHaveBeenCalled()
351+
})
352+
})
353+
286354
describe('and the entity is not found', () => {
287355
let event: DeploymentToSqs
288356

test/unit/logic/message-processor.spec.ts

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ describe('message processor', () => {
2222
const logs = createLogMockComponent()
2323
const config = createConfigMockComponent()
2424
;(config.getNumber as jest.Mock).mockResolvedValue(3) // MAX_RETRIES = 3
25+
;(config.requireString as jest.Mock).mockResolvedValue('peer.decentraland.org') // ALLOWED_CONTENT_SERVER_HOSTS
2526

2627
// Create mock handlers
2728
const deploymentHandler: IEventHandlerComponent<DeploymentToSqs> = {
@@ -247,4 +248,22 @@ describe('message processor', () => {
247248
})
248249
})
249250
})
251+
252+
describe('when the message is a deployment with an off-allowlist content-server host', () => {
253+
it('should drop the message before any handler runs, so no handler creates an orphaned entry', async () => {
254+
const message = {
255+
entity: { entityId: '123', authChain: [] },
256+
contentServerUrls: ['https://evil.internal/ssrf']
257+
}
258+
259+
const processor = await createMessageProcessorComponent(mockComponents)
260+
261+
const result = await processor.process(message)
262+
263+
expect(result.ok).toBe(true)
264+
expect(deploymentHandler.handle).not.toHaveBeenCalled()
265+
expect(texturesHandler.handle).not.toHaveBeenCalled()
266+
expect(statusHandler.handle).not.toHaveBeenCalled()
267+
})
268+
})
250269
})

0 commit comments

Comments
 (0)