fix(security): close 3 watchdog findings (stub prefix, ReDoS, md)

[iamwhatever]

Problem / Motivation

Three security findings, each verified live against origin/main before this branch was cut, in two unrelated subsystems:

1. Model-hidden-tool filter bypass at stub registration (MEDIUM) — Backend exempts any stub uuid starting with INTERNAL_STUB_PREFIXES from both the MCP Apps render path and the model-visibility filter (backend.py:3294). _handle_stub_conn rejected only an empty stub_uuid, so a registrant could simply name itself with the reserved prefix and inherit both exemptions.
2. ReDoS in the markdown one-line fold (MEDIUM) — _md_one_line folded with re.sub(r"\s*\n\s*", " ", text). \s matches a newline too, so the two runs and the anchor compete for the same characters.
3. Markdown code-span breakout (MEDIUM) — _md_inline_code fenced ADF code-marked text but let interior newlines through verbatim.

Why it matters

#1 hands a registrant two exemptions it should never hold. The model-visibility filter is what withholds hidden tools from a tools/list, and its SEL audit is what records the withhold — so a stub that inherits the internal exemption is served the unfiltered surface and leaves no audit trail of the withhold that never happened.

#2 is a denial of service on provider-supplied content. _md_one_line is called per heading (:3501) and per table cell (:4007), and the content is bounded only by the 8MiB fetch cap. Measured against this branch's base: 200k spaces took 45.3s in a single call, 20k took 455ms. A document with many such cells stalls the handler for as long as the provider cares to make it.

#3 is a full escape from the markdown sanitizer. A blank line inside the code content ends the enclosing paragraph, and everything after it is parsed as fresh markdown — outside the fence, and so past _md_escape_inline, _md_link_target and the redaction gate. Whatever those three exist to stop is reachable through it.

What changed (motivation → approach → change)

#1 src/kiro_crew/mcp_gateway/gatewayd.py. The backend.py:3294 guard is correct and is untouched — the gap is registration-side, so that is where the fix goes. Immediately after the existing empty-stub_uuid reject, and in the same shape, a stub_uuid starting with INTERNAL_STUB_PREFIXES is refused with {"type": "rejected", "reason": "reserved stub_uuid prefix"} plus a warning. INTERNAL_STUB_PREFIXES is imported from backend rather than re-spelled, so adding a prefix there keeps registration and interception in agreement.

The refusal is also audited, via a new _audit_reserved_stub_prefix_denied beside its closest sibling _audit_peer_identity_denied. This is the one reject on this path that is a security control rather than a schema check: a malformed Register (:2413) and an empty stub_uuid (:2485) stay WARNING-only because nothing is being evaded, whereas claiming a reserved prefix is an attempt to acquire an exemption — the same class of access decision as the _audit_peer_denied / _audit_pool_rejected family this module already records. Leaving it unaudited would also have contradicted this PR's own rationale above, which counts the missing SEL record as part of the harm.

The peer-supplied stub_uuid is recorded as received rather than pre-sanitized, and that was traced rather than assumed: log_api_access runs resources through _redact_and_clip, which redacts and clips but does not strip control characters, so the escaping comes from the writer — sel.py:1094 serializes each event as json.dumps(asdict(event)) + "\n", and JSON escapes CR/LF. A forged log line is not reachable, so a sanitizing pass would be dead code.

No false-positive risk, checked rather than assumed: the gateway's own internal stubs are attached in-process — Backend.attach_stub for __app_call__ (app_call.py:95), probe_tool_surface for __tool_surface__ — and never arrive as a Register frame over the socket. Verified by grepping every use of both prefixes.

#2 _md_one_line. Replaced the regex with " ".join(seg for seg in (line.strip() for line in text.split("\n")) if seg). Split/strip/join reads each character a fixed number of times, so the pathological input is linear.

An intermediate pattern ([^\S\n]*\n[^\S\n]*(?:\n[^\S\n]*)*) was tried first and measured 4x worse — the leading run still backtracks through every offset. Recorded because it looks like the fix and is not one.

Output is unchanged, not merely believed to be: a differential harness compared old and new over 40,031 inputs — 31 hand-written edge cases plus 40,000 random strings over an alphabet of a, space, \t, \n, \r, \v, \f, backtick, \x85, \u00a0, \u2028, \x1c, \u3000 — with zero mismatches. That alphabet is load-bearing: str.split("\n") splits only on \n, where str.splitlines() would also split on \v, \f and \r, which the old pattern did not match.

input	old	new
200k spaces + x	45,349ms	0.34ms
20k spaces + x	455ms	0.04ms

#3 _md_inline_code. One line, text = re.sub(r"\r\n?|\n", " ", text), ahead of the fence computation, so every downstream step (fence length, boundary padding) sees content that cannot contain a line break. \r\n?|\n rather than \n alone because CommonMark ends a line on a bare CR and on CRLF too.

Collapsed rather than promoted to a fenced block: the caller is _adf_apply_marks, which composes an inline run, so a block would change the document structure at every call site — and the escape, not the rendering shape, is what has to hold.

Scope note. An earlier revision of this branch also carried a fix for the bare-name kiro-cli spawn in the gateway's unattended auto-update (src/kiro_crew/slack/gateway.py). It is dropped here because #7712 already owns that fix under #7704: same resolve_kiro_cli() approach, plus the asyncio.to_thread offload, plus the cli_server.py sibling call site and the docs/system-specs/modules/cli.md update this branch did not carry. Both touched the same lines, so a second copy would only conflict with it. The one thing this branch had that #7712 does not — an explicit env= on the spawn — is raised as a comment on #7712 rather than kept here.

Tests

• test_gatewayd_more_coverage.py::test_register_naming_a_reserved_stub_prefix_is_rejected — iterates INTERNAL_STUB_PREFIXES (asserting it is non-empty first, so it cannot pass vacuously) and pins the exact reject frame for each prefix.
• test_gatewayd_more_coverage.py::test_reserved_stub_prefix_rejection_is_audited — pins that the reject path emits the SEL event, not merely that the helper exists.
• test_mcp_gatewayd_coverage.py _AUDIT_CASES — the new emitter joins the shared table, inheriting both contracts every sibling emitter is held to: the documented operation name, and that a SecurityEventLog failure never propagates to the caller.
• test_source_providers.py::test_newline_in_a_code_span_cannot_break_out_of_the_fence — safe\n\n# Injected\n[x](javascript:alert(1)) yields a single-line span with no newline anywhere in the output.
• test_source_providers.py::test_carriage_return_in_a_code_span_is_collapsed_too — bare CR and CRLF, the two cases a \n-only fix would miss.
• test_source_providers.py::test_folding_a_long_whitespace_run_is_linear — 200k spaces under a 2.0s budget, ~100x the measured linear cost of 0.34ms, so it fails only on a return to quadratic scanning rather than on machine noise.

Manual verification

• Full suite on the affected surfaces: test_source_providers, test_gatewayd_more_coverage, test_gatewayd_diag, test_gatewayd_self_exit, test_mcp_gatewayd_coverage, test_source_providers_comment_guard, test_source_provider_plugin, test_slack_gateway, test_governance_updates, test_spawn_audit → 1155 passed.
• Two failures are pre-existing, not this diff's: test_provider_executable_accepts_user_owned_install and ..._symlinked_install fail on this host from home ownership. Reproduced identically on origin/main in a clean git worktree, and they pass in this PR's own CI run — environmental, outside this diff.
• Gates: flake8, isort, mypy --platform linux clean. black — the two test files fail --check on origin/main too under Python 3.12 against a py314 target, so it was scoped: black was run on a copy of each changed file and no reformat hunk overlaps a line this branch adds.
• Diff-scoped gates run with their base ref exported (BRAND_BASE_REF, FOCUS_CUE_BASE_REF, CHANGELOG_BASE_REF, HARNESS_BASE_REF = git merge-base HEAD origin/main) so they enforce rather than report — all four pass, each naming the base sha in its output. docs-lint: 259 files, pass. No doc or spec references the changed behaviour (grepped), so none needed updating.

