Skip to content

fix(security): stabilize exception-redaction successor - #1612

Open
seonghobae wants to merge 52 commits into
developfrom
codex/sentinel-exception-logging-successor
Open

seonghobae wants to merge 52 commits into
developfrom
codex/sentinel-exception-logging-successor

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Current authority — 2026-09-09

  • protected base: develop@042b0c70531b229af3acbd0421a2f23098d848b3
  • exact head: 3da3ae8e60e1bb049f59ae86bfe82db12b7e3cc7
  • exact tree: 228763942b82072ccb260d4f842e627f54cbda34
  • lifecycle: Ready / canonical security successor / repository-owned exact-head GREEN / CodeRabbit exact-head APPROVED / shared required gates failed / not merge-authorized
  • predecessor 🛡️ Sentinel: [MEDIUM] 예외 정보 유출 방지 #1591 remains provenance until protected integration proves complete succession
  • stacked fix(security): bound fixture archive extraction errors #1616 was normally merged into this owner at 41c6dd8a35e1b55e65b0a54a6e894ed1449bfa83; it is no longer a competing source writer

Owned security contract and repaired findings

This lane owns bounded exception redaction and fixture-import failure behavior. Raw secret-bearing exception values must not enter logs, API/CLI errors, __cause__, or __context__; archive extraction limits and unsafe-path rejection remain owned by services.archive.

Two reality RED → causal repairs are in the exact lineage:

  1. Archive rejection had become log-and-success. RED 43a952b95d41b7e630e951101a27006933fff947 requires rejected fixture extraction to report failure. GREEN 5813a5cfd198c04735f9e0531f45deb12914e001 makes process_zip_file() return False for expected extraction rejection and lets main() fail closed with the generic Fixture archive extraction failed error. Ordinary two-parent merge 41c6dd8... adopted the repaired fix(security): bound fixture archive extraction errors #1616 lineage without force-push/rebase.
  2. raise ... from None still retained the sensitive exception object. RED 9554ceb911355f0d3543fff4cb7be92ce38eb186 requires replacement ArchiveError to have both __cause__ is None and __context__ is None, with no secret/path in logs. GREEN 3da3ae8e60e1bb049f59ae86bfe82db12b7e3cc7 records only a non-sensitive sentinel inside the handler and raises the generic error after leaving the active exception handler.

Exact-head evidence

For unchanged 3da3ae8...:

  • Application CI backend/frontend: GREEN — backend lint/tests; frontend tests/lint/build/full-product Playwright smoke succeeded.
  • Bandit: GREEN.
  • Docker image validation: GREEN.
  • Security Scan: GREEN; Trivy filesystem and Scorecard evidence succeeded. Scope-based skips are not counted as executed GREEN.
  • Semgrep: GREEN.
  • coverage source/evidence: GREEN.
  • direct GitHub Advanced Security CodeQL analyses: GREEN/no new PR source finding observed.
  • required CodeQL compatibility workflow: FAIL on the shared terminal-receipt ordering path. Compatibility jobs read the current-head dispatch verdict and fail before the same-generation Dispatch current-head CodeQL scan job later succeeds. Canonical owner remains .github#1929; this is not repaired from Naruon with synthetic status/no-op commits.
  • opencode-review: FAIL. Exact-head review transport/admission ran but no current-head OpenCode verdict was available before the fail-closed gate completed; canonical shared owner path remains .github#1929.
  • noema-review: FAIL. orchestrator/free admitted 61 routes, selected 24, reported 3 ready and passed gateway preflight, but the substantive verdict request failed after about 365.3 s with HTTP 502 on served model google/gemma-4-31b-it. Canonical repair is contextual-orchestrator#1106 → immutable release → .github#2042 consumer bump/bridge removal → unchanged Naruon replay. No leaf provider/model pin, paid fallback, or extra retry is authorized.
  • strix: FAIL. Gateway readiness succeeded, but strix-agent==1.5.3 could not bootstrap its Caido sandbox (loginAsGuest could not reach 127.0.0.1:48080); the one classified same-model sandbox retry failed identically. Typed terminal state is STRIX_SANDBOX_UNAVAILABLE, owned by .github#891.

