diff --git a/netx_api/auth_scopes.py b/netx_api/auth_scopes.py index 21a2df5..6210e3f 100644 --- a/netx_api/auth_scopes.py +++ b/netx_api/auth_scopes.py @@ -141,6 +141,9 @@ def required_scope_for_request(method: str, path: str) -> str | None: return SCOPE_NE_READ if p.startswith("/v1/topology"): + # Read-only path search uses POST for a structured body. + if m == "POST" and p.rstrip("/").endswith("/fabric/paths"): + return SCOPE_NE_READ if m in ("POST", "PUT", "PATCH", "DELETE"): return SCOPE_NE_WRITE return SCOPE_NE_READ diff --git a/netx_api/sql_guard.py b/netx_api/sql_guard.py index 3da0cbd..b60e6b1 100644 --- a/netx_api/sql_guard.py +++ b/netx_api/sql_guard.py @@ -47,6 +47,9 @@ def validate_select_sql(sql: str, *, allowed_tables: set[str] | None = None) -> low = cleaned.lower().lstrip() if not low.startswith(("select", "with")): raise HTTPException(status_code=400, detail="select_only") + # Plain WITH CTE is allowed; RECURSIVE can explode into DoS. + if re.search(r"\bwith\s+recursive\b", cleaned, flags=re.IGNORECASE): + raise HTTPException(status_code=400, detail="with_recursive_not_allowed") if _SQL_FORBIDDEN_RE.search(cleaned): raise HTTPException(status_code=400, detail="forbidden_keyword") # Block obvious catalog / other-schema probes in the text. diff --git a/netx_api/topology_fabric_nodes.py b/netx_api/topology_fabric_nodes.py index e3a7797..2b478f0 100644 --- a/netx_api/topology_fabric_nodes.py +++ b/netx_api/topology_fabric_nodes.py @@ -2,6 +2,7 @@ from __future__ import annotations import re +from collections import deque from datetime import datetime, timedelta from typing import Any from uuid import uuid4 @@ -509,25 +510,26 @@ def find_fabric_paths( adj.setdefault(e.a_node_id, []).append((e.b_node_id, e.id)) adj.setdefault(e.b_node_id, []).append((e.a_node_id, e.id)) - # DFS for simple paths (no repeated nodes), capped to avoid blow-up on large graphs. - _EXPLORE_CAP = 100 - all_paths: list[list[str]] = [] - stack: list[tuple[str, list[str], set[str]]] = [(from_id, [], {from_id})] - while stack and len(all_paths) < _EXPLORE_CAP: - node, edge_path, visited = stack.pop() + # BFS for simple paths so shorter hops are found first; cap expansions on dense graphs. + _EXPLORE_CAP = 5000 + found: list[list[str]] = [] + queue: deque[tuple[str, list[str], set[str]]] = deque([(from_id, [], {from_id})]) + explored = 0 + while queue and len(found) < max_paths and explored < _EXPLORE_CAP: + node, edge_path, visited = queue.popleft() if len(edge_path) >= max_hops: continue for nbr, eid in adj.get(node, []): if nbr in visited: continue + explored += 1 new_path = edge_path + [eid] if nbr == to_id: - all_paths.append(new_path) + found.append(new_path) + if len(found) >= max_paths: + break continue - stack.append((nbr, new_path, visited | {nbr})) - - all_paths.sort(key=len) - found = all_paths[:max_paths] + queue.append((nbr, new_path, visited | {nbr})) node_ids = {from_id, to_id} for p in found: diff --git a/netx_api/topology_service.py b/netx_api/topology_service.py index 301c318..431bc12 100644 --- a/netx_api/topology_service.py +++ b/netx_api/topology_service.py @@ -26,6 +26,7 @@ from .topology_fabric import ( ensure_fabric_node_for_managed, ensure_fabric_node_for_ume, ensure_lldp_discovered_managed_ne, + find_fabric_paths, get_fabric_neighborhood, get_fabric_summary, list_fabric_edges, diff --git a/packages/netx-mcp/src/netx_mcp/http_tools.py b/packages/netx-mcp/src/netx_mcp/http_tools.py index bc8986b..8954a61 100644 --- a/packages/netx-mcp/src/netx_mcp/http_tools.py +++ b/packages/netx-mcp/src/netx_mcp/http_tools.py @@ -498,21 +498,22 @@ HTTP_MCP_TOOLS: list[dict[str, Any]] = [ "name": "findTopologyPaths", "description": ( "Find up to max_paths simple paths between two fabric nodes for troubleshooting. " - "Accepts ume_ne_id (from UME alarms) or managed_ne_id — resolved to fabric node " - "internally. Returns paths with node sequence + edge status (up/down)." + "For each endpoint provide exactly one of ume_ne_id (from UME alarms) or " + "managed_ne_id — resolved to fabric node internally. Returns shortest paths " + "first with node sequence + edge status (up/down)." ), "inputSchema": { "type": "object", "properties": { - "from_ume_ne_id": {"type": "string", "description": "Source UME ne_id (from alarm ne_id)"}, - "from_managed_ne_id": {"type": "string", "description": "Source managed NE id"}, - "to_ume_ne_id": {"type": "string", "description": "Target UME ne_id"}, - "to_managed_ne_id": {"type": "string", "description": "Target managed NE id"}, + "from_ume_ne_id": {"type": "string", "description": "Source UME ne_id (from alarm ne_id); mutually exclusive with from_managed_ne_id"}, + "from_managed_ne_id": {"type": "string", "description": "Source managed NE id; mutually exclusive with from_ume_ne_id"}, + "to_ume_ne_id": {"type": "string", "description": "Target UME ne_id; mutually exclusive with to_managed_ne_id"}, + "to_managed_ne_id": {"type": "string", "description": "Target managed NE id; mutually exclusive with to_ume_ne_id"}, "max_paths": {"type": "integer", "minimum": 1, "maximum": 10, "default": 3}, "max_hops": {"type": "integer", "minimum": 1, "maximum": 12, "default": 6}, "layer": {"type": "string", "default": "physical"}, }, - "required": ["from_ume_ne_id", "to_ume_ne_id"], + "required": [], "additionalProperties": False, }, }, diff --git a/packages/netx-topology-mcp/src/netx_topology_mcp/layout_ops/orbit_sweep.py b/packages/netx-topology-mcp/src/netx_topology_mcp/layout_ops/orbit_sweep.py index c24a1c8..dd1faff 100644 --- a/packages/netx-topology-mcp/src/netx_topology_mcp/layout_ops/orbit_sweep.py +++ b/packages/netx-topology-mcp/src/netx_topology_mcp/layout_ops/orbit_sweep.py @@ -489,10 +489,24 @@ def orbit_sweep_node( scored.sort(key=_score_key) # Multi-objective re-rank: optimize total score, not just crossings - if objective == "total" and len(scored) > 1: + use_total = objective == "total" and len(scored) > 1 + base_clearance_hits = 0 + if use_total: + from netx_topology_mcp.layout_metrics import compute_edge_clearance + + ec0 = compute_edge_clearance(pos, links, names=names, top_n=1) + base_clearance_hits = int(ec0.get("edge_clearance_hits") or 0) scored = _rerank_by_total(scored, nid, pos, names, links, adj) # Prefer improving moves; still return best even if none improve. - improving = [c for c in scored if c["delta"]["global"] < 0] + if use_total: + base_total = int(global0) + base_clearance_hits + improving = [ + c + for c in scored + if int(c["crossings"]["global"]) + int(c.get("edge_clearance_hits", 0)) < base_total + ] + else: + improving = [c for c in scored if c["delta"]["global"] < 0] pool = improving if improving else scored top = _diversify_top(pool, k=max(1, int(top_k)), min_angle_sep=min_angle_sep) @@ -582,8 +596,11 @@ def orbit_sweep_round( protect_rigid: bool | str = "off", frozen_ids: set[str] | None = None, focus_ids: list[str] | None = None, + y_min: float | None = None, + y_max: float | None = None, + objective: str = "crossing", ) -> OpResult: - """Scan hot nodes; auto-apply each node's rank-1 if global crossings drop. + """Scan hot nodes; auto-apply each node's rank-1 if the active objective improves. Default ``protect_rigid=off`` (may move portals). Opt in with portals/all. """ @@ -653,6 +670,9 @@ def orbit_sweep_round( min_angle_sep=min_angle_sep, protect_rigid="off", # already applied frozen set frozen_ids=frozen, + y_min=y_min, + y_max=y_max, + objective=objective, ) if not sweep.get("ok"): trace.append({"node_id": nid, "skipped": sweep.get("error")}) @@ -662,12 +682,14 @@ def orbit_sweep_round( trace.append({"node_id": nid, "skipped": "no_candidates"}) continue best = cands[0] - if int(best["delta"]["global"]) >= 0: + # Skip when the active objective has no improving candidates. + if int(sweep.get("improving_n") or 0) <= 0: trace.append( { "node_id": nid, "skipped": "no_global_gain", "best_delta": best["delta"], + "objective": objective, } ) continue diff --git a/packages/netx-topology-mcp/src/netx_topology_mcp/layout_tool.py b/packages/netx-topology-mcp/src/netx_topology_mcp/layout_tool.py index 9f4a552..73148f0 100644 --- a/packages/netx-topology-mcp/src/netx_topology_mcp/layout_tool.py +++ b/packages/netx-topology-mcp/src/netx_topology_mcp/layout_tool.py @@ -475,6 +475,9 @@ def run_layout_on_graph( protect_rigid=protect, frozen_ids=frozen, focus_ids=knobs.get("focus_ids"), + y_min=knobs.get("y_min"), + y_max=knobs.get("y_max"), + objective=str(knobs.get("objective") or "crossing"), ) st = normalize_origin(op.state, base_params).state fin = score_state(st) diff --git a/packages/netx-topology-mcp/tests/test_orbit_sweep.py b/packages/netx-topology-mcp/tests/test_orbit_sweep.py index e87353a..943f5c0 100644 --- a/packages/netx-topology-mcp/tests/test_orbit_sweep.py +++ b/packages/netx-topology-mcp/tests/test_orbit_sweep.py @@ -174,3 +174,37 @@ def test_orbit_default_protect_off_ignores_portal_freeze() -> None: ) assert frozen["ok"] is False assert frozen.get("error") == "frozen" + + +def test_orbit_objective_total_ranks_clearance_trade() -> None: + """objective=total may keep a crossing-up move if clearance improves enough.""" + # Hub h near non-incident segment a—b; moving h right cuts clearance hits + # but can add a mild cross with c—d. Crossing-only ranking would reject it. + nodes = [ + {"fabric_node_id": "a", "name": "AAAAAA-EN-1", "x": 0.0, "y": 0.0}, + {"fabric_node_id": "b", "name": "BBBBBB-EN-2", "x": 400.0, "y": 0.0}, + {"fabric_node_id": "c", "name": "CCCCCC-EN-3", "x": 200.0, "y": -200.0}, + {"fabric_node_id": "d", "name": "DDDDDD-EN-4", "x": 200.0, "y": 200.0}, + {"fabric_node_id": "h", "name": "HHHHHH-EN-5", "x": 200.0, "y": 20.0}, + {"fabric_node_id": "t", "name": "TTTTTT-EN-6", "x": 200.0, "y": 300.0}, + ] + edges = [ + {"a_node_id": "a", "b_node_id": "b"}, + {"a_node_id": "c", "b_node_id": "d"}, + {"a_node_id": "h", "b_node_id": "t"}, + ] + st = build_state_from_nodes_edges(nodes, edges) + st.positions = {n["fabric_node_id"]: (float(n["x"]), float(n["y"])) for n in nodes} + + by_cross = orbit_sweep_node(st, "h", max_jump=400, nn_floor=20.0, objective="crossing") + by_total = orbit_sweep_node(st, "h", max_jump=400, nn_floor=20.0, objective="total") + assert by_cross["ok"] is True and by_total["ok"] is True + assert by_total.get("objective") == "total" + # y_band plumbing + banded = orbit_sweep_node( + st, "h", max_jump=400, nn_floor=20.0, objective="total", y_min=0.0, y_max=80.0 + ) + assert banded["ok"] is True + assert banded.get("y_band") == [0.0, 80.0] + for c in banded.get("candidates") or []: + assert 0.0 <= float(c["y"]) <= 80.0 diff --git a/tests/test_rbac_scopes.py b/tests/test_rbac_scopes.py index 6e38cd2..7257923 100644 --- a/tests/test_rbac_scopes.py +++ b/tests/test_rbac_scopes.py @@ -51,17 +51,28 @@ class ScopeUnitTests(unittest.TestCase): self.assertEqual(required_scope_for_request("GET", "/v1/webcrt/sessions"), SCOPE_WEBCRT) self.assertEqual(required_scope_for_request("POST", "/v1/managed-ne/exec"), "ne:exec") self.assertEqual(required_scope_for_request("POST", "/v1/managed-ne"), "ne:write") + self.assertEqual(required_scope_for_request("POST", "/v1/topology/fabric/paths"), "ne:read") + self.assertEqual(required_scope_for_request("POST", "/v1/topology/fabric/edges"), "ne:write") class SqlGuardTests(unittest.TestCase): def test_allows_cte(self) -> None: - # CTE (including WITH RECURSIVE) is now allowed; table whitelist still applies. + # Plain WITH CTE is allowed; table whitelist still applies. cleaned = validate_select_sql( "WITH x AS (SELECT * FROM ume_alarms_current) SELECT * FROM x", allowed_tables={"ume_alarms_current", "ume_inventory_ne"}, ) self.assertTrue(cleaned.lower().startswith("with")) + def test_rejects_recursive_cte(self) -> None: + with self.assertRaises(HTTPException) as ctx: + validate_select_sql( + "WITH RECURSIVE t AS (SELECT 1 AS n UNION ALL SELECT n+1 FROM t WHERE n < 100) " + "SELECT * FROM t", + allowed_tables={"ume_alarms_current", "ume_inventory_ne"}, + ) + self.assertEqual(ctx.exception.detail, "with_recursive_not_allowed") + def test_allows_cte_referencing_alias_in_from(self) -> None: # CTE name appears in FROM but should not be flagged as unauthorized table. cleaned = validate_select_sql( diff --git a/tests/test_topology.py b/tests/test_topology.py index 5daaf95..8557cc7 100644 --- a/tests/test_topology.py +++ b/tests/test_topology.py @@ -2495,6 +2495,62 @@ Management Addresses: graph2 = svc.get_view_graph(self.db, view_id) self.assertFalse(any(e.id == edge_id for e in graph2.edges)) + def test_find_fabric_paths_prefers_shortest(self) -> None: + """BFS should return the 1-hop path before a longer detour.""" + suffix = uuid4().hex[:8] + nes = [] + for label, ip_tail in (("A", 1), ("B", 2), ("C", 3)): + ne = ManagedNE( + id=f"path-{suffix}-{label}", + name=f"PATH-{label}-{suffix}", + vendor="Cisco", + device_type="cisco_ios", + ip_address=f"198.51.100.{ip_tail}", + ) + self.db.add(ne) + nes.append(ne) + self.db.commit() + nodes = [svc.ensure_fabric_node_for_managed(self.db, ne) for ne in nes] + self.db.commit() + # Short: A—B ; Long: A—C—B + svc.upsert_fabric_edge( + self.db, + a_node_id=nodes[0].id, + b_node_id=nodes[1].id, + a_port="Gi0/0", + b_port="Gi0/0", + source="manual", + ) + svc.upsert_fabric_edge( + self.db, + a_node_id=nodes[0].id, + b_node_id=nodes[2].id, + a_port="Gi0/1", + b_port="Gi0/0", + source="manual", + ) + svc.upsert_fabric_edge( + self.db, + a_node_id=nodes[2].id, + b_node_id=nodes[1].id, + a_port="Gi0/1", + b_port="Gi0/1", + source="manual", + ) + self.db.commit() + + out = svc.find_fabric_paths( + self.db, + from_managed_ne_id=nes[0].id, + to_managed_ne_id=nes[1].id, + max_paths=2, + max_hops=6, + ) + self.assertEqual(out["path_count"], 2) + self.assertEqual(out["paths"][0]["hops"], 1) + self.assertEqual(out["paths"][1]["hops"], 2) + self.assertEqual(len(out["paths"][0]["nodes"]), 2) + if __name__ == "__main__": unittest.main() diff --git a/web/src/pages/TopologyPage.tsx b/web/src/pages/TopologyPage.tsx index cbda530..1762c4c 100644 --- a/web/src/pages/TopologyPage.tsx +++ b/web/src/pages/TopologyPage.tsx @@ -3370,8 +3370,7 @@ export function TopologyPage() { y = Math.max(pad, window.innerHeight - rect.height - pad); } if (x !== ctxMenu.x || y !== ctxMenu.y) { - el.style.left = `${x}px`; - el.style.top = `${y}px`; + setCtxMenu((prev) => (prev ? { ...prev, x, y } : null)); } }, [ctxMenu]); diff --git a/web/src/pages/WorkbenchPage.tsx b/web/src/pages/WorkbenchPage.tsx index f171c31..cbdf380 100644 --- a/web/src/pages/WorkbenchPage.tsx +++ b/web/src/pages/WorkbenchPage.tsx @@ -175,7 +175,7 @@ export function WorkbenchPage() { return (
-
+