Harden Universal Guard PII scrubber config normalization

[Copilot]

src/plugins/universal-guard/pii-scrubber.js could throw at runtime when piiScrubber.patterns was missing or malformed (not an array), especially after partial config updates from the Universal Guard UI/API. This change makes scrubber config handling deterministic and safe under invalid input.

• Config hardening

  • Added internal normalization for scrubber config in both constructor and updateConfig.
  • Enforced safe defaults:
    • enabled / logDetections coerced to booleans
    • action constrained to redact | flag (default redact)
    • patterns normalized to an array of non-empty strings

• Failure-mode change

  • Invalid patterns input now degrades to [] (no detection) instead of throwing.
  • Scrubbing path remains non-fatal and returns original message content when no valid patterns are configured.

• Targeted coverage

  • Added focused unit coverage for null/invalid patterns and invalid updateConfig payloads to lock in non-throwing behavior.

// before: could throw when cfg.patterns is null/string
const enabledPatterns = this._cfg.patterns.filter(...)

// after: always safe
this._cfg = {
  enabled: Boolean(next.enabled),
  action: next.action === 'flag' ? 'flag' : 'redact',
  patterns: Array.isArray(next.patterns)
    ? next.patterns.filter(p => typeof p === 'string' && p.trim().length > 0)
    : [],
  logDetections: Boolean(next.logDetections),
}

[Copilot]

Pull request overview

Hardens the Universal Guard PII scrubber against malformed/partial configuration updates by normalizing config inputs so runtime scrubbing stays non-throwing and deterministic.

Changes:

• Normalize PII scrubber config in the constructor and updateConfig() (defaults + coercion + safe patterns handling).
• Ensure invalid/missing patterns degrades to [] instead of throwing during scrubbing.
• Add Jest unit tests covering null/invalid patterns inputs.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/plugins/universal-guard/pii-scrubber.js	Adds _normalizeConfig() and uses it in constructor/update path to prevent runtime throws on malformed config.
tests/universal-guard-pii-scrubber.test.js	Adds focused unit tests for null/invalid patterns and invalid updateConfig() payloads.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

patterns normalization filters out whitespace-only strings using p.trim().length > 0 but keeps the original (untrimmed) values. This means a config like patterns: [' email '] will be accepted but will never match PATTERNS[name] in scrub(), silently disabling detection. Consider trimming (and optionally de-duplicating) pattern names during normalization so the stored config contains canonical keys.

Suggested change

	? next.patterns.filter(p => typeof p === 'string' && p.trim().length > 0)
	? [...new Set(
	next.patterns
	.filter(p => typeof p === 'string')
	.map(p => p.trim())
	.filter(p => p.length > 0)
	)]

[Copilot]

The test name says it "continues detecting" after updateConfig receives invalid patterns, but the assertions expect detections to stop and the message to remain unchanged. Rename the test to match the behavior being validated (e.g., that invalid patterns degrades safely to no detection).

