Skip to content

Commit 906b23f

Browse files
committed
fix(integrations): surface corrupt Splunk bodies and partial CrowdStrike deletes
Narrows readSplunkJson's non-JSON tolerance to XML. The dispatching and job-control endpoints answer in XML, but a body that is neither empty nor XML was meant to be JSON, so swallowing its parse failure handed get_search_results an empty envelope and reported a lost result set as a search with zero events. Annotates a batched CrowdStrike delete that fails partway with the IDs its earlier batches already removed. Falcon cannot roll those back, so a bare failure left the caller unable to tell what was gone and a blind retry re-targeted IDs that no longer existed. Registers the Cloudflare subblock-ID migration the registry-stability check requires. The suffixed read-filter IDs never shipped in a release and every block already materializes the restored IDs, so they are dropped rather than renamed onto values the collision guard would discard anyway.
1 parent a7eb8c5 commit 906b23f

6 files changed

Lines changed: 218 additions & 10 deletions

File tree

apps/sim/app/api/tools/crowdstrike/query/operations.test.ts

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1036,6 +1036,58 @@ describe('CrowdStrike extended operations', () => {
10361036
expect(data.error).toBe('Rate limit exceeded')
10371037
})
10381038

1039+
/**
1040+
* A batched delete has no rollback: every batch that answered `ok` really removed
1041+
* its indicators. Reporting only the failing batch's message would leave the
1042+
* caller unable to tell what is already gone, and a blind retry would re-target
1043+
* IDs that no longer exist.
1044+
*/
1045+
it('names the indicators earlier batches already deleted when a later batch fails', async () => {
1046+
const indicatorIds = longIds(300, 'ioc')
1047+
let deleteCall = 0
1048+
const deletedByFirstBatch: string[] = []
1049+
1050+
fetchMock.mockImplementation((rawUrl: string) => {
1051+
deleteCall += 1
1052+
if (deleteCall === 2) {
1053+
return Promise.resolve(
1054+
jsonResponse({ errors: [{ code: 429, message: 'Rate limit exceeded' }] }, 429)
1055+
)
1056+
}
1057+
const ids = idsFromUrl(rawUrl)
1058+
deletedByFirstBatch.push(...ids)
1059+
return Promise.resolve(jsonResponse({ resources: ids }))
1060+
})
1061+
1062+
const response = await POST(
1063+
requestFor({ operation: 'crowdstrike_delete_indicators', indicatorIds })
1064+
)
1065+
const data = await response.json()
1066+
1067+
expect(response.status).toBe(429)
1068+
expect(data.success).toBe(false)
1069+
expect(deletedByFirstBatch.length).toBeGreaterThan(0)
1070+
expect(data.error).toContain('Rate limit exceeded')
1071+
expect(data.error).toContain(`${deletedByFirstBatch.length} ID(s) were already deleted`)
1072+
expect(data.error).toContain(deletedByFirstBatch[0])
1073+
})
1074+
1075+
it('leaves a first-batch delete failure unannotated — nothing was committed', async () => {
1076+
const indicatorIds = longIds(300, 'ioc')
1077+
1078+
fetchMock.mockImplementation(() =>
1079+
Promise.resolve(jsonResponse({ errors: [{ code: 403, message: 'Access denied' }] }, 403))
1080+
)
1081+
1082+
const response = await POST(
1083+
requestFor({ operation: 'crowdstrike_delete_indicators', indicatorIds })
1084+
)
1085+
const data = await response.json()
1086+
1087+
expect(response.status).toBe(403)
1088+
expect(data.error).toBe('Access denied')
1089+
})
1090+
10391091
it('splits an oversized vulnerability lookup at the Spotlight cap', async () => {
10401092
const vulnerabilityIds = longIds(400, 'vuln')
10411093

apps/sim/app/api/tools/crowdstrike/query/operations.ts

Lines changed: 47 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { isRecordLike } from '@sim/utils/object'
2+
import { truncate } from '@sim/utils/string'
23
import type { CrowdstrikeQueryBody } from '@/lib/api/contracts/tools/crowdstrike'
34
import {
45
buildUrl,
@@ -125,6 +126,45 @@ export function chunkIdsByUrlBudget(ids: string[], budget: number): string[][] {
125126
return chunks
126127
}
127128

129+
/**
130+
* Caps how much of the already-committed ID list is spelled out in a partial-failure
131+
* message. A by-ids delete can carry 1000 IDs at ~68 bytes each, so the full list
132+
* would bury the actual failure under ~68 KB of text.
133+
*/
134+
const MAX_COMMITTED_IDS_IN_MESSAGE = 400
135+
136+
/**
137+
* Rewrites a failed batch's envelope so the reported error names the deletions the
138+
* earlier batches already committed.
139+
*
140+
* Batches run sequentially and each one that answered `ok` really did delete its
141+
* indicators — Falcon has no way to roll them back. Short-circuiting on a later
142+
* batch would therefore report a bare failure over work that already happened, and
143+
* a blind retry would target IDs that no longer exist. Only the message survives to
144+
* the caller ({@link fail} keeps `status` and the message, not `data`), so the
145+
* committed prefix is written onto `errors[0].message`, which is the first thing
146+
* {@link getFalconErrorMessage} reads.
147+
*/
148+
function withCommittedIds(
149+
result: CrowdStrikeCallResult,
150+
committed: string[]
151+
): CrowdStrikeCallResult {
152+
if (committed.length === 0) return result
153+
154+
const envelope = isRecordLike(result.data) ? result.data : {}
155+
const existing = getRecordArray(envelope.errors)
156+
const reason = getFalconErrorMessage(result.data, 'CrowdStrike rejected a later batch.')
157+
const message =
158+
`${reason} This request was split into batches and ${committed.length} ID(s) were already deleted ` +
159+
`before the failing batch; they were not rolled back, so retry only the remainder. ` +
160+
`Deleted: ${truncate(committed.join(', '), MAX_COMMITTED_IDS_IN_MESSAGE)}`
161+
162+
return {
163+
...result,
164+
data: { ...envelope, errors: [{ ...(existing[0] ?? {}), message }, ...existing.slice(1)] },
165+
}
166+
}
167+
128168
interface ByIdsRequestOptions {
129169
method: 'GET' | 'DELETE'
130170
path: string
@@ -139,7 +179,9 @@ interface ByIdsRequestOptions {
139179
*
140180
* Batches run sequentially: resource order matches the caller's ID order, the
141181
* endpoint's rate limit only ever sees one request at a time, and a failing batch
142-
* short-circuits with its own status instead of being merged away. `meta` comes
182+
* short-circuits with its own status instead of being merged away. A `DELETE` that
183+
* fails partway also carries the IDs its earlier batches already removed — see
184+
* {@link withCommittedIds}. `meta` comes
143185
* from the first batch — pagination is meaningless for a lookup that names every
144186
* ID it wants, and no by-ids operation here reads it.
145187
*/
@@ -169,6 +211,7 @@ async function callCrowdStrikeByIds(
169211

170212
const resources: unknown[] = []
171213
const errors: unknown[] = []
214+
const committed: string[] = []
172215
let meta: unknown
173216
let status = 200
174217

@@ -181,9 +224,11 @@ async function callCrowdStrikeByIds(
181224
})
182225

183226
if (!result.ok) {
184-
return result
227+
return options.method === 'DELETE' ? withCommittedIds(result, committed) : result
185228
}
186229

230+
committed.push(...chunk)
231+
187232
if (index === 0) {
188233
status = result.status
189234
meta = isRecordLike(result.data) ? result.data.meta : undefined

apps/sim/lib/workflows/migrations/subblock-migrations.test.ts

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -423,6 +423,71 @@ describe('migrateSubblockIds', () => {
423423
expect(blocks.b3.subBlocks.code).toBeDefined()
424424
})
425425

426+
/**
427+
* The suffixed Cloudflare read-filter ids existed only between #6740 and the
428+
* restore, and never shipped in a release. They are dropped rather than renamed
429+
* onto `name`/`type`/`content`/`proxied`/`tags`, which every Cloudflare block
430+
* already materializes — a rename would hit the collision guard and discard the
431+
* value regardless, while leaving the stale key parked in state.
432+
*/
433+
describe('cloudflare block', () => {
434+
it('drops the staging-only read-filter ids without disturbing the restored ids', () => {
435+
const input: Record<string, BlockState> = {
436+
b1: makeBlock({
437+
type: 'cloudflare',
438+
subBlocks: {
439+
operation: { id: 'operation', type: 'dropdown', value: 'list_dns_records' },
440+
zoneNameFilter: { id: 'zoneNameFilter', type: 'short-input', value: 'example.com' },
441+
dnsNameFilter: { id: 'dnsNameFilter', type: 'short-input', value: 'www' },
442+
dnsTypeFilter: { id: 'dnsTypeFilter', type: 'dropdown', value: 'A' },
443+
dnsContentFilter: { id: 'dnsContentFilter', type: 'short-input', value: '1.2.3.4' },
444+
dnsProxiedFilter: { id: 'dnsProxiedFilter', type: 'dropdown', value: 'true' },
445+
purgeTags: { id: 'purgeTags', type: 'short-input', value: 'tag-a' },
446+
cursor: { id: 'cursor', type: 'short-input', value: 'abc' },
447+
name: { id: 'name', type: 'short-input', value: '' },
448+
},
449+
}),
450+
}
451+
452+
const { blocks, migrated } = migrateSubblockIds(input)
453+
454+
expect(migrated).toBe(true)
455+
for (const legacyId of [
456+
'zoneNameFilter',
457+
'dnsNameFilter',
458+
'dnsTypeFilter',
459+
'dnsContentFilter',
460+
'dnsProxiedFilter',
461+
'purgeTags',
462+
'cursor',
463+
]) {
464+
expect(blocks.b1.subBlocks[legacyId]).toBeUndefined()
465+
expect(blocks.b1.subBlocks[`_removed_${legacyId}`]).toBeUndefined()
466+
}
467+
expect(blocks.b1.subBlocks.operation.value).toBe('list_dns_records')
468+
expect(blocks.b1.subBlocks.name.value).toBe('')
469+
})
470+
471+
it('leaves a workflow saved on the restored ids untouched', () => {
472+
const input: Record<string, BlockState> = {
473+
b1: makeBlock({
474+
type: 'cloudflare',
475+
subBlocks: {
476+
operation: { id: 'operation', type: 'dropdown', value: 'list_dns_records' },
477+
name: { id: 'name', type: 'short-input', value: 'www' },
478+
type: { id: 'type', type: 'dropdown', value: 'A' },
479+
},
480+
}),
481+
}
482+
483+
const { blocks, migrated } = migrateSubblockIds(input)
484+
485+
expect(migrated).toBe(false)
486+
expect(blocks.b1.subBlocks.name.value).toBe('www')
487+
expect(blocks.b1.subBlocks.type.value).toBe('A')
488+
})
489+
})
490+
426491
it('should handle blocks with empty subBlocks', () => {
427492
const input: Record<string, BlockState> = {
428493
b1: makeBlock({ type: 'knowledge', subBlocks: {} }),

apps/sim/lib/workflows/migrations/subblock-migrations.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,35 @@ export const SUBBLOCK_ID_MIGRATIONS: Record<string, Record<string, string>> = {
114114
host: '_removed_host',
115115
apiKey: '_removed_apiKey',
116116
},
117+
/**
118+
* The Cloudflare block briefly gave its DNS/zone read filters and its cache-purge
119+
* tag list operation-suffixed IDs, and added a single shared `cursor`. This PR
120+
* restores the shipped IDs (`name`, `type`, `content`, `proxied`, `tags`) so saved
121+
* workflows keep filtering, and splits the cursor per endpoint.
122+
*
123+
* The suffixed IDs are dropped rather than renamed onto their shipped
124+
* counterparts. They existed only between #6740 and this change and never
125+
* appeared in a release, so no deployed workflow carries them — and a rename
126+
* could not restore a value even for a workflow edited in that window. Block
127+
* state materializes an entry for every subblock the config declares, not just
128+
* the active operation's, so `name`/`type`/`content`/`proxied`/`tags` are always
129+
* already present; {@link migrateBlockSubblockIds} would hit its collision guard
130+
* and discard the source value anyway. Mapping them as renames would therefore
131+
* claim a recovery that never happens, while leaving the stale value parked in
132+
* state and riding along in exports.
133+
*
134+
* `cursor` split into `r2Cursor` and `rulesetCursor`, so there is no single
135+
* replacement to name.
136+
*/
137+
cloudflare: {
138+
zoneNameFilter: '_removed_zoneNameFilter',
139+
dnsNameFilter: '_removed_dnsNameFilter',
140+
dnsTypeFilter: '_removed_dnsTypeFilter',
141+
dnsContentFilter: '_removed_dnsContentFilter',
142+
dnsProxiedFilter: '_removed_dnsProxiedFilter',
143+
purgeTags: '_removed_purgeTags',
144+
cursor: '_removed_cursor',
145+
},
117146
rippling: {
118147
action: '_removed_action',
119148
candidateDepartment: '_removed_candidateDepartment',

apps/sim/tools/splunk/utils.test.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,20 @@ describe('readSplunkJson', () => {
9696
readSplunkJson(new Response('<response><sid>1457683115.100</sid></response>'))
9797
).resolves.toEqual({})
9898
})
99+
100+
/**
101+
* The XML tolerance must not extend to a body that was meant to be JSON.
102+
* Swallowing a truncated result payload would hand `get_search_results` an empty
103+
* envelope, reporting a lost result set as a search that legitimately matched
104+
* nothing.
105+
*/
106+
it('throws on a truncated JSON body rather than reporting an empty result set', async () => {
107+
await expect(readSplunkJson(new Response('{"results":[{"_raw":"partial'))).rejects.toThrow()
108+
})
109+
110+
it('throws on a non-JSON, non-XML body', async () => {
111+
await expect(readSplunkJson(new Response('upstream connect error'))).rejects.toThrow()
112+
})
99113
})
100114

101115
describe('getSplunkPaging', () => {

apps/sim/tools/splunk/utils.ts

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -160,25 +160,28 @@ export function buildSplunkFormBody(
160160
* still-running job would surface `Unexpected end of JSON input` instead of an
161161
* empty result set.
162162
*
163-
* A non-JSON body is tolerated the same way because `output_mode` does not appear
164-
* in the documented parameter table for the dispatching and job-control endpoints,
163+
* An XML body is tolerated the same way because `output_mode` does not appear in
164+
* the documented parameter table for the dispatching and job-control endpoints,
165165
* and the only response the reference documents for them is the XML
166166
* `<response><sid>...</sid></response>`. Throwing on that XML would report a
167167
* request that succeeded server-side as a failure — cancelling a job really does
168168
* cancel it — and would pre-empt the specific error the caller raises when the
169169
* envelope carries no usable value. Returning `{}` lets that error surface
170170
* instead, and a JSON body (which these endpoints do return in practice when
171171
* `output_mode=json` is honored) still parses normally.
172+
*
173+
* The tolerance stops at XML. A body that is neither empty nor XML is meant to be
174+
* JSON, so a parse failure there means the payload is corrupt or truncated —
175+
* swallowing it would hand `get_search_results` an empty envelope and report a
176+
* lost result set as a successful search with zero events.
172177
*/
173178
export async function readSplunkJson(response: Response): Promise<unknown> {
174179
if (response.status === 204) return {}
175180
const text = await response.text()
176-
if (!text.trim()) return {}
177-
try {
178-
return JSON.parse(text)
179-
} catch {
180-
return {}
181-
}
181+
const body = text.trim()
182+
if (!body) return {}
183+
if (body.startsWith('<')) return {}
184+
return JSON.parse(body)
182185
}
183186

184187
/**

0 commit comments

Comments
 (0)