From e423dd5870637bc99785c35f7ed36c4f688c5b5a Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 29 Jul 2026 01:23:10 -0400 Subject: [PATCH] fix(mcp): remediate B8, B6/B11, and B15 break-glass restart blockers (#664) - B8: Correct redaction boundary for GITEA_TOKEN= and URI userinfo without destroying adjacent audit evidence or benign sec- text - B6/B11: Remove false restart execution claims from default executor when GITEA_SANCTIONED_RESTART_HOOK is non-empty - B15: Document deployable production grant set (runtime.break_glass_restart and gitea.issue.create) for prgs-controller - Preserve B13, B1, B14 and previously accepted corrections --- docs/mcp-restart-coordinator.md | 2 +- gitea_audit.py | 37 +++--- gitea_mcp_server.py | 38 +++--- tests/test_issue_664_break_glass_restart.py | 122 ++++++++++++++++++++ 4 files changed, 163 insertions(+), 36 deletions(-) diff --git a/docs/mcp-restart-coordinator.md b/docs/mcp-restart-coordinator.md index e7a5b87..06659f0 100644 --- a/docs/mcp-restart-coordinator.md +++ b/docs/mcp-restart-coordinator.md @@ -148,7 +148,7 @@ so a bypass is never silent. The dedicated MCP tool `gitea_break_glass_restart` provides the privileged emergency break-glass restart workflow when graceful drain cannot complete: -- **Authorization (#664 AC1 / B1 / B13)**: Requires the exact trusted profile `prgs-controller` **and** an explicit `runtime.break_glass_restart` grant enforced by the real production operation gate (no `gitea.read` fallback). Ordinary roles, non-controller reconcilers, lookalike profile names (`fake-controller`, …), and env vars cannot authorize. A narrow break-glass capability does **not** redefine the profile's declared global role. +- **Authorization (#664 AC1 / B1 / B13 / B15)**: Requires the exact trusted profile `prgs-controller` **and** explicit `runtime.break_glass_restart` and `gitea.issue.create` grants enforced by the real production operation gate (no `gitea.read` fallback). Incident creation is mandatory prior to execution (`gitea.issue.create`), so the deployable production policy for `prgs-controller` includes `allowed_operations`: `["gitea.read", "gitea.pr.close", "gitea.pr.comment", "gitea.issue.comment", "gitea.issue.create", "runtime.break_glass_restart", "gitea.branch.delete", "gitea.decision_lock.irrecoverable_recovery"]`. Ordinary roles, non-controller reconcilers, lookalike profile names (`fake-controller`, …), and env vars cannot authorize. A narrow break-glass capability does **not** redefine the profile's declared global role. Updating a live running `prgs-controller` profile in production requires an operator configuration update and daemon reload post-merge. - **Required Parameters (#664 AC2)**: - `reason`: Mandatory non-empty string (min 10 characters). - `confirmation`: Must equal exactly `"I_ACKNOWLEDGE_BREAK_GLASS_MCP_RESTART_DISRUPTION"`. diff --git a/gitea_audit.py b/gitea_audit.py index 133ecfb..d3e08cb 100644 --- a/gitea_audit.py +++ b/gitea_audit.py @@ -76,17 +76,16 @@ _BARE_SECRET_PATTERN = re.compile( r')\b' ) -# Key/value credentials embedded in free text (password=..., api_key: ..., etc.). -# Value group accepts quoted strings, Authorization scheme + credential -# (Bearer/Basic/Token + token), or a single non-space run. +# Key/value credentials embedded in free text (password=..., api_key: ..., GITEA_TOKEN=..., etc.). +# Group 1 captures the key name (e.g. GITEA_TOKEN, password, api_key). +# Group 2 captures delimiter/whitespace (=, : ). +# Group 3 captures the secret value, stopping at whitespace or non-secret delimiters (&, ;, ,, quotes, closing brackets). _ASSIGNMENT_SECRET_PATTERN = re.compile( - r'(?i)\b(' - r'token|password|passwd|pwd|secret|api[_-]?key|access[_-]?key|' - r'client[_-]?secret|private[_-]?key|authorization|credential' - r')\b(\s*[:=]\s*)(' + r'(?i)\b([A-Za-z0-9_]*?(?:token|password|passwd|pwd|secret|api[_-]?key|access[_-]?key|' + r'client[_-]?secret|private[_-]?key|authorization|credential))\b(\s*[:=]\s*)(' r'"[^"]*"|\'[^\']*\'|' - r'(?:Bearer|Basic|Token)\s+\S+|' - r'\S+' + r'(?:Bearer|Basic|Token)\s+[^\s;&,"\'\)\}\]\>]+|' + r'[^\s;&,"\'\)\}\]\>]+' r')' ) @@ -120,7 +119,8 @@ def redact_urls(text: str) -> str: if not isinstance(text, str) or not text: return text - url_pattern = re.compile(r'(https?://[^\s)>\]}]+)', re.IGNORECASE) + # Match any URI scheme (http, https, postgres, mysql, mongodb, redis, etc.) + url_pattern = re.compile(r'([a-z0-9\+\.\-]+://[^\s)>\]}]+)', re.IGNORECASE) def replace_url(match): url_str = match.group(1) @@ -134,11 +134,11 @@ def redact_urls(text: str) -> str: is_synthetic = True break - if is_synthetic: - # Rebuild synthetic URL to redact any credentials or query secrets + if is_synthetic or (parsed.username or parsed.password) or parsed.scheme.lower() not in ("http", "https"): + # Rebuild URL to redact any credentials or query secrets new_netloc = parsed.netloc if parsed.username or parsed.password: - netloc_clean = parsed.hostname + netloc_clean = parsed.hostname or "" if parsed.port: netloc_clean = f"{netloc_clean}:{parsed.port}" new_netloc = f"[REDACTED_USER]:[REDACTED_PASS]@{netloc_clean}" @@ -177,11 +177,17 @@ def redact_urls(text: str) -> str: def _mask_assignment(match: re.Match) -> str: """Keep the key and separator; replace only the secret value.""" + val = match.group(3) + if val.startswith(REDACTED) or val.startswith("%5BREDACTED") or val.startswith("[REDACTED"): + return f"{match.group(1)}{match.group(2)}{val}" return f"{match.group(1)}{match.group(2)}{REDACTED}" def _mask_conn_secret(match: re.Match) -> str: """Keep the connection-string key; replace only the credential value.""" + val = match.group(2) + if val.startswith(REDACTED) or val.startswith("%5BREDACTED") or val.startswith("[REDACTED"): + return f"{match.group(1)}{val}" return f"{match.group(1)}{REDACTED}" @@ -195,7 +201,8 @@ def _redact_str(text): """ if not isinstance(text, str) or not text: return text - out = _BARE_SECRET_PATTERN.sub(REDACTED, text) + out = redact_urls(text) + out = _BARE_SECRET_PATTERN.sub(REDACTED, out) out = _ASSIGNMENT_SECRET_PATTERN.sub(_mask_assignment, out) out = _CONN_STRING_SECRET_PATTERN.sub(_mask_conn_secret, out) out_lower = out.lower() @@ -212,7 +219,7 @@ def _redact_str(text): out = out[:i] + prefix + REDACTED + out[j:] out_lower = out.lower() idx = i + len(prefix) + len(REDACTED) - return redact_urls(out) + return out def redact(value): diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index bfd371f..fef35f1 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -23936,37 +23936,35 @@ _break_glass_restart_executor = None def _default_break_glass_restart_executor(request: dict) -> dict: """Canonical non-dry-run break-glass executor/delegate (#664 B6/B11). - Never kills, signals, or restarts the live MCP cohort in-process. When a - sanctioned host-managed restart hook reference is configured - (``GITEA_SANCTIONED_RESTART_HOOK``), the request is *delegated* to that - host supervisor and the contract reports proven execution only for the - accepted handoff. Without a configured hook, apply is unsupported. + Never kills, signals, or restarts the live MCP cohort in-process. + An environment string alone (``GITEA_SANCTIONED_RESTART_HOOK``) does not + prove restart execution without an active confirmed delegate handoff. """ hook = (os.environ.get("GITEA_SANCTIONED_RESTART_HOOK") or "").strip() - if not hook: + if hook: return { "success": False, - "apply_supported": False, - "apply_authorized": False, + "apply_supported": True, + "apply_authorized": True, "restart_performed": False, "break_glass_executed": False, - "execution_mode": "unsupported", + "execution_mode": "accepted_not_executed", + "host_hook_configured": True, "reasons": [ - "break-glass apply is unsupported: no sanctioned host restart " - "hook is configured (GITEA_SANCTIONED_RESTART_HOOK) (#664)" + "sanctioned host restart hook reference is configured, but environment text " + "cannot prove execution without a confirmed delegate handoff (fail closed) (#664 B6/B11)" ], } - # Opaque host reference only — never treat the hook string as a command. return { - "success": True, - "apply_supported": True, - "apply_authorized": True, - "restart_performed": True, - "break_glass_executed": True, - "execution_mode": "host_delegate_accepted", - "host_hook_configured": True, + "success": False, + "apply_supported": False, + "apply_authorized": False, + "restart_performed": False, + "break_glass_executed": False, + "execution_mode": "unsupported", "reasons": [ - "break-glass restart delegated to sanctioned host supervisor (#664)" + "break-glass apply is unsupported: no sanctioned host restart " + "executor is configured (#664)" ], } diff --git a/tests/test_issue_664_break_glass_restart.py b/tests/test_issue_664_break_glass_restart.py index 9401651..6e41823 100644 --- a/tests/test_issue_664_break_glass_restart.py +++ b/tests/test_issue_664_break_glass_restart.py @@ -855,6 +855,128 @@ class TestBreakGlassRestart(unittest.TestCase): entry2 = TASK_CAPABILITY_MAP.get("break_glass_restart") self.assertEqual(entry2["permission"], "runtime.break_glass_restart") + # ── B8 / B6 / B15 remediation tests ───────────────────────────────────── + + def test_b8_redaction_gitea_token_and_uri_credentials(self) -> None: + """B8: GITEA_TOKEN= and URI userinfo credentials redacted without erasing neighbours.""" + # GITEA_TOKEN= with underscore key + out1 = gitea_audit._redact_str("failed with GITEA_TOKEN=synthetic_tok_123456789") + self.assertIn("GITEA_TOKEN=[REDACTED]", out1) + self.assertNotIn("synthetic_tok_123456789", out1) + + # Connection string with URI userinfo + out2 = gitea_audit._redact_str("conn postgres://user:s3cr3tpw@db.internal:5432/app") + self.assertIn("postgres://[REDACTED_USER]:[REDACTED_PASS]@db.internal:5432/app", out2) + self.assertNotIn("s3cr3tpw", out2) + + # Value boundary preserving adjacent audit evidence (correlation_id, incident_number) + raw_audit = "password=secret123;correlation_id=bg-7f2a1c;incident_number=4242" + out3 = gitea_audit._redact_str(raw_audit) + self.assertIn("password=[REDACTED]", out3) + self.assertIn("correlation_id=bg-7f2a1c", out3) + self.assertIn("incident_number=4242", out3) + self.assertNotIn("secret123", out3) + + # Query param boundary in URL preserving adjacent parameters + raw_url = "token=abc-123&pr=908&issue=664&head=c67f39b4" + out4 = gitea_audit._redact_str(raw_url) + self.assertIn("token=[REDACTED]", out4) + self.assertIn("pr=908", out4) + self.assertIn("issue=664", out4) + self.assertIn("head=c67f39b4", out4) + + def test_b6_default_executor_environment_text_cannot_imply_execution(self) -> None: + """B6/B11: GITEA_SANCTIONED_RESTART_HOOK string alone returns break_glass_executed=False.""" + os.environ["GITEA_SANCTIONED_RESTART_HOOK"] = "this-string-is-never-invoked" + prof = _controller_profile() + p_parity, p_runtime, p_switch = _gate_open_patches() + with p_parity, p_runtime, p_switch: + with patch.object(gitea_mcp_server, "get_profile", return_value=prof), patch.object( + gitea_mcp_server, "_auth", return_value={"Authorization": "token test"} + ), patch.object( + gitea_mcp_server, "_authenticated_username", return_value="sysadmin" + ), patch.object( + gitea_mcp_server, "gitea_request_mcp_restart", + return_value={"affected_sessions": []}, + ), patch.object( + gitea_audit, "audit_enabled", return_value=True + ), patch.object( + gitea_audit, "write_event", return_value=True + ), patch.object( + gitea_mcp_server, "api_request", + return_value={"number": 555, "title": "[INCIDENT]"}, + ): + res = gitea_mcp_server.gitea_break_glass_restart( + reason="Privileged restart request with non-empty hook env var", + confirmation="I_ACKNOWLEDGE_BREAK_GLASS_MCP_RESTART_DISRUPTION", + impact_ack=True, + dry_run=False, + create_incident_issue=True, + remote="prgs", + ) + self.assertFalse(res["success"]) + self.assertFalse(res["performed"]) + self.assertFalse(res["break_glass_executed"]) + self.assertEqual(res["blocker_kind"], "restart_delegation_failed") + + def test_b15_production_prgs_controller_grant_set_and_gates(self) -> None: + """B15: Genuine prgs-controller carrying runtime.break_glass_restart and gitea.issue.create passes real gates.""" + # Full production-shaped prgs-controller profile + prod_profile = { + "profile_name": "prgs-controller", + "execution_profile": "prgs-controller", + "role": "reconciler", + "allowed_operations": [ + "gitea.read", + "gitea.pr.close", + "gitea.pr.comment", + "gitea.issue.comment", + "gitea.issue.create", + "runtime.break_glass_restart", + "gitea.branch.delete", + ], + "forbidden_operations": [ + "gitea.pr.approve", + "gitea.pr.merge", + "gitea.pr.create", + "gitea.branch.push", + ], + } + + # 1. Real _profile_operation_gate checks + p_parity, p_runtime, p_switch = _gate_open_patches() + with p_parity, p_runtime, p_switch: + with patch.object(gitea_mcp_server, "get_profile", return_value=prod_profile): + # Both required operations pass the real operation gate (no stubs) + self.assertEqual( + gitea_mcp_server._profile_operation_gate("runtime.break_glass_restart"), + [], + ) + self.assertEqual( + gitea_mcp_server._profile_operation_gate("gitea.issue.create"), + [], + ) + + # 2. Missing gitea.issue.create fails incident creation gate + no_issue_create = dict(prod_profile) + no_issue_create["allowed_operations"] = [ + "gitea.read", "gitea.pr.close", "runtime.break_glass_restart" + ] + with p_parity, p_runtime, p_switch: + with patch.object(gitea_mcp_server, "get_profile", return_value=no_issue_create): + res = gitea_mcp_server.gitea_break_glass_restart( + reason="Restart testing missing gitea.issue.create permission", + confirmation="I_ACKNOWLEDGE_BREAK_GLASS_MCP_RESTART_DISRUPTION", + impact_ack=True, + dry_run=False, + create_incident_issue=True, + remote="prgs", + ) + self.assertFalse(res["success"]) + self.assertEqual(res["blocker_kind"], "permission_denied") + self.assertIn("gitea.issue.create", " ".join(res.get("reasons") or [])) + if __name__ == "__main__": unittest.main() +