diff --git a/LIMITATIONS.md b/LIMITATIONS.md index ac9c0cb..fd6bf87 100644 --- a/LIMITATIONS.md +++ b/LIMITATIONS.md @@ -75,12 +75,17 @@ do. Read this before trusting any single output. - Four built-in claims (route-test-coverage, admin-authorization, billing-webhook-signature, jwt-authentication). User-defined claims are not supported yet, deliberately. -- Route test coverage is attributed statically (test imports and route names), - not by executing tests. A route exercised only indirectly can be reported as - uncovered. -- Admin authorization reports WEAK when it finds no authorization evidence. - That means DevTime found nothing, never that a route is confirmed - unprotected: global middleware and framework decorators are not detected. +- Route test association is established from test imports only, and a static + import association is not execution coverage. Tests that exercise a route + through a running server (supertest, TestClient) or through an unresolved + helper are reported as unassociated, so this claim currently abstains on many + real repositories. +- Admin authorization is established only from a guard at the route's own call + site. Guards applied by a router mount, a server-wide middleware, or a + framework decorator are reported as unresolved. WEAK means DevTime found no + connected evidence, never that a route is confirmed unprotected. +- Billing webhook signature verification is connected per handler by file. + Verification reached through an imported helper is not resolved yet. - Verification is rule-driven over scanner signals; it inherits every scanner coverage limitation listed here. - Statuses mean "per DevTime's evidence rules", not formal proof or a security diff --git a/QUICKSTART.md b/QUICKSTART.md index 177b283..e85cdcb 100644 --- a/QUICKSTART.md +++ b/QUICKSTART.md @@ -27,7 +27,7 @@ python -m venv .venv source .venv/bin/activate # Windows (PowerShell): .venv\Scripts\Activate.ps1 # Windows (Git Bash): source .venv/Scripts/activate pip install -e ".[dev]" -pytest # optional: all tests pass (142 at v0.5.0) +pytest # optional: all tests pass (157 at v0.5.1) ``` ## 3. Create the demo repo @@ -132,7 +132,7 @@ A fresh-clone check was run on the current candidate: - **OS:** Windows 11 (Git Bash) - **Python:** 3.11.9 - **Install:** `pip install -e ".[dev]"` -- **Tests:** all passing (142 at v0.5.0) +- **Tests:** all passing (157 at v0.5.1) - **Demo:** `dtc init` / `dtc scan` / `dtc concepts` / `dtc explain "Billing Webhooks"` all produced the expected output from a clean `git clone`. diff --git a/README.md b/README.md index 2390a87..caff47d 100644 --- a/README.md +++ b/README.md @@ -29,9 +29,10 @@ dtc scan dtc verify ``` -On the demo repo that ends with signature verification SUPPORTED, JWT -authentication SUPPORTED, and `2 of 3 routes have a referencing test`. Point it at -your own repository and the answers change: +On the demo repo that ends with billing webhook signature verification +SUPPORTED, JWT authentication SUPPORTED, and route test association WEAK, naming +the routes with no test importing them. Point it at your own repository and the +answers change: ```bash cd your-repo @@ -106,7 +107,7 @@ Contradictions: - **UNKNOWN** - the surface exists but coverage cannot responsibly decide - **NOT_APPLICABLE** - the repository has no surface this claim is about -Four built-in claims ship: route test coverage, admin authorization, billing +Four built-in claims ship: route test association, admin authorization, billing webhook signatures, and JWT authentication. `dtc verify` leads with what it can actually verify in your repository, and when nothing applies it says what would make a claim verifiable instead of dead-ending. diff --git a/RELEASE_NOTES_v0.5.1.md b/RELEASE_NOTES_v0.5.1.md new file mode 100644 index 0000000..c98708b --- /dev/null +++ b/RELEASE_NOTES_v0.5.1.md @@ -0,0 +1,93 @@ +# DevTime v0.5.1 - three false SUPPORTED results, fixed + +An external review of v0.5.0 reproduced three cases where `dtc verify` reported +SUPPORTED with no justifying evidence. All three reproduced against the released +code. This release fixes them. + +A reproducible false reassurance is still a product failure, and these were the +exact failure DevTime exists to catch: a file's *name* being treated as evidence +about the file's *behavior*. + +## What was wrong + +**Administrative routes reported as protected when nothing checked them.** +`src/admin/permissions.ts` containing `router.get("/admin/permissions", handler)` +reported "1 of 1 administrative route(s) show an authorization check". The +evaluator matched a combined text that included the file path, so the word +"permissions" in the filename satisfied its "permission" token. No guard existed +anywhere in the repository. This was security-adjacent and is the reason this +release exists. + +**Any test sharing a word with a route counted as testing it.** +A route `GET /users` and an unrelated test named "formats users display names" +produced SUPPORTED. Imports were matched by substring, so `superusers` counted as +`users`, and different HTTP methods on one path were merged. + +**A signature helper anywhere in the repository protected every webhook.** +A billing webhook handler with no verification, plus an unrelated helper that +nothing called, reported SUPPORTED. + +## What changed + +**Authorization must be at the route's own call site.** The route extractor now +captures each route's own arguments, so a guard is attributed to the route it +actually wraps. Guards that are imported but never applied, named in a comment, +named in a string, or applied to a different route in the same file are no +longer evidence. Authentication is distinguished from authorization: +`requireAuth` establishes identity, not permission, and is reported as such. +Guards applied by a router mount or a server-wide middleware are reported as +unresolved, never as protected. A missing signal stays WEAK, never CONTRADICTED. + +**Test association requires an import.** Only a test importing a route's +implementation module can support the claim, matched on exact module stems. +Route identity keeps the HTTP method. Name similarity is now reported as an +explicit unverified suggestion that can never raise the status. The claim is +renamed in output to **Route Test Association** and its statement no longer says +"exercised by tests", because a static association is not execution coverage. + +**Webhook verification is connected per handler.** A signature call must appear +in the handler's own file. Verification in an unrelated module, or inside a test +file, no longer protects production code. Mixed repositories report per-handler +counts ("1 of 2 handlers verify a provider signature"). + +**Routes in test, example, and fixture directories are not application +surface.** They are excluded from the route inventory rather than counted as +untested. On the Express repository this changed the inventory from 142 routes, +nearly all of them from its own `test/` and `examples/` directories, to 9. + +## An honest trade-off + +Removing the false positives means `route-test-association` now abstains far more +often. On three real repositories it reports zero associated routes, because +their tests exercise routes through a running server (supertest, TestClient) +rather than by importing route modules. That is the correct trade - "I cannot +establish this" is better than a false "your routes are tested" - but the claim +is less useful than its v0.5.0 numbers suggested. Those numbers were mostly +name collisions. Resolving request-based association is the next step. + +## Compatibility + +- The claim id `route-test-coverage` is unchanged. Only its display name and + statement changed. +- JSON output stays `schema_version: 2`. +- No command, concept, or MCP tool was renamed or removed. +- Route signals gain `handlers` metadata and a line number; existing databases + are re-scanned normally with no migration. + +## Notes + +- 157 passing tests (16 new). Every reproduction above is now a permanent + regression test, including the adversarial cases: commented guards, unused + imports, guards inside strings, authentication-only guards, two routes in one + file with only one guarded, import stem collisions, and signature calls in + test files. +- The legitimate patterns still resolve: a guard at the call site is SUPPORTED, + a test importing its route is SUPPORTED, and a handler verifying signatures in + its own file is SUPPORTED. +- Known gaps, unchanged by this release: router-level and application-level + guards, verification through an imported helper, and request-based test + association are all reported as unresolved rather than assumed. + +## Names + +- PyPI distribution: `devtime-ei`. Python import: `devtime`. CLI: `dtc`. diff --git a/VERIFICATION.md b/VERIFICATION.md index af1ed73..bdd84de 100644 --- a/VERIFICATION.md +++ b/VERIFICATION.md @@ -62,18 +62,28 @@ so plainly is more useful than an ominous UNKNOWN. ## Built-in claims -- **route-test-coverage** (v0.5) - "HTTP routes are exercised by tests." Reports - how many routes have a referencing test and names the ones that do not. - Attribution is by test imports and route names; end-to-end specs are excluded - because they match by accident. Absence of tests is missing evidence, never a - contradiction. +- **route-test-coverage** (id kept for compatibility; now presented as *Route + Test Association*) - "HTTP routes have tests that import their + implementation." Association is established from test imports only, matched on + exact module stems so `users` does not match `superusers`. Route identity keeps + the HTTP method. A test that merely shares a word with a route path is reported + as an unverified suggestion and can never raise the status. Routes defined in + test, example, or fixture files are not application surface and are excluded + from the inventory. A static association is not execution coverage. - **admin-authorization** (v0.5) - "Administrative routes require an - authorization check." A missing authorization signal is WEAK, never - CONTRADICTED: authorization can be applied globally or by a wrapper the - scanner cannot see, and reporting an endpoint as unprotected when it is not - would destroy the trust this tool is built on. + authorization check." Authorization is established only from a guard applied at + the route's own call site. Authentication is not authorization: `requireAuth` + establishes identity, not permission. Guards that are imported but unused, + named in a comment, named in a string, or applied to a different route in the + same file are not evidence. Guards applied by a router mount or a server-wide + middleware are reported as unresolved, never as protected. A missing signal is + WEAK, never CONTRADICTED. - **billing-webhook-signature** - "Incoming billing webhooks verify the payment - provider's signature." + provider's signature." Verification is connected per handler: a signature call + must appear in the handler's own file. A helper elsewhere in the repository - + even one nothing calls - does not protect a handler, and a call inside a test + file does not protect production code. Mixed repositories report per-handler + counts. - **jwt-authentication** (v0.3) - "Authentication uses JWT access tokens." Includes the documentation-vs-implementation detector: documentation claiming JWT while the only JWT usage found is invitation/verification tokens is a diff --git a/pyproject.toml b/pyproject.toml index c52eb05..cbd0f8c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "devtime-ei" -version = "0.5.0" +version = "0.5.1" description = "Local-first Engineering Intelligence for software repositories" readme = "README.md" requires-python = ">=3.11" @@ -50,7 +50,7 @@ dev = [ Homepage = "https://github.com/Shakargy/devtime" Repository = "https://github.com/Shakargy/devtime" Issues = "https://github.com/Shakargy/devtime/issues" -"Release Notes" = "https://github.com/Shakargy/devtime/releases/tag/v0.5.0" +"Release Notes" = "https://github.com/Shakargy/devtime/releases/tag/v0.5.1" Demo = "https://youtu.be/1Hiu3Y9J_SI" [project.scripts] diff --git a/server.json b/server.json index 46decdb..b3483ea 100644 --- a/server.json +++ b/server.json @@ -8,12 +8,12 @@ "source": "github" }, "websiteUrl": "https://github.com/Shakargy/devtime", - "version": "0.5.0", + "version": "0.5.1", "packages": [ { "registryType": "pypi", "identifier": "devtime-ei", - "version": "0.5.0", + "version": "0.5.1", "transport": { "type": "stdio" } diff --git a/src/devtime/__init__.py b/src/devtime/__init__.py index 8d3a49d..410310a 100644 --- a/src/devtime/__init__.py +++ b/src/devtime/__init__.py @@ -1,6 +1,6 @@ """DevTime - local-first Engineering Intelligence for repository memory.""" -__version__ = "0.5.0" +__version__ = "0.5.1" # Version metadata (Builder Edition, Chapter 20). EVIDENCE_MODEL = "2026.06.1" diff --git a/src/devtime/intelligence/verification.py b/src/devtime/intelligence/verification.py index 89e7a56..3c7d0cb 100644 --- a/src/devtime/intelligence/verification.py +++ b/src/devtime/intelligence/verification.py @@ -32,6 +32,7 @@ from __future__ import annotations import json +import re import sqlite3 import uuid from dataclasses import dataclass, field @@ -165,10 +166,12 @@ class ClaimDefinition: statement="Authentication uses JWT access tokens.", category="authentication", ), + # Slug kept for compatibility; the name and statement no longer claim + # execution coverage, which static association cannot establish (v0.5.1). "route-test-coverage": ClaimDefinition( slug="route-test-coverage", - name="Route Test Coverage", - statement="HTTP routes are exercised by tests.", + name="Route Test Association", + statement="HTTP routes have tests that import their implementation.", category="testing", ), "admin-authorization": ClaimDefinition( @@ -216,6 +219,20 @@ def _load_signals(conn: sqlite3.Connection, scan_id: str) -> list[sqlite3.Row]: ).fetchall() +def _meta(row: sqlite3.Row) -> dict: + try: + return json.loads(row["metadata_json"] or "{}") + except json.JSONDecodeError: + return {} + + +def _route_label(row: sqlite3.Row) -> str: + """Human-facing route identity: method, path, and file.""" + meta = _meta(row) + name = row["name"] or meta.get("path") or "route" + return f"{name} ({row['path']})" + + def _hay(row: sqlite3.Row) -> str: return " ".join( str(x).lower() @@ -224,6 +241,36 @@ def _hay(row: sqlite3.Row) -> str: ) +def _is_test_path(path: str) -> bool: + low = (path or "").lower().replace("\\", "/") + if ".test." in low or ".spec." in low or "_test." in low: + return True + segments = low.split("/") + return any( + seg in ("test", "tests", "__tests__", "spec", "specs", "e2e") + for seg in segments[:-1] + ) + + +# Directories whose routes are illustrations or fixtures, not application surface. +_NON_APP_SEGMENTS = ("examples", "example", "samples", "demo", "demos", + "fixtures", "benchmarks", "docs") + + +def _is_non_app_path(path: str) -> bool: + """True for test, example, fixture, and benchmark code. + + A route defined in a test or an example is not part of the application's + HTTP surface, so it does not belong in an inventory of routes that ought to + have tests (v0.5.1: express reported 142 such "routes", nearly all of them + from its own test/ and examples/ directories). + """ + low = (path or "").lower().replace("\\", "/") + if _is_test_path(low): + return True + return any(seg in _NON_APP_SEGMENTS for seg in low.split("/")[:-1]) + + def _is_billingish(hay: str) -> bool: return any(t in hay for t in _PAYMENT_TOKENS) @@ -287,6 +334,13 @@ def verify_all(conn: sqlite3.Connection) -> list[VerificationResult]: def _verify_billing_webhook_signature( definition: ClaimDefinition, rows: list[sqlite3.Row], scan_id: str ) -> VerificationResult: + # v0.5.1: signature verification must be connected to a webhook handler. + # Previously a verification helper anywhere in the repository - even one + # nothing called - supported the claim for every webhook endpoint. + verify_files: set[str] = set() # non-test files containing a verification call + verification_rows: list[sqlite3.Row] = [] + webhook_route_rows: list[sqlite3.Row] = [] + verifications: list[EvidenceRef] = [] webhook_routes: list[EvidenceRef] = [] stub_webhooks: list[EvidenceRef] = [] @@ -298,10 +352,14 @@ def _verify_billing_webhook_signature( kind = row["kind"] if kind == "webhook_signature_verification": + verification_rows.append(row) + if not _is_test_path(row["path"]): + verify_files.add(row["path"]) verifications.append( _ref(row, "Verifies the provider's webhook signature.", "strong") ) elif kind == "route" and "webhook" in hay and _is_billingish(hay): + webhook_route_rows.append(row) webhook_routes.append( _ref(row, "Billing webhook route is handled here.", "moderate") ) @@ -328,6 +386,84 @@ def _verify_billing_webhook_signature( contradictions: list[Contradiction] = [] supporting: list[EvidenceRef] = [] + # Connect each webhook handler to verification in its own file. + connected = [r for r in webhook_route_rows if r["path"] in verify_files] + unconnected = [r for r in webhook_route_rows if r["path"] not in verify_files] + # A verification call that no webhook handler is connected to. + orphan_verifications = sorted( + verify_files - {r["path"] for r in webhook_route_rows} + ) + + if webhook_route_rows: + n_total = len(webhook_route_rows) + n_connected = len(connected) + supporting = [ + _ref( + r, + "Webhook handler verifies the provider's signature in this file.", + "strong", + ) + for r in connected[:6] + ] + signature_tests[:2] + why.append( + f"{n_connected} of {n_total} billing webhook handler(s) verify a " + "provider signature in the handler's own file." + ) + if unconnected: + why.append( + "The remaining handler(s) have no signature verification connected " + "to them. This is missing evidence, not proof they are unverified." + ) + missing.append( + "Signature verification connected to: " + + ", ".join(sorted({_route_label(r) for r in unconnected})[:6]) + ) + if orphan_verifications: + why.append( + "Signature verification exists in " + + ", ".join(orphan_verifications[:3]) + + " but no webhook handler there was resolved, so it does not " + "establish protection for the handlers above." + ) + for stub in stub_webhooks: + contradictions.append( + Contradiction( + summary="One webhook endpoint is a disabled stub.", + claimed_side=f"{stub.path} is routed as a billing webhook.", + observed_side="Its only behavior is a 404/501 response. This " + "shows the endpoint is disabled; it is not evidence of a " + "runtime vulnerability.", + evidence=[stub], + ) + ) + status = SUPPORTED if n_connected == n_total else WEAK + if not signature_tests: + missing.append("A test that exercises webhook signature verification.") + limitations = [ + "Signature verification is connected to a handler when both appear in " + "the same file. Verification reached through an imported helper is not " + "resolved yet and is reported as unconnected, never as protected.", + "Recognized for known provider patterns (e.g. Stripe constructEvent); " + "custom verification schemes are not detected.", + "Static evidence does not establish that verification runs before the " + "handler's side effects.", + "Coverage follows scanner language support; see LIMITATIONS.md.", + ] + return VerificationResult( + claim_slug=definition.slug, + claim_name=definition.name, + statement=definition.statement, + status=status, + why=why, + supporting=supporting, + contradictions=contradictions, + missing=missing, + limitations=limitations, + scan_id=scan_id, + verified_at=_now(), + engine_version=__version__, + ) + if stub_webhooks and not verifications: for stub in stub_webhooks: contradictions.append( @@ -561,13 +697,21 @@ def _route_tokens(route_path: str) -> list[str]: def _verify_route_test_coverage( definition: ClaimDefinition, rows: list[sqlite3.Row], scan_id: str ) -> VerificationResult: - """Verify that HTTP routes are exercised by tests. + """Verify that HTTP routes have tests importing their implementation. + + v0.5.1 correction: static association is not execution coverage, and a test + that merely shares a word with a route path proves nothing. Two evidence + levels are now distinguished: - Matching is deliberately conservative and explainable. A route counts as - covered when a test file either imports the route's implementation module, - or names a distinctive segment of the route path. Test files are aggregated - first so the comparison stays linear in test FILES, not test cases (large - repos have thousands of test cases across a few dozen files). + import association - a test file imports the route's implementation + module (exact module-stem match, so `users` does not + match `superusers`). This is the only level that can + support the claim. + name similarity - a test name mentions a route segment. Reported as an + unverified suggestion; it can never raise the status. + + Route identity keeps the HTTP method, so a test touching GET does not + establish anything about POST on the same path. Absence of tests is missing evidence, never a contradiction. """ @@ -591,97 +735,132 @@ def _verify_route_test_coverage( imports.add(str(imp).lower()) test_blobs.setdefault(path, []).append(str(row["name"] or "").lower()) - # One joined blob per test file keeps matching linear in test FILES and turns - # each check into a single substring scan. + # One joined blob per test file keeps name matching linear in test FILES. test_name_blob = {p: " ".join(names) for p, names in test_blobs.items()} - test_import_blob = {p: " ".join(sorted(i)) for p, i in test_imports.items()} - - # Deduplicate routes: several methods on one path are one surface to cover. - routes: dict[tuple[str, str], sqlite3.Row] = {} + # Imports are reduced to exact module stems, so importing "superusers" is not + # treated as importing "users" (v0.5.1: substring matching did exactly that). + test_import_stems = { + p: {_module_token(imp) for imp in imports} for p, imports in test_imports.items() + } + + # Route identity keeps the HTTP method: a test touching GET establishes + # nothing about POST on the same path. + routes: dict[tuple[str, str, str], sqlite3.Row] = {} + excluded_non_app = 0 for row in rows: if row["kind"] != "route": continue - try: - meta = json.loads(row["metadata_json"] or "{}") - except json.JSONDecodeError: - meta = {} + # Routes defined inside tests, examples, or fixtures are not the + # application's HTTP surface and must not be counted as untested. + if _is_non_app_path(row["path"]): + excluded_non_app += 1 + continue + meta = _meta(row) route_path = str(meta.get("path") or row["name"] or "").strip() - routes.setdefault((row["path"], route_path.lower()), row) + method = str(meta.get("method") or "ANY").upper() + routes.setdefault((row["path"], route_path.lower(), method), row) if not routes: + reason = "No application HTTP routes were found in the scanned files." + if excluded_non_app: + reason += ( + f" {excluded_non_app} route(s) were found only in test, example, " + "or fixture files, which are not application surface." + ) return _not_applicable( definition, scan_id, - "No HTTP routes were found in the scanned files.", - "Any HTTP route (Express, Next.js, or FastAPI style).", + reason, + "Any HTTP route in application code (Express, Next.js, or FastAPI style).", ) - covered: list[tuple[str, str, str]] = [] # (impl path, route path, reason) - uncovered: list[tuple[str, str]] = [] - for (impl_path, route_path), row in sorted(routes.items()): + associated: list[tuple[str, str, str]] = [] # (impl path, label, reason) + suggested: list[tuple[str, str, str]] = [] # name similarity only + unassociated: list[tuple[str, str]] = [] + for (impl_path, route_path, method), row in sorted(routes.items()): + label = f"{method} {route_path}" if route_path else impl_path token = _module_token(impl_path) reason = "" - # 1. A test that imports the implementation module. + # Only level that can support the claim: a test importing this module. if token and len(token) >= 3: - for test_path, blob in test_import_blob.items(): - if token in blob: + for test_path, stems in test_import_stems.items(): + if token in stems: reason = f"{test_path} imports {token}" break - # 2. A test whose names mention a distinctive segment of the route path. - if not reason: - segments = _route_tokens(route_path) - for test_path, blob in test_name_blob.items(): - if segments and any(seg in blob for seg in segments): - reason = f"{test_path} names {segments[0]}" - break if reason: - covered.append((impl_path, route_path, reason)) - else: - uncovered.append((impl_path, route_path)) + associated.append((impl_path, label, reason)) + continue + # Name similarity is a suggestion, never support. + hint = "" + segments = _route_tokens(route_path) + for test_path, blob in test_name_blob.items(): + if segments and any(seg in blob for seg in segments): + hint = f"{test_path} mentions '{segments[0]}'" + break + if hint: + suggested.append((impl_path, label, hint)) + unassociated.append((impl_path, label)) total = len(routes) - n_covered = len(covered) - # Evidence is bounded (responses and stored fingerprints must stay bounded), - # and truncation is disclosed below rather than hidden. + n_assoc = len(associated) + # Evidence shown to the user is bounded; the dependency set used for + # invalidation is tracked separately and is not capped. _EVIDENCE_CAP = 25 sha_by_path = {row["path"]: row["sha256"] for row in rows} supporting = [ EvidenceRef( path=impl, - observation=f"Route {route or impl} is referenced by a test ({reason}).", + observation=f"Route {label} has a test importing its implementation " + f"({reason}).", kind="route", strength="moderate", sha256=sha_by_path.get(impl), ) - for impl, route, reason in covered[:_EVIDENCE_CAP] + for impl, label, reason in associated[:_EVIDENCE_CAP] ] - why = [f"{n_covered} of {total} routes have a referencing test."] + why = [ + f"{n_assoc} of {total} routes have a test importing their implementation." + ] missing: list[str] = [] - if n_covered == total: + if n_assoc == total: status = SUPPORTED - why.append("Every detected route has at least one test referencing it.") + why.append( + "Every detected route has a test that imports its implementation. " + "This is a static association, not proof the route was executed." + ) else: status = WEAK why.append( - "Routes without a referencing test are not proven to be exercised." + "Routes without an importing test have no established association." ) - shown = [r or p for p, r in uncovered[:8]] + shown = [label for _, label in unassociated[:8]] missing.append( - f"Tests referencing {total - n_covered} route(s): " + ", ".join(shown) - + (" ..." if len(uncovered) > 8 else "") + f"Tests importing {total - n_assoc} route(s): " + ", ".join(shown) + + (" ..." if len(unassociated) > 8 else "") + ) + if suggested: + why.append( + f"{len(suggested)} route(s) share vocabulary with a test name but no " + "import was found. Name similarity is a suggestion, not evidence: " + + suggested[0][2] ) - limitations = _LIMITATIONS + [ - "Coverage is attributed by test imports and route names, not by executing " - "tests; a route exercised only indirectly may be reported as uncovered.", - "End-to-end specs are excluded from attribution because they match by " - "accident.", + limitations = [ + "Association is established from test imports only. A test that exercises " + "a route indirectly, through a running server, or through a helper that " + "DevTime cannot resolve is reported as unassociated.", + "A static import association is not execution coverage. It does not " + "establish that the route ran, that assertions covered its behavior, or " + "that the test passes.", + "End-to-end specs are excluded from attribution because their names match " + "by accident; a direct request made by an e2e test is not yet resolved.", + "Coverage follows scanner language support; see LIMITATIONS.md.", ] - if len(covered) > _EVIDENCE_CAP: + if len(associated) > _EVIDENCE_CAP: limitations.append( - f"Evidence is capped at {_EVIDENCE_CAP} routes; freshness tracks only " - f"those recorded files, not all {len(covered)} covered routes." + f"Displayed evidence is capped at {_EVIDENCE_CAP} of {len(associated)} " + "associated routes." ) return VerificationResult( claim_slug=definition.slug, @@ -704,13 +883,33 @@ def _verify_route_test_coverage( # --------------------------------------------------------------------------- # _ADMIN_TOKENS = ("admin", "superuser", "staff", "backoffice", "back-office") + +# Authorization: establishes a role or permission decision. _AUTHZ_TOKENS = ( "requireadmin", "require_admin", "isadmin", "is_admin", "adminonly", - "admin_only", "hasrole", "has_role", "authorize", "authorization", - "permission", "rbac", "requireauth", "require_auth", "isauthenticated", - "current_user", "get_current_user", "authmiddleware", "auth_middleware", + "admin_only", "hasrole", "has_role", "requirerole", "require_role", + "authorize", "requirepermission", "require_permission", "checkpermission", + "check_permission", "haspermission", "has_permission", "rbac", "withadmin", + "with_admin", "adminguard", "admin_guard", "ensureadmin", "ensure_admin", +) + +# Authentication: establishes identity only. Knowing WHO the caller is does not +# establish that they are ALLOWED to use an administrative endpoint, so these +# never satisfy the authorization claim on their own. +_AUTHN_ONLY_TOKENS = ( + "requireauth", "require_auth", "isauthenticated", "is_authenticated", + "ensureauth", "ensure_auth", "authmiddleware", "auth_middleware", + "current_user", "currentuser", "get_current_user", "getcurrentuser", + "requirelogin", "require_login", "withauth", "with_auth", ) +_COMMENT_RE = re.compile(r"/\*.*?\*/|//[^\n]*", re.S) + + +def _executable_text(fragment: str) -> str: + """Strip comments so commented-out guards cannot become evidence.""" + return _COMMENT_RE.sub(" ", fragment or "").lower() + def _verify_admin_authorization( definition: ClaimDefinition, rows: list[sqlite3.Row], scan_id: str @@ -722,22 +921,17 @@ def _verify_admin_authorization( wrapper the scanner cannot see. Telling someone their admin endpoint is unprotected when it is not would destroy the trust this tool is built on. """ + # Surface detection uses the ROUTE PATH only. A file named permissions.ts is + # not evidence about behavior (v0.5.1: it previously was, and produced + # SUPPORTED on routes with no guard at all). admin_routes: list[sqlite3.Row] = [] - authz_files: set[str] = set() - authz_rows: list[sqlite3.Row] = [] - for row in rows: - hay = _hay(row) - kind = row["kind"] - if kind == "route" and any(t in hay for t in _ADMIN_TOKENS): + if row["kind"] != "route": + continue + meta = _meta(row) + route_path = str(meta.get("path") or row["name"] or "").lower() + if any(t in route_path for t in _ADMIN_TOKENS): admin_routes.append(row) - if kind in ("middleware", "auth_dependency") or any( - t in hay for t in _AUTHZ_TOKENS - ): - if kind in ("middleware", "auth_dependency", "route", "test"): - authz_files.add(row["path"]) - if kind in ("middleware", "auth_dependency"): - authz_rows.append(row) if not admin_routes: return _not_applicable( @@ -747,48 +941,84 @@ def _verify_admin_authorization( "An admin, staff, or back-office route.", ) - protected: list[sqlite3.Row] = [] - unprotected: list[sqlite3.Row] = [] + guarded: list[sqlite3.Row] = [] + identity_only: list[sqlite3.Row] = [] + unresolved: list[sqlite3.Row] = [] # no call-site evidence available + no_guard: list[sqlite3.Row] = [] + for row in admin_routes: - hay = _hay(row) - # Authorization evidence in the route's own file, or in the route itself. - if row["path"] in authz_files or any(t in hay for t in _AUTHZ_TOKENS): - protected.append(row) + meta = _meta(row) + if "handlers" not in meta: + # Next.js and other file-based routes have no argument list to read. + unresolved.append(row) + continue + call_site = _executable_text(str(meta.get("handlers") or "")) + if any(t in call_site for t in _AUTHZ_TOKENS): + guarded.append(row) + elif any(t in call_site for t in _AUTHN_ONLY_TOKENS): + identity_only.append(row) else: - unprotected.append(row) + no_guard.append(row) supporting = [ - _ref(r, "Admin route shows an authorization check in its file.", "moderate") - for r in protected[:5] - ] + [ - _ref(r, "Authorization middleware or dependency.", "moderate") - for r in authz_rows[:2] + _ref( + r, + f"Authorization guard is applied at this route's own call site " + f"({str(_meta(r).get('handlers') or '')[:80]}).", + "strong", + ) + for r in guarded[:5] ] total = len(admin_routes) - why = [f"{len(protected)} of {total} administrative route(s) show an " - "authorization check."] + why = [ + f"{len(guarded)} of {total} administrative route(s) have an authorization " + "guard at their own call site." + ] missing: list[str] = [] - if not unprotected: + if len(guarded) == total: status = SUPPORTED - why.append("Every detected admin route has authorization evidence.") + why.append("Every detected admin route applies an authorization guard directly.") else: status = WEAK - why.append( - "No authorization evidence was found for the remaining admin route(s). " - "This is missing evidence, not proof that they are unprotected." - ) - missing.append( - "Authorization evidence for: " - + ", ".join(sorted({r["path"] for r in unprotected})[:6]) - ) + if identity_only: + why.append( + f"{len(identity_only)} route(s) apply an authentication check but no " + "role or permission check. Knowing who the caller is does not " + "establish that they may use an administrative endpoint." + ) + missing.append( + "A role or permission check for: " + + ", ".join(sorted({_route_label(r) for r in identity_only})[:6]) + ) + if no_guard: + why.append( + f"{len(no_guard)} route(s) have no guard at their call site. This is " + "missing evidence, not proof that they are unprotected." + ) + missing.append( + "Authorization evidence for: " + + ", ".join(sorted({_route_label(r) for r in no_guard})[:6]) + ) + if unresolved: + why.append( + f"{len(unresolved)} route(s) use a routing style whose guard " + "association DevTime cannot resolve yet." + ) + missing.append( + "A resolvable guard association for: " + + ", ".join(sorted({_route_label(r) for r in unresolved})[:6]) + ) - limitations = _LIMITATIONS + [ - "Authorization applied globally (a server-wide middleware, a router " - "mount, or a framework decorator the scanner does not parse) is not " - "detected. A WEAK result means DevTime found no evidence, never that a " - "route is confirmed unprotected.", + limitations = [ + "Authorization is established only from a guard applied at the route's own " + "call site. Guards applied by a router mount, a server-wide middleware, or " + "a framework decorator are not resolved yet and are reported as unresolved, " + "never as protected.", + "A WEAK result means DevTime found no connected authorization evidence. It " + "is never proof that a route is unprotected.", + "Coverage follows scanner language support; see LIMITATIONS.md.", ] return VerificationResult( claim_slug=definition.slug, diff --git a/src/devtime/scanner/extractors/typescript.py b/src/devtime/scanner/extractors/typescript.py index 2474fe8..c5c853b 100644 --- a/src/devtime/scanner/extractors/typescript.py +++ b/src/devtime/scanner/extractors/typescript.py @@ -15,12 +15,33 @@ _IMPORT_RE = re.compile(r"""import\s+.*?from\s+['"]([^'"]+)['"]""") # Match app/router as well as named routers (authRouter, exportRouter, etc). _ROUTE_RE = re.compile( - r"""\b(?:app|\w*[Rr]outer)\.(get|post|put|patch|delete)\(\s*['"]([^'"]+)['"]""", + r"""\b(?:app|\w*[Rr]outer)\.(get|post|put|patch|delete)(\()\s*['"]([^'"]+)['"]""", re.I, ) _MIDDLEWARE_RE = re.compile( r"""\b(requireAuth|authMiddleware|isAuthenticated|ensureAuth|requireAdmin)\b""" ) + + +def _call_arguments(text: str, open_paren: int, limit: int = 600) -> str: + """Return the argument text of a call whose '(' is at ``open_paren``. + + v0.5.1: evidence about a route must come from that route's own call site, + not from anywhere in the file. Walking the balanced parentheses is enough + to capture `router.get("/admin", requireAdmin, handler)` without pulling in + unrelated code, comments, or other routes further down the file. + """ + depth = 0 + end = min(len(text), open_paren + limit) + for i in range(open_paren, end): + ch = text[i] + if ch == "(": + depth += 1 + elif ch == ")": + depth -= 1 + if depth == 0: + return text[open_paren + 1 : i] + return text[open_paren + 1 : end] _BULLMQ_WORKER_RE = re.compile(r"""new\s+Worker\(\s*['"]([^'"]+)['"]""") _BULLMQ_QUEUE_RE = re.compile(r"""new\s+Queue\(\s*['"]([^'"]+)['"]""") # Custom task-runner infrastructure (v0.1.3, Cal.com proof run): Cal.com's Tasker @@ -48,14 +69,23 @@ def extract_typescript_signals(file: WalkedFile) -> list[Signal]: for match in _ROUTE_RE.finditer(text): method = match.group(1).upper() - path = match.group(2) + path = match.group(3) + # v0.5.1: the route's own arguments are what can protect it. Everything + # else in the file is a different route's business. + handlers = _call_arguments(text, match.start(2)) signals.append( signal( "route", name=f"{method} {path}", file=file, + start_line=text.count("\n", 0, match.start()) + 1, confidence=0.8, - metadata={"method": method, "path": path, "framework": "express"}, + metadata={ + "method": method, + "path": path, + "framework": "express", + "handlers": handlers.strip(), + }, ) ) diff --git a/tests/integration/test_evidence_precision.py b/tests/integration/test_evidence_precision.py index 90968ca..7c57e80 100644 --- a/tests/integration/test_evidence_precision.py +++ b/tests/integration/test_evidence_precision.py @@ -14,14 +14,14 @@ # --- version ------------------------------------------------------------------ -def test_version_is_release_0_5_0(): - # v0.5.0 release: package metadata and __version__ agree on the release version. +def test_version_is_release_0_5_1(): + # v0.5.1 release: package metadata and __version__ agree on the release version. import importlib.metadata as m - assert devtime.__version__ == "0.5.0" + assert devtime.__version__ == "0.5.1" # Distribution is published as "devtime-ei" (the name "devtime" is reserved on # PyPI); the import package and the dtc command stay "devtime"/"dtc". - assert m.version("devtime-ei") == "0.5.0" + assert m.version("devtime-ei") == "0.5.1" # --- P0 Authentication headline precision ------------------------------------ diff --git a/tests/integration/test_verification.py b/tests/integration/test_verification.py index 89361ff..eb6f72f 100644 --- a/tests/integration/test_verification.py +++ b/tests/integration/test_verification.py @@ -477,7 +477,11 @@ def test_admin_authorization_missing_authz_is_weak_never_contradicted( assert result.status == ver.WEAK assert result.contradictions == [] assert any("not proof" in w.lower() for w in result.why) - assert any("globally" in lim for lim in result.limitations) + # The safety-critical disclosures: guards DevTime cannot resolve are named, + # and WEAK is never presented as proof that a route is unprotected. + blob = " ".join(result.limitations).lower() + assert "server-wide" in blob or "router mount" in blob + assert "never proof" in blob or "not proof" in blob def test_admin_authorization_not_applicable_without_admin_routes(tmp_path, monkeypatch): @@ -570,3 +574,258 @@ def test_single_claim_that_does_not_apply_still_explains_itself(tmp_path, monkey assert result.exit_code == 0 assert "NOT_APPLICABLE" in result.stdout assert "does not apply" in result.stdout + + +# --- v0.5.1: false-SUPPORTED regressions ----------------------------------------- +# +# Each of these produced SUPPORTED in v0.5.0 with no justifying evidence. They +# are the reason this release exists; none of them may ever pass again. + +ADMIN_NO_GUARD = """ +import express from "express"; +const router = express.Router(); +router.get("/admin/permissions", listPermissions); +export default router; +""" + +ADMIN_AUTHN_ONLY = """ +import express from "express"; +import { requireAuth } from "./auth"; +const router = express.Router(); +router.get("/admin/settings", requireAuth, editSettings); +""" + +ADMIN_FAKE_GUARDS = """ +import express from "express"; +import { requireAdmin } from "./auth"; +const router = express.Router(); +// TODO: add requireAdmin here +const note = "requireAdmin hasRole"; +router.get("/admin/a", handler); +""" + +ADMIN_GUARDED = """ +import express from "express"; +import { requireAdmin } from "./auth"; +const router = express.Router(); +router.get("/admin/a", requireAdmin, handler); +""" + +ADMIN_MIXED = """ +import express from "express"; +import { requireAdmin } from "./auth"; +const router = express.Router(); +router.get("/admin/users", requireAdmin, listUsers); +router.get("/admin/logs", readLogs); +""" + + +def test_admin_filename_is_not_authorization_evidence(tmp_path, monkeypatch): + # v0.5.0 bug: the evaluator matched combined text that included the FILE + # PATH, so src/admin/permissions.ts satisfied the "permission" token and + # reported "1 of 1 administrative route(s) show an authorization check" + # on a repository containing no guard at all. + _repo(tmp_path, {"src/admin/permissions.ts": ADMIN_NO_GUARD}) + _init_scan(tmp_path, monkeypatch) + result = _verify("admin-authorization") + assert result.status == ver.WEAK + assert "0 of 1" in " ".join(result.why) + + +def test_admin_authentication_alone_is_not_authorization(tmp_path, monkeypatch): + _repo(tmp_path, {"src/admin/settings.ts": ADMIN_AUTHN_ONLY}) + _init_scan(tmp_path, monkeypatch) + result = _verify("admin-authorization") + assert result.status == ver.WEAK + assert any("role or permission" in w.lower() for w in result.why) + + +def test_admin_commented_and_unused_guards_are_not_evidence(tmp_path, monkeypatch): + _repo(tmp_path, {"src/admin/a.ts": ADMIN_FAKE_GUARDS}) + _init_scan(tmp_path, monkeypatch) + assert _verify("admin-authorization").status == ver.WEAK + + +def test_admin_guard_at_call_site_is_supported(tmp_path, monkeypatch): + # The legitimate pattern must keep working, or the fix is useless. + _repo(tmp_path, {"src/admin/a.ts": ADMIN_GUARDED}) + _init_scan(tmp_path, monkeypatch) + assert _verify("admin-authorization").status == ver.SUPPORTED + + +def test_admin_two_routes_one_guarded_is_not_supported(tmp_path, monkeypatch): + # A guard on one route does not protect its neighbour in the same file. + _repo(tmp_path, {"src/admin/a.ts": ADMIN_MIXED}) + _init_scan(tmp_path, monkeypatch) + result = _verify("admin-authorization") + assert result.status == ver.WEAK + assert "1 of 2" in " ".join(result.why) + assert any("/admin/logs" in m for m in result.missing) + + +ROUTE_USERS = """ +import express from "express"; +const router = express.Router(); +router.get("/users", listUsers); +""" + +UNRELATED_USERS_TEST = """ +import { describe, it } from "vitest"; +describe("display", () => { it("formats users display names", () => {}); }); +""" + +SUPERUSERS_TEST = """ +import { listSuperusers } from "../src/routes/superusers"; +import { describe, it } from "vitest"; +describe("superusers", () => { it("lists", () => {}); }); +""" + +IMPORTING_USERS_TEST = """ +import router from "../src/routes/users"; +import { describe, it } from "vitest"; +describe("users", () => { it("lists", () => {}); }); +""" + + +def test_name_similarity_alone_does_not_associate_a_test(tmp_path, monkeypatch): + # v0.5.0 bug: a test merely sharing the word "users" produced SUPPORTED. + _repo(tmp_path, { + "src/routes/users.ts": ROUTE_USERS, + "tests/display.test.ts": UNRELATED_USERS_TEST, + }) + _init_scan(tmp_path, monkeypatch) + result = _verify("route-test-coverage") + assert result.status == ver.WEAK + assert "0 of 1" in " ".join(result.why) + # The similarity may be offered as a suggestion, but never as support. + assert any("suggestion" in w.lower() for w in result.why) + + +def test_import_stem_collision_does_not_associate(tmp_path, monkeypatch): + # Importing "superusers" is not importing "users". + _repo(tmp_path, { + "src/routes/users.ts": ROUTE_USERS, + "tests/superusers.test.ts": SUPERUSERS_TEST, + }) + _init_scan(tmp_path, monkeypatch) + assert _verify("route-test-coverage").status == ver.WEAK + + +def test_importing_test_associates_and_never_claims_execution(tmp_path, monkeypatch): + _repo(tmp_path, { + "src/routes/users.ts": ROUTE_USERS, + "tests/users.test.ts": IMPORTING_USERS_TEST, + }) + _init_scan(tmp_path, monkeypatch) + result = _verify("route-test-coverage") + assert result.status == ver.SUPPORTED + blob = (" ".join(result.why) + " " + " ".join(result.limitations)).lower() + assert "not execution coverage" in blob or "not proof the route was executed" in blob + assert "exercised by tests" not in result.statement.lower() + + +WEBHOOK_ROUTE_BARE = """ +import express from "express"; +const router = express.Router(); +router.post("/api/stripe/webhook", (req, res) => { res.json({ ok: true }); }); +""" + +UNUSED_SIG_HELPER = """ +import Stripe from "stripe"; +const stripe = new Stripe(process.env.KEY); +export function verifyIt(body, sig, secret) { + return stripe.webhooks.constructEvent(body, sig, secret); +} +""" + +WEBHOOK_ROUTE_VERIFIED = """ +import express from "express"; +import Stripe from "stripe"; +const stripe = new Stripe(process.env.KEY); +const router = express.Router(); +router.post("/api/stripe/webhook", (req, res) => { + const event = stripe.webhooks.constructEvent(req.body, sig, secret); + res.json({ received: true }); +}); +""" + +WEBHOOK_ROUTE_PAYPAL_BARE = """ +import express from "express"; +const router = express.Router(); +router.post("/api/paypal/webhook", (req, res) => { res.json({ ok: true }); }); +""" + + +def test_unconnected_signature_helper_does_not_support_handler(tmp_path, monkeypatch): + # v0.5.0 bug: a verification helper anywhere in the repo - even one nothing + # calls - reported SUPPORTED for every billing webhook endpoint. + _repo(tmp_path, { + "src/billing/webhook-route.ts": WEBHOOK_ROUTE_BARE, + "src/util/sig-helper.ts": UNUSED_SIG_HELPER, + }) + _init_scan(tmp_path, monkeypatch) + result = _verify("billing-webhook-signature") + assert result.status == ver.WEAK + assert "0 of 1" in " ".join(result.why) + + +def test_handler_local_verification_is_supported(tmp_path, monkeypatch): + _repo(tmp_path, {"src/billing/stripe-webhook.ts": WEBHOOK_ROUTE_VERIFIED}) + _init_scan(tmp_path, monkeypatch) + assert _verify("billing-webhook-signature").status == ver.SUPPORTED + + +def test_partial_webhook_coverage_is_reported_per_handler(tmp_path, monkeypatch): + _repo(tmp_path, { + "src/billing/stripe-webhook.ts": WEBHOOK_ROUTE_VERIFIED, + "src/billing/paypal-webhook.ts": WEBHOOK_ROUTE_PAYPAL_BARE, + }) + _init_scan(tmp_path, monkeypatch) + result = _verify("billing-webhook-signature") + assert result.status == ver.WEAK + assert "1 of 2" in " ".join(result.why) + assert any("paypal" in m.lower() for m in result.missing) + + +def test_signature_call_in_test_file_does_not_protect_production(tmp_path, monkeypatch): + # A verification call inside a test fixture is not handler protection. + _repo(tmp_path, { + "src/billing/webhook-route.ts": WEBHOOK_ROUTE_BARE, + "tests/webhook.test.ts": UNUSED_SIG_HELPER, + }) + _init_scan(tmp_path, monkeypatch) + assert _verify("billing-webhook-signature").status == ver.WEAK + + +def test_routes_in_tests_and_examples_are_not_application_surface(tmp_path, monkeypatch): + # v0.5.1: express reported "142 routes", nearly all of them defined inside + # its own test/ and examples/ directories. Those are fixtures, not the + # application's HTTP surface, and counting them as untested is noise. + _repo(tmp_path, { + "test/acceptance/auth.js": + 'const express = require("express");\n' + "const app = express();\n" + 'app.get("/fixture-route", handler);\n', + "examples/hello/index.js": + 'const express = require("express");\n' + "const app = express();\n" + 'app.get("/example-route", handler);\n', + }) + _init_scan(tmp_path, monkeypatch) + result = _verify("route-test-coverage") + assert result.status == ver.NOT_APPLICABLE + assert any("test, example" in w for w in result.why) + + +def test_application_routes_are_still_counted_alongside_fixtures(tmp_path, monkeypatch): + _repo(tmp_path, { + "src/routes/users.ts": ROUTE_USERS, + "test/acceptance/auth.js": + 'const express = require("express");\n' + "const app = express();\n" + 'app.get("/fixture-route", handler);\n', + }) + _init_scan(tmp_path, monkeypatch) + result = _verify("route-test-coverage") + # Only the application route is in the inventory. + assert "of 1 routes" in " ".join(result.why)