Suggested change

	test('continues detecting after updateConfig receives invalid patterns', () => {
	test('degrades safely to no detection after updateConfig receives invalid patterns', () => {

[llamapreview]

AI Code Review by LlamaPReview

🎯 TL;DR & Recommendation

Recommendation: Approve with suggestions.

This PR hardens the PII scrubber config handling to prevent runtime errors, but introduces silent degradation risks and has gaps in test coverage.

🌟 Strengths

• Centralizes validation logic in _normalizeConfig, improving code maintainability.
• Ensures non-throwing behavior for invalid inputs, enhancing runtime stability.

Priority	File	Category	Impact Summary (≤12 words)	Anchors
P2	src/plugins/.../pii-scrubber.js	Architecture	Silently discards unknown config properties, breaking extensions.	-
P2	src/plugins/.../pii-scrubber.js	Maintainability	Silent degradation of patterns disables PII protection without warning.	-
P2	src/plugins/.../pii-scrubber.js	Maintainability	Action default lacks feedback for invalid values.	-
P2	tests/.../pii-scrubber.test.js	Testing	Missing direct tests for updateConfig with invalid input.	-
P2	src/plugins/.../pii-scrubber.js	Architecture	Constructor resets stats, but updateConfig does not (observation).	-

🔍 Notable Themes

• Silent Configuration Failures: Multiple findings highlight risks where invalid configs are silently normalized without warnings, potentially disabling security features.
• Test Coverage Gaps: The test suite could be strengthened with more direct assertions for edge cases in updateConfig.

---

💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.

[llamapreview]

P2 | Confidence: High

The new _normalizeConfig method introduces a robust, defensive pattern for configuration handling, which is architecturally sound. However, it creates a subtle but significant behavioral shift: the method now silently discards any configuration properties not explicitly listed (enabled, action, patterns, logDetections). While the PR description focuses on preventing runtime errors from malformed patterns, this change also means that any future extension of the configuration schema (e.g., adding a severity level or a custom replacementText) would be silently ignored by the scrubber after this change. The method spreads next but then overwrites only the four known keys, effectively filtering out any extras. This is a breaking change for any existing or future code that might attach metadata to the config object expecting it to be preserved through updateConfig. The safer pattern is to merge the normalized values into the provided config, preserving unknown properties, or to explicitly validate and reject configs with unknown properties.

Code Suggestion:

_normalizeConfig(cfg) {
    const next = (cfg && typeof cfg === 'object') ? { ...cfg } : {};
    // Normalize only the known properties, preserving any others.
    return {
        ...next,
        enabled:       Boolean(next.enabled),
        action:        next.action === 'flag' ? 'flag' : 'redact',
        patterns:      Array.isArray(next.patterns)
            ? next.patterns.filter(p => typeof p === 'string' && p.trim().length > 0)
            : [],
        logDetections: Boolean(next.logDetections),
    };
}

[llamapreview]

P2 | Confidence: High

The normalization logic for patterns silently degrades invalid input (non-array, or array containing non-strings/empty strings) to an empty array []. This aligns with the PR's goal of making the scrubber non-fatal. However, this creates a silent failure mode where a configuration error (e.g., a typo like paterns) or a UI/API bug that sends a malformed payload results in the PII scrubber being completely disabled without any warning or log. For a security-critical component, silently turning off protection is risky. The scrubber should at least log a warning when patterns is normalized to an empty array, especially if the scrubber is enabled. The logDetections flag is for logging actual PII findings, not configuration errors.

Code Suggestion:

patterns:      Array.isArray(next.patterns)
    ? next.patterns.filter(p => typeof p === 'string' && p.trim().length > 0)
    : (() => {
        if (next.enabled && next.patterns !== undefined) {
            console.warn(`PiiScrubber: Invalid 'patterns' config (${typeof next.patterns}). Defaulting to empty list.`);
        }
        return [];
      })(),

[llamapreview]

P2 | Confidence: High

The logic for the action property uses a strict equality check (=== 'flag') and defaults everything else to 'redact'. This is simple and safe. However, it lacks case-insensitivity and does not provide feedback for invalid values. If a UI sends 'FLAG' or 'log', it will silently be treated as 'redact'. While not a security issue (defaulting to the more conservative redact action), it is a potential source of confusion for operators. Consider adding a warning for unrecognized actions or normalizing the input (e.g., next.action?.toLowerCase()).

Code Suggestion:

const normalizedAction = String(next.action).toLowerCase();
action: normalizedAction === 'flag' ? 'flag' : 'redact',
// Optional: Log if original value was not 'redact' or 'flag'
if (normalizedAction !== 'redact' && normalizedAction !== 'flag') {
    console.warn(`PiiScrubber: Unrecognized action "${next.action}". Defaulting to "redact".`);
}

[llamapreview]

P2 | Confidence: High

The new unit tests are valuable for locking in the non-throwing behavior. However, they do not test the updateConfig method in isolation. The test 'continues detecting after updateConfig receives invalid patterns' validates the outcome of a subsequent scrub call but does not assert that the updateConfig call itself succeeds without throwing. While the implementation makes this likely, a direct test that scrubber.updateConfig(invalidPayload) does not throw would be more precise and would catch regressions where _normalizeConfig might be removed or altered. Furthermore, the test suite lacks coverage for updateConfig with a null or undefined argument, which the _normalizeConfig method is designed to handle (it defaults to an empty object).

Code Suggestion:

test('updateConfig does not throw on invalid input', () => {
    const scrubber = new PiiScrubber({ enabled: true, patterns: [] });
    expect(() => scrubber.updateConfig({ patterns: 'not-an-array' })).not.toThrow();
    expect(() => scrubber.updateConfig(null)).not.toThrow();
    expect(() => scrubber.updateConfig(undefined)).not.toThrow();
});

[llamapreview]

P2 | Confidence: High

The constructor now delegates to _normalizeConfig. This is a positive change that centralizes validation logic. However, there is a potential state inconsistency risk on re-initialization. The _detections and _byType counters are reset to zero in the constructor, but they are not reset when updateConfig is called. This is correct behavior, as the detection statistics are about the scrubber's runtime activity, not its configuration. The PR does not change this, but it's worth noting that if the scrubber were ever re-instantiated with a new config (instead of using updateConfig), the stats would reset. This is fine, but it highlights that updateConfig is for dynamic reconfiguration, while creating a new instance is for a fresh state. The architecture is sound.