Skip to content

fix(security): stop exception messages reaching ops/system-agent responses (#1917) - #1959

Merged
vybe merged 1 commit into
devfrom
fix/1917-stack-trace-exposure
Aug 3, 2026
Merged

fix(security): stop exception messages reaching ops/system-agent responses (#1917)#1959
vybe merged 1 commit into
devfrom
fix/1917-stack-trace-exposure

Conversation

@dolho

@dolho dolho commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Problem

PR #1912 fixed restart_fleet after CodeQL flagged py/stack-trace-exposure, and deliberately left the sibling sites alone under the minimal-changes rule. This closes them with the same pattern: the response carries platform-authored text or the exception class name; the full message + traceback go to the backend log via exc_info=True.

Exposure is bounded — every endpoint here is admin-gated — but exception messages routinely embed internals. Docker and httpx errors lead with the socket path or the internal container host, and learnings.md (2026-07-14) records git stderr carrying a PAT into operator-visible state. "Admin-only" is a blast-radius argument, not a reason for the string to be there.

Sites

Located by content, not the issue's line numbersdev has moved since it was filed, and every cited line had drifted.

File Sites
routers/ops.py fleet health probe · stop_fleet per-agent · _stop_agent_container (feeds emergency_stop) · ops costs
routers/system_agent.py health probe (CodeQL alert #231) · workspace cleanup · template copy · re-initialize 500 · restart 500 · terminal WebSocket error frame
routers/agents.py start / stop / logs HTTPException details

The health probe had str(e)[:50]. Truncation bounds a leak, it doesn't remove one — docker/httpx messages lead with the host, socket path or URL, so the first 50 characters are the sensitive part.

Two sites weren't in the issue and are the same defect, found by grepping the named files rather than trusting the list: system_agent's re-initialize/restart 500 details, and its terminal WebSocket error frame — the only one here that reaches a browser rather than an admin API client.

Guards — 8 tests, all 8 failing against pre-fix code

Each drives the real router function with a sentinel planted in the exception message, and asserts both halves:

assert SENTINEL not in body        # the leak is gone
assert "RuntimeError" in body      # ...and the failure is still diagnosable

The second assertion matters: a response that dropped the error entirely would pass a sentinel-only check while making the failure impossible to debug.

Plus a static ban on str(e) in the two routers where every occurrence was this defect. agents.py is deliberately excluded from that ban — it's a 1000+ line router with many str(e) uses that never reach a response, so a blanket rule there would be false; its three sites get a targeted window check instead.

One test was vacuous, and the pre-fix run caught it

My first version of the fleet-health test patched a get_agent_context_info that does not exist. raising=False made that silent, the probe never raised, and the test passed against the unpatched router — 7 of 8 failing pre-fix instead of 8. It now patches the real seam (get_agent_client(...).get_session()) and asserts await_count == 1, so it cannot pass without the probe running. Same vacuous-negative class as #1932/#1951.

Verification

tests/unit/test_1917_stack_trace_exposure.py                         8 passed  (8 failed pre-fix)
+ test_1860_fleet_restart_adoption / test_1816_system_agent_adoption  102 passed
pytest unit/ -k "ops or system_agent or agents"                      275 passed, 2 skipped

Acceptance criteria

Closes #1917

🤖 Generated with Claude Code

…onses (#1917)

PR #1912 fixed `restart_fleet` after CodeQL flagged py/stack-trace-exposure,
and deliberately left the sibling sites alone under the minimal-changes rule.
This closes them with the same pattern — response carries platform-authored
text or the exception CLASS NAME; the full message + traceback go to the
backend log via exc_info=True.

Sites (located by content, not the issue's line numbers — dev has moved):

  routers/ops.py           fleet health probe, stop_fleet per-agent,
                           _stop_agent_container (feeds emergency_stop),
                           ops costs
  routers/system_agent.py  health probe (open CodeQL alert #231), workspace
                           cleanup, template copy, the re-initialize and
                           restart 500s, and the terminal WebSocket error frame
  routers/agents.py        start / stop / logs HTTPException details

The health probe had `str(e)[:50]`. Truncation bounds a leak, it does not
remove one: docker and httpx messages lead with the host, socket path or URL,
so the first 50 characters are exactly the sensitive part.

Two sites were not in the issue and are the same defect, found by grepping the
named files rather than trusting the list: system_agent's re-initialize/restart
500 details, and its terminal WebSocket error frame — the only one here that
reaches a browser rather than an admin API client.

Guards: 8 tests, all 8 failing against pre-fix code. Each drives the real
router function with a sentinel in the exception message and asserts BOTH that
the sentinel is gone and that the class name survives — a response that dropped
the error entirely would pass a sentinel-only check while making the failure
undiagnosable. Plus a static ban on str(e) in the two routers where every
occurrence was this defect (agents.py is deliberately excluded: it has many
str(e) uses that never reach a response, so a blanket ban there would be false).

The fleet-health test initially patched a `get_agent_context_info` that does
not exist — `raising=False` made that silent, the probe never raised, and the
test passed against the UNPATCHED router. The pre-fix run is what caught it;
it now patches `get_agent_client` and asserts the probe actually ran. Same
vacuous-negative class as #1932/#1951.

Related to #1917

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@dolho
dolho force-pushed the fix/1917-stack-trace-exposure branch from dd9562e to 09b5b70 Compare August 3, 2026 11:00

@vybe vybe 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.

Validated via /validate-pr. Closes #1917 (closing keyword added so the issue auto-promotes to status-in-dev).

Security fix, no schema/API/config surface, no new top-level backend module, no secrets in the diff. Three things I checked specifically and liked:

  • Sites located by content, not the issue's stale line numbers — and two sites that weren't in the issue (system_agent's re-init/restart 500 details, and the terminal WS error frame, the only one reaching a browser) were found by grepping the named files rather than trusting the list.
  • str(e)[:50] correctly treated as a leak, not a mitigation — docker/httpx messages lead with the socket path or internal host, so the first 50 chars are the sensitive part.
  • Both-halves assertion (SENTINEL not in body AND "RuntimeError" in body) — a response that dropped the error entirely would pass a sentinel-only check while making the failure undebuggable. The disclosed vacuous first draft (patching a get_agent_context_info that doesn't exist, silent under raising=False) is exactly the class worth catching, and 8/8 now fail pre-fix.

The static str(e) ban scoped to the two routers where every occurrence was this defect — with agents.py deliberately excluded and given targeted window checks instead — is the right call over a blanket rule that would be false there.

CodeQL alert #231 can only close on the next main analysis; noted, not blocking.

@vybe
vybe merged commit 36d07b3 into dev Aug 3, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants