From 60d2a1a92c9eccd1e573515fc159ef14efb03687 Mon Sep 17 00:00:00 2001 From: Marius Ciepluch <11855163+norandom@users.noreply.github.com> Date: Wed, 9 Sep 2026 15:39:58 +0200 Subject: [PATCH] feat(model): an endpoint its contract declares open carries no obligation contributor-scan 2.8. VAmPI's last two false positives were register_user and login_user, both declared open by its OpenAPI spec: the arbiter was reporting a missing guard on endpoints whose contract says there is nothing to miss. **The fix keys on provenance, not on the value, and that distinction is the whole task.** The mapper says "public" in two very different situations. An OpenAPI operation with no `security` block is DECLARED open, because the spec is a complete contract. A Flask handler with no `@login_required` is "public" only because nothing was found -- which is precisely the bug this family exists to report. Suppressing on the value alone would have silenced the detector on its primary case, so EntryPointRecord now carries `access_declared` and only a declaration removes the obligation. WordPress supplied the second source of declared access on the same day -- wp_ajax_nopriv_ for a logged-out handler, wp_ajax_ for a logged-in one, permission_callback => __return_true for an open REST route. A rule resting on one framework's convention would be a special case; two make it a shape. vampi entailed 6 -> 4 access-control precision 2 of 4 -> 2 of 2 known vulnerabilities 3 of 3 still entailed access_control offered to 6 of 14 handlers, exactly the six the spec declares authenticated **And the guard that decides whether it is a real improvement:** the access-control pair corpus, which nobody tuned this against, is unmoved -- pair_correct 3 and leak 1 across all nine, pair_correct 2 and leak 0.0% across the seven from projects unrelated to VAmPI, identical to task 2.6. The precision was not bought by silencing the family. Pair excerpts carry no route manifest and no hook, so nothing in them is declared open and the rule correctly does not fire. Gates byte-identical. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01T7XD7x5KonMdmGRkp63EQG --- .kiro/specs/contributor-scan/tasks.md | 2 +- .../2026-09-09-declared-public.json | 24 +++++++++++++ src/openultrasast/mapping.py | 23 ++++++++++-- src/openultrasast/model/regions.py | 8 ++++- tests/test_model_regions.py | 36 ++++++++++++++++++- 5 files changed, 88 insertions(+), 5 deletions(-) create mode 100644 benchmarks/measurements/2026-09-09-declared-public.json diff --git a/.kiro/specs/contributor-scan/tasks.md b/.kiro/specs/contributor-scan/tasks.md index e6ba84f..e10a8c1 100644 --- a/.kiro/specs/contributor-scan/tasks.md +++ b/.kiro/specs/contributor-scan/tasks.md @@ -169,7 +169,7 @@ - _Requirements: 5.1_ - _Depends: 2.2_ -- [ ] 2.8 An endpoint that declares itself public carries no obligation +- [x] 2.8 An endpoint that declares itself public carries no obligation - **2.6's remainder.** Both false positives left on VAmPI are `register_user` and `login_user`, which its OpenAPI spec declares public with no `security` block. The arbiter cannot see that declaration, so it reports a missing guard on an endpoint whose contract says there is nothing to miss. diff --git a/benchmarks/measurements/2026-09-09-declared-public.json b/benchmarks/measurements/2026-09-09-declared-public.json new file mode 100644 index 0000000..8c96d3c --- /dev/null +++ b/benchmarks/measurements/2026-09-09-declared-public.json @@ -0,0 +1,24 @@ +{ + "caveat": "One repository has a route manifest and one framework has been read for hook conventions. Every other language still infers 'public' from an absent decorator, and for those the obligation is correctly kept -- so this rule improves precision exactly where a contract exists and changes nothing anywhere else.", + "generated": "2026-09-09", + "measured": { + "known_vulnerabilities": "3 of 3 still entailed -- the SQL injection and both documented BOLAs", + "regions_offered_access_control": "6 of 14 handlers, exactly the six the spec declares authenticated", + "removed": [ + "api_views/users.py:register_user", + "api_views/users.py:login_user" + ], + "vampi_access_control_precision": "2 of 4 (50.0%) -> 2 of 2 (100.0%)", + "vampi_entailed": "6 -> 4" + }, + "reading": "An endpoint whose own contract declares it open carries no authorization obligation -- there is nothing for a guard to be missing from. VAmPI's last two false positives were register_user and login_user, both declared open by its OpenAPI spec, and both are gone. Access-control precision on the repository goes from 2 of 4 to 2 of 2, with every known vulnerability still entailed and the pair corpus unmoved.", + "second_framework": "WordPress supplied the second source of declared access on the same day: wp_ajax_nopriv_ for a logged-out admin-ajax handler, wp_ajax_ for a logged-in one, and permission_callback => __return_true for an open REST route. A rule resting on one framework's convention would have been a special case; two makes it a shape.", + "spec": "contributor-scan 2.8 (Req 8.1, 8.2)", + "the_guard_that_matters": { + "all_pairs": "pair_correct 3, leak 1 (11.1%) -- unchanged from task 2.6", + "check": "the access-control pair corpus, which nobody tuned this against", + "excluding_vampi_excerpts": "pair_correct 2, leak 0 (0.0%) -- unchanged", + "reading": "The precision was not bought by silencing the family. Pair excerpts carry no route manifest and no WordPress hook, so nothing in them is declared open and the rule does not fire there -- which is the correct behaviour and the reason the numbers are identical." + }, + "why_provenance_and_not_the_value": "The mapper's access_level says 'public' in two very different situations. An OpenAPI operation with no `security` block is DECLARED open, because the spec is a complete contract; a WordPress `wp_ajax_nopriv_` hook is WordPress saying logged-out callers reach this. But a Flask handler with no `@login_required` is 'public' only because nothing was found -- which is precisely the bug this family exists to report. Suppressing on the value alone would have silenced the detector on its primary case, so EntryPointRecord now carries `access_declared` and only a declaration removes the obligation." +} diff --git a/src/openultrasast/mapping.py b/src/openultrasast/mapping.py index 8c3164a..5c21c35 100644 --- a/src/openultrasast/mapping.py +++ b/src/openultrasast/mapping.py @@ -74,6 +74,14 @@ class EntryPointRecord: conditions: list[str] provenance: str rationale: str + # Did the access level come from a DECLARATION, or from the absence of one? + # + # The distinction decides whether "public" can be trusted. An OpenAPI operation with no `security` block + # is declared open, because the spec is a complete contract; a WordPress `wp_ajax_nopriv_` hook is + # WordPress saying logged-out callers reach this. But a Flask handler with no `@login_required` is + # "public" only because nothing was found -- which is precisely the bug the access-control family exists + # to report, so treating it as a declaration would silence the detector on its primary case. + access_declared: bool = False def ingest_sarif(path: Path) -> list[StaticHint]: @@ -221,7 +229,9 @@ def _php_entry_points(target: FileTarget, text: str) -> list[EntryPointRecord]: hook, handler = match.group("hook"), match.group("handler") access, evidence = _wordpress_hook_access(match.group("call"), hook) start, end = bounds.get(handler, (number, number)) - records.append(_entry(target, start, end, handler, f"wp:{hook}", "route", access, "http_request", evidence, [])) + records.append( + _entry(target, start, end, handler, f"wp:{hook}", "route", access, "http_request", evidence, [], access_declared=True) + ) if _PHP_REST_ROUTE.search(line): window = "\n".join(lines[number - 1 : number + 12]) callback = _PHP_CALLBACK.search(window) @@ -230,7 +240,11 @@ def _php_entry_points(target: FileTarget, text: str) -> list[EntryPointRecord]: permission = _PHP_PERMISSION.search(window) access, evidence = _rest_route_access(permission.group("value").strip() if permission else None) start, end = bounds.get(handler, (number, number)) - records.append(_entry(target, start, end, handler, "wp:rest_route", "route", access, "http_request", evidence, [])) + records.append( + _entry( + target, start, end, handler, "wp:rest_route", "route", access, "http_request", evidence, [], access_declared=True + ) + ) registered = {record.function_name for record in records} for name, (start, end) in sorted(bounds.items()): @@ -425,6 +439,9 @@ def _route_manifest_entry_points(root: Path, targets: Sequence[FileTarget]) -> l conditions=[], provenance=f"entrypoint:route:{access}", rationale=f"registered by operationId {operation} in {relative}", + # A route manifest is a complete contract: an operation with no `security` block is + # declared open, not merely undecorated. + access_declared=True, ) ) return records @@ -679,6 +696,7 @@ def _entry( trust_boundary: str, access_evidence: list[str], conditions: list[str], + access_declared: bool = False, ) -> EntryPointRecord: return EntryPointRecord( path=target.path, @@ -693,6 +711,7 @@ def _entry( conditions=conditions, provenance=f"entrypoint:{kind}:{access_level}", rationale=f"{kind} surface classified as {access_level} at {trust_boundary}", + access_declared=access_declared, ) diff --git a/src/openultrasast/model/regions.py b/src/openultrasast/model/regions.py index d0abfcc..04bddc3 100644 --- a/src/openultrasast/model/regions.py +++ b/src/openultrasast/model/regions.py @@ -86,7 +86,13 @@ def regions_for(entries: Sequence[object], targets: Sequence[object], *, shipped language = by_path.get(path) if language is None: continue - families = _families(language, has_handler=True) + # A handler its own contract declares OPEN carries no authorization obligation: there is nothing + # for a guard to be missing from. The declaration is what makes this sound -- an OpenAPI operation + # with no `security` block, a `wp_ajax_nopriv_` hook, a REST `permission_callback => __return_true`. + # An INFERRED "public" is the opposite case and must keep the family, because for a decorator + # framework it means only that no `@login_required` was found, which is the bug itself. + declared_open = bool(getattr(entry, "access_declared", False)) and str(getattr(entry, "access_level", "")) == "public" + families = _families(language, has_handler=not declared_open) if not families: continue function = getattr(entry, "function_name", None) or None diff --git a/tests/test_model_regions.py b/tests/test_model_regions.py index 856aa9b..ddfe0c5 100644 --- a/tests/test_model_regions.py +++ b/tests/test_model_regions.py @@ -9,7 +9,7 @@ language actually admit. from __future__ import annotations -def _entry(path="api/users.py", function="update_password", access="public", boundary="network"): # type: ignore[no-untyped-def] +def _entry(path="api/users.py", function="update_password", access="public", boundary="network", declared=False): # type: ignore[no-untyped-def] from openultrasast.mapping import EntryPointRecord return EntryPointRecord( @@ -25,6 +25,7 @@ def _entry(path="api/users.py", function="update_password", access="public", bou conditions=[], provenance="decorator", rationale="", + access_declared=declared, ) @@ -238,3 +239,36 @@ def test_what_the_project_does_not_ship_ranks_below_what_it_does() -> None: ordered = [(r.path, r.shipped) for r in regions] assert ordered[0] == ("lib.py", True), "shipped code comes first whatever the rank says" assert ("example.py", False) in ordered, "not shipped is not unscanned" + + +def test_a_declared_open_endpoint_carries_no_authorization_obligation() -> None: + """contributor-scan 2.8. VAmPI's OpenAPI spec declares register_user and login_user open, and the arbiter + entailed a missing guard on both -- reporting an absence on endpoints whose contract says there is + nothing to miss.""" + from openultrasast.model.regions import regions_for + + declared_open = _entry(path="api.py", function="register_user", access="public", declared=True) + assert declared_open.access_declared, "the fixture must actually carry the declaration it is testing" + + regions = regions_for([declared_open], [_target(path="api.py")]) + handler = next(r for r in regions if r.function == "register_user") + + assert "access_control" not in handler.families + assert "injection" in handler.families, "only the obligation goes; the flow families stay" + + +def test_an_inferred_public_endpoint_keeps_the_obligation() -> None: + """The case this rule must not break, and the reason it keys on provenance rather than on the level. + + For a decorator framework "public" means only that no `@login_required` was found -- which is precisely + the bug the access-control family exists to report. Suppressing on the value alone would silence the + detector on its primary case. + """ + from openultrasast.model.regions import regions_for + + inferred = _entry(path="app.py", function="handler", access="public") # access_declared defaults False + + regions = regions_for([inferred], [_target(path="app.py")]) + handler = next(r for r in regions if r.function == "handler") + + assert "access_control" in handler.families