Skip to content

Use config.default_mask_strategy instead of hardcoded values#3820

Draft
smrutisahoo10 wants to merge 2 commits intomainfrom
bugfix/3724-pii-filter-mask-strategy
Draft

Use config.default_mask_strategy instead of hardcoded values#3820
smrutisahoo10 wants to merge 2 commits intomainfrom
bugfix/3724-pii-filter-mask-strategy

Conversation

@smrutisahoo10
Copy link
Copy Markdown
Collaborator

🔗 Related Issue

Closes #3724


📝 Summary

Fixes bug where default_mask_strategy configuration was ignored for built-in PII types in PIIFilterPlugin. The plugin was using hardcoded MaskingStrategy.PARTIAL values instead of respecting the configured strategy.

Changes:

  • Changed 20+ hardcoded mask_strategy=MaskingStrategy.PARTIAL to mask_strategy=self.config.default_mask_strategy in _compile_patterns() method

Impact:

  • Users can now control PII masking behavior via configuration (redact, partial, hash, tokenize, remove) instead of being forced to use hardcoded partial masking.

🏷️ Type of Change

  • Bug fix
  • Feature / Enhancement
  • Documentation
  • Refactor
  • Chore (deps, CI, tooling)
  • Other (describe below)

🧪 Verification

Check Command Status
Lint suite make lint Pass
Unit tests make test Pass
Coverage ≥ 80% make coverage Pass

✅ Checklist

  • Code formatted (make black isort pre-commit)
  • Tests added/updated for changes
  • Documentation updated (if applicable)
  • No secrets or credentials committed

📓 Notes (optional)

Screenshots, design decisions, or additional context.

@smrutisahoo10 smrutisahoo10 force-pushed the bugfix/3724-pii-filter-mask-strategy branch from d06aa77 to c1c70fa Compare March 24, 2026 11:21
@smrutisahoo10 smrutisahoo10 added bug Something isn't working plugins labels Mar 25, 2026
@crivetimihai crivetimihai added the SHOULD P2: Important but not vital; high-value items that are not crucial for the immediate release label Mar 29, 2026
@crivetimihai crivetimihai added this to the Release 1.1.0 milestone Mar 29, 2026
@crivetimihai
Copy link
Copy Markdown
Member

Thanks @smrutisahoo10 — good cleanup, replacing hardcoded mask strategy values with config-driven ones.

smrutisahoo10 and others added 2 commits April 5, 2026 17:47
Signed-off-by: Smruti Sahoo <talktodaisy19@gmail.com>
Adds regression tests verifying that the Python PIIDetector now respects
the configured default_mask_strategy for all built-in PII types. Covers
REDACT, PARTIAL, HASH, TOKENIZE, and REMOVE strategies, plus a check
that custom patterns keep their explicit strategy.

Also adds detect-secrets pragma to pre-existing fake AWS key test fixture.

Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
@crivetimihai crivetimihai force-pushed the bugfix/3724-pii-filter-mask-strategy branch from c1c70fa to 3a24966 Compare April 5, 2026 17:08
@crivetimihai
Copy link
Copy Markdown
Member

Thanks @smrutisahoo10 — good cleanup, replacing hardcoded mask strategy values with config-driven ones.

Rebased onto main and resolved merge conflicts (the AWS_KEY/API_KEY PII types were removed upstream, so those sections were dropped). Added differential regression tests covering all five masking strategies and a detect-secrets pragma fix for the test fixture.

A few notes from the review:

  • Correctness: The fix is sound. The hardcoded mask_strategy values on each PIIPattern made the default_mask_strategy config a no-op for built-in types, since mask() always found an explicit value and the fallback was dead code. This PR makes the config work as documented.

  • Security: The default MaskingStrategy.REDACT is preserved, so out-of-the-box behavior is actually more secure now — types like SSN, email, and phone that previously defaulted to PARTIAL (leaking partial data) now default to full redaction.

  • Rust parity note: The Rust implementation (pii_filter_rust) still has its own hardcoded per-type strategies that override the global default. The existing Rust-specific tests verify this is intentional. If Rust should also honor default_mask_strategy, that would be a separate follow-up issue.

Changes I pushed on top of the original commit:

  • Resolved rebase conflict (dropped removed AWS_KEY/API_KEY sections)
  • Added TestPythonDefaultMaskStrategyHonored class with 11 regression tests
  • Added PIIPattern to test imports and # pragma: allowlist secret on a pre-existing test fixture line

Copy link
Copy Markdown
Member

@crivetimihai crivetimihai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good — fix is correct, default behavior is more secure, and differential tests are in place. Approving.

@jonpspri jonpspri marked this pull request as draft April 9, 2026 21:30
@jonpspri
Copy link
Copy Markdown
Collaborator

jonpspri commented Apr 9, 2026

@lucarlig Can we please check this change against the Rust plugin? Leaving in draft pending resolution.

@lucarlig
Copy link
Copy Markdown
Collaborator

lucarlig commented Apr 10, 2026

@jonpspri checked. The Rust-backed detector was still hardcoding built-in mask strategies, so default_mask_strategy was still ignored when Rust is active.

The correct follow-up lives in IBM/cpex-plugins:

I also closed the mistaken mcp-context-forge follow-up issue/PR that I opened first.

cpex-plugins#23 updates the Rust pattern handling to honor the global default while keeping explicit custom-pattern overrides, and it includes verified Rust and plugin-level test coverage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working plugins SHOULD P2: Important but not vital; high-value items that are not crucial for the immediate release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG][PLUGINS]: PIIFilterPlugin ignores default_mask_strategy config — built-in PII types always use hardcoded masking strategies

4 participants