From e8e239b4324337b806920dcccc455c03cf9f05cd Mon Sep 17 00:00:00 2001 From: Aviad Date: Sat, 5 Sep 2026 22:04:11 +0300 Subject: [PATCH] fix: three false SUPPORTED results in claim verification (v0.5.1) 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. - admin-authorization matched a combined text that included the FILE PATH, so src/admin/permissions.ts satisfied its "permission" token and reported "1 of 1 administrative route(s) show an authorization check" on a repository with no guard anywhere. Security-adjacent, and the reason for this release. - route-test-coverage accepted name similarity as evidence: an unrelated test named "formats users display names" supported GET /users. Imports matched by substring (superusers counted as users) and HTTP methods were merged. - billing-webhook-signature accepted a verification helper anywhere in the repository, including one nothing called, as protection for every handler. Fixes: - The route extractor captures each route's own call arguments, so a guard is attributed to the route it actually wraps. Comments are stripped; imported but unapplied guards, guards in strings, and guards on a sibling route are no longer evidence. Authentication (requireAuth) is distinguished from authorization; router-level and server-wide guards are reported as unresolved, never as protected. - Test association requires an import matched on exact module stems; route identity keeps the HTTP method; name similarity is reported as an unverified suggestion that cannot raise status. The claim is presented as Route Test Association and no longer says "exercised by tests": static association is not execution coverage. - Webhook signature verification is connected per handler by file. Calls in unrelated modules or in test files no longer protect production handlers. Mixed repositories report per-handler counts. - Routes defined in test, example, and fixture directories are excluded from the application route inventory (Express reported 142 such routes; 9 are application surface). Honest trade-off: route-test-association now abstains on real repositories whose tests exercise routes through a running server rather than by import. Its v0.5.0 numbers were mostly name collisions. Request-based association is the next step. 157 tests (16 new); every reproduction and adversarial case is now a permanent regression test. Claim ids and JSON schema_version are unchanged. Version 0.5.1. --- LIMITATIONS.md | 17 +- QUICKSTART.md | 4 +- README.md | 9 +- RELEASE_NOTES_v0.5.1.md | 93 ++++ VERIFICATION.md | 30 +- pyproject.toml | 4 +- server.json | 4 +- src/devtime/__init__.py | 2 +- src/devtime/intelligence/verification.py | 434 ++++++++++++++----- src/devtime/scanner/extractors/typescript.py | 36 +- tests/integration/test_evidence_precision.py | 8 +- tests/integration/test_verification.py | 261 ++++++++++- 12 files changed, 765 insertions(+), 137 deletions(-) create mode 100644 RELEASE_NOTES_v0.5.1.md 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)