Skip to content

Commit f14cd7e

Browse files
committed
fix(algolia): stop rejecting slash-bearing objectIDs
A '/' is legal in an Algolia objectID -- URL-keyed ids are a common site-search pattern -- and safeUrlPathSegment rejected them, breaking working workflows. Algolia distinguishes the cases itself: a slashed objectID returns 404 'ObjectID does not exist' while a slashed indexName returns 400 'indexName is not valid'. Adds safeOpaqueUrlSegment: rejects an exact '.'/'..' but encodes '/' to %2F so the value collapses to one inert segment. indexName keeps the strict guard. Both path-safety suites applied one flat 'a separator is always hostile' list to every param, which is why this shipped green twice. The vectors are now per-param, and a test pins the carve-out to objectID alone.
1 parent fbf0e9f commit f14cd7e

9 files changed

Lines changed: 368 additions & 24 deletions

apps/sim/tools/algolia/add_record.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import type { AlgoliaAddRecordParams, AlgoliaAddRecordResponse } from '@/tools/algolia/types'
22
import type { ToolConfig } from '@/tools/types'
3-
import { safeUrlPathSegment } from '@/tools/url-path'
3+
import { safeOpaqueUrlSegment, safeUrlPathSegment } from '@/tools/url-path'
44

55
export const addRecordTool: ToolConfig<AlgoliaAddRecordParams, AlgoliaAddRecordResponse> = {
66
id: 'algolia_add_record',
@@ -45,7 +45,7 @@ export const addRecordTool: ToolConfig<AlgoliaAddRecordParams, AlgoliaAddRecordR
4545
url: (params) => {
4646
const base = `https://${params.applicationId}.algolia.net/1/indexes/${safeUrlPathSegment(params.indexName, 'indexName')}`
4747
if (params.objectID) {
48-
return `${base}/${safeUrlPathSegment(params.objectID, 'objectID')}`
48+
return `${base}/${safeOpaqueUrlSegment(params.objectID, 'objectID')}`
4949
}
5050
return base
5151
},

apps/sim/tools/algolia/delete_record.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import type { AlgoliaDeleteRecordParams, AlgoliaDeleteRecordResponse } from '@/tools/algolia/types'
22
import type { ToolConfig } from '@/tools/types'
3-
import { safeUrlPathSegment } from '@/tools/url-path'
3+
import { safeOpaqueUrlSegment, safeUrlPathSegment } from '@/tools/url-path'
44

55
export const deleteRecordTool: ToolConfig<AlgoliaDeleteRecordParams, AlgoliaDeleteRecordResponse> =
66
{
@@ -39,7 +39,7 @@ export const deleteRecordTool: ToolConfig<AlgoliaDeleteRecordParams, AlgoliaDele
3939
request: {
4040
method: 'DELETE',
4141
url: (params) =>
42-
`https://${params.applicationId}.algolia.net/1/indexes/${safeUrlPathSegment(params.indexName, 'indexName')}/${safeUrlPathSegment(params.objectID, 'objectID')}`,
42+
`https://${params.applicationId}.algolia.net/1/indexes/${safeUrlPathSegment(params.indexName, 'indexName')}/${safeOpaqueUrlSegment(params.objectID, 'objectID')}`,
4343
headers: (params) => ({
4444
'x-algolia-application-id': params.applicationId,
4545
'x-algolia-api-key': params.apiKey,

apps/sim/tools/algolia/get_record.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import type { AlgoliaGetRecordParams, AlgoliaGetRecordResponse } from '@/tools/algolia/types'
22
import type { ToolConfig } from '@/tools/types'
3-
import { safeUrlPathSegment } from '@/tools/url-path'
3+
import { safeOpaqueUrlSegment, safeUrlPathSegment } from '@/tools/url-path'
44

55
export const getRecordTool: ToolConfig<AlgoliaGetRecordParams, AlgoliaGetRecordResponse> = {
66
id: 'algolia_get_record',
@@ -44,7 +44,7 @@ export const getRecordTool: ToolConfig<AlgoliaGetRecordParams, AlgoliaGetRecordR
4444
request: {
4545
method: 'GET',
4646
url: (params) => {
47-
const base = `https://${params.applicationId}-dsn.algolia.net/1/indexes/${safeUrlPathSegment(params.indexName, 'indexName')}/${safeUrlPathSegment(params.objectID, 'objectID')}`
47+
const base = `https://${params.applicationId}-dsn.algolia.net/1/indexes/${safeUrlPathSegment(params.indexName, 'indexName')}/${safeOpaqueUrlSegment(params.objectID, 'objectID')}`
4848
if (params.attributesToRetrieve) {
4949
return `${base}?attributesToRetrieve=${encodeURIComponent(params.attributesToRetrieve)}`
5050
}

apps/sim/tools/algolia/get_task_status.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ export const getTaskStatusTool: ToolConfig<
4444
request: {
4545
method: 'GET',
4646
url: (params) =>
47-
`https://${params.applicationId}-dsn.algolia.net/1/indexes/${safeUrlPathSegment(params.indexName, 'indexName')}/task/${safeUrlPathSegment(String(params.taskID), 'taskID')}`,
47+
`https://${params.applicationId}-dsn.algolia.net/1/indexes/${safeUrlPathSegment(params.indexName, 'indexName')}/task/${safeUrlPathSegment(params.taskID, 'taskID')}`,
4848
headers: (params) => ({
4949
'x-algolia-application-id': params.applicationId,
5050
'x-algolia-api-key': params.apiKey,

apps/sim/tools/algolia/partial_update_record.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import type {
33
AlgoliaPartialUpdateRecordResponse,
44
} from '@/tools/algolia/types'
55
import type { ToolConfig } from '@/tools/types'
6-
import { safeUrlPathSegment } from '@/tools/url-path'
6+
import { safeOpaqueUrlSegment, safeUrlPathSegment } from '@/tools/url-path'
77

88
export const partialUpdateRecordTool: ToolConfig<
99
AlgoliaPartialUpdateRecordParams,
@@ -56,7 +56,7 @@ export const partialUpdateRecordTool: ToolConfig<
5656

5757
request: {
5858
url: (params) => {
59-
const base = `https://${params.applicationId}.algolia.net/1/indexes/${safeUrlPathSegment(params.indexName, 'indexName')}/${safeUrlPathSegment(params.objectID, 'objectID')}/partial`
59+
const base = `https://${params.applicationId}.algolia.net/1/indexes/${safeUrlPathSegment(params.indexName, 'indexName')}/${safeOpaqueUrlSegment(params.objectID, 'objectID')}/partial`
6060
if (params.createIfNotExists === false) {
6161
return `${base}?createIfNotExists=false`
6262
}

apps/sim/tools/algolia/path_safety.test.ts

Lines changed: 123 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,13 @@
55
* path against path traversal.
66
*
77
* The index name and object ID are `visibility: 'user-or-llm'`, so prompt
8-
* injection controls them. A value like `..` pops a path segment once `fetch`
9-
* normalizes the URL, re-aiming the request and the caller's admin API key at
10-
* a sibling endpoint — `DELETE /1/indexes/<index>/<objectID>` becomes
11-
* `DELETE /1/indexes/<index>`, deleting the whole index instead of one record.
8+
* injection controls them. A value of exactly `.` pops nothing but the record:
9+
* `DELETE /1/indexes/myindex/.` normalizes to `DELETE /1/indexes/myindex`,
10+
* deleting the whole index instead of one record. A value of exactly `..` pops
11+
* one segment further — `/1/indexes/myindex/..` becomes `/1/indexes/` — re-aiming
12+
* the request and the caller's admin API key at the list-indices route. Both were
13+
* verified through `new URL(...)`; neither is the "delete the index" escalation
14+
* the other one is, so both are rejected.
1215
*
1316
* `applicationId` is deliberately NOT asserted on here: it is interpolated
1417
* into the HOST (`https://<appId>-dsn.algolia.net`), not the path, and the
@@ -21,15 +24,40 @@
2124
* resolves the built URL through `new URL(...)` — the same normalization
2225
* `fetch` performs — and compares *segment shape* rather than template text,
2326
* because `pathname.startsWith(prefix)` stays green after a segment is popped.
27+
*
28+
* The vector lists are **per parameter**, because the two path parameters do
29+
* not share a charset. Algolia validates `indexName` (`/1/indexes/instant%2Fsearch/settings`
30+
* → `400 indexName is not valid`) but not `objectID`: `foo%2Fbar` and a
31+
* URL-keyed id both return `404 ObjectID does not exist`, i.e. well-formed but
32+
* absent, and Algolia's own conformance suite exercises `Batman and Robin`.
33+
* A single shared list that contained no `/`-bearing legitimate value is what
34+
* let a guard rejecting `/` in `objectID` ship green.
2435
*/
2536
import { describe, expect, it } from 'vitest'
2637
import * as toolModule from '@/tools/algolia/index'
2738
import type { ToolConfig } from '@/tools/types'
2839

2940
type AnyTool = ToolConfig<any, any>
3041

31-
/** Vectors the guard must reject outright; no encoding neutralizes them. */
32-
const REJECTED = ['..', '.', ' .. ', 'a/../../b', '\\..\\..'] as const
42+
/**
43+
* Parameters whose value is an opaque record id rather than a named resource.
44+
* Algolia treats `objectID` as an arbitrary string — a `/` inside it is a
45+
* legal, common (URL-keyed) id, not a separator — so it is guarded by
46+
* collapsing the whole value into one percent-encoded segment instead of
47+
* rejecting separators.
48+
*/
49+
const OPAQUE_ID_PARAMS = new Set(['objectID'])
50+
51+
/** No encoding scheme neutralizes these; every guard must reject them. */
52+
const DOT_SEGMENT_VECTORS = ['..', '.', ' .. '] as const
53+
54+
/**
55+
* Separator-bearing vectors. These are rejected only by parameters that
56+
* address a *named* resource, where a separator means the caller passed
57+
* something other than what the parameter addresses. For an opaque id they are
58+
* legitimate values and appear in {@link OPAQUE_LEGITIMATE} instead.
59+
*/
60+
const SEPARATOR_VECTORS = ['a/../../b', '\\..\\..'] as const
3361

3462
/**
3563
* Vectors `encodeURIComponent` genuinely does neutralize — `%` and `?` are
@@ -49,6 +77,32 @@ const LEGITIMATE = [
4977
'foo..',
5078
] as const
5179

80+
/**
81+
* Legitimate ids that only an opaque-id parameter accepts. The URL-keyed form
82+
* is the common site-search pattern; `Batman and Robin` is lifted from
83+
* Algolia's own client conformance suite (`/1/indexes/cts_e2e_browse/Batman%20and%20Robin`
84+
* → 200); `a/../../b` and `docs/` are legal ids whose dot and trailing
85+
* separators must survive as inert `%2F`-joined text rather than be rejected
86+
* or emitted as real separators.
87+
*/
88+
const OPAQUE_LEGITIMATE = [
89+
'https://example.com/docs/getting-started',
90+
'Batman and Robin',
91+
'foo/bar',
92+
'a/../../b',
93+
'docs/',
94+
] as const
95+
96+
function rejectedFor(param: string): readonly string[] {
97+
return OPAQUE_ID_PARAMS.has(param)
98+
? DOT_SEGMENT_VECTORS
99+
: [...DOT_SEGMENT_VECTORS, ...SEPARATOR_VECTORS]
100+
}
101+
102+
function legitimateFor(param: string): readonly string[] {
103+
return OPAQUE_ID_PARAMS.has(param) ? [...LEGITIMATE, ...OPAQUE_LEGITIMATE] : LEGITIMATE
104+
}
105+
52106
const ID_PREFIX = 'SAFE'
53107
const TOOL_ID_PREFIX = 'algolia_'
54108

@@ -66,9 +120,9 @@ function isTool(value: unknown): value is AnyTool {
66120
}
67121

68122
/**
69-
* Number-typed parameters are stringified into the path by the tool
70-
* (`algolia_get_task_status` does `String(params.taskID)`), so they need a
71-
* numeric marker to be discoverable at all. Skipping them would silently drop
123+
* Number-typed parameters (`algolia_get_task_status`'s `taskID`) reach the
124+
* guard as a JSON number and are stringified by `toGuardedString`, so they need
125+
* a numeric marker to be discoverable at all. Skipping them would silently drop
72126
* a real guard site from coverage.
73127
*/
74128
const NUMBER_MARKERS = new Map<string, string>()
@@ -153,7 +207,7 @@ describe('Algolia path-parameter traversal safety', () => {
153207
expect(slot).toBeGreaterThan(0)
154208
})
155209

156-
it.each(REJECTED)('rejects %j instead of reshaping the path', (value) => {
210+
it.each(rejectedFor(param))('rejects %j instead of reshaping the path', (value) => {
157211
expect(() => buildUrl(tool, param, value)).toThrow(new RegExp(param))
158212
})
159213

@@ -170,7 +224,7 @@ describe('Algolia path-parameter traversal safety', () => {
170224
expect(url.searchParams.get('foo')).toBeNull()
171225
})
172226

173-
it.each(LEGITIMATE)('passes %j through byte-identical', (value) => {
227+
it.each(legitimateFor(param))('passes %j through byte-identical', (value) => {
174228
const url = buildUrl(tool, param, value)
175229
const segments = url.pathname.split('/')
176230

@@ -181,6 +235,12 @@ describe('Algolia path-parameter traversal safety', () => {
181235
index === slot ? value : segment
182236
)
183237
})
238+
239+
expect(segments[slot]).toBe(encodeURIComponent(value))
240+
241+
if (encodeURIComponent(value) === value) {
242+
expect(segments[slot]).toBe(value)
243+
}
184244
})
185245
})
186246
})
@@ -211,3 +271,55 @@ describe('Algolia guards every path param independently', () => {
211271
})
212272
})
213273
})
274+
275+
/**
276+
* Pins the two dot-segment normalizations the guards exist to prevent, through
277+
* the same `new URL(...)` the `fetch` implementation applies, and shows that the
278+
* relaxation for opaque ids did not re-open either one.
279+
*
280+
* The escalation is asymmetric, which the previous docstring here had backwards:
281+
* `.` pops only the record segment and leaves the *index* addressed — that is
282+
* the "delete the whole index" vector on `DELETE /1/indexes/<index>/<objectID>`.
283+
* `..` pops one further and lands on `/1/indexes/`, re-aiming the admin key at
284+
* the list/create-index route. Both are rejected; neither is merely encoded.
285+
*/
286+
describe('Algolia dot-segment normalization', () => {
287+
const RECORD_ROUTE = 'https://app.algolia.net/1/indexes/myindex/'
288+
289+
it('confirms "." collapses the record route onto the index itself', () => {
290+
expect(new URL(`${RECORD_ROUTE}.`).pathname).toBe('/1/indexes/myindex/')
291+
})
292+
293+
it('confirms ".." pops the index segment too', () => {
294+
expect(new URL(`${RECORD_ROUTE}..`).pathname).toBe('/1/indexes/')
295+
expect(encodeURIComponent('..')).toBe('..')
296+
})
297+
298+
it('still rejects an exact dot segment in an opaque objectID', () => {
299+
for (const { tool, pathParams } of PATH_TOOLS) {
300+
for (const param of pathParams.filter((name) => OPAQUE_ID_PARAMS.has(name))) {
301+
expect(() => buildUrl(tool, param, '.')).toThrow(new RegExp(param))
302+
expect(() => buildUrl(tool, param, '..')).toThrow(new RegExp(param))
303+
}
304+
}
305+
})
306+
307+
it('collapses a slash-bearing opaque objectID into one %2F-joined segment', () => {
308+
const opaque = PATH_TOOLS.flatMap(({ tool, pathParams }) =>
309+
pathParams.filter((name) => OPAQUE_ID_PARAMS.has(name)).map((name) => ({ tool, name }))
310+
)
311+
312+
expect(opaque.length).toBeGreaterThan(0)
313+
314+
for (const { tool, name } of opaque) {
315+
const baselineSegments = buildUrl(tool).pathname.split('/')
316+
const url = buildUrl(tool, name, 'a/../../b')
317+
const segments = url.pathname.split('/')
318+
const slot = baselineSegments.indexOf(markerFor(name, (tool.params as any)[name]?.type))
319+
320+
expect(segments).toHaveLength(baselineSegments.length)
321+
expect(segments[slot]).toBe('a%2F..%2F..%2Fb')
322+
expect(decodeURIComponent(segments[slot])).toBe('a/../../b')
323+
}
324+
})
325+
})

0 commit comments

Comments
 (0)