diff --git a/scripts/apimap_engine.py b/scripts/apimap_engine.py index b2460dd7..7a702dfc 100755 --- a/scripts/apimap_engine.py +++ b/scripts/apimap_engine.py @@ -60,6 +60,10 @@ "authenticate", "auth", "jwt", "passport", "requireAuth", "isAuthenticated", "verifyToken", "checkAuth", "ensureAuthenticated", "login_required", "permission_required", "auth_required", + # v6.5 (issue #214): JS camelCase permission middleware — must classify as auth + # so that router.use(requirePermission('admin')) bumps auth_protected count. + "requirepermission", "haspermission", "checkpermission", + "verifypermission", "ensurepermission", } CORS_MIDDLEWARE_PATTERNS = { @@ -85,6 +89,40 @@ def _is_test_file(file_path: str) -> bool: return any(p in lower for p in test_patterns) +# v6.5 (issue #214): shared Router() instance detection — used by both +# _extract_js_routes (for prefix application) and _extract_js_middleware +# (for router-scoped middleware attachment). Previously inline in +# _extract_js_routes only, which left _extract_js_middleware unable to +# recognise .use(mw) patterns. +_ROUTER_ASSIGNMENT_RE = re.compile( + r'(?:const|let|var)\s+(\w+)\s*=\s*(?:new\s+)?' + r'(?:(\w+)\s*\.\s*)?' # optional receiver: express, koa, etc. + r'(?:Router|router)\s*\(([^)]*)\)' +) + + +def _detect_router_vars(content: str) -> Dict[str, str]: + """Detect ``Router()`` assignments and return ``{var_name: prefix}``. + + Matches the common Express / Koa patterns: + const router = Router({ prefix: '/api' }) + let accountingRouter = express.Router() + var adminRouter = new Router() + + The optional receiver (e.g. ``express`` in ``express.Router()``) is + accepted but not stored — only the variable name matters for scoping + middleware to that router instance. + """ + router_vars: Dict[str, str] = {} + for m in _ROUTER_ASSIGNMENT_RE.finditer(content): + var_name = m.group(1) + args = m.group(3) or "" + prefix_match = re.search(r'prefix\s*:\s*[\'"]([^\'"]+)[\'"]', args) + prefix = prefix_match.group(1) if prefix_match else "" + router_vars[var_name] = prefix + return router_vars + + def map_api_routes( workspace: str, method: Optional[str] = None, @@ -153,11 +191,17 @@ def map_api_routes( # ─── Express / Koa / Hono / Fastify ────────────── if ext in {".js", ".mjs", ".cjs", ".ts", ".tsx", ".jsx"}: - js_routes = _extract_js_routes(content, rel_path, frameworks_detected) + # v6.5 (issue #214): detect Router() instances once, share with + # both _extract_js_routes (for prefix + router_var tagging) and + # _extract_js_middleware (for router-scoped .use() attachment). + router_vars = _detect_router_vars(content) + js_routes = _extract_js_routes( + content, rel_path, frameworks_detected, router_vars + ) routes.extend(js_routes) - # Detect global middleware - mw = _extract_js_middleware(content, rel_path) + # Detect global + router-scoped middleware + mw = _extract_js_middleware(content, rel_path, router_vars) global_middleware.extend(mw) # ─── Next.js API Routes ─────────────────────────── @@ -350,6 +394,20 @@ def map_api_routes( "file": mw["file"], "line": mw["line"], }) + elif scope.startswith("router:"): + # v6.5 (issue #214): router-instance-scoped middleware — attach + # ONLY to routes registered via the same router variable. + # accountRouter.use(authMiddleware) must NOT leak to routes on + # userRouter or to top-level app.get/post routes. + router_var = scope.split(":", 1)[1] + for route in routes: + if route.get("router_var") == router_var: + route.setdefault("middleware_chain", []).append({ + "name": mw["name"], + "type": mw.get("type", "unknown"), + "file": mw["file"], + "line": mw["line"], + }) # Build middleware map for route in routes: @@ -434,9 +492,23 @@ def map_api_routes( # ─── JS Route Extraction ─────────────────────────────────────── def _extract_js_routes( - content: str, rel_path: str, frameworks: Set[str] + content: str, + rel_path: str, + frameworks: Set[str], + router_vars: Optional[Dict[str, str]] = None, ) -> List[Dict[str, Any]]: - """Extract routes from Express / Fastify / Koa / Hono JS/TS files.""" + """Extract routes from Express / Fastify / Koa / Hono JS/TS files. + + ``router_vars`` maps Router() instance variable names to their optional + prefix (e.g. ``{"accountingRouter": "/api/accounting"}``). It is used to + (a) apply path prefixes to routes registered on that router, and + (b) tag each route with the ``router_var`` it was registered via — which + lets router-scoped middleware (``accountingRouter.use(authMiddleware)``) + be attached to exactly the right routes in post-processing. + + When ``router_vars`` is ``None`` (e.g. legacy direct callers/tests), it + is detected inline via :func:`_detect_router_vars`. + """ routes = [] lines = content.split('\n') @@ -455,26 +527,19 @@ def _extract_js_routes( if is_hono: frameworks.add("hono") - # Track current router variable names and prefixes - router_vars: Dict[str, str] = {} # var_name → prefix - - # Detect Router() assignments: const router = Router({ prefix: '/api' }) - for m in re.finditer( - r'(?:const|let|var)\s+(\w+)\s*=\s*(?:new\s+)?(?:Router|router)\s*\(([^)]*)\)', - content - ): - var_name = m.group(1) - args = m.group(2) - prefix_match = re.search(r'prefix\s*:\s*[\'"]([^\'"]+)[\'"]', args) - prefix = prefix_match.group(1) if prefix_match else "" - router_vars[var_name] = prefix + # v6.5 (issue #214): router_vars is normally passed in by the caller + # (map_api_routes), which shares it with _extract_js_middleware. Fall + # back to inline detection for backward compat with any direct callers. + if router_vars is None: + router_vars = _detect_router_vars(content) # Detect app.route('/path') chains for m in re.finditer( - r'(?:app|router|server|fastify|hono)\s*\.\s*route\s*\(\s*[\'"]([^\'"]+)[\'"]\s*\)', + r'(app|router|server|fastify|hono)\s*\.\s*route\s*\(\s*[\'"]([^\'"]+)[\'"]\s*\)', content ): - base_path = m.group(1) + receiver = m.group(1) + base_path = m.group(2) line_num = content[:m.start()].count('\n') + 1 # Look for chained methods after this chain_start = m.end() @@ -491,6 +556,9 @@ def _extract_js_routes( "request_type": None, "response_type": None, "framework": _detect_js_framework(is_express, is_fastify, is_koa, is_hono), + # v6.5 (issue #214): record receiver so router-scoped + # middleware on `app`/`server`/etc can be attached. + "router_var": receiver, }) # Direct method calls: app.get('/path', ...), router.post('/path', ...) @@ -537,6 +605,10 @@ def _extract_js_routes( "request_type": req_type, "response_type": resp_type, "framework": _detect_js_framework(is_express, is_fastify, is_koa, is_hono), + # v6.5 (issue #214): record which variable received the .get/.post + # call so router-scoped `.use()` middleware can be attached to + # exactly these routes (not all routes, not routes on other routers). + "router_var": obj_name, }) return routes @@ -715,44 +787,57 @@ def _detect_request_response_types( # ─── JS Middleware Extraction ────────────────────────────────── -def _extract_js_middleware(content: str, rel_path: str) -> List[Dict]: - """Extract global/app-level middleware from JS files.""" +def _extract_js_middleware( + content: str, + rel_path: str, + router_vars: Optional[Dict[str, str]] = None, +) -> List[Dict]: + """Extract global/app-level and router-scoped middleware from JS files. + + Three scope types are emitted: + + * ``"global"`` — middleware attached via ``app.use(mw)`` / + ``server.use(mw)`` / ``fastify.use(mw)`` / ``hono.use(mw)``. Attached + to every route in post-processing. + * ``"router:"`` — middleware attached via ``.use(mw)`` + where ```` is a variable previously bound to a ``Router()`` + call (e.g. ``accountingRouter.use(authMiddleware)``). Attached only + to routes registered via the same router variable (issue #214). + * ``"path:"`` — middleware attached via ``app.use('/path', mw)``. + Currently collected but not auto-attached to per-route chains + (pre-existing gap; out of scope for #214). + + ``router_vars`` is normally supplied by :func:`map_api_routes` so that + route extraction and middleware extraction share the same Router() + detection. When ``None`` (legacy direct callers), it is detected + inline via :func:`_detect_router_vars`. + """ middleware = [] lines = content.split('\n') + if router_vars is None: + router_vars = _detect_router_vars(content) + + # Receivers that mean "the global application instance" — anything that + # matches these in a `.use(mw)` call is treated as global middleware. + _GLOBAL_RECEIVERS = {"app", "server", "fastify", "hono"} + for i, line in enumerate(lines): stripped = line.strip() - # app.use(middleware) patterns + # .use(middleware) — no path argument + # Captures any identifier as receiver; scope is decided below based + # on whether the receiver is the global app, a known Router() var, + # or something we should skip (NON_ROUTER_OBJECTS, unknown vars). m = re.match( - r'(?:app|server|fastify|hono)\s*\.\s*use\s*\(\s*(\w+)', + r'(\w+)\s*\.\s*use\s*\(\s*(\w+)', stripped ) if m: - mw_name = m.group(1) + receiver = m.group(1) + mw_name = m.group(2) mw_type = _classify_middleware(mw_name) - middleware.append({ - "name": mw_name, - "type": mw_type, - "scope": "global", - "file": rel_path, - "line": i + 1, - }) - - # app.use('/path', middleware) — route-scoped middleware - m = re.match( - r'(?:app|server|fastify|hono)\s*\.\s*use\s*\(\s*[\'"`]([^\'"`]+)[\'"`]\s*,\s*(\w+)', - stripped - ) - if m: - mw_path = m.group(1) - # Only treat as route-scoped middleware if the path looks like a real route - # (starts with /) — filter out cookie names, variable names, etc. - if not mw_path.startswith('/'): - # Might be a config string (e.g., cookie secret), not a route path - # Treat as global middleware instead - mw_name = m.group(2) - mw_type = _classify_middleware(mw_name) + if receiver in _GLOBAL_RECEIVERS: middleware.append({ "name": mw_name, "type": mw_type, @@ -760,16 +845,71 @@ def _extract_js_middleware(content: str, rel_path: str) -> List[Dict]: "file": rel_path, "line": i + 1, }) - else: - mw_name = m.group(2) - mw_type = _classify_middleware(mw_name) + elif receiver in NON_ROUTER_OBJECTS: + # e.g. obj.use(...) where obj is a known non-router — skip + pass + elif receiver in router_vars: + # v6.5 (issue #214): router-instance-scoped middleware. + # e.g. accountingRouter.use(authMiddleware) — attach only + # to routes registered via accountingRouter.get/post/... middleware.append({ "name": mw_name, "type": mw_type, - "scope": f"path:{mw_path}", + "scope": f"router:{receiver}", "file": rel_path, "line": i + 1, }) + # else: unknown receiver — be conservative, skip (avoids false + # positives from arbitrary foo.use(bar) calls in user code). + + # .use('/path', middleware) — path-scoped middleware + m = re.match( + r'(\w+)\s*\.\s*use\s*\(\s*[\'"`]([^\'"`]+)[\'"`]\s*,\s*(\w+)', + stripped + ) + if m: + receiver = m.group(1) + mw_path = m.group(2) + mw_name = m.group(3) + mw_type = _classify_middleware(mw_name) + # Only treat as route-scoped middleware if the path looks like a real route + # (starts with /) — filter out cookie names, variable names, etc. + if not mw_path.startswith('/'): + # Might be a config string (e.g., cookie secret), not a route path + # Treat as global middleware instead — but only if receiver is one + # of the canonical global app names. Router vars with a config + # string arg are skipped (rare, not in scope for #214). + if receiver in _GLOBAL_RECEIVERS: + middleware.append({ + "name": mw_name, + "type": mw_type, + "scope": "global", + "file": rel_path, + "line": i + 1, + }) + else: + if receiver in _GLOBAL_RECEIVERS: + middleware.append({ + "name": mw_name, + "type": mw_type, + "scope": f"path:{mw_path}", + "file": rel_path, + "line": i + 1, + }) + elif receiver in router_vars: + # v6.5 (issue #214): router-scoped path middleware — + # e.g. accountingRouter.use('/reports', auditLogger). + # Stored as router-path:: for future per-route + # attachment; currently not auto-attached (same gap as + # path: scope above), but tracked in middleware_map. + middleware.append({ + "name": mw_name, + "type": mw_type, + "scope": f"router-path:{receiver}:{mw_path}", + "file": rel_path, + "line": i + 1, + }) + # else: unknown receiver — skip (conservative). return middleware diff --git a/tests/test_apimap_router_middleware.py b/tests/test_apimap_router_middleware.py new file mode 100644 index 00000000..17ce6096 --- /dev/null +++ b/tests/test_apimap_router_middleware.py @@ -0,0 +1,308 @@ +"""Tests for router-instance-scoped middleware detection in apimap_engine. + +Reproduces issue #214: ``.use(middleware)`` (the standard Express +modular-routing pattern) was silently dropped because ``_extract_js_middleware`` +hardcoded the receiver to ``app|server|fastify|hono``. As a result every +route registered via a custom ``Router()`` instance was reported as +``auth_protected: false`` even when the router explicitly mounted an auth +middleware via ``router.use(authMiddleware)``. + +These tests lock in the fix: + +* ``.use(mw)`` is detected and scoped to that router instance only +* Router-scoped middleware does NOT leak to routes on other routers or to + top-level ``app.get/post`` routes +* Public routes (no auth middleware anywhere in their chain) stay public +* Global ``app.use(mw)`` middleware is still attached to every route +* ``requirePermission`` / ``hasPermission`` / ``checkPermission`` are + classified as ``auth`` (so they bump ``auth_protected`` count) +* ``Router()`` and ``express.Router()`` are both recognised +""" + +import os +import sys +import tempfile +import shutil + +import pytest + +SCRIPT_DIR = os.path.join( + os.path.dirname(os.path.dirname(os.path.abspath(__file__))), "scripts" +) +sys.path.insert(0, SCRIPT_DIR) + +from apimap_engine import ( # noqa: E402 + _classify_middleware, + _detect_router_vars, + _extract_js_middleware, + _extract_js_routes, + map_api_routes, +) + + +# --------------------------------------------------------------------------- +# _detect_router_vars +# --------------------------------------------------------------------------- + +class TestDetectRouterVars: + def test_bare_router_assignment(self): + content = "const router = Router();" + assert _detect_router_vars(content) == {"router": ""} + + def test_express_router_assignment(self): + # The issue body explicitly calls out `= express.Router()` as a pattern + # that must be recognised (the pre-fix inline regex only matched `Router()`). + content = "const accountingRouter = express.Router();" + assert _detect_router_vars(content) == {"accountingRouter": ""} + + def test_router_with_prefix(self): + content = "const r = Router({ prefix: '/api/v2' });" + assert _detect_router_vars(content) == {"r": "/api/v2"} + + def test_new_router(self): + content = "var adminRouter = new Router();" + assert _detect_router_vars(content) == {"adminRouter": ""} + + def test_multiple_routers(self): + content = """ + const accountingRouter = express.Router({ prefix: '/api/accounting' }); + const publicRouter = Router(); + """ + assert _detect_router_vars(content) == { + "accountingRouter": "/api/accounting", + "publicRouter": "", + } + + def test_no_router_assignment(self): + content = "const app = express();\napp.get('/x', h);" + assert _detect_router_vars(content) == {} + + +# --------------------------------------------------------------------------- +# _extract_js_middleware — router-scoped detection +# --------------------------------------------------------------------------- + +class TestExtractJsMiddlewareRouterScoped: + def test_router_use_middleware_no_path(self): + content = "accountingRouter.use(authMiddleware);" + mw = _extract_js_middleware(content, "f.ts", {"accountingRouter": ""}) + assert len(mw) == 1 + assert mw[0]["name"] == "authMiddleware" + assert mw[0]["type"] == "auth" + assert mw[0]["scope"] == "router:accountingRouter" + + def test_router_use_require_permission_with_args(self): + # The exact pattern from issue #214 / KDS backend. + # `requirePermission('admin')` is a call, not a bare identifier — the + # `(\w+)` capture must grab `requirePermission` and the `('admin')` + # part is stripped by the existing classification path. + content = "accountingRouter.use(requirePermission('admin'));" + mw = _extract_js_middleware(content, "f.ts", {"accountingRouter": ""}) + assert len(mw) == 1 + assert mw[0]["name"] == "requirePermission" + assert mw[0]["type"] == "auth" + assert mw[0]["scope"] == "router:accountingRouter" + + def test_router_use_skips_unknown_receiver(self): + # `foo` is not a known Router() var and not a global app name — + # we must NOT treat this as middleware (avoids false positives from + # arbitrary `foo.use(bar)` calls). + content = "foo.use(someHandler);" + mw = _extract_js_middleware(content, "f.ts", {}) + assert mw == [] + + def test_router_use_skips_non_router_objects(self): + # `cache.use(...)` should not be picked up as middleware. + content = "cache.use(new Map());" + mw = _extract_js_middleware(content, "f.ts", {}) + assert mw == [] + + def test_global_app_use_still_global_scope(self): + content = "app.use(cors());" + mw = _extract_js_middleware(content, "f.ts", {}) + assert len(mw) == 1 + assert mw[0]["scope"] == "global" + + def test_router_use_path_scoped(self): + content = 'accountingRouter.use("/reports", auditLogger);' + mw = _extract_js_middleware(content, "f.ts", {"accountingRouter": ""}) + assert len(mw) == 1 + assert mw[0]["name"] == "auditLogger" + assert mw[0]["scope"] == "router-path:accountingRouter:/reports" + + def test_router_use_does_not_leak_to_global(self): + # Two routers in the same file — middleware on one must not appear + # with scope referencing the other. + content = """ + const accountingRouter = Router(); + const publicRouter = Router(); + accountingRouter.use(authMiddleware); + publicRouter.use(cors); + """ + rv = _detect_router_vars(content) + mw = _extract_js_middleware(content, "f.ts", rv) + scopes = {m["scope"] for m in mw} + assert "router:accountingRouter" in scopes + assert "router:publicRouter" in scopes + # No global scope (neither receiver is app/server/fastify/hono) + assert "global" not in scopes + + +# --------------------------------------------------------------------------- +# _extract_js_routes — router_var tagging +# --------------------------------------------------------------------------- + +class TestExtractJsRoutesRouterVarTagging: + def test_route_records_router_var(self): + content = """ + const accountingRouter = Router({ prefix: '/api/accounting' }); + accountingRouter.get('/invoices', getInvoices); + """ + rv = _detect_router_vars(content) + routes = _extract_js_routes(content, "f.ts", set(), rv) + assert len(routes) == 1 + assert routes[0]["router_var"] == "accountingRouter" + # prefix must still be applied + assert routes[0]["path"] == "/api/accounting/invoices" + + def test_app_route_records_app_var(self): + content = "app.get('/users', getUsers);" + routes = _extract_js_routes(content, "f.ts", set(), {}) + assert len(routes) == 1 + assert routes[0]["router_var"] == "app" + + +# --------------------------------------------------------------------------- +# map_api_routes — end-to-end DoD scenarios from issue #214 +# --------------------------------------------------------------------------- + +_KDS_STYLE_SOURCE = """ +import { Router } from 'express'; +import { authMiddleware, requirePermission } from '../middleware/auth'; +import { requireOutletAccess } from '../middleware/access'; + +const accountingRouter = Router({ prefix: '/api/accounting' }); + +accountingRouter.use(authMiddleware); +accountingRouter.use(requireOutletAccess); +accountingRouter.use(requirePermission('admin')); + +accountingRouter.get('/invoices', getInvoices); +accountingRouter.post('/invoices', createInvoice); +accountingRouter.delete('/invoices/:id', deleteInvoice); +accountingRouter.put('/invoices/:id', updateInvoice); + +// Public router — no auth +const publicRouter = Router(); +publicRouter.get('/health', healthCheck); +publicRouter.get('/version', versionInfo); +""" + + +@pytest.fixture +def kds_workspace(): + ws = tempfile.mkdtemp() + routes_dir = os.path.join(ws, "src", "routes") + os.makedirs(routes_dir) + with open(os.path.join(routes_dir, "accounting.ts"), "w") as f: + f.write(_KDS_STYLE_SOURCE) + yield ws + shutil.rmtree(ws, ignore_errors=True) + + +class TestMapApiRoutesIssue214: + def test_auth_protected_count_above_baseline(self, kds_workspace): + # DoD #1: auth_protected count is significantly higher than 3 (the + # pre-fix false-negative baseline reported in the issue body). + result = map_api_routes(kds_workspace) + assert result["status"] == "ok" + # All 4 accounting routes are auth-protected via router.use(authMiddleware) + assert result["stats"]["auth_protected"] == 4 + assert result["stats"]["total_routes"] == 6 + + def test_public_routes_stay_public(self, kds_workspace): + # DoD #2: routes not under any auth-bearing router stay auth_protected=False. + result = map_api_routes(kds_workspace) + public_routes = [ + r for r in result["routes"] if not r.get("auth_protected") + ] + assert len(public_routes) == 2 + public_paths = sorted(r["path"] for r in public_routes) + assert public_paths == ["/health", "/version"] + + def test_router_middleware_does_not_leak_to_public_routes(self, kds_workspace): + # Constraint: accountingRouter.use(authMiddleware) must NOT attach to + # routes on publicRouter. + result = map_api_routes(kds_workspace) + for r in result["routes"]: + mw_names = {m["name"] for m in r.get("middleware_chain", [])} + if r["path"] in ("/health", "/version"): + # publicRouter routes must not carry any accounting-scoped mw + assert "authMiddleware" not in mw_names + assert "requirePermission" not in mw_names + assert "requireOutletAccess" not in mw_names + else: + # accounting routes must have all three + assert "authMiddleware" in mw_names + assert "requirePermission" in mw_names + assert "requireOutletAccess" in mw_names + + def test_router_var_recorded_on_routes(self, kds_workspace): + # Helper for downstream tooling: each JS route carries the receiver + # variable name so consumers can group routes by router instance. + result = map_api_routes(kds_workspace) + for r in result["routes"]: + assert "router_var" in r + assert r["router_var"] in {"accountingRouter", "publicRouter"} + + def test_app_use_global_middleware_no_regression(self): + # DoD #3: app.use()/server.use() global middleware still attached to + # every route like before. + ws = tempfile.mkdtemp() + try: + with open(os.path.join(ws, "app.ts"), "w") as f: + f.write( + "const express = require('express');\n" + "const app = express();\n" + "app.use(cors());\n" + "app.use(jwt);\n" + "app.get('/users', getUsers);\n" + "app.post('/users', createUser);\n" + "app.get('/health', healthCheck);\n" + ) + result = map_api_routes(ws) + for r in result["routes"]: + mw_names = {m["name"] for m in r.get("middleware_chain", [])} + assert "cors" in mw_names + assert "jwt" in mw_names + # jwt is auth-classified → all routes auth_protected + assert r.get("auth_protected") is True + finally: + shutil.rmtree(ws, ignore_errors=True) + + +# --------------------------------------------------------------------------- +# _classify_middleware — auth pattern expansion (issue #214 DoD #1) +# --------------------------------------------------------------------------- + +class TestAuthMiddlewarePatternExpansion: + @pytest.mark.parametrize( + "name,expected", + [ + ("requirePermission", "auth"), + ("hasPermission", "auth"), + ("checkPermission", "auth"), + ("verifyPermission", "auth"), + ("ensurePermission", "auth"), + ("authMiddleware", "auth"), + ("requireAuth", "auth"), + # Non-auth examples must NOT be misclassified + ("cors", "cors"), + ("rateLimit", "rate_limit"), + ("validate", "validation"), + ("auditLogger", "custom"), + ("requireOutletAccess", "custom"), # not auth; "access" too broad + ], + ) + def test_classification(self, name, expected): + assert _classify_middleware(name) == expected