Skip to content

test: close auth-route regression gaps for previously-open routes (PER-15250) - #326

Merged
dshoen619 merged 7 commits into
mainfrom
david/per-15250-pdp-integration-tests-for-previously-open-routes-missing-bad
Aug 25, 2026
Merged

test: close auth-route regression gaps for previously-open routes (PER-15250)#326
dshoen619 merged 7 commits into
mainfrom
david/per-15250-pdp-integration-tests-for-previously-open-routes-missing-bad

Conversation

@dshoen619

Copy link
Copy Markdown
Contributor

Closes PER-15250.

What & why

PER-15250 asked for integration tests covering the routes that were gated by the auth-hardening work (PER-15244/15245/15246). Those PRs (#317/#320/#321) shipped most of the requested coverage alongside the fixes; this PR closes the two remaining gaps and tightens now-obsolete assertions. No production code changes — tests only.

Changes

1. Trigger routes now assert a real 200, not just != 401 (test_opal_trigger_auth.py)
The valid-token test previously only asserted the request wasn't blocked. Replaced it with two tests that boundary-mock the real (unstarted) updater instances on _sidecar._opal and assert an actual 200 {"status": "ok"} with the correct awaited kwargs (force_full_update=True / data_fetch_reason="request from sdk"). Docstring updated to match.

2. Malformed-Authorization-header coverage on every gated route (test_legacy_update_routes.py, test_enforcer_api.py)
Parametrized ["garbage", "Bearer", "Bearer ", "Bearer a b c"]401, never 500 for the legacy aliases (asserting the updater is never awaited) and across all 9 PROTECTED_ENFORCER_ENDPOINTS (covers /kong, /allowed, etc.). Pins the header-safe handling from PER-15245.

3. Tightened obsolete dual-regime assertions (test_legacy_update_routes.py)
assert status in (401, 422)== 401 (+ detail) now that enforce_pdp_token defaults its credentials param; removed the stale merge-order comments.

Issue-case → test map

Issue case Covering test
Trigger routes no/wrong token → 401 test_trigger_route_without_token_is_401, test_trigger_route_with_wrong_token_is_401
Trigger routes valid token → 200 (mocked updater) new: test_{policy,data}_updater_trigger_route_with_valid_token_returns_200
Legacy aliases 401 / 200 + updater called / no redirect test_update_policy* (missing-token check tightened here)
/kong 401 (integration off & on), 200 w/ token+OPA, 503 ordering test_kong_endpoint_*, test_enforcer_endpoint_{missing,invalid}_token_returns_401[/kong]
/health stays public test_health_endpoint_is_public, test_health_is_public
/allowed spot-check test_enforce_endpoint[/allowed]
Malformed header → 401 not 500, per gated route new: test_*_malformed_header_is_401_not_500 (three files)
Whole-app structural guarantee test_route_auth_audit.py (pre-existing)

Deliberate deviation from the issue text

The issue (written before its blockers merged) specified a new file test_unauthenticated_routes_regression.py. Those blockers created purpose-built test files that already carry the exact fixtures these cases need, so the new cases are appended next to the routes they gate rather than duplicating a fourth MockPermitPDP scaffold. The issue's enforce_pdp_token split(" ")ValueError premise and its mock_opa fixture reference are also stale (superseded by HTTPBearer(auto_error=False) and the aioresponses pattern).

Verification

  • python -m pytest horizon/tests/ -q162 passed, 0 failed
  • ruff format --check and ruff check → clean
  • New negative-path tests confirmed non-vacuous: all 46 malformed-header parametrizations flip off 401 when the auth gate is neutered.

🤖 Generated with Claude Code

…R-15250)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@linear-code

linear-code Bot commented Jul 13, 2026

Copy link
Copy Markdown

PER-15250

@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown

🔍 Vulnerabilities of permitio/pdp-v2:next

📦 Image Reference permitio/pdp-v2:next
digestsha256:88df417d3920ee96f2b2032fb8ede5d39481756bfa71386f5e712d5f8ea8aea8
vulnerabilitiescritical: 0 high: 3 medium: 3 low: 1 unspecified: 1
platformlinux/amd64
size133 MB
packages247
📦 Base Image python:3.13-alpine3.23
also known as
  • 3.13.15-alpine3.23
  • 3595881807616fb0ca649f5a5d1b280cecbb93891e92539c4ccae1282ab84293