Revert-verify

Every guard was mutated back to the defect and confirmed to fail, then restored and confirmed to pass:

finding	mutation	result
#1	delete the prefix reject	FAIL — AssertionError: __app_call__
#1 (audit)	delete the _audit_reserved_stub_prefix_denied call	FAIL — assert [] == ['__app_call__deadbeef']
#2	restore re.sub(r"\s*\n\s*", ...)	FAIL — assert (… - …) < 2.0 at 45.58s
#3	delete the collapse	FAIL — assert '\n' not in 'safe\n\n\# …', and the CR test on ab`

All four then pass on the restored tree, and each mutation script asserted the file was byte-identical to the original afterwards.

Related Issues

no linked issue: the findings came from a security watchdog sweep rather than filed issues. The dropped fourth finding is tracked separately as #7704 and fixed by #7712.

Pattern harvest

Rule candidate: review-prompt
Pattern: a regex whose quantified run can also match its own anchor (\s*\n\s*, .*X.*), making the run and the anchor compete and the scan quadratic.

Finding #2 reads as obviously correct and is a DoS on attacker-controlled input. The mis-fix attempted here — a variant that measured 4x worse — shows the trap survives a careful first attempt, so the reviewable signal is the shape of the pattern, not the intent behind it. A review prompt that flags "quantifier whose character class contains the anchor it is searching for" would catch both the original and the bad fix.

Findings #1 and #3 are each a single missing check at a boundary, and the general lesson ("validate at the door, not only at the consumer") is already project doctrine — no new rule.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable) — N/A, no doc references the changed behaviour
• No secrets, credentials, or internal references in the diff

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of 5f0b886f0cdbc61b7add473345756b7ef35fb3c8 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Sound fixes at the right layers, but the "reserved namespace" invariant is only half-closed: the equality sentinels remain claimable at the same door.

Watch

• The new reject enforces "the prefix is not a namespace a client may enter" only for INTERNAL_STUB_PREFIXES, yet backend.py gives the same internal treatment by equality to __init__, __apps__ (_APPS_STUB_SENTINEL), and __release__ (_RELEASE_STUB_SENTINEL) — e.g. if pending.stub_uuid == "__init__": await self._on_upstream_initialize(msg) (backend.py:2017). Registration validates nothing about stub_uuid shape beyond non-emptiness, so a stub registering as __init__ has every backend response to its own requests misrouted into gateway-internal initialize handling (and __apps__/__release__ responses swallowed by their sentinel arms) — the same acquire-internal-treatment-by-naming pattern this PR fixes, with a weaker but real state-confusion consequence.

Suggestions

• Extend the registration reject to the three equality sentinels (or fold them into one reserved-names structure beside INTERNAL_STUB_PREFIXES), so the door check and the consumer checks name the same reserved set.

[DESIGN-REVIEWED] 5f0b886

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 5f0b886f0cdbc61b7add473345756b7ef35fb3c8 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims verified: the internal prefixes are only minted in-process (app_call.py:95, backend.py:1656), the Register frame is the single external entry for a stub name, the \s+ patterns elsewhere are single-run and linear (no ReDoS siblings), and the audit helper joins an existing per-decision emitter family rather than duplicating one. One piece of now-dead code rode in with fix #3.

First-Principles-Verdict: PASS

Three named, measured defects, each fixed at its mechanism with the single rider (the SEL audit) grounded in the PR's own harm statement.

What this change ships

Intent: close three verified security findings — a stub-prefix exemption grab, a quadratic markdown fold, and a code-span fence escape. FIX.

