fix(api-map): detect router.use() middleware, fixes false-negative auth_protected (closes #214) - #224
Merged
Conversation
…th_protected (closes #214) _extract_js_middleware regex was hardcoded to app|server|fastify|hono receivers, silently missing the standard Express modular-router pattern (accountingRouter.use(authMiddleware)). All routes registered on a Router() instance with .use() middleware were reported as auth_protected: false. Changes: - Add _detect_router_vars() helper, shared between _extract_js_routes and _extract_js_middleware. Extends detection from bare Router() to also cover express.Router() / obj.Router() forms (was missed before). - _extract_js_routes: add router_var field to each route dict (set to the receiver obj_name when it's a detected Router() var, else None). - _extract_js_middleware: new router:<var> scope for <routerVar>.use(mw). Bare-name capture (\w+) also covers call-style middleware like requirePermission('admin') — captured identifier is enough for _classify_middleware. - Assembly layer (map_api_routes): attach router-scoped mw only to routes whose file AND router_var both match — prevents cross-file and cross-router leakage. - AUTH_MIDDLEWARE_PATTERNS: add 'requirepermission' (camelCase form of permission_required) so router.use(requirePermission('admin')) classifies as auth. Tests: tests/test_apimap_router_middleware.py — 24 tests covering detection helper, middleware extraction, classifier, route field, and end-to-end map_api_routes (KDS pattern repro, cross-file no-leak, cross-router no-leak, global app.use regression guard, express.Router() form, prefix support, inline coexistence).
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Closed
4 tasks
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Closes #214
Patch Peta (delta struktural)
Files:
Endpoints: none
Schema: none
Konvensi:
Router()MAUPUNexpress.Router()form (yang sebelumnya missed). Helper ini dipakai bersama oleh _extract_js_routes (prefix) dan _extract_js_middleware (router-scope .use()).router_var(var name receiver kalau Router() instance, None kalau app/server/fastify/hono). Field ini yang dipakai assembly untuk match router-scope middleware ke route yang benar.router:<var>— attached HANYA ke route dengan file AND router_var yang cocok. Tidak ada global path-scope middleware baru yang di-attach (existingpath:<path>scope tetap collected tapi not assembled, sesuai behavior sebelumnya).requirepermission(camelCase form, paralel dengan snake_casepermission_required). Diperlukan karenarouter.use(requirePermission('admin'))pattern eksplisit disebut di issue.Klaim + Bukti
auth_protectedcount SEBELUM fix (KDS-pattern fixture, 4 routes: 3 pada accountingRouter.use(authMiddleware), 1 pada publicRouter tanpa middleware):auth_protected: 0 — false negative parah, semua 3 route yang seharusnya protected dilaporkan public.
auth_protectedcount SESUDAH fix (fixture sama):auth_protected: 3 — benar (3 route di accountingRouter protected via authMiddleware), 1 public (/health di publicRouter tetap public, no over-detection).
Sample route yang sekarang benar terdeteksi auth_protected (manual grep verifikasi):
accountingRouter.use(authMiddleware)applies; authMiddleware classified asauthvia "auth" substring match.Konfirmasi tidak ada over-detection:
publicRouter(Router instance tanpa.use(authMiddleware)): auth_protected=False. ✓assignments.tspunyaassignmentsRoutertanpa auth middleware; route/tasksdi assignments.ts auth_protected=False bahkan kalauaccounting.tsdi workspace yang sama punyaaccountingRouter.use(authMiddleware). ✓ (Test: test_no_cross_leak_between_files)Tests:
24 tests cover: _detect_router_vars (6 forms: bare/lowercase/new/express.Router()/new express.Router()/prefix/multiple), _extract_js_middleware (router-scope emit, global preservation, call-style middleware, no false router-scope on non-Router receivers), _classify_middleware (requirePermission=auth, authMiddleware=auth, requireOutletAccess=custom — outlet-scope bukan generic auth), _extract_js_routes router_var field (Router var recorded, app=None), end-to-end map_api_routes (KDS pattern, cross-router no-leak same file, cross-file no-leak, global app.use regression guard, express.Router() qualified form, prefix support, inline coexistence).
Regression sweep (relevant test files):
0 regression pada area yang relevan.
Command count unchanged:
Identity: Executor·Pragmatist
Catatan Pendekatan
none — implementasi mengikuti instruksi issue. Pendekatan: track
router_varper route saat extraction, emitrouter:<var>scope saat middleware extraction, match keduanya saat assembly. Pendekatan alternatif yang dipertimbangkan: store router_vars sebagai field di middleware_map global dan lakukan matching di assembly — tapi pendekatan yang dipilih lebih clean karena route sudah punya context receiver-nya saat extraction.Breaking / Found-not-fixed
Found, not fixed (Tier 2 — pre-existing, di luar scope #214):
scripts/apimap_engine.py:_extract_inline_middleware(~line 587) — bug pre-existing:paren_start = content.find('(', start_pos - 1)mencari(SETELAH path string, padahal(pembuka route call ADA SEBELUM path string. Untukr.get("/x", validateInput, handler), tidak ada(setelah"/x", jadi function return empty list — inline middleware antara path dan handler TIDAK pernah terekstrak. Test fixture lama (tests/fixtures/server.js) tidak mengandung inline middleware (semua route hanya path+handler), jadi bug ini silent. Impact: route denganapp.get("/x", validateInput, handler)kehilanganvalidateInputdari middleware_chain. Tidak mempengaruhi auth_protected calc (auth biasanya via app.use global atau router.use, bukan inline). Bukan scope fix(api-map): router.use() middleware not detected, causes false-negative auth_protected #214.tests/test_cli.py— ada 1 test yang fail/hang saat run full file (passes in isolation). Pre-existing — terkonfirmasi via git stash: failure terjadi pada main tanpa perubahan ini. Test isolation issue, bukan related ke apimap.tests/test_doctor.py— 11 failures (KeyError: 'fixes', pip-related). Pre-existing — terkonfirmasi via git stash pada main. Environment-related (deps missing).Per issue Constraint #4 ("investigasi mandiri — jangan asumsikan scope perbaikan hanya di situ"), saya juga periksa assembly logic (~line 343-370) — found that existing
path:<path>scope dariapp.use('/path', mw)di-collect tapi TIDAK pernah di-attach ke route manapun. Pre-existing bug, bukan scope #214 (issue spesifik tentang router.use, bukan path-scoped global). Tidak di-fix untuk hindari scope creep.