Skip to content

Commit 38b6095

Browse files
mzxchandramzxchandra
committed
fix(agiloft): refuse redirects on credentialed calls and settle unconfirmed writes
Adversarial review findings, two of which two independent reviewers raised. secureFetchWithPinnedIP replays the whole options object to a redirect's Location - same method, same body - and its stripAuthOnRedirect only removes the Authorization header. Moving credentials into the request body therefore made a 3xx from the instance POST the Agiloft username and password to whatever public host it named, and on a create it would re-send the write. The redirect target is screened for private addresses but is not held to the original host. Every Agiloft call that carries a credential or a token now refuses redirects outright; none of these operations redirect in normal use. A create that failed after the request was on the wire - a timeout, a reset, a refused redirect, an oversized body - still escaped to the outer handler and returned 500. That is the retryable status this operation exists to avoid, on precisely the paths where the write may already have committed. Those failures are now settled with the same do-not-retry warning, and the instance URL is resolved up front so a rejected URL stays a 400. The warning itself was being applied too widely: a typed Agiloft exception means the create was declined and nothing was written, so a corrected retry is safe. Only an unexplained missing ID leaves the write in doubt. The two now read differently. Also: describes the error before truncating rather than after, which was cutting the exception text out of the message it was meant to explain; refuses a record ID that is not a number, since it is chained straight into reads and updates; redacts the instance credentials from any relayed transport error; normalises a non-array search result that would otherwise throw past the handler as a 500; and logs the field names and creator login so the "check the table" instruction has something to search on.
1 parent 2fe4b82 commit 38b6095

4 files changed

Lines changed: 224 additions & 54 deletions

File tree

‎apps/sim/app/api/tools/agiloft/create_record/route.test.ts‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -239,6 +239,66 @@ describe('EWCreate', () => {
239239
expect(data.error).toContain('retrying creates a second record')
240240
})
241241

