Harden the audit trail: origin-only destination targets, atomic SSO provisioning row

[richardmhope]

Summary

The review of #351 approved and merged it with two medium findings against invariants that PR itself set. Both are real; this fixes them. No linked issue — the parent issues (#33, #34) closed with #351.

1. Destination targets were recorded verbatim in audit detail (alerts.py, FYI · medium). A Slack or Teams incoming-webhook URL is a capability token — the path is the credential — and the trail is readable by auditors and retained forever, including after the destination is deleted. destination.create / update / delete now record the target's origin (scheme + host) via a new _audit_target() and never the path or query. Non-URL targets (email recipients) are not credentials and are kept as-is, so the trail still answers "where did alerts go".

2. user.provision was not atomic with the JIT insert (oidc/service.py, follow-up · medium). The insert committed, then the audit row was staged and committed separately — the one place #351 broke its own "row commits with the change" rule, done to survive the username-collision retry. A crash between the two commits would leave a durable account with no trail entry, and the returning-user path never records it. The row is now staged after the insert is flushed (so it carries the id) and lands in the same commit, on both the first attempt and the retry: the unique-violation rollback discards the user and the staged row together, so the retry starts clean and exactly one row exists naming the final username.

Test evidence

$ uv run pytest -q -p no:warnings                    1054 passed in 104.30s
$ uvx ruff@0.16.5 check app tests e2e                All checks passed!
$ uvx ruff@0.16.5 format --check app tests e2e alembic   147 files already formatted
$ uv run mypy app                                    Success: no issues found in 65 source files
$ uvx vulture@2.16 app vulture_whitelist.py          exit=0
$ uv run bandit -q -c pyproject.toml -r app          (no findings)

Two new/extended tests, one per finding:

• test_destination_audit_records_origin_not_capability_url — creates a Slack destination with target https://hooks.slack.com/services/T000/B000/SECRETTOKENxyz, renames and deletes it, then asserts all three rows record target == "https://hooks.slack.com" and that neither SECRETTOKEN nor /services/ appears anywhere in the stored detail. Also pins _audit_target on an email-recipient list (unchanged) and a Jira URL with path + query (origin only).
• test_recovery_widens_username_suffix_on_collision (extended) — the existing retry test, which forces the first insert to collide on username, now also asserts that exactly one user.provision row exists and that it names the final (width-32) username and id — i.e. the first attempt's staged row was rolled back with the failed insert, not leaked.

The existing test_every_mutating_route_is_audited_or_allowlisted and test_audit_detail_never_carries_a_secret_shaped_key guards still pass; the second one doesn't catch a target key by design (it's not credential-shaped), which is why the origin reduction lives in code rather than a key rename.

Checklist

• The four CI gates pass locally — output above.
• Tests added — one regression test per finding (above).
• CHANGELOG.md — the Audit log of mutating actions #34 entry's "detail never carries secrets" line now names the origin-only rule for destinations.
• CLAUDE.md — the audit-log detail-hygiene note records _audit_target and why.
• app/templates/help.html — the audit-log section's secrets bullet mentions destinations.
• uv lock — n/a, no dependency change.
• No secrets, credentials, or internal hostnames in the diff or the logs above (the test's webhook token is a placeholder string).

🤖 Generated with Claude Code

https://claude.ai/code/session_01XY2M1RbUVgATMDceLubuBz

---

Generated by Claude Code

[icebergai-review-bot]

Verdict

CHANGES_REQUESTED

Completed bounded review across 1 immutable scope(s). One security issue found in the new audit-target redaction.

Scope health

Convergence: healthy. Review mode: initial.
Recommended action: CONTINUE_INCREMENTAL.

• No escalation signals.

Prior findings

Finding	Status
—	No prior finding state

New findings

Root cause: Using netloc, which contains userinfo, instead of constructing the origin from hostname and optional port.

• BLOCKER · high: Audit redaction retains URL userinfo credentials — app/routes/alerts.py:43
  Status: NEW. Attribution: new_in_scope.
  urlsplit().netloc includes URL userinfo, so this is not an origin-only representation. A destination such as https://api-user:api-password@alerts.example.test/webhook is recorded as https://api-user:api-password@alerts.example.test. The credential is then retained in the audit trail and exposed to audit readers.
  Invariant: Audit details must never include destination credentials and must record only scheme, host, and optional port.
  Ownership: Alert destination audit-detail sanitization.. Behaviour: Audit entries for URL-based alert destinations..
  Evidence: _audit_target returns f"{parts.scheme}://{parts.netloc}"; create, update, and delete audit records store that result as target.
  Independent assessment: urlsplit(...).netloc retains URL userinfo. _audit_target writes that netloc into audit records on create, update, and delete, so https://user:password@host/... exposes credentials in the retained audit trail.

Fix-induced regressions

• None evidenced.

Uncertainty

• No material uncertainty recorded.

Validation

• Reviewed the supplied immutable diff and associated tests.
• No prior findings were supplied; no rechecks are applicable.

Residual risks

• None identified.

[icebergai-review-bot]

Verdict

APPROVE

Completed bounded review across 1 immutable scope(s). No remaining actionable findings. The supplied change removes the prior audit credential disclosure.

Scope health

Convergence: healthy. Review mode: delta.
Recommended action: CONTINUE_INCREMENTAL.

• No escalation signals.

Prior findings

Finding	Status
F-5D83B294435B	Resolved

New findings

No new findings.

Fix-induced regressions

• None evidenced.

Uncertainty

• No material uncertainty recorded.

Validation

• Prior blocker F-5D83B294435B resolved: The supplied tests assert userinfo-free output for hostname, port, and IPv6 URL forms, plus that credentials do not reach route-created audit rows.
• Reviewed the supplied immutable diff.
• Exact-head CI was reported as passed.
• The new _audit_target constructs URL audit targets from hostname and optional port, excluding userinfo; tests cover userinfo, ports, IPv6, and route-level audit behavior.

Residual risks

• None identified.