Independent review

CodeRabbit submitted formal APPROVED review PRR_kwDOSNjZ2s8AAAABMseIMw at 2026-09-08T20:54:34Z, explicitly anchored to exact commit 3da3ae8e60e1bb049f59ae86bfe82db12b7e3cc7. Current inline review-thread inventory is empty. The independent-review requirement is therefore satisfied for this unchanged head.

That approval does not override failed protected required contexts. Keep the source head stable while canonical shared owners repair their failures; do not push merely to retrigger them.

Merge boundary

Merge only when this unchanged exact head has every then-live required context terminal-success and zero valid unresolved findings/threads. Current CodeQL/OpenCode/Noema/Strix failures prohibit merge despite the exact-head independent approval.

No synthetic status, no-op requeue commit, central workflow copy, authorization widening, provider/model fallback, force-push, destructive rebase, self-approval, review dismissal/fabrication, admin bypass, or gate weakening.

seonghobae and others added 30 commits September 7, 2026 14:38
Signed-off-by: Seongho Bae <me@seonghobae.me>

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

Please review only current exact head 41c6dd8a35e1b55e65b0a54a6e894ed1449bfa83. Previous-head approvals/checks are stale. Focus on the fail-closed fixture archive rejection contract, sanitized failure boundary, and whether the inherited #1616 delta preserves the existing exception-redaction invariants without weakening services.archive limits.

@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

head='41c6dd8a35e1b55e65b0a54a6e894ed1449bfa83'
previous_head='b3a51ab1a16f24ed20597861f65ada2aea2f229f'
develop_base='042b0c70531b229af3acbd0421a2f23098d848b3'

echo '== target verification =='
printf 'checked_out='
git rev-parse HEAD
git cat-file -t "$head"
git status --short
echo '== ancestry and merge base =='
git merge-base "$develop_base" "$head"
git merge-base "$previous_head" "$head" || true
git merge-base --is-ancestor "$previous_head" "$head" && echo 'previous_head_is_ancestor=true' || echo 'previous_head_is_ancestor=false'

echo '== commit metadata =='
git show -s --format='commit=%H%nparents=%P%nsubject=%s' "$head"

echo '== effective changes against develop base =='
git diff --name-status "$develop_base" "$head"
echo '== current delta against prior reviewed head =='
git diff --name-status "$previous_head" "$head" || true

echo '== candidate file locations =='
fd -HI -t f '^(import_fixtures\.py|archive\.py|safe_logging\.py|test_.*(archive|fixture|exception|safe_logging).*\.(py))$' backend 2>/dev/null || true

echo '== outlines =='
for file in backend/scripts/import_fixtures.py backend/services/archive.py backend/core/safe_logging.py; do
  if [ -f "$file" ]; then
    echo "--- $file"
    ast-grep outline "$file" --items all --type function,class
  fi
done

echo '== security-relevant source =='
for file in backend/scripts/import_fixtures.py backend/services/archive.py backend/core/safe_logging.py; do
  if [ -f "$file" ]; then
    echo "--- $file"
    cat -n "$file"
  fi
done

echo '== focused tests =='
for file in backend/tests/test_exception_logging_boundaries.py backend/tests/test_fixture_archive_extraction_boundary.py backend/tests/test_safe_logging.py; do
  if [ -f "$file" ]; then
    echo "--- $file"
    cat -n "$file"
  fi
done

echo '== relevant changed hunks =='
git diff --unified=50 "$develop_base" "$head" -- \
  backend/scripts/import_fixtures.py \
  backend/services/archive.py \
  backend/core/safe_logging.py \
  backend/import_fixtures.py \
  backend/tests/test_exception_logging_boundaries.py \
  backend/tests/test_fixture_archive_extraction_boundary.py \
  backend/tests/test_safe_logging.py

