fix(s52): scrub every URL-bearing exception path in WebhookNotifier

This commit is contained in:
Kjell Tore Guttormsen 2026-07-17 03:15:57 +02:00
commit c48e2102d0
3 changed files with 38 additions and 8 deletions

View file

@ -18,6 +18,7 @@ from __future__ import annotations
import json import json
import sys import sys
from collections.abc import Callable from collections.abc import Callable
from http.client import HTTPException
from pathlib import Path from pathlib import Path
from typing import TYPE_CHECKING, Any, Protocol, TextIO, runtime_checkable from typing import TYPE_CHECKING, Any, Protocol, TextIO, runtime_checkable
from urllib.error import URLError from urllib.error import URLError
@ -126,10 +127,16 @@ class WebhookNotifier:
def __call__(self, verdict: Verdict) -> None: def __call__(self, verdict: Verdict) -> None:
try: try:
self._post(self._url, json.dumps(_verdict_payload(verdict), sort_keys=True)) self._post(self._url, json.dumps(_verdict_payload(verdict), sort_keys=True))
except (URLError, OSError) as exc: except (URLError, OSError, ValueError, HTTPException) as exc:
# STRICTER than ingest.py:314: the message NEVER carries the url — a Slack/Teams # STRICTER than ingest.py:314: NO exception type may carry the url past this point —
# webhook URL embeds the receiver secret (the original cause stays chained). # a Slack/Teams webhook URL embeds the receiver secret. The tuple covers every
raise NotifyError("webhook POST failed") from exc # URL-carrier _urllib_post can raise (ValueError incl. UnicodeError; HTTPException
# incl. InvalidURL; OSError incl. URLError/HTTPError/TimeoutError). The chain is
# severed (`from None`) because __cause__ renders in tracebacks/logging.exception;
# the exception type name in the message is the diagnostic substitute for the lost
# chain. Deliberately NOT a bare `except Exception`: the transport is injectable,
# and a test transport's AssertionError/TypeError (programming errors) must surface.
raise NotifyError(f"webhook POST failed ({type(exc).__name__})") from None
# --- config + factory (fail-fast, egress never config-grantable) ---------------------------------- # --- config + factory (fail-fast, egress never config-grantable) ----------------------------------

View file

@ -108,7 +108,8 @@ def test_webhook_posts_correct_payload() -> None:
def test_webhook_default_post_is_urllib_post() -> None: def test_webhook_default_post_is_urllib_post() -> None:
"""inspect.signature proves the stdlib default WITHOUT opening a socket — ``_urllib_post`` is """inspect.signature proves the stdlib default WITHOUT opening a socket — ``_urllib_post`` is
the single socket path and is never called by the offline suite.""" the single socket path; the offline suite never lets it reach a socket (the only in-suite
call uses a scheme-less URL that fails at parse time)."""
sig = inspect.signature(WebhookNotifier.__init__) sig = inspect.signature(WebhookNotifier.__init__)
assert sig.parameters["post"].default is _urllib_post assert sig.parameters["post"].default is _urllib_post

View file

@ -12,6 +12,7 @@ Seams pinned here:
from __future__ import annotations from __future__ import annotations
import ast import ast
import traceback
from pathlib import Path from pathlib import Path
import pytest import pytest
@ -109,18 +110,22 @@ def test_no_socket_call_outside_the_post_seam() -> None:
def test_webhook_error_never_leaks_url() -> None: def test_webhook_error_never_leaks_url() -> None:
"""LOAD-BEARING (secret discipline): a Slack/Teams webhook URL embeds the receiver secret, so """LOAD-BEARING (secret discipline): a Slack/Teams webhook URL embeds the receiver secret, so
the wrapped ``NotifyError`` must NOT contain it (STRICTER than ``ingest.py:314``, which may the wrapped ``NotifyError`` must NOT contain it (STRICTER than ``ingest.py:314``, which may
include its url). Positive control: the url IS delivered to a successful transport so the include its url). The injected ``OSError`` EMBEDS the url in its message, so the traceback
negative assertion has teeth. Detach (put the url in the error message) RED.""" assertion guards the severed cause-chain: restore ``from exc`` the URL-bearing cause renders
via ``traceback.format_exception`` RED. Positive control: the url IS delivered to a
successful transport so the negative assertion has teeth. Detach (put the url in the error
message, or restore ``from exc``) RED."""
url = "https://hooks.example.test/T000/B000/secret-token" url = "https://hooks.example.test/T000/B000/secret-token"
def failing_post(post_url: str, body: str) -> None: def failing_post(post_url: str, body: str) -> None:
raise OSError("connection refused") raise OSError(f"connection refused for {url}")
notifier = WebhookNotifier(url, allow_egress=True, post=failing_post) notifier = WebhookNotifier(url, allow_egress=True, post=failing_post)
with pytest.raises(NotifyError) as excinfo: with pytest.raises(NotifyError) as excinfo:
notifier(_verdict()) notifier(_verdict())
assert url not in str(excinfo.value) assert url not in str(excinfo.value)
assert url not in repr(excinfo.value) assert url not in repr(excinfo.value)
assert url not in "".join(traceback.format_exception(excinfo.value))
# Positive control: the same url reaches a working transport verbatim. # Positive control: the same url reaches a working transport verbatim.
posts: list[tuple[str, str]] = [] posts: list[tuple[str, str]] = []
@ -132,6 +137,23 @@ def test_webhook_error_never_leaks_url() -> None:
assert posts[0][0] == url assert posts[0][0] == url
def test_webhook_malformed_url_never_leaks_through_real_transport() -> None:
"""LOAD-BEARING (secret discipline, real transport): a scheme-less webhook URL makes the
DEFAULT ``_urllib_post`` raise a bare ``ValueError`` carrying the URL verbatim (``unknown url
type: 'hooks…secret'``) the raise happens at URL parsing, BEFORE any socket, so the suite
stays socket-free. Direct construction bypasses ``NotifierConfig``'s scheme gate, so the catch
in ``WebhookNotifier.__call__`` is the actual safety net. Detach points: narrow the catch back
to ``(URLError, OSError)`` the ValueError escapes unscrubbed RED; restore ``from exc``
the URL renders via ``__cause__`` in ``traceback.format_exception`` RED."""
url = "hooks.example.test/T000/B000/secret-token"
notifier = WebhookNotifier(url, allow_egress=True) # default _urllib_post, no injection
with pytest.raises(NotifyError) as excinfo:
notifier(_verdict())
assert "secret-token" not in str(excinfo.value)
assert "secret-token" not in repr(excinfo.value)
assert "secret-token" not in "".join(traceback.format_exception(excinfo.value))
def test_notify_registered_maf_free() -> None: def test_notify_registered_maf_free() -> None:
"""Meta: notify.py is registered in the MAF-free guard list, so ``test_okf_is_maf_free`` """Meta: notify.py is registered in the MAF-free guard list, so ``test_okf_is_maf_free``
actually scans it otherwise the MAF-free claim would be green-but-dead (never checked).""" actually scans it otherwise the MAF-free claim would be green-but-dead (never checked)."""