digestsha256:0306b86d5dbbf72135e5e0fcd630005f339b0050b2a2aa5a3946567b14fe0efe
vulnerabilitiescritical: 0 high: 5 medium: 2 low: 0
critical: 0 high: 2 medium: 2 low: 1 starlette 0.50.0 (pypi)

pkg:pypi/starlette@0.50.0

high 7.5: CVE--2026--54283 Allocation of Resources Without Limits or Throttling

Affected range>=0.4.1
<1.3.1
Fixed version1.3.1
CVSS Score7.5
CVSS VectorCVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H
EPSS Score0.481%
EPSS Percentile39th percentile
Description

Summary

request.form() accepts max_fields and max_part_size to bound resource consumption while parsing form data. These limits are enforced for multipart/form-data, but silently ignored for application/x-www-form-urlencoded. An unauthenticated attacker can therefore send a urlencoded body with an arbitrarily large number of fields or an arbitrarily large field, even when the application configured limits it believed would apply.

Details

request.form() dispatches to a different parser depending on the Content-Type. For multipart/form-data the max_files, max_fields, and max_part_size limits are forwarded to the parser, but for application/x-www-form-urlencoded the parser is constructed without them. It has no max_fields or max_part_size parameter to receive them, and it appends every field with no count check and accumulates each field's name and value with no size check. The configured limits are therefore both unreachable and unenforced for url-encoded bodies.

Because the url-encoded parser does its work synchronously between stream reads, the two attack shapes have different effects:

  • Field count drives CPU and event-loop blocking. A body of ~1,000,000 fields (a sub-10MB payload such as f0=v&f1=v&...) blocks the worker's event loop for several seconds while parsing, during which the worker serves no other request.
  • Field size drives memory. A single large field value (e.g. a 50MB value) is buffered in full to build the FormData, forcing memory allocation proportional to the request body.

The equivalent multipart/form-data request is correctly rejected with 400 Too many fields / 400 Field exceeded maximum size.

Impact

This Denial of service (DoS) vulnerability affects all applications built with Starlette (or FastAPI) that call request.form() on application/x-www-form-urlencoded requests. A single request with a very large number of fields blocks the event loop for several seconds, and a single request with a very large field forces unbounded memory allocation; in either case, parallel requests can render the service unusable. A reverse proxy that enforces a request body size limit reduces but does not eliminate the exposure, since a sub-10MB body is already enough to block the event loop.

Mitigation

Upgrade to a patched version, which forwards max_fields and max_part_size to the url-encoded parser and enforces them while parsing, raising before the oversized field or excess fields are accumulated. The defaults match multipart/form-data (max_fields=1000, max_part_size=1MB) and can be customized via request.form(max_fields=..., max_part_size=...).

high 7.5: CVE--2026--48818 Server-Side Request Forgery (SSRF)

Affected range<1.1.0
Fixed version1.1.0
CVSS Score7.5
CVSS VectorCVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:N/A:N
EPSS Score0.368%
EPSS Percentile29th percentile
Description

Summary

When serving static files on Windows, StaticFiles resolves the requested path with os.path.realpath. If a UNC path (such as \\attacker.com\share) reaches the resolver, realpath causes the process to open a connection to the remote host over SMB (port 445). This is a server-side request forgery (SSRF) that leaks the service account's NTLMv2 credentials to the attacker-controlled host, which can then be cracked offline or relayed to other hosts.

Details

StaticFiles.lookup_path() joins the requested path onto the served directory and calls os.path.realpath on the result before checking containment with os.path.commonpath. On Windows, a UNC path is absolute, so os.path.join discards the served directory and realpath resolves the bare UNC path, triggering the outbound SMB connection and NTLM authentication before the containment check rejects the path. The HTTP response is a benign 404, but the credential disclosure has already happened. POSIX systems are not affected.

