fix(webui): validate externally influenced event_type at the timeline boundary (#637)
Review #526 found that event_type reached the serialized timeline payload without crossing the redaction/validation boundary every other free-text field on the same event crosses. A synthetic secret-shaped 40-hex canary was redacted through message but survived verbatim through event_type on the same control-plane record. CTH heading path: CTH_TYPES is now the single authority for what a CTH type may be. canonical_thread_handoff.is_known_cth_type() is that authority, used by format_cth_body (write), assess_cth_comment (assess), and now the read path too; parse_cth_comment reports membership as cth_type_known and stays total. adapt_cth_comments serializes handoff:<type> only for a declared type and otherwise emits the constant handoff:unrecognized, so arbitrary, malformed, secret-shaped, or whitespace-manipulated heading content never becomes an event_type. Control-plane path: a stored event_type is treated as source data. _safe_cp_event_type accepts only an ordinary identifier that is not a bare secret-shaped hex run and that a redaction pass leaves unchanged; anything else fails closed to the constant unsafe:redacted and marks the event sensitive. The value is never emitted verbatim, never partially sanitized, and never rewritten into a different valid-looking type. Remaining serialized-field audit: event_key ids must be plain numeric identifiers, the CTH adapter refuses a scope it cannot express, per-source failure reasons are redacted (they can quote an authenticated fetch error), and echoed scope/filter values are guarded so reflection is not a bypass. Legitimate values are preserved: every declared CTH type, the real producer types (assigned, lease_released, lease_adopted, dependency_edge_state_change, allocation, pr.opened, lease.renew), and the existing issue, PR, evidence, and full-SHA references. Tests seed the canary independently through both event_type paths with a benign message, so message redaction cannot be why they pass; each inspects event_type directly, asserts the canary is absent from the complete serialized payload, and asserts scan_for_secrets finds nothing. tests/test_webui_timeline.py 64 passed, 15 subtests pytest -k webui 362 passed, 285 subtests pytest -k redact 79 passed tests/test_canonical_thread_handoff.py tests/test_control_plane_db.py 29 passed Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This commit is contained in:
Claude Opus 4.8
+135
-13
@@ -203,6 +203,28 @@ _REF_SHA_SHAPE = re.compile(r"^[0-9a-f]{7,40}$")
|
||||
# material wherever it appears, never as an identifier.
|
||||
_BARE_SECRET_SHAPE = re.compile(r"^[0-9a-f]{32,}$")
|
||||
|
||||
# An event type reads like an identifier, but a stored one is externally
|
||||
# influenced: any producer that writes the control-plane ``events`` table
|
||||
# chooses the string. It reaches ``to_dict`` verbatim, so it is validated here
|
||||
# rather than trusted because of where it came from.
|
||||
_CP_EVENT_TYPE_SHAPE = re.compile(r"^[A-Za-z][A-Za-z0-9._:+-]{0,63}$")
|
||||
|
||||
# Emitted in place of a value that cannot be proven safe. Deliberately not a
|
||||
# plausible workflow type: an unsafe value is refused, never quietly rewritten
|
||||
# into a different valid-looking one that would misdescribe the record.
|
||||
UNSAFE_EVENT_TYPE = "unsafe:redacted"
|
||||
|
||||
# Emitted for a CTH heading that is not a declared member of ``CTH_TYPES``. The
|
||||
# contract is enforced on write (``format_cth_body``) and on assess; the read
|
||||
# path the timeline uses enforces it too rather than assuming it was.
|
||||
UNKNOWN_HANDOFF_EVENT_TYPE = "handoff:unrecognized"
|
||||
|
||||
# A source record id is a plain integer in both sources it comes from: the
|
||||
# control-plane ``events`` primary key and a Gitea comment id. ``event_key`` is
|
||||
# serialized verbatim and is the pagination tiebreak, so anything else is
|
||||
# refused rather than interpolated into it.
|
||||
_RECORD_ID_SHAPE = re.compile(r"^[0-9]{1,19}$")
|
||||
|
||||
|
||||
def _kind_to_numbers(kind: str | None, number: int | None) -> tuple[int | None, int | None]:
|
||||
"""Map a control-plane work-item (kind, number) to (issue_no, pr_no)."""
|
||||
@@ -297,6 +319,64 @@ def _safe_session_id(value: Any) -> str | None:
|
||||
return text if _redact(text) == text else None
|
||||
|
||||
|
||||
def _safe_record_id(value: Any) -> str | None:
|
||||
"""Return a source record id only when it is a plain numeric identifier.
|
||||
|
||||
``event_key`` is serialized verbatim and is the deterministic pagination
|
||||
tiebreak, so an id is interpolated into it only when it has the shape both
|
||||
real sources actually produce. A record whose identity cannot be trusted is
|
||||
refused by the caller rather than keyed on.
|
||||
"""
|
||||
if value is None or isinstance(value, bool):
|
||||
return None
|
||||
if isinstance(value, int):
|
||||
return str(value)
|
||||
text = str(value).strip()
|
||||
return text if _RECORD_ID_SHAPE.match(text) else None
|
||||
|
||||
|
||||
def _safe_cp_event_type(value: Any) -> tuple[str, bool]:
|
||||
"""Validate a stored control-plane event type. Returns ``(type, unsafe)``.
|
||||
|
||||
The stored value is externally influenced — whichever producer wrote the
|
||||
``events`` row chose the string — and ``to_dict`` serializes it verbatim, so
|
||||
it passes a boundary of its own instead of relying on the one ``message``
|
||||
passes. A value survives only when it is an ordinary identifier, is not a
|
||||
bare secret-shaped hex run, and is unchanged by a redaction pass. Anything
|
||||
else fails closed to :data:`UNSAFE_EVENT_TYPE`: the record stays visible as
|
||||
an audit entry, but the value itself is never republished — not verbatim,
|
||||
not partially sanitized, and not rewritten into some other valid-looking
|
||||
type that would misdescribe what happened.
|
||||
"""
|
||||
text = ("" if value is None else str(value)).strip()
|
||||
if not text:
|
||||
return ("", False)
|
||||
if _BARE_SECRET_SHAPE.match(text):
|
||||
return (UNSAFE_EVENT_TYPE, True)
|
||||
if not _CP_EVENT_TYPE_SHAPE.match(text):
|
||||
return (UNSAFE_EVENT_TYPE, True)
|
||||
if _redact(text) != text:
|
||||
return (UNSAFE_EVENT_TYPE, True)
|
||||
return (text, False)
|
||||
|
||||
|
||||
def _safe_echo(value: Any) -> Any:
|
||||
"""Guard a scalar that is echoed back rather than derived from a record.
|
||||
|
||||
Query scope and filter values are caller-supplied and are reflected in the
|
||||
response so an operator can see what was asked. Reflection is still
|
||||
emission: a value redaction would alter, or a bare secret-shaped hex run, is
|
||||
replaced by the placeholder instead of being echoed verbatim. Ordinary
|
||||
scope and filter values pass through untouched.
|
||||
"""
|
||||
if value is None or isinstance(value, (int, bool)):
|
||||
return value
|
||||
text = str(value)
|
||||
if _BARE_SECRET_SHAPE.match(text.strip()):
|
||||
return console_redaction.REDACTED
|
||||
return _redact(text)
|
||||
|
||||
|
||||
def adapt_cp_events(rows: Iterable[dict[str, Any]]) -> list[WorkflowEvent]:
|
||||
"""Adapt control-plane ``events`` rows (joined to work_items) into events.
|
||||
|
||||
@@ -307,14 +387,20 @@ def adapt_cp_events(rows: Iterable[dict[str, Any]]) -> list[WorkflowEvent]:
|
||||
events: list[WorkflowEvent] = []
|
||||
for row in rows or []:
|
||||
try:
|
||||
event_id = row.get("event_id")
|
||||
event_type = (row.get("event_type") or "").strip()
|
||||
if event_id is None or not event_type:
|
||||
event_id = _safe_record_id(row.get("event_id"))
|
||||
raw_event_type = (row.get("event_type") or "").strip()
|
||||
if event_id is None or not raw_event_type:
|
||||
continue
|
||||
# The stored type is source data, not a trusted constant: validate
|
||||
# it before it is serialized, exactly as `message` below is redacted
|
||||
# before it is serialized.
|
||||
event_type, event_type_unsafe = _safe_cp_event_type(raw_event_type)
|
||||
kind = row.get("kind")
|
||||
number = row.get("number")
|
||||
issue_no, pr_no = _kind_to_numbers(kind, number)
|
||||
sensitive = any(hint in event_type.lower() for hint in _SENSITIVE_EVENT_HINTS)
|
||||
sensitive = event_type_unsafe or any(
|
||||
hint in raw_event_type.lower() for hint in _SENSITIVE_EVENT_HINTS
|
||||
)
|
||||
events.append(
|
||||
WorkflowEvent(
|
||||
source=SOURCE_CONTROL_PLANE,
|
||||
@@ -355,7 +441,18 @@ def adapt_cth_comments(
|
||||
"""
|
||||
# Imported lazily so this module has no import-time dependency on the
|
||||
# handoff parser when only the control-plane adapter is used.
|
||||
from canonical_thread_handoff import parse_cth_comment
|
||||
from canonical_thread_handoff import is_known_cth_type, parse_cth_comment
|
||||
|
||||
# ``kind``/``number`` are interpolated into event_key and correlation_id, so
|
||||
# they are normalised once here. A scope this adapter cannot express is
|
||||
# refused outright rather than serialized into an identifier.
|
||||
kind = (kind or "").strip().lower()
|
||||
if kind not in ("issue", "pr"):
|
||||
return []
|
||||
try:
|
||||
number = int(number)
|
||||
except (TypeError, ValueError):
|
||||
return []
|
||||
|
||||
issue_no, pr_no = _kind_to_numbers(kind, number)
|
||||
correlation = _correlation_for(kind, number)
|
||||
@@ -367,8 +464,17 @@ def adapt_cth_comments(
|
||||
if not parsed:
|
||||
continue
|
||||
fields = parsed.get("fields") or {}
|
||||
cth_type = parsed.get("cth_type") or "handoff"
|
||||
comment_id = comment.get("id")
|
||||
cth_type = parsed.get("cth_type") or ""
|
||||
comment_id = _safe_record_id(comment.get("id"))
|
||||
if comment_id is None:
|
||||
continue
|
||||
# The CTH heading is free text: the parser accepts whatever follows
|
||||
# "## CTH:", and only the write and assess paths check it against
|
||||
# the contract. Check it here too — an unrecognised heading is
|
||||
# reported as such rather than serialized into event_type, so
|
||||
# arbitrary, malformed, or secret-shaped heading content has no way
|
||||
# through. Declared types are preserved exactly.
|
||||
cth_type_known = is_known_cth_type(cth_type)
|
||||
# Redaction runs first, and every derived value is taken from the
|
||||
# redacted text — deriving evidence refs from the raw proof would
|
||||
# re-emit exactly what redaction was about to remove.
|
||||
@@ -381,7 +487,11 @@ def adapt_cth_comments(
|
||||
events.append(
|
||||
WorkflowEvent(
|
||||
source=SOURCE_GITEA_HANDOFF,
|
||||
event_type=f"handoff:{cth_type}",
|
||||
event_type=(
|
||||
f"handoff:{cth_type.strip()}"
|
||||
if cth_type_known
|
||||
else UNKNOWN_HANDOFF_EVENT_TYPE
|
||||
),
|
||||
event_key=f"cth:{kind}:{number}:{comment_id}",
|
||||
timestamp=_parse_ts(comment.get("created_at")),
|
||||
actor=_redact((comment.get("user") or {}).get("login")),
|
||||
@@ -396,7 +506,7 @@ def adapt_cth_comments(
|
||||
message=next_action or _redact(fields.get("status")),
|
||||
correlation_id=correlation,
|
||||
evidence_refs=refs,
|
||||
sensitive=refs_dropped,
|
||||
sensitive=refs_dropped or not cth_type_known,
|
||||
)
|
||||
)
|
||||
except Exception:
|
||||
@@ -453,7 +563,10 @@ def _cp_status(*, ok: bool, reason: str | None = None, count: int = 0) -> Source
|
||||
return SourceStatus(
|
||||
SOURCE_CONTROL_PLANE,
|
||||
ok=ok,
|
||||
reason=reason,
|
||||
# A failure reason is serialized like any other field and is often an
|
||||
# exception string carrying a path or a transport error, so it crosses
|
||||
# the redaction boundary too. Static reasons pass through unchanged.
|
||||
reason=_redact(reason),
|
||||
count=count,
|
||||
supported_filters=_SOURCE_FILTER_SUPPORT[SOURCE_CONTROL_PLANE],
|
||||
)
|
||||
@@ -463,7 +576,9 @@ def _handoff_status(*, ok: bool, reason: str | None = None, count: int = 0) -> S
|
||||
return SourceStatus(
|
||||
SOURCE_GITEA_HANDOFF,
|
||||
ok=ok,
|
||||
reason=reason,
|
||||
# Same boundary as the control-plane status: this reason can quote an
|
||||
# error raised by a live authenticated fetch.
|
||||
reason=_redact(reason),
|
||||
count=count,
|
||||
supported_filters=_SOURCE_FILTER_SUPPORT[SOURCE_GITEA_HANDOFF],
|
||||
)
|
||||
@@ -623,8 +738,15 @@ class TimelineSnapshot:
|
||||
"ok": self.ok,
|
||||
"error": self.error,
|
||||
"schema_version": self.schema_version,
|
||||
"scope": {"remote": self.remote, "org": self.org, "repo": self.repo},
|
||||
"filters": self.filters,
|
||||
# Scope and filters are echoed caller input, not derived record
|
||||
# data. Reflecting a value is still emitting it, so both cross the
|
||||
# same boundary; ordinary scope and filter values are unchanged.
|
||||
"scope": {
|
||||
"remote": _safe_echo(self.remote),
|
||||
"org": _safe_echo(self.org),
|
||||
"repo": _safe_echo(self.repo),
|
||||
},
|
||||
"filters": {key: _safe_echo(value) for key, value in self.filters.items()},
|
||||
"sources": [s.to_dict() for s in self.sources],
|
||||
**self.page.to_dict(),
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user