PR Review: feat: add sensitive content scanner for palace drawers

Executive Summary

Affected Areas: Scanner core (scanner.py), MCP ingestion (mcp_server.py), file mining (miner.py), CLI audit (cli.py), tests (test_scanner.py)

Business Impact: Users trust this scanner to flag sensitive data before it's stored. False negatives create a false sense of security. Warning messages that include the matched secret content undermine the scanner's purpose.

Flow Changes: Scanner hooks into three existing ingestion paths — MCP tool_add_drawer, miner process_file, and a new cmd_scan CLI command. All advisory-only (content still stored). MCP path adds a warnings field to the return dict.

Ratings

PR Health

• Has clear description
• References ticket/issue (if applicable) — no issue linked
• Appropriate size (236 additions across 5 files)
• Has relevant tests (124 lines of test coverage)

Guidelines Compliance

No project guidelines loaded.

High Priority Issues

(Must fix before merge)

[Security] #1: Scanner Warnings Leak Partial Secret Content to MCP Callers

Location: mempalace/scanner.py:33-38, mempalace/mcp_server.py:382 | Confidence: HIGH

scan_content() stores the full matched secret in findings["match"]. format_warnings() truncates to 40 chars but still includes a significant portion of the actual secret. In the MCP path, result["warnings"] = format_warnings(findings) sends this back to the MCP caller — an AI agent that may log it, display it, or forward it to an AI provider's API. The CLI cmd_scan also prints up to 50 chars of matched secrets to stdout. A scanner designed to protect against secret leakage should not itself propagate secrets in its output.

The miner.py integration is the only caller that handles this correctly — it prints only pattern names, not matched content.

 def format_warnings(findings):
     """Format findings into a human-readable warning string."""
     if not findings:
         return ""
     lines = ["WARNING: Sensitive content detected:"]
     for f in findings:
-        preview = f["match"][:40] + "..." if len(f["match"]) > 40 else f["match"]
-        lines.append(f"  - {f['pattern_name']}: {preview}")
+        lines.append(f"  - {f['pattern_name']} (chars {f['start']}-{f['end']})")
     return "\n".join(lines)

And in cli.py:cmd_scan, use position instead of content:

             for f in findings:
-                preview = f["match"][:50] + "..." if len(f["match"]) > 50 else f["match"]
-                print(f"    - {f['pattern_name']}: {preview}")
+                print(f"    - {f['pattern_name']} (chars {f['start']}-{f['end']})")

[Security] #2: Incomplete Pattern Coverage Creates False Confidence

Location: mempalace/scanner.py:10-23 | Confidence: HIGH

The scanner covers 4 pattern families (OpenAI/AWS/GitHub keys, Bearer tokens, password assignments, private keys) but misses many common secret formats. Users who see "0 sensitive patterns found" may trust that result when secrets exist. For a security feature, false negatives are worse than missing the feature entirely — they create a false sense of security.

Missing patterns include:

