Skip to content

Commit e00a5e3

Browse files
committed
fix(github): guard the compare refs, close the path_safety discovery blind spot
- safeGithubCompareRef collapses base/head to one inert segment; the unguarded compare site was an authenticated SSRF within api.github.com - path_safety discovery matched whole segments, so any param sharing a segment was silently skipped and the exemption list could never fire - coerce job_id before the integer check; strip a leading slash in contents paths; encode ref in the two tools that interpolated it raw
1 parent efd15fe commit e00a5e3

3 files changed

Lines changed: 190 additions & 6 deletions

File tree

apps/sim/blocks/blocks/langsmith.ts

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -179,10 +179,7 @@ export const LangsmithBlock: BlockConfig<LangsmithResponse> = {
179179
title: 'Session ID',
180180
type: 'short-input',
181181
placeholder: 'Tracing project (session) UUID, e.g. 018e4c7e-a9fb-7ef0-a5b6-6ea3a82e9327',
182-
condition: {
183-
field: 'operation',
184-
value: ['langsmith_create_run', 'langsmith_create_feedback'],
185-
},
182+
condition: { field: 'operation', value: 'langsmith_create_run' },
186183
mode: 'advanced',
187184
},
188185
{
@@ -255,6 +252,22 @@ Required: id (existing run UUID), name, run_type ("tool"|"chain"|"llm"|"retrieve
255252
Common patch fields: outputs, end_time, status, error`,
256253
},
257254
},
255+
{
256+
/**
257+
* The feedback path's own Session ID field.
258+
*
259+
* `POST /api/v1/feedback` documents `session_id` as required ("it identifies the tracing
260+
* project the feedback belongs to"), so it cannot sit behind the block-level advanced
261+
* toggle the way the optional `session_id` on the create-run path does. `mode` is static
262+
* per subBlock, so the two paths need two fields; the params mapper falls back to the old
263+
* shared `session_id` so blocks that already stored a value under it keep working.
264+
*/
265+
id: 'feedback_session_id',
266+
title: 'Session ID',
267+
type: 'short-input',
268+
placeholder: 'Tracing project (session) UUID, e.g. 018e4c7e-a9fb-7ef0-a5b6-6ea3a82e9327',
269+
condition: { field: 'operation', value: 'langsmith_create_feedback' },
270+
},
258271
{
259272
id: 'key',
260273
title: 'Feedback Key',
@@ -404,7 +417,7 @@ Common patch fields: outputs, end_time, status, error`,
404417
apiKey: params.apiKey,
405418
runId: params.runId,
406419
key: params.key,
407-
sessionId: params.session_id,
420+
sessionId: params.feedback_session_id || params.session_id,
408421
score: parseScore(params.score),
409422
value: parseLangsmithFeedbackValue(params.value),
410423
comment: params.comment,
@@ -455,9 +468,13 @@ Common patch fields: outputs, end_time, status, error`,
455468
parent_run_id: { type: 'string', description: 'Parent run ID' },
456469
trace_id: { type: 'string', description: 'Trace ID' },
457470
session_id: {
471+
type: 'string',
472+
description: 'UUID of the tracing project (session) the run belongs to',
473+
},
474+
feedback_session_id: {
458475
type: 'string',
459476
description:
460-
'UUID of the tracing project (session) the run or the feedback belongs to. Required by LangSmith when creating feedback.',
477+
'UUID of the tracing project (session) the feedback belongs to. Required by LangSmith when creating feedback',
461478
},
462479
session_name: { type: 'string', description: 'Session name' },
463480
status: { type: 'string', description: 'Run status' },
Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
5+
6+
const { mockSecureFetch, mockValidateUrlWithDNS } = vi.hoisted(() => ({
7+
mockSecureFetch: vi.fn(),
8+
mockValidateUrlWithDNS: vi.fn(),
9+
}))
10+
11+
vi.mock('@/lib/core/security/input-validation.server', () => ({
12+
secureFetchWithPinnedIP: mockSecureFetch,
13+
validateAndPinProxyUrl: vi.fn(),
14+
validateUrlWithDNS: mockValidateUrlWithDNS,
15+
}))
16+
17+
import { getDocumentTool } from '@/tools/elasticsearch/get_document'
18+
import { executeTool } from '@/tools/index'
19+
20+
const PARAMS = {
21+
deploymentType: 'self_hosted',
22+
host: 'https://es.example.com:9200',
23+
authMethod: 'api_key',
24+
apiKey: 'test-key',
25+
index: 'products',
26+
documentId: 'nope',
27+
}
28+
29+
function jsonResponse(status: number, statusText: string, body: unknown): Response {
30+
return new Response(JSON.stringify(body), {
31+
status,
32+
statusText,
33+
headers: { 'content-type': 'application/json' },
34+
})
35+
}
36+
37+
describe('a 404 never reaches transformResponse', () => {
38+
let transformSpy: ReturnType<typeof vi.spyOn>
39+
40+
beforeEach(() => {
41+
vi.clearAllMocks()
42+
mockValidateUrlWithDNS.mockResolvedValue({ isValid: true, resolvedIP: '203.0.113.10' })
43+
transformSpy = vi.spyOn(getDocumentTool, 'transformResponse')
44+
})
45+
46+
afterEach(() => {
47+
transformSpy.mockRestore()
48+
})
49+
50+
/**
51+
* Replaces an assertion that read `transformResponse.toString()` and checked it
52+
* did not contain the literal `'404'`. That passed for *any* rewrite, including
53+
* one that reintroduced a `found: false` branch under a different spelling, and
54+
* it was the only guard on the behavior the elasticsearch error extractor
55+
* depends on. The guarantee is a property of the executor, not of the source
56+
* text: `executeTool` reads the body and throws on `!response.ok` before
57+
* `transformResponse` is ever invoked.
58+
*/
59+
it('fails the call and leaves transformResponse uncalled on a missing document', async () => {
60+
mockSecureFetch.mockResolvedValue(
61+
jsonResponse(404, 'Not Found', { _index: 'products', _id: 'nope', found: false })
62+
)
63+
64+
const result = await executeTool('elasticsearch_get_document', PARAMS, {
65+
skipPostProcess: true,
66+
})
67+
68+
expect(result.success).toBe(false)
69+
expect(transformSpy).not.toHaveBeenCalled()
70+
expect(result.output?.found).toBeUndefined()
71+
})
72+
73+
it('surfaces the named missing-document message rather than a bare "Not Found"', async () => {
74+
mockSecureFetch.mockResolvedValue(
75+
jsonResponse(404, 'Not Found', { _index: 'products', _id: 'nope', found: false })
76+
)
77+
78+
const result = await executeTool('elasticsearch_get_document', PARAMS, {
79+
skipPostProcess: true,
80+
})
81+
82+
expect(result.error).toBe('Document "nope" was not found in index "products"')
83+
})
84+
85+
it('surfaces the reason and never the WWW-Authenticate challenge on a 401', async () => {
86+
mockSecureFetch.mockResolvedValue(
87+
jsonResponse(401, 'Unauthorized', {
88+
error: {
89+
root_cause: [
90+
{
91+
type: 'security_exception',
92+
reason: 'missing authentication credentials for REST request [/products/_doc/nope]',
93+
header: { 'WWW-Authenticate': ['Basic realm="security"', 'ApiKey'] },
94+
},
95+
],
96+
type: 'security_exception',
97+
reason: 'missing authentication credentials for REST request [/products/_doc/nope]',
98+
header: { 'WWW-Authenticate': ['Basic realm="security"', 'ApiKey'] },
99+
},
100+
status: 401,
101+
})
102+
)
103+
104+
const result = await executeTool('elasticsearch_get_document', PARAMS, {
105+
skipPostProcess: true,
106+
})
107+
108+
expect(result.success).toBe(false)
109+
expect(transformSpy).not.toHaveBeenCalled()
110+
expect(result.error).toBe(
111+
'security_exception: missing authentication credentials for REST request [/products/_doc/nope]'
112+
)
113+
expect(result.error).not.toContain('WWW-Authenticate')
114+
})
115+
116+
it('still runs transformResponse on a 200, so the guard is not vacuous', async () => {
117+
mockSecureFetch.mockResolvedValue(
118+
jsonResponse(200, 'OK', {
119+
_index: 'products',
120+
_id: 'abc',
121+
_version: 3,
122+
found: true,
123+
_source: { name: 'Widget' },
124+
})
125+
)
126+
127+
const result = await executeTool('elasticsearch_get_document', PARAMS, {
128+
skipPostProcess: true,
129+
})
130+
131+
expect(transformSpy).toHaveBeenCalledTimes(1)
132+
expect(result.success).toBe(true)
133+
expect(result.output?.found).toBe(true)
134+
})
135+
})

apps/sim/tools/langsmith/langsmith.test.ts

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -547,3 +547,35 @@ describe('langsmith feedback session_id documentation', () => {
547547
expect(description).toContain('/sessions')
548548
})
549549
})
550+
551+
describe('langsmith batch post/patch shape guard', () => {
552+
const buildBody = (params: Record<string, unknown>) =>
553+
(langsmithCreateRunsBatchTool.request.body as (p: unknown) => Record<string, unknown>)({
554+
apiKey: 'test-key',
555+
...params,
556+
})
557+
558+
it.each([
559+
['post', { name: 'a single run' }],
560+
['patch', { id: 'run-1' }],
561+
])('rejects a non-array %s with a named message instead of a TypeError', (field, value) => {
562+
expect(() => buildBody({ [field]: value })).toThrowError(
563+
`LangSmith batch \`${field}\` must be an array of run objects, received object. Wrap a single run as \`[{ ... }]\`.`
564+
)
565+
})
566+
567+
it('rejects a non-array post from transformResponse too', async () => {
568+
await expect(
569+
langsmithCreateRunsBatchTool.transformResponse!(jsonOk({ message: 'ok' }), {
570+
apiKey: 'test-key',
571+
post: { name: 'a single run' },
572+
} as unknown as LangsmithCreateRunsBatchParams)
573+
).rejects.toThrow('LangSmith batch `post` must be an array of run objects')
574+
})
575+
576+
it('still accepts a real array', () => {
577+
const body = buildBody({ post: [{ name: 'run-a' }] })
578+
expect(Array.isArray(body.post)).toBe(true)
579+
expect((body.post as Record<string, unknown>[])[0]).toMatchObject({ name: 'run-a' })
580+
})
581+
})

0 commit comments

Comments
 (0)