Skip to content

Commit 52cfcce

Browse files
committed
fix(elasticsearch): require a scheme on the host, restore numeric index coercion
buildBaseUrl accepted a scheme-less host and returned it relative, so the executor resolved it against Sim's own origin while buildAuthHeaders still attached the credential: 'es.internal' -> https://sim.ai/es.internal/... credential to Sim '//evil.com' -> https://evil.com/... credential to an attacker-chosen origin The protocol-relative form is the serious one -- it inherits Sim's scheme and exfiltrates the ES ApiKey to any host. The SSRF check passes in both cases because the resolved host is not the user's cluster. A scheme is now required, and the original string is returned rather than the parser's normalized href so hosts reach ES exactly as typed. No auto-prefixing: silently upgrading a plaintext cluster to https would turn a typo into a different host. safeIndexPathSegment, added for the date-math carve-out, had copied the 'typeof value === string ? trim : ""' form that toGuardedString had just been changed away from -- so a numeric index threw 'index is required' while a numeric documentId in the same URL worked. It now mirrors the shared helper, and a test pins the two producing identical output. The parseCloudId comment attributed its rules to the wrong clients. The first-colon split is Python and .NET; Beats uses last-colon. Per-service ports are Beats and .NET. Omitting :443 is .NET only. The forbidden-character set is Beats only, and only on recent branches. Rewritten to attribute each rule and to state where Sim deliberately diverges. The block required an index for bulk while the tool declares it optional -- and _bulk legitimately runs against /_bulk with per-action _index.
1 parent 94d432a commit 52cfcce

4 files changed

Lines changed: 330 additions & 50 deletions

File tree

apps/sim/blocks/blocks/elasticsearch.ts

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,28 @@ export const ElasticsearchBlock: BlockConfig<ElasticsearchResponse> = {
187187
title: 'Index Name',
188188
type: 'short-input',
189189
placeholder: 'my-index',
190-
required: true,
190+
/**
191+
* Required for every operation that addresses an index, but *not* for
192+
* `_bulk`. Bulk legitimately runs against `/_bulk` with a per-action
193+
* `_index` — the `operations` placeholder below shows exactly that — so
194+
* `bulk.ts` declares `index` optional and its URL builder branches on
195+
* the absence. The field still renders for bulk, where it supplies the
196+
* default target for actions that omit `_index`.
197+
*/
198+
required: {
199+
field: 'operation',
200+
value: [
201+
'elasticsearch_search',
202+
'elasticsearch_index_document',
203+
'elasticsearch_get_document',
204+
'elasticsearch_update_document',
205+
'elasticsearch_delete_document',
206+
'elasticsearch_count',
207+
'elasticsearch_create_index',
208+
'elasticsearch_delete_index',
209+
'elasticsearch_get_index',
210+
],
211+
},
191212
condition: {
192213
field: 'operation',
193214
value: [

apps/sim/tools/elasticsearch/block-params.test.ts

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,3 +102,47 @@ describe('elasticsearch block declares no unreachable aggregations', () => {
102102
expect(transform({ timeout: '45s' }).esTimeout).toBe('45s')
103103
})
104104
})
105+
106+
/**
107+
* `_bulk` legitimately runs against `/_bulk` with a per-action `_index`, which
108+
* the block's own `operations` placeholder demonstrates
109+
* (`{"index":{"_index":"my-index","_id":"1"}}`). `bulk.ts` therefore declares
110+
* `index` optional, and `bulk`'s URL builder branches on its absence — the
111+
* block marking it `required: true` blocked a call the tool supports.
112+
*/
113+
describe('elasticsearch block index requirement agrees with the tools', () => {
114+
const indexSubBlock = ElasticsearchBlock.subBlocks.find((b) => b.id === 'index')
115+
116+
it('does not require index for bulk', () => {
117+
expect(indexSubBlock?.required).not.toBe(true)
118+
119+
const required = indexSubBlock?.required as { field: string; value: string[] }
120+
121+
expect(required.field).toBe('operation')
122+
expect(required.value).not.toContain('elasticsearch_bulk')
123+
})
124+
125+
it('still renders the index field for bulk, where it sets the default target', () => {
126+
const condition = indexSubBlock?.condition as { field: string; value: string[] }
127+
128+
expect(condition.value).toContain('elasticsearch_bulk')
129+
})
130+
131+
it('keeps requiring index for every operation that addresses one', () => {
132+
const required = indexSubBlock?.required as { field: string; value: string[] }
133+
134+
for (const operation of [
135+
'elasticsearch_search',
136+
'elasticsearch_index_document',
137+
'elasticsearch_get_document',
138+
'elasticsearch_update_document',
139+
'elasticsearch_delete_document',
140+
'elasticsearch_count',
141+
'elasticsearch_create_index',
142+
'elasticsearch_delete_index',
143+
'elasticsearch_get_index',
144+
]) {
145+
expect(required.value).toContain(operation)
146+
}
147+
})
148+
})

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

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
parseCloudId,
1010
safeIndexPathSegment,
1111
} from '@/tools/elasticsearch/utils'
12+
import { safeUrlPathSegment } from '@/tools/url-path'
1213