echo '== archive limits and unbounded diagnostics =='
rg -n -C 4 --glob '*.py' \
  '(max_|limit|size|member|extract_backup_async|ArchiveError|zip_path|exc_info|logger\.(error|warning|info|exception)|raise\s+.*\bfrom\b)' \
  backend/services/archive.py backend/scripts/import_fixtures.py backend/core/safe_logging.py backend/tests 2>/dev/null || true

echo '== diff whitespace check =='
git diff --check "$develop_base" "$head"

Length of output: 180447


🏁 Script executed:

#!/bin/bash
set -euo pipefail

head='41c6dd8a35e1b55e65b0a54a6e894ed1449bfa83'
previous_head='b3a51ab1a16f24ed20597861f65ada2aea2f229f'
develop_base='042b0c70531b229af3acbd0421a2f23098d848b3'

echo '== revision and ancestry =='
printf 'checked_out='; git rev-parse HEAD
printf 'merge_base_develop='; git merge-base "$develop_base" "$head"
printf 'commit='; git show -s --format='%H' "$head"
git merge-base --is-ancestor "$previous_head" "$head" && echo 'previous_head_is_ancestor=true' || echo 'previous_head_is_ancestor=false'

echo '== files changed from previous reviewed head =='
git diff --name-status "$previous_head" "$head"
echo '== files changed from develop =='
git diff --name-status "$develop_base" "$head"

echo '== direct delta: archive and redaction files =='
git diff --unified=20 "$previous_head" "$head" -- \
  backend/scripts/import_fixtures.py \
  backend/services/archive.py \
  backend/core/safe_logging.py \
  backend/import_fixtures.py \
  backend/tests/test_exception_logging_boundaries.py \
  backend/tests/test_fixture_archive_extraction_boundary.py \
  backend/tests/test_archive.py

echo '== import script =='
cat -n backend/scripts/import_fixtures.py

echo '== archive service =='
cat -n backend/services/archive.py

echo '== safe logging helper =='
cat -n backend/core/safe_logging.py

echo '== fixture archive boundary tests =='
cat -n backend/tests/test_fixture_archive_extraction_boundary.py

echo '== exception logging boundary tests, archive-focused range =='
sed -n '140,350p' backend/tests/test_exception_logging_boundaries.py | nl -ba -v140

echo '== archive tests =='
cat -n backend/tests/test_archive.py

echo '== all calls to process_zip_file and extract_backup_async outside tests =='
rg -n -C 3 --glob '*.py' \
  '\b(process_zip_file|extract_backup_async)\s*\(' backend \
  -g '!backend/tests/**'

echo '== changed added traceback/log calls =='
git diff --unified=0 "$develop_base" "$head" -- '*.py' |
rg -n '^\+.*(exc_info\s*=\s*True|logger\.(exception|error|warning|info)|raise\s+.*\bfrom\b|str\(.*exc|f["'\''].*\{.*exc)' || true

Length of output: 41329


🏁 Script executed:

#!/bin/bash
set -euo pipefail

head='41c6dd8a35e1b55e65b0a54a6e894ed1449bfa83'
previous_head='b3a51ab1a16f24ed20597861f65ada2aea2f229f'
develop_base='042b0c70531b229af3acbd0421a2f23098d848b3'

show_file() {
  local revision="$1"
  local file="$2"
  echo "--- ${revision}:${file}"
  git show "${revision}:${file}" | nl -ba
}

echo '== exact target identity =='
git show -s --format='commit=%H%nparents=%P%nsubject=%s' "$head"
echo '== target status relative to relevant revisions =='
git diff --name-status "$previous_head" "$head"
git diff --name-status "$develop_base" "$head"

echo '== exact target: fixture importer =='
show_file "$head" backend/scripts/import_fixtures.py

echo '== exact target: archive service =='
show_file "$head" backend/services/archive.py

echo '== exact target: redaction helper =='
show_file "$head" backend/core/safe_logging.py