• Stripe: sk_live_, pk_live_, rk_live_ (note: underscore not dash — current sk- won't match)
• Slack: xoxb-, xoxp-, xoxo-, xapp-
• npm: npm_
• Database connection strings: postgres://user:password@, mongodb+srv://user:password@
• Generic secret assignment: SECRET_KEY=, API_SECRET=, CLIENT_SECRET=

 PATTERNS = {
     "api_key": re.compile(
-        r"(?:sk-|AKIA|ghp_|gho_|github_pat_)[A-Za-z0-9_-]{20,}"
+        r"(?:sk-|sk_live_|pk_live_|rk_live_|AKIA|ghp_|gho_|github_pat_|xoxb-|xoxp-|xapp-|npm_)[A-Za-z0-9_-]{20,}"
     ),
     "bearer_token": re.compile(
         r"Bearer\s+[A-Za-z0-9_-]{20,}"
     ),
     "password_assignment": re.compile(
         r"""(?:password|passwd|pwd)["']?\s*[=:]\s*['"][^'"]+['"]""", re.IGNORECASE
     ),
     "private_key": re.compile(
         r"-----BEGIN (?:RSA |EC )?PRIVATE KEY-----"
     ),
+    "connection_string": re.compile(
+        r"(?:postgres|mysql|mongodb\+srv|redis)://[^:\s]+:[^@\s]+@"
+    ),
+    "generic_secret": re.compile(
+        r"""(?:SECRET|PRIVATE)[_-]?(?:KEY|TOKEN)["']?\s*[=:]\s*['"][^'"]{8,}['"]""", re.IGNORECASE
+    ),
 }

Medium Priority Issues

(Should fix, not blocking)

[Bug] #3: scan_content Has No Null/Type Guard

Location: mempalace/scanner.py:27 | Confidence: HIGH

If content is None (possible from ChromaDB col.get() when a document has been deleted or corrupted), pattern.finditer(None) raises TypeError. The CLI cmd_scan iterates over data["documents"] from ChromaDB, which can contain None entries.

 def scan_content(content):
+    if not content:
+        return []
     findings = []
     for name, pattern in PATTERNS.items():

[Architecture] #4: Inconsistent Finding Display Across Three Integration Points

Location: mempalace/scanner.py:42, mempalace/cli.py:255, mempalace/miner.py:435 | Confidence: HIGH

Three callers format scanner findings three different ways:

The miner's approach (names only) is the safest and should be the standard. All callers should use format_warnings() for consistency, and format_warnings() should not include match content (see Issue #1).

 # cli.py — cmd_scan
-            for f in findings:
-                preview = f["match"][:50] + "..." if len(f["match"]) > 50 else f["match"]
-                print(f"    - {f['pattern_name']}: {preview}")
+            print(f"    {format_warnings(findings)}")

[Code Quality] #5: bearer_token Regex Case-Sensitivity Inconsistency

Location: mempalace/scanner.py:14-16 | Confidence: MED

password_assignment uses re.IGNORECASE but bearer_token does not. HTTP Authorization headers use Bearer (capital B), but in conversation logs, code comments, and config files, both bearer and BEARER appear. Since the scanner processes conversation content and code files (not raw HTTP headers), case-insensitive matching is more appropriate.

     "bearer_token": re.compile(
-        r"Bearer\s+[A-Za-z0-9_-]{20,}"
+        r"Bearer\s+[A-Za-z0-9_-]{20,}", re.IGNORECASE
     ),

Low Priority Issues

(Nice to have)

[Performance] #6: CLI Scan Loads All Drawers Into Memory At Once

Location: mempalace/cli.py:241 | Confidence: MED

col.get(limit=10000) loads up to 10,000 documents into memory simultaneously. For large palaces with many drawers, this could cause significant memory pressure. Consider batching with offset/limit pagination.

-    kwargs = {"include": ["documents", "metadatas"], "limit": 10000}
-    ...
-    data = col.get(**kwargs)
+    batch_size = 500
+    offset = 0
+    while True:
+        kwargs = {"include": ["documents", "metadatas"], "limit": batch_size, "offset": offset}
+        if args.wing:
+            kwargs["where"] = {"wing": args.wing}
+        data = col.get(**kwargs)
+        if not data["ids"]:
+            break
+        # ... process batch ...
+        offset += batch_size

Flow Impact Analysis

                    INGESTION PATHS (all advisory-only)
                    ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  MCP Client              CLI Mine              CLI Scan (NEW)
      │                      │                       │
      ▼                      ▼                       ▼
  tool_add_drawer()     process_file()          cmd_scan()
      │                      │                       │
      ├─ scan_content()      ├─ scan_content()       ├─ scan_content()
      │    ↓                 │    ↓                  │    ↓
      │  findings            │  findings             │  findings
      │    ↓                 │    ↓                  │    ↓
      ├─ format_warnings()   ├─ print names only     ├─ print match[:50]
      │    ↓                 │    (stderr) ✓          │    (stdout) ⚠
      │  [LEAKS secret       │                       │  [LEAKS secret
      │   to MCP response]   │                       │   to terminal]
      │    ⚠                 │                       │
      ▼                      ▼                       ▼
  Content STORED         Content STORED          (read-only scan)
  regardless             regardless

Blast Radius: scan_content() is called from 3 locations. Changes to the function signature or return format affect all 3 callers. The format_warnings() function is used by MCP only — CLI has its own inline formatting.

Breaking Changes: None — the scanner is purely additive. MCP callers that don't check for "warnings" in the response are unaffected.

Relationship to RFC: The existing RFC-mempalace-security-hardening identifies "Sensitive data in WAL" (Finding 5) and "Internal error leakage" (Finding 8) as security concerns. This PR's warning leakage (Issue #1) is a new instance of the same class of problem — the scanner that detects secrets also propagates them through its output.

Created by Octocode MCP https://octocode.ai