1. A registrant naming a reserved internal stub prefix is refused at the door — justified
2. That refusal lands in the security event log (mcp-gateway.reserved-stub-prefix-denied) — declared; justified (the missing audit record is part of harm test: validate CI workflows on KiroCrew #1, and the SEL is a documented keep)
3. Folding provider headings/table cells no longer stalls on long whitespace runs (measured 45.3s → 0.34ms) — justified
4. A line break inside a rendered code span becomes a space instead of new document structure — justified

Depth checks out: grepped attach_stub (5 call sites) — both internal prefixes are minted in-process only, and the Register frame is the sole external entry for a stub name, so the refusal sits at the choke point, not a call site. Grepped \s*\n|\n\s* under src/: 0 sibling patterns with the competing-run shape, so the regex replacement leaves no unfixed siblings. The audit emitter is not a second spelling of _audit_peer_identity_denied — different decision, same family table. The dropped fourth fix (deferred to #7712) is the subtraction this lane would have asked for, already taken.

Subtractions

• Drop "\n" from the first/last edge tuples in _md_inline_code (source_providers.py:3367) — the collapse at :3364 makes a newline unreachable there; " " alone suffices.

[FIRST-PRINCIPLES-REVIEWED] 5f0b886

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 5f0b886f0cdbc61b7add473345756b7ef35fb3c8 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 5f0b886

Verdict parsed from the review's SHA-scoped output markers for commit 5f0b886f0cdbc61b7add473345756b7ef35fb3c8.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 5f0b886f0cdbc61b7add473345756b7ef35fb3c8: <one-sentence reason>

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 5f0b886f0cdbc61b7add473345756b7ef35fb3c8 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 5f0b886

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 5f0b886f0cdbc61b7add473345756b7ef35fb3c8: <one-sentence reason>

[iamwhatever]

Scope reduced: the kiro-cli auto-update fix is dropped from this PR — #7712 owns it.

Revision 1 of this branch carried a fourth fix: the bare-name kiro-cli spawn in _auto_apply_update (src/kiro_crew/slack/gateway.py), resolved via resolve_kiro_cli(). Both GPT 5.6 blocking findings on that revision landed on that one hunk, and looking into them turned up the reason: #7712 already owns this fix, under #7704, opened two days before this branch.

#7712 is a strict superset of what I had:

	this branch (rev 1)	#7712
resolve_kiro_cli() in the gateway	yes	yes
await asyncio.to_thread(...) offload	no	yes, pinned by a test that asserts the resolve thread is not the loop thread
cli_server.py sibling call site	no	yes
docs/system-specs/modules/cli.md	no	yes
explicit env= on the spawn	yes	no

It also touches the same lines, so both landing would conflict — and #7712 has already worked through the residual/security objection on the record, with the is_sensitive_path / is_sensitive_write_path predicates actually run rather than argued from memory.

So this PR now carries only the three findings that overlap nothing: the reserved stub-uuid prefix at MCP-gateway registration, and the two markdown ones in source_providers.py. src/kiro_crew/slack/gateway.py and test/test_slack_gateway.py are byte-identical to origin/main again — verified with git diff origin/main -- <those two paths> returning empty, so shutil.which("kiro-cli") is back to main's state for #7712 to fix.

The one thing rev 1 had that #7712 does not — passing an explicit env= to the spawn instead of inheriting — is raised as a comment on #7712 rather than kept here.

[iamwhatever]

• BLOCKING -- src/kiro_crew/mcp_gateway/gatewayd.py:2498 -- Reserved-prefix rejection is not audited (span=0e21ddd67f91; Anchor: backend-security-controls) — fixed in 5f0b886f0cdb.

if stub_uuid.startswith(INTERNAL_STUB_PREFIXES): ... logger.warning(...); return
Reserved-prefix Register -> rejection branch -> denied connection is absent from SEL.
Anchor: backend-security-controls
Fix: Emit a denied SEL event before returning.

Legitimate, and I checked the distinction rather than pattern-matching on "nearby rejects don't audit".

The two sibling rejects on this path — a malformed Register (:2413) and an empty stub_uuid (:2485) — are logger.warning only, and that is correct for them: they are schema failures, where nothing was being evaded. Mine is different in kind. The prefix is what decides whether Backend treats a request as the gateway's own and skips both the MCP Apps render path and the model-visibility filter, so a registrant claiming one is attempting to acquire an exemption. That is an access decision, and this module audits access decisions — _audit_peer_denied, _audit_peer_identity_denied, _audit_pool_rejected and _audit_recaller_rejected all exist for exactly that.

It also lands against my own stated rationale: this PR's description argues the bypass matters partly because it leaves "no SEL record of the withhold that never happened". Refusing to audit my own denial of that bypass would have contradicted the reason the fix exists.

What changed. Added _audit_reserved_stub_prefix_denied(stub_uuid), placed beside _audit_peer_identity_denied because they are the closest siblings — both register-path denials keyed on the peer-supplied stub_uuid — and modelled on it exactly: log_api_access(caller="unverified-peer", operation="mcp-gateway.reserved-stub-prefix-denied", outcome="denied", source="gateway", resources=f"stub_uuid={stub_uuid}"), wrapped in the same try/except + logger.debug so an audit failure can never break connection handling. Called before the return.

On the peer-supplied stub_uuid reaching the log, since it is fully attacker-controlled and this is the log-injection shape: I traced the writer rather than assuming. SecurityEventLog.log_api_access passes resources through _redact_and_clip, which redacts secrets and clips but does not strip control characters — so the escaping has to come from somewhere else, and it does: the event is serialized at sel.py:1094 as json.dumps(asdict(event)) + "\n", and JSON escapes CR/LF. A forged log line is therefore not reachable and pre-sanitizing would be dead code. The logger.warning beside it already uses %r, which escapes too.

Pinned by two tests, not left to review:

• test_gatewayd_more_coverage.py::test_reserved_stub_prefix_rejection_is_audited asserts the reject path emits it. The helper merely existing is not the fix, so this stubs _audit_reserved_stub_prefix_denied and asserts it received the offending uuid.
• The emitter is added to test_mcp_gatewayd_coverage.py's shared _AUDIT_CASES table, which holds every sibling emitter to two contracts: the documented operation name, and that a SecurityEventLog blowing up never propagates to the caller.

Revert-verify: removing just the _audit_reserved_stub_prefix_denied(stub_uuid) call fails the first test on assert [] == ['__app_call__deadbeef']; restoring it passes, and the mutation script confirmed the file was byte-identical afterwards.

Gates re-run on the rebased head (origin/main = 74eab0ca9af3): flake8, isort, mypy --platform linux, and the four diff-scoped gates with their BASE_REFs exported all exit 0; 874 tests pass across the source-provider and mcp-gateway files, with only the two pre-existing test_provider_executable_accepts_* environment failures that also fail on origin/main here and pass in CI.

[bolichen97]

Approving on the strength of a full readiness audit of every open PR against main, not a
line-by-line reading of this diff — recording that plainly so the next reader knows what this
stamp does and does not cover.

Verified against this exact head SHA:

• readiness: passed present, and PR Readiness — the one required status context on main
  (ruleset protected-branches) — is success on this head.
• No check run on this head is failure, cancelled, timed_out or still in flight. Skipped
  jobs are path-filtered conditionals, none of them required.
• mergeable: true, and the head is not far enough behind main for its green CI to describe a
  base that no longer exists.
• No surviving reviewer CHANGES_REQUESTED: any such review is on an older commit and therefore
  already dismissed by dismiss_stale_reviews_on_push.
• Every issue comment, inline review comment and review thread was read and classified. Nothing
  left is an unresolved human change request — the remainder is bot review-lane output, resolved
  or outdated threads, explicitly non-blocking suggestions, and author status notes.

Auto-merge (squash) is armed, so this lands once every other ruleset requirement is met.