echo '== exact target: archive boundary test =='
show_file "$head" backend/tests/test_fixture_archive_extraction_boundary.py

echo '== exact target: extraction and ZIP processing tests =='
git show "${head}:backend/tests/test_exception_logging_boundaries.py" |
  sed -n '145,355p' | nl -ba -v145

echo '== exact target: archive-service regression tests =='
show_file "$head" backend/tests/test_archive.py

echo '== prove archive limits are unchanged in the inherited delta =='
git diff --unified=12 "$previous_head" "$head" -- backend/services/archive.py
git diff --unified=12 "$develop_base" "$head" -- backend/services/archive.py

echo '== exact target: changed exception/redaction call sites =='
git diff --unified=0 "$develop_base" "$head" -- \
  backend/api/emails.py backend/api/prompts.py backend/import_fixtures.py \
  backend/scripts/import_fixtures.py backend/services/imap_worker.py \
  backend/services/llm_service.py backend/services/pop3_worker.py \
  backend/core/safe_logging.py |
rg -n -C 2 '^\+.*(exc_info\s*=\s*True|logger\.(exception|error|warning|info)|raise\s+.*\bfrom\b|str\(.*exc|f["'\''].*\{.*exc)' || true

echo '== exact target: source references for archive extraction =='
git grep -n -E '\b(extract_backup_async|extract_backup|process_zip_file)\s*\(' "$head" -- 'backend/*.py' 'backend/**/*.py' || true

echo '== exact target: whitespace validation =='
git diff --check "$develop_base" "$head"

Length of output: 3684


🏁 Script executed:

#!/bin/bash
set -euo pipefail

head='41c6dd8a35e1b55e65b0a54a6e894ed1449bfa83'
first_parent='b3a51ab1a16f24ed20597861f65ada2aea2f229f'
second_parent='5813a5cfd198c04735f9e0531f45deb12914e001'
develop_base='042b0c70531b229af3acbd0421a2f23098d848b3'

numbered_show() {
  local revision="$1"
  local file="$2"
  echo "--- ${revision}:${file}"
  git show "${revision}:${file}" | awk '{ printf "%6d\t%s\n", NR, $0 }'
}

echo '== exact target source: archive fixture importer =='
numbered_show "$head" backend/scripts/import_fixtures.py

echo '== exact target source: archive service =='
numbered_show "$head" backend/services/archive.py

echo '== exact target source: redaction helper =='
numbered_show "$head" backend/core/safe_logging.py

echo '== exact target test: archive rejection boundary =='
numbered_show "$head" backend/tests/test_fixture_archive_extraction_boundary.py

echo '== exact target test: ZIP extraction and processing =='
git show "${head}:backend/tests/test_exception_logging_boundaries.py" |
  awk 'NR >= 140 && NR <= 350 { printf "%6d\t%s\n", NR, $0 }'

echo '== exact target tests: archive limits =='
numbered_show "$head" backend/tests/test_archive.py

echo '== merge-result differences from each parent =='
echo '-- first parent --'
git diff --name-status "$first_parent" "$head"
git diff --unified=20 "$first_parent" "$head" -- \
  backend/scripts/import_fixtures.py backend/services/archive.py \
  backend/core/safe_logging.py backend/tests/test_exception_logging_boundaries.py \
  backend/tests/test_fixture_archive_extraction_boundary.py backend/tests/test_archive.py
echo '-- second parent --'
git diff --name-status "$second_parent" "$head"
git diff --unified=20 "$second_parent" "$head" -- \
  backend/scripts/import_fixtures.py backend/services/archive.py \
  backend/core/safe_logging.py backend/tests/test_exception_logging_boundaries.py \
  backend/tests/test_fixture_archive_extraction_boundary.py backend/tests/test_archive.py

echo '== exact limit declarations across merge parents and result =='
for revision in "$first_parent" "$second_parent" "$head"; do
  echo "--- $revision"
  git show "${revision}:backend/services/archive.py" |
    grep -nE '^(MAX_EXTRACT_SIZE|MAX_FILE_COUNT)\b|file_count >|total_size >' || true