This only affects the default configuration (follow_symlink=False), which uses os.path.realpath. The follow_symlink=True branch uses os.path.abspath, which performs no I/O.

Impact

Applications running on Windows that serve files with StaticFiles (directly, or via a framework built on Starlette such as FastAPI) in the default configuration are affected. StaticFiles is typically unauthenticated, so any client can trigger the SMB connection and leak the service account's NTLMv2 hash. A secondary impact is discovering internal hosts reachable over SMB by timing responses for valid versus invalid addresses.

Mitigation

Applications not running on Windows are not affected. On Windows, serving static files through a dedicated web server (such as nginx or IIS) instead of StaticFiles avoids the issue. Blocking outbound SMB (port 445) from the application host prevents the credential disclosure even if a UNC path is resolved.

medium 6.5: CVE--2026--48710 Improper Validation of Unsafe Equivalence in Input

Affected range<=1.0.0
Fixed version1.0.1
CVSS Score6.5
CVSS VectorCVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:L/I:L/A:N
EPSS Score1.905%
EPSS Percentile78th percentile
Description

Summary

In affected versions, the HTTP Host request header was not validated before being used to reconstruct request.url. Because the routing algorithm relies on the raw HTTP path while request.url is rebuilt from the Host header, a malformed header could make request.url.path differ from the path that was actually requested. Middleware and endpoints that apply security restrictions based on request.url (rather than the raw scope path) could therefore be bypassed.

Details

When a client requests http://example.com/foo, it sends:

GET /foo HTTP/1.1
Host: example.com

Affected versions reconstructed the URL by concatenating http://{host}{path} and re-parsing the result. The Host value is only valid as a uri-host [ ":" port ] per RFC 9112 §3.2, where uri-host follows the restricted host grammar of RFC 3986 §3.2.2. When it contains characters outside that grammar - notably /, ?, or # - those characters move the path/query/fragment boundaries during re-parsing, so the parsed request.url.path no longer matches the path the server actually received. For example:

GET /foo HTTP/1.1
Host: example.com/abc?bar=

reconstructs to http://example.com/abc?bar=/foo, whose parsed path is /abc - even though routing used the real path /foo. The router still dispatches to /foo and the endpoint executes, but any middleware or code that reads request.url.path sees /abc, so path-based authorization checks can be bypassed.

Impact

Any application running an affected version that relies on request.url (or request.url.path) for security-sensitive decisions is affected. The most common case is middleware that gates access to certain path prefixes based on request.url.path. Deployments fronted by a proxy or load balancer are mitigated only if that proxy rejects or normalizes the malformed Host header before forwarding and the application does not trust attacker-controlled host headers (e.g. X-Forwarded-Host) elsewhere.

Mitigation

Upgrade to a patched version, which validates the Host header against the grammar of RFC 9112 §3.2 / RFC 3986 §3.2.2 when constructing request.url and falls back to scope["server"] for malformed values.

medium 5.3: CVE--2026--48817 Use of Externally-Controlled Input to Select Classes or Code ('Unsafe Reflection')

Affected range<1.1.0
Fixed version1.1.0
CVSS Score5.3
CVSS VectorCVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:L/A:N
EPSS Score0.213%
EPSS Percentile11th percentile
Description

Summary

When dispatching a request, HTTPEndpoint selects the handler by lowercasing the HTTP method and looking it up as an attribute with getattr, without restricting the lookup to a known set of HTTP verbs.

When an HTTPEndpoint subclass is registered through Route(...) without an explicit methods= argument, the route does not constrain the method and every method reaches the endpoint. If a non-standard HTTP method whose lowercased name matches an attribute on the endpoint subclass reaches the endpoint, that attribute is invoked as if it were a request handler. An attacker can use this to reach methods that were never meant to be HTTP handlers, such as internal helpers, without the authorization checks applied by the intended public handler.

Details

HTTPEndpoint uses the client-supplied method name to resolve an instance attribute, without validating it against the set of HTTP verbs the endpoint supports. A method such as _DO_DELETE therefore resolves an attribute like _do_delete and invokes it. Non-standard methods are valid RFC 9110 token methods, so an endpoint must not treat the method name as a trusted attribute selector.

