From 9c4f06106425cf2ba3daa08fe0afb24f97560d26 Mon Sep 17 00:00:00 2001 From: scottschreckengaust <345885+scottschreckengaust@users.noreply.github.com> Date: Tue, 15 Sep 2026 23:13:03 +0000 Subject: [PATCH] fix(security): clear the remaining silent-success-masking sites (#756) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The whole-repo `security:sast:masking` leg has been red continuously, so it ratcheted nothing — two of the six live findings (`server.py`, `registry-publish.ts`) are absent from #756's original list of 21 because they landed behind an already-failing gate. This clears the tail: 6 findings -> 0. Root cause for `observability.py`: the site already carried a `nosemgrep: py-silent-success-masking` justification, but the rule uses `focus-metavariable: $RET`, so the finding anchors to the `return None` token and semgrep only scans the finding's own line plus the one directly above it. The justification wrapped onto two lines, which pushed the marker to N-2 — where semgrep never looks. It read correctly to a human and suppressed nothing. Per-site disposition (#756 asks for merits, not blanket annotation): - `cdk/src/handlers/registry-publish.ts` — structural fix, no suppression. Deleted the local `parseBody` duplicate and imported the shared helper from `shared/validation`, which seven sibling handlers already use. One implementation, one rationale; the 400 message stays repo-consistent. - `agent/src/observability.py` — repaired the marker AND made the degrade observable. A tracer fault and "no recording span" both yield `None` and a caller cannot tell them apart, so a `log("WARN", ...)` breadcrumb is now the only place that difference survives. Raising is not an option: callers read this inside DDB-write try-blocks, where it would be misclassified as a DDB failure and trip the shared progress circuit breaker (#245 review). - `agent/src/server.py` — marker only. `/terminate` must answer 200 for ANY body (ADR-021 P2-F8) and `""` is the EXPECTED production id; the unreadable body is already logged. - `cdk/src/handlers/shared/linear-oauth-resolver.ts` — marker only. `null` IS this resolver's documented failure encoding; no success path returns `null`, so it can never be read as an empty success, and the cause is logged with full context immediately above. - `cdk/src/handlers/shared/orchestration-store.ts` — marker only. A corrupt stored JSON blob is permanent, not transient: failing closed would make every later read of that epic throw, so an epic mid-flight could never settle and no retry could clear it. The attachments are advisory context for the children. - `cli/src/linear-oauth.ts` — marker only, and this one supersedes my earlier claim on #756 that the site was unfixable-by-cleanup. `isNotFound` narrows to `ResourceNotFoundException`, so this is an answered read with an empty answer; every other error throws on the next line (fail closed, #612 review B1). The rule flags conditional re-raise deliberately — see `masked_conditional_reraise` in `.semgrep/silent-success-masking.py` — so the allowlist is its intended remedy for this shape. #790 is not a prerequisite. Markers proven load-bearing, not decorative: stripping the `observability.py` marker in a scratch copy and re-scanning with the same rule brings the finding straight back, and `semgrep test .semgrep/` still passes 2/2 (12 `ruleid:` fixtures would fail if the rule had stopped matching). Verified: `security:sast:masking` rc=0 (whole repo); ruff/ruff-format/ty clean; agent pytest 1783 passed with the single failure being a #855 git-fixture-leak artifact (`test_registry_loader.py` passes 27/27 in a scrubbed env); cdk jest 134 passed across the three touched suites; cli jest 55 passed; `//cdk:compile`, `//cli:compile` and both eslint `--fix` legs clean with zero mutations. Co-Authored-By: Claude Opus 5 --- agent/src/observability.py | 18 ++++++++++--- agent/src/server.py | 1 + agent/tests/test_observability.py | 27 +++++++++++++++++++ cdk/src/handlers/registry-publish.ts | 16 +++++------ .../handlers/shared/linear-oauth-resolver.ts | 2 +- .../handlers/shared/orchestration-store.ts | 2 +- cli/src/linear-oauth.ts | 2 +- 7 files changed, 52 insertions(+), 16 deletions(-) diff --git a/agent/src/observability.py b/agent/src/observability.py index 526c437d5..81e86af5e 100644 --- a/agent/src/observability.py +++ b/agent/src/observability.py @@ -15,6 +15,8 @@ from opentelemetry import baggage, context, trace from opentelemetry.trace import StatusCode +from shell import log + if TYPE_CHECKING: from collections.abc import Generator @@ -69,6 +71,12 @@ def current_otel_trace_id() -> str | None: propagating. Callers read this inside DDB-write try-blocks (progress_writer), where a raised trace error would otherwise be misclassified as a DDB failure and trip the shared progress circuit breaker. + + The degrade is *logged*, not silent (#756): a tracer fault and "no recording + span" both yield ``None``, and a caller cannot tell them apart, so the WARN + line is the only place that difference survives. It is deliberately emitted + per occurrence rather than once — this runs per progress event, so repetition + is the signal that the tracer is broken for the whole task, not one event. """ try: span = trace.get_current_span() @@ -80,9 +88,13 @@ def current_otel_trace_id() -> str | None: # the X-Ray console renders trace ids as ``1-{8hex}-{24hex}``; to look this # up there, transform to that form (the timestamp is the first 8 hex chars). return trace.format_trace_id(ctx.trace_id) - except Exception: - # nosemgrep: py-silent-success-masking -- trace id is a graceful-missing - # correlation field; a tracer fault must not fail the caller's write path. + except Exception as exc: + log( + "WARN", + f"current_otel_trace_id: tracer fault ({type(exc).__name__}: {exc}); " + "persisting the record without a correlation id", + ) + # nosemgrep: py-silent-success-masking -- graceful-missing correlation field, logged above; raising would surface inside the caller's DDB-write try-block and trip the progress circuit breaker (see docstring) # noqa: E501 return None diff --git a/agent/src/server.py b/agent/src/server.py index 50942d8f8..9357d13c6 100644 --- a/agent/src/server.py +++ b/agent/src/server.py @@ -1444,6 +1444,7 @@ def _parse_terminate_microvm_id(raw: bytes) -> str: # Not silent: an unreadable body is worth a breadcrumb even though it # cannot change the outcome. _emit_stdout_line(f"[server/warn] /terminate hook body is not JSON ({exc}); ignoring it") + # nosemgrep: py-silent-success-masking -- /terminate must answer 200 for ANY body (see docstring); "" is the EXPECTED production id, raising would report a hook failure for a teardown that succeeded, and the unreadable body is logged above # noqa: E501 return "" if not isinstance(parsed, dict): _emit_stdout_line( diff --git a/agent/tests/test_observability.py b/agent/tests/test_observability.py index d07b8d498..aa7bbcd52 100644 --- a/agent/tests/test_observability.py +++ b/agent/tests/test_observability.py @@ -54,6 +54,33 @@ def test_degrades_to_none_when_tracer_raises(self): ): assert observability.current_otel_trace_id() is None + def test_logs_the_tracer_fault_rather_than_degrading_silently(self, capfd): + # The degrade is justified but must not be invisible (#756): a tracer + # fault and "no recording span" both return None, so the WARN line is the + # only surviving evidence of which one happened. capfd (not capsys) + # because ``shell.log`` writes at the fd level — see its docstring. + with patch.object( + observability.trace, "get_current_span", side_effect=RuntimeError("tracer boom") + ): + assert observability.current_otel_trace_id() is None + out = capfd.readouterr().out + assert "current_otel_trace_id: tracer fault" in out + assert "RuntimeError" in out + assert "tracer boom" in out + + def test_does_not_log_when_there_is_simply_no_recording_span(self, capfd): + # The common, uninteresting case: tracing disabled locally. An invalid + # span context is not a fault, so it must stay quiet — otherwise every + # progress event in a non-traced run emits a WARN and the real fault + # signal above becomes noise. + span = MagicMock() + ctx = MagicMock() + ctx.is_valid = False + span.get_span_context.return_value = ctx + with patch.object(observability.trace, "get_current_span", return_value=span): + assert observability.current_otel_trace_id() is None + assert "tracer fault" not in capfd.readouterr().out + class TestPropagateCorrelationContext: """``propagate_correlation_context`` propagates the correlation envelope diff --git a/cdk/src/handlers/registry-publish.ts b/cdk/src/handlers/registry-publish.ts index b23a1a7f4..786964e19 100644 --- a/cdk/src/handlers/registry-publish.ts +++ b/cdk/src/handlers/registry-publish.ts @@ -31,6 +31,7 @@ import { REGISTRY_KINDS, RESERVED_KINDS, parseConstraint } from './shared/regist import { RegistryPublishIncompleteError, type PublishInput, type RuntimePayload } from './shared/registry/types'; import { ErrorCode, errorResponse, successResponse } from './shared/response'; import type { RegistryPublishRequest, RegistryRecordResponse } from './shared/types'; +import { parseBody } from './shared/validation'; const NAMESPACE_RE = /^[a-z][a-z0-9-]*$/; const NAME_RE = /^[a-z0-9][a-z0-9._-]*$/; @@ -52,7 +53,11 @@ export async function handler(event: APIGatewayProxyEvent): Promise(event.body); if (!body) { return errorResponse(400, ErrorCode.VALIDATION_ERROR, 'Request body must be valid JSON.', requestId); } @@ -108,15 +113,6 @@ export async function handler(event: APIGatewayProxyEvent): Promise