done

echo '== exact target: extraction call sites and logging/raising in fixture importer =='
git show "${head}:backend/scripts/import_fixtures.py" |
  grep -nE 'extract_backup_async|process_zip_file|logger\.|except |raise |commit|execute' || true

echo '== exact target: added redaction-sensitive lines against develop =='
git diff --unified=0 "$develop_base" "$head" -- '*.py' |
grep -nE '^\+.*(exc_info[[:space:]]*=[[:space:]]*True|logger\.(exception|error|warning|info)|raise .* from |str\(.*exc|f["'\''].*\{.*exc)' || true

echo '== whitespace validation =='
git diff --check "$develop_base" "$head"

Length of output: 58624


@seonghobae I reviewed Git object 41c6dd8a35e1b55e65b0a54a6e894ed1449bfa83 directly. The checked-out worktree was at a different commit.

I found no issue in the requested scope.

  • process_zip_file() logs fixed messages only. It does not log the archive path or exception text.
  • Expected ArchiveError failures return False.
  • Unexpected extraction failures raise the fixed ArchiveError("Fixture archive extraction failed") with from None.
  • main() stops at the first rejected archive and raises the same bounded error. The exception has no cause or context.
  • The added tests cover expected rejection, unexpected secret-bearing failures, fail-closed behavior, and fixture-directory path redaction.
  • The inherited exception-redaction changes remain present.
  • services.archive keeps MAX_EXTRACT_SIZE at 10 GB and MAX_FILE_COUNT at 100,000. Both enforcement checks are unchanged in both merge parents and the merge result.

I did not execute tests in this environment.

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

Please review only the current exact head 3da3ae8e60e1bb049f59ae86bfe82db12b7e3cc7. The prior CodeRabbit pass stopped at 41c6dd8... and predates the __context__ disposal RED/GREEN. Do not carry predecessor approval forward.

@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

head='3da3ae8e60e1bb049f59ae86bfe82db12b7e3cc7'
previous_head='41c6dd8a35e1b55e65b0a54a6e894ed1449bfa83'
develop_base='042b0c70531b229af3acbd0421a2f23098d848b3'

echo '== target identity =='
printf 'checked_out='; git rev-parse HEAD
git cat-file -t "$head"
git show -s --format='commit=%H%nparents=%P%nsubject=%s' "$head"
git status --short

echo '== ancestry and changed files =='
git merge-base --is-ancestor "$previous_head" "$head" \
  && echo 'previous_head_is_ancestor=true' \
  || echo 'previous_head_is_ancestor=false'
echo '-- delta from prior reviewed head --'
git diff --name-status "$previous_head" "$head"
echo '-- effective delta from develop --'
git diff --name-status "$develop_base" "$head"

echo '== current-head delta related to exception context =='
git diff --unified=60 "$previous_head" "$head" -- \
  backend/core/safe_logging.py \
  backend/scripts/import_fixtures.py \
  backend/api/emails.py \
  backend/api/prompts.py \
  backend/import_fixtures.py \
  backend/services/llm_service.py \
  backend/services/imap_worker.py \
  backend/services/pop3_worker.py \
  backend/tests/test_safe_logging.py \
  backend/tests/test_exception_logging_boundaries.py \
  backend/tests/test_fixture_archive_extraction_boundary.py \
  backend/tests/test_email_exception_context.py

echo '== relevant file outlines at target =='
for file in \
  backend/core/safe_logging.py \
  backend/scripts/import_fixtures.py \
  backend/services/archive.py \
  backend/tests/test_safe_logging.py \
  backend/tests/test_exception_logging_boundaries.py \
  backend/tests/test_fixture_archive_extraction_boundary.py \
  backend/tests/test_email_exception_context.py
do
  if git cat-file -e "${head}:${file}" 2>/dev/null; then
    echo "--- $file"
    git show "${head}:${file}" > "/tmp/$(basename "$file")"
    ast-grep outline "/tmp/$(basename "$file")" --items all --type function,class || true
  fi