Impact

An application is affected when all of the following hold:

  • It defines an HTTPEndpoint subclass and registers it via Route(...) without an explicit methods= argument.
  • The subclass defines additional methods whose names match a non-standard HTTP-method token shape and that accept a single request argument and return a response.

This also affects frameworks built on Starlette, like FastAPI.

Mitigation

Register HTTPEndpoint subclasses with an explicit methods= argument on the Route, listing only the HTTP verbs the endpoint supports. The route then rejects any other method with 405 Method Not Allowed before it reaches the endpoint, so non-standard methods cannot resolve an attribute.

low 3.7: CVE--2026--54282 Improper Input Validation

Affected range<1.3.0
Fixed version1.3.0
CVSS Score3.7
CVSS VectorCVSS:3.1/AV:N/AC:H/PR:N/UI:N/S:U/C:N/I:L/A:N
EPSS Score0.273%
EPSS Percentile19th percentile
Description

Summary

In affected versions, the HTTP request path is not validated before being used to reconstruct request.url. Because request.url is rebuilt by concatenating {scheme}://{host}{path} and re-parsing the result, a path that does not begin with / (for example @<!-- -->google.com) moves the authority boundary during re-parsing, so request.url.hostname and request.url.netloc become attacker-controlled. Code that reads request.url.hostname (rather than the Host header or scope) can therefore be misled into trusting an attacker-supplied host.

Details

When a client requests a path that does not start with /:

GET @<!-- -->google.com HTTP/1.1
Host: localhost

affected versions reconstruct the URL as http://localhost@<!-- -->google.com. Per RFC 3986 §3.2.1, the substring before @ in the authority is userinfo, so re-parsing yields username = "localhost" and hostname = "google.com", with an empty path:

request.url          == "http://localhost@<!-- -->google.com"
request.url.hostname == "google.com"
request.url.path     == ""

The root cause is that the path is concatenated directly after the host without a separating /, and without validating that it begins with one. Only the Host header was validated when constructing request.url; the path was not.

This requires an ASGI server that forwards a request-target lacking a leading / into scope["path"].

Impact

Any application running an affected version that uses request.url, request.url.netloc, or request.url.hostname for a security-sensitive decision (host-based authorization, redirect/callback base, SSRF target, cache key, audit log) may be affected, when no fronting proxy or load balancer rejects the malformed request-target first.

Note that this is less exploitable than GHSA-86qp-5c8j-p5mr: there, the poison is carried in the Host header, so the real path still routes to a valid endpoint while request.url.path lies. Here, the poison must be carried in the path itself, and that path (@<!-- -->google.com) does not match any registered route, so routing returns 404 and no endpoint handler runs. The exposure is limited to code that reads request.url before routing - notably middleware - or in 404/exception handlers.

Mitigation

Upgrade to a patched version, which prevents the request path from crossing into the URL authority. The request above instead yields http://localhost/@<!-- -->google.com with request.url.hostname == "localhost".

critical: 0 high: 1 medium: 0 low: 0 ddtrace 3.19.8 (pypi)

pkg:pypi/ddtrace@3.19.8

high 7.5: CVE--2026--50271 Uncontrolled Resource Consumption

Affected range<4.8.2
Fixed version4.8.2
CVSS Score7.5
CVSS VectorCVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H
EPSS Score0.793%
EPSS Percentile54th percentile
Description

Impact

Datadog tracing libraries that implement W3C baggage propagation parse incoming baggage HTTP headers without enforcing item-count or byte-size limits on the extract path. The DD_TRACE_BAGGAGE_MAX_ITEMS (default 64) and DD_TRACE_BAGGAGE_MAX_BYTES (default 8192) limits were applied only to baggage injection, not extraction. A remote, unauthenticated attacker can send a request whose baggage header contains an arbitrarily large number of comma-separated key-value pairs (or a single very large value). The tracer allocates a hash-map entry for each pair on every request, causing unbounded CPU and memory consumption and enabling a remote Denial of Service against any HTTP service that has the baggage propagation style enabled.
The baggage propagation style is enabled by default in most affected tracers, so any internet-facing service that has been instrumented with an affected tracer version is exposed unless the propagation style has been explicitly narrowed.

