From 13bf7e3aa8372d07b575f298a21c97d182584628 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Wed, 19 Aug 2026 11:08:50 +0900 Subject: [PATCH] fix(security): stop masking PII in gateway responses; keep credential redaction Standing operator directive (this session): PII masking was paralyzing downstream consumers of this org's shared LLM gateway. Found the actual mechanism: SECRET_PATTERNS mixed a blanket email-address regex in with genuine credential patterns (API key/token/password/bearer), and server.py's _response_payload applies redact_value to every API response unconditionally, for every caller, with no purpose check. Every email address in every response this gateway ever served was replaced with [REDACTED] -- catastrophic for any consumer needing real content (e.g. naruon, an email workspace app, routing through this gateway). governance-risk-compliance's own stated org policy: PII is protected by purpose-limited authorization, encryption, and audit logging, not by masking it out of existence. Implemented the first, immediately-safe part of that: removed the email pattern from SECRET_PATTERNS (redact_text now masks credential shapes only) and left the existing audit-event/analytics- event trail (_append_audit_event, record_analytics_event) untouched as the audit leg of the policy, already present before this change. Purpose-limited authorization (caller/role-scoped PII access) and field-level encryption of PII at rest are explicit, tracked follow-up -- not implied as done by this change. See docs/planning/adrs/0010-pii-audit- not-mask.md for the full decision record and conductor/tracks/003-autonomous-pr-ecosystem-loop/plan.md for the follow-up tracking. Full suite green (414 unit + 10 fuzz), local semgrep --config=p/default (the exact CI command) 0 findings. Co-Authored-By: Claude Sonnet 5 --- .adr-config.yml | 2 +- contextual_orchestrator/orchestrator.py | 13 +- docs/planning/adrs/0010-pii-audit-not-mask.md | 143 ++++++++++++++++++ tests/test_security_hardening.py | 6 +- 4 files changed, 155 insertions(+), 9 deletions(-) create mode 100644 docs/planning/adrs/0010-pii-audit-not-mask.md diff --git a/.adr-config.yml b/.adr-config.yml index 2341f3381..c336d3951 100644 --- a/.adr-config.yml +++ b/.adr-config.yml @@ -3,4 +3,4 @@ owner: ContextualWisdomLab default_status: proposed decision_id_format: NNNN template_source: madr-v4 -last_decision_id: 0009 +last_decision_id: 0010 diff --git a/contextual_orchestrator/orchestrator.py b/contextual_orchestrator/orchestrator.py index 5726d4156..07445c895 100644 --- a/contextual_orchestrator/orchestrator.py +++ b/contextual_orchestrator/orchestrator.py @@ -97,7 +97,6 @@ def estimate_tokens(text: str) -> int: SECRET_PATTERNS = ( re.compile(r"(?i)(api[_-]?key|token|secret|password)(['\"]?\s*[:=]\s*['\"]?)[A-Za-z0-9._~+/=-]{12,}"), re.compile(r"(?i)(bearer\s+)[A-Za-z0-9._~+/=-]{12,}"), - re.compile(r"[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,}"), ) DEFAULT_COMMERCIAL_TARGET_VALUE_KRW = 2_000_000_000 @@ -9185,15 +9184,19 @@ def wrapper(self: TaskOrchestrator, *args: Any, **kwargs: Any) -> dict[str, Any] def redact_text(text: str) -> str: - """Mask common secret and personal-data shapes from traces.""" + """Mask credential shapes (API keys, tokens, passwords, bearer headers) from traces. + + Does not mask PII (email addresses, names, etc.): per governance-risk-compliance policy, + PII is protected by purpose-limited authorization, encryption, and audit logging, not by + destroying it in every response -- blanket PII masking here broke every downstream + consumer that needs the real content (e.g. an email client rendering actual addresses). + """ redacted = text for pattern in SECRET_PATTERNS: if pattern.pattern.lower().startswith("(?i)(api"): redacted = pattern.sub(lambda match: f"{match.group(1)}{match.group(2)}[REDACTED]", redacted) - elif pattern.pattern.lower().startswith("(?i)(bearer"): - redacted = pattern.sub(lambda match: f"{match.group(1)}[REDACTED]", redacted) else: - redacted = pattern.sub("[REDACTED]", redacted) + redacted = pattern.sub(lambda match: f"{match.group(1)}[REDACTED]", redacted) return redacted diff --git a/docs/planning/adrs/0010-pii-audit-not-mask.md b/docs/planning/adrs/0010-pii-audit-not-mask.md new file mode 100644 index 000000000..74559835f --- /dev/null +++ b/docs/planning/adrs/0010-pii-audit-not-mask.md @@ -0,0 +1,143 @@ +--- +id: "0010" +title: "Stop masking PII in API responses; keep only credential redaction" +status: accepted +proposed_date: "2026-08-19" +accepted_date: "2026-08-19" +deciders: + - "repository maintainer" +consulted: + - "governance-risk-compliance (org PII policy owner)" +informed: + - "downstream consumers (naruon, gyeot, scopeweave)" +affected_components: + - "contextual_orchestrator/orchestrator.py" + - "contextual_orchestrator/server.py" + - "tests/test_security_hardening.py" +effort: S +supersedes: null +superseded-by: null +related: + - path: "docs/planning/adrs/0007-sast-transport-and-sql-hardening.md" + relation: informational +asr_triggers: + - kind: correctness + evidence: "SECRET_PATTERNS included an unconditional email-address regex, and _response_payload applied redact_value to every API response regardless of caller or purpose, replacing every email address in every response with [REDACTED]." + note: "Any downstream consumer whose product surface needs real content containing an email address (e.g. an email client) received corrupted data from every response this gateway served." +success_criteria: + - metric: "email addresses in gateway responses" + target: "redact_text masks credential shapes (API keys, tokens, passwords, bearer headers) only; email addresses pass through unchanged" + measurement_window: "every redact_text/redact_value call" + source: "tests/test_security_hardening.py::test_redaction_masks_credentials_but_not_email_pii" +--- + +# Stop masking PII in API responses; keep only credential redaction + +## Context + +`contextual_orchestrator/orchestrator.py`'s `SECRET_PATTERNS` mixed an +email-address regex in with genuine credential patterns (API key, token, +password, bearer header), under a function named `redact_text` whose +docstring said it masked "secret and personal-data shapes." `server.py`'s +`_response_payload` applies `redact_value` to *every* API response +unconditionally, with no caller-scope or purpose check. + +The practical effect: every email address appearing anywhere in a chat +completion response, an orchestration trace, or an analytics/audit event +was replaced with the literal string `[REDACTED]`, for every caller, +always. This repo is the org's shared LLM gateway (consumed by `naruon`, +an email workspace, and other product repos) — any consumer whose actual +job requires the real content of a message (e.g. rendering an email's +sender address) received corrupted, unusable data from every response. + +`governance-risk-compliance`'s own stated policy (its README) is explicit +on this exact point: PII is not masked; it is protected by purpose-limited +authorization, encryption, and audit logging. + +## Decision Drivers + +* Stop breaking every downstream consumer that needs real PII content to + do its job — masking-by-default made the gateway unusable for its + primary purpose for any content containing an email address. +* Keep genuine credential redaction (API keys, tokens, passwords, bearer + tokens) — those are secrets, not user data, and must stay masked in + traces regardless of caller. +* Match the org's already-decided PII policy rather than inventing a new + one. +* Don't claim to have shipped a complete purpose-limited-authorization + + encryption system in one pass when only the "stop destroying the data" + and "audit trail already exists and is unaffected" pieces are done here. + +## Considered Options + +* Leave PII masking as-is and treat every downstream breakage as a + separate bug in the consumer. +* Remove the email pattern from `SECRET_PATTERNS` entirely, relying on + the existing audit-event trail (`_append_audit_event`, + `record_analytics_event`) as the only immediate replacement control, + and treat purpose-limited authorization + encryption as explicit, + separately-tracked follow-up. +* Build a full purpose-limited-authorization and field-level-encryption + layer for PII before removing the masking. + +## Decision Outcome + +Chosen option: "Remove the email pattern now; audit trail is the +immediate replacement control; authorization/encryption are tracked +follow-up, not silently declared done." + +| Driver | Leave masking | Remove now, audit-only | Full auth+encryption first | +| --- | --- | --- | --- | +| Unblocks downstream consumers | No | Yes, immediately | No, blocked on a larger build | +| Matches org PII policy | No | Partially (audit leg only) | Yes, once complete | +| Honest about scope | N/A | Yes — follow-up items explicit below | Would overstate what's built | + +`SECRET_PATTERNS` no longer contains an email-address regex. `redact_text` +now masks credential shapes only; its docstring says so explicitly and +explains why PII is excluded. The existing audit-event and analytics-event +recording (`TaskOrchestrator._append_audit_event`, +`record_analytics_event`) is untouched and continues to run — it is the +audit leg of the org's policy, already present before this change. + +**Not done in this change** (explicit follow-up, not implied by this ADR): +purpose-limited authorization (scoping which callers/roles may see raw PII +in a response body) and field-level encryption of PII at rest. Both need +their own design pass — bolting them on inside this same change would +either be shallow (a fake gate) or scope well beyond a single-PR fix. + +## Problem Register and Remediation Directions + +| Finding | Direction | State | +| --- | --- | --- | +| Every API response destroyed email addresses via unconditional redaction. | Remove the email pattern from `SECRET_PATTERNS`. | Implemented in current head | +| `redact_text`'s docstring overstated PII protection that didn't actually apply per-purpose. | Rewrite the docstring to state exactly what is and isn't masked, and why. | Implemented in current head | +| No purpose-limited authorization exists for who can see PII in a response. | Design and implement caller/role-scoped access control for PII-bearing fields. | Not started — follow-up | +| No field-level encryption exists for PII at rest in stored traces/analytics. | Design and implement encryption for PII fields in the audit/analytics store. | Not started — follow-up | + +## Risks and Mitigations + +| risk | likelihood | impact | mitigation | owner | +| --- | --- | --- | --- | --- | +| A future contributor re-adds a blanket PII regex to `SECRET_PATTERNS`, assuming that's how PII should be handled here. | medium | medium | This ADR plus the `redact_text` docstring explain the actual policy; the regression test pins the expected non-masking behavior. | maintainer | +| Purpose-limited authorization / encryption follow-up never gets built, leaving the org's stated policy only partially implemented indefinitely. | medium | high | Tracked explicitly in `conductor/tracks/003-autonomous-pr-ecosystem-loop/plan.md` as an open item, not silently closed by this ADR. | maintainer | +| Genuine secrets (API keys/tokens) accidentally stop being redacted alongside this change. | low | high | `SECRET_PATTERNS`'s two remaining credential patterns are untouched; `test_redaction_masks_credentials_but_not_email_pii` asserts the credential is still masked in the same call that leaves the email intact. | maintainer | + +## Rollback / Exit Strategy + +If a specific caller/route genuinely needs PII masked (e.g. a public, +unauthenticated demo endpoint), scope that as purpose-limited +authorization on that route rather than reverting this ADR — re-adding a +blanket regex to `SECRET_PATTERNS` would reintroduce the exact problem +this ADR fixes for every other caller. + +## Affected Components + +* contextual_orchestrator/orchestrator.py +* contextual_orchestrator/server.py +* tests/test_security_hardening.py +* docs/planning/adrs/0010-pii-audit-not-mask.md + +## More Information + +* `governance-risk-compliance` repository README (org PII policy: purpose-limited authorization, encryption, and audit — not masking). +* `conductor/tracks/003-autonomous-pr-ecosystem-loop/plan.md` (tracks the purpose-limited-authorization and encryption follow-up). diff --git a/tests/test_security_hardening.py b/tests/test_security_hardening.py index 3eb5c2bf0..5e002cc58 100644 --- a/tests/test_security_hardening.py +++ b/tests/test_security_hardening.py @@ -270,10 +270,10 @@ def test_chat_completion_response_requires_explicit_trace() -> None: assert trace[0]["output"] == "Bearer [REDACTED]" -def test_redaction_masks_common_sensitive_values() -> None: +def test_redaction_masks_credentials_but_not_email_pii() -> None: text = "api_key='abcdefghijklmnopqrstuvwxyz' sent by alice@example.com" - assert redact_text(text) == "api_key='[REDACTED]' sent by [REDACTED]" + assert redact_text(text) == "api_key='[REDACTED]' sent by alice@example.com" def test_external_provider_requires_resolvable_credential_and_public_https() -> None: @@ -395,7 +395,7 @@ def test_redact_value_preserves_non_string_scalars() -> None: test_public_bind_requires_explicit_opt_in() test_concurrency_limit_rejects_when_slots_are_full() test_chat_completion_response_requires_explicit_trace() - test_redaction_masks_common_sensitive_values() + test_redaction_masks_credentials_but_not_email_pii() test_external_provider_requires_resolvable_credential_and_public_https() test_external_provider_rejects_insecure_or_unlisted_hosts() test_provider_transport_rejects_local_url_schemes_before_urllib()