Skip to content

Commit 7bce07a

Browse files
waleedlatif1waleedlatif1
committed
fix(datadog): route every host builder through the allowlist, coerce scalars safely
Review round 1 on #7244 found the first pass incomplete, and both findings reproduce. The site allowlist only covered the shared `datadogApiUrl` and the logs intake. Ten tools build the host inline -- `const site = params.site || 'datadoghq.com'` in cancel_downtime, create_downtime, create_event, create_monitor, get_monitor, list_downtimes, list_monitors, query_logs, query_timeseries and submit_metrics -- so they never reached the validator while still attaching DD-API-KEY and, where the endpoint needs it, DD-APPLICATION-KEY. All ten now resolve through it. The new test sweeps the tool registry rather than naming tools, so a future tool that reintroduces an inline builder fails instead of shipping an unguarded request. `(value ?? '').toString()` was itself unsafe: an object whose `toString` is not a function, and one with a null prototype, both throw TypeError, and `String(value)` throws on the same two. That read sits above the try block, so it escaped the structured `success: false` result this operation promises. `normalizeScalarText` converts only the scalar kinds `String()` cannot fail on and returns '' otherwise, matching how `normalizeStringList` already treats a value of the wrong type. The identical hazard on `decision` two lines above is fixed with it as well.
1 parent f817089 commit 7bce07a

14 files changed

Lines changed: 136 additions & 26 deletions

‎apps/sim/lib/internal/managed-agent/operations/respond-tool-confirmation.ts‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { getErrorMessage } from '@sim/utils/errors'
22
import type { InternalToolOperationImplementation } from '@/lib/internal/tool-operations/types'
33
import { sendToolConfirmations } from '@/lib/managed-agents/session-client'
4-
import { normalizeStringList } from '@/tools/managed_agent/normalizers'
4+
import { normalizeScalarText, normalizeStringList } from '@/tools/managed_agent/normalizers'
55
import { resolveSessionTarget } from '@/tools/managed_agent/shared'
66
import type {
77
ManagedAgentToolConfirmationParams,
@@ -17,7 +17,7 @@ export const executeManagedAgentRespondToolConfirmationOperation: InternalToolOp
1717
return { success: false, output: emptyOutput, error: target.error }
1818
}
1919

20-
const decision = (params.decision ?? '').toString().trim().toLowerCase()
20+
const decision = normalizeScalarText(params.decision).toLowerCase()
2121
if (decision !== 'allow' && decision !== 'deny') {
2222
return {
2323
success: false,
@@ -36,10 +36,7 @@ export const executeManagedAgentRespondToolConfirmationOperation: InternalToolOp
3636
}
3737
}
3838

