fix(security): bound exception telemetry without raw traceback - #1700
seonghobae wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe safe logging helper now returns a redacted exception message, no traceback object, and a location-based fingerprint. Tests verify secret exclusion, fingerprint behavior, and unraised exceptions. A design document records the confidentiality boundary. ChangesConfidential exception telemetry
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The change removes raw exception values and tracebacks, with no verified issue currently preventing merge. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review Please review exact head |
Rate Limit Exceeded
|
|
@coderabbitai review exact head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== HEAD =='
git rev-parse HEAD
printf '%s\n' '== status =='
git status --short
printf '%s\n' '== effective diff names =='
git diff --name-status 3da3ae8e60e1bb049f59ae86bfe82db12b7e3cc7..HEAD
printf '%s\n' '== relevant file candidates =='
fd -t f -e py . | rg '(^|/)(safe_logging|test_safe_logging)\.py$' || true
printf '%s\n' '== Python symbol outline =='
for f in $(fd -t f -e py . | rg '(^|/)(safe_logging|test_safe_logging)\.py$'); do
echo "--- $f"
ast-grep outline "$f" --items all --type function,class || true
done
printf '%s\n' '== effective diff =='
git diff --find-renames --find-copies --unified=80 3da3ae8e60e1bb049f59ae86bfe82db12b7e3cc7..HEAD -- \
':(glob)**/safe_logging.py' \
':(glob)**/test_safe_logging.py' \
':(glob)**/exception-telemetry-confidentiality.md'
printf '%s\n' '== call sites =='
rg -n -C 4 '\bredacted_exception_info\s*\(' --glob '*.py' . || true
printf '%s\n' '== logging format configuration =='
rg -n -C 3 '(format\s*=|Formatter\s*\(|basicConfig\s*\(|exc_info\s*=)' --glob '*.py' . | head -n 400 || trueLength of output: 41781 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== safe_logging.py =='
cat -n backend/core/safe_logging.py
printf '%s\n' '== test_safe_logging.py =='
cat -n backend/tests/test_safe_logging.py
printf '%s\n' '== existing boundary test =='
cat -n backend/tests/test_exception_logging_boundaries.py
printf '%s\n' '== imports and packaging context =='
rg -n -C 2 'from core\.safe_logging import|import core\.safe_logging' --glob '*.py' backend
printf '%s\n' '== standard library formatter probe =='
python - <<'PY'
import logging
secret = "postgresql://user:password@internal.example/db"
exc_type = type(f"ProviderError_{secret}", (Exception,), {})
record = logging.LogRecord(
name="probe",
level=logging.ERROR,
pathname="/internal/path.py",
lineno=1,
msg="operation failed",
args=(),
exc_info=(RuntimeError, RuntimeError(
f"Exception details redacted exception_type={exc_type.__name__} "
"exception_fingerprint=0123456789abcdef"
), None),
)
print(logging.Formatter("%(message)s").format(record))
PY
printf '%s\n' '== type name assignment probe =='
python - <<'PY'
class ProbeError(Exception):
pass
ProbeError.__name__ = "token=super-secret-value"
print(ProbeError.__name__)
PYLength of output: 23993
I found two issues.
The traceback suppression and deepest-site fingerprint design otherwise prevent the original exception value and traceback frames from reaching Python’s formatter. The same-function, different-raise-site test also covers the intended line and bytecode distinction.
|
|
@coderabbitai review Please review only exact current head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
head='2d6800aea110ac04eeedcd5faa91479f62877408'
base='3da3ae8e60e1bb049f59ae86bfe82db12b7e3cc7'
printf '%s\n' '== revision verification =='
git rev-parse HEAD
git cat-file -e "${head}^{commit}"
git cat-file -e "${base}^{commit}"
printf '%s\n' '== effective changed files =='
git diff --name-status "$base..$head"
printf '%s\n' '== candidate files =='
fd -t f . | rg '(^|/)(safe_logging\.py|test_safe_logging\.py|test_exception_logging_boundaries\.py|exception-telemetry-confidentiality\.md)$' || true
printf '%s\n' '== outlines =='
for f in backend/core/safe_logging.py backend/tests/test_safe_logging.py backend/tests/test_exception_logging_boundaries.py; do
if [ -f "$f" ]; then
echo "--- $f"
ast-grep outline "$f" --items all --type function,class || true
fi
done
printf '%s\n' '== effective diff =='
git diff --find-renames --unified=100 "$base..$head" -- \
backend/core/safe_logging.py \
backend/tests/test_safe_logging.py \
backend/tests/test_exception_logging_boundaries.py \
docs/doctoring/exception-telemetry-confidentiality.md
printf '%s\n' '== current helper =='
sed -n '1,240p' backend/core/safe_logging.py
printf '%s\n' '== current helper tests =='
sed -n '1,320p' backend/tests/test_safe_logging.py
printf '%s\n' '== boundary tests =='
sed -n '1,180p' backend/tests/test_exception_logging_boundaries.py
printf '%s\n' '== helper call sites =='
rg -n -C 5 '\bredacted_exception_info\s*\(' --glob '*.py' backend || true
printf '%s\n' '== logger formatter configurations =='
rg -n -C 3 '(logging\.Formatter\s*\(|Formatter\s*\(|format\s*=|exc_info\s*=)' --glob '*.py' backend | head -n 500 || true
printf '%s\n' '== exception name mutations and dynamic exception types =='
rg -n -C 3 '(__name__\s*=|type\s*\([^,]+,\s*\(\s*Exception|type\s*\([^,]+,\s*\(\s*BaseException)' --glob '*.py' backend || trueLength of output: 58455
I found two issues.
The synthetic Repository tests were not run. The review environment permits static inspection only. ✏️ Learnings added
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
seonghobae
added
bug
ChatGPT Codex Connector
This branch has not been deployed
1 participant
Authority
2d6800aea110ac04eeedcd5faa91479f62877408a6974aad9b0f7f455e0e5a09875bf67cb3b7dc2ecodex/sentinel-exception-logging-successor@3da3ae8e60e1bb049f59ae86bfe82db12b7e3cc7develop@042b0c70531b229af3acbd0421a2f23098d848b3Verified RED
#1612 correctly stopped rendering raw exception values on its bounded security surface, but its shared
redacted_exception_info()retained the original traceback. Python logging can therefore still render internal source locations. #1698 requires a sink-aware structured correlation contract rather than treating tracebacks as redaction.The first #1700 head removed raw traceback rendering and introduced a deterministic fingerprint, but the key was only
exception type + module + function. Two independent exception statements inside one function therefore collapsed into the same incident signature. That weakens RCA and can hide a new causal path behind an existing noisy one.Protected
developstill contains additionalexc_info=Trueand raw exception-interpolation sinks outside #1612's bounded surface. This PR does not blanket-rewrite those sites or claim #1698 complete.Minimal causal repair
The #1612 caller API remains unchanged.
core.safe_logging.redacted_exception_info(error)now:str(error)/repr(error);LogRecord;Fingerprint stability is intentionally build-scoped: source edits or interpreter/compiler changes may change line/bytecode identity and therefore the fingerprint. This is preferable to falsely merging distinct failure sites.
Verification contract
backend/tests/test_safe_logging.pydrives the real Pythonlogging.Formatterwith token-like values and a PostgreSQL connection string. It requires:docs/doctoring/exception-telemetry-confidentiality.mdrecords the decision, rejected module/function-only alternative, build-scoped stability boundary, verification, CWE-532 / OWASP traceability, and why #1698 remains open for sink-by-sink migration.Independent review
CodeRabbit reviewed exactly
3da3ae8e60e1bb049f59ae86bfe82db12b7e3cc7 → 2d6800aea110ac04eeedcd5faa91479f62877408, selected all three effective files, generated no actionable comment, and submitted formalAPPROVEDreviewPRR_kwDOSNjZ2s8AAAABNs7L5gat 2026-09-15T18:52:19Z. This is current-head review evidence; it does not substitute for hosted required checks or protected-lineage integration.Evidence boundary
Exact
2d6800aea110ac04eeedcd5faa91479f62877408has zero PR-triggered repository workflow runs because this PR is stacked on #1612 and the repository-local stacked-PR trigger defect is still owned by #1691. Do not manufacture checks through temporary retargeting, dummy/no-op commits, copied workflows, synthetic statuses, or predecessor receipt transfer.Stack boundary
Do not merge this PR into #1612 while #1612's exact-head evidence is still the active parent gate, because doing so would move the parent head and invalidate that evidence. After #1612 normally integrates to protected
develop, reconcile/retarget this effective delta onto the integrated owner lineage and regenerate hosted evidence. No force push, destructive rebase, self-approval, check suppression, no-op requeue commit, or gate weakening.Gate
Keep Draft until the unchanged final integrated head has terminal success for every then-live required context, no valid unresolved current-head finding/thread, and qualifying post-last-push independent review. Issue #1698 remains open until the remaining production sinks and their trust/retention boundaries are inventoried and migrated with source-backed tests.
Summary by CodeRabbit
Bug Fixes
Documentation