done

echo '== exception, log, and archive references at target =='
git grep -n -E \
  '(__context__|__cause__|__suppress_context__|redacted_exception_info|exc_info[[:space:]]*=|logger\.(error|warning|info|exception)|except |raise |extract_backup_async|process_zip_file|MAX_EXTRACT_SIZE|MAX_FILE_COUNT)' \
  "$head" -- \
  'backend/core/safe_logging.py' \
  'backend/scripts/import_fixtures.py' \
  'backend/services/archive.py' \
  'backend/api/emails.py' \
  'backend/api/prompts.py' \
  'backend/import_fixtures.py' \
  'backend/services/llm_service.py' \
  'backend/services/imap_worker.py' \
  'backend/services/pop3_worker.py' \
  'backend/tests/test_safe_logging.py' \
  'backend/tests/test_exception_logging_boundaries.py' \
  'backend/tests/test_fixture_archive_extraction_boundary.py' \
  'backend/tests/test_email_exception_context.py' || true

echo '== exact target source and tests changed since prior head =='
git diff --name-only "$previous_head" "$head" -- '*.py' |
while IFS= read -r file; do
  [ -n "$file" ] || continue
  echo "--- $file"
  git show "${head}:${file}" | nl -ba
done

echo '== archive limits compared with prior reviewed head =='
git diff --unified=20 "$previous_head" "$head" -- backend/services/archive.py
git diff --check "$previous_head" "$head"

Length of output: 39333


🏁 Script executed:

#!/bin/bash
set -euo pipefail

head='3da3ae8e60e1bb049f59ae86bfe82db12b7e3cc7'
previous_head='41c6dd8a35e1b55e65b0a54a6e894ed1449bfa83'

echo '== full target: fixture ZIP importer =='
git show "${head}:backend/scripts/import_fixtures.py" | sed -n '1,190p'

echo '== full target: archive extraction service =='
git show "${head}:backend/services/archive.py" | sed -n '1,150p'

echo '== target: ZIP-related exception-boundary tests =='
git show "${head}:backend/tests/test_exception_logging_boundaries.py" | sed -n '140,345p'

echo '== target: archive extraction boundary tests =='
git show "${head}:backend/tests/test_fixture_archive_extraction_boundary.py" | sed -n '1,170p'

echo '== target: redacted exception helper and direct test =='
git show "${head}:backend/core/safe_logging.py"
git show "${head}:backend/tests/test_safe_logging.py"

echo '== all current-delta hunks =='
git diff --unified=12 "$previous_head" "$head" -- '*.py'

echo '== explicit target searches for exception chaining and log interpolation in changed importer =='
git show "${head}:backend/scripts/import_fixtures.py" |
  grep -nE 'except |raise |logger\.|exc_info|from |__context__|__cause__|zip_path|file_path' || true

echo '== check no archive service delta exists =='
git diff --name-only "$previous_head" "$head" -- backend/services/archive.py
git diff --check "$previous_head" "$head"

Length of output: 29476


@seonghobae I reviewed only exact head 3da3ae8e60e1bb049f59ae86bfe82db12b7e3cc7.

I found no issue in the requested scope.

  • process_zip_file() now raises the bounded ArchiveError after the except Exception block.
  • The raised ArchiveError has no implicit __context__.
  • The new RED/GREEN test uses secret-bearing exception text and confirms that __cause__ and __context__ are None.
  • The test also confirms that logs do not contain the secret text or fixture path.
  • Expected ArchiveError rejection still logs a fixed message and returns False.
  • main() still fails closed on archive rejection.
  • The current delta does not change backend/services/archive.py. MAX_EXTRACT_SIZE and MAX_FILE_COUNT remain unchanged.
  • Existing exception-redaction call sites remain unchanged in this delta.

I did not execute tests because this sandbox does not permit repository code execution.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority: critical type: bug Defect or incorrect behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant