Skip to content

Fix multi-tenant security: RLS-safe cleanup, scoped clear, stronger argon2#93

Merged
cmeans merged 1 commit into
mainfrom
feature/security-fixes
Mar 29, 2026
Merged

Fix multi-tenant security: RLS-safe cleanup, scoped clear, stronger argon2#93
cmeans merged 1 commit into
mainfrom
feature/security-fixes

Conversation

@cmeans
Copy link
Copy Markdown
Owner

@cmeans cmeans commented Mar 29, 2026

Summary

  • cleanup_expired RLS-safe: background cleanup now uses SET LOCAL row_security = off in a transaction so expired entries are cleaned across all owners regardless of RLS enforcement
  • clear() scoped to owner: requires owner_id parameter, deletes only that owner's data via WHERE owner_id = %s + RLS context — prevents cross-tenant data loss
  • Argon2 time_cost bumped to 3: stronger password hashing for new/changed passwords (existing hashes remain valid — argon2 stores params in the hash string)

QA

Prerequisites

  • pip install -e ".[dev]"
  • Deploy to test instance on alternate port (AWARENESS_PORT=8421)

Manual tests (via MCP tools)

    • Expired entry cleanup works across owners
      Create entries with short TTL for two different owners, wait for expiry + cleanup trigger, verify both are gone:
    add_context(source="test", tags=["qa"], description="owner1 expiring", ttl_sec=5)
    # Switch to second owner (different JWT)
    add_context(source="test", tags=["qa"], description="owner2 expiring", ttl_sec=5)
    # Wait ~15s for cleanup to trigger
    get_knowledge(tags=["qa"])  # as owner1 — should be empty
    get_knowledge(tags=["qa"])  # as owner2 — should be empty
    

    Expected: both expired entries cleaned regardless of which owner triggered cleanup

    • Argon2 time_cost=3 on new passwords
    mcp-awareness-user add --user-id qa-test --name "QA Test"
    # Enter password when prompted
    mcp-awareness-user export --user-id qa-test
    

    Expected: exported hash contains t=3 parameter (e.g., $argon2id$v=19$m=65536,t=3,p=4$...)

    • Existing passwords still verify after time_cost change
    # Generate a token with an existing user's credentials
    mcp-awareness-token generate --user-id <existing-user> --secret <jwt-secret>
    

    Expected: token generates successfully (password verification uses hash-embedded params, not current PasswordHasher config)

🤖 Generated with Claude Code

…rgon2

- _do_cleanup() uses SET LOCAL row_security = off for cross-owner expired entry cleanup
- clear() now requires owner_id, scoped DELETEs with RLS context
- Argon2 time_cost bumped from 2 to 3 for stronger password hashing
- New tests: cleanup cross-owner deletion, clear owner isolation
- Externalized SQL for clear operations and disable_row_security

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@cmeans cmeans added the Dev Active Developer is actively working on this PR; QA should not start label Mar 29, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 29, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@cmeans cmeans removed the Dev Active Developer is actively working on this PR; QA should not start label Mar 29, 2026
@github-actions github-actions Bot added the Ready for QA Dev work complete — QA can begin review label Mar 29, 2026
@cmeans cmeans added the QA Active QA is actively reviewing; Dev should not push changes label Mar 29, 2026
@github-actions github-actions Bot removed the Ready for QA Dev work complete — QA can begin review label Mar 29, 2026
Copy link
Copy Markdown
Owner Author

@cmeans cmeans left a comment

Choose a reason for hiding this comment

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

QA Review — PR #93: Fix multi-tenant security

Summary

Three targeted security fixes. All well-scoped, no over-engineering.

Code Review

1. _do_cleanup() RLS-safe

  • Now uses conn.transaction() + SET LOCAL row_security = off before cleanup
  • SET LOCAL scopes to transaction — pool connections stay clean for next user
  • SQL extracted to disable_row_security.sql
  • Correct: cleanup is a system-wide task, not tenant-scoped

2. clear(owner_id) scoped

  • Was clear() with no owner filter — cross-tenant data loss risk
  • Now takes owner_id, all 4 DELETE statements use WHERE owner_id = %s + RLS context
  • 4 new SQL files: clear_reads.sql, clear_actions.sql, clear_embeddings.sql, clear_entries.sql
  • Store protocol updated, all callers updated (conftest, benchmark, tests)
  • New test_clear_isolates_owners verifies cross-tenant isolation
  • New test_cleanup_respects_rls verifies cross-owner cleanup

3. Argon2 time_cost=3

  • PasswordHasher(time_cost=3) — one line in cli.py
  • argon2 self-describes params in hash string, so existing t=2 hashes still verify
  • Verified: hash output contains t=3

CI Gate Check

Check Conclusion
lint ✅ SUCCESS
typecheck ✅ SUCCESS
test (3.10) ✅ SUCCESS
test (3.11) ✅ SUCCESS
test (3.12) ✅ SUCCESS
codecov/patch SUCCESS

Zero non-SUCCESS/non-SKIPPED checks.

Test Results

Check Result
pytest (457 tests) ✅ 457/457 pass
ruff (src/, alembic/) ✅ Clean
mypy ✅ Clean
CI ✅ All green
Manual #1: Cross-owner cleanup ✅ Expired entries for owner-a and owner-b both cleaned after debounce
Manual #2: Argon2 time_cost=3 ✅ Hash contains t=3 parameter

Verdict

Zero findings. All CI green. All tests pass. Ready for QA Signoff.

@cmeans
Copy link
Copy Markdown
Owner Author

cmeans commented Mar 29, 2026

Adding Ready for QA Signoff — all 3 security fixes verified (RLS-safe cleanup, owner-scoped clear, argon2 time_cost=3). CI fully green (codecov SUCCESS), 457/457 pytest, manual tests pass.

@cmeans cmeans added Ready for QA Signoff QA passed — ready for maintainer final review and merge and removed QA Active QA is actively reviewing; Dev should not push changes labels Mar 29, 2026
Copy link
Copy Markdown
Owner Author

@cmeans cmeans left a comment

Choose a reason for hiding this comment

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

LGTM

@cmeans cmeans added QA Approved Manual QA testing completed and passed and removed Ready for QA Signoff QA passed — ready for maintainer final review and merge labels Mar 29, 2026
@cmeans cmeans merged commit e03ba50 into main Mar 29, 2026
43 checks passed
@cmeans cmeans deleted the feature/security-fixes branch March 29, 2026 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

QA Approved Manual QA testing completed and passed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant