fix: keep Pingora diagnostics payload-free - #31
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (13)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough프로세스 전역 ChangesPayload-safe 로깅
Priority: ➖ Normal — Schedule the payload-safe logging change because Pingora diagnostics can expose request URIs and sensitive headers across both production gateway binaries. Estimated code review effort: 4 (Complex) | ~45 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The gateway preserves request delivery and safe application observability while preventing Pingora diagnostic logs from exposing request-derived secrets. Sequence Diagram(s)sequenceDiagram
participant Test
participant Gateway
participant Origin
participant Logger
Test->>Gateway: Start with RUST_LOG=trace
Test->>Gateway: Send request with sensitive values
Gateway->>Origin: Forward request unchanged
Origin-->>Gateway: Return 200 response
Gateway->>Logger: Emit Pingora diagnostic record
Logger-->>Test: Write redacted marker to stderr
Test->>Logger: Check marker and absence of sensitive values
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation 공유 로거 적용, Pingora 메시지 본문 마스킹, CWL 로그 수준 보존, 컴파일 프로세스 기반 비밀값 회귀 테스트는 Issue Full details: Docstring CoverageExplanation Docstring coverage is 46.15% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 26 functions across 5 files. (8 skipped: 8 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
seonghobae
added
bug
ChatGPT Codex Connector
|
@coderabbitai review Please review the ordinary-restacked exact range |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base='084ef0fd8c5cf2e55dccffd23961933270347925'
head='985d4df311c93966a5ef4a8212c89f1d249dd652'
printf '%s\n' '== Exact commits and ancestry =='
git show -s --format='%H%nparents: %P%nsubject: %s' "$base" "$head"
printf '\n== Merge base and ancestry checks ==\n'
git merge-base "$base" "$head"
git merge-base --is-ancestor "$base" "$head" && echo 'base_is_ancestor=true'
printf '\n== Changed paths ==\n'
git diff --name-status "$base..$head"
printf '\n== Diff statistics ==\n'
git diff --stat "$base..$head"
printf '\n== Rust file outlines in requested range ==\n'
git diff --name-only "$base..$head" -- '*.rs' |
while IFS= read -r f; do
test -n "$f" || continue
echo "--- $f"
ast-grep outline "$f" --items all --view expanded || true
done
printf '\n== Relevant definitions and logger-installation call sites ==\n'
rg -n -C 4 --type rust \
'(logging_policy|env_logger|set_logger|try_init|Builder::|Pingora|pingora|LogRecord|Record|diagnostic|redact)' \
src tests 2>/dev/null || true
printf '\n== Relevant manifest dependency lines ==\n'
rg -n -C 2 '(env_logger|^log\s*=|pingora)' Cargo.tomlLength of output: 141978 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='084ef0fd8c5cf2e55dccffd23961933270347925'
head='985d4df311c93966a5ef4a8212c89f1d249dd652'
printf '%s\n' '== Exact changed implementation diff =='
git diff --find-renames --find-copies --unified=80 "$base..$head" -- \
Cargo.toml src/lib.rs src/logging_policy.rs \
src/bin/cwl-pingora-gateway.rs src/bin/cwl-pingora-pg-erd-migration.rs \
tests/pingora_diagnostic_log_safety.rs
printf '\n== logging_policy.rs ==\n'
cat -n src/logging_policy.rs
printf '\n== Diagnostic test, full source ==\n'
cat -n tests/pingora_diagnostic_log_safety.rs
printf '\n== Every process logger setup call site ==\n'
rg -n -C 5 --type rust \
'(env_logger::|init_runtime_logging\s*\(|set_logger\s*\(|set_boxed_logger\s*\(|try_init\s*\(|\.init\s*\(\))' \
. -g '!target/**'
printf '\n== Previous composition-root logger setup ==\n'
git show "$base:src/bin/cwl-pingora-gateway.rs" | nl -ba | sed -n '1,80p'
git show "$base:src/bin/cwl-pingora-pg-erd-migration.rs" | nl -ba | sed -n '1,100p'
printf '\n== Available Pingora source directories and log target declarations ==\n'
find "$HOME/.cargo" -type f -path '*pingora*' -name '*.rs' -print 2>/dev/null |
head -n 20 || true
rg -n -C 2 --glob '*.rs' \
'(target:\s*"[^"]*pingora[^"]*"|target:\s*module_path!|log::(trace|debug|info|warn|error)!|(?:trace|debug|info|warn|error)!)' \
"$HOME/.cargo/git/checkouts" 2>/dev/null | head -n 300 || trueLength of output: 52156 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='084ef0fd8c5cf2e55dccffd23961933270347925'
head='985d4df311c93966a5ef4a8212c89f1d249dd652'
adoption='bf0a5fcd265c100b18a446d0718c9163ae90a946'
pingora_rev='09696b51bc59315353d96686355861604d0bb48c'
printf '%s\n' '== Requested-range commit topology =='
git rev-list --reverse --parents "$base..$head"
printf '\n== Adoption commit ==\n'
git show -s --format='%H%nparents: %P%ntree: %T%nsubject: %s' "$adoption"
parents=($(git show -s --format='%P' "$adoption"))
printf 'parent_1_tree=%s\n' "$(git show -s --format='%T' "${parents[0]}")"
printf 'parent_2_tree=%s\n' "$(git show -s --format='%T' "${parents[1]}")"
printf 'adoption_tree=%s\n' "$(git show -s --format='%T' "$adoption")"
printf '\n== Adoption delta against each parent ==\n'
git diff --stat "${parents[0]}" "$adoption"
git diff --stat "${parents[1]}" "$adoption"
printf '\n== Merge base exact tree identity ==\n'
printf 'base_tree=%s\nhead_tree=%s\n' \
"$(git show -s --format='%T' "$base")" "$(git show -s --format='%T' "$head")"
workdir="$(mktemp -d)"
trap 'rm -rf "$workdir"' EXIT
for path in \
pingora-core/src/protocols/http/client.rs \
pingora-proxy/src/proxy_h1.rs \
pingora-proxy/src/lib.rs \
pingora-core/src/lib.rs
do
url="https://raw.githubusercontent.com/cloudflare/pingora/${pingora_rev}/${path}"
out="$workdir/$(basename "$path")"
if curl --fail --silent --show-error --location "$url" --output "$out"; then
echo "--- $path"
rg -n -C 3 '(trace!|debug!|info!|warn!|error!|target:)' "$out" || true
else
echo "--- unavailable: $path"
fi
doneLength of output: 23281
I found no actionable technical findings. Key checks:
I did not execute the Rust test suite in this inspection environment. ✅ Action performedReview finished.
|
|
Exact-head CI RCA and forward repair — 2026-09-09 KST. CI run 34246262369 on Forward commit |
|
Exact-head CI RCA and forward repair — The prior formatting repair passed compilation and every Rust test, but CI run 34247829130 correctly failed the owned-production 100% gate: The new ordinary descendant adds one focused unit contract that executes |
|
Review admission — exact head The two causal forward repairs are now verified on the unchanged head: CI run 34249033063 and Supply Chain run 34249033179 are terminal GREEN. The first repair applied only the required rustfmt layout; the second executed the previously uncovered Marked Ready for independent review. This remains a stacked PR targeting |
|
@coderabbitai review Please review the current exact head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
head='8230f8d1311adccc133b7f6ed86df441533e486c'
prior='985d4df311c93966a5ef4a8212c89f1d249dd652'
printf '%s\n' '== Exact head, ancestry, and range =='
git show -s --format='head=%H%nparents=%P%ntree=%T%nsubject=%s%n%nbody=%B' "$head"
printf '\nmerge_base_with_prior='
git merge-base "$prior" "$head"
git merge-base --is-ancestor "$prior" "$head" && echo 'prior_is_ancestor=true' || echo 'prior_is_ancestor=false'
printf '\n== Commits after prior reviewed head ==\n'
git log --reverse --format='%H %P%n%s%n%b%n---' "$prior..$head"
printf '\n== Changed paths after prior reviewed head ==\n'
git diff --name-status "$prior..$head"
printf '\n== Diff after prior reviewed head ==\n'
git diff --find-renames --find-copies --unified=60 "$prior..$head"
printf '\n== Rust structure at exact head ==\n'
for path in src/logging_policy.rs src/lib.rs src/bin/cwl-pingora-gateway.rs src/bin/cwl-pingora-pg-erd-migration.rs tests/pingora_diagnostic_log_safety.rs; do
if test -f "$path"; then
echo "--- $path"
ast-grep outline "$path" --items all --view expanded || true
fi
done
printf '\n== Logging policy, composition roots, and test source ==\n'
for path in src/logging_policy.rs src/bin/cwl-pingora-gateway.rs src/bin/cwl-pingora-pg-erd-migration.rs tests/pingora_diagnostic_log_safety.rs; do
if test -f "$path"; then
echo "--- $path"
cat -n "$path"
fi
done
printf '\n== All global logger installation sites ==\n'
rg -n -C 4 --type rust \
'(?:env_logger::|init_runtime_logging\s*\(|set_logger\s*\(|set_boxed_logger\s*\(|try_init\s*\(|\.init\s*\(\))' \
. -g '!target/**' || true
printf '\n== Doc-comment locations for touched functions ==\n'
rg -n -C 3 --type rust '^\s*///|^\s*pub\s+(?:\(crate\)\s+)?(?:fn|struct|enum|trait|mod)\b|^\s*fn\b' \
src/logging_policy.rs tests/pingora_diagnostic_log_safety.rs 2>/dev/null || true
printf '\n== Merge topology from original base ==\n'
git rev-list --reverse --parents 084ef0fd8c5cf2e55dccffd23961933270347925.."$head"Length of output: 83803 🏁 Script executed: #!/bin/bash
set -euo pipefail
head='8230f8d1311adccc133b7f6ed86df441533e486c'
prior='985d4df311c93966a5ef4a8212c89f1d249dd652'
printf '%s\n' '== Patch validation =='
git diff --check "$prior..$head"
git diff --check 084ef0fd8c5cf2e55dccffd23961933270347925.."$head"
printf '\n== Individual forward commits ==\n'
for commit in $(git rev-list --reverse "$prior..$head"); do
echo "--- $(git show -s --format='%H %s' "$commit")"
git show --format= --find-renames --find-copies --unified=12 "$commit"
done
printf '\n== File-level history after previous head ==\n'
git log --format='%H %s' "$prior..$head" -- \
src/bin/cwl-pingora-gateway.rs \
src/bin/cwl-pingora-pg-erd-migration.rs \
src/logging_policy.rs \
tests/pingora_diagnostic_log_safety.rs
printf '\n== Pull request metadata and exact-head checks ==\n'
gh pr view 31 --repo ContextualWisdomLab/pingora-gateway \
--json number,state,isDraft,baseRefName,baseRefOid,headRefName,headRefOid,statusCheckRollup \
|| trueLength of output: 27953
I found no actionable technical findings. Key checks:
|
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Owner technical review on exact head 8230f8d1311adccc133b7f6ed86df441533e486c.
Reviewed the full retained #31 logging boundary and the changed range after the prior CodeRabbit-reviewed head. The new commits are documentation-only: they add constraint/evidence-oriented rustdoc to touched logging/composition-root/test helpers and do not alter emitted records, target classification, RUST_LOG filtering, request delivery, fixture time/byte bounds, traffic, routing, or security semantics. The compiled acceptance still proves exact secret-bearing origin delivery, post-readiness supplier-redaction activity, exact bounded completion vocabulary, and whole-stderr sentinel absence. No actionable source, test, or documentation finding remains from this exact-range technical review.
This COMMENT is technical evidence only. It is not an independent human APPROVED governance decision, and it does not transfer predecessor CI/Supply Chain receipts to this changed head. Exact 8230f8d... hosted CI/Supply Chain must still finish terminal GREEN before #33 succession.
|
Review-admission rationale for exact head |
1 participant
Valid finding
The gateway's canonical
observabilitytarget is payload-free, but both production composition roots previously installedenv_loggerdirectly from operatorRUST_LOG. At the pinned Pingora revision, supplier diagnostics include request-derived material before the CWL request callback. Broad operator diagnostics could therefore bypass the process data-minimization invariant.Fixes #30.
RED → causal fix
Historical RED
f31987fb92d5b8b24a7286db6e1702b54c472898added compiled-process acceptance underRUST_LOG=trace: the origin must receive URI/query/Host/Authorization/Cookie sentinels while process stderr contains none of them.The retained fix introduces one shared
logging_policyused by both production composition roots. Operator level/target selection is preserved, but Pingora-family dependency record messages are replaced with a static marker beforeenv_loggerformats them. CWL-owned bounded observability remains unchanged. Product/consumer logging, auth/business logic, routing, Wardnet/EgressWeave policy and Keyverse identity remain outside this boundary.Fixture hardening retains simultaneous traffic/metrics reservations, five-second/64 KiB origin bounds, exact request target and HTTP-field identity, exact completion-line and Prometheus oracles, and post-readiness marker-count increase so startup diagnostics cannot manufacture non-vacuity.
Ordinary/non-force succession
Final #29
084ef0fd8c5cf2e55dccffd23961933270347925is the exact parent. Ordinary two-parentbf0a5fcd265c100b18a446d0718c9163ae90a946preserved historical #31 while adopting the final #29 tree and reapplying only the valid logging delta. No force-push, destructive rebase, stale-parent source, or predecessor receipt was replayed.Hosted RED → repair → exact closure
Exact
985d4df311c93966a5ef4a8212c89f1d249dd652first exposed Rust 1.98.0 formatting only;468e8b2fec322b67be61aa511d1616a4de52c3f6applied the formatter-prescribed layout. A later exact head passed formatting/compile/tests but failed the mandatory 100% owned-production coverage gate only atPayloadSafeLogger::enabled;4d901f89c8e38195af8d9ac3efb0f089cb15df8badded a focused unit contract proving exact filter delegation without weakening coverage.The later CodeRabbit review reported touched-function Docstring Coverage below the repository expectation. Documentation-only descendants through final
8230f8d1311adccc133b7f6ed86df441533e486cadd constraint/evidence-oriented rustdoc to the touched logging/composition-root/fixture helpers without changing runtime behavior.Final exact #31 head is
8230f8d1311adccc133b7f6ed86df441533e486cand is unchanged through closure:34251472731: terminal success on this exact SHA.34251472751: terminal success on this exact SHA.APPROVEDgovernance decision.#33 has now ordinary/non-force adopted this unchanged final #31 tree and must independently reacquire its own exact-head hosted/current-range evidence. No #31 receipt transfers to that changed child.
Keep Draft. Supplier #54/#62, #56 independent approval, protected integration, immutable release, canary/cutover, rollback and legacy removal remain separate earlier/later gates. No self-approval, force-push/destructive rebase, gate weakening or predecessor-evidence transfer is claimed.