242+
/**
243+
* A typed exception is Agiloft declining the create outright, so nothing was
244+
* written and the caller must not be warned about a phantom record.
245+
*/
246+
it('reports a typed refusal as a definite failure, not an unconfirmed write', async () => {
247+
arrangeCreate(
248+
res({
249+
text: '<html><body>EWWrongDataException has occurred: No column bogus in table contract</body></html>',
250+
})
251+
)
252+
253+
const response = await POST(createMockRequest('POST', { ...baseBody, data: '{"bogus":"x"}' }))
254+
const data = (await response.json()) as { success: boolean; error?: string }
255+
256+
expect(response.status).toBe(200)
257+
expect(data.success).toBe(false)
258+
expect(data.error).toContain('no record was written')
259+
expect(data.error).not.toContain('may exist')
260+
})
261+
262+
it('refuses a record ID that is not a number rather than chaining it downstream', async () => {
263+
arrangeCreate(res({ text: "EWREST_id='353;DROP';" }))
264+
265+
const response = await POST(createMockRequest('POST', { ...baseBody, data: '{"a":"b"}' }))
266+
const data = (await response.json()) as { success: boolean; output: { id: string | null } }
267+
268+
expect(data.success).toBe(false)
269+
expect(data.output.id).toBeNull()
270+
})
271+
272+
/**
273+
* The request was already on the wire, so the write may have committed. A 500
274+
* here is what would have the caller retry and duplicate the record.
275+
*/
276+
it('settles a transport failure after the request was sent instead of returning 500', async () => {
277+
inputValidationMockFns.mockSecureFetchWithPinnedIP.mockRejectedValueOnce(
278+
new Error('socket hang up')
279+
)
280+
281+
const response = await POST(createMockRequest('POST', { ...baseBody, data: '{"a":"b"}' }))
282+
const data = (await response.json()) as { success: boolean; error?: string }
283+
284+
expect(response.status).toBe(200)
285+
expect(data.success).toBe(false)
286+
expect(data.error).toContain('could not be confirmed')
287+
expect(data.error).toContain('retrying creates a second record')
288+
})
289+
290+
it('keeps the instance password out of a relayed transport error', async () => {
291+
inputValidationMockFns.mockSecureFetchWithPinnedIP.mockRejectedValueOnce(
292+
new Error(`upstream echoed $password=${PLACEHOLDER_PASSWORD}`)
293+
)
294+
295+
const response = await POST(createMockRequest('POST', { ...baseBody, data: '{"a":"b"}' }))
296+
const data = (await response.json()) as { error?: string }
297+
298+
expect(data.error).not.toContain(PLACEHOLDER_PASSWORD)
299+
expect(data.error).toContain('[redacted]')
300+
})
301+
242302
it('rejects a data parameter that is not JSON without calling Agiloft', async () => {
243303
const response = await POST(createMockRequest('POST', { ...baseBody, data: 'title = X' }))
244304
const data = (await response.json()) as { success: boolean; error?: string }

‎apps/sim/app/api/tools/agiloft/create_record/route.ts‎

Lines changed: 138 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,38 @@ import {
1414
buildCreateRecordUrl,
1515
describeAgiloftError,
1616
} from '@/tools/agiloft/utils'
17-
import { executeEwRequest } from '@/tools/agiloft/utils.server'
17+
import { executeEwRequest, resolveAgiloftInstance } from '@/tools/agiloft/utils.server'
1818

1919
export const dynamic = 'force-dynamic'
2020

2121
const logger = createLogger('AgiloftCreateRecordAPI')
2222

23+
/**
24+
* Opens every failure where the write may already have committed. The caller
25+
* has to be told not to retry blindly: a second create writes a second record
26+
* rather than recovering the first.
27+
*/
28+
const UNCONFIRMED_PREFIX =
29+
'The create could not be confirmed, so the record may exist. Check the table before retrying - retrying creates a second record.'
30+
31+
/** A typed Agiloft exception means the create was declined, not left in doubt. */
32+
const AGILOFT_EXCEPTION = /EW[A-Za-z]*Exception/
33+
34+
/**
35+
* Strips the instance credentials out of an error string.
36+
*
37+
* Errors relayed to the caller can carry upstream text, and a servlet error
38+
* page that echoes submitted request parameters would carry the password that
39+
* now travels in the request body.
40+
*/
41+
function redactSecrets(message: string, params: { login: string; password: string }): string {
42+
let safe = message
43+
for (const secret of [params.password, params.login]) {
44+
if (secret) safe = safe.split(secret).join('[redacted]')
45+
}
46+
return safe
47+
}
48+
2349
export const POST = withRouteHandler(async (request: NextRequest) => {
2450
const requestId = generateRequestId()
2551

@@ -86,64 +112,123 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
86112
})
87113
}
88114

89-
const result = await executeEwRequest<AgiloftRecordResponse>(
90-
params,
91-
(base) => ({
92-
url: buildCreateRecordUrl(base),
93-
method: 'POST',
94-
headers: { 'Content-Type': 'application/x-www-form-urlencoded' },
95-
body: requestBody,
96-
}),
97-
async (response) => {
98-
const text = await response.text()
99-
100-
/**
101-
* Every failure below is one Agiloft already decided on, so each is
102-
* reported in a 200 body with `success: false`. A non-2xx status would
103-
* make the tool runner retry, and a retried create writes a second
104-
* record rather than converging on the first.
105-
*/
106-
if (!response.ok) {
107-
return {
108-
success: false,
109-
output: { id: null, fields: {} },
110-
error: `Agiloft error ${response.status}: ${describeAgiloftError(truncate(text, 300))}`,
115+
/**
116+
* Resolved here rather than only inside the executor so a rejected instance
117+
* URL stays a pre-flight failure. Everything thrown after this point has to
118+
* be treated as "the request may have been transmitted".
119+
*/
120+
try {
121+
await resolveAgiloftInstance(params.instanceUrl)
122+
} catch (error) {
123+
logger.warn(`[${requestId}] Rejected Agiloft instance URL`, { error })
124+
return NextResponse.json(
125+
{ success: false, output: { id: null, fields: {} }, error: toError(error).message },
126+
{ status: 400 }
127+
)
128+
}
129+
130+
let result: AgiloftRecordResponse
131+
try {
132+
result = await executeEwRequest<AgiloftRecordResponse>(
133+
params,
134+
(base) => ({
135+
url: buildCreateRecordUrl(base),
136+
method: 'POST',
137+
headers: { 'Content-Type': 'application/x-www-form-urlencoded' },
138+
body: requestBody,
139+
}),
140+
async (response) => {
141+
const text = await response.text()
142+
const described = truncate(describeAgiloftError(text), 300)
143+
144+
/**
145+
* Every failure below is one Agiloft already decided on, so each is
146+
* reported in a 200 body with `success: false`. A non-2xx status would
147+
* make the tool runner retry, and a retried create writes a second
148+
* record rather than converging on the first.
149+
*/
150+
if (!response.ok) {
151+
return {
152+
success: false,
153+
output: { id: null, fields: {} },
154+
error: `${UNCONFIRMED_PREFIX} Agiloft answered ${response.status}: ${described}`,
155+
}
111156
}
112-
}
113157

114-
const values = parseEwRest(text)
115-
const id = values.get('id')
116-
117-
/**
118-
* EWCreate publishes the new record's ID as `EWREST_id`. Reaching this
119-
* branch means the write may well have landed while the ID did not come
120-
* back, so the caller is told not to retry blindly: a second attempt
121-
* would create a duplicate rather than recover the first record.
122-
*/
123-
if (!id) {
124-
logger.error(`[${requestId}] Agiloft create returned no record ID`, {
125-
table: params.table,
126-
})
127-
return {
128-
success: false,
129-
output: { id: null, fields: {} },
130-
error: `Agiloft accepted the create but returned no record ID, so the record may exist. Check the table before retrying - retrying creates a second record. Response: ${describeAgiloftError(truncate(text, 300))}`,
158+
const values = parseEwRest(text)
159+
const id = values.get('id')
160+
161+
/**
162+
* A typed exception in the body is Agiloft declining the create, so
163+
* nothing was written and a corrected retry is safe. Only an
164+
* unexplained missing ID leaves the write in doubt.
165+
*/
166+
if (!id) {
167+
const refused = AGILOFT_EXCEPTION.test(described)
168+
logger.error(`[${requestId}] Agiloft create returned no record ID`, {
169+
table: params.table,
170+
login: params.login,
171+
fields: Object.keys(fieldValues),
172+
refused,
173+
})
174+
return {
175+
success: false,
176+
output: { id: null, fields: {} },
177+
error: refused
178+
? `Agiloft refused the create, so no record was written: ${described}`
179+
: `${UNCONFIRMED_PREFIX} Agiloft accepted the create but returned no record ID: ${described}`,
180+
}
131181
}
132-
}
133182

134-
/**
135-
* EWCreate documents only the ID, but any other assignment it returns is
136-
* a field value on the new record, so it is passed through rather than
137-
* dropped. Usually empty.
138-
*/
139-
const fields: Record<string, unknown> = {}
140-
for (const [key, value] of values) {
141-
if (key !== 'id') fields[key] = value
183+
/**
184+
* The ID is chained straight into reads, updates, and deletes, and the
185+
* response format is unescaped text, so a value that is not a plain
186+
* record ID is refused rather than passed downstream.
187+
*/
188+
if (!/^\d+$/.test(id)) {
189+
logger.error(`[${requestId}] Agiloft create returned a non-numeric record ID`, {
190+
table: params.table,
191+
})
192+
return {
193+
success: false,
194+
output: { id: null, fields: {} },
195+
error: `${UNCONFIRMED_PREFIX} Agiloft returned a record ID that is not a number.`,
196+
}
197+
}
198+
199+
/**
200+
* EWCreate documents only the ID, but any other assignment it returns is
201+
* a field value on the new record, so it is passed through rather than
202+
* dropped. Usually empty.
203+
*/
204+
const fields: Record<string, unknown> = {}
205+
for (const [key, value] of values) {
206+
if (key !== 'id') fields[key] = value
207+
}
208+
209+
return { success: true, output: { id, fields } }
142210
}
211+
)
212+
} catch (error) {
213+
/**
214+
* The request was already on the wire when this threw — a timeout, a
215+
* reset, a refused redirect, an oversized body. The write may have
216+
* committed, so this is a settled failure carrying the same warning. A
217+
* 500 is what would have the caller retry and duplicate the record, which
218+
* is the failure this operation exists to prevent.
219+
*/
220+
logger.error(`[${requestId}] Agiloft create failed after the request was sent`, {
221+
error,
222+
table: params.table,
223+
login: params.login,
224+
})
143225

144-
return { success: true, output: { id, fields } }
145-
}
146-
)
226+
return NextResponse.json({
227+
success: false,
228+
output: { id: null, fields: {} },
229+
error: `${UNCONFIRMED_PREFIX} ${redactSecrets(toError(error).message, params)}`,
230+
})
231+
}
147232

148233
return NextResponse.json(result)
149234
} catch (error) {

‎apps/sim/app/api/tools/agiloft/nlp_search/route.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,14 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
7070
* the two halves deliberately use different conventions. Do not
7171
* "correct" this to `parseEwRest` to match create.
7272
*/
73-
const returned = (await readAlrestJson<Record<string, unknown>[]>(response)) ?? []
73+
const payload = await readAlrestJson<Record<string, unknown>[]>(response)
74+
75+
/**
76+
* `result` is documented as an array, but a single-record or empty-object
77+
* answer would make an unchecked `.slice` throw a TypeError that escapes
78+
* as a 500. Normalising keeps an unexpected shape a readable result.
79+
*/
80+
const returned = Array.isArray(payload) ? payload : payload ? [payload] : []
7481
const records = returned.slice(0, AGILOFT_MAX_SEARCH_RECORDS)
7582

7683
if (returned.length > records.length) {

‎apps/sim/tools/agiloft/utils.server.ts‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,21 @@ import type { HttpMethod, ToolResponse } from '@/tools/types'
1212

1313
const logger = createLogger('AgiloftAuthServer')
1414

15+
/**
16+
* Refuses redirects on any request that carries Agiloft credentials.
17+
*
18+
* `secureFetchWithPinnedIP` replays the whole options object to a redirect's
19+
* `Location` — same method, same body — and `stripAuthOnRedirect` only removes
20+
* the `Authorization` header, which does nothing for `$login`/`$password` in a
21+
* form body. The redirect target is checked for private addresses but is not
22+
* held to the original host, so a 3xx would POST the instance's credentials to
23+
* whatever public host it names. On a write it would also re-send the create.
24+
*
25+
* None of these operations redirect in normal use; Agiloft's redirect decorator
26+
* is opt-in per call and this connector never asks for it.
27+
*/
28+
const AGILOFT_NO_REDIRECT = { maxRedirects: 0 } as const
29+
1530
export interface AgiloftRequestConfig {
1631
url: string
1732
method: HttpMethod
@@ -75,6 +90,7 @@ export async function agiloftLoginPinned(
7590
const response = await secureFetchWithPinnedIP(`${base}/ewws/EWLogin`, resolvedIP, {
7691
method: 'POST',
7792
headers: { 'Content-Type': 'application/x-www-form-urlencoded' },
93+
...AGILOFT_NO_REDIRECT,
7894
body: formEncode(
7995
filterUndefined({
8096
$KB: params.knowledgeBase,
@@ -263,6 +279,7 @@ export async function executeAlrestRequest<R extends ToolResponse>(
263279
method: req.method,
264280
headers: { ...req.headers, Authorization: session.authorization },
265281
body: req.body,
282+
...AGILOFT_NO_REDIRECT,
266283
})
267284
return await transformResponse(response)
268285
} finally {
@@ -294,6 +311,7 @@ export async function executeEwRequest<R extends ToolResponse>(
294311
method: req.method,
295312
headers: req.headers,
296313
body: req.body,
314+
...AGILOFT_NO_REDIRECT,
297315
})
298316
return await transformResponse(response)
299317
}

0 commit comments

Comments
 (0)