Skip to content

Commit cd16012

Browse files
mzxchandramzxchandra
committed
fix(agiloft): redact encoded credentials and separate declines from unconfirmed writes
Review round 2. Redaction only replaced the raw credential strings, but the values are sent form-encoded, so an error page quoting the submitted parameters quotes the encoded spelling. A password with a space leaves as a%20b or a+b and sailed past a replace that only knew a b. All three spellings are now replaced, longest first. Redaction also ran after truncation on the create path, so clipping the text could cut through a credential and leave a prefix that no longer matched anything being replaced. The order is reversed and the reason recorded, since the two read as interchangeable and are not. The non-OK create branch always warned that the record might exist, including on a 4xx validation decline where Agiloft had refused the request and written nothing. That is the opposite of the rule this branch introduces: it told the caller not to retry a create that never happened. A 4xx carrying a typed exception is now reported as a definite refusal; a 5xx keeps the warning, because a server fault may have committed first. Redirects were refused on the calls carrying credentials in a body but not on executeAgiloftRequest's operation fetch or on logout, both of which send a Bearer token that secureFetchWithPinnedIP would replay to a redirect host. Those are the calls behind list tables and saved search.
1 parent f020433 commit cd16012

5 files changed

Lines changed: 110 additions & 6 deletions

File tree

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

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -311,6 +311,43 @@ describe('EWCreate', () => {
311311
expect(data.error).toContain('[redacted]')
312312
})
313313

314+
/**
315+
* A 4xx carrying a typed exception is Agiloft declining the request, so
316+
* nothing was written and warning about a phantom record is wrong.
317+
*/
318+
it('reports a 4xx validation decline as a definite failure, not an unconfirmed write', async () => {
319+
arrangeCreate(
320+
res({
321+
ok: false,
322+
status: 400,
323+
text: '<html><body>EWWrongDataException has occurred: Wrong format/value pointed to start_date</body></html>',
324+
})
325+
)
326+
327+
const response = await POST(createMockRequest('POST', { ...baseBody, data: '{"a":"b"}' }))
328+
const data = (await response.json()) as { success: boolean; error?: string }
329+
330+
expect(response.status).toBe(200)
331+
expect(data.success).toBe(false)
332+
expect(data.error).toContain('no record was written')
333+
expect(data.error).not.toContain('may exist')
334+
})
335+
336+
it('keeps the unconfirmed warning on a 5xx, which may have committed first', async () => {
337+
arrangeCreate(
338+
res({
339+
ok: false,
340+
status: 502,
341+
text: '<html><body>EWUnexpectedException has occurred: upstream failure</body></html>',
342+
})
343+
)
344+
345+
const response = await POST(createMockRequest('POST', { ...baseBody, data: '{"a":"b"}' }))
346+
const data = (await response.json()) as { error?: string }
347+
348+
expect(data.error).toContain('may exist')
349+
})
350+
314351
it('keeps the instance password out of a relayed transport error', async () => {
315352
inputValidationMockFns.mockSecureFetchWithPinnedIP.mockRejectedValueOnce(
316353
new Error(`upstream echoed $password=${PLACEHOLDER_PASSWORD}`)

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

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
130130
* and the credentials are in the submitted form body, so an error page
131131
* that echoes request parameters would carry them back.
132132
*/
133-
const described = redactAgiloftSecrets(truncate(describeAgiloftError(text), 300), params)
133+
const described = truncate(redactAgiloftSecrets(describeAgiloftError(text), params), 300)
134134

135135
/**
136136
* Every failure below is one Agiloft already decided on, so each is
@@ -139,10 +139,19 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
139139
* record rather than converging on the first.
140140
*/
141141
if (!response.ok) {
142+
/**
143+
* A 4xx carrying a typed exception is Agiloft validating the request
144+
* and declining it, so nothing was written and a corrected retry is
145+
* safe. A 5xx is a server fault that may have committed first, so it
146+
* keeps the unconfirmed warning whatever the body says.
147+
*/
148+
const declined = response.status < 500 && AGILOFT_EXCEPTION.test(described)
142149
return {
143150
success: false,
144151
output: { id: null, fields: {} },
145-
error: `${UNCONFIRMED_PREFIX} Agiloft answered ${response.status}: ${described}`,
152+
error: declined
153+
? `Agiloft refused the create, so no record was written: ${described}`
154+
: `${UNCONFIRMED_PREFIX} Agiloft answered ${response.status}: ${described}`,
146155
}
147156
}
148157

@@ -155,17 +164,17 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
155164
* unexplained missing ID leaves the write in doubt.
156165
*/
157166
if (!id) {
158-
const refused = AGILOFT_EXCEPTION.test(described)
167+
const declined = AGILOFT_EXCEPTION.test(described)
159168
logger.error(`[${requestId}] Agiloft create returned no record ID`, {
160169
table: params.table,
161170
login: params.login,
162171
fields: Object.keys(fieldValues),
163-
refused,
172+
declined,
164173
})
165174
return {
166175
success: false,
167176
output: { id: null, fields: {} },
168-
error: refused
177+
error: declined
169178
? `Agiloft refused the create, so no record was written: ${described}`
170179
: `${UNCONFIRMED_PREFIX} Agiloft accepted the create but returned no record ID: ${described}`,
171180
}

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,7 @@ export async function agiloftLogoutPinned(
144144
{
145145
method: 'POST',
146146
headers: { Authorization: authorization },
147+
...AGILOFT_NO_REDIRECT,
147148
}
148149
)
149150
} catch (error) {
@@ -189,6 +190,7 @@ export async function executeAgiloftRequest<R extends ToolResponse>(
189190
Authorization: session.authorization,
190191
},
191192
body: req.body,
193+
...AGILOFT_NO_REDIRECT,
192194
})
193195
return await transformResponse(response)
194196
} finally {

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import {
2020
describeAgiloftError,
2121
ewCredentialBody,
2222
parseFieldList,
23+
redactAgiloftSecrets,
2324
} from '@/tools/agiloft/utils'
2425

2526
/** Obvious non-secret so credential scanners do not flag these fixtures. */
@@ -386,3 +387,36 @@ describe('pagination input', () => {
386387
expect(sent.get('limit')).toBe('10')
387388
})
388389
})
390+
391+
describe('credential redaction', () => {
392+
const creds = { login: 'svc.user', password: 'p@ss word&1' }
393+
394+
/**
395+
* The value goes out form-encoded, so an error page quoting the submitted
396+
* parameters quotes the encoded spelling, not the raw one.
397+
*/
398+
it('redacts the encoded spellings the request actually sent', () => {
399+
const echoed = [
400+
`raw=${creds.password}`,
401+
`enc=${encodeURIComponent(creds.password)}`,
402+
`form=${encodeURIComponent(creds.password).replace(/%20/g, '+')}`,
403+
].join(' ')
404+
405+
const safe = redactAgiloftSecrets(echoed, creds)
406+
407+
expect(safe).not.toContain(creds.password)
408+
expect(safe).not.toContain(encodeURIComponent(creds.password))
409+
expect(safe).not.toContain('p%40ss+word%261')
410+
expect(safe.match(/\[redacted\]/g)).toHaveLength(3)
411+
})
412+
413+
it('redacts the login as well as the password', () => {
414+
expect(redactAgiloftSecrets('user=svc.user', creds)).not.toContain('svc.user')
415+
})
416+
417+
it('leaves text carrying no credential untouched', () => {
418+
expect(redactAgiloftSecrets('EWWrongDataException: no column bogus', creds)).toBe(
419+
'EWWrongDataException: no column bogus'
420+
)
421+
})
422+
})

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

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

