Skip to content

Commit f020433

Browse files
mzxchandramzxchandra
committed
fix(agiloft): redact credentials from relayed upstream response text
Review finding. The credentials for these operations travel in the submitted form body, so an Agiloft error page or an intermediary that echoes request parameters hands them back in the response. The non-OK create branch and the natural language search refusal both relayed that text to the workflow caller untouched; only the transport-error path was redacting. Redaction now happens where the description is built, so every branch that relays upstream text is covered rather than each one remembering, and the helper moved to the shared utils since a second route needs it. Natural language search grows the same nested try create has, so the refusal branch can see the parsed parameters it needs to redact against.
1 parent 38b6095 commit f020433

5 files changed

Lines changed: 133 additions & 70 deletions

File tree

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,30 @@ describe('EWCreate', () => {
287287
expect(data.error).toContain('retrying creates a second record')
288288
})
289289

290+
/**
291+
* The credentials travel in the submitted form body, so an Agiloft error page
292+
* or an intermediary that echoes request parameters would hand them straight
293+
* back to the workflow caller.
294+
*/
295+
it('keeps the instance password out of an echoed upstream error body', async () => {
296+
arrangeCreate(
297+
res({
298+
ok: false,
299+
status: 500,
300+
text: `<html><body>Error processing request: $KB=Contract+Templates&$login=svc.user&$password=${PLACEHOLDER_PASSWORD}</body></html>`,
301+
})
302+
)
303+
304+
const response = await POST(createMockRequest('POST', { ...baseBody, data: '{"a":"b"}' }))
305+
const data = (await response.json()) as { success: boolean; error?: string }
306+
307+
expect(response.status).toBe(200)
308+
expect(data.success).toBe(false)
309+
expect(data.error).not.toContain(PLACEHOLDER_PASSWORD)
310+
expect(data.error).not.toContain('svc.user')
311+
expect(data.error).toContain('[redacted]')
312+
})
313+
290314
it('keeps the instance password out of a relayed transport error', async () => {
291315
inputValidationMockFns.mockSecureFetchWithPinnedIP.mockRejectedValueOnce(
292316
new Error(`upstream echoed $password=${PLACEHOLDER_PASSWORD}`)

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

Lines changed: 8 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
buildCreateRecordBody,
1414
buildCreateRecordUrl,
1515
describeAgiloftError,
16+
redactAgiloftSecrets,
1617
} from '@/tools/agiloft/utils'
1718
import { executeEwRequest, resolveAgiloftInstance } from '@/tools/agiloft/utils.server'
1819

@@ -31,21 +32,6 @@ const UNCONFIRMED_PREFIX =
3132
/** A typed Agiloft exception means the create was declined, not left in doubt. */
3233
const AGILOFT_EXCEPTION = /EW[A-Za-z]*Exception/
3334

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-
4935
export const POST = withRouteHandler(async (request: NextRequest) => {
5036
const requestId = generateRequestId()
5137

@@ -139,7 +125,12 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
139125
}),
140126
async (response) => {
141127
const text = await response.text()
142-
const described = truncate(describeAgiloftError(text), 300)
128+
/**
129+
* Redacted here, not at each use: every branch below relays this text,
130+
* and the credentials are in the submitted form body, so an error page
131+
* that echoes request parameters would carry them back.
132+
*/
133+
const described = redactAgiloftSecrets(truncate(describeAgiloftError(text), 300), params)
143134

144135
/**
145136
* Every failure below is one Agiloft already decided on, so each is
@@ -226,7 +217,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
226217
return NextResponse.json({
227218
success: false,
228219
output: { id: null, fields: {} },
229-
error: `${UNCONFIRMED_PREFIX} ${redactSecrets(toError(error).message, params)}`,
220+
error: `${UNCONFIRMED_PREFIX} ${redactAgiloftSecrets(toError(error).message, params)}`,
230221
})
231222
}
232223

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,22 @@ describe('EWNLPSearch response', () => {
186186
expect(data.output).toEqual({ records: [], totalCount: 0, truncated: false })
187187
})
188188

189+
it('keeps the instance password out of an echoed upstream error body', async () => {
190+
inputValidationMockFns.mockSecureFetchWithPinnedIP.mockResolvedValueOnce(
191+
res({
192+
ok: false,
193+
status: 500,
194+
text: `<html><body>Error: $login=svc.user&$password=${PLACEHOLDER_PASSWORD}</body></html>`,
195+
})
196+
)
197+
198+
const response = await POST(createMockRequest('POST', baseBody))
199+
const data = (await response.json()) as { error?: string }
200+
201+
expect(data.error).not.toContain(PLACEHOLDER_PASSWORD)
202+
expect(data.error).not.toContain('svc.user')
203+
})
204+
189205
it('caps the records it returns and reports the result as truncated', async () => {
190206
inputValidationMockFns.mockSecureFetchWithPinnedIP.mockResolvedValueOnce(
191207
res({

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

Lines changed: 65 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
AGILOFT_MAX_SEARCH_RECORDS,
1212
buildNlpSearchBody,
1313
buildNlpSearchUrl,
14+
redactAgiloftSecrets,
1415
} from '@/tools/agiloft/utils'
1516
import { executeEwRequest, isAgiloftRefusal, readAlrestJson } from '@/tools/agiloft/utils.server'
1617

@@ -53,67 +54,78 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
5354
if (!parsed.success) return parsed.response
5455
const params = parsed.data.body
5556

56-
const result = await executeEwRequest<AgiloftNlpSearchResponse>(
57-
params,
58-
(base) => ({
59-
url: buildNlpSearchUrl(base),
60-
method: 'POST',
61-
headers: { 'Content-Type': 'application/x-www-form-urlencoded' },
62-
body: buildNlpSearchBody(params),
63-
}),
64-
async (response) => {
65-
/**
66-
* EWNLPSearch is the one legacy `EW*` operation that answers in the JSON
67-
* envelope the `alrest` surface uses — `{success, message, result}` with
68-
* `result` as the record array — rather than `EWREST_` assignments. Its
69-
* request half is form-encoded like the rest of the `EW*` surface, so
70-
* the two halves deliberately use different conventions. Do not
71-
* "correct" this to `parseEwRest` to match create.
72-
*/
73-
const payload = await readAlrestJson<Record<string, unknown>[]>(response)
57+
let result: AgiloftNlpSearchResponse
58+
try {
59+
result = await executeEwRequest<AgiloftNlpSearchResponse>(
60+
params,
61+
(base) => ({
62+
url: buildNlpSearchUrl(base),
63+
method: 'POST',
64+
headers: { 'Content-Type': 'application/x-www-form-urlencoded' },
65+
body: buildNlpSearchBody(params),
66+
}),
67+
async (response) => {
68+
/**
69+
* EWNLPSearch is the one legacy `EW*` operation that answers in the JSON
70+
* envelope the `alrest` surface uses — `{success, message, result}` with
71+
* `result` as the record array — rather than `EWREST_` assignments. Its
72+
* request half is form-encoded like the rest of the `EW*` surface, so
73+
* the two halves deliberately use different conventions. Do not
74+
* "correct" this to `parseEwRest` to match create.
75+
*/
76+
const payload = await readAlrestJson<Record<string, unknown>[]>(response)
7477

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] : []
81-
const records = returned.slice(0, AGILOFT_MAX_SEARCH_RECORDS)
78+
/**
79+
* `result` is documented as an array, but a single-record or empty-object
80+
* answer would make an unchecked `.slice` throw a TypeError that escapes
81+
* as a 500. Normalising keeps an unexpected shape a readable result.
82+
*/
83+
const returned = Array.isArray(payload) ? payload : payload ? [payload] : []
84+
const records = returned.slice(0, AGILOFT_MAX_SEARCH_RECORDS)
8285

83-
if (returned.length > records.length) {
84-
logger.warn(
85-
`[${requestId}] Agiloft NLP search returned ${returned.length} records; truncated to ${AGILOFT_MAX_SEARCH_RECORDS}`
86-
)
87-
}
86+
if (returned.length > records.length) {
87+
logger.warn(
88+
`[${requestId}] Agiloft NLP search returned ${returned.length} records; truncated to ${AGILOFT_MAX_SEARCH_RECORDS}`
89+
)
90+
}
8891

89-
return {
90-
success: true,
91-
output: {
92-
records,
93-
totalCount: records.length,
94-
truncated: returned.length > records.length,
95-
},
92+
return {
93+
success: true,
94+
output: {
95+
records,
96+
totalCount: records.length,
97+
truncated: returned.length > records.length,
98+
},
99+
}
96100
}
101+
)
102+
} catch (error) {
103+
/**
104+
* A refusal Agiloft already decided on is a final answer, not a transient
105+
* fault, so it is reported in a 200 body like every sibling operation does.
106+
* A 500 would have the tool runner retry a search Agiloft has already
107+
* declined. The message carries upstream response text and this request's
108+
* credentials travel in its form body, so it is redacted first.
109+
*/
110+
if (isAgiloftRefusal(error)) {
111+
const described = redactAgiloftSecrets(error.message, params)
112+
logger.warn(`[${requestId}] Agiloft refused the request`, { error: described })
113+
return NextResponse.json({
114+
success: false,
115+
output: { records: [], totalCount: 0, truncated: false },
116+
error: described,
117+
})
97118
}
98-
)
99119

100-
return NextResponse.json(result)
101-
} catch (error) {
102-
/**
103-
* A refusal Agiloft already decided on is a final answer, not a transient
104-
* fault, so it is reported in a 200 body like every sibling operation does.
105-
* A 500 would have the tool runner retry a search Agiloft has already
106-
* declined.
107-
*/
108-
if (isAgiloftRefusal(error)) {
109-
logger.warn(`[${requestId}] Agiloft refused the request`, { error: error.message })
110-
return NextResponse.json({
111-
success: false,
112-
output: { records: [], totalCount: 0, truncated: false },
113-
error: error.message,
114-
})
120+
logger.error(`[${requestId}] Error running Agiloft NLP search:`, error)
121+
return NextResponse.json(
122+
{ success: false, error: redactAgiloftSecrets(toError(error).message, params) },
123+
{ status: 500 }
124+
)
115125
}
116126

127+
return NextResponse.json(result)
128+
} catch (error) {
117129
logger.error(`[${requestId}] Error running Agiloft NLP search:`, error)
118130

119131
return NextResponse.json({ success: false, error: toError(error).message }, { status: 500 })

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,26 @@ export function describeAgiloftError(body: string): string {
4343
return detail ? `${typed[1]}: ${detail}` : typed[1]
4444
}
4545

46+
/**
47+
* Strips the instance credentials out of any text relayed to the caller.
48+
*
49+
* The credentials for the operations that accept them travel in the request
50+
* body, and an Agiloft error page or an intermediary that echoes submitted form
51+
* parameters would carry them straight back. Anything derived from an upstream
52+
* response or a transport error goes through here before it reaches a workflow
53+
* result or a log.
54+
*/
55+
export function redactAgiloftSecrets(
56+
message: string,
57+
credentials: { login: string; password: string }
58+
): string {
59+
let safe = message
60+
for (const secret of [credentials.password, credentials.login]) {
61+
if (secret) safe = safe.split(secret).join('[redacted]')
62+
}
63+
return safe
64+
}
65+
4666
/** Language sent on every Agiloft call; EWLogin rejects the request without it. */
4767
export const AGILOFT_LANG = 'en'
4868

0 commit comments

Comments
 (0)