fix(pii): redact names and addresses, unpin analyzer language, harden tool denylist - #39
fix(pii): redact names and addresses, unpin analyzer language, harden tool denylist#39Shaivpidadi wants to merge 4 commits into
Conversation
… tool denylist Adds bench/, a 438-item span-annotated corpus across en/es/fr/de/zh that measures the deployed redaction path rather than the detector in isolation, and fixes the four defects it surfaced. Overall leakage 51.3% -> 8.2%. High-sensitivity entity leakage 89.1% -> 10.6%. PERSON and LOCATION 100% -> ~11.6%. Over-redaction stays at 0% and p50 latency is unchanged (2.46 -> 2.48 ms). Entity allowlist: the analyzer was asked only for the 16 keys of ANONYMIZE_OPERATORS, which contain no PERSON, LOCATION or NRP. spaCy detected names at score 0.85 and the pipeline discarded them, so names and addresses were never redacted in any language. Replaced with an explicit DETECT_ENTITIES list. ORGANIZATION and DATE_TIME stay out: including them modified 61.5% of PII-free control text, and a layer that redacts everything gets switched off. Language pin: analyze() was called with language="en" at all three sites and the engine was built with supported_languages=["en"], so the es/fr/de/zh models the Dockerfile installs were unreachable. Adds PRESIDIO_LANGUAGES, PRESIDIO_LANGUAGE_MODE, a per-request hint via tool_config.metadata.language, and a shared analyze_text() so the three sites cannot drift apart again. Per-language recognizer gaps: Presidio ships CreditCardRecognizer for en/es only and its default phone regions exclude ES/FR/CN, so a hinted fr/de/zh request leaked 61.5% of card numbers. Both now registered per language. False-positive filter: is_false_positive() received the whole input text rather than the matched span, so its rules almost never fired. The US_SSN recognizer also passed deny_list=["password", "key", ...], but Presidio's deny_list is a list of terms to detect, not suppress, so the literal word "key" matched as an SSN. Removed; suppression stays in is_false_positive(). Denylist matching: `tool in deny_tools` was an exact string test that Python.Exec, python_exec, exec.python and agent.python.exec.v2 all walked past. Replaced with is_denied_tool() (normalisation, component-set match, containment, namespace.* wildcards) and extended the defaults with subprocess.run, os.system and the other direct execution primitives. Tests: +69. Suite 258 -> 327 passing, coverage 75.1% -> 76.8%.
Formatting only, no behaviour change. Fixes the black failure this branch introduced for app/policies.py and the two new test modules.
All three of these fail on dev at 351d59b independently of this branch — verified by checking out dev and running the CI commands. They are in this PR only because a red CI blocks the merge; the commit is self-contained and can be dropped or split out if you would rather land it separately. - black + isort: app/api.py, app/policy_source.py, app/storage.py, tests/test_policy_invalidate_endpoint.py, tests/test_policy_source.py. Mechanical reformatting, no behaviour change. - mypy: app/policy_source.py:75 "Need type annotation for raw_defaults". Annotated Any rather than Dict[str, Any] because row.defaults is a SQLAlchemy Column[Any] at type-check time and a concrete dict annotation trades the missing-annotation error for an incompatible-assignment one. - Load Test: the job sets DEBUG=false, so Settings runs its production validators, but PII_TOKEN_SALT was "ci-load-salt" (13 chars) against a 32-character minimum. The job has been failing at startup ever since that validator landed. Padded all three throwaway CI values past the minimum. Verified locally against the exact CI commands: black --check, isort --check-only, flake8 with the workflow's flags, and mypy app/ all pass.
|
CI was red on this branch. Two commits pushed; worth flagging the split before review.
They are in this PR only because red CI blocks the merge. One judgment call inside it: the mypy fix annotates Verified locally against the exact workflow commands — |
Padding the CI secrets got the service past startup, which revealed the next
failure underneath: k6 reported 96.23% failed requests, 113 of 3000 succeeding.
The limiter keys on hash_api_key(raw_key) and the load test sends every request
with the same LOAD_TEST_API_KEY — LOAD_USER_POOL_SIZE=120 only varies the
user_id inside the payload. Against the 100/min per-key default, the profile
(100 iters/s for 30s) can only ever get ~100 requests through before the rest
are 429ed. Simulating specs_for_request over the k6 profile:
key/min=100 org/min=1000 -> allowed 100/3000
key/min=20000 org/min=20000 -> allowed 3000/3000
The 113 observed in CI is the 100 from the first window plus the handful that
landed after the 30s run crossed a minute boundary.
Raises the four rate-limit dimensions for this job only, so the run exercises
the precheck hot path rather than the limiter in front of it. Note this is a
judgment call about what the job is for: if the intent was to assert limiter
behaviour under load, the fix is the opposite one — give k6 a pool of API keys
and assert the 429s. Happy to swap it.
|
CI is fully green now — Format & Lint, Type Check, Tests, Load Test, Smoke, label.
The limiter keys on The 113 seen in CI is the 100 from the first window plus the few that landed after the 30s run crossed a minute boundary. Two things I'd flag rather than leave you to find: 1. This was a judgment call. I raised the limits so the job measures the precheck hot path instead of the limiter in front of it. If the intent was to assert limiter behaviour under load, the correct fix is the opposite — give k6 a pool of API keys and assert the 429s. Say the word and I'll swap it. 2. The job is green but not measuring what it says. Now that requests actually reach the service: It only sustained 52 iters/s against a target of 100, dropping 1232 iterations, and p95 was 151ms against the ~130ms p95 quoted elsewhere for precheck. Both were invisible before, because everything used to 429 in ~1.3ms. Neither blocks this PR — thresholds pass — but the arrival-rate config and that p95 are worth a look before anyone quotes load numbers externally. Ready for review. The detection work is |
1 participant
What
Adds
bench/— a 438-item, span-annotated PII corpus across en/es/fr/de/zh that measures the deployed redaction path, not the detector in isolation — and fixes the four defects it surfaced.Published detector benchmarks score Presidio as a standalone model on text records. This measures what precheck actually returns, with the entity allowlist, language configuration and false-positive filter in the path, because that is what decides whether a customer's SSN leaves the building.
Results
Five languages,
PRESIDIO_LANGUAGE_MODE=hint, 438 items / 630 annotated spans:regex(fallback)legacy(what ships today)fixedPERSON and LOCATION: 100% → 11.6% / 11.7%. Utility and latency are unchanged.
The four defects
1. Entity allowlist dropped names and addresses. The analyzer was asked only for the 16 keys of
ANONYMIZE_OPERATORS, which contain noPERSON,LOCATIONorNRP. spaCy detected names at score 0.85 and the pipeline discarded them:A "redact PII" policy left the patient's name and address fully intact — in English, not just in the languages we thought were the problem. Replaced with an explicit
DETECT_ENTITIES.ORGANIZATIONandDATE_TIMEstay excluded: including them modified 61.5% of PII-free control text ("Acme Corporation", "quarterly", "UTC"), and a layer that redacts everything gets switched off.2. Analyzer pinned to English.
analyze(..., language="en")at all three call sites andsupported_languages=["en"]on the engine, so the es/fr/de/zh models the Dockerfile installs were unreachable. AddsPRESIDIO_LANGUAGES,PRESIDIO_LANGUAGE_MODE, a per-request hint viatool_config.metadata.language, and a sharedanalyze_text()so the three sites cannot drift apart again.3. Per-language recognizer gaps. Presidio ships
CreditCardRecognizerfor en/es only, and its default phone regions exclude ES/FR/CN — a hinted fr/de/zh request leaked 61.5% of card numbers. Both now registered per configured language.4. False-positive filter was inert, and the SSN denylist was inverted.
is_false_positive()received the whole input text instead of the matched span, so its rules almost never fired. Separately, theUS_SSNrecognizer passeddeny_list=["password", "key", ...]— but Presidio'sdeny_listis a list of terms to detect, so the literal word "key" matched as an SSN.Bonus — denylist matching.
if tool in deny_toolswas an exact string test.Python.Exec,python_exec,exec.pythonandagent.python.exec.v2all walked past it. Replaced withis_denied_tool()(normalisation, component-set match, containment,namespace.*wildcards); defaults extended withsubprocess.run,os.systemand the other direct execution primitives.Known residual gaps
Measured, documented in
bench/README.md, not fixed:j dot smith at acme dot com)US_SSN14.3%,IBAN8.3%The first two match the published failure profile for pattern-plus-NER detectors (REDACT, arXiv:2606.19881, reports 0.07 and 0.02 recall for Presidio on exactly these). Closing them needs a different class of detector, not a better pattern.
Behaviour changes for operators
DEFAULT_DENY_TOOLSgained six entries. A tool literally namedos.systemis now denied.PRESIDIO_LANGUAGES=en,PRESIDIO_LANGUAGE_MODE=hint.Tests
+69 (
test_deny_tools_matching.py,test_multilingual_pii.py). Suite 258 → 327 passing, coverage 75.1% → 76.8%. Multilingual tests carry the existingmultilingualmarker and skip when the models are absent.Note
The first push was blocked by GitHub push protection: the corpus used Stripe's documentation sample key and the
sk_live_prefix trips secret scanning. Replaced with a test-prefixed synthetic value and the commit was amended, so the literal is not in history. The control worked as intended.