Skip to content

Commit d645927

Browse files
authored
fix(pii): stop one half-emoji from failing log redaction (#8867)
* fix(pii): stop one half-emoji from failing log redaction A lone UTF-16 surrogate (e.g. an emoji cut by display truncation) made Presidio fail encoding its /redact_batch response, so mask-batch answered 500, each chunk was retried 8 times, and the default scrub mode replaced every string in the execution log with [REDACTION_FAILED]. - validate_pii: every string sent to Presidio goes out well-formed (toWellFormed, length-preserving U+FFFD); Presidio 4xx (other than 408/429) becomes PiiServiceRejectedError, unreachable / 502-504 becomes PiiServiceUnavailableError - mask-batch route: rejected -> 422 (never resent), unavailable -> 503 - mask client: optional failedChunkPlaceholder scrubs only the failed chunk and reports scrubbedCount; an outage (connection error or 502/503/504) stops sending queued chunks, a 500 does not - pii-redaction: scrub mode uses it and logs scrubbedCount, so one bad chunk no longer erases the whole payload; throw mode is unchanged - display-filters: truncate on a code-point boundary with an exact count - Presidio: any unencodable string in a request body is a 422, and 422 bodies no longer echo request input * test(pii): assert the outage breaker through masked output, not request counts * refactor(pii): drop unused error status, exports, and dead 422 escaping
1 parent 722510e commit d645927

18 files changed

Lines changed: 527 additions & 167 deletions

File tree

‎apps/pii/server.py‎

Lines changed: 46 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,15 @@
66
single PII_URL serves both.
77
"""
88

9+
import json
910
import logging
1011
import time
1112
from typing import Any
1213

1314
import regex as regex_module
14-
from fastapi import FastAPI, HTTPException
15+
from fastapi import FastAPI, HTTPException, Request
16+
from fastapi.exceptions import RequestValidationError
17+
from fastapi.responses import JSONResponse
1518
from presidio_analyzer import (
1619
AnalyzerEngine,
1720
BatchAnalyzerEngine,
@@ -46,7 +49,7 @@
4649
)
4750
from presidio_anonymizer import AnonymizerEngine
4851
from presidio_anonymizer.entities import OperatorConfig
49-
from pydantic import BaseModel
52+
from pydantic import BaseModel, model_validator
5053

5154
# Languages served. Each needs its spaCy model installed in the image; the
5255
# es/it/pl/fi predefined recognizers (ES_NIF, IT_FISCAL_CODE, PL_PESEL, ...)
@@ -307,6 +310,41 @@ def _analyze_many(
307310

308311
app = FastAPI(title="Sim Presidio", docs_url=None, redoc_url=None)
309312

313+
314+
@app.exception_handler(RequestValidationError)
315+
async def request_validation_error(_request: Request, exc: RequestValidationError) -> JSONResponse:
316+
"""422 without FastAPI's echo of each offending input value, which would copy
317+
request text (possibly PII) into the error."""
318+
return JSONResponse(
319+
status_code=422,
320+
content={
321+
"detail": [
322+
{
323+
"loc": list(error.get("loc", ())),
324+
"msg": error.get("msg", ""),
325+
"type": error.get("type", ""),
326+
}
327+
for error in exc.errors()
328+
]
329+
},
330+
)
331+
332+
333+
class RequestModel(BaseModel):
334+
"""Base for every request body. JSON can carry an escaped unpaired UTF-16
335+
surrogate that decodes into a str with no UTF-8 encoding; echoing it in the
336+
response would fail as an opaque 500, so reject it up front as a 422."""
337+
338+
@model_validator(mode="before")
339+
@classmethod
340+
def _require_utf8_encodable(cls, data: Any) -> Any:
341+
try:
342+
json.dumps(data, ensure_ascii=False).encode("utf-8")
343+
except UnicodeEncodeError:
344+
raise ValueError("body contains an unpaired surrogate") from None
345+
return data
346+
347+
310348
# Internal entity id assigned to the i-th user-supplied custom pattern. Never
311349
# surfaced: the anonymizer maps it back to the pattern's chosen `replacement`, and
312350
# callers relabel any leftover CUSTOM_<i> span to the pattern's display name.
@@ -387,7 +425,7 @@ def resolve_entities(
387425
return list(req_entities) + custom_entity_ids
388426

389427

390-
class AnalyzeRequest(BaseModel):
428+
class AnalyzeRequest(RequestModel):
391429
text: str
392430
language: str = "en"
393431
entities: list[str] | None = None
@@ -396,15 +434,15 @@ class AnalyzeRequest(BaseModel):
396434
patterns: list[CustomPattern] | None = None
397435

398436

399-
class AnalyzeBatchRequest(BaseModel):
437+
class AnalyzeBatchRequest(RequestModel):
400438
texts: list[str]
401439
language: str = "en"
402440
entities: list[str] | None = None
403441
score_threshold: float | None = None
404442
patterns: list[CustomPattern] | None = None
405443

406444

407-
class AnonymizeRequest(BaseModel):
445+
class AnonymizeRequest(RequestModel):
408446
text: str
409447
analyzer_results: list[dict[str, Any]] = []
410448
anonymizers: dict[str, dict[str, Any]] | None = None
@@ -417,14 +455,14 @@ class AnonymizeBatchItem(BaseModel):
417455
analyzer_results: list[dict[str, Any]] = []
418456

419457

420-
class AnonymizeBatchRequest(BaseModel):
458+
class AnonymizeBatchRequest(RequestModel):
421459
items: list[AnonymizeBatchItem] = []
422460
anonymizers: dict[str, dict[str, Any]] | None = None
423461
operators: dict[str, dict[str, Any]] | None = None
424462
patterns: list[CustomPattern] | None = None
425463

426464

427-
class RedactRequest(BaseModel):
465+
class RedactRequest(RequestModel):
428466
text: str
429467
language: str = "en"
430468
entities: list[str] | None = None
@@ -434,7 +472,7 @@ class RedactRequest(BaseModel):
434472
patterns: list[CustomPattern] | None = None
435473

436474

437-
class RedactBatchRequest(BaseModel):
475+
class RedactBatchRequest(RequestModel):
438476
texts: list[str]
439477
language: str = "en"
440478
entities: list[str] | None = None
Lines changed: 32 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,17 @@
1-
import { createMockRequest, hybridAuthMockFns } from '@sim/testing'
1+
import { hybridAuthMockFns } from '@sim/testing/mocks/hybrid-auth.mock'
2+
import { createMockRequest } from '@sim/testing/mocks/request.mock'
23
import { beforeEach, describe, expect, it, vi } from 'vitest'
3-
4-
const { mockMaskPIIBatch } = vi.hoisted(() => ({
5-
mockMaskPIIBatch: vi.fn(),
6-
}))
4+
import { POST } from '@/app/api/guardrails/mask-batch/route'
75

86
const mockCheckInternalAuth = hybridAuthMockFns.mockCheckInternalAuth
97

10-
vi.mock('@/lib/guardrails/validate_pii', () => ({
11-
maskPIIBatch: mockMaskPIIBatch,
12-
}))
13-
14-
import { POST } from '@/app/api/guardrails/mask-batch/route'
15-
168
describe('POST /api/guardrails/mask-batch', () => {
9+
let presidioFetch: ReturnType<typeof vi.fn>
10+
1711
beforeEach(() => {
1812
mockCheckInternalAuth.mockResolvedValue({ success: true })
19-
mockMaskPIIBatch.mockImplementation(async (texts: string[]) => texts.map((t) => `M(${t})`))
13+
presidioFetch = vi.fn()
14+
vi.stubGlobal('fetch', presidioFetch)
2015
})
2116

2217
it('returns 401 without internal auth', async () => {
@@ -30,6 +25,30 @@ describe('POST /api/guardrails/mask-batch', () => {
3025
)
3126

3227
expect(res.status).toBe(401)
33-
expect(mockMaskPIIBatch).not.toHaveBeenCalled()
28+
expect(presidioFetch).not.toHaveBeenCalled()
29+
})
30+
31+
it.each([
32+
[
33+
'422 when Presidio rejects the input, so it is never resent',
34+
422,
35+
() => new Response('{}', { status: 422 }),
36+
],
37+
[
38+
'503 when Presidio is unreachable, so it reads as an outage',
39+
503,
40+
() => Promise.reject(new TypeError('fetch failed')),
41+
],
42+
[
43+
'500 for any other Presidio failure, retried on its own',
44+
500,
45+
() => new Response('boom', { status: 500 }),
46+
],
47+
] as const)('answers %s', async (_name, status, presidio) => {
48+
presidioFetch.mockImplementationOnce(presidio)
49+
50+
const res = await POST(createMockRequest('POST', { texts: ['a'], entityTypes: [] }))
51+
52+
expect(res.status).toBe(status)
3453
})
3554
})

‎apps/sim/app/api/guardrails/mask-batch/route.ts‎

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,25 @@ import { guardrailsMaskBatchContract } from '@/lib/api/contracts'
55
import { parseRequest } from '@/lib/api/server'
66
import { checkInternalAuth } from '@/lib/auth/hybrid'
77
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
8-
import { maskPIIBatch } from '@/lib/guardrails/validate_pii'
8+
import {
9+
maskPIIBatch,
10+
PiiServiceRejectedError,
11+
PiiServiceUnavailableError,
12+
} from '@/lib/guardrails/validate_pii'
913

1014
const logger = createLogger('GuardrailsMaskBatchAPI')
1115

16+
/**
17+
* Tells the mask client how to react to a failure: 422 = Presidio rejected this
18+
* input (never resend it), 503 = Presidio is down (retry, and stop sending queued
19+
* chunks), 500 = anything else (retry this chunk alone).
20+
*/
21+
function statusForMaskError(error: unknown): 422 | 503 | 500 {
22+
if (error instanceof PiiServiceRejectedError) return 422
23+
if (error instanceof PiiServiceUnavailableError) return 503
24+
return 500
25+
}
26+
1227
/**
1328
* Internal batch PII masking. The log-redaction persist path runs in both the
1429
* Next.js server and the trigger.dev runtime, but only the app task reaches the
@@ -36,15 +51,13 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
3651
})
3752
return NextResponse.json({ masked })
3853
} catch (error) {
39-
// An unreachable/misconfigured Presidio service makes maskPIIBatch throw; fail
40-
// loudly here (the caller scrubs to REDACTION_FAILED, so PII is never leaked).
54+
// Fail loudly; the caller scrubs to REDACTION_FAILED, so PII is never leaked.
55+
const status = statusForMaskError(error)
4156
logger.error('PII batch masking failed', {
4257
error: getErrorMessage(error),
4358
count: texts.length,
59+
status,
4460
})
45-
return NextResponse.json(
46-
{ error: getErrorMessage(error, 'PII masking failed') },
47-
{ status: 500 }
48-
)
61+
return NextResponse.json({ error: getErrorMessage(error, 'PII masking failed') }, { status })
4962
}
5063
})

‎apps/sim/executor/execution/block-executor.test.ts‎

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { loggerMock } from '@sim/testing'
2+
import { maskClientMock, maskClientMockFns } from '@sim/testing/mocks/mask-client.mock'
23
import { permissionCheckMock } from '@sim/testing/mocks/permission-check.mock'
34
import { storageServiceMockFns } from '@sim/testing/mocks/storage-service.mock'
45
import { uploadsMock } from '@sim/testing/mocks/uploads.mock'
@@ -19,6 +20,7 @@ import { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-tr
1920
import { VariableResolver } from '@/executor/variables/resolver'
2021
import type { SerializedBlock, SerializedWorkflow } from '@/serializer/types'
2122

23+
const mockMaskBatch = maskClientMockFns.mockMaskPIIBatchViaHttp
2224
const mockUploadFile = storageServiceMockFns.mockUploadFile
2325
const mockDownloadFile = storageServiceMockFns.mockDownloadFile
2426

@@ -29,17 +31,11 @@ const blockExecutorBaseLogger =
2931
loggerMock.createLogger.mock.results[blockExecutorLoggerCallIndex]?.value
3032
if (!blockExecutorBaseLogger) throw new Error('BlockExecutor logger mock was not initialized')
3133

32-
const { mockMaskBatch } = vi.hoisted(() => ({
33-
mockMaskBatch: vi.fn(),
34-
}))
35-
3634
vi.mock('@/ee/access-control/utils/permission-check', () => permissionCheckMock)
3735

3836
vi.mock('@/lib/uploads', () => uploadsMock)
3937

40-
vi.mock('@/lib/guardrails/mask-client', () => ({
41-
maskPIIBatchViaHttp: mockMaskBatch,
42-
}))
38+
vi.mock('@/lib/guardrails/mask-client', () => maskClientMock)
4339

4440
vi.mock('@/lib/logs/execution/pii-redaction', async (importOriginal) => {
4541
const actual = await importOriginal<typeof import('@/lib/logs/execution/pii-redaction')>()
@@ -139,9 +135,10 @@ describe('BlockExecutor', () => {
139135
clearLargeValueCacheForTests()
140136
mockUploadFile.mockClear()
141137
mockDownloadFile.mockResolvedValue(Buffer.from(JSON.stringify(items)))
142-
mockMaskBatch.mockImplementation(async (texts: string[]) =>
143-
texts.map((text) => text.replaceAll('alice@example.com', '<EMAIL_ADDRESS>'))
144-
)
138+
mockMaskBatch.mockImplementation(async (texts: string[]) => ({
139+
masked: texts.map((text) => text.replaceAll('alice@example.com', '<EMAIL_ADDRESS>')),
140+
scrubbedCount: 0,
141+
}))
145142
const block = createBlock()
146143
const workflow: SerializedWorkflow = {
147144
version: '1',
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
import { describe, expect, it } from 'vitest'
2+
import { filterForDisplay } from '@/lib/core/utils/display-filters'
3+
4+
describe('filterForDisplay', () => {
5+
it('never truncates a string inside a surrogate pair', () => {
6+
const value = `${'x'.repeat(14_999)}😀${'y'.repeat(10)}`
7+
8+
const shown: string = filterForDisplay({ value }).value
9+
10+
expect(shown).toBe(`${'x'.repeat(14_999)}... [truncated 12 chars]`)
11+
})
12+
})

‎apps/sim/lib/core/utils/display-filters.ts‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,17 @@
1+
import { truncateAtCodePoint } from '@sim/utils/string'
12
import { filterUserFileForDisplay, isUserFile } from '@/lib/core/utils/user-file'
23

34
const MAX_STRING_LENGTH = 15000
45
const MAX_DEPTH = 50
56

7+
/**
8+
* Never cuts inside a surrogate pair: a lone half has no UTF-8 encoding, and
9+
* these values flow on to PII redaction, whose service rejects such text.
10+
*/
611
function truncateString(value: string, maxLength = MAX_STRING_LENGTH): string {
7-
if (value.length <= maxLength) {
8-
return value
9-
}
10-
return `${value.substring(0, maxLength)}... [truncated ${value.length - maxLength} chars]`
12+
if (value.length <= maxLength) return value
13+
const kept = truncateAtCodePoint(value, maxLength, '')
14+
return `${kept}... [truncated ${value.length - kept.length} chars]`
1115
}
1216

1317
function filterUserFile(data: any): any {

‎apps/sim/lib/guardrails/mask-client.test.ts‎

Lines changed: 44 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ describe('maskPIIBatchViaHttp', () => {
3535
it('splits by count into multiple requests, preserving global order', async () => {
3636
const texts = Array.from({ length: 5000 }, (_, i) => `t${i}`)
3737

38-
const out = await maskPIIBatchViaHttp(texts, [])
38+
const { masked: out } = await maskPIIBatchViaHttp(texts, [])
3939

4040
expect(out).toHaveLength(5000)
4141
expect(out[0]).toBe('M(t0)')
@@ -54,9 +54,9 @@ describe('maskPIIBatchViaHttp', () => {
5454
it('retries a transient 5xx with backoff and then succeeds', async () => {
5555
fetchMock.mockResolvedValueOnce(new Response('deploying', { status: 503 }))
5656

57-
const out = await maskPIIBatchViaHttp(['a'], [])
57+
const { masked } = await maskPIIBatchViaHttp(['a'], [])
5858

59-
expect(out).toEqual(['M(a)'])
59+
expect(masked).toEqual(['M(a)'])
6060
expect(fetchMock).toHaveBeenCalledTimes(2)
6161
expect(mockSleep).toHaveBeenCalledTimes(1)
6262
})
@@ -69,6 +69,47 @@ describe('maskPIIBatchViaHttp', () => {
6969
expect(mockSleep).toHaveBeenCalledTimes(7)
7070
})
7171

72+
describe('with a failed-chunk placeholder', () => {
73+
const PLACEHOLDER = '[FAILED]'
74+
const CHUNK_SIZE = 2000
75+
const CONCURRENCY = 64
76+
/**
77+
* Fills every concurrent slot with chunks the service fails, then queues two more
78+
* chunks (`'queued'`) that it would mask if they were ever sent.
79+
*/
80+
const failingThenQueued = () => [
81+
...Array.from({ length: CHUNK_SIZE * CONCURRENCY }, () => 'failing'),
82+
...Array.from({ length: CHUNK_SIZE + 1 }, () => 'queued'),
83+
]
84+
const serviceFailingWith = (status: number) =>
85+
fetchMock.mockImplementation(async (_url: string, init: { body: string }) => {
86+
const { texts } = JSON.parse(init.body) as { texts: string[] }
87+
if (texts[0] !== 'queued') return new Response('failed', { status })
88+
return Response.json({ masked: texts.map((t) => `M(${t})`) })
89+
})
90+
91+
it('scrubs queued chunks without sending them once an outage outlives its retries', async () => {
92+
serviceFailingWith(503)
93+
94+
const { masked } = await maskPIIBatchViaHttp(failingThenQueued(), [], undefined, undefined, {
95+
failedChunkPlaceholder: PLACEHOLDER,
96+
})
97+
98+
expect(masked.every((value) => value === PLACEHOLDER)).toBe(true)
99+
})
100+
101+
it('still masks queued chunks after a persistent 500, which is not an outage', async () => {
102+
serviceFailingWith(500)
103+
104+
const { masked } = await maskPIIBatchViaHttp(failingThenQueued(), [], undefined, undefined, {
105+
failedChunkPlaceholder: PLACEHOLDER,
106+
})
107+
108+
expect(masked.slice(0, CHUNK_SIZE * CONCURRENCY).every((v) => v === PLACEHOLDER)).toBe(true)
109+
expect(masked.slice(CHUNK_SIZE * CONCURRENCY).every((v) => v === 'M(queued)')).toBe(true)
110+
})
111+
})
112+
72113
it('does not retry a null 200 body (deterministic, not a transient TypeError)', async () => {
73114
fetchMock.mockResolvedValueOnce(
74115
new Response('null', { status: 200, headers: { 'content-type': 'application/json' } })

0 commit comments

Comments
 (0)