39-
/* Coerced the same way `decision` is above: this runs outside the try below, so a
40-
non-string arriving from a stored workflow would throw past every `success: false`
41-
path this operation otherwise returns. */
42-
const denyMessage = (params.denyMessage ?? '').toString().trim()
39+
const denyMessage = normalizeScalarText(params.denyMessage)
4340
try {
4441
await sendToolConfirmations({
4542
apiKey: target.apiKey,

‎apps/sim/tools/datadog/cancel_downtime.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import type { CancelDowntimeParams, CancelDowntimeResponse } from '@/tools/datadog/types'
2-
import { datadogErrorMessage, datadogPathSegment } from '@/tools/datadog/utils'
2+
import { datadogErrorMessage, datadogPathSegment, resolveDatadogSite } from '@/tools/datadog/utils'
33
import type { ToolConfig } from '@/tools/types'
44

55
export const cancelDowntimeTool: ToolConfig<CancelDowntimeParams, CancelDowntimeResponse> = {
@@ -37,7 +37,7 @@ export const cancelDowntimeTool: ToolConfig<CancelDowntimeParams, CancelDowntime
3737

3838
request: {
3939
url: (params) => {
40-
const site = params.site || 'datadoghq.com'
40+
const site = resolveDatadogSite(params.site)
4141
const downtimeId = datadogPathSegment(params.downtimeId)
4242
return `https://api.${site}/api/v2/downtime/${downtimeId}`
4343
},

‎apps/sim/tools/datadog/create_downtime.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,12 @@ import type {
33
CreateDowntimeResponse,
44
DowntimeAttributes,
55
} from '@/tools/datadog/types'
6-
import { datadogErrorMessage, parseMonitorIds, splitCommaList } from '@/tools/datadog/utils'
6+
import {
7+
datadogErrorMessage,
8+
parseMonitorIds,
9+
resolveDatadogSite,
10+
splitCommaList,
11+
} from '@/tools/datadog/utils'
712
import type { ToolConfig } from '@/tools/types'
813

914
export const createDowntimeTool: ToolConfig<CreateDowntimeParams, CreateDowntimeResponse> = {
@@ -85,7 +90,7 @@ export const createDowntimeTool: ToolConfig<CreateDowntimeParams, CreateDowntime
8590

8691
request: {
8792
url: (params) => {
88-
const site = params.site || 'datadoghq.com'
93+
const site = resolveDatadogSite(params.site)
8994
return `https://api.${site}/api/v2/downtime`
9095
},
9196
method: 'POST',

‎apps/sim/tools/datadog/create_event.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import type {
44
EventAlertType,
55
EventPriority,
66
} from '@/tools/datadog/types'
7-
import { datadogErrorMessage } from '@/tools/datadog/utils'
7+
import { datadogErrorMessage, resolveDatadogSite } from '@/tools/datadog/utils'
88
import type { ToolConfig } from '@/tools/types'
99

1010
export const createEventTool: ToolConfig<CreateEventParams, CreateEventResponse> = {
@@ -88,7 +88,7 @@ export const createEventTool: ToolConfig<CreateEventParams, CreateEventResponse>
8888

8989
request: {
9090
url: (params) => {
91-
const site = params.site || 'datadoghq.com'
91+
const site = resolveDatadogSite(params.site)
9292
return `https://api.${site}/api/v1/events`
9393
},
9494
method: 'POST',

‎apps/sim/tools/datadog/create_monitor.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import type { CreateMonitorParams, CreateMonitorResponse, MonitorType } from '@/tools/datadog/types'
2-
import { datadogErrorMessage, parseJsonParam } from '@/tools/datadog/utils'
2+
import { datadogErrorMessage, parseJsonParam, resolveDatadogSite } from '@/tools/datadog/utils'
33
import type { ToolConfig } from '@/tools/types'
44

55
export const createMonitorTool: ToolConfig<CreateMonitorParams, CreateMonitorResponse> = {
@@ -77,7 +77,7 @@ export const createMonitorTool: ToolConfig<CreateMonitorParams, CreateMonitorRes
7777

7878
request: {
7979
url: (params) => {
80-
const site = params.site || 'datadoghq.com'
80+
const site = resolveDatadogSite(params.site)
8181
return `https://api.${site}/api/v1/monitor`
8282
},
8383
method: 'POST',

‎apps/sim/tools/datadog/datadog.test.ts‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
*/
44
import { beforeEach, describe, expect, it, vi } from 'vitest'
55
import { executeUpdateSloOperation } from '@/lib/internal/datadog/operations/update-slo'
6+
import * as datadogTools from '@/tools/datadog'
67
import { cancelDowntimeTool } from '@/tools/datadog/cancel_downtime'
78
import { createDowntimeTool } from '@/tools/datadog/create_downtime'
89
import { createEventTool } from '@/tools/datadog/create_event'
@@ -652,3 +653,60 @@ describe('datadog site is validated before it reaches the request host', () => {
652653
).toThrow(/Datadog "site" must be one of/)
653654
})
654655
})
656+
657+
describe('every Datadog tool routes its host through the allowlist', () => {
658+
/*
659+
* The first pass guarded only the shared `datadogApiUrl`; ten tools built the
660+
* host inline from `params.site` and bypassed it entirely. Sweeping the registry
661+
* rather than listing tools means a new tool that reintroduces an inline builder
662+
* fails here instead of shipping an unguarded credentialed request.
663+
*/
664+
const REQUIRED = {
665+
monitorId: '1',
666+
downtimeId: '1',
667+
dashboardId: 'abc-def-ghi',
668+
incidentId: '1',
669+
sloId: 'abc',
670+
signalId: 'abc',
671+
testId: 'abc',
672+
publicId: 'abc',
673+
resultId: 'abc',
674+
query: 'x',
675+
from: '1',
676+
to: '2',
677+
logs: '[]',
678+
metrics: '[]',
679+
series: '[]',
680+
title: 't',
681+
text: 't',
682+
name: 'n',
683+
type: 'metric alert',
684+
scope: '*',
685+
start: '1',
686+
end: '2',
687+
testIds: 'a',
688+
}
689+
690+
it('rejects an attacker-chosen site in every tool that builds a URL', () => {
691+
const builders = Object.values(datadogTools).filter(
692+
(tool) => typeof tool?.request?.url === 'function'
693+
)
694+
expect(builders.length).toBeGreaterThan(20)
695+
696+
const unguarded: string[] = []
697+
for (const tool of builders) {
698+
const params = { ...REQUIRED, apiKey: 'k', applicationKey: 'a', site: 'evil.com' }
699+
try {
700+
const url = String((tool.request as { url: (p: unknown) => string }).url(params))
701+
if (!/^https:\/\/(api|http-intake\.logs)\.(datadoghq\.com|datadoghq\.eu)/.test(url)) {
702+
unguarded.push(`${tool.id} -> ${url}`)
703+
}
704+
} catch (error) {
705+
if (!/Datadog "site" must be one of/.test(String(error))) {
706+
unguarded.push(`${tool.id} -> ${String(error)}`)
707+
}
708+
}
709+
}
710+
expect(unguarded).toEqual([])
711+
})
712+
})

‎apps/sim/tools/datadog/get_monitor.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import type { GetMonitorParams, GetMonitorResponse } from '@/tools/datadog/types'
2-
import { datadogErrorMessage, datadogPathSegment } from '@/tools/datadog/utils'
2+
import { datadogErrorMessage, datadogPathSegment, resolveDatadogSite } from '@/tools/datadog/utils'
33
import type { ToolConfig } from '@/tools/types'
44

55
export const getMonitorTool: ToolConfig<GetMonitorParams, GetMonitorResponse> = {
@@ -50,7 +50,7 @@ export const getMonitorTool: ToolConfig<GetMonitorParams, GetMonitorResponse> =
5050

5151
request: {
5252
url: (params) => {
53-
const site = params.site || 'datadoghq.com'
53+
const site = resolveDatadogSite(params.site)
5454
const queryParams = new URLSearchParams()
5555

5656
if (params.groupStates) queryParams.set('group_states', params.groupStates)

‎apps/sim/tools/datadog/list_downtimes.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import type {
44
ListDowntimesParams,
55
ListDowntimesResponse,
66
} from '@/tools/datadog/types'
7-
import { datadogErrorMessage } from '@/tools/datadog/utils'
7+
import { datadogErrorMessage, resolveDatadogSite } from '@/tools/datadog/utils'
88
import type { ToolConfig } from '@/tools/types'
99

1010
export const listDowntimesTool: ToolConfig<ListDowntimesParams, ListDowntimesResponse> = {
@@ -55,7 +55,7 @@ export const listDowntimesTool: ToolConfig<ListDowntimesParams, ListDowntimesRes
5555

5656
request: {
5757
url: (params) => {
58-
const site = params.site || 'datadoghq.com'
58+
const site = resolveDatadogSite(params.site)
5959
const queryParams = new URLSearchParams()
6060

6161
if (params.currentOnly) queryParams.set('current_only', 'true')

‎apps/sim/tools/datadog/list_monitors.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import type { ListMonitorsParams, ListMonitorsResponse, MonitorData } from '@/tools/datadog/types'
2-
import { datadogErrorMessage } from '@/tools/datadog/utils'
2+
import { datadogErrorMessage, resolveDatadogSite } from '@/tools/datadog/utils'
33
import type { ToolConfig } from '@/tools/types'
44

55
export const listMonitorsTool: ToolConfig<ListMonitorsParams, ListMonitorsResponse> = {
@@ -77,7 +77,7 @@ export const listMonitorsTool: ToolConfig<ListMonitorsParams, ListMonitorsRespon
7777

7878
request: {
7979
url: (params) => {
80-
const site = params.site || 'datadoghq.com'
80+
const site = resolveDatadogSite(params.site)
8181
const queryParams = new URLSearchParams()
8282

8383
if (params.groupStates) queryParams.set('group_states', params.groupStates)

‎apps/sim/tools/datadog/query_logs.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import type {
44
QueryLogsParams,
55
QueryLogsResponse,
66
} from '@/tools/datadog/types'
7-
import { datadogErrorMessage } from '@/tools/datadog/utils'
7+
import { datadogErrorMessage, resolveDatadogSite } from '@/tools/datadog/utils'
88
import type { ToolConfig } from '@/tools/types'
99

1010
export const queryLogsTool: ToolConfig<QueryLogsParams, QueryLogsResponse> = {
@@ -83,7 +83,7 @@ export const queryLogsTool: ToolConfig<QueryLogsParams, QueryLogsResponse> = {
8383

8484
request: {
8585
url: (params) => {
86-
const site = params.site || 'datadoghq.com'
86+
const site = resolveDatadogSite(params.site)
8787
return `https://api.${site}/api/v2/logs/events/search`
8888
},
8989
method: 'POST',

0 commit comments

Comments
 (0)