Skip to content

SEP-2018: Keep an upstream error body out of the case-search failure log - #1514

Open
marcuscruz-percona wants to merge 1 commit into
mainfrom
SEP-2018
Open

marcuscruz-percona wants to merge 1 commit into
mainfrom
SEP-2018

Conversation

@marcuscruz-percona

Copy link
Copy Markdown
Contributor

Summary

  • atw_case_search's degraded path now logs the caught exception's type instead of passing exc_info=True. RemoteAPI.request maps an upstream error body's detail onto the exception it raises, so rendering that exception put a receiver-supplied value into a WARNING-level record. This mirrors the contract read_delivery_connection already holds for its own route-written log line (SEP-1997), and touches neither RemoteAPI.request() nor search_cases().
  • Adds a regression test to TestAtwCaseSearch that mocks a non-2xx response carrying a detail body and asserts the value never reaches caplog.text captured at the default level, with a positive control on the route's own line so the sentinel cannot hold vacuously.
  • Strengthens the neighbouring test_a_failed_search_leaves_no_secret_in_the_logs: its str(record.exc_info) guard could no longer fail once the traceback was gone, so it now asserts that no record carries exc_info at all — reintroducing a traceback at this call site fails the suite.

The transport's own DEBUG-level response-body line at this call site is unchanged and stays SEP-1999's contract; the new test captures at the default level for exactly that reason.

Tested

  • Configure a delivery plan with a case_search section, make the receiver answer the search with a non-2xx status and a detail body, issue a search from the Collect Diagnostic Data send dialog's case-reference field, and confirm the resulting WARNING line names only the exception type
  • Confirm the dialog still renders the degraded "no matches" state rather than an error
  • Confirm a successful search still returns its matches unchanged

Checklist

  • New/modified functions have type hints and rST docstrings
  • New tests added for new features or bug fixes
  • All tests pass locally (make test) — 12996 passed; the only 6 failures are tests/app/tasks/db/test_seed.py::TestArtifactLauncher, which fail identically on main (macOS xargs has no --arg-file)
  • Pre-commit hooks pass (make run-pre-commit)
  • Database migrations generated if models changed (make makemigrations) — N/A, no model change
  • User-facing changes documented (README, inline help, UI text) — N/A, the response shape and dialog behaviour are unchanged
  • Configuration changes documented with examples — N/A
  • Changelog fragment added under changelog.d/ if the change is user-facing (make changelog-add), or confirmed N/A (internal-only change, or a same-release-cycle fix for an unreleased sibling ticket)

🤖 Generated with Claude Code

The case-search route's degraded path logged with `exc_info=True`. RemoteAPI
maps an upstream error body's `detail` onto the exception it raises, and
Starlette renders that detail in `HTTPException.__str__`, so a receiver-supplied
string reached a WARNING record every SEP admin can read.

The route now logs the caught exception's type name and no traceback, matching
the contract `read_delivery_connection` already holds for its own log line.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings September 14, 2026 13:52
@github-actions github-actions Bot added python app:atw PR touches the atw app slice app-isolated All changes confined to a single app slice (low blast radius) labels Sep 14, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The focused logging change addresses the leak and includes non-vacuous regression coverage.

Pull request overview

Prevents receiver-supplied upstream error details from entering ATW case-search warning logs while preserving degraded-response behavior.

Changes:

  • Logs only the caught exception type without traceback data.
  • Adds regression coverage for response-body and credential leakage.
File summaries
File Description
app/sep/apps/atw/api_routes.py Sanitizes degraded case-search logging.
tests/app/sep/apps/atw/test_send_api.py Verifies sensitive values and tracebacks are not logged.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

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

Labels

app:atw PR touches the atw app slice app-isolated All changes confined to a single app slice (low blast radius) python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants