fix(audit_log): gate entity labels on the owning module's permission - #308
Merged
Conversation
The audit table stopped showing bare uuids by asking each module to name its own rows, and the users module answers with `full_name or email`. Nothing checked whether the reader was allowed that answer, so `audit_log.view` on its own became a second, unguarded read of the user directory: page the log and collect the display name of every account that has ever been edited — and, for any account still holding an unaccepted invite, its email address, because those have no `full_name` yet and the resolver falls through to the email. The CSV export had the same hole in its `entity_label` column, and the users edit page's activity card resolved through the same helper. Naming a row is a read of that row, so it now takes that row's permission. `AuditLink` gains `label_permission`; `resolve_entity_labels` takes the requesting principal's grants and skips — before dispatch, so the resolver never even queries — any entity type whose owner declared a permission the reader lacks. `users` declares `users.manage`, the same permission that opens the user list at the front door. Everything else is left ungated by default: a setting key, a filename, a task name and a flag name are what changed, not who somebody is, and withholding them would blind a legitimate auditor for nothing. The entry itself is never withheld. A reader without `users.manage` still sees that a `User` was updated, when, by whom, which id and which fields — that is the audit trail, and an auditor who cannot see it is not an auditor. The actor column stays ungated, and that is the deliberate half of the answer the issue asks for: an audit trail that will not say who acted is not one. The two columns disclose the same field and answer different questions — the entity column names people who were merely *edited*. `docs/modules/audit_log.md` now says so outright under § Entity and actor names, so `audit_log.view` is granted knowing what it discloses. Supporting changes: - `simple_module_core.permissions.grants()` — one place for the wildcard rule, so gating part of a *response* reads a grant the way `RequiresPermission` reads it at the door. A hand-rolled `in` check forgets that `admin` holds `*` and no named permission at all; `RequiresPermission` now routes through it. - `simple_module_hosting.permissions.resolved_permissions_for(request)` — extracted from `RequiresPermission.__call__`, which already had this middleware-cache-or-resolve dance inline. - `Browse.tsx` was 286 lines against the 300-line cap, so the table and the pager move to `components/EntriesTable.tsx` and `components/Pager.tsx`. Pure extraction, no behaviour change — it is now 168 lines, and the next change to this screen is a change rather than a forced split. Known adjacent surface, deliberately not addressed here: the `changes` column records the values that were written, so a `User` create entry still contains the email that was set. That is the audit trail doing its job rather than a label leaking, it needs its own decision about redaction versus `__audit_exclude_fields__`, and folding it into this fix would have changed what the log records rather than who may read a name. Closes #300
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Deploying simple-module-python with
|
| Latest commit: |
0cf35e4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7446e667.simple-module-python.pages.dev |
| Branch Preview URL: | https://fix-audit-entity-labels.simple-module-python.pages.dev |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #300
The problem
The audit table stopped showing bare uuids by asking each module to name its own rows (
resolve_entity_labels→ the owning module'sAuditLink.label_resolver), and the users module answers withfull_name or email. Nothing checked whether the reader was allowed that answer.The audit views are gated on
audit_log.viewalone, so a role granted audit access and nothing else could:full_nameyet, and the resolver falls through to the email;GET /api/audit_log/export.csv, whoseentity_labelcolumn runs the identical resolver.That is a back door onto the user directory that
users.manageguards at the front. The users edit page's "Recent activity" card resolves through the same helper.The fix
Naming a row is a read of that row, so it now takes that row's permission.
AuditLinkgainslabel_permission: str = "". A module sets it when naming its rows discloses something it gates elsewhere.resolve_entity_labels(db, registry, refs, permissions)takes the requesting principal's grants and skips any entity type whose owner declared a permission the reader lacks. Skipped before dispatch — a resolver that may not answer must not get to query either.usersdeclareslabel_permission="users.manage"— the same permission that opens the user list.The entry itself is never withheld. A reader without
users.managestill sees that aUserwas updated, when, by whom, which id, which fields, and the link to the record (whose own route enforces its own permission). They just do not get the name.The actor column stays ungated, and that is the deliberate half of the answer #300 asks for: an audit trail that will not say who acted is not one. The two columns disclose the same field and answer different questions — the entity column names people who were merely edited, which reviewing the trail does not require.
docs/modules/audit_log.mdnow states this outright under a new § Entity and actor names, soaudit_log.viewis granted knowing what it discloses.Everything else is ungated by default. A setting key, a filename, a task name, a flag name are what changed, not who somebody is — gating them would blind a legitimate auditor for nothing. The mechanism is there for the module that needs it.
Supporting changes
simple_module_core.permissions.grants(held, required)— one place for the wildcard rule, so code gating part of a response reads a grant the wayRequiresPermissionreads it at the door. A hand-rolledincheck forgets thatadminholds*and no named permission at all.RequiresPermissionnow routes through it.simple_module_hosting.permissions.resolved_permissions_for(request)— extracted fromRequiresPermission.__call__, which already had this middleware-cache-or-resolve dance inline.Browse.tsxsplit (the issue's second bullet): it was 286 lines against the 300-line cap. The table and the pager move tocomponents/EntriesTable.tsx(121) andcomponents/Pager.tsx(48);Browse.tsxis now 168. Pure extraction, no behaviour change.docs/modules/audit_log.md,docs/modules/users.md,docs/framework/lifecycle.md(register_audit_links),docs/framework/permissions.md.Tests
modules/audit_log/tests/test_entity_label_permission.py— 12 tests covering the permitted and the denied caller, built the long way (real account, real role, forged session cookie) rather than by stubbing the permission set, so a view that consults nothing cannot pass:audit_log.view-only reader gets the id on the browse page and in the CSV;full_name) does not leak the email through the entity column;Setting) is still named for them;users.manageand theadminwildcard both see the name;*, and does not run the resolver at all.Verified failing before, passing after: with
label_permissionreverted to"", 3 of the end-to-end tests fail; with thegrants()check inresolve_entity_labelsdisabled, 5 fail. Both restored, all 12 pass.Also added:
AuditLink.label_permissiondefaults/conflict tests (framework/core/tests/test_audit_links.py) andgrants()tests (framework/core/tests/test_permissions.py).Known adjacent surface, deliberately not addressed
The
changescolumn records the values that were written, so aUsercreate entry still contains the email that was set — readable by anyaudit_log.viewholder. That is the audit trail doing its job rather than a label leaking; it needs its own decision (redaction vs.__audit_exclude_fields__on the model) and folding it in here would change what the log records rather than who may read a name. Called out in the new docs section and in the test that brushes against it.Verification
uv run pytest -q(full suite)uv run pytest modules/audit_log -quv run pytest modules/users framework/core framework/hosting -quv run ruff format --check .uv run ruff check .uv run ty check framework modules hostuv run python scripts/check_file_size.pynpx tsc --noEmit -p modules/audit_lognpx vitest run modules/audit_log/tests-jsmake ci-check-untranslateduv run python -m simple_module_core(doctor)Biome could not be run locally: the checked-in
biome.jsonuseslinter.rules.preset, which the installed Biome 2.4.13 rejects as an unknown key (package.jsonasks for^2.5.7). This is pre-existing onmain— it fails identically on untouched files. The changed.tsxfiles were formatted with an equivalent config against the same binary, so CI's Biome job should be clean.