Patches

This is resolved in version 4.8.2 and later of the dd-trace-py library

Workarounds

If users cannot upgrade immediately:

  1. Disable baggage extraction by removing baggage from DD_TRACE_PROPAGATION_STYLE (or DD_TRACE_PROPAGATION_STYLE_EXTRACT if set independently).
  2. Cap the maximum HTTP request header size at an upstream proxy or web server (for example, Apache LimitRequestFieldSize, Nginx large_client_header_buffers, Envoy max_request_headers_kb).

Resources

Related upstream advisories:
opentelemetry-go GHSA-mh2q-q3fh-2475
opentelemetry-dotnet GHSA-g94r-2vxg-569j

critical: 0 high: 0 medium: 1 low: 0 busybox 1.37.0-r30 (apk)

pkg:apk/alpine/busybox@1.37.0-r30?os_name=alpine&os_version=3.23

medium : CVE--2025--60876

Affected range<=1.37.0-r30
Fixed versionNot Fixed
EPSS Score0.291%
EPSS Percentile21st percentile
Description
critical: 0 high: 0 medium: 0 low: 0 unspecified: 1golang.org/x/crypto 0.53.0 (golang)

pkg:golang/golang.org/x/crypto@0.53.0

unspecified : GO--2026--5932

Affected range>=0
Fixed versionNot Fixed
Description

The golang.org/x/crypto/openpgp package is unsafe by design, has numerous known security issues, is not maintained, and should not be used.

If you are required to interoperate with OpenPGP systems and need a maintained package, consider github.com/ProtonMail/go-crypto/openpgp which is a maintained fork that aims to be a drop-in replacement for this package.

@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown

🔍 Vulnerabilities of permitio/pdp-v2:next

📦 Image Reference permitio/pdp-v2:next
digestsha256:88df417d3920ee96f2b2032fb8ede5d39481756bfa71386f5e712d5f8ea8aea8
vulnerabilitiescritical: 0 high: 0 medium: 0 low: 0
platformlinux/amd64
size133 MB
packages247
📦 Base Image python:3.13-alpine3.23
also known as
  • 3.13.15-alpine3.23
  • 3595881807616fb0ca649f5a5d1b280cecbb93891e92539c4ccae1282ab84293
digestsha256:0306b86d5dbbf72135e5e0fcd630005f339b0050b2a2aa5a3946567b14fe0efe
vulnerabilitiescritical: 0 high: 5 medium: 2 low: 0

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds missing integration-test coverage for previously open/auth-regressed routes in the Horizon PDP test suite, completing PER-15250 without changing production code.

Changes:

  • Strengthens OPAL trigger-route “valid token” tests to assert real 200 {"status":"ok"} responses by boundary-mocking OPAL updaters, instead of only asserting “not 401”.
  • Adds parameterized malformed-Authorization header coverage to ensure gated routes return 401 (and never 500), including legacy update aliases and all protected enforcer endpoints.
  • Tightens legacy-route missing-header assertions from a transitional (401|422) allowance to strict 401 + expected detail message.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
horizon/tests/test_opal_trigger_auth.py Replaces the “!= 401” valid-token assertion with deterministic 200 tests by AsyncMock’ing OPAL updater calls.
horizon/tests/test_legacy_update_routes.py Tightens missing-header checks to 401 and adds malformed-header param tests ensuring updater methods are not awaited.
horizon/tests/test_enforcer_api.py Adds malformed-header param test across all protected enforcer endpoints to pin 401 behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

dshoen619 added a commit that referenced this pull request Aug 11, 2026
)

