test: close auth-route regression gaps for previously-open routes (PER-15250) - #326
Conversation
…R-15250) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🔍 Vulnerabilities of
|
| digest | sha256:88df417d3920ee96f2b2032fb8ede5d39481756bfa71386f5e712d5f8ea8aea8 |
| vulnerabilities | |
| platform | linux/amd64 |
| size | 133 MB |
| packages | 247 |
📦 Base Image python:3.13-alpine3.23
| also known as |
|
| digest | sha256:0306b86d5dbbf72135e5e0fcd630005f339b0050b2a2aa5a3946567b14fe0efe |
| vulnerabilities |
Description
Description
Description
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
|
There was a problem hiding this comment.
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-
Authorizationheader coverage to ensure gated routes return401(and never500), including legacy update aliases and all protected enforcer endpoints. - Tightens legacy-route missing-header assertions from a transitional
(401|422)allowance to strict401+ 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.
…eviously-open-routes-missing-bad
…eviously-open-routes-missing-bad
) 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
left a comment
There was a problem hiding this comment.
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
|
|
||
|
|
||
| @pytest.mark.parametrize("endpoint", PROTECTED_ENFORCER_ENDPOINTS) | ||
| @pytest.mark.parametrize("value", ["garbage", "Bearer", "Bearer ", "Bearer a b c"]) |
There was a problem hiding this comment.
[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}",
],
)
There was a problem hiding this comment.
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 eitherI 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"]) |
There was a problem hiding this comment.
[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)
There was a problem hiding this comment.
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.
…eviously-open-routes-missing-bad
…eviously-open-routes-missing-bad
…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>
| # 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 | ||
|
|
There was a problem hiding this comment.
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.
| # 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 = [ |
There was a problem hiding this comment.
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.
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._opaland assert an actual200 {"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 9PROTECTED_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 thatenforce_pdp_tokendefaults its credentials param; removed the stale merge-order comments.Issue-case → test map
test_trigger_route_without_token_is_401,test_trigger_route_with_wrong_token_is_401test_{policy,data}_updater_trigger_route_with_valid_token_returns_200test_update_policy*(missing-token check tightened here)/kong401 (integration off & on), 200 w/ token+OPA, 503 orderingtest_kong_endpoint_*,test_enforcer_endpoint_{missing,invalid}_token_returns_401[/kong]/healthstays publictest_health_endpoint_is_public,test_health_is_public/allowedspot-checktest_enforce_endpoint[/allowed]test_*_malformed_header_is_401_not_500(three files)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 fourthMockPermitPDPscaffold. The issue'senforce_pdp_tokensplit(" ")→ValueErrorpremise and itsmock_opafixture reference are also stale (superseded byHTTPBearer(auto_error=False)and theaioresponsespattern).Verification
python -m pytest horizon/tests/ -q→ 162 passed, 0 failedruff format --checkandruff check→ clean🤖 Generated with Claude Code