1314
/**
1415
* Builds a Cloud ID from its decoded parts the way the Elastic Cloud console does.
@@ -456,3 +457,93 @@ describe('parseCloudId rejects URL-special characters in decoded components', ()
456457
expect(parseCloudId(id)).toBe(`https://${ES_UUID}.es-prod.eu-west-1.aws.found.io`)
457458
})
458459
})
460+
461+
/**
462+
* A self-hosted host without an explicit scheme stays *relative*, and the
463+
* executor resolves the tool URL with `new URL(endpointUrl, getBaseUrl())` —
464+
* Sim's own origin. `buildAuthHeaders` attaches the caller's Elasticsearch
465+
* credential regardless, so the credential is sent to Sim, and the SSRF check
466+
* passes because the resolved host is Sim itself.
467+
*/
468+
describe('buildBaseUrl requires an explicit http(s) scheme', () => {
469+
const SIM_ORIGIN = 'https://sim.ai'
470+
471+
function selfHosted(host: string) {
472+
return buildBaseUrl({ deploymentType: 'self_hosted', host, authMethod: 'api_key' })
473+
}
474+
475+
it('rejects a bare hostname that would otherwise resolve against Sim’s own origin', () => {
476+
expect(new URL('es.internal/products/_search', SIM_ORIGIN).origin).toBe(SIM_ORIGIN)
477+
478+
expect(() => selfHosted('es.internal')).toThrow(/must start with "http:\/\/" or "https:\/\//)
479+
})
480+
481+
it('rejects a scheme-less host:port, which parses as a bogus "localhost:" scheme', () => {
482+
expect(new URL('localhost:9200/products/_search', SIM_ORIGIN).protocol).toBe('localhost:')
483+
484+
expect(() => selfHosted('localhost:9200')).toThrow(/must start with "http:\/\/" or "https:\/\//)
485+
})
486+
487+
it('rejects a protocol-relative host, which silently inherits Sim’s scheme', () => {
488+
expect(new URL('//evil.example.com/x', SIM_ORIGIN).origin).toBe('https://evil.example.com')
489+
490+
expect(() => selfHosted('//evil.example.com')).toThrow(
491+
/must start with "http:\/\/" or "https:\/\//
492+
)
493+
})
494+
495+
it('rejects a non-http scheme', () => {
496+
expect(() => selfHosted('ftp://es.example.com')).toThrow(
497+
/must start with "http:\/\/" or "https:\/\//
498+
)
499+
expect(() => selfHosted('file:///etc/passwd')).toThrow(
500+
/must start with "http:\/\/" or "https:\/\//
501+
)
502+
})
503+
504+
it('rejects a scheme-ful host the URL parser cannot give an origin', () => {
505+
expect(() => selfHosted('https://')).toThrow(/is not a valid URL/)
506+
})
507+
508+
it('keeps every legitimate host shape working', () => {
509+
expect(selfHosted('https://localhost:9200')).toBe('https://localhost:9200')
510+
expect(selfHosted('http://localhost:9200')).toBe('http://localhost:9200')
511+
expect(selfHosted('https://es.example.com')).toBe('https://es.example.com')
512+
expect(selfHosted('https://es.example.com:9200///')).toBe('https://es.example.com:9200')
513+
expect(selfHosted(' https://es.example.com ')).toBe('https://es.example.com')
514+
expect(selfHosted('HTTPS://es.example.com')).toBe('HTTPS://es.example.com')
515+
expect(selfHosted('http://10.0.0.5:9200')).toBe('http://10.0.0.5:9200')
516+
expect(selfHosted('https://user:pw@es.example.com:9200')).toBe(
517+
'https://user:pw@es.example.com:9200'
518+
)
519+
})
520+
521+
it('produces an absolute URL that no longer resolves against Sim', () => {
522+
const built = new URL(`${selfHosted('https://localhost:9200')}/products/_search`, SIM_ORIGIN)
523+
524+
expect(built.origin).toBe('https://localhost:9200')
525+
})
526+
})
527+
528+
/**
529+
* `safeIndexPathSegment` is a local variant of the shared `safeUrlPathSegment`,
530+
* whose `toGuardedString` deliberately abandoned the
531+
* `typeof value === 'string' ? value.trim() : ''` form: a non-string became
532+
* `''` and was then reported as *"is required"*, a confusing error for a value
533+
* the caller did supply. The local variant only needed to drop the `/`
534+
* rejection, not the number coercion.
535+
*/
536+
describe('safeIndexPathSegment coerces a non-string index', () => {
537+
it('stringifies a numeric index emitted as a JSON number', () => {
538+
expect(safeIndexPathSegment(2024, 'index')).toBe('2024')
539+
})
540+
541+
it('matches the shared helper, which already accepts a numeric documentId', () => {
542+
expect(safeIndexPathSegment(2024, 'index')).toBe(safeUrlPathSegment(2024, 'documentId'))
543+
})
544+
545+
it('still reports null and undefined as missing rather than addressing "null"', () => {
546+
expect(() => safeIndexPathSegment(null as never, 'index')).toThrow(/index is required/)
547+
expect(() => safeIndexPathSegment(undefined as never, 'index')).toThrow(/index is required/)
548+
})
549+
})

0 commit comments

Comments
 (0)