Docker Scout began flagging two HIGH CVEs in cryptography 48.0.1 on
2026-08-04, turning docker-scout red on every open PR (#326, #327) and on
main. Neither is caused by any code change - the pin has been
cryptography>=48.0.1,<49 since #318.

  CVE-2026-69249  CVSS 8.7  fixed in 49.0.0
  CVE-2026-69247  CVSS 8.2  affects >=44.0.0, fixed only in 50.0.0
                            (Observable Timing Discrepancy)

Clearing both requires 50.0.0, so the floor moves past our own <49 major
cap. Nothing external bounds cryptography: opal-common 0.9.6 requires it
unpinned and its pyjwt[crypto]<3,>=2.4.0 carries no upper bound, so the
new <51 cap is ours - it keeps a major out of an image build that has no
lockfile, same reasoning as the websockets pin.

musllinux_1_2 cp311-abi3 wheels are published for x86_64 and aarch64, so
the alpine image keeps installing a prebuilt wheel and still needs no
Rust toolchain.

No VEX changes: both CVEs are fixable by upgrade, so neither needs a
waiver in .docker/scout/pdp-v2.vex.json.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

@zeevmoney zeevmoney left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved — no CRITICAL or HIGH issues found.

Non-blocking:

  • LOW horizon/tests/test_enforcer_api.py:101 — Malformed-header set never uses a non-bearer scheme, so the scheme check stays untested
  • LOW horizon/tests/test_legacy_update_routes.py:50 — Malformed-header value list now copy-pasted verbatim across three test files

Details are in the inline comments on each line.

Outside this diff (not blocking this PR, listed for awareness — these lines are
not changed here, so they could not be posted as inline comments):

  • LOW horizon/enforcer/api.py:70 — Second auth parser echoes the raw Authorization header into the 401 body

Comment thread horizon/tests/test_enforcer_api.py Outdated


@pytest.mark.parametrize("endpoint", PROTECTED_ENFORCER_ENDPOINTS)
@pytest.mark.parametrize("value", ["garbage", "Bearer", "Bearer ", "Bearer a b c"])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] Malformed-header set never uses a non-bearer scheme, so the scheme check stays untested

Problem: The four parametrized values only exercise two states of the header parser: an empty credential ("garbage", "Bearer", "Bearer " all partition to an empty param) and a well-formed bearer with a wrong token ("Bearer a b c" partitions to scheme="Bearer", credentials="a b c", which is the same path test_enforcer_endpoint_invalid_token_returns_401 already covers). The one branch that is genuinely load-bearing and genuinely untested is a non-bearer scheme carrying a non-empty credential — e.g. Authorization: Basic mock_api_key. horizon/authentication.py:37-38 documents that exact case as behaviour the design depends on ("auto_error=False makes it return None ... for a missing, malformed, or non-bearer header"), and horizon/tests/test_authentication.py:4-5 explicitly delegates it away: "Header parsing itself is now delegated to fastapi.security.HTTPBearer (and exercised end-to-end with real header strings in test_opal_trigger_auth.py)". Neither file actually covers it. Mutation check: neutering HTTPBearer's scheme comparison so any scheme is accepted leaves the entire 162-test suite green, while making Basic <api-key> and Negotiate <api-key> authenticate against every gated route. The PR's whole stated purpose is pinning malformed-Authorization handling, so this is the variant most worth having.

Suggestion: Add a non-bearer scheme carrying a syntactically valid credential to the shared value list, at all four parametrize sites (test_enforcer_api.py:101, test_legacy_update_routes.py:50 and :96, test_opal_trigger_auth.py:93). Using the real API key as the credential is the strongest form, because it fails only if the scheme check itself is gone.

Example:

@pytest.mark.parametrize(
    "value",
    [
        "garbage",
        "Bearer",
        "Bearer ",
        "Bearer a b c",
        f"Basic {sidecar_config.API_KEY}",   # right secret, wrong scheme -> must still 401
        f"basic {sidecar_config.API_KEY}",
    ],
)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 93c3866.

Added two non-bearer entries carrying the real API key, as suggested:

f"Basic {sidecar_config.API_KEY}",  # right secret, wrong scheme -> must still 401
f"basic {sidecar_config.API_KEY}",  # ... and lowercasing the scheme must not help either

I ran your mutation check to confirm they actually have teeth. Neutering HTTPBearer's scheme comparison so any scheme with a non-empty credential is accepted:

  • 227 pre-existing tests: all still green — exactly the blind spot you described
  • 26 new cases: all red, across all four sites (9 enforcer endpoints, both legacy aliases, both trigger routes)

