From a85c16de48d217a6439c88fcfa59fd42558a4bcb Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 5 Nov 2026 15:17:55 +0100 Subject: [PATCH 1/5] fix(audit): redact credential values in audit entries; scrub stored history Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 9 + docs/modules/audit_log.md | 30 +++ .../core/simple_module_core/secret_fields.py | 7 +- .../simple_module_db/_bulk_audit_records.py | 8 +- framework/db/simple_module_db/audit.py | 11 +- .../db/simple_module_db/audit_redaction.py | 58 +++++ framework/db/simple_module_db/bulk_audit.py | 3 +- framework/db/simple_module_db/leases.py | 4 +- .../db/simple_module_db/stored_settings.py | 11 + .../db/tests/test_db_credential_audit.py | 243 ++++++++++++++++++ .../b8d4f2a6c901_redact_audit_credentials.py | 72 ++++++ .../test_host_audit_redaction_migration.py | 120 +++++++++ host/tests/test_migration_graph.py | 3 +- modules/audit_log/README.md | 30 +++ modules/auth/auth/models.py | 4 +- modules/settings/settings/_row_masking.py | 6 +- modules/settings/settings/constants.py | 3 +- modules/tenants/tenants/models.py | 3 + modules/users/users/models/access_token.py | 3 + modules/users/users/models/oauth_account.py | 3 + modules/users/users/models/refresh_token.py | 3 + modules/users/users/models/user.py | 2 + 22 files changed, 620 insertions(+), 16 deletions(-) create mode 100644 framework/db/simple_module_db/audit_redaction.py create mode 100644 framework/db/tests/test_db_credential_audit.py create mode 100644 host/migrations/versions/b8d4f2a6c901_redact_audit_credentials.py create mode 100644 host/tests/test_host_audit_redaction_migration.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f6c16b03..8821a2003 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,15 @@ All notable changes to this project are documented in this file. The format is b ## [Unreleased] +### Security +- **Audit hardening:** audit entries no longer record credential values. + Redacted fields retain change indicators using `[redacted]`; credential + primary-key identities are masked too. Migration `b8d4f2a6c901` redacts + historical credential entries and, conservatively, all historical settings + values. Downgrading does not restore these values. Operators should rotate + credentials saved through the settings UI on earlier versions, and handle + existing backups and audit exports according to their retention policies. + ## [0.0.43] — 2026-11-04 ### Added diff --git a/docs/modules/audit_log.md b/docs/modules/audit_log.md index 79314a927..a3fa1d681 100644 --- a/docs/modules/audit_log.md +++ b/docs/modules/audit_log.md @@ -221,3 +221,33 @@ pip install simple_module_audit_log ``` It depends on no other module. Actor names, the Actor search and actor links come from the app's [user directory](/framework/user-directory), so they work under `users`, `keycloak` (users who have signed in) or no provider at all (raw ids). The module also installs the core [activity feed](/framework/user-directory#activity-feed) the users edit page reads. + + +## Credential redaction + +Set `__audit_redact_fields__: ClassVar[set[str]] = {"credential"}` on a +model to keep a field in the audit diff while replacing every recorded `old` +and `new` value with `[redacted]`. Unlike `__audit_exclude_fields__`, which +omits a field entirely, redaction preserves the fact that it changed. +Exclusion takes precedence. A redacted primary key also masks `entity_id`. + +For row-dependent rules, define a static `__audit_redact_row__(row)` method +returning a set of field names. ORM capture evaluates the current and pre-edit +column dictionaries and redacts fields identified by either. Bulk and manual +capture pass `None`; the predicate must conservatively return every field +that could hold a credential when row context is unavailable. Manual capture +accepts the mapped class or its registered class name to apply its policy. + +Built-in policies cover stored secret settings, user password hashes, OAuth +credentials, tenant invitation hashes, bearer tokens, lease ownership tokens and auth refresh outcomes. +Settings use the same secret rule as the settings UI. Non-secret ORM settings +retain their values; bulk settings values are always redacted because the +statement may affect rows with different keys or prior values. API token +hashes remain excluded from automatic diffs. + +Migration `b8d4f2a6c901` hardens existing entries, conservatively redacting all +historical settings values because old diffs may lack the key or type needed +to classify them. It also masks historical credential primary-key identities. +Its downgrade is a no-op: credential values cannot be recovered. Operators +should rotate credentials saved through the settings UI on earlier versions; +the migration does not rewrite backups or previously exported audit data. diff --git a/framework/core/simple_module_core/secret_fields.py b/framework/core/simple_module_core/secret_fields.py index b635d3d1b..3b66731a5 100644 --- a/framework/core/simple_module_core/secret_fields.py +++ b/framework/core/simple_module_core/secret_fields.py @@ -20,6 +20,8 @@ _SECRET_PATTERNS = re.compile( r"(password|secret|api[_-]?key|private[_-]?key|token[_-]?secret)", re.I ) +SENSITIVE_KEYS = frozenset({"host.secret_key"}) + SECRET_MASK = "••••••••" # Value types that cannot carry credential material, so a secret-ish *name* @@ -92,7 +94,10 @@ def conceals_secret(name: str, value: Any, value_type: str | None = None) -> boo ``broker_url``, while that field's password-free default is still shown. """ return ( - name in _TYPED_SECRET_KEYS or is_named_secret(name, value_type) or embeds_credential(value) + name in SENSITIVE_KEYS + or name in _TYPED_SECRET_KEYS + or is_named_secret(name, value_type) + or embeds_credential(value) ) diff --git a/framework/db/simple_module_db/_bulk_audit_records.py b/framework/db/simple_module_db/_bulk_audit_records.py index b4286f7aa..d0cc62f39 100644 --- a/framework/db/simple_module_db/_bulk_audit_records.py +++ b/framework/db/simple_module_db/_bulk_audit_records.py @@ -10,7 +10,8 @@ from sqlalchemy import inspect as sa_inspect from sqlalchemy.orm import ORMExecuteState, Session -from simple_module_db.audit import AuditRecord, _serialize, format_pk +from simple_module_db.audit import AuditRecord, _excluded_fields, _serialize, format_pk +from simple_module_db.audit_redaction import model_for_audit, redact_changes, redact_entity_id from simple_module_db.query_filter import engine_policy logger = logging.getLogger("simple_module_db.bulk_audit") @@ -45,7 +46,7 @@ def record( ) -> AuditRecord: return AuditRecord( entity_type=self.entity_type, - entity_id=entity_id, + entity_id=redact_entity_id(model_for_audit(self.entity_type), entity_id), action=action or self.action, changes=self.changes if changes is None else changes, user_id=self.user_id, @@ -113,6 +114,9 @@ def record_bulk_audit( return name = entity_type if isinstance(entity_type, str) else entity_type.__name__ diff = [{"field": k, "new": _serialize(v)} for k, v in (changes or {}).items()] + model = model_for_audit(entity_type) + if model is not None: + diff = redact_changes(model, [c for c in diff if c["field"] not in _excluded_fields(model)]) ctx = _Ctx(name, action, diff) keys = [SUMMARY_ENTITY_ID] if ids is None else [str(i) for i in ids] _deliver(sync, callback, [ctx.record(k) for k in keys]) diff --git a/framework/db/simple_module_db/audit.py b/framework/db/simple_module_db/audit.py index a52eaa10b..c7b1db26b 100644 --- a/framework/db/simple_module_db/audit.py +++ b/framework/db/simple_module_db/audit.py @@ -23,6 +23,7 @@ from sqlalchemy import inspect as sa_inspect from sqlalchemy.orm import Session +from simple_module_db.audit_redaction import redact_changes, redact_entity_id from simple_module_db.mixins import SoftDeleteMixin # Fields injected by AuditMixin — always excluded from change diffs @@ -97,8 +98,10 @@ def _entity_pk_str(obj: object) -> str: if identity is None: # Pre-flush: fall back to the mapper-level key attrs mapper = inspector.mapper - return format_pk([getattr(obj, col.key) for col in mapper.primary_key]) - return format_pk(identity) + return redact_entity_id( + type(obj), format_pk([getattr(obj, col.key) for col in mapper.primary_key]) + ) + return redact_entity_id(type(obj), format_pk(identity)) def _column_names(obj: object) -> list[str]: @@ -128,7 +131,7 @@ def _column_diffs(obj: object, inspector: Any, excl: frozenset[str]) -> list[dic old_val = hist.deleted[0] if hist.deleted else None new_val = hist.added[0] if hist.added else None changes.append({"field": col_name, "old": _serialize(old_val), "new": _serialize(new_val)}) - return changes + return redact_changes(type(obj), changes, obj) # --------------------------------------------------------------------------- @@ -182,7 +185,7 @@ def _get_pk_cols(obj: object) -> set[str]: obj_ref=obj, entity_type=type(obj).__name__, action="created", - changes=changes, + changes=redact_changes(type(obj), changes, obj), user_id=user_id, correlation_id=correlation_id, ) diff --git a/framework/db/simple_module_db/audit_redaction.py b/framework/db/simple_module_db/audit_redaction.py new file mode 100644 index 000000000..86993e4af --- /dev/null +++ b/framework/db/simple_module_db/audit_redaction.py @@ -0,0 +1,58 @@ +"""Shared credential redaction for flush, statement and manual audit capture.""" + +from __future__ import annotations + +from typing import Any, Final + +from sqlalchemy import inspect + +AUDIT_REDACTED: Final = "[redacted]" + + +def model_for_audit(model: type | str) -> type | None: + if isinstance(model, type): + return model + from simple_module_db.model_registry import classes_by_table + + return next((cls for cls in classes_by_table.values() if cls.__name__ == model), None) + + +def redact_changes( + model: type, changes: list[dict[str, Any]], obj: object | None = None +) -> list[dict[str, Any]]: + """Keep fields and diff shape, replacing credential values before delivery. + + A row predicate receives both the current and pre-edit column values. With + no row (statement/manual capture) it receives None and must fail closed. + """ + fields = set(getattr(model, "__audit_redact_fields__", ())) + predicate = getattr(model, "__audit_redact_row__", None) + if predicate is not None: + if obj is None: + fields.update(predicate(None)) + else: + state = inspect(obj) + current = {attr.key: state.dict.get(attr.key) for attr in state.mapper.column_attrs} + previous = dict(current) + for attr in state.mapper.column_attrs: + history = state.attrs[attr.key].history + if history.has_changes(): + previous[attr.key] = history.deleted[0] if history.deleted else None + fields.update(predicate(current)) + if state.persistent: + fields.update(predicate(previous)) + return [ + { + k: AUDIT_REDACTED if c["field"] in fields and k in {"old", "new"} else v + for k, v in c.items() + } + for c in changes + ] + + +def redact_entity_id(model: type | None, entity_id: str) -> str: + if model is not None: + fields = getattr(model, "__audit_redact_fields__", ()) + if any(col.key in fields for col in inspect(model).primary_key): + return AUDIT_REDACTED + return entity_id diff --git a/framework/db/simple_module_db/bulk_audit.py b/framework/db/simple_module_db/bulk_audit.py index d23955661..d652f2767 100644 --- a/framework/db/simple_module_db/bulk_audit.py +++ b/framework/db/simple_module_db/bulk_audit.py @@ -52,6 +52,7 @@ record_bulk_audit, ) from simple_module_db.audit import _excluded_fields, format_pk +from simple_module_db.audit_redaction import redact_changes from simple_module_db.bulk_audit_targets import _is_set, _shown, _targets from simple_module_db.query_filter import engine_policy from simple_module_db.tenancy import ALL_TENANTS_OPTION @@ -112,7 +113,7 @@ def _plan(execute_state: ORMExecuteState, cls: type) -> _Ctx | None: changes = [{"field": k} for k in kept] else: changes = [{"field": k, "new": _shown(rows[0][k])} for k in kept] - ctx = _Ctx(name, actions[0] if actions else default, changes) + ctx = _Ctx(name, actions[0] if actions else default, redact_changes(cls, changes)) ctx.row_actions = actions or [default] return ctx diff --git a/framework/db/simple_module_db/leases.py b/framework/db/simple_module_db/leases.py index 51d2ba664..2724c096c 100644 --- a/framework/db/simple_module_db/leases.py +++ b/framework/db/simple_module_db/leases.py @@ -19,7 +19,7 @@ import contextlib from collections.abc import AsyncGenerator from datetime import UTC, datetime, timedelta -from typing import Any, Final +from typing import Any, ClassVar, Final from sqlalchemy import and_, func, select, text, update from sqlalchemy.dialects import postgresql, sqlite @@ -48,6 +48,8 @@ class LeaseRow(Base, table=True): # ty: ignore[unsupported-base] """One lease slot. ``holder`` is the owner's token, ``""`` when never held.""" + __audit_redact_fields__: ClassVar[set[str]] = {"holder"} + __tablename__ = TABLE_LEASE name: str = Field(primary_key=True, max_length=NAME_MAX_LENGTH) diff --git a/framework/db/simple_module_db/stored_settings.py b/framework/db/simple_module_db/stored_settings.py index bb314e2d1..88076d620 100644 --- a/framework/db/simple_module_db/stored_settings.py +++ b/framework/db/simple_module_db/stored_settings.py @@ -16,6 +16,7 @@ from typing import Final from pydantic_settings import BaseSettings +from simple_module_core.secret_fields import conceals_secret from simple_module_core.settings_hydrate import build_settings from sqlalchemy import Select, UniqueConstraint, inspect, select from sqlalchemy.ext.asyncio import AsyncConnection, AsyncSession @@ -84,6 +85,16 @@ class Setting(Base, AuditMixin, table=True): # ty: ignore[unsupported-base] inputs that don't parse. """ + @staticmethod + def __audit_redact_row__(row: dict | None) -> set[str]: + if row is None or any(row.get(k) is None for k in (COL_KEY, COL_VALUE, COL_VALUE_TYPE)): + return {COL_VALUE} + return ( + {COL_VALUE} + if conceals_secret(row[COL_KEY], row[COL_VALUE], row[COL_VALUE_TYPE]) + else set() + ) + __tablename__ = TABLE_SETTING __table_args__ = (UniqueConstraint(COL_SCOPE, COL_SCOPE_ID, COL_KEY, name=UQ_SCOPE_KEY),) diff --git a/framework/db/tests/test_db_credential_audit.py b/framework/db/tests/test_db_credential_audit.py new file mode 100644 index 000000000..b8e0f96d8 --- /dev/null +++ b/framework/db/tests/test_db_credential_audit.py @@ -0,0 +1,243 @@ +"""Credential changes remain visible without recording their values.""" + +import uuid +from datetime import UTC, datetime + +import pytest +from auth.models import RefreshOutcome +from simple_module_db.audit import collect_audit_records +from simple_module_db.leases import LeaseRow +from simple_module_db.stored_settings import Setting +from sqlalchemy.orm import Session +from tenants.models import Invitation +from users.models import OAuthAccount, RefreshToken, User, UserAccessToken + + +@pytest.mark.parametrize( + "key,value,value_type,expected", + [ + ("mail.smtp_password", "private", "string", "[redacted]"), + ("host.secret_key", "private", "int", "[redacted]"), + ("mail.url", "smtp://user:private@localhost", "string", "[redacted]"), + ("mail.port", "587", "int", "587"), + ("users.reset_password_token_lifetime_seconds", "3600", "int", "3600"), + ], +) +def test_setting_create_redaction(key, value, value_type, expected): + with Session() as session: + session.add(Setting(key=key, value=value, value_type=value_type)) + record = collect_audit_records(session)[0] + assert next(c["new"] for c in record.changes if c["field"] == "value") == expected + + +@pytest.mark.parametrize( + "entity,fields", + [ + (User(email="audit@example.test", hashed_password="private"), {"hashed_password"}), + ( + OAuthAccount( + user_id=uuid.uuid4(), + access_token="private", + refresh_token="private", + oauth_name="test", + account_id="1", + account_email="a@test", + ), + {"access_token", "refresh_token"}, + ), + (Invitation(token_hash="private"), {"token_hash"}), + ( + RefreshOutcome( + outcome_key="private", + lock_key="private", + expires_at=1, + session={"token": "private"}, + ), + {"lock_key", "session"}, + ), + ], +) +def test_model_credentials_are_redacted(entity, fields): + with Session() as session: + session.add(entity) + record = collect_audit_records(session)[0] + changes = {c["field"]: c["new"] for c in record.changes} + assert {field: changes[field] for field in fields} == dict.fromkeys(fields, "[redacted]") + + +@pytest.mark.parametrize("cls", [UserAccessToken, RefreshToken]) +def test_token_primary_key_is_redacted(cls): + entity = cls(token="private", user_id=uuid.uuid4(), expires_at=datetime.now(UTC)) + with Session() as session: + session.add(entity) + assert collect_audit_records(session)[0].entity_id == "[redacted]" + + +@pytest.fixture +def credential_session(): + from simple_module_db.listeners import attach_session_listeners + from simple_module_db.query_filter import EngineTenancy, bind_engine_policy + from sqlalchemy import create_engine + + engine = create_engine("sqlite://") + records = [] + policy = EngineTenancy( + tenant_strict=False, audit_callback=lambda session, batch: records.extend(batch) + ) + bind_engine_policy(engine, policy) + + class AuditSession(Session): + pass + + attach_session_listeners(AuditSession) + try: + Setting.metadata.create_all(engine) + User.metadata.create_all(engine) + with AuditSession(engine) as session: + yield session, records + finally: + engine.dispose() + + +@pytest.mark.parametrize("kind", ["password", "setting", "renamed", "url"]) +def test_credential_update_redacts_both_sides(credential_session, kind): + session, records = credential_session + if kind == "password": + entity = User(email="update@test", hashed_password="old-private") + field = "hashed_password" + else: + entity = Setting( + key="mail.url" if kind == "url" else "mail.smtp_password", + value="smtp://u:private@localhost" if kind == "url" else "old-private", + ) + field = "value" + session.add(entity) + session.flush() + records.clear() + setattr(entity, field, "public" if kind == "url" else "new-private") + if kind == "renamed": + entity.key = "mail.label" + session.flush() + assert next(c for c in records[0].changes if c["field"] == field) == { + "field": field, + "old": "[redacted]", + "new": "[redacted]", + } + + +@pytest.mark.parametrize("core", [False, True]) +def test_bulk_credentials_redacted(credential_session, core): + from sqlalchemy import update + + session, records = credential_session + user = User(email="bulk@test", hashed_password="old-private") + setting = Setting(key="mail.smtp_password", value="old-private") + session.add_all([user, setting]) + session.flush() + records.clear() + for model, field in [(User, "hashed_password"), (Setting, "value")]: + target = model.__table__ if core else model + session.execute(update(target).values({field: "new-private"})) + for record, field in zip(records, ["hashed_password", "value"], strict=True): + assert record.changes == [{"field": field, "new": "[redacted]"}] + + +@pytest.mark.parametrize("by_name", [False, True]) +def test_manual_bulk_redaction(credential_session, by_name): + from simple_module_db import record_bulk_audit + + session, records = credential_session + record_bulk_audit( + session, "User" if by_name else User, "bulk_updated", ["1"], {"hashed_password": "private"} + ) + assert records[0].changes == [{"field": "hashed_password", "new": "[redacted]"}] + + +def test_nonsecret_setting_update_retains_values(credential_session): + session, records = credential_session + setting = Setting(key="mail.port", value="587", value_type="int") + session.add(setting) + session.flush() + records.clear() + setting.value = "465" + session.flush() + assert records[0].changes == [{"field": "value", "old": "587", "new": "465"}] + + +@pytest.mark.parametrize("core", [False, True]) +def test_bulk_insert_and_summary_redaction(credential_session, monkeypatch, core): + import simple_module_db.bulk_audit as bulk + from sqlalchemy import insert, update + + session, records = credential_session + target = Setting.__table__ if core else Setting + session.execute( + insert(target).values( + key="mail.smtp_password", + value="private", + value_type="string", + scope="system", + scope_id="", + ) + ) + assert next(c["new"] for c in records[0].changes if c["field"] == "value") == "[redacted]" + records.clear() + monkeypatch.setattr(bulk, "BULK_AUDIT_ID_CAP", 0) + session.execute(update(target).values(value="new-private")) + assert records[0].entity_id == "*" + assert records[0].changes[0] == {"field": "value", "new": "[redacted]"} + + +@pytest.mark.parametrize("cls", [UserAccessToken, RefreshToken]) +def test_token_identity_redacted_on_update_delete_and_bulk(credential_session, cls): + from simple_module_db import record_bulk_audit + from sqlalchemy import delete, update + + session, records = credential_session + user = User(email="token-owner@test") + session.add(user) + session.flush() + token = cls(token="private", user_id=user.id, expires_at=datetime.now(UTC)) + session.add(token) + session.flush() + records.clear() + token.expires_at = datetime(2030, 1, 1, tzinfo=UTC) + session.flush() + session.execute(update(cls).values(expires_at=datetime(2031, 1, 1, tzinfo=UTC))) + record_bulk_audit(session, cls.__name__, "bulk_updated", ["private"]) + session.execute(delete(cls)) + assert len(records) == 4 + assert all(r.entity_id == "[redacted]" for r in records) + + +def test_typed_setting_redaction(): + from pydantic import SecretStr + from simple_module_core.secret_fields import register_typed_secrets + from sqlmodel import SQLModel + + class CredentialSettings(SQLModel): + credential: SecretStr + + register_typed_secrets("audit_test", CredentialSettings) + with Session() as session: + session.add(Setting(key="audit_test.credential", value="private")) + changes = collect_audit_records(session)[0].changes + assert next(c["new"] for c in changes if c["field"] == "value") == "[redacted]" + + +def test_manual_bulk_honors_field_exclusions(credential_session): + from api_tokens.models import ApiToken + from simple_module_db import record_bulk_audit + + session, records = credential_session + record_bulk_audit( + session, ApiToken, "bulk_updated", ["1"], {"token_hash": "private", "name": "visible"} + ) + assert records[0].changes == [{"field": "name", "new": "visible"}] + + +def test_lease_ownership_value_is_redacted(): + with Session() as session: + session.add(LeaseRow(name="audit", slot=0, holder="private", expires_at=datetime.now(UTC))) + changes = collect_audit_records(session)[0].changes + assert next(c["new"] for c in changes if c["field"] == "holder") == "[redacted]" diff --git a/host/migrations/versions/b8d4f2a6c901_redact_audit_credentials.py b/host/migrations/versions/b8d4f2a6c901_redact_audit_credentials.py new file mode 100644 index 000000000..e17f4f76b --- /dev/null +++ b/host/migrations/versions/b8d4f2a6c901_redact_audit_credentials.py @@ -0,0 +1,72 @@ +"""Harden historical audit credential values. + +Revision ID: b8d4f2a6c901 +Revises: a7c2e91d4b58 +""" + +import sqlalchemy as sa +from alembic import op + +revision = "b8d4f2a6c901" +down_revision = "a7c2e91d4b58" +branch_labels = None +depends_on = None + +# Frozen policy: historical settings entries may lack their key or declared +# type, so all historical setting values are redacted conservatively. +_REDACTED = "[redacted]" +_FIELDS = { + "Setting": {"value"}, + "LeaseRow": {"holder"}, + "User": {"hashed_password"}, + "OAuthAccount": {"access_token", "refresh_token"}, + "Invitation": {"token_hash"}, + "ApiToken": {"token_hash"}, + "UserAccessToken": {"token"}, + "RefreshToken": {"token"}, + "RefreshOutcome": {"session", "outcome_key", "lock_key"}, +} +_REDACT_IDS = {"UserAccessToken", "RefreshToken", "RefreshOutcome"} + + +def upgrade() -> None: + conn = op.get_bind() + audit = sa.Table("audit_log_audit_entry", sa.MetaData(), autoload_with=conn) + # Keyset batches bound memory and close each cursor before updating rows. + last_id = None + while True: + query = ( + sa.select(audit).where(audit.c.entity_type.in_(_FIELDS)).order_by(audit.c.id).limit(500) + ) + if last_id is not None: + query = query.where(audit.c.id > last_id) + rows = conn.execute(query).mappings().all() + if not rows: + break + for row in rows: + changes = row["changes"] + fields = _FIELDS[row["entity_type"]] + if isinstance(changes, list): + changes = [ + { + k: _REDACTED if c.get("field") in fields and k in {"old", "new"} else v + for k, v in c.items() + } + if isinstance(c, dict) + else c + for c in changes + ] + elif isinstance(changes, dict): + changes = {k: _REDACTED if k in fields else v for k, v in changes.items()} + identity = _REDACTED if row["entity_type"] in _REDACT_IDS else row["entity_id"] + conn.execute( + audit.update() + .where(audit.c.id == row["id"]) + .values(changes=changes, entity_id=identity) + ) + last_id = rows[-1]["id"] + + +def downgrade() -> None: + # Credential values are intentionally unrecoverable; retain the hardened trail. + pass diff --git a/host/tests/test_host_audit_redaction_migration.py b/host/tests/test_host_audit_redaction_migration.py new file mode 100644 index 000000000..bb8de6ff6 --- /dev/null +++ b/host/tests/test_host_audit_redaction_migration.py @@ -0,0 +1,120 @@ +"""Historical audit hardening keeps the trail and removes credential values.""" + +import importlib.util +from pathlib import Path + +import sqlalchemy as sa +from alembic.migration import MigrationContext +from alembic.operations import Operations + + +def test_upgrade_scrubs_credentials_and_preserves_other_changes(): + path = ( + Path(__file__).parents[1] / "migrations/versions/b8d4f2a6c901_redact_audit_credentials.py" + ) + spec = importlib.util.spec_from_file_location("audit_hardening_revision", path) + revision = importlib.util.module_from_spec(spec) + spec.loader.exec_module(revision) + engine = sa.create_engine("sqlite://") + metadata = sa.MetaData() + audit = sa.Table( + "audit_log_audit_entry", + metadata, + sa.Column("id", sa.String, primary_key=True), + sa.Column("entity_type", sa.String), + sa.Column("entity_id", sa.String), + sa.Column("changes", sa.JSON), + ) + metadata.create_all(engine) + cases = [ + ("Setting", "1", "value"), + ("LeaseRow", "lease", "holder"), + ("User", "2", "hashed_password"), + ("OAuthAccount", "3", "access_token"), + ("OAuthAccount", "4", "refresh_token"), + ("Invitation", "5", "token_hash"), + ("RefreshOutcome", "private-id", "session"), + ("UserAccessToken", "private-id", "token"), + ("RefreshToken", "private-id", "token"), + ("ApiToken", "9", "token_hash"), + ] + with engine.begin() as conn: + for i, (entity, identity, field) in enumerate(cases): + conn.execute( + audit.insert().values( + id=str(i), + entity_type=entity, + entity_id=identity, + changes=[ + {"field": field, "old": "old-private", "new": "private"}, + {"field": "label", "new": "visible"}, + ], + ) + ) + conn.execute( + audit.insert().values( + id="ordinary", + entity_type="User", + entity_id="10", + changes=[{"field": "email", "new": "visible"}], + ) + ) + conn.execute( + audit.insert(), + [ + { + "id": f"batch-{i:04}", + "entity_type": "Setting", + "entity_id": str(i), + "changes": [{"field": "value", "new": "private"}], + } + for i in range(501) + ], + ) + conn.execute( + audit.insert().values( + id="legacy", + entity_type="User", + entity_id="11", + changes={"hashed_password": {"old": "private"}, "label": "visible"}, + ) + ) + conn.execute( + audit.insert().values( + id="sparse", + entity_type="User", + entity_id="12", + changes=[{"field": "hashed_password"}, "note"], + ) + ) + conn.execute( + audit.insert().values(id="empty", entity_type="User", entity_id="13", changes=None) + ) + with Operations.context(MigrationContext.configure(conn)): + revision.upgrade() + revision.upgrade() # idempotent data rewrite + revision.downgrade() + rows = conn.execute(sa.select(audit).order_by(audit.c.id)).mappings().all() + assert len(rows) == len(cases) + 505 + for row in rows: + if row["id"].startswith("batch-"): + assert row["changes"] == [{"field": "value", "new": "[redacted]"}] + continue + if row["id"] == "legacy": + assert row["changes"] == {"hashed_password": "[redacted]", "label": "visible"} + continue + if row["id"] == "sparse": + assert row["changes"] == [{"field": "hashed_password"}, "note"] + continue + if row["id"] == "empty": + assert row["changes"] is None + continue + if row["id"] == "ordinary": + assert row["changes"] == [{"field": "email", "new": "visible"}] + continue + assert row["changes"][0]["old"] == "[redacted]" + assert row["changes"][0]["new"] == "[redacted]" + assert row["changes"][1] == {"field": "label", "new": "visible"} + if row["entity_type"] in {"UserAccessToken", "RefreshToken", "RefreshOutcome"}: + assert row["entity_id"] == "[redacted]" + engine.dispose() diff --git a/host/tests/test_migration_graph.py b/host/tests/test_migration_graph.py index 6749026ac..fc8e9e4fc 100644 --- a/host/tests/test_migration_graph.py +++ b/host/tests/test_migration_graph.py @@ -52,6 +52,7 @@ "f1a9c3d7b2e4": "tenants", "70786227af4c": "audit_log", "a7c2e91d4b58": "audit_log", + "b8d4f2a6c901": "audit_log", "f733c7938fd1": "sharing", "a738c2e4f601": "auth", "d263ea727663": "leases", # framework-owned sm_lease (#539) @@ -68,7 +69,7 @@ # head revision -> module owning the tables it creates or alters. _HEADS = { - "a7c2e91d4b58": "audit_log", + "b8d4f2a6c901": "audit_log", "e731a1b2c3d4": "users", # also carries ``api_tokens``, ``background_tasks``, ``file_storage`` "f1a9c3d7b2e4": "tenants", "f6a3b9d2e8c4": "keycloak", diff --git a/modules/audit_log/README.md b/modules/audit_log/README.md index e573be54a..0ad228ab8 100644 --- a/modules/audit_log/README.md +++ b/modules/audit_log/README.md @@ -52,3 +52,33 @@ class Cache(Base, table=True): ## License MIT + + +## Credential redaction + +Set `__audit_redact_fields__: ClassVar[set[str]] = {"credential"}` on a +model to keep a field in the audit diff while replacing every recorded `old` +and `new` value with `[redacted]`. Unlike `__audit_exclude_fields__`, which +omits a field entirely, redaction preserves the fact that it changed. +Exclusion takes precedence. A redacted primary key also masks `entity_id`. + +For row-dependent rules, define a static `__audit_redact_row__(row)` method +returning a set of field names. ORM capture evaluates the current and pre-edit +column dictionaries and redacts fields identified by either. Bulk and manual +capture pass `None`; the predicate must conservatively return every field +that could hold a credential when row context is unavailable. Manual capture +accepts the mapped class or its registered class name to apply its policy. + +Built-in policies cover stored secret settings, user password hashes, OAuth +credentials, tenant invitation hashes, bearer tokens, lease ownership tokens and auth refresh outcomes. +Settings use the same secret rule as the settings UI. Non-secret ORM settings +retain their values; bulk settings values are always redacted because the +statement may affect rows with different keys or prior values. API token +hashes remain excluded from automatic diffs. + +Migration `b8d4f2a6c901` hardens existing entries, conservatively redacting all +historical settings values because old diffs may lack the key or type needed +to classify them. It also masks historical credential primary-key identities. +Its downgrade is a no-op: credential values cannot be recovered. Operators +should rotate credentials saved through the settings UI on earlier versions; +the migration does not rewrite backups or previously exported audit data. diff --git a/modules/auth/auth/models.py b/modules/auth/auth/models.py index 85021a6ec..7e0847dbf 100644 --- a/modules/auth/auth/models.py +++ b/modules/auth/auth/models.py @@ -1,6 +1,6 @@ """Short-lived cross-worker outcomes for browser token renewal.""" -from typing import Any +from typing import Any, ClassVar from simple_module_db.base import create_module_base from sqlalchemy import JSON, BigInteger @@ -10,6 +10,8 @@ class RefreshOutcome(Base, table=True): # ty: ignore[unsupported-base] + __audit_redact_fields__: ClassVar[set[str]] = {"outcome_key", "lock_key", "session"} + __tablename__ = "auth_refresh_outcome" # sha256(refresh key : identity : spent sealed token); not a session id. diff --git a/modules/settings/settings/_row_masking.py b/modules/settings/settings/_row_masking.py index 9ea41c241..75cd8291d 100644 --- a/modules/settings/settings/_row_masking.py +++ b/modules/settings/settings/_row_masking.py @@ -12,7 +12,7 @@ from simple_module_core.secret_fields import conceals_secret from simple_module_db.stored_settings import Setting -from settings.constants import SENSITIVE_KEYS, SENSITIVE_PLACEHOLDER +from settings.constants import SENSITIVE_PLACEHOLDER from settings.contracts.schemas import SettingOut @@ -27,9 +27,7 @@ def is_masked(entity: Setting) -> bool: clear text in the browse table and pre-filled into the edit form purely because the allowlist had a single entry in it. """ - return entity.key in SENSITIVE_KEYS or conceals_secret( - entity.key, entity.value, entity.value_type - ) + return conceals_secret(entity.key, entity.value, entity.value_type) def out(entity: Setting) -> SettingOut: diff --git a/modules/settings/settings/constants.py b/modules/settings/settings/constants.py index bcce41550..de07fa78d 100644 --- a/modules/settings/settings/constants.py +++ b/modules/settings/settings/constants.py @@ -10,6 +10,8 @@ from typing import Final +from simple_module_core.secret_fields import SENSITIVE_KEYS as SENSITIVE_KEYS + # Re-exported (#500): these now live with the table in the framework. from simple_module_db.stored_settings import ( # noqa: F401 ALL_VALUE_TYPES, @@ -51,7 +53,6 @@ # anyone who can read it can forge a session cookie for any account. Masked on # the way out only — the boot-time reader goes straight to SQL, so hiding it # here costs nothing. -SENSITIVE_KEYS: Final = frozenset({"host.secret_key"}) SENSITIVE_PLACEHOLDER: Final = "********" # ── Routing ────────────────────────────────────────────────────────── diff --git a/modules/tenants/tenants/models.py b/modules/tenants/tenants/models.py index c135e3eef..6c2fe2484 100644 --- a/modules/tenants/tenants/models.py +++ b/modules/tenants/tenants/models.py @@ -10,6 +10,7 @@ import uuid from datetime import datetime +from typing import ClassVar from simple_module_db import is_valid_tenant_id from simple_module_db.base import create_module_base @@ -81,6 +82,8 @@ class Membership(Base, AuditMixin, table=True): # ty: ignore[unsupported-base] class Invitation(Base, AuditMixin, table=True): # ty: ignore[unsupported-base] """A pending invitation. Only the token's SHA-256 is stored.""" + __audit_redact_fields__: ClassVar[set[str]] = {"token_hash"} + __tablename__ = "tenants_invitation" __table_args__ = ( # One open invitation per address: the service's duplicate check is a diff --git a/modules/users/users/models/access_token.py b/modules/users/users/models/access_token.py index 2161234a0..2d7caa181 100644 --- a/modules/users/users/models/access_token.py +++ b/modules/users/users/models/access_token.py @@ -7,6 +7,7 @@ import uuid from datetime import datetime, timedelta +from typing import ClassVar from fastapi_users_db_sqlalchemy.generics import GUID, TIMESTAMPAware, now_utc from sqlmodel import Field @@ -30,6 +31,8 @@ def _default_expires_at() -> datetime: class UserAccessToken(Base, table=True): # ty: ignore[unsupported-base] """fastapi-users DatabaseStrategy-backed access tokens.""" + __audit_redact_fields__: ClassVar[set[str]] = {"token"} + __tablename__ = "users_access_token" token: str = Field(max_length=64, primary_key=True) diff --git a/modules/users/users/models/oauth_account.py b/modules/users/users/models/oauth_account.py index 26f26b036..775caa82b 100644 --- a/modules/users/users/models/oauth_account.py +++ b/modules/users/users/models/oauth_account.py @@ -10,6 +10,7 @@ # Relationship resolution requires runtime annotations. import uuid +from typing import ClassVar from fastapi_users_db_sqlalchemy.generics import GUID from sqlmodel import Field @@ -20,6 +21,8 @@ class OAuthAccount(Base, table=True): # ty: ignore[unsupported-base] """One row per (provider, account_id) link to a local User.""" + __audit_redact_fields__: ClassVar[set[str]] = {"refresh_token", "access_token"} + __tablename__ = "users_oauth_account" id: uuid.UUID = Field( diff --git a/modules/users/users/models/refresh_token.py b/modules/users/users/models/refresh_token.py index 34f77f774..ffdb2c19f 100644 --- a/modules/users/users/models/refresh_token.py +++ b/modules/users/users/models/refresh_token.py @@ -4,6 +4,7 @@ import uuid as uuid_mod from datetime import UTC, datetime +from typing import ClassVar from fastapi_users_db_sqlalchemy.generics import GUID from sqlmodel import Field @@ -14,6 +15,8 @@ class RefreshToken(Base, table=True): # ty: ignore[unsupported-base] """Opaque refresh token exchanged for a new access + refresh pair.""" + __audit_redact_fields__: ClassVar[set[str]] = {"token"} + __tablename__ = "users_refresh_token" token: str = Field(primary_key=True, max_length=64) diff --git a/modules/users/users/models/user.py b/modules/users/users/models/user.py index 405426044..2de3b3b57 100644 --- a/modules/users/users/models/user.py +++ b/modules/users/users/models/user.py @@ -33,6 +33,8 @@ class User(Base, AuditMixin, table=True): # ty: ignore[unsupported-base] """Local user. Column surface mirrors fastapi-users' SQLAlchemyBaseUserTableUUID.""" + __audit_redact_fields__: ClassVar[set[str]] = {"hashed_password"} + __tablename__ = "users_user" id: uuid.UUID = Field( From 074f92c2426ea2d5476cd87e4693035c9c6fda9b Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 5 Nov 2026 15:19:16 +0100 Subject: [PATCH 2/5] test(audit): construct Invitation with its required fields Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/db/tests/test_db_credential_audit.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/framework/db/tests/test_db_credential_audit.py b/framework/db/tests/test_db_credential_audit.py index b8e0f96d8..30cd544c4 100644 --- a/framework/db/tests/test_db_credential_audit.py +++ b/framework/db/tests/test_db_credential_audit.py @@ -45,7 +45,15 @@ def test_setting_create_redaction(key, value, value_type, expected): ), {"access_token", "refresh_token"}, ), - (Invitation(token_hash="private"), {"token_hash"}), + ( + Invitation( + tenant_id="default", + email="invitee@example.test", + token_hash="private", + expires_at=datetime.now(UTC), + ), + {"token_hash"}, + ), ( RefreshOutcome( outcome_key="private", From b869afdcdaf4d69fddc9cccddaa366fd524cd58d Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 5 Nov 2026 15:31:18 +0100 Subject: [PATCH 3/5] refactor(optimize): resolve audit models once, skip redaction work for unredacted models Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- docs/modules/audit_log.md | 4 ++-- .../core/simple_module_core/secret_fields.py | 10 ++++++-- .../simple_module_db/_bulk_audit_records.py | 11 +++++---- framework/db/simple_module_db/audit.py | 8 +++---- .../db/simple_module_db/audit_redaction.py | 24 ++++++++++++------- .../db/simple_module_db/stored_settings.py | 12 +++++----- modules/audit_log/README.md | 4 ++-- modules/settings/settings/constants.py | 7 +----- 8 files changed, 46 insertions(+), 34 deletions(-) diff --git a/docs/modules/audit_log.md b/docs/modules/audit_log.md index a3fa1d681..cb2ddf669 100644 --- a/docs/modules/audit_log.md +++ b/docs/modules/audit_log.md @@ -222,7 +222,6 @@ pip install simple_module_audit_log It depends on no other module. Actor names, the Actor search and actor links come from the app's [user directory](/framework/user-directory), so they work under `users`, `keycloak` (users who have signed in) or no provider at all (raw ids). The module also installs the core [activity feed](/framework/user-directory#activity-feed) the users edit page reads. - ## Credential redaction Set `__audit_redact_fields__: ClassVar[set[str]] = {"credential"}` on a @@ -239,7 +238,8 @@ that could hold a credential when row context is unavailable. Manual capture accepts the mapped class or its registered class name to apply its policy. Built-in policies cover stored secret settings, user password hashes, OAuth -credentials, tenant invitation hashes, bearer tokens, lease ownership tokens and auth refresh outcomes. +credentials, tenant invitation hashes, bearer tokens, lease ownership tokens +and auth refresh outcomes. Settings use the same secret rule as the settings UI. Non-secret ORM settings retain their values; bulk settings values are always redacted because the statement may affect rows with different keys or prior values. API token diff --git a/framework/core/simple_module_core/secret_fields.py b/framework/core/simple_module_core/secret_fields.py index 3b66731a5..5da99b7ee 100644 --- a/framework/core/simple_module_core/secret_fields.py +++ b/framework/core/simple_module_core/secret_fields.py @@ -9,7 +9,7 @@ from __future__ import annotations import re -from typing import Any, get_args, get_origin +from typing import Any, Final, get_args, get_origin from urllib.parse import urlsplit from pydantic import BaseModel, SecretBytes, SecretStr @@ -20,9 +20,15 @@ _SECRET_PATTERNS = re.compile( r"(password|secret|api[_-]?key|private[_-]?key|token[_-]?secret)", re.I ) -SENSITIVE_KEYS = frozenset({"host.secret_key"}) SECRET_MASK = "••••••••" +# Keys whose stored value must never leave the settings API or the raw +# key/value browse screen. ``host.secret_key`` is the session-signing key: the +# hosting layer generates and persists it here when SM_SECRET_KEY is unset, and +# anyone who can read it can forge a session cookie for any account. Masked on +# the way out only — the boot-time reader goes straight to SQL, so hiding it +# here costs nothing. +SENSITIVE_KEYS: Final = frozenset({"host.secret_key"}) # Value types that cannot carry credential material, so a secret-ish *name* # on one of them is a false positive rather than something to hide. diff --git a/framework/db/simple_module_db/_bulk_audit_records.py b/framework/db/simple_module_db/_bulk_audit_records.py index d0cc62f39..58888bbfe 100644 --- a/framework/db/simple_module_db/_bulk_audit_records.py +++ b/framework/db/simple_module_db/_bulk_audit_records.py @@ -32,6 +32,7 @@ class _Ctx: def __init__(self, entity_type: str, action: str, changes: list[dict[str, Any]]) -> None: self.entity_type = entity_type + self._model = model_for_audit(entity_type) self.action = action self.row_actions = [action] self.changes = changes @@ -46,7 +47,7 @@ def record( ) -> AuditRecord: return AuditRecord( entity_type=self.entity_type, - entity_id=redact_entity_id(model_for_audit(self.entity_type), entity_id), + entity_id=redact_entity_id(self._model, entity_id), action=action or self.action, changes=self.changes if changes is None else changes, user_id=self.user_id, @@ -114,9 +115,11 @@ def record_bulk_audit( return name = entity_type if isinstance(entity_type, str) else entity_type.__name__ diff = [{"field": k, "new": _serialize(v)} for k, v in (changes or {}).items()] - model = model_for_audit(entity_type) - if model is not None: - diff = redact_changes(model, [c for c in diff if c["field"] not in _excluded_fields(model)]) ctx = _Ctx(name, action, diff) + model = ctx._model + if model is not None: + ctx.changes = redact_changes( + model, [c for c in diff if c["field"] not in _excluded_fields(model)] + ) keys = [SUMMARY_ENTITY_ID] if ids is None else [str(i) for i in ids] _deliver(sync, callback, [ctx.record(k) for k in keys]) diff --git a/framework/db/simple_module_db/audit.py b/framework/db/simple_module_db/audit.py index c7b1db26b..419315228 100644 --- a/framework/db/simple_module_db/audit.py +++ b/framework/db/simple_module_db/audit.py @@ -98,10 +98,10 @@ def _entity_pk_str(obj: object) -> str: if identity is None: # Pre-flush: fall back to the mapper-level key attrs mapper = inspector.mapper - return redact_entity_id( - type(obj), format_pk([getattr(obj, col.key) for col in mapper.primary_key]) - ) - return redact_entity_id(type(obj), format_pk(identity)) + pk = format_pk([getattr(obj, col.key) for col in mapper.primary_key]) + else: + pk = format_pk(identity) + return redact_entity_id(type(obj), pk) def _column_names(obj: object) -> list[str]: diff --git a/framework/db/simple_module_db/audit_redaction.py b/framework/db/simple_module_db/audit_redaction.py index 86993e4af..dbedf3519 100644 --- a/framework/db/simple_module_db/audit_redaction.py +++ b/framework/db/simple_module_db/audit_redaction.py @@ -10,6 +10,7 @@ def model_for_audit(model: type | str) -> type | None: + """Accept a mapped class or its registered class name; None for unknown names.""" if isinstance(model, type): return model from simple_module_db.model_registry import classes_by_table @@ -32,15 +33,21 @@ def redact_changes( fields.update(predicate(None)) else: state = inspect(obj) - current = {attr.key: state.dict.get(attr.key) for attr in state.mapper.column_attrs} - previous = dict(current) + current: dict[str, Any] = {} + previous: dict[str, Any] = {} for attr in state.mapper.column_attrs: - history = state.attrs[attr.key].history + key = attr.key + value = current[key] = state.dict.get(key) + history = state.attrs[key].history if history.has_changes(): - previous[attr.key] = history.deleted[0] if history.deleted else None + previous[key] = history.deleted[0] if history.deleted else None + else: + previous[key] = value fields.update(predicate(current)) if state.persistent: fields.update(predicate(previous)) + if not fields: + return changes return [ { k: AUDIT_REDACTED if c["field"] in fields and k in {"old", "new"} else v @@ -51,8 +58,9 @@ def redact_changes( def redact_entity_id(model: type | None, entity_id: str) -> str: - if model is not None: - fields = getattr(model, "__audit_redact_fields__", ()) - if any(col.key in fields for col in inspect(model).primary_key): - return AUDIT_REDACTED + fields = getattr(model, "__audit_redact_fields__", None) + if model is None or not fields: + return entity_id + if any(col.key in fields for col in inspect(model).primary_key): + return AUDIT_REDACTED return entity_id diff --git a/framework/db/simple_module_db/stored_settings.py b/framework/db/simple_module_db/stored_settings.py index 88076d620..2555047e7 100644 --- a/framework/db/simple_module_db/stored_settings.py +++ b/framework/db/simple_module_db/stored_settings.py @@ -87,13 +87,13 @@ class Setting(Base, AuditMixin, table=True): # ty: ignore[unsupported-base] @staticmethod def __audit_redact_row__(row: dict | None) -> set[str]: - if row is None or any(row.get(k) is None for k in (COL_KEY, COL_VALUE, COL_VALUE_TYPE)): + if ( + row is None + or any(row.get(k) is None for k in (COL_KEY, COL_VALUE, COL_VALUE_TYPE)) + or conceals_secret(row[COL_KEY], row[COL_VALUE], row[COL_VALUE_TYPE]) + ): return {COL_VALUE} - return ( - {COL_VALUE} - if conceals_secret(row[COL_KEY], row[COL_VALUE], row[COL_VALUE_TYPE]) - else set() - ) + return set() __tablename__ = TABLE_SETTING __table_args__ = (UniqueConstraint(COL_SCOPE, COL_SCOPE_ID, COL_KEY, name=UQ_SCOPE_KEY),) diff --git a/modules/audit_log/README.md b/modules/audit_log/README.md index 0ad228ab8..9e71ba308 100644 --- a/modules/audit_log/README.md +++ b/modules/audit_log/README.md @@ -53,7 +53,6 @@ class Cache(Base, table=True): MIT - ## Credential redaction Set `__audit_redact_fields__: ClassVar[set[str]] = {"credential"}` on a @@ -70,7 +69,8 @@ that could hold a credential when row context is unavailable. Manual capture accepts the mapped class or its registered class name to apply its policy. Built-in policies cover stored secret settings, user password hashes, OAuth -credentials, tenant invitation hashes, bearer tokens, lease ownership tokens and auth refresh outcomes. +credentials, tenant invitation hashes, bearer tokens, lease ownership tokens +and auth refresh outcomes. Settings use the same secret rule as the settings UI. Non-secret ORM settings retain their values; bulk settings values are always redacted because the statement may affect rows with different keys or prior values. API token diff --git a/modules/settings/settings/constants.py b/modules/settings/settings/constants.py index de07fa78d..d86233f9a 100644 --- a/modules/settings/settings/constants.py +++ b/modules/settings/settings/constants.py @@ -47,12 +47,7 @@ ALL_SCOPES: Final = (SCOPE_SYSTEM, SCOPE_TENANT, SCOPE_USER) # ── Secrets ────────────────────────────────────────────────────────── -# Keys whose stored value must never leave the settings API or the raw -# key/value browse screen. ``host.secret_key`` is the session-signing key: the -# hosting layer generates and persists it here when SM_SECRET_KEY is unset, and -# anyone who can read it can forge a session cookie for any account. Masked on -# the way out only — the boot-time reader goes straight to SQL, so hiding it -# here costs nothing. +# ``SENSITIVE_KEYS`` is re-exported from ``simple_module_core.secret_fields``. SENSITIVE_PLACEHOLDER: Final = "********" # ── Routing ────────────────────────────────────────────────────────── From a9046dad7a7c6fc7728dc1c4707da8cdc3133070 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 5 Nov 2026 15:42:43 +0100 Subject: [PATCH 4/5] fix: address code review findings (round 1, pass 1) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 6 + .../simple_module_db/_bulk_audit_records.py | 19 ++- .../db/simple_module_db/audit_redaction.py | 15 ++- framework/db/simple_module_db/bulk_audit.py | 4 +- framework/db/simple_module_db/listeners.py | 9 +- .../db/tests/test_db_credential_audit.py | 119 +++--------------- .../tests/test_db_credential_log_masking.py | 16 +++ .../b8d4f2a6c901_redact_audit_credentials.py | 11 +- modules/audit_log/README.md | 31 +---- .../auth/tests/test_auth_audit_redaction.py | 18 +++ modules/settings/settings/_row_masking.py | 4 +- modules/settings/settings/constants.py | 4 +- .../tests/test_tenants_audit_redaction.py | 21 ++++ .../users/tests/test_users_audit_redaction.py | 118 +++++++++++++++++ 14 files changed, 242 insertions(+), 153 deletions(-) create mode 100644 framework/db/tests/test_db_credential_log_masking.py create mode 100644 modules/auth/tests/test_auth_audit_redaction.py create mode 100644 modules/tenants/tests/test_tenants_audit_redaction.py create mode 100644 modules/users/tests/test_users_audit_redaction.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 8821a2003..dcd1a7414 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,12 @@ All notable changes to this project are documented in this file. The format is b values. Downgrading does not restore these values. Operators should rotate credentials saved through the settings UI on earlier versions, and handle existing backups and audit exports according to their retention policies. +- Entity update/delete log events mask primary keys that are themselves + credentials (session and refresh tokens), as the audit trail does. + +### Removed +- `settings.constants.SENSITIVE_KEYS`; the set now lives in + `simple_module_core.secret_fields.SENSITIVE_KEYS`. ## [0.0.43] — 2026-11-04 diff --git a/framework/db/simple_module_db/_bulk_audit_records.py b/framework/db/simple_module_db/_bulk_audit_records.py index 58888bbfe..3b44ccd6c 100644 --- a/framework/db/simple_module_db/_bulk_audit_records.py +++ b/framework/db/simple_module_db/_bulk_audit_records.py @@ -30,9 +30,16 @@ class _Ctx: """What one audited statement records, built before it runs.""" - def __init__(self, entity_type: str, action: str, changes: list[dict[str, Any]]) -> None: + def __init__( + self, + entity_type: str, + action: str, + changes: list[dict[str, Any]], + model: type | None = None, + ) -> None: self.entity_type = entity_type - self._model = model_for_audit(entity_type) + # The statement's own class when known: a name lookup scans the registry. + self._model = model or model_for_audit(entity_type) self.action = action self.row_actions = [action] self.changes = changes @@ -47,7 +54,11 @@ def record( ) -> AuditRecord: return AuditRecord( entity_type=self.entity_type, - entity_id=redact_entity_id(self._model, entity_id), + entity_id=( + entity_id + if entity_id == SUMMARY_ENTITY_ID + else redact_entity_id(self._model, entity_id) + ), action=action or self.action, changes=self.changes if changes is None else changes, user_id=self.user_id, @@ -115,7 +126,7 @@ def record_bulk_audit( return name = entity_type if isinstance(entity_type, str) else entity_type.__name__ diff = [{"field": k, "new": _serialize(v)} for k, v in (changes or {}).items()] - ctx = _Ctx(name, action, diff) + ctx = _Ctx(name, action, diff, entity_type if isinstance(entity_type, type) else None) model = ctx._model if model is not None: ctx.changes = redact_changes( diff --git a/framework/db/simple_module_db/audit_redaction.py b/framework/db/simple_module_db/audit_redaction.py index dbedf3519..794339f20 100644 --- a/framework/db/simple_module_db/audit_redaction.py +++ b/framework/db/simple_module_db/audit_redaction.py @@ -26,6 +26,8 @@ def redact_changes( A row predicate receives both the current and pre-edit column values. With no row (statement/manual capture) it receives None and must fail closed. """ + if not changes: + return changes fields = set(getattr(model, "__audit_redact_fields__", ())) predicate = getattr(model, "__audit_redact_row__", None) if predicate is not None: @@ -57,10 +59,13 @@ def redact_changes( ] -def redact_entity_id(model: type | None, entity_id: str) -> str: +def pk_is_redacted(model: type | None) -> bool: + """True when a primary-key column of ``model`` is itself a redacted credential.""" fields = getattr(model, "__audit_redact_fields__", None) if model is None or not fields: - return entity_id - if any(col.key in fields for col in inspect(model).primary_key): - return AUDIT_REDACTED - return entity_id + return False + return any(col.key in fields for col in inspect(model).primary_key) + + +def redact_entity_id(model: type | None, entity_id: str) -> str: + return AUDIT_REDACTED if pk_is_redacted(model) else entity_id diff --git a/framework/db/simple_module_db/bulk_audit.py b/framework/db/simple_module_db/bulk_audit.py index d652f2767..c9c6a16af 100644 --- a/framework/db/simple_module_db/bulk_audit.py +++ b/framework/db/simple_module_db/bulk_audit.py @@ -86,7 +86,7 @@ def _plan(execute_state: ORMExecuteState, cls: type) -> _Ctx | None: """The record shape for this statement, or ``None`` when nothing is audited.""" name = cls.__name__ if execute_state.is_delete: - return _Ctx(name, ACTION_BULK_DELETED, []) + return _Ctx(name, ACTION_BULK_DELETED, [], cls) rows, many = _targets(execute_state, cls) default = ACTION_BULK_INSERTED if execute_state.is_insert else ACTION_BULK_UPDATED # The soft-delete flag picks a row's action; it is never a change, and @@ -113,7 +113,7 @@ def _plan(execute_state: ORMExecuteState, cls: type) -> _Ctx | None: changes = [{"field": k} for k in kept] else: changes = [{"field": k, "new": _shown(rows[0][k])} for k in kept] - ctx = _Ctx(name, actions[0] if actions else default, redact_changes(cls, changes)) + ctx = _Ctx(name, actions[0] if actions else default, redact_changes(cls, changes), cls) ctx.row_actions = actions or [default] return ctx diff --git a/framework/db/simple_module_db/listeners.py b/framework/db/simple_module_db/listeners.py index e0be095ac..9a840abd0 100644 --- a/framework/db/simple_module_db/listeners.py +++ b/framework/db/simple_module_db/listeners.py @@ -13,6 +13,7 @@ from sqlalchemy.orm import Session from simple_module_db._persisted_owner import mark_unverified, owner_mark, restore_owner_mark +from simple_module_db.audit_redaction import AUDIT_REDACTED, pk_is_redacted from simple_module_db.bulk_audit import audit_statement from simple_module_db.bulk_guard import install_bulk_guard from simple_module_db.flush_guard import guard_flush @@ -76,8 +77,14 @@ def _entity_label(obj: object) -> str: def _entity_pk(obj: object) -> object: - """Return the primary key value(s) if available, else None.""" + """Return the primary key value(s) if available, else None. + + A primary key that is itself a credential (a session or refresh token) is + masked: this value goes to the application log. + """ try: + if pk_is_redacted(type(obj)): + return AUDIT_REDACTED inspector = sa_inspect(obj) if inspector is None: return None diff --git a/framework/db/tests/test_db_credential_audit.py b/framework/db/tests/test_db_credential_audit.py index 30cd544c4..c429cb380 100644 --- a/framework/db/tests/test_db_credential_audit.py +++ b/framework/db/tests/test_db_credential_audit.py @@ -1,16 +1,12 @@ """Credential changes remain visible without recording their values.""" -import uuid from datetime import UTC, datetime import pytest -from auth.models import RefreshOutcome from simple_module_db.audit import collect_audit_records from simple_module_db.leases import LeaseRow from simple_module_db.stored_settings import Setting from sqlalchemy.orm import Session -from tenants.models import Invitation -from users.models import OAuthAccount, RefreshToken, User, UserAccessToken @pytest.mark.parametrize( @@ -30,57 +26,6 @@ def test_setting_create_redaction(key, value, value_type, expected): assert next(c["new"] for c in record.changes if c["field"] == "value") == expected -@pytest.mark.parametrize( - "entity,fields", - [ - (User(email="audit@example.test", hashed_password="private"), {"hashed_password"}), - ( - OAuthAccount( - user_id=uuid.uuid4(), - access_token="private", - refresh_token="private", - oauth_name="test", - account_id="1", - account_email="a@test", - ), - {"access_token", "refresh_token"}, - ), - ( - Invitation( - tenant_id="default", - email="invitee@example.test", - token_hash="private", - expires_at=datetime.now(UTC), - ), - {"token_hash"}, - ), - ( - RefreshOutcome( - outcome_key="private", - lock_key="private", - expires_at=1, - session={"token": "private"}, - ), - {"lock_key", "session"}, - ), - ], -) -def test_model_credentials_are_redacted(entity, fields): - with Session() as session: - session.add(entity) - record = collect_audit_records(session)[0] - changes = {c["field"]: c["new"] for c in record.changes} - assert {field: changes[field] for field in fields} == dict.fromkeys(fields, "[redacted]") - - -@pytest.mark.parametrize("cls", [UserAccessToken, RefreshToken]) -def test_token_primary_key_is_redacted(cls): - entity = cls(token="private", user_id=uuid.uuid4(), expires_at=datetime.now(UTC)) - with Session() as session: - session.add(entity) - assert collect_audit_records(session)[0].entity_id == "[redacted]" - - @pytest.fixture def credential_session(): from simple_module_db.listeners import attach_session_listeners @@ -100,25 +45,20 @@ class AuditSession(Session): attach_session_listeners(AuditSession) try: Setting.metadata.create_all(engine) - User.metadata.create_all(engine) with AuditSession(engine) as session: yield session, records finally: engine.dispose() -@pytest.mark.parametrize("kind", ["password", "setting", "renamed", "url"]) +@pytest.mark.parametrize("kind", ["setting", "renamed", "url"]) def test_credential_update_redacts_both_sides(credential_session, kind): session, records = credential_session - if kind == "password": - entity = User(email="update@test", hashed_password="old-private") - field = "hashed_password" - else: - entity = Setting( - key="mail.url" if kind == "url" else "mail.smtp_password", - value="smtp://u:private@localhost" if kind == "url" else "old-private", - ) - field = "value" + entity = Setting( + key="mail.url" if kind == "url" else "mail.smtp_password", + value="smtp://u:private@localhost" if kind == "url" else "old-private", + ) + field = "value" session.add(entity) session.flush() records.clear() @@ -138,16 +78,11 @@ def test_bulk_credentials_redacted(credential_session, core): from sqlalchemy import update session, records = credential_session - user = User(email="bulk@test", hashed_password="old-private") - setting = Setting(key="mail.smtp_password", value="old-private") - session.add_all([user, setting]) + session.add(Setting(key="mail.smtp_password", value="old-private")) session.flush() records.clear() - for model, field in [(User, "hashed_password"), (Setting, "value")]: - target = model.__table__ if core else model - session.execute(update(target).values({field: "new-private"})) - for record, field in zip(records, ["hashed_password", "value"], strict=True): - assert record.changes == [{"field": field, "new": "[redacted]"}] + session.execute(update(Setting.__table__ if core else Setting).values(value="new-private")) + assert records[0].changes == [{"field": "value", "new": "[redacted]"}] @pytest.mark.parametrize("by_name", [False, True]) @@ -156,9 +91,9 @@ def test_manual_bulk_redaction(credential_session, by_name): session, records = credential_session record_bulk_audit( - session, "User" if by_name else User, "bulk_updated", ["1"], {"hashed_password": "private"} + session, "Setting" if by_name else Setting, "bulk_updated", ["1"], {"value": "private"} ) - assert records[0].changes == [{"field": "hashed_password", "new": "[redacted]"}] + assert records[0].changes == [{"field": "value", "new": "[redacted]"}] def test_nonsecret_setting_update_retains_values(credential_session): @@ -196,28 +131,6 @@ def test_bulk_insert_and_summary_redaction(credential_session, monkeypatch, core assert records[0].changes[0] == {"field": "value", "new": "[redacted]"} -@pytest.mark.parametrize("cls", [UserAccessToken, RefreshToken]) -def test_token_identity_redacted_on_update_delete_and_bulk(credential_session, cls): - from simple_module_db import record_bulk_audit - from sqlalchemy import delete, update - - session, records = credential_session - user = User(email="token-owner@test") - session.add(user) - session.flush() - token = cls(token="private", user_id=user.id, expires_at=datetime.now(UTC)) - session.add(token) - session.flush() - records.clear() - token.expires_at = datetime(2030, 1, 1, tzinfo=UTC) - session.flush() - session.execute(update(cls).values(expires_at=datetime(2031, 1, 1, tzinfo=UTC))) - record_bulk_audit(session, cls.__name__, "bulk_updated", ["private"]) - session.execute(delete(cls)) - assert len(records) == 4 - assert all(r.entity_id == "[redacted]" for r in records) - - def test_typed_setting_redaction(): from pydantic import SecretStr from simple_module_core.secret_fields import register_typed_secrets @@ -234,14 +147,18 @@ class CredentialSettings(SQLModel): def test_manual_bulk_honors_field_exclusions(credential_session): - from api_tokens.models import ApiToken + from _audit_models import PartialExcludeModel from simple_module_db import record_bulk_audit session, records = credential_session record_bulk_audit( - session, ApiToken, "bulk_updated", ["1"], {"token_hash": "private", "name": "visible"} + session, + PartialExcludeModel, + "bulk_updated", + ["1"], + {"password_hash": "private", "username": "visible"}, ) - assert records[0].changes == [{"field": "name", "new": "visible"}] + assert records[0].changes == [{"field": "username", "new": "visible"}] def test_lease_ownership_value_is_redacted(): diff --git a/framework/db/tests/test_db_credential_log_masking.py b/framework/db/tests/test_db_credential_log_masking.py new file mode 100644 index 000000000..edd5133f7 --- /dev/null +++ b/framework/db/tests/test_db_credential_log_masking.py @@ -0,0 +1,16 @@ +"""A credential used as a primary key never reaches the entity-change log.""" + +import uuid + +from simple_module_db.listeners import _entity_pk +from users.models import RefreshToken, User + + +def test_token_primary_key_is_masked_in_log_identity(): + token = RefreshToken(token="private", user_id=uuid.uuid4()) + assert _entity_pk(token) == "[redacted]" + + +def test_ordinary_primary_key_is_kept(): + user = User(email="log@test") + assert _entity_pk(user) != "[redacted]" diff --git a/host/migrations/versions/b8d4f2a6c901_redact_audit_credentials.py b/host/migrations/versions/b8d4f2a6c901_redact_audit_credentials.py index e17f4f76b..076fe3251 100644 --- a/host/migrations/versions/b8d4f2a6c901_redact_audit_credentials.py +++ b/host/migrations/versions/b8d4f2a6c901_redact_audit_credentials.py @@ -59,11 +59,12 @@ def upgrade() -> None: elif isinstance(changes, dict): changes = {k: _REDACTED if k in fields else v for k, v in changes.items()} identity = _REDACTED if row["entity_type"] in _REDACT_IDS else row["entity_id"] - conn.execute( - audit.update() - .where(audit.c.id == row["id"]) - .values(changes=changes, entity_id=identity) - ) + if changes != row["changes"] or identity != row["entity_id"]: + conn.execute( + audit.update() + .where(audit.c.id == row["id"]) + .values(changes=changes, entity_id=identity) + ) last_id = rows[-1]["id"] diff --git a/modules/audit_log/README.md b/modules/audit_log/README.md index 9e71ba308..483158b85 100644 --- a/modules/audit_log/README.md +++ b/modules/audit_log/README.md @@ -17,6 +17,7 @@ Add `simple_module_audit_log` to your host's dependencies and the entry point wi - `GET /api/audit_log/` REST endpoint with `entity_type`, `entity_id`, `action`, `user_id`, `from_date`, `to_date`, paging filters. - `/audit_log` Inertia browse page with the same filter set rendered as a table, gated by the `audit_log.view` permission. - Opt-outs: set `__audit_exclude__ = True` on a model to skip it entirely, or `__audit_exclude_fields__ = {"password_hash"}` to skip specific columns. `AuditMixin` / `SoftDeleteMixin` bookkeeping fields are excluded automatically. +- Credential redaction: set `__audit_redact_fields__` (or a `__audit_redact_row__` predicate) to keep a field in the diff with its values replaced by `[redacted]`; see the [audit_log docs](https://github.com/antosubash/simple_module_python/blob/main/docs/modules/audit_log.md) for details. ## Usage @@ -52,33 +53,3 @@ class Cache(Base, table=True): ## License MIT - -## Credential redaction - -Set `__audit_redact_fields__: ClassVar[set[str]] = {"credential"}` on a -model to keep a field in the audit diff while replacing every recorded `old` -and `new` value with `[redacted]`. Unlike `__audit_exclude_fields__`, which -omits a field entirely, redaction preserves the fact that it changed. -Exclusion takes precedence. A redacted primary key also masks `entity_id`. - -For row-dependent rules, define a static `__audit_redact_row__(row)` method -returning a set of field names. ORM capture evaluates the current and pre-edit -column dictionaries and redacts fields identified by either. Bulk and manual -capture pass `None`; the predicate must conservatively return every field -that could hold a credential when row context is unavailable. Manual capture -accepts the mapped class or its registered class name to apply its policy. - -Built-in policies cover stored secret settings, user password hashes, OAuth -credentials, tenant invitation hashes, bearer tokens, lease ownership tokens -and auth refresh outcomes. -Settings use the same secret rule as the settings UI. Non-secret ORM settings -retain their values; bulk settings values are always redacted because the -statement may affect rows with different keys or prior values. API token -hashes remain excluded from automatic diffs. - -Migration `b8d4f2a6c901` hardens existing entries, conservatively redacting all -historical settings values because old diffs may lack the key or type needed -to classify them. It also masks historical credential primary-key identities. -Its downgrade is a no-op: credential values cannot be recovered. Operators -should rotate credentials saved through the settings UI on earlier versions; -the migration does not rewrite backups or previously exported audit data. diff --git a/modules/auth/tests/test_auth_audit_redaction.py b/modules/auth/tests/test_auth_audit_redaction.py new file mode 100644 index 000000000..19ef5be9d --- /dev/null +++ b/modules/auth/tests/test_auth_audit_redaction.py @@ -0,0 +1,18 @@ +"""Refresh-outcome credentials are redacted from audit diffs.""" + +from auth.models import RefreshOutcome +from simple_module_db.audit import collect_audit_records +from sqlalchemy.orm import Session + + +def test_refresh_outcome_credentials_are_redacted(): + with Session() as session: + session.add( + RefreshOutcome( + outcome_key="private", lock_key="private", expires_at=1, session={"token": "x"} + ) + ) + changes = {c["field"]: c["new"] for c in collect_audit_records(session)[0].changes} + assert {f: changes[f] for f in ("lock_key", "session")} == dict.fromkeys( + ("lock_key", "session"), "[redacted]" + ) diff --git a/modules/settings/settings/_row_masking.py b/modules/settings/settings/_row_masking.py index 75cd8291d..497778981 100644 --- a/modules/settings/settings/_row_masking.py +++ b/modules/settings/settings/_row_masking.py @@ -19,8 +19,8 @@ def is_masked(entity: Setting) -> bool: """Whether this row's stored value must not leave the service. - :data:`SENSITIVE_KEYS` names the one key the hosting layer owns - (``host.secret_key``). Everything else is judged by the same name/value + ``conceals_secret`` covers the one key the hosting layer owns + (``host.secret_key``) and judges everything else by the same name/value rule the module editor uses, because the store is the *same data* seen through a different screen: an override named ``mail.smtp_password``, or any override holding a ``postgresql://user:pw@host/db``, was rendered in diff --git a/modules/settings/settings/constants.py b/modules/settings/settings/constants.py index d86233f9a..d299cbf80 100644 --- a/modules/settings/settings/constants.py +++ b/modules/settings/settings/constants.py @@ -10,8 +10,6 @@ from typing import Final -from simple_module_core.secret_fields import SENSITIVE_KEYS as SENSITIVE_KEYS - # Re-exported (#500): these now live with the table in the framework. from simple_module_db.stored_settings import ( # noqa: F401 ALL_VALUE_TYPES, @@ -47,7 +45,7 @@ ALL_SCOPES: Final = (SCOPE_SYSTEM, SCOPE_TENANT, SCOPE_USER) # ── Secrets ────────────────────────────────────────────────────────── -# ``SENSITIVE_KEYS`` is re-exported from ``simple_module_core.secret_fields``. +# The sensitive-key allowlist lives in ``simple_module_core.secret_fields``. SENSITIVE_PLACEHOLDER: Final = "********" # ── Routing ────────────────────────────────────────────────────────── diff --git a/modules/tenants/tests/test_tenants_audit_redaction.py b/modules/tenants/tests/test_tenants_audit_redaction.py new file mode 100644 index 000000000..001c280ec --- /dev/null +++ b/modules/tenants/tests/test_tenants_audit_redaction.py @@ -0,0 +1,21 @@ +"""Invitation token hashes are redacted from audit diffs.""" + +from datetime import UTC, datetime + +from simple_module_db.audit import collect_audit_records +from sqlalchemy.orm import Session +from tenants.models import Invitation + + +def test_invitation_token_hash_is_redacted(): + with Session() as session: + session.add( + Invitation( + tenant_id="default", + email="invitee@example.test", + token_hash="private", + expires_at=datetime.now(UTC), + ) + ) + changes = {c["field"]: c["new"] for c in collect_audit_records(session)[0].changes} + assert changes["token_hash"] == "[redacted]" diff --git a/modules/users/tests/test_users_audit_redaction.py b/modules/users/tests/test_users_audit_redaction.py new file mode 100644 index 000000000..32ccde63e --- /dev/null +++ b/modules/users/tests/test_users_audit_redaction.py @@ -0,0 +1,118 @@ +"""User credentials and token identities are redacted from audit capture.""" + +import uuid +from datetime import UTC, datetime + +import pytest +from simple_module_db import record_bulk_audit +from simple_module_db.audit import collect_audit_records +from simple_module_db.listeners import attach_session_listeners +from simple_module_db.query_filter import EngineTenancy, bind_engine_policy +from sqlalchemy import create_engine, delete, update +from sqlalchemy.orm import Session +from users.models import OAuthAccount, RefreshToken, User, UserAccessToken + +TOKENS = [UserAccessToken, RefreshToken] + + +@pytest.fixture +def credential_session(): + engine = create_engine("sqlite://") + records = [] + bind_engine_policy( + engine, + EngineTenancy( + tenant_strict=False, audit_callback=lambda session, batch: records.extend(batch) + ), + ) + + class AuditSession(Session): + pass + + attach_session_listeners(AuditSession) + try: + User.metadata.create_all(engine) + with AuditSession(engine) as session: + yield session, records + finally: + engine.dispose() + + +def _created(entity): + with Session() as session: + session.add(entity) + return collect_audit_records(session)[0] + + +def test_user_and_oauth_credentials_are_redacted(): + user = { + c["field"]: c["new"] for c in _created(User(email="a@test", hashed_password="x")).changes + } + assert user["hashed_password"] == "[redacted]" + oauth = OAuthAccount( + user_id=uuid.uuid4(), + access_token="x", + refresh_token="x", + oauth_name="test", + account_id="1", + account_email="a@test", + ) + changes = {c["field"]: c["new"] for c in _created(oauth).changes} + assert changes["access_token"] == changes["refresh_token"] == "[redacted]" + + +@pytest.mark.parametrize("cls", TOKENS) +def test_token_primary_key_is_redacted(cls): + token = cls(token="private", user_id=uuid.uuid4(), expires_at=datetime.now(UTC)) + assert _created(token).entity_id == "[redacted]" + + +def test_password_update_redacts_both_sides(credential_session): + session, records = credential_session + user = User(email="update@test", hashed_password="old-private") + session.add(user) + session.flush() + records.clear() + user.hashed_password = "new-private" + session.flush() + assert records[0].changes == [ + {"field": "hashed_password", "old": "[redacted]", "new": "[redacted]"} + ] + + +def test_bulk_password_update_redacted(credential_session): + session, records = credential_session + session.add(User(email="bulk@test", hashed_password="old-private")) + session.flush() + records.clear() + session.execute(update(User).values(hashed_password="new-private")) + session.execute(update(User.__table__).values(hashed_password="newer-private")) + assert [r.changes for r in records] == [[{"field": "hashed_password", "new": "[redacted]"}]] * 2 + + +@pytest.mark.parametrize("by_name", [False, True]) +def test_manual_bulk_redaction(credential_session, by_name): + session, records = credential_session + record_bulk_audit( + session, "User" if by_name else User, "bulk_updated", ["1"], {"hashed_password": "private"} + ) + assert records[0].changes == [{"field": "hashed_password", "new": "[redacted]"}] + + +@pytest.mark.parametrize("cls", TOKENS) +def test_token_identity_redacted_on_update_delete_and_bulk(credential_session, cls): + session, records = credential_session + user = User(email="token-owner@test") + session.add(user) + session.flush() + token = cls(token="private", user_id=user.id, expires_at=datetime.now(UTC)) + session.add(token) + session.flush() + records.clear() + token.expires_at = datetime(2030, 1, 1, tzinfo=UTC) + session.flush() + session.execute(update(cls).values(expires_at=datetime(2031, 1, 1, tzinfo=UTC))) + record_bulk_audit(session, cls.__name__, "bulk_updated", ["private"]) + session.execute(delete(cls)) + assert len(records) == 4 + assert all(r.entity_id == "[redacted]" for r in records) From 18b9cd9691aefb1bcc447e2a34dbb0ffb3dce935 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 5 Nov 2026 15:47:00 +0100 Subject: [PATCH 5/5] fix: address code review findings (round 1, pass 2) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/core/simple_module_core/secret_fields.py | 4 ++-- framework/db/simple_module_db/stored_settings.py | 1 + modules/auth/tests/test_auth_audit_redaction.py | 4 +--- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/framework/core/simple_module_core/secret_fields.py b/framework/core/simple_module_core/secret_fields.py index 5da99b7ee..20801be24 100644 --- a/framework/core/simple_module_core/secret_fields.py +++ b/framework/core/simple_module_core/secret_fields.py @@ -22,8 +22,8 @@ ) SECRET_MASK = "••••••••" -# Keys whose stored value must never leave the settings API or the raw -# key/value browse screen. ``host.secret_key`` is the session-signing key: the +# Keys whose stored value must never leave the settings API, the raw +# key/value browse screen or the audit trail. ``host.secret_key`` is the session-signing key: the # hosting layer generates and persists it here when SM_SECRET_KEY is unset, and # anyone who can read it can forge a session cookie for any account. Masked on # the way out only — the boot-time reader goes straight to SQL, so hiding it diff --git a/framework/db/simple_module_db/stored_settings.py b/framework/db/simple_module_db/stored_settings.py index 2555047e7..94fff4887 100644 --- a/framework/db/simple_module_db/stored_settings.py +++ b/framework/db/simple_module_db/stored_settings.py @@ -87,6 +87,7 @@ class Setting(Base, AuditMixin, table=True): # ty: ignore[unsupported-base] @staticmethod def __audit_redact_row__(row: dict | None) -> set[str]: + # Fail closed: no row (bulk/manual capture) or a partly loaded one hides the value. if ( row is None or any(row.get(k) is None for k in (COL_KEY, COL_VALUE, COL_VALUE_TYPE)) diff --git a/modules/auth/tests/test_auth_audit_redaction.py b/modules/auth/tests/test_auth_audit_redaction.py index 19ef5be9d..88e65889b 100644 --- a/modules/auth/tests/test_auth_audit_redaction.py +++ b/modules/auth/tests/test_auth_audit_redaction.py @@ -13,6 +13,4 @@ def test_refresh_outcome_credentials_are_redacted(): ) ) changes = {c["field"]: c["new"] for c in collect_audit_records(session)[0].changes} - assert {f: changes[f] for f in ("lock_key", "session")} == dict.fromkeys( - ("lock_key", "session"), "[redacted]" - ) + assert changes["lock_key"] == changes["session"] == "[redacted]"