fix(api-map): detect router.use() middleware to fix auth_protected false-negative (closes #214) - #226
fix(api-map): detect router.use() middleware to fix auth_protected false-negative (closes #214)#226Wolfvin wants to merge 1 commit into
Conversation
…lse-negative (closes #214) _extract_js_middleware() hardcoded its regex receiver to app|server|fastify|hono, so any <routerVar>.use(middleware) call on a custom Router() instance (the standard Express modular-routing pattern) was silently dropped. Every route registered via that router was then reported as auth_protected:false even when the router explicitly mounted authMiddleware / requirePermission(...) etc. Fix: - _detect_router_vars() helper extracted and shared between _extract_js_routes (prefix + router_var tagging) and _extract_js_middleware (router-scoped .use()). Regex now also accepts express.Router() / koa.Router() (previously only bare Router()). - _extract_js_middleware emits a new 'router:<var>' scope for <routerVar>.use(mw) calls where <routerVar> is a known Router() instance. Unknown receivers are skipped (no false positives from arbitrary foo.use(bar)). - Post-processing attaches router-scoped middleware ONLY to routes whose router_var matches — no leak to routes on other routers or to top-level app.get/post routes. - Each JS route dict now carries a 'router_var' field (the receiver of .get/.post /etc.) so router-scoped middleware can be attached precisely. - AUTH_MIDDLEWARE_PATTERNS extended with requirepermission/haspermission/ checkpermission/verifypermission/ensurepermission (JS camelCase permission middleware) so requirePermission('admin') is classified as 'auth'. Backward compat: app.use(mw) / server.use(mw) / fastify.use(mw) / hono.use(mw) still emit scope='global' and attach to every route as before. Signatures of _extract_js_routes and _extract_js_middleware accept router_vars:Optional[...]=None (inline detection fallback for legacy direct callers).
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
|
PR ditutup — collision dengan PR #224 yang sudah merge duluan (issue #214 sudah closed). Kedua implementasi kualitasnya bagus (24 vs 32 test, keduanya well-documented). Tapi ditemukan gap correctness di implementasi ini yang tidak ada di #224: post-processing assembly ( Ini berpotensi bug nyata: kalau 2 file BERBEDA punya router variable dengan NAMA SAMA (contoh sangat umum: banyak route file konvensinya PR #224 eksplisit cek KEDUANYA: Terima kasih untuk investigasi mendalam dan tambahan pattern auth (haspermission/checkpermission/verifypermission/ensurepermission) — kalau mau, silakan buka PR baru khusus untuk expand AUTH_MIDDLEWARE_PATTERNS itu saja (di atas main yang sudah include #224), karena itu genuinely value tambahan yang tidak overlap. |
1 participant
Closes #214
Patch Peta (delta struktural)
Files:
M scripts/apimap_engine.py — added _detect_router_vars() helper (shared between routes & middleware extraction); extended Router() regex to accept express.Router() / koa.Router(); _extract_js_routes now records router_var on every JS route dict; _extract_js_middleware emits new scope="router:" for .use(mw); post-processing attaches router-scoped mw only to routes on the same router_var (no leak); AUTH_MIDDLEWARE_PATTERNS extended with requirepermission/haspermission/checkpermission/verifypermission/ensurepermission.
Endpoints: none
Schema: each JS route dict now carries a new
router_varfield (string, the receiver of .get/.post/.put/.delete/.patch/.head/.options). Middleware dicts now emit scope valuesrouter:<var>androuter-path:<var>:<path>in addition to the pre-existingglobalandpath:<path>.Konvensi: when adding new JS framework support, pass router_vars into _extract_js_routes / _extract_js_middleware rather than re-detecting inline. When adding new auth-middleware name patterns, append to AUTH_MIDDLEWARE_PATTERNS in lowercase (matching is substring + case-insensitive).
Klaim + Bukti
import:
python3 -c "import sys; sys.path.insert(0,'scripts'); import apimap_engine, commands.api_map"→ exit 0 (no errors)unit tests:
python3 -m pytest tests/test_apimap_router_middleware.py tests/test_command_count.py tests/test_command_registry.py tests/test_framework_detect.py tests/test_design_doc_check.py→ 113 passed in 2.44sdod #1 (auth_protected count >> 3): end-to-end on KDS-style fixture (4 auth-bearing routes + 2 public routes) →
stats.auth_protected: 4(was 0 before fix; baseline in issue body says 3 for the real KDS backend). Sample output:total_routes=6 auth_protected=4 public=2 GET /api/accounting/invoices auth=True mw=['authMiddleware','requireOutletAccess','requirePermission'] POST /api/accounting/invoices auth=True mw=['authMiddleware','requireOutletAccess','requirePermission'] DELETE /api/accounting/invoices/:id auth=True mw=['authMiddleware','requireOutletAccess','requirePermission'] PUT /api/accounting/invoices/:id auth=True mw=['authMiddleware','requireOutletAccess','requirePermission'] GET /health auth=False mw=[] GET /version auth=False mw=[]dod #2 (no over-detection): publicRouter routes
/healthand/version→auth_protected: False, mw_chain=[] — accountingRouter.use(authMiddleware) does NOT leak.dod #3 (no regression app.use global):
app.use(cors()); app.use(jwt); app.get('/users', ...); app.get('/health', ...)→ every route has mw_chain containingcorsandjwt, all routes auth_protected=True (jwt is auth). Same as pre-fix behavior.dod #4 (new unit test):
tests/test_apimap_router_middleware.py::TestMapApiRoutesIssue214::test_auth_protected_count_above_baselineand 31 other tests → 32 passed.classify:
_classify_middleware('requirePermission')→ 'auth' (was 'custom' before fix); same for hasPermission, checkPermission, verifyPermission, ensurePermission.Klaim yang TIDAK bisa dibuktikan di sini:
api-map <real-kds-workspace>count comparison before/after: tidak dijalankan karena KDS backend workspace tidak tersedia di sesi worker ini. Issue body melaporkanauth_protected: 3daritotal_routes: 160di KDS asli; verifikasi pada codebase nyata diserahkan ke BOS saat review. Reproduksi di fixture sintetik (4 routes auth, 2 public) sudah memvalidasi structural fix.Catatan Pendekatan
Issue body menyebutkan
_extract_js_middlewarebaris ~726 dan assembly middleware_chain per route baris ~340-370 sebagai starting point. Investigasi menemukan bahwa:_extract_js_middleware(regex receiver hardcoded keapp|server|fastify|hono), tetapi_extract_js_routes(untuk recordrouter_varfield pada setiap route) dan post-processing block dimap_api_routes(untuk attach router-scoped middleware hanya ke route denganrouter_varyang cocok).Router()assignments di-expand untuk acceptexpress.Router()/koa.Router()(pre-fix hanya match bareRouter()). Constraint di issue body eksplisit menyebutkan= Router()atau= express.Router()— keduanya sekarang dikenali.Tidak ada pendekatan alternatif yang dipilih — implementasi mengikuti instruksi issue.
Breaking / Found-not-fixed
scope="path:<path>"(dariapp.use('/path', mw)) maupunscope="router-path:<var>:<path>"(dari<routerVar>.use('/path', mw)) saat ini dikumpulkan kemiddleware_maptetapi TIDAK di-attach otomatis kemiddleware_chainper route di post-processing. Ini adalah pre-existing behavior —path:scope sudah ada sebelum PR ini dan tidak di-attach; saya menambahkanrouter-path:scope dengan behavior yang sama (konsisten). Kalau BOS mau path-scoped middleware ikut di-attach ke route yang match path-nya, itu follow-up issue terpisah. Tidak dibuat issue otomatis karena ini bukan regression dari PR ini — hanya dokumen konstatasi bahwa layer itu belum diimplementasikan.requireOutletAccess(disebut di issue body) TIDAK diklasifikasi sebagaiauthkarena substringaccessterlalu broad (bisa false-positive ke banyak nama variabel unrelated). Auth-protected count di fixture tetap naik ke 4 karenaauthMiddleware(substringauth) sudah cukup. Kalau BOS inginrequireOutletAccessdianggap auth, perlu pattern yang lebih spesifik.tests/test_cli.py::TestArgparseFormatConflictRegression::test_scan_with_format_long_does_not_crashdantest_scan_with_format_short_does_not_crashfail karena subprocess timeout 60s (codelens scan .lambat tanpa pre-built registry di env ini).tests/test_compact_format.py::TestGraphSchemaCommand::test_returns_correct_countsfail karenatree-sitter-typescriptgrammar tidak terinstall di env worker (CALLSedge count beda: 97 vs expected 76). Keduanya ter-verifikasi gagal juga pada clean main checkout — bukan regression dari PR ini.