A clean 1:1, so the added coverage fails on the scheme check and nothing else.

Suite is now 253 passed (was 227).

Your third finding (horizon/enforcer/api.py:70 echoing the raw Authorization header into the 401 body) is untouched here, as you flagged — it wants its own ticket.

trigger.assert_not_awaited()


@pytest.mark.parametrize("value", ["garbage", "Bearer", "Bearer ", "Bearer a b c"])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] Malformed-header value list now copy-pasted verbatim across three test files

Problem: ["garbage", "Bearer", "Bearer ", "Bearer a b c"] now appears identically at four parametrize sites in three files. This PR added three of the four, crossing the "same code written three times" threshold. Concretely, this is what makes finding 1 a four-place edit instead of a one-place edit: any future addition to the malformed set (a non-bearer scheme, a Bearer\ttoken, a non-ASCII credential) has to be applied four times or the coverage silently diverges between routers. A shared constant is already trivially reachable — test_legacy_update_routes.py:10 demonstrates the suite's cross-module import convention (from test_enforcer_api import MockPermitPDP, basename import because CI installs the package non-editably).

Suggestion: Hoist the list to a single module-level constant (e.g. MALFORMED_AUTH_HEADERS in test_enforcer_api.py next to PROTECTED_ENFORCER_ENDPOINTS, or in horizon/tests/conftest.py) and import it at the other three sites, matching the existing basename-import convention.

Example:

# test_enforcer_api.py, next to PROTECTED_ENFORCER_ENDPOINTS
MALFORMED_AUTH_HEADERS = ["garbage", "Bearer", "Bearer ", "Bearer a b c"]

# test_legacy_update_routes.py / test_opal_trigger_auth.py
from test_enforcer_api import MalformedAuthHeaders  # noqa: same basename convention as MockPermitPDP

@pytest.mark.parametrize("value", MALFORMED_AUTH_HEADERS)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 93c3866.

Hoisted to MALFORMED_AUTH_HEADERS in test_enforcer_api.py beside PROTECTED_ENFORCER_ENDPOINTS, imported by basename at the other three sites (test_legacy_update_routes.py x2, test_opal_trigger_auth.py) — matching the MockPermitPDP convention documented at test_legacy_update_routes.py:7-9.

You were right that the ordering mattered: doing this first made the non-bearer variants for your other finding a one-place edit instead of four.

dshoen619 and others added 4 commits August 23, 2026 16:23
…PER-15250)

Merging main brought in #327 (PER-15248), which routes /policy-updater/trigger
and /data-updater/trigger through DebouncedTrigger and returns a TriggerResponse
of {"status": "ok", "triggered": <bool>} instead of the old bare {"status": "ok"}.
Both changes merged cleanly on text, but the two valid-token tests here still
asserted the pre-debounce body and failed:

    AssertionError: assert {'status': 'ok', 'triggered': True} == {'status': 'ok'}

Assert the full current body, matching test_legacy_update_routes.py. `triggered`
is True in both cases because the autouse _reset_trigger_debouncers fixture gives
each test a fresh DebouncedTrigger, so these are always first-dispatch calls.

pytest -s --cache-clear horizon/tests/ -> 227 passed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ix (PER-15250)

Addresses both non-blocking LOW findings from Zeev's review on #326.

1. Non-bearer scheme was untested (test_enforcer_api.py:101). The four existing
   values only exercised two parser states: an empty credential ("garbage",
   "Bearer", "Bearer ") and a well-formed bearer with a wrong token
   ("Bearer a b c"), the latter already covered by
   test_enforcer_endpoint_invalid_token_returns_401. The load-bearing branch -
   a non-bearer scheme carrying a VALID credential - was covered nowhere;
   horizon/authentication.py:37-38 documents it as behaviour the design relies
   on, and test_authentication.py explicitly delegates it away.

   Added `Basic <real API key>` and `basic <real API key>`. Using the real
   secret is what gives them teeth: they can only 401 because HTTPBearer
   rejects the scheme.

   Mutation check, per the review: neutering HTTPBearer's scheme comparison so
   any scheme is accepted leaves the pre-existing 227 tests fully green, while
   exactly the 26 new cases go red - across all four sites (enforcer, both
   legacy aliases, both trigger routes). Before this commit that mutation was
   invisible and `Basic <api-key>` authenticated against every gated route.

