fix(security): redact sensitive logging + refurb cleanups (cluster A)#1814
Conversation
always_allow.py:294,302 - CodeQL py/clear-text-logging-sensitive-data flagged both logger.error(reason) sites because the reason string embeds rules_path/manifest_path, both transitively derived from the BERNSTEIN_ALWAYS_ALLOW_PATH env value (a tainted source). The full reason still goes to .sdd/metrics/guardrails.jsonl via _record_tamper_event (already digest-sanitized) for operator forensic review; the application-log line is now a static safety message that names the failure category and points operators at the metrics file. This preserves debuggability without leaking the env-derived path to the log stream. audit_chain.py:122 - FURB123: dict(details) -> details.copy() for an already-typed dict[str, Any] input. task_lifecycle.py:568 - FURB143: drop the redundant `or ""` since original_error is typed str. jwt_tokens.py:158-159 - FURB184: chain the cast and .get() call; header_dict had no other users.
There was a problem hiding this comment.
Sorry @chernistry, you have reached your weekly rate limit of 2500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR applies five small maintenance refinements across security modules, task lifecycle telemetry, and test documentation. Security error logging favors operator-friendly generic messages over detailed paths, audit event copying simplifies to ChangesSecurity and Telemetry Refinements
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Sonar insights (advisory, no merge-block)Snapshot of
Run This comment is a soft signal. The Sonar scan runs on push to |
Review-bot acknowledgement summary
All must-address findings are resolved or acknowledged. |
|
bernstein doctor observe for PR #1814 ( sonar -- WARN (project bernstein)
code-scanning -- WARN (33 open alert(s))
Skipped backends (credentials not configured)
See docs/observability/unified-doctor.md for backend setup notes. |
Auto-applied by contract-drift-autofix.yml on PR #1814. Regenerated via scripts/regen_contract_drift.py. Refs #1273. Source CI run: https://github.com/sipyourdrink-ltd/bernstein/actions/runs/26253016689
The prior commit (contract-drift allow-list regen) was authored by github-actions[bot], so GitHub suppressed the pull_request workflow triggers and the required `CI gate` context never published, leaving the PR BLOCKED with no failing check. This empty commit re-runs the pull_request workflows so the gate can report.
Summary
Closes #2446 #2447 (CodeQL
py/clear-text-logging-sensitive-data, HIGH). Also fixes three refurb findings in the same cluster.src/bernstein/core/security/always_allow.py:294logger.error(reason)embedded env-var-derived paths (rules_path,manifest_path). Switched to a static safety message; full path-bearing reason still goes to.sdd/metrics/guardrails.jsonlvia_record_tamper_eventfor operator forensic review.src/bernstein/core/security/always_allow.py:302src/bernstein/core/security/audit_chain.py:122dict(details)->details.copy()for an already-typeddict[str, Any]input.src/bernstein/core/tasks/task_lifecycle.py:568or ""sinceoriginal_erroris typedstr.src/bernstein/core/security/jwt_tokens.py:159cast()and.get()call; the intermediateheader_dicthad no other users.Redaction strategy (both HIGH sites)
CodeQL traced the
BERNSTEIN_ALWAYS_ALLOW_PATHenv value throughrules_path->reason->logger.error(reason)and classified the resolved path as sensitive. The fix:.sdd/metrics/guardrails.jsonl.reasonis still forwarded to_record_tamper_event, which writes it (with sha256 digests pre-redacted) to.sdd/metrics/guardrails.jsonlfor forensic review.AlwaysAllowTamperErrorstill carries the full reason for callers that catch and re-emit it through orchestrator-side channels.Net effect: operators keep full debuggability via the metrics file; the application-log stream no longer carries the env-derived path.
Verification
uv run pytest tests/unit/ -q --no-cov --timeout=120 -k "always_allow or audit_chain or task_lifecycle or jwt_tokens or agent_identity_jwt or jwt_refresh"-> 145 passeduv run ruff check .-> All checks passeduv run ruff format .-> 1 file reformatted (always_allow.py whitespace only)uv run refurbon the four files -> the three targeted findings cleared (FURB186 at task_lifecycle:1328 was out of scope and untouched)uv run pyright-> no new errors near the edited lines (255 pre-existing baseline errors unchanged)Test plan
Summary by CodeRabbit
New Features
scheduleCLI command for managing scheduled operations.Security & Stability