46+
/**
47+
* Every spelling of a credential that could come back in an echoed response.
48+
*
49+
* The value is sent form-encoded, so an error page that quotes the submitted
50+
* parameters quotes the encoded form, not the raw one. A password with a space
51+
* leaves as `a%20b` or `a+b` and would sail past a replace that only knows
52+
* `a b`. Longest first so a variant that contains another is replaced whole.
53+
*/
54+
function secretSpellings(secret: string): string[] {
55+
const encoded = encodeURIComponent(secret)
56+
return [...new Set([secret, encoded, encoded.replace(/%20/g, '+')])].sort(
57+
(a, b) => b.length - a.length
58+
)
59+
}
60+
4661
/**
4762
* Strips the instance credentials out of any text relayed to the caller.
4863
*
@@ -51,14 +66,21 @@ export function describeAgiloftError(body: string): string {
5166
* parameters would carry them straight back. Anything derived from an upstream
5267
* response or a transport error goes through here before it reaches a workflow
5368
* result or a log.
69+
*
70+
* Redact before truncating, never after: clipping the text first can cut
71+
* through a credential and leave a prefix that no longer matches anything this
72+
* replaces.
5473
*/
5574
export function redactAgiloftSecrets(
5675
message: string,
5776
credentials: { login: string; password: string }
5877
): string {
5978
let safe = message
6079
for (const secret of [credentials.password, credentials.login]) {
61-
if (secret) safe = safe.split(secret).join('[redacted]')
80+
if (!secret) continue
81+
for (const spelling of secretSpellings(secret)) {
82+
safe = safe.split(spelling).join('[redacted]')
83+
}
6284
}
6385
return safe
6486
}

0 commit comments

Comments
 (0)