2. Matrix was copy-pasted at four sites in three files
   (test_legacy_update_routes.py:50). Hoisted to MALFORMED_AUTH_HEADERS in
   test_enforcer_api.py beside PROTECTED_ENFORCER_ENDPOINTS, imported by
   basename at the other three - the convention already used for MockPermitPDP
   and documented at test_legacy_update_routes.py:7-9. This is what kept
   finding 1 a one-place edit.

pytest -s --cache-clear horizon/tests/ -> 253 passed (was 227).

Zeev's third finding (horizon/enforcer/api.py:70 echoing the raw Authorization
header into the 401 body) is outside this diff and left for a follow-up, as
flagged in the review.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 25, 2026 09:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comment on lines +30 to +35
# Basename import (not horizon.tests.*): CI installs the package non-editably, so the
# wheel ships no tests/ package; pytest's prepend import mode puts this directory on
# sys.path and imports test modules by basename. Same convention as
# test_legacy_update_routes.py.
from test_enforcer_api import MALFORMED_AUTH_HEADERS

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged, but leaving as-is for this PR.

The coupling is this suite's existing convention rather than something new: test_legacy_update_routes.py:10 already imports MockPermitPDP from test_enforcer_api, with the basename rationale documented at :7-9. This change follows that pattern instead of introducing a second one.

On cost: CI runs pytest horizon/tests/, which imports every module regardless, so there is no added cost in the run that gates this PR. The only affected case is running this file alone, where the marginal cost of the extra module-level MockPermitPDP measures ~33ms locally.

One caveat worth recording for whoever picks this up later: the obvious version of this fix — moving the constant to conftest.py — would silently defang the tests. The two non-bearer entries interpolate sidecar_config.API_KEY, which is only "mock_api_key" because MockPermitPDP.__init__ sets it. conftest.py is imported before any test module, when API_KEY is still MOCK_API_KEY ("MUST BE DEFINED"), so the entries would carry a wrong credential and 401 on the credential rather than the scheme — passing under the very mutation they exist to catch.

So the factory form in your suggestion is the load-bearing part, not the module move. Worth doing if this constant gains more consumers; out of scope here.

Comment on lines +64 to +73
# The malformed-Authorization matrix, shared by every gated router's parametrized 401 test
# (this file, test_legacy_update_routes.py x2, test_opal_trigger_auth.py). Kept in one place
# so a new variant lands once instead of four times and cannot silently diverge between
# routers; the other modules pull it in by basename, same convention as MockPermitPDP.
#
# The two non-bearer entries carry the REAL API key, so they 401 *only* because HTTPBearer
# rejects the scheme (horizon/authentication.py:37-38). Neuter that scheme comparison and
# these are the entries that go red - the rest still 401 on an empty/wrong credential.
# Interpolated below `sidecar = MockPermitPDP()`, which is what sets sidecar_config.API_KEY.
MALFORMED_AUTH_HEADERS = [

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same answer as on the test_opal_trigger_auth.py thread: leaving as-is for this PR.

The placement follows the human review on this PR, which suggested test_enforcer_api.py next to PROTECTED_ENFORCER_ENDPOINTS. The import-time MockPermitPDP costs ~33ms marginally and nothing at all under pytest horizon/tests/, which is what CI runs.

Noting for the record that the factory form in your suggestion is the load-bearing part, not the module move: a plain constant in a helper or conftest.py module would be evaluated before sidecar_config.API_KEY is set, leaving the non-bearer entries carrying "MUST BE DEFINED" instead of the real key. They would then 401 on the credential rather than the scheme, and pass under the mutation they exist to catch.

@dshoen619
dshoen619 merged commit 7770ea4 into main Aug 25, 2026
10 checks passed
@dshoen619
dshoen619 deleted the david/per-15250-pdp-integration-tests-for-previously-open-routes-missing-bad branch August 25, 2026 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants