fix(deps): support Starlette 1.x to address CVE-2026-48710 (#956) by EliMoshkovich · Pull Request #959 · permitio/opal

EliMoshkovich · 2026-09-14T14:14:05Z

Fixes Issue

The CVE

CVE-2026-48710 / GHSA-86qp-5c8j-p5mr — medium, CVSS 6.5, published 2026-06-04.

Starlette did not validate the HTTP Host header before rebuilding request.url. Routing uses the raw ASGI path, but request.url was reconstructed as http://{host}{path} and re-parsed, so a malformed Host could make request.url.path differ from the path actually routed. Middleware or endpoints that enforce security on request.url rather than the raw scope path can therefore be bypassed. Affected <= 1.0.0; first patched 1.0.1.

packages/requires.txt pinned starlette>=0.40.0,<1, so no patched version was reachable. That file is consumed by all three packages and installed directly in both Docker build stages, opal-server included.

Exposure: none

OPAL was never exploitable through this CVE. The vulnerable pattern is enforcing security on request.url rather than on the routed path, and there is no use of request.url or request.base_url anywhere in the source — auth is header-based and authorization reads JWT claims and routed Path() params.

This is therefore dependency hygiene to clear scanners, not incident response. That matters for how it ships: there is no clock on it, so it should land as a proper 0.10.0 (with an rc and release notes) rather than as a patch. on_release.yml pushes :latest on any non-prerelease tag, which is precisely how a mixed fleet forms.

Why this isn't a one-line bump

Starlette cannot move on its own. Verified against PyPI requires_dist for every non-prerelease fastapi from 0.125.0 to 0.141.1:

boundary	version	constraint at that version
last fastapi admitting pydantic 1.x	0.125.0	`pydantic <3.0.0,>=1.7.4`
first fastapi requiring pydantic 2	0.126.0	`pydantic >=2.7.0`
last fastapi capping starlette below 1.x	0.132.1	`starlette <1.0.0,>=0.40.0`
first fastapi admitting starlette 1.x	0.133.0	`starlette >=0.40.0`
first fastapi requiring pydantic ≥ 2.9	0.135.2	`pydantic >=2.9.0`

So: py ≥ 3.10 → pydantic 2 → fastapi ≥ 0.133 → starlette ≥ 1.0.1. Note the pydantic-2 requirement arrives at 0.126.0, seven releases before the starlette change and independent of it.

We floor fastapi at 0.135.0 rather than 0.133.0 deliberately: resolving with 0.133.0 does reach a patched starlette, but nobody has run the suite against it, so declaring support would be an untested claim. The floor is conservative at no cost.

fastapi<1 was never the blocker — FastAPI is still 0.x.

Changes proposed

Dependencies (`packages/requires.txt`)

starlette >=0.40.0,<1 → >=1.3.1,<2. Not 1.0.1: per OSV, starlette 1.0.1 is still affected by GHSA-82w8-qh3p-5jfq (HIGH, form limits ignored → DoS, fixed 1.3.1), GHSA-wqp7-x3pw-xc5r (HIGH, SSRF + NTLM theft via UNC paths, fixed 1.1.0), GHSA-x746-7m8f-x49c (MODERATE, fixed 1.1.0) and GHSA-jp82-jpqv-5vv3 (fixed 1.3.0 — the direct sibling of the bug being fixed here). A fresh resolve installs 1.6.0 so the built image would have been clean either way, but the declared constraint is what downstream lockfiles and scanners read, which is the whole point of this PR. 1.3.1 and 1.6.0 are both clean per OSV.
fastapi >=0.109.1,<1 → >=0.135.0,<1
pydantic[email] >=1.9.1,<2 → >=2.9.0,<3. The <3 ceiling is load-bearing, not boilerplate: fastapi_websocket_pubsub 1.0.1 calls v1 .copy() at event_notifier.py:205 on the notify path, and 1.0.1 is the newest published release. Under pydantic 3 that would raise inside a handler that swallows exceptions, silently dropping data updates.
fastapi_websocket_pubsub → >=1.0.1, fastapi_websocket_rpc → >=0.1.29. Both already ship is_pydantic_pre_v2() shims and run fine under v2; floors raised to make the requirement explicit.
fastapi-utils removed entirely, along with the typing-inspect entry it required. from fastapi_utils.tasks import repeat_every in opal_server/data/data_update_publisher.py was the only reference to the package in the repo, and repeat_every is never called — periodic publishing is PeriodicPublisher in opal_common/topics/publisher.py. That one dead import also dragged in a transitive psutil<6 cap (fastapi-utils 0.8.0 declares psutil>=5,<6) which silently held psutil at 5.9.8, including on Alpine where it then builds from source. Verified: a clean uncached resolve now installs neither package, and psutil moves 5.9.8 → 7.2.2.
setuptools >=70.0.0 → >=78.1.1 (GHSA-5rjg-fvgr-3xxf, HIGH path traversal). Same in the root requirements.txt.
dropped typing-extensions;python_version<'3.8' (dead once the floor is 3.10).

Python floor

python_requires >=3.9,<3.13 → >=3.10,<3.13 in all three packages, plus the 3.9 classifier and the 3.9 CI matrix leg. The shipped images were already on python:3.10-bookworm — only sdist metadata and CI still claimed 3.9, so no runtime loses support.

pydantic v1 → v2

Mechanical part: @validator/@root_validator → @field_validator/@model_validator; class Config → model_config = ConfigDict(...) (orm_mode→from_attributes, allow_population_by_field_name→populate_by_name); .dict()/.json()/.copy()/.parse_obj()/.parse_raw()/.construct() → their model_* equivalents; pydantic.json.pydantic_encoder (removed) → pydantic_core.to_jsonable_python; pydantic.error_wrappers.ValidationError → pydantic.ValidationError.

Behaviour changes that are not renames. Each was reproduced A/B against a pydantic 1.10.26 baseline on py3.11.

Bare Optional[X] is required in v2 (it defaulted to None in v1). Hit PolicyBundle.deleted_files, AccessToken.details, ClientInfo.source_host/source_port. The last two were caught only by diffing the generated OpenAPI against master. An AST sweep confirms no others remain.
X = Field(None, ...) on a non-Optional annotation no longer accepts an explicit None. Annotations widened across store.py, data.py, events.py, http_fetch_provider.py.
v2 wraps only ValueError/AssertionError into ValidationError — a TypeError raised inside a validator now escapes raw. JSONPatchAction.value_must_be_present and DataSourceEntry.validate_save_method now raise ValueError.
v2 unions default to "smart" mode. JsonableValue = Union[List[JSONPatchAction], List[Any], Dict[str, Any]] is order-dependent; smart mode matches the catch-all List[Any] first and leaves raw dicts, breaking isinstance(elem, JSONPatchAction) on the save_method="PATCH" path. Pinned to union_mode="left_to_right" (v1 semantics).
use_enum_values moved from dump time to validation time, and defaults are never validated. Unset enum fields therefore survive model_dump() as enum members. This broke inline OPA startup on python 3.11+ and left non-JSON-serialisable published payloads — see "Fixes from review" below.
v2 does not coerce numbers to strings — a breaking change for external data providers, documented rather than reverted. See "Breaking changes" below.
timedelta serializes as an ISO-8601 duration, breaking mixed-version fleets. Pinned back to float seconds.
v1 coerced a BaseModel into a dict field via dict(model); v2 rejects it. Restored with a mode="before" validator that fires only when the field declares a plain dict — see below.

Fixes from review

Two independent reviews (@zeevmoney, @dshoen619) found 16 distinct findings between them. All reproduced, all addressed.

Blocking (commit 1):

what	why it was invisible
Inline OPA failed to start on py3.11/3.12 — enum defaults reached argv as `--log-level=LogLevel.info`	`Enum.__format__` changed in 3.11; images and the E2E leg are 3.10, and `get_cli_options_dict()` had no test coverage
`GET /stats` returned 500 on every multi-worker boot — `server_count: int` fed by `len(servers) / workers`	unchanged context; v1 truncated the float, v2 rejects a fractional one
422 responses echoed the rejected body, including fetcher `Authorization` headers and git deploy keys, across 5 routes	v2 attaches `input` to every error and FastAPI serializes `exc.errors()` wholesale; routes around `RedactedReprMixin` because pydantic-core never consults the model's repr
`starlette>=1.0.1` still admitted 2 HIGH advisories	a fresh resolve installs 1.6.0, so only the declared constraint was wrong

Non-blocking (commit 2): the config shim was inherited by subclasses that re-declare config, where it silently dropped subclass-only fields and blanked alias-only fields — a third-party provider config lost api_key='SECRET-KEY' to None with no exception, then fetched with empty credentials. DataSourceEntry.config and DataSourceConfig.entries were missing the coercion entirely, so the documented external-data-source pattern raised. Plus ttl wire compat, the dead fastapi-utils branch, JSON-safe publish payloads, three remaining .json() call sites, and the docs.

A note on the regression test for the OPA enum bug. The natural test — "assert every value in the returned dict is a str" — passes on the broken code. These enums are str mixins, so isinstance(LogLevel.info, str) is True and opts["--authentication"] == "off" is also True. Only type(v) is str and the rendered argv expose the difference. The tests here assert on both.

Breaking changes

1. External data providers: numbers are no longer coerced to strings.

POST /data/config and POST /scopes/{scope_id}/data/update now reject numeric values in string-typed fields — reason, and per entry url, dst_path, save_method and each item of topics. A numeric reason (a ticket or revision number) is the most likely to bite. This is deliberate: restoring v1 leniency with coerce_numbers_to_str=True would be a design regression. Documented in configure_external_data_sources.mdx; integrators must cast before sending.

2. Custom fetch providers must be pydantic v2 compatible.

The only change that breaks at import time is a bare @root_validator, which raises PydanticUserError. @validator, inner class Config and .dict() all still work through v2's deprecation shims (they are removed in pydantic v3). A bare Optional[X] annotation becoming required is the quiet one. write_your_own_fetch_provider.mdx has the migration note.

Two cases are handled for providers and need no work on their side: passing a config model where a dict is declared, and a provider event that re-declares config with its own config type (the instance is preserved intact, including subclass-only and alias-populated fields).

Validation

526/526 unit tests pass in a clean uncached venv (py3.11 · starlette 1.6.0 · fastapi 0.141.1 · pydantic 2.13.5). 475 was the baseline; 51 are new regression tests from the review. The same suite is green on master in an identical venv, so there is no baseline drift.
Every fix verified to fail before and pass after — each was reverted in place and the tests confirmed red.
OpenAPI contract diffed against master for both apps: identical routes, identical schema set, nothing newly required, no property dropped. ttl is back to type: number / default 31536000.0; the only remaining deltas are pydantic v2's richer ValidationError schema (ctx/input) and v1's non-standard format: "time-delta" annotation, neither of which changes a wire type or a default.
Redis scope serialization verified byte-identical between v1 .json() and v2 .model_dump_json(), so existing persisted Scope records are unaffected.
docker build --target client-standalone succeeds; the image runs python 3.10.21 with starlette 1.6.0 and imports opal_client cleanly.
flake8 (the CI gate) and the full pre-commit hook set pass on all files.

Note on scope

There is prior art — PRs #488 / #504 (per-7303-update-pydantic-to-v2, 2023) attempted this and were closed unmerged. This branch is written fresh against current master rather than rebased on those.

Not addressed here: #899 (python base images → 3.12) is orthogonal and still applies on top; the <3.13 ceiling is left alone.

🤖 Generated with Claude Code

Starlette's Host-header handling could poison `request.url.path` so that it differs from the routed path, letting URL-based security checks be bypassed (CVE-2026-48710 / GHSA-86qp-5c8j-p5mr, CVSS 6.5). The advisory marks `starlette <= 1.0.0` affected and `1.0.1` as the first patched release, but `packages/requires.txt` pinned `starlette>=0.40.0,<1` - excluding every patched version. Starlette cannot be bumped on its own. The unlock chain is: starlette >= 1.0.1 -> needs fastapi >= 0.135.0 (first release whose own pin, `starlette>=0.46.0`, has no upper bound; 0.130-0.134 still cap at `starlette<1.0.0`) -> fastapi >= 0.130 requires pydantic >= 2.7 -> starlette 1.x and fastapi >= 0.130 require py >= 3.10 So this bumps the Python floor to 3.10, migrates the codebase from pydantic v1 to v2, and moves fastapi/starlette to the patched line. Dependencies (packages/requires.txt) - starlette >=0.40.0,<1 -> >=1.0.1,<2 (the CVE fix) - fastapi >=0.109.1,<1 -> >=0.135.0,<1 - pydantic[email] >=1.9.1,<2 -> >=2.9.0,<3 - fastapi-utils >=0.2.1,<1 -> >=0.8.0,<1 - 0.2.1 pins `pydantic<2.0` and was what the resolver actually picked on master, so it was a second, undocumented pydantic-v2 blocker. - typing-inspect: added. fastapi-utils >= 0.6 imports it unconditionally (fastapi_utils/cbv.py, reached via `import fastapi_utils.tasks`) but declares it only under its `all` extra - upstream bug fastapiutils/fastapi-utils#318 (still open). Declared directly so we don't drag in `all` (sqlalchemy, pydantic-settings). - fastapi_websocket_pubsub >=1.0.1, fastapi_websocket_rpc >=0.1.29: both already carry `is_pydantic_pre_v2()` shims and work under v2; floors raised to make that explicit. - dropped `typing-extensions;python_version<'3.8'` (dead at py>=3.10). Python floor - `python_requires` `>=3.9,<3.13` -> `>=3.10,<3.13` in all three packages, 3.9 classifier and CI matrix leg dropped. The shipped images were already on `python:3.10-bookworm`, so only sdist metadata and CI claimed 3.9. pydantic v1 -> v2 migration - `@validator`/`@root_validator` -> `@field_validator`/`@model_validator`; `values[...]` -> `ValidationInfo.data`. - `class Config` -> `model_config = ConfigDict(...)`; `orm_mode` -> `from_attributes`, `allow_population_by_field_name` -> `populate_by_name`. `OpaServerOptions`/`CedarServerOptions` had `alias_generator` as a Config classmethod; it is now the module-level `_cli_alias` callable. - `.dict()`/`.json()`/`.copy()`/`.parse_obj()`/`.parse_raw()`/`.construct()` -> `.model_dump()`/`.model_dump_json()`/`.model_copy()`/`.model_validate()` /`.model_validate_json()`/`.model_construct()`. - `pydantic.json.pydantic_encoder` (removed in v2) -> `pydantic_core. to_jsonable_python`; `pydantic.error_wrappers.ValidationError` -> `pydantic.ValidationError`. Four v2 semantic traps that are behaviour changes, not renames: 1. Bare `Optional[X]` is REQUIRED in v2 (it defaulted to None in v1). Fixed `PolicyBundle.deleted_files`, `AccessToken.details` and `ClientInfo.source_host`/`source_port`. The last two were caught only by diffing the generated OpenAPI schema against master. 2. `X = Field(None, ...)` on a non-Optional annotation no longer accepts an explicit None. Annotations widened to `Optional[X]` across store.py/data.py/events.py/http_fetch_provider.py. 3. v2 wraps only ValueError/AssertionError into ValidationError - a TypeError raised inside a validator now escapes raw (v1 wrapped it). `JSONPatchAction.value_must_be_present` and `DataSourceEntry.validate_save_method` now raise ValueError. 4. v2 unions default to "smart" mode. `JsonableValue` is order-dependent - smart mode matched the catch-all `List[Any]` first and left raw dicts, breaking the `isinstance(elem, JSONPatchAction)` check on the PATCH save_method path. Pinned to `union_mode="left_to_right"` (v1 semantics). Plus one coercion v1 did implicitly: a BaseModel passed to a `dict` field was accepted via `dict(model)`. v2 rejects it, which broke `FetchingEngine.queue_url(url, cb, HttpFetcherConfig(...))`. Restored with a `mode="before"` validator on `FetchEvent.config`. Docs - write_your_own_fetch_provider: custom fetch providers are pydantic models, so a v2 bump is breaking for them. Added a migration note and fixed the `config: X = None` samples to `Optional[X] = None`. Validation - 475/475 unit tests pass (py3.11, starlette 1.6.0, fastapi 0.141.1, pydantic 2.13.5). Same suite is green on master, so no baseline drift. - Generated OpenAPI for both apps diffed against master: identical routes, identical schemas, nothing newly required, no property dropped. - flake8 (the CI gate) and the full pre-commit hook set pass. - `docker build --target client-standalone` succeeds; the image runs python 3.10.21 with starlette 1.6.0 and imports opal_client cleanly.

linear-code · 2026-09-14T14:14:09Z

PER-15885

netlify · 2026-09-14T14:14:12Z

✅ Deploy Preview for opal-docs canceled.

Name	Link
🔨 Latest commit	`1307ac7`
🔍 Latest deploy log	https://app.netlify.com/projects/opal-docs/deploys/6ab1b9c3f7d2be0008d662fc

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

zeevmoney

Changes requested — 1 HIGH, 6 MEDIUM, 2 LOW.

Blocking:

HIGH packages/opal-client/opal_client/engine/options.py:97 — enum defaults reach the OPA command line as LogLevel.info, so inline OPA fails to start on Python 3.11/3.12; the 3.10 image and the 3.10 E2E leg hide it

Non-blocking:

MEDIUM packages/opal-common/opal_common/schemas/data.py:54 — the model-to-dict coercion added for FetchEvent.config is missing here, breaking the pattern documented in configure_external_data_sources.mdx:224
MEDIUM packages/opal-common/opal_common/fetcher/events.py:63 — the new coercion is inherited by subclasses that re-declare config, where it silently drops their extra fields
MEDIUM packages/opal-common/opal_common/schemas/security.py:32 — ttl now serializes as P1Y, which a pydantic-v1 server rejects, breaking obtain-token from an upgraded CLI during a rolling upgrade
MEDIUM packages/requires.txt:11 — fastapi-utils, the new typing-inspect entry and a hidden psutil<6 cap all exist to satisfy an unused import
MEDIUM packages/opal-common/opal_common/schemas/data.py:183 — numeric values that v1 coerced to strings are now rejected at POST /data/config, an undocumented breaking change for external data providers
MEDIUM packages/opal-common/opal_common/middleware.py:16 — 422 bodies now echo the full request body, including Authorization headers and git deploy keys, across five routes
LOW packages/requires.txt:3 — the dependency table names 0.135 as the first fastapi without a starlette<1 ceiling; it is 0.133.0, and pydantic 2 was required from 0.126.0
LOW documentation/docs/tutorials/write_your_own_fetch_provider.mdx:97 — "Since OPAL 0.9" names releases that ship pydantic v1, and two samples further down the page still use .dict()

Not posted inline, outside this diff:

LOW packages/opal-common/opal_common/cli/commands.py:73, :180 and packages/opal-server/opal_server/redis_utils.py:48 still call the deprecated .json(). They work today through the v2 shim; the description's claim that all such call sites were migrated does not hold.
LOW packages/requires.txt:8 — the pydantic<3 ceiling is load-bearing. fastapi_websocket_pubsub 1.0.1 calls v1 .copy() at event_notifier.py:205 on the notify path, and 1.0.1 is the newest published release. Under pydantic 3 that would raise inside a handler that swallows exceptions, silently dropping data updates.

The five behaviour traps listed in the description were each reproduced against both pydantic versions and are handled correctly.

Details are in the inline comments on each line.

dshoen619 · 2026-09-15T09:07:10Z

Review: verified A/B against a pydantic v1 baseline

I reviewed this by building a venv from this branch's own requires.txt (py3.11 · pydantic 2.13.5 · fastapi 0.141.1 · starlette 1.6.0) and an A/B baseline on master under pydantic 1.10.26, then running identical probes through both. Everything below is a reproduced side-by-side, not a reading of the diff.

The hard part of this PR is right. The dependency chain is real, the subtle traps were found, and the headline claims hold up under independent check:

Claim	Result
Full unit suite	475 / 475 reproduced
Trap 1 — `Optional` becoming required	0 changes across all 53 models
OpenAPI contract	10→10 routes, 24→24 schemas, 0 newly required, 0 dropped
Wire payloads	byte-identical (`DataUpdate`, `StoreTransaction`, `PolicyBundle`, alias round-trip)
Redis-persisted `Scope`	v2 reads a v1 record and re-serialises identically
Credential redaction	intact — 29 tests pass, repr output byte-identical to v1
Validation latency	faster (650ms→79ms at N=200k)

What the suite structurally cannot see is a class of bug where v2 changed when a conversion happens, not what it does — so code that never re-validates keeps the raw value. That's four of the five blockers below.

Blocking

1. Inline OPA no longer starts on Python 3.11+

packages/opal-client/opal_client/engine/options.py:89-99 → engine/runner.py:323

use_enum_values was moved faithfully into ConfigDict, but v1 applied it at dump time and v2 applies it only at validation time — and fields sitting at their defaults are never validated. So default enums survive model_dump() as Enum objects:

master:  --authentication=off                      --log-level=info
this PR: --authentication=AuthenticationScheme.off --log-level=LogLevel.info

OPA rejects those. It hides because it's Python-gated — these are str, Enum mixins and 3.11 changed Enum.__format__:

py3.10  f"{LogLevel.info}" -> 'info'          <- Docker base: safe by luck
py3.11  f"{LogLevel.info}" -> 'LogLevel.info' <- breaks
py3.12  f"{LogLevel.info}" -> 'LogLevel.info' <- breaks

The shipped image is python:3.10 so it's masked, but the CI matrix is ["3.10","3.11","3.12"] and python_requires is >=3.10,<3.13. CI stays green because get_cli_options_dict() has no test coverage — the only engine-options test exercises CedarServerOptions.get_args(), a different path that never dumps.

Fix: model_dump(mode="json", ...).

2. `GET /stats` returns 500 on any multi-worker server

packages/opal-server/opal_server/statistics.py:119

server_count=len(self._state.servers) / self._workers_count — ServerStatsBrief.server_count is int. v1 truncated the float; v2 rejects any float with a fractional part.

                        master    this PR
|servers|=1 workers=4   0         ValidationError -> 500
|servers|=3 workers=4   0         ValidationError -> 500
|servers|=4 workers=4   1         1
|servers|=5 workers=4   1         ValidationError -> 500

_state.servers is the fleet-wide worker set, so the ratio is only integral once every worker has exchanged keepalives. In practice /stats breaks on every fresh multi-worker boot, throughout every rolling restart, after any worker crash, and permanently when replicas run uneven worker counts.

Fix: integer division (//).

3. 422 responses now echo credentials

packages/opal-server/opal_server/scopes/api.py:357 · data/api.py:112

v2 attaches the rejected input to every ValidationError and FastAPI serialises it into the 422 body. Same request, PUT /scopes with a missing field:

// master
{"detail":[{"loc":["body","scope_id"],"msg":"field required","type":"value_error.missing"}]}

// this PR
{"detail":[{"loc":["body","scope_id"],"msg":"Field required",
            "input":{"policy":{"auth":{"token":"ghp_aBcDeFgH..."}}}}]}

An SSH-auth scope missing username returns the full OpenSSH private key. POST /data/config echoes config.headers.Authorization.

This routes around both redaction mechanisms this repo built on purpose — RedactedReprMixin and the explicit exclude={"config","data"} at data/api.py:58 — because pydantic-core builds the error from raw input and never touches the model's repr. CredentialScrubbingStream doesn't catch it either: it only rewrites URL userinfo and known query params, so a bearer token passes straight through.

To be fair on severity: the echoed body is always the caller's own, so this is self-disclosure rather than cross-principal disclosure. The damage is the new secret sink — 422 bodies land in proxy logs, APM and error trackers, and the truncated repr() reaches OPAL's own logs at data/updater.py:252 and statistics.py:222,266,298.

Fix: a RequestValidationError handler keeping only loc/msg/type, and e.errors(include_input=False, include_url=False, include_context=False) at the log sites.

4. The CVE floor still admits two HIGH advisories

packages/requires.txt — starlette>=1.0.1,<2

1.0.1 fixes the named CVE but not what landed after it. Per OSV, starlette 1.0.1 is still affected by:

GHSA-82w8-qh3p-5jfq   HIGH       fixed 1.3.1   form() limits ignored -> DoS
GHSA-wqp7-x3pw-xc5r   HIGH       fixed 1.1.0   SSRF + NTLM theft via UNC paths
GHSA-x746-7m8f-x49c   MODERATE   fixed 1.1.0   arbitrary method -> HTTPEndpoint
GHSA-jp82-jpqv-5vv3   LOW        fixed 1.3.0   unvalidated path poisons url.hostname

GHSA-jp82-jpqv-5vv3 is the direct sibling of the bug being fixed here. A fresh resolve installs 1.6.0 so the built image is clean — but the declared constraint is what downstream lockfiles and scanners read, which is the whole point of the PR.

Fix: starlette>=1.3.1,<2.

High

5. The new config shim silently blanks aliased third-party configs

packages/opal-common/opal_common/fetcher/events.py:49-64

The before-validator does dict(value), which yields field names. Every FetchEvent subclass inherits it — including third-party providers. A FetcherConfig with Field(alias=...) and no populate_by_name can only be populated by alias, so re-validation drops those fields to their defaults, silently:

master:  connection_params={'dsn': 'postgres://h/db'}  api_key='SECRET-KEY'
this PR: connection_params=None                        api_key=None

The provider then fetches with empty credentials and no exception. v1 never re-validated here, so this comes from the shim, not from v2.

Fix: flatten only when the declared config annotation is a plain dict; pass an already-conforming instance through. That keeps the queue_url(..., HttpFetcherConfig(...)) case the shim exists for.

6. `ttl` breaks mixed-version fleets — servers must upgrade first

packages/opal-common/opal_common/schemas/security.py:29 · cli/commands.py:44,73

ttl: timedelta now serialises ISO-8601 instead of float seconds, and v2 collapses 365 days to P1Y — which pydantic v1 cannot parse (its duration regex has no year group):

new client -> old server:  'P1Y' -> ValidationError (422)   BROKEN
old client -> new server:  31536000.0 -> OK                 safe

365 days is the CLI's default, and the published OpenAPI default is now literally "P1Y", so a codegen'd client sends exactly the failing value. This is the only genuine mixed-version break in the PR, and it's one-directional.

Fix: a field_serializer emitting seconds restores wire compatibility in one decorator. Otherwise document that servers must be upgraded before clients.

Worth fixing

7. Trap 5 was fixed in one place but not its two siblings — data.py:

DataSourceEntry(config=HttpFetcherConfig(...))     v1: OK   v2: ValidationError
DataSourceConfig(entries=[DataSourceEntry(...)])   v1: OK   v2: ValidationError

Both appear together in configure_external_data_sources.mdx:219 — the external-data-source reference pattern. That documented snippet does not run on this branch. The suite misses it because every fixture builds entries from dict literals.

8. Model dumps carry non-JSON-serialisable enums — same root cause as #1. json.dumps(update.model_dump(by_alias=True)) at data_update_publisher.py:113 now raises TypeError: Object of type HttpMethods is not JSON serializable. The in-repo broadcaster survives (it serialises via model_dump_json), but that's the published payload shape, and get_data_with_input() (opa_client.py:934,945) is a public extension point third parties call with their own models.

9. Two of the new dependencies serve a dead import. from fastapi_utils.tasks import repeat_every at data_update_publisher.py:5 — repeat_every is never called anywhere. That one line drives the fastapi-utils 0.2.1→0.8.0 bump, the new typing-inspect pin, the four-line comment in requires.txt, and a transitive psutil<6 cap. Deleting it removes all of them.

Two corrections to that comment: the import isn't unconditional — cbv.py branches on pydantic.VERSION and only takes the typing_inspect path on v2; and 0.8.x doesn't require pydantic v2, it declares pydantic>1.0,<3.0.

10. POST /data/config silently tightened. v2 dropped v1's number→string coercion: {"id": 12345, "reason": 42, "dst_path": 7} was accepted (coerced to strings) and now 422s. Nothing in OPAL sends those, so it's invisible to the suite, but it's a real break for external publishers.

11. Stale setuptools floor. setuptools>=70.0.0 is commented "pinned by Snyk" but still admits GHSA-5rjg-fvgr-3xxf (HIGH, path traversal, fixed 78.1.1). Pre-existing, but this is a security-hygiene PR editing that exact file.

12. Three missed .json() sites — redis_utils.py:48 (the scope-persistence serialiser, so it fires on every scope write, and its read half was migrated), cli/commands.py:73,180. All work today via the shim with byte-identical output; they die at pydantic v3.

13. Docs. The annotated tutorial contradicts itself two screens down — write_your_own_fetch_provider.mdx:261,282 still show .dict(). Two pages still claim "Python 3.7 or greater" (install_as_python_packages.mdx:10, as-python-package/overview.mdx:9).

Two framing notes

OPAL was never exposed to this CVE. The vulnerable pattern is enforcing security on request.url rather than the routed path. There is no use of request.url or request.base_url anywhere in the source — auth is header-based and authz reads JWT claims and routed Path() params. The bump is still worth making to clear scanners, but this is dependency hygiene rather than incident response, which is the argument for landing it as a proper 0.10.0 (with an rc and a release note) rather than as a patch. Worth noting on_release.yml pushes :latest on any non-prerelease tag — which is exactly how a mixed fleet forms, given #6.

Also: the note says "Since OPAL 0.9", but 0.9.0–0.9.9 all shipped pydantic v1 and 0.9.9 is current master.

Third-party provider breakage is narrower than the note implies. Tested against real provider shapes, config: X = None, inner class Config, @validator and .dict()-based parse_event() all still work through v2's shims. The one hard break is a bare @root_validator, which raises PydanticUserError at import. Naming that specifically — alongside #5 and #7, which the note doesn't mention — would serve provider authors better than the current broad warning.

Happy to push fixes for the blockers if useful.

@zeevmoney

Four blockers found in review, each reproduced A/B against a pydantic 1.10.26 baseline on py3.11 and verified fixed. 1. Inline OPA failed to start on python 3.11/3.12. `use_enum_values` is applied at validation time under v2 and defaults are never validated, so unset enum options survived `model_dump()` as enum members and `runner.py` f-stringed them into argv as `--log-level=LogLevel.info`. `Enum.__format__` changed in 3.11, so the 3.10 images and the 3.10 E2E leg masked it. Dump in JSON mode instead. Note the regression test asserts on the *rendered* argv and on `type(v) is str`: these enums are `str` mixins, so both `isinstance(v, str)` and `v == "off"` pass on the broken value. 2. `GET /stats` returned 500 on every multi-worker server. `server_count` is an `int` fed by `len(servers) / workers`; v1 truncated the float, v2 rejects a fractional one. The ratio is only integral once every worker has exchanged keepalives, so this broke on every fresh boot and throughout every rolling restart. Use floor division. 3. 422 responses echoed the rejected request body back to the caller. From v2 each validation error carries `input`, and FastAPI serializes `exc.errors()` wholesale - so fetcher `Authorization` headers and git deploy keys came back on /data/config, /data/callback_report, /scopes and /callbacks, and landed in every proxy log and APM trace in front of them. This also routes around `RedactedReprMixin`, since pydantic-core builds the error from raw input. Strip `input`/`ctx`/ `url` in a `RequestValidationError` handler on `configure_middleware`, which both apps already call. 4. `starlette>=1.0.1` still admitted two HIGH advisories. Per OSV, 1.0.1 is affected by GHSA-82w8-qh3p-5jfq (HIGH, fixed 1.3.1), GHSA-wqp7-x3pw-xc5r (HIGH, fixed 1.1.0), GHSA-x746-7m8f-x49c (MODERATE, fixed 1.1.0) and GHSA-jp82-jpqv-5vv3 (fixed 1.3.0) - the last being the direct sibling of the CVE this PR fixes. A fresh resolve installs 1.6.0 so the image was clean, but the declared constraint is what downstream lockfiles and scanners read. Floor to 1.3.1. Unit suite: 501 passed (475 baseline + 26 new). flake8 gate clean, pre-commit clean. Addresses review comments: - #959 (comment) (@zeevmoney) - #959 (comment) (@zeevmoney) - #959 (comment) (@dshoen619) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@zeevmoney

The non-blocking half of the review. Each behaviour change reproduced A/B against a pydantic 1.10.26 baseline on py3.11. Config coercion (one rule, two models) -------------------------------------- The `mode="before"` shim on `FetchEvent.config` fired on every subclass, including ones that re-declare `config` with a concrete model type. There the value was flattened to a dict and re-validated into the declared type, which silently: - dropped fields a subclass added on top of that type, and - blanked fields that are alias-only, since `dict(model)` yields field names and a model without `populate_by_name` cannot read them back. A third-party provider config with `Field(alias=...)` lost its credentials outright - `api_key='SECRET-KEY'` became `None` with no exception, and the provider then fetched with empty auth. v1 never re-validated an instance there, so this came from the shim, not from v2. Now coerce only when the field actually declares a plain `dict`, via a shared `coerce_config_model_to_dict` helper, and apply the same rule to `DataSourceEntry.config`, which was missing it entirely - breaking the `DataSourceEntry(config=HttpFetcherConfig(...))` pattern documented in `configure_external_data_sources.mdx`. `DataSourceConfig.entries` needed the matching fix: it declares the `DataSourceEntryWithPollingInterval` subclass, so a base `DataSourceEntry` instance is not an instance of the declared type and v2 rejected the nested form of that same documented sample. The subclass only adds an optional field, so flattening the parent is lossless. Token wire format ----------------- `AccessTokenRequest.ttl` is a `timedelta`. v2 serializes it as an ISO-8601 duration and collapses 365 days - the CLI default, and now the published OpenAPI default - to "P1Y", which a v1 duration parser cannot read. An upgraded CLI therefore 422'd against a server that had not restarted yet. `ser_json_timedelta="float"` restores the numeric wire form; the reverse direction already worked. Dead dependency branch ---------------------- `from fastapi_utils.tasks import repeat_every` in `data_update_publisher` was the only reference to `fastapi_utils` in the repo, and `repeat_every` is never called - periodic publishing is `PeriodicPublisher`. That one import was the entire reason for the `fastapi-utils` floor, the new `typing-inspect` entry, and a transitive `psutil<6` cap that silently held psutil at 5.9.8 (building from source on Alpine). Removing it drops all three from every published OPAL package. Verified: the five modules that previously failed without `fastapi_utils` import cleanly, and the full suite passes with both packages uninstalled. Also bumps `setuptools` 70.0.0 -> 78.1.1 (GHSA-5rjg-fvgr-3xxf, HIGH) in `packages/requires.txt` and `requirements.txt`. Serialization ------------- - `data_update_publisher` publishes with `mode="json"`; a python-mode dump kept enum members (`HttpMethods.GET` in a fetcher config) that v1's `.dict()` unwrapped, leaving a payload downstream `json.dumps` rejects. - The three remaining v1 `.json()` calls migrated to `model_dump_json()`: `redis_utils._serialize` (scope persistence, fires on every scope write), and `cli/commands.py` at :73 and :180. Output for the scope serializer verified byte-identical, so existing Redis records are unaffected. Docs ---- - fetch-provider migration note: "Since OPAL 0.9" named releases that ship pydantic v1; corrected to 0.10, and rewritten to lead with the one thing that breaks at import time (a bare `@root_validator`) rather than listing renames that still work through shims. Notes the two cases OPAL now handles for providers. - the two `.dict()` samples further down that same page. - `configure_external_data_sources.mdx`: documents that v2 no longer coerces numbers to strings, so `POST /data/config` now 422s on a numeric `reason`/`url`/`dst_path`/`topics` entry. Deliberately not restoring v1 leniency - the gap was that the change was undocumented. - two pages still claiming "python 3.7 or greater". Unit suite: 526 passed (501 after the previous commit + 25 new). flake8 gate clean, pre-commit clean on all files. Addresses review comments: - #959 (comment) (@zeevmoney) - #959 (comment) (@zeevmoney) - #959 (comment) (@zeevmoney) - #959 (comment) (@zeevmoney) - #959 (comment) (@zeevmoney) - #959 (comment) (@zeevmoney) - #959 (comment) (@dshoen619) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Zivxx · 2026-09-15T12:26:20Z

Review addressed — 2 commits, 51 new regression tests

Thanks both. @dshoen619 and @zeevmoney found 16 distinct findings between them; I reproduced every one A/B against a pydantic 1.10.26 baseline on py3.11 and none were false positives. Ownership of this PR has moved from @EliMoshkovich to our team, so we've taken it from here.

d634d9b — the four blockers · 89f36a8 — follow-ups and dependency cleanup

Two findings only one of you had

@dshoen619's #2 (GET /stats 500) and #4 (starlette floor) weren't in the inline review, and I'd rate both blocking:

/stats: server_count: int fed by len(servers) / workers — unchanged context that breaks purely from the floor bump. Reproduced 1/4, 3/4, 5/4 -> ValidationError where master truncated to 0, 0, 1. Breaks on every fresh multi-worker boot and throughout every rolling restart.
the floor: per OSV, starlette==1.0.1 is still affected by GHSA-82w8-qh3p-5jfq (HIGH, fixed 1.3.1) and GHSA-wqp7-x3pw-xc5r (HIGH, fixed 1.1.0), plus GHSA-jp82-jpqv-5vv3 — the direct sibling of the CVE being fixed. Now >=1.3.1. A fresh resolve installed 1.6.0 anyway, so only the declared constraint was wrong — which is the whole point of the PR.

The one thing I'd push back on

@zeevmoney's suggested test for the OPA enum bug — "a test asserting every value in the returned dict is a str" — passes on the broken code. I wrote it first and it went green. These are str mixins: isinstance(LogLevel.info, str) is True, and opts["--authentication"] == "off" is True too. Only type(v) is str and the rendered argv expose it. Flagging it because it's a plausible test to write and it would have shipped false confidence.

Where a finding was bigger than described

@dshoen619's #7 — fixing DataSourceEntry.config wasn't enough. DataSourceConfig.entries is typed List[DataSourceEntryWithPollingInterval], a subclass, so a base DataSourceEntry instance fails v2's instance check on the outer field as well. The nested form of that documented sample needed its own fix. My test for the inner finding is what caught it.

The two off-diff LOWs from @zeevmoney's review body

The three remaining .json() sites — fixed in 89f36a8 (redis_utils._serialize, cli/commands.py:73 and :180). You were right that the description's "all call sites migrated" claim didn't hold. Since _serialize is the scope-persistence writer, I verified v1 .json() and v2 .model_dump_json() are byte-identical on a real Scope with SSH auth before changing it, so persisted records are unaffected.
The pydantic<3 ceiling being load-bearing — agreed, and no change needed: that's a reason to keep the ceiling, which we have. I've written your reasoning into the description so the next person doesn't "tidy" it away: fastapi_websocket_pubsub 1.0.1 calls v1 .copy() at event_notifier.py:205 on the notify path, 1.0.1 is the newest release, and under pydantic 3 that raises inside a handler that swallows exceptions — silently dropping data updates.

Verification

526/526 unit tests in a clean uncached venv (475 baseline + 51 new)
Every fix verified red-before / green-after — each reverted in place to confirm the test catches it
fastapi-utils and typing-inspect absent from a clean resolve, and psutil 5.9.8 -> 7.2.2 — observable proof the hidden transitive cap lifted
OpenAPI back to parity with master, ttl included
flake8 gate and full pre-commit clean

Accepted framing, and what's next

@dshoen619's exposure analysis is accepted and now in the description: OPAL was never exploitable through this CVE — no request.url / request.base_url anywhere in the source. This is scanner hygiene, not incident response. Since there's no clock on it, we're shipping it as a 0.10.0 with an rc rather than a patch.

Unit tests aren't enough for a migration this size, so before GA we're building a test kit on top of our existing OPAL staging bed. The key property: that bed already runs 60 real opal-client:0.9.6 PDPs, which are pydantic v1 — so pointing it at a v2 server is the mixed-version wire-compat test, and its integrity checker reads data out of each PDP's real OPA store rather than trusting a socket metric. Adding to it: a golden wire-payload corpus asserted byte-identical across versions, a Python 3.10/3.11/3.12 image matrix running inline OPA against the real opa binary, a mixed v1/v2 fleet on one broadcaster, and a Redis round-trip written by v1 and read by v2.

@dshoen619 — you offered to push fixes for the blockers; they're all in, but a second pass on the two commits would be very welcome, particularly the config-coercion rule.

Unit tests cannot see a wire break, because they exercise both sides at the same version. Every opal-client in the field is pydantic v1 (0.9.9-rc.2, the newest tag, still pins `pydantic[email]>=1.9.1,<2`), so a v2 server publishes to, and is posted to by, v1 peers. This captures the v1 wire form of 28 models - everything that is published to clients, served by a route, persisted to Redis or posted back by a client - as goldens generated under pydantic 1.10.26 against a pre-migration checkout, and asserts the v2 tree against them in four directions: forward v2 serializes the JSON document a v1 peer expects reverse v2 parses (and round-trips) the document a v1 peer sends py-mode model_dump() holds what v1's .dict() held dumps json.dumps survives a dump `generate_golden.py` refuses to run under v2: regenerating the goldens from this tree would assert v2 against itself and prove nothing. Python mode is asserted separately because model_dump_json() unwraps enums, datetimes and Paths whatever the config says - so a JSON-only corpus cannot see the trap that broke inline OPA startup. `canon.py` renders a python-mode dump type-preservingly, making `'get'` vs `HttpMethods.GET` an ordinary diff. Found three deltas that neither review reported ----------------------------------------------- 1. `get_data_with_input()` (opa_client.py:934) json.dumps's a python-mode dump of a CALLER-SUPPLIED model. It is a public extension point, so a caller whose model carries an enum worked under v1 - whose .dict() unwrapped it - and raises TypeError under v2. **Fixed**: mode="json". 2. Enum members survive python-mode dumps on HttpFetcherConfig.method and AccessTokenRequest.type. Cause: `use_enum_values` fires at validation time, then a `force_enum` validator puts the member back, and v1 unwrapped only at .dict() time. Every in-tree path to a wire was checked and all go through JSON mode, so the wire is unchanged - recorded in ACCEPTED_DELTAS rather than "fixed", since removing force_enum would break callers relying on `.value` and isinstance. 3. `AnyHttpUrl` is a str subclass in v1 and a `pydantic_core.Url` object in v2, so a python-mode dump holds an object. Both in-tree consumers already call str() on it, and those calls pre-date this migration. Also verified the harness catches what the reviewers found by hand: reverting the ttl fix reproduces `31536000.0 (float) -> 'P1Y' (str)`, and reverting use_enum_values reproduces `'get' -> HttpMethods.GET`, both with a readable path into the payload. An accepted delta must carry the evidence that established it is safe, not just a claim - the datetime entry records that a pydantic 1.10.26 peer parses both spellings to the same instant. Also folds in a black reformat of access_token_wire_format_test.py, which was created after the previous commit's scoped pre-commit run and would otherwise have failed CI. Unit suite: 640 passed (526 + 114 new). flake8 gate clean, pre-commit clean on all files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Zivxx · 2026-09-15T12:39:41Z

Wire-compat corpus added — and it found three more deltas

1665727 adds packages/opal-common/opal_common/tests/wire_compat/: the pydantic v1 wire form of 28 models, captured under pydantic 1.10.26 against a pre-migration checkout, asserted against this tree on every CI run.

The premise: unit tests can't see a wire break, because they exercise both sides at the same version. Every opal-client in the field is v1 — 0.9.9-rc.2 still pins pydantic[email]>=1.9.1,<2 — so a v2 server publishes to, and is posted to by, v1 peers. generate_golden.py refuses to run under v2, since regenerating from this tree would assert v2 against itself and prove nothing.

Four assertions per case: forward (v2 emits what a v1 peer expects), reverse (v2 parses and round-trips what a v1 peer sends), python-mode dump parity, and json.dumps survival.

It found three things neither review caught

1. get_data_with_input() — fixed. opa_client.py:934 does json.dumps() on a python-mode dump of a caller-supplied model. @dshoen619 named this as a public extension point; the corpus showed why it matters. A caller whose model carries an enum worked under v1 (whose .dict() unwrapped it) and raises TypeError under v2. Now mode="json".

2. Enum members survive python-mode dumps on HttpFetcherConfig.method and AccessTokenRequest.type. The cause is subtler than the inline-OPA case: use_enum_values fires at validation time, and then a force_enum field validator puts the member back, so the attribute is an enum on both versions — v1 just unwrapped it at .dict() time. I traced every in-tree path from these to a wire and they all go through JSON mode (data_update_publisher mode="json", callbacks/register.py:64 and reporter.py:47 model_dump_json()), so the wire is unchanged. Recorded as an accepted delta rather than "fixed": removing force_enum would break callers relying on .value and isinstance.

3. AnyHttpUrl is no longer a str. It's a str subclass in v1 and a pydantic_core.Url object in v2, so a python-mode dump holds an object. Both in-tree consumers already call str() on it (data/api.py:91, scopes/api.py:810) and — worth noting — those calls pre-date this migration, so nothing regressed.

Why python mode is asserted separately

This is the part I'd flag for a reviewer. model_dump_json() unwraps enums, datetimes and Paths whatever the config says, so a JSON-only corpus cannot see the trap that broke inline OPA startup. My first version of this harness was JSON-only and passed with use_enum_values deliberately removed. canon.py now renders a python-mode dump type-preservingly, so 'get' vs HttpMethods.GET is an ordinary readable diff.

The harness catches what you both found by hand

Verified by reverting each fix in place:

ttl fix reverted     ttl: 31536000.0 (float) -> 'P1Y' (str)
                     ttl: 3600.0 (float) -> 'PT1H' (str)     <- also the explicit-ttl case
enum fix reverted    config.method: 'get' -> {'__enum__': 'HttpMethods.GET'}
                     callback.callbacks[1].__tuple__[1].method: ...  <- nested in a union tuple

An accepted delta has to carry the evidence that established it's safe, not just a claim — the datetime entry records that a pydantic 1.10.26 peer parses both Z and +00:00 to the same instant (timestamp 1821002400.0), which I measured rather than assumed.

Where this sits in the plan

This is the cheap, permanent half: pure Python, no cluster, runs in CI forever. The live-fleet half is next — pointing our OPAL staging bed at a v2 server, since that bed already runs 60 real opal-client:0.9.6 PDPs (pydantic v1) and its integrity checker reads data back out of each PDP's own OPA store rather than trusting a socket metric. Plus a 3.10/3.11/3.12 image matrix running inline OPA against the real opa binary, and a Redis round-trip written by v1 and read by v2.

640 unit tests pass (526 + 114 new). flake8 gate and pre-commit clean.

One process note: the --all-files pre-commit run caught a black reformat that slipped into 89f36a8 — I'd only run the hooks over the files I listed, and created one test file after that. It would have failed CI. Folded into this commit.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

…the matrix The inline-OPA regression was python-version-gated, so nothing that runs on a single interpreter could have caught it. This closes that directly. Asserting `get_cli_options_dict()` returns strings is necessary but NOT sufficient: these enums are `str` mixins, so a broken value satisfies both `isinstance(v, str)` and `v == "off"`. The only assertion that cannot be fooled is handing the argv to `opa` and seeing whether it parses. `opa_argv_real_binary_test.py` builds argv through the production path (`OpaRunner.get_arguments()`, not a replica) for seven option shapes including the default production one, and runs the real binary. The binary is found via OPAL_TEST_OPA_BINARY / OPAL_INLINE_OPA_EXEC_PATH / PATH; absent it, the tests SKIP with that reason rather than passing, because a missing instrument is not evidence that the argv is correct. It carries its own control (`test_real_opa_rejects_the_regression_argv`): without proving the probe can detect a known-bad argv, every other assertion in the file could pass vacuously. CI already runs pytest on 3.10/3.11/3.12, so this inherits the version matrix for free - which the 3.10-only images and E2E leg never gave us. Verified locally against opa 1.20.2 on all three interpreters, with the fix reverted in place: with fix without fix py3.10 15 passed 15 passed <- masked, exactly as reported py3.11 15 passed 14 failed py3.12 15 passed 14 failed The single test still passing on 3.11/3.12 is the control, correctly. The real binary's answer to the bad argv: Error: invalid argument "AuthenticationScheme.off" for "--authentication" flag: must be one of {token,tls,off} Also adds `wire_compat/enum_inventory_test.py`: a new enum-bearing model must either have a wire_compat case or be listed in NOT_ON_THE_WIRE with a reason. It is an inventory check, failing when a model is added - which is when the decision should be made rather than after a release. It found two gaps on its first run: TokenDetails (a false negative in the first coverage heuristic, now fixed by walking the model graph so a case covers what it embeds) and GitWebhookRequestParams (genuinely uncovered - excused, since it is OPAL_POLICY_REPO_WEBHOOK_PARAMS config read via confi.model, never sent to a peer, and its enum is compared as an enum at webhook/deps.py:52). Current inventory: 5 enum-bearing models, 4 in the corpus, 1 excused; 29 models reachable from the corpus overall. Full suite: 662 passed on py3.10, py3.11 AND py3.12 - so nothing else in the tree is version-sensitive either. flake8 gate clean, pre-commit clean on all files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

`Scope` is the only OPAL model that is PERSISTED rather than sent. It lives in Redis under `permit.io/Scope:*`, written by whichever server handles the PUT and read by every server afterwards - so during a rolling upgrade both pydantic versions read the same keyspace, in both directions: v1 writes -> v2 reads an upgraded server reading existing records v2 writes -> v1 reads a not-yet-upgraded server reading new ones, which is also what a rollback depends on The unit test added earlier proves v1 `.json()` and v2 `model_dump_json()` are byte-identical for one synthetic record. This exercises the real `ScopeRepository` and the real `RedisDB._serialize` against a real Redis, across two interpreters, over the corpus's Scope cases (ssh auth, github token auth, no auth - all three arms of the discriminated auth union), and checks the repository's `all()` scan path as well as per-key `get()`. ./run_redis_roundtrip.sh /path/to/pre-migration/venv/bin/python brings up its own Redis, runs both directions and tears down. Result here, against redis 7 with pydantic 1.10.26 and 2.13.5: v1 writes wrote 3 scopes under pydantic 1.10.26 -> v2 reads verified 3 scopes: written under 1.10.26, read under 2.13.5 v2 writes wrote 3 scopes under pydantic 2.13.5 -> v1 reads verified 3 scopes: written under 2.13.5, read under 1.10.26 PASS Not a pytest: it needs Redis and two interpreters, so it stays a driver with the kit exit contract - 0 PASS, 1 FAIL, 2 INVALID RUN. The three INVALID/FAIL guards were each verified to fire, so the PASS above is not vacuous: same-version writer and reader -> INVALID (2), asserts nothing a record deleted behind its back -> FAIL (1), caught by BOTH get() and all() unreachable Redis -> INVALID (2), never FAIL Also folds in formatting the hooks applied to the two files added in e1495c9. Running `pre-commit --all-files` and then editing before staging let unformatted content through; the fix is to run the hooks immediately before `git add` and confirm the status is clean afterwards, which is what this commit did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

…1 client The E2E suite builds BOTH images from the tree under test, so it proves a version works against itself. That cannot see the case that actually matters here: opal-client is customer-deployed and upgrades on its own schedule, so a pydantic-v2 server serving pydantic-v1 clients is the steady state in the field, not a transient rollout window. `OPAL_CLIENT_IMAGE_TAG` now overrides the client image independently, defaulting to `OPAL_IMAGE_TAG` so the existing same-version run is unchanged. Ran it: `permitio/opal-client:0.9.6` (pydantic 1.10.26 / starlette 0.50.0 / fastapi 0.125.0) against a server built from this branch (pydantic 2.13.5 / starlette 1.6.0 / fastapi 0.141.1). The v1 client completed the entire suite against the v2 server: - connected to PubSub, got the policy bundle - applied the server's DataSourceConfig PUT /v1/data/static -> 204 - four data publishes bob, alice, sunil, eve - three policy pushes via git - the statistics endpoint - graceful AND ungraceful broadcaster restarts - cross-instance consistency across a backbone outage, including the freeze ("buffered for replay" / "Replaying" / "resyncing this worker's clients") and the negative assertion that the update is ABSENT during the freeze and present after Zero ValidationErrors, zero 422s. In the direction the corpus cannot exercise behaviourally - v1 client serializing to a v2 server - 29 of 29 `POST /data/callback_report` returned 200, no 4xx/5xx. That payload is a DataUpdateReport carrying DataSourceEntry and HttpFetcherConfig, which is exactly the model set the goldens pin. Attribution for the one flake seen: the first mixed run failed on attempt 1 at the data publish following the UNGRACEFUL broadcaster kill and passed on attempt 2. Three runs on the same machine: v2 server + v1 client PASS (attempt 2) v2 server + v2 client PASS (attempt 1) <- control v2 server + v1 client PASS (attempt 1) So the mixed configuration reaches attempt-1 passes just as the control does; the failure was the known reconnect timing sensitivity that `run.sh` already carries MAX_RETRIES=5 for, not the version mix. Recorded in the README so the next person does not re-diagnose it. One note on reading these logs: `run.sh` only dumps container logs on a FAILED attempt, so a clean run captures 183 lines and no access logs at all, while the 2-attempt run captured 2555. The callback evidence above comes from that run. A clean run showing no POSTs is the harness being quiet, not the callbacks being absent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

…atibility Two tracks that needed no cluster, both driven by discovery rather than a hand-written list, so a route or shape added later is covered without anyone remembering. P4 — 422 leak sweep (opal-server) --------------------------------- The 422 sanitiser was written against the five routes a review named. This sweeps EVERY body-accepting route, discovered from the app's own OpenAPI spec, with six malformed body shapes each carrying a canary secret under five plausible credential keys. Discovery independently found exactly the five routes the review found by hand: POST /data/config, /data/callback_report, /scopes/{id}/data/update, /token and PUT /scopes. 36 assertions. It carries two controls, because a leak sweep that cannot detect a leak passes for the wrong reason: - `test_sweep_found_routes` fails if discovery returns an empty list - `test_canary_would_be_visible_without_the_handler` proves the canary IS reachable in an unsanitized 422 Verified by removing the handler from configure_middleware: 30 failed, 6 passed. With it: 36 passed. Also asserts the sanitiser does not strip the DIAGNOSTICS with the input - loc/msg/type must survive, which is what makes a 422 actionable. P5 — provider conformance matrix (opal-common) ---------------------------------------------- Custom fetch providers live outside this repo, so the population cannot be enumerated. It can be characterised: eleven realistic v1-era provider shapes, run under BOTH pydantic versions, producing the table for the release notes. Measured, pydantic 1.10.26 -> 2.13.5: 9 shapes identical bare_root_validator BREAKS at import (PydanticUserError) optional_without_default BREAKS at runtime (ValidationError) So there are TWO breaks for provider authors, not one. A review reported the bare @root_validator as "the one hard break", which is right about import time specifically - the second breaks at runtime, after the module has loaded, when the config is constructed. The matrix also settled a distinction I had wrong in the docs: `x: X = None` is NOT affected (an explicit default keeps the field optional); only a BARE `Optional[X]` with no default becomes required. Both shapes are in the corpus now so the difference stays pinned. It likewise established that `@root_validator(pre=True)` is fine - only the bare form breaks - which the migration note previously did not say. `provider_conformance_test.py` asserts the v2 column in CI with a reason per shape, and fails if the set of breaking shapes changes in EITHER direction: a new break is a new burden on every provider, and a shape that starts working means the docs are telling authors to fix something that no longer needs fixing. The fetch-provider migration note is rewritten around the measurement: the two breaks with their fixes, then a table of what still works through v2's shims. Unit suite: 712 passed, 1 skipped. flake8 gate and pre-commit clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Zivxx · 2026-09-17T10:15:56Z

Staging bed: a pydantic-v2 server serving 60 pydantic-v1 PDPs at prod scale

Brought up env B on stg-eks with a server built from this branch and the stock released client, which is the configuration that actually ships:

	version
3 x opal-server (4 workers each = 12 workers)	pydantic 2.13.5 / starlette 1.6.0 / fastapi 0.141.1
60 x PDP (`permitio/opal-client:0.9.6`)	pydantic 1.10.26 / starlette 0.50.0 / fastapi 0.125.0

2160 scopes - 2000 shared-repo branches, 60 PDP scopes, 36 gitops, and 32 on six classes of deliberately-broken git remotes (401 / 404 / slow-fail / large-slow / connection-refused). Gitea seeded with 41 repos and 2000 branches.

Results

Fleet registration — PASS. 2160/2160 scopes registered through the v2 scope API, 0 failures, in 56.3s. GET /scopes confirms 2163 (2160 + 3 heartbeat), and 20/20 sampled bundles verified actually served. Server boot to Ready: 6m25s on all three pods, zero tracebacks and zero pydantic errors in the logs.

T6.3 cross-server data integrity — PASS. This is the one that answers the question unit tests cannot. A value is published through one v2 server; the PDPs probed are pinned to a different one, so the update has to cross the broadcaster; and the check reads the value back out of each PDP's real OPA store rather than trusting a socket metric:

matches: 20   mismatches: 0   errors: 0   pods_read: 20/20
no_expectation: 0             <- not vacuous
converged_within_s: 37.3
sample: {"pod": "pdp-s1-0", "expected": "x3-100120", "token": "x3-100120", "result": "PASS"}

So: v2 server -> broadcaster -> second v2 server -> v1 client -> OPA, with the exact value intact, 20 for 20.

T6 bundle integrity — PASS. 48 matches, 0 mismatches, 0 wrong-tenant, 0 errors, 60/60 pods read. The seed makes every branch differ by exactly one tenants/tenant_NNNN module, so a PDP holding a tenant key that is not its own branch's would be caught and reported separately.

T3 contract surface — PASS, on every worker of every pod. The route contract PR3 introduced is unchanged under v2, including the exact headers:

case	verdict
3.1 record present, clone healthy	200 everywhere, one consistent hash
3.2 clone in progress	unanimous 503 + `Retry-After: 30` across 12 worker pids
3.3 branch genuinely wrong	409, no `Retry-After`
3.4 clone vanished/corrupt	503 + `Retry-After: 5`, every worker, every variant
3.5 record missing, no default	404 everywhere
3.6 record missing, default healthy	served the default's bundle - cross-tenant hand-off confirmed
3.7 record missing, default broken	503 + `Retry-After: 5`
smoke	every backend-contract status unchanged
3.8 non-git scope	SKIP (documented below)

The per-worker part matters: the bug this track was written for answered correctly on the leader and wrongly on the other N-1 workers, so every case is asked of all 12.

One honest caveat on a PASS

The first T6 probe reported tokens: PASS, and it was vacuous. The artifact says why: matches: 0, mismatches: 0, no_expectation: 60 - there was no per-scope expectation to compare against, so it asserted nothing. The driver records that honestly; the summary line does not surface it. T6.3 above is the same check run with a real expectation, which is why that one counts. Flagging it because "it said PASS" is exactly the failure mode this kit's INVALID-vs-FAIL discipline exists to prevent.

Two incidental confirmations

t3_contract case 3.8 still SKIPs for the same reason, which is itself a pydantic result. It is unreachable because Scope.policy is Union[GitPolicyScopeSource] and source_type is a plain str, so any value validates into a GitPolicyScopeSource. v2's "smart" union mode did not change that - worth knowing, since union-mode changes are one of this migration's documented traps.
FastAPI 0.141 stopped flattening include_router into app.routes (they are _IncludedRouter objects now). OPAL never iterates app.routes, and the OpenAPI spec still emits all 17 server paths, so this is harmless here - but it would silently break any code that walked that list.

Still to come on this bed

A sustained-load run with RSS sampling is in flight (pydantic-core is Rust, so the allocation profile differs from pure-python v1, and prod-us already showed a glibc arena high-water ratchet under PR3). I will report the trend rather than a two-point delta.

Not run, and deliberately: 02-induce-outage.sh and the chaos track that depends on it. Env B shares the broadcast RDS with live staging - only the channel is separate - so inducing the outage blips live staging's pub/sub. That needs announcing first, not doing quietly during someone else's work.

Zivxx · 2026-09-17T10:42:23Z

Bed run complete — sustained load, memory, and teardown

Follow-up to the bed results above. The fleet ran a 5093-request churn (data updates + scope refreshes) against the 12-worker v2 fleet with 2160 scopes and 60 pydantic-v1 PDPs attached.

Server-side: clean

5077 x HTTP 200     0 x 4xx     0 x 5xx
across all three pods:  0 Internal Server Errors, 0 tracebacks, 0 pydantic errors

The 16 non-200s were mine, not the server's. They are status 0 — transport failures with no HTTP response at all (8 ConnectionRefused, 6 ConnectionReset, 2 RemoteDisconnected), and the port-forward log ends with error: lost connection to pod. A kubectl port-forward is a single tunnel and it dropped under load; the server never saw those requests.

I also chased down a scarier-looking number before reporting it: a first grep suggested 36 and 24 "5xx" lines on opal-1/opal-2. They are INFO lines about gitops-largeslow-0 — one of the bed's deliberately-slow remotes — and my 5[0-9][0-9] pattern was matching the timestamp. Actual count of -> 500 / Internal Server Error: zero.

Memory: flat, and the window is sensitive enough to say so

RSS sampled every 60s across the whole load (25.5 min):

pod	10:14	10:39	drift
opal-0	1290 Mi	1289 Mi	−1 Mi
opal-1	1231 Mi	1230 Mi	−1 Mi
opal-2	1236 Mi	1233 Mi	−3 Mi

This mattered because pydantic-core is Rust, so the allocation profile is not the one v1 had, and prod-us already showed a glibc arena high-water ratchet under PR3's per-op git threads. That ratchet ran 35–60 MB/h, which over 25.5 minutes would be 15–26 MB — an order of magnitude above the ±3 MB observed here. So this window could have detected it, and it is absent.

Not a substitute for the 48h soak, and I am not claiming it is. It rules out a leak at that magnitude, on this workload, at 2160 scopes.

One number I am deliberately NOT quoting

The load generator shed 3835 of 8935 ticks because I set --concurrency 8 and it wanted >= 210. Its own warning says not to quote an achieved rate from such a run, so the "4.0/s" in the summary is not a throughput result and the p50/p95 latencies are pacing artifacts rather than measurements. The run is valid as churn — 5093 real requests hit the fleet — and that is all I am using it for.

Teardown

opal-scale-test deleted, port-forwards stopped, permitio/opal-server-v2 on live staging confirmed 2/2 ready and untouched. The ECR images are kept by design (teardown.sh prints the delete commands rather than running them).

What was NOT run, and why

track	why not
T7 chaos, T5 fault injection	need `02-induce-outage.sh`, which blips live staging's pub/sub — env B shares the broadcast RDS, only the channel is separate. Needs announcing first.
T5 48h soak	needs 48h
T4 fan-out ladder	needs the wsgen image built and pushed
T1 memory leak	runs against env A, i.e. live staging

The bed rebuilds in ~40 minutes (Gitea seeding is the long pole) whenever we want the outage-dependent tracks.

zeevmoney

Approved — no CRITICAL or HIGH issues found.

All nine findings from the previous review were reproduced as fixed, each verified against a pydantic 1.10.26 peer rather than taken from the commit message: the OPA argv regression is gone on 3.10, 3.11 and 3.12 and against the real opa binary; the 422 handler closes the echo on every affected route on both apps; ttl is back to float seconds and a v1 server accepts it; the coercion helper restores the documented DataSourceEntry pattern while preserving subclass configs; and fastapi-utils, typing-inspect and the psutil<6 cap are gone.

Non-blocking, one runtime behaviour and eight in the new test infrastructure:

MEDIUM packages/opal-common/opal_common/schemas/data.py:217 — callback reports silently drop periodic_update_interval; a subclass instance in a parent-typed field
MEDIUM packages/opal-common/opal_common/tests/wire_compat/wire_compat_test.py:147 — ACCEPTED_DELTAS keyed by (case, path) only, so a python-mode exemption also silences the JSON wire assertions
MEDIUM packages/opal-common/opal_common/tests/wire_compat/enum_inventory_test.py:32 — the sweep never walks opal_client or opal_server, so its two opal_client exclusions are inert and PolicyStoreDetails is uncovered
MEDIUM packages/opal-common/opal_common/tests/wire_compat/enum_inventory_test.py:81 — an unimportable module is silently dropped from the sweep and the guard only checks non-empty
MEDIUM packages/opal-common/opal_common/tests/wire_compat/run_redis_roundtrip.sh:61 — the orchestrator can never report FAIL; ${PIPESTATUS[0]} reads grep
MEDIUM packages/opal-client/opal_client/tests/opa_argv_real_binary_test.py:10 — the binary-backed assertions skip on every CI leg, and three places claim otherwise
MEDIUM packages/opal-server/opal_server/tests/validation_error_leak_sweep_test.py:38 — "both apps" builds only the server app
MEDIUM documentation/docs/tutorials/configure_external_data_sources.mdx:117 — the breaking-change note lists five fields for a model-wide change and sits on a page that does not document the affected flow
MEDIUM documentation/docs/tutorials/write_your_own_fetch_provider.mdx:135 — "inner class Config works" is wrong for renamed v1 keys, which are dropped silently
LOW packages/opal-common/opal_common/tests/wire_compat/wire_compat_test.py:247 — the JSON-serializable assertion is tautological under mode="json"
LOW packages/opal-common/opal_common/tests/wire_compat/README.md:87 — one accepted delta is a JSON-form difference, so "the wire itself is unchanged" is not accurate
LOW packages/opal-common/opal_common/tests/wire_compat/provider_conformance_test.py:87 — asserts status only, discarding the captured values
LOW packages/opal-common/opal_common/tests/wire_compat/payloads.py:380 — no userpass Scope variant
LOW documentation/docs/getting-started/running-opal/as-python-package/overview.mdx:9 — omits the <3.13 ceiling the sibling page states

Not posted inline, outside the diff: the PR description is now stale in three places — the test count, the "byte-identical" Redis claim, and the OpenAPI delta claim.

Two dependency floors moved without being mentioned in the description: starlette >=1.0.1 → >=1.3.1 and setuptools >=70.0.0 → >=78.1.1. Neither changes what resolves, and the 422 handler's use of the renamed HTTP_422_UNPROCESSABLE_CONTENT does not require either, so a line of rationale would help the next reader.

Details are in the inline comments on each line.

…ess gaps Round-two review. All 14 findings reproduced before fixing; every one held. Product code ------------ `DataUpdate.entries` and `DataEntryReport.entry` are typed with the PARENT `DataSourceEntry`, and pydantic v2 serializes a nested model with the DECLARED type's serializer. So a `DataSourceEntryWithPollingInterval` assigned there silently lost `periodic_update_interval` - v1 serialized the runtime instance and kept it. Measured A/B: master (1.10.26) entry keys include periodic_update_interval this PR, before dropped, no warning this PR, after restored, identical to v1 `SerializeAsAny` on both fields. OpenAPI re-diffed against master: routes, schemas, required and properties all still identical. The corpus could not see this: `payloads.py` builds every case from a dict, and a dict can only validate into the DECLARED type, so the subclass path was never taken. Added `payload_builders.py` - cases with a `builder=` are CONSTRUCTED as real objects - plus two cases using it. Verified they fail without SerializeAsAny and pass with it. That also surfaced a semantic the corpus had wrong: v1 does not round-trip its own subclass-bearing document either (parsing a dict yields the declared type on BOTH versions). The reverse assertion now compares against `json_reparsed`, what v1 produced when parsing its own output, instead of demanding a round trip neither version performs. Harness integrity ----------------- * `run_redis_roundtrip.sh` could never report FAIL. `out=$(cmd | grep)` makes `${PIPESTATUS[0]}` read grep, not the driver. Measured on bash 3.2.57: exit 1 and exit 2 both reported 0, so a real divergence printed PASS. Capture the status directly; print the whole filtered output on failure rather than `tail -1`, which showed only the success-looking summary line. Also fixes the DEBUG filter, which never matched loguru's two-token prefix. Now 0->0, 1->1, 2->2 with reasons shown. * `ACCEPTED_DELTAS` was keyed by `(case, path)`, so an exemption written for a python-mode dump also silenced the JSON-wire assertion at that path - the check the corpus exists for. 16 coordinates were silenced with no cause; three were real holes, including a `PeerType` emitted as "CLIENT" instead of "client", which a v1 peer rejects. Now keyed by `(case, assertion, path)`. * The enum sweep walked only `opal_common.schemas` and `opal_common.fetcher`, so `opal_client.engine.options` - where the inline-OPA regression actually lived - was invisible, and two of the three `NOT_ON_THE_WIRE` entries were INERT, never consulted. Widened to the client and server packages. Following through on what that surfaced: `PolicyStoreDetails` gets corpus cases (it is a FastAPI `response_model` with two NON-str enums - the exact OPA-bug shape, and its python-mode dump does raise in `json.dumps`), and `ConnRetryOptions` gets an excuse with its evidence. * A module that failed to import was dropped from that sweep silently, and the guard only asserted non-empty. Import failures are now collected and asserted empty, and `SWEPT_PACKAGES` entries are checked to be packages rather than modules (a module has no `__path__` and contributes nothing while reading as covered). * The 422 leak sweep said "both apps" and built only the server, leaving the client's `POST /callbacks` - the one client route taking a credential- bearing body - unasserted. Now parameterised over both apps. Verified: with the handler removed, 7 client-route assertions fail. * `test_published_payload_is_json_serializable` dumped with `mode="json"` and then asserted the result was JSON-serializable, which JSON mode guarantees by construction. Now asserts the PYTHON-mode dump, where the trap lives, with documented exceptions. * The provider conformance test read only `status`, discarding the `preserved` and `value` evidence the shapes already collect. Now asserts the full record, with a guard so an "ok" shape cannot be added without evidence. * Added the `userpass` Scope variant - the only auth arm carrying a credential in two fields - which also extends the Redis driver, since it enumerates the same cases. Round trip now covers 4 scopes, both directions. CI -- The docstring claimed the binary-backed OPA tests "inherit the CI python matrix". No job installed `opa`, so 8 of 15 skipped on every leg. CI now installs a pinned opa 1.20.2 in the unit job, making the claim true rather than softening it. Docs ---- * fetch providers: three removed `Field` kwargs (`regex`, `const`, `unique_items`) also break at IMPORT time, and renamed `class Config` keys are accepted then silently ignored - `allow_population_by_field_name` yields `None` where v1 gave the value, so a provider fetches with an empty credential and nothing raises. Both measured, both now documented. * the number-coercion note stated five fields; the rule is general, and it sat on the page documenting the flow it does NOT affect. Moved to `trigger_data_updates.mdx` (the publishing flow) as a rule, with a pointer left behind. * `overview.mdx` omitted the `<3.13` ceiling its sibling page states. * `wire_compat/README.md`: corrected the claim that all deltas are python-mode-only (one is a JSON-form difference), the CI-matrix claim, and the absolute "every client in the field" claim. Verification: 746 passed, 1 skipped. flake8 gate clean, pre-commit clean. NOTE one pre-existing failure NOT introduced here: `data_updater_test.py::test_data_updater_with_report_callback` fails on this machine, including on origin/master under pydantic 1.10.26 with the PR's changes stashed. It is timing-sensitive (a 1s sleep waiting for a callback) and CI has been green on it throughout, so it is environmental. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Zivxx · 2026-09-21T21:03:55Z

Round two addressed — `c116153`

@zeevmoney thanks, this was a better review than the first round and the first round was good. All 14 findings reproduced before fixing; all 14 held. That is 30 for 30 across both rounds.

One of them is product code; the other thirteen are in the test harness I added — which is the uncomfortable and correct place for them to be, since that harness is the argument for trusting the rest.

The product fix

DataUpdate.entries and DataEntryReport.entry are parent-typed, and v2 serializes a nested model with the declared type's serializer, so a DataSourceEntryWithPollingInterval silently lost periodic_update_interval. SerializeAsAny on both; OpenAPI re-diffed against master and still identical.

The corpus couldn't see it, and not by accident: every case was built from a dict, and a dict can only validate into the declared type. Added payload_builders.py so a case can be CONSTRUCTED as a real object.

That then surfaced a semantic I'd had wrong: v1 doesn't round-trip its own subclass-bearing document either. The reverse assertion was demanding something neither version does. Goldens now record json_reparsed — v1's own answer — and the test compares against that.

The one I'd most want on the record

run_redis_roundtrip.sh could never report FAIL. Measured on bash 3.2.57 before touching it:

shipped: driver exit 1 -> orchestrator 0      FAIL lost
shipped: driver exit 2 -> orchestrator 0      INVALID lost
fixed:   0->0, 1->1, 2->2, with reasons printed

I cited that harness's PASS in an earlier comment on this PR. The result is still true — I re-ran it after the fix, now 4 scopes, both directions, exit 0 — but it was not evidence at the time I offered it. A harness that cannot fail is worse than none, because its PASS gets quoted.

The filter was broken too: ^\S+ \| DEBUG never matched loguru's two-token prefix, so it had been filtering nothing.

The rest

#	what	outcome
2	`ACCEPTED_DELTAS` keyed `(case, path)` silenced JSON assertions too	keyed by `(case, assertion, path)`; python-mode exemptions can no longer reach a JSON assertion by construction
3	enum sweep couldn't see `opal_client` — where the OPA bug lived; 2 of 3 exclusions inert	widened; `PolicyStoreDetails` gets corpus cases (not an excuse — it's a `response_model`), `ConnRetryOptions` gets an excuse with evidence
4	import failures dropped silently	collected and asserted empty; plus a guard that `SWEPT_PACKAGES` entries are packages
6	CI never installed `opa`, docstring said it did	CI now installs pinned opa 1.20.2 — claim made true, not softened
7	sweep said "both apps", built only the server	both apps; verified 7 client-route assertions fail without the handler
10	`mode="json"` then assert JSON-serializable — circular	asserts the python-mode dump, with documented exceptions
12	conformance read only `status`	asserts the full record incl. `preserved`/`value`
13	no `userpass` Scope variant	added; Redis round trip now covers 4 scopes
8, 9, 11, 14	docs	number-coercion note moved to the publishing page and stated as a rule; 3 removed `Field` kwargs + silently-ignored `class Config` keys documented; README's three unsupported claims corrected

Verification

746 passed, 1 skipped. flake8 gate and pre-commit clean. Every fix verified red-before / green-after where that applies — the new corpus cases fail without SerializeAsAny, the client routes fail without the handler, the round-trip reports 1 and 2 correctly.

One pre-existing failure, NOT from this PR

data_updater_test.py::test_data_updater_with_report_callback fails on my machine. Before attributing it I stashed every change and ran it on origin/master under pydantic 1.10.26 — it fails there too. It's timing-sensitive (a 1s sleep waiting for a callback to land) and CI has been green on it throughout, including on the commit before these changes. Environmental, not ours. Flagging rather than quietly deselecting it.

Two things I did not do

Finding 2's stronger form — storing the expected v1/v2 value pair and suppressing only an exact match. Assertion-scoping closes the hole you found; the value-pair form closes a smaller residual one. Say the word and I'll add it now rather than leave it as a follow-up.
Pinning an expected module count in the sweep (finding 4). The explicit failure list gives a better message, and a count goes stale on every legitimate file addition.

PR is approved and, from my side, ready to cut as 0.10.0-rc once CI is green on this.

…tion The second half of review finding 2, which I had deferred as "a smaller residual hole". On reflection that was the wrong call: keying an exemption by (case, assertion, path) alone still let ANY value through at that coordinate - including the field being DROPPED, which is precisely the shape of the `periodic_update_interval` bug the corpus was extended to catch in the same review. `_diff` now returns structured `Delta` records carrying the values rather than pre-rendered strings, and each `ACCEPTED_DELTAS` entry carries the v1 and v2 values it documents. A diff is suppressed only when both match. Rendering moved to `Delta.render()` so the failure messages are unchanged. Demonstrated at an exempted coordinate (http_fetcher_config / dict / method): suppressed the documented transition FAILS a different enum member FAILS a wrong plain value FAILS the field dropped by v2 Two new meta-tests keep it that way: * `test_exemption_suppresses_only_its_documented_transition` - per entry, asserts it suppresses what it documents AND that it still fails a third value, a dropped field and an added field. An entry that suppresses more than its transition is a blanket hole, and one that suppresses nothing is inert; both now fail. * `test_every_exemption_is_still_needed` - an entry that no longer matches any difference at head is stale, and a stale exemption is a hole waiting for a future change. That second test immediately earned itself by catching two entries I had just added on a wrong assumption: I exempted both `PolicyStoreDetails` enums, assuming they behaved alike. They do not. `force_enum` is declared on `type` alone, so when the fields are SET, `use_enum_values` unwraps `auth_type` and nothing restores it - it matches v1 and needs no exemption. Both differ only in the *_defaults case, where neither field is validated so `use_enum_values` never fires at all. The distinction is now recorded in the table rather than papered over by an exemption that happened to be harmless today. The same strictness also caught a wrong literal while writing it: I guessed the v2 rendering of `AnyHttpUrl` and the test rejected it, which is the behaviour being added. Unit suite: 769 passed, 1 skipped. flake8 gate clean, pre-commit clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Zivxx · 2026-09-21T23:12:23Z

Finding 2's stronger form, done — `1307ac7`

@zeevmoney I deferred the value-pair half of your suggestion as "a smaller residual hole". That was the wrong call, and the reason is one you'd already written a few threads over: matching on path alone let any value through at an exempted coordinate — including the field being dropped. That is exactly the shape of the periodic_update_interval bug you found in the same review. So the residual hole was the same class as the finding sitting next to it.

_diff now returns structured Delta records carrying values instead of pre-rendered strings, and each entry carries the v1/v2 pair it documents. Suppressed only when both match:

exempted coordinate: (http_fetcher_config, dict, method)
  suppressed   the documented transition
  FAILS        a different enum member
  FAILS        a wrong plain value
  FAILS        the field dropped by v2

Two meta-tests keep it honest: one asserts per entry that it suppresses its own transition and still fails a third value / dropped field / added field, so an entry can be neither a blanket hole nor inert; the other fails on any entry that no longer matches a real difference, since a stale exemption is a hole waiting for a future change.

It caught me twice while I was writing it

Both worth recording, because they're the argument for the strictness:

I guessed the v2 rendering of AnyHttpUrl and the test rejected it. Under the old path-only matching that guess would have been silently accepted forever.
The staleness test caught two entries I'd just added on a wrong assumption. I exempted both PolicyStoreDetails enums, assuming they behaved alike. They don't — force_enum is on type alone, so when the fields are SET, use_enum_values unwraps auth_type and nothing restores it, meaning it matches v1 and needs no exemption at all. Both differ only in the _defaults case, where neither field is validated so use_enum_values never fires. That distinction is now in the table instead of being papered over by an exemption that happened to be harmless today.

That second one is the better argument for your suggestion than anything I could have reasoned about in advance: an over-broad exemption doesn't just hide future regressions, it hides the fact that you didn't understand the current behaviour.

769 passed, 1 skipped. flake8 and pre-commit clean.

That closes both halves of finding 2 and everything else from round two. The only thing still open on my side is the pre-existing data_updater_test.py::test_data_updater_with_report_callback, which fails on my machine and on origin/master under pydantic 1.10.26, and passes in CI.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

The `<3.13` ceiling came from PR #924 (f6c3d83, 2026-08-18), not #959, and has already shipped in 0.9.9. Its stated basis was the CPython `concurrent.futures.thread` internals that opal-server's `_DaemonThreadPoolExecutor` mirrors, and "opal-client capped to match opal-common". Both parts were re-checked on 3.13.14: - the internals mirror in opal_server/git_fetcher.py takes its guarded path on 3.13.14 (signature and attribute shape identical to 3.11), so the original rationale no longer holds on its own; - the only thing that actually fails to install on 3.13 is opal-server's `pygit2>=1.14.1,<1.15` - no cp313 wheel exists before pygit2 1.16.0, and the 1.14.x sdist needs libgit2 1.7 headers. That dependency is opal-server's alone; neither opal-common nor opal-client uses it. A full resolve of the shared + opal-common + opal-client requirements on 3.13 finds no other blocker (every pin has a 3.13-usable wheel), and all three packages pass their suites on 3.13.14 - 770 passed - when opal-server is given a cp313 pygit2. So the ceiling is metadata for the two packages that don't pull pygit2, and real for the one that does. This relaxes opal-common and opal-client to `<3.14` and adds the 3.13 classifier. opal-server stays at `<3.13` until pygit2 is bumped, which is a behaviour change (the PR3 timeout comments reason about the pinned libgit2) and belongs in its own PR. The two must move together: opal-client pins opal-common by exact version. 3.14 is untested, hence the cap. Why now: the PDP runs `python:3.13-alpine` (PER-15358 - three CPython CVEs with no 3.12 backport) and cannot downgrade. pip refuses the current metadata outright on 3.13 ("requires a different Python: 3.13.14 not in '<3.13,>=3.10'"), so this ceiling is the critical-path blocker for the PDP taking any pydantic-v2 opal-client at all. CI gets a 3.13 leg that installs only the two relaxed packages, WITH PIP - deliberately. uv does not enforce Requires-Python for local path sources and would have passed this leg even with the old `<3.13` metadata; pip is what the Dockerfiles and this workflow use, and it does enforce it. Three test modules that need an importable opal-server are ignored on that leg with the reason stated inline. `enum_inventory_test.py` needed to cope with an absent package rather than fail collection: a swept package whose distribution is not installed is recorded in NOT_INSTALLED, distinct from IMPORT_FAILURES (installed but broken - still a failure). A new guard test asserts nothing importable can ever land in NOT_INSTALLED and that every swept package is either walked or recorded absent, so the absent-package path cannot become a quiet way to shrink the sweep. On a full install NOT_INSTALLED is empty; on the 3.13 leg it is exactly `opal_server.scopes`. Verified: - 3.13.14, common+client only, pip-installed, as CI will run it: 366 passed - 3.11 full install, whole suite: 770 passed, 1 skipped - flake8 gate clean, pre-commit clean on all files - opal-server/setup.py untouched Also surfaced, not fixed here: `opal_common/sources/api_policy_source.py:22` imports `opal_server.config` at module level, so opal-common installed alone cannot import that module. Pre-existing, unrelated to this change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

The `<3.13` ceiling came from PR #924 (f6c3d83, 2026-08-18), not #959, and has already shipped in 0.9.9. Its stated basis was the CPython `concurrent.futures.thread` internals that opal-server's `_DaemonThreadPoolExecutor` mirrors, and "opal-client capped to match opal-common". Both parts were re-checked on 3.13.14: - the internals mirror in opal_server/git_fetcher.py takes its guarded path on 3.13.14 (signature and attribute shape identical to 3.11), so the original rationale no longer holds on its own; - the only thing that actually fails to install on 3.13 is opal-server's `pygit2>=1.14.1,<1.15` - no cp313 wheel exists before pygit2 1.16.0, and the 1.14.x sdist needs libgit2 1.7 headers. That dependency is opal-server's alone; neither opal-common nor opal-client uses it. A full resolve of the shared + opal-common + opal-client requirements on 3.13 finds no other blocker (every pin has a 3.13-usable wheel), and all three packages pass their suites on 3.13.14 - 770 passed - when opal-server is given a cp313 pygit2. So the ceiling is metadata for the two packages that don't pull pygit2, and real for the one that does. This relaxes opal-common and opal-client to `<3.14` and adds the 3.13 classifier. opal-server stays at `<3.13` until pygit2 is bumped, which is a behaviour change (the PR3 timeout comments reason about the pinned libgit2) and belongs in its own PR. The two must move together: opal-client pins opal-common by exact version. 3.14 is untested, hence the cap. Why now: Python 3.13 has been the current stable release since October 2024, and downstream projects have moved to it - often for CPython security fixes with no 3.12 backport. pip refuses both packages on it outright ("requires a different Python: 3.13.14 not in '<3.13,>=3.10'") for a cap whose stated reason no longer applies and whose real cause lives in opal-server alone. A project that embeds opal-client on 3.13 has no path to any 0.10.x release until this moves. CI gets a 3.13 leg that installs only the two relaxed packages, WITH PIP - deliberately. uv does not enforce Requires-Python for local path sources and would have passed this leg even with the old `<3.13` metadata; pip is what the Dockerfiles and this workflow use, and it does enforce it. Three test modules that need an importable opal-server are ignored on that leg with the reason stated inline. `enum_inventory_test.py` needed to cope with an absent package rather than fail collection: a swept package whose distribution is not installed is recorded in NOT_INSTALLED, distinct from IMPORT_FAILURES (installed but broken - still a failure). A new guard test asserts nothing importable can ever land in NOT_INSTALLED and that every swept package is either walked or recorded absent, so the absent-package path cannot become a quiet way to shrink the sweep. On a full install NOT_INSTALLED is empty; on the 3.13 leg it is exactly `opal_server.scopes`. Verified: - 3.13.14, common+client only, pip-installed, as CI will run it: 366 passed - 3.11 full install, whole suite: 770 passed, 1 skipped - flake8 gate clean, pre-commit clean on all files - opal-server/setup.py untouched Also surfaced, not fixed here: `opal_common/sources/api_policy_source.py:22` imports `opal_server.config` at module level, so opal-common installed alone cannot import that module. Pre-existing, unrelated to this change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

@zeevmoney

* fix(deps): admit Python 3.13 for opal-common and opal-client The `<3.13` ceiling came from PR #924 (f6c3d83, 2026-08-18), not #959, and has already shipped in 0.9.9. Its stated basis was the CPython `concurrent.futures.thread` internals that opal-server's `_DaemonThreadPoolExecutor` mirrors, and "opal-client capped to match opal-common". Both parts were re-checked on 3.13.14: - the internals mirror in opal_server/git_fetcher.py takes its guarded path on 3.13.14 (signature and attribute shape identical to 3.11), so the original rationale no longer holds on its own; - the only thing that actually fails to install on 3.13 is opal-server's `pygit2>=1.14.1,<1.15` - no cp313 wheel exists before pygit2 1.16.0, and the 1.14.x sdist needs libgit2 1.7 headers. That dependency is opal-server's alone; neither opal-common nor opal-client uses it. A full resolve of the shared + opal-common + opal-client requirements on 3.13 finds no other blocker (every pin has a 3.13-usable wheel), and all three packages pass their suites on 3.13.14 - 770 passed - when opal-server is given a cp313 pygit2. So the ceiling is metadata for the two packages that don't pull pygit2, and real for the one that does. This relaxes opal-common and opal-client to `<3.14` and adds the 3.13 classifier. opal-server stays at `<3.13` until pygit2 is bumped, which is a behaviour change (the PR3 timeout comments reason about the pinned libgit2) and belongs in its own PR. The two must move together: opal-client pins opal-common by exact version. 3.14 is untested, hence the cap. Why now: Python 3.13 has been the current stable release since October 2024, and downstream projects have moved to it - often for CPython security fixes with no 3.12 backport. pip refuses both packages on it outright ("requires a different Python: 3.13.14 not in '<3.13,>=3.10'") for a cap whose stated reason no longer applies and whose real cause lives in opal-server alone. A project that embeds opal-client on 3.13 has no path to any 0.10.x release until this moves. CI gets a 3.13 leg that installs only the two relaxed packages, WITH PIP - deliberately. uv does not enforce Requires-Python for local path sources and would have passed this leg even with the old `<3.13` metadata; pip is what the Dockerfiles and this workflow use, and it does enforce it. Three test modules that need an importable opal-server are ignored on that leg with the reason stated inline. `enum_inventory_test.py` needed to cope with an absent package rather than fail collection: a swept package whose distribution is not installed is recorded in NOT_INSTALLED, distinct from IMPORT_FAILURES (installed but broken - still a failure). A new guard test asserts nothing importable can ever land in NOT_INSTALLED and that every swept package is either walked or recorded absent, so the absent-package path cannot become a quiet way to shrink the sweep. On a full install NOT_INSTALLED is empty; on the 3.13 leg it is exactly `opal_server.scopes`. Verified: - 3.13.14, common+client only, pip-installed, as CI will run it: 366 passed - 3.11 full install, whole suite: 770 passed, 1 skipped - flake8 gate clean, pre-commit clean on all files - opal-server/setup.py untouched Also surfaced, not fixed here: `opal_common/sources/api_policy_source.py:22` imports `opal_server.config` at module level, so opal-common installed alone cannot import that module. Pre-existing, unrelated to this change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * test(wire_compat): bound the tolerated absent package to the 3.13-leg gap `test_sweep_skips_only_genuinely_absent_packages` checked that a package recorded in NOT_INSTALLED was truly absent, but never that it was ALLOWED to be absent. So an environment missing opal-client would drop three swept packages and still report 14 passed - exactly the silent shrink the NOT_INSTALLED mechanism was added to prevent, and which the docstring claimed was closed. Add ALLOWED_ABSENT = {"opal_server.scopes"} - the one package the CI 3.13 leg is designed to omit - and assert every absent entry is in it. Rewrite the docstring to describe what is now enforced. Verified by uninstalling opal-client from a 3.13 environment: the guard now fails naming opal_client.engine, opal_client.policy_store and opal_client.policy, where before it passed. Also teach the two remaining import_module sites about absent distributions: `_covered_by_corpus` would have turned a future opal_server.* corpus case into a whole-file collection error on the 3.13 leg, and `test_exclusions_still_exist` would have reported an opal_server.* exclusion as "no longer exists". Both now skip a known-absent distribution; the full-install legs still check it. Addresses review comments: - #960 (comment) (@zeevmoney) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * ci(tests): drop the dead tests/requirements.txt conditional `./tests/requirements.txt` has never existed in this repo, so the line was a no-op that suggested a test-requirements file exists. It was carried into the rewritten install block instead of being removed. `requirements.txt` does exist, so its guard goes too. Addresses review comments: - #960 (comment) (@zeevmoney) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(setup): say why opal-server keeps <3.13 while common and client move Two problems with the python_requires comments. opal-common's said the executor guard "is verified to pass on 3.13". That rests on a one-off manual check: the guard and its tests live in opal-server, which cannot install on 3.13, so no CI leg can maintain the claim. Drop it as justification and lean on what is durable - this package depends on neither the executor nor pygit2, and the CI 3.13 leg installs it with pip, which enforces the field. The manual check is attributed to #960 as not CI-covered. Also note that on 3.14 the mirrored internals have already changed shape (`_worker` takes 3 parameters; `_initializer` and `_initargs` are gone), which is part of why <3.14 is the right cap. opal-client's said the two "must move together" because it pins opal-common by exact version - but opal-server carries the identical pin and stays at <3.13, so read literally the rule forbids what this PR does. Say what actually distinguishes them: no pygit2 dependency. Addresses review comments: - #960 (comment) (@zeevmoney) - #960 (comment) (@zeevmoney) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs: state the per-package Python ceiling in the install guides Both python-package guides still said "3.10 or greater (and below 3.13)", which is now wrong for opal-client and opal-common and does not describe the split. State it per package, and warn about the footgun the split creates: on 3.13 `pip install opal-server` does not fail, it silently resolves to 0.9.6 - the newest release whose metadata admits 3.13, since 0.9.7 and 0.9.8 were never published to PyPI and 0.9.9 declares <3.13. Addresses review comments: - #960 (review) (@zeevmoney) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>

- The starlette <1 cap is gone (raised to >=1.3.1,<2 by #959), so drop it from the downstream note; ddtrace <4 and the gitpython 3.1.32 floor still stand. - The workflow action-version inventory now matches master (no checkout@v4; login-action@v2 added). - Drop an unverifiable claim about GitHub's default cooldown, the OPA version pin that #906 changes, and prefix-development on github-actions (no dev deps). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI lite review requested due to automatic review settings September 14, 2026 14:14

Copilot started reviewing on behalf of EliMoshkovich September 14, 2026 14:14

View session

Copilot AI reviewed Sep 14, 2026

EliMoshkovich requested review from dshoen619 and zeevmoney September 14, 2026 14:19

zeevmoney requested changes Sep 15, 2026

View reviewed changes

Zivxx and others added 2 commits September 15, 2026 14:12

Copilot AI review requested due to automatic review settings September 15, 2026 12:22

Copilot started reviewing on behalf of EliMoshkovich September 15, 2026 12:23

View session

Copilot AI reviewed Sep 15, 2026

Copilot AI review requested due to automatic review settings September 15, 2026 12:39

Copilot started reviewing on behalf of EliMoshkovich September 15, 2026 12:39

View session

Copilot AI reviewed Sep 15, 2026

Copilot AI review requested due to automatic review settings September 16, 2026 15:31

Copilot started reviewing on behalf of EliMoshkovich September 16, 2026 15:32

View session

Copilot AI reviewed Sep 16, 2026

Copilot AI review requested due to automatic review settings September 16, 2026 15:37

Copilot started reviewing on behalf of EliMoshkovich September 16, 2026 15:37

View session

Copilot AI reviewed Sep 16, 2026

Copilot AI review requested due to automatic review settings September 17, 2026 09:21

Copilot started reviewing on behalf of EliMoshkovich September 17, 2026 09:22

View session

Copilot AI reviewed Sep 17, 2026

Zivxx requested a review from zeevmoney September 17, 2026 11:07

zeevmoney approved these changes Sep 17, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 21, 2026 21:01

Copilot started reviewing on behalf of EliMoshkovich September 21, 2026 21:01

View session

Copilot AI reviewed Sep 21, 2026

Copilot AI review requested due to automatic review settings September 21, 2026 23:12

Copilot started reviewing on behalf of EliMoshkovich September 21, 2026 23:12

View session

Copilot AI reviewed Sep 21, 2026

Zivxx merged commit 904b5fd into master Sep 22, 2026
11 of 12 checks passed

Zivxx deleted the eli/per-15885-opal-issue-cve-956 branch September 22, 2026 02:49

Zivxx mentioned this pull request Sep 22, 2026

fix(deps): admit Python 3.13 for opal-common and opal-client #960

Merged

EliMoshkovich mentioned this pull request Oct 1, 2026

ci: add Dependabot version updates (PER-15358) #957

Closed

This was referenced Oct 22, 2026

fix(opal-server): configure logging first in the gunicorn master (0.9.x) #975

Merged

fix(opal-server): cut per-update and policy-push INFO lines (0.9.x) #982

Merged

Conversation

EliMoshkovich commented Sep 14, 2026 • edited by Zivxx Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixes Issue

The CVE

Exposure: none

Why this isn't a one-line bump

Changes proposed

Dependencies (packages/requires.txt)

Python floor

pydantic v1 → v2

Fixes from review

Breaking changes

Validation

Note on scope

Uh oh!

linear-code Bot commented Sep 14, 2026

Uh oh!

netlify Bot commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for opal-docs canceled.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

zeevmoney left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dshoen619 commented Sep 15, 2026

Review: verified A/B against a pydantic v1 baseline

Blocking

1. Inline OPA no longer starts on Python 3.11+

2. GET /stats returns 500 on any multi-worker server

3. 422 responses now echo credentials

4. The CVE floor still admits two HIGH advisories

High

5. The new config shim silently blanks aliased third-party configs

6. ttl breaks mixed-version fleets — servers must upgrade first

Worth fixing

Two framing notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Zivxx commented Sep 15, 2026

Review addressed — 2 commits, 51 new regression tests

Two findings only one of you had

The one thing I'd push back on

Where a finding was bigger than described

The two off-diff LOWs from @zeevmoney's review body

Verification

Accepted framing, and what's next

Uh oh!

Zivxx commented Sep 15, 2026

Wire-compat corpus added — and it found three more deltas

It found three things neither review caught

Why python mode is asserted separately

The harness catches what you both found by hand

Where this sits in the plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

EliMoshkovich commented Sep 14, 2026 •

edited by Zivxx

Loading

Dependencies (`packages/requires.txt`)

netlify Bot commented Sep 14, 2026 •

edited

Loading

2. `GET /stats` returns 500 on any multi-worker server

6. `ttl` breaks mixed-version fleets — servers must upgrade first

Round two addressed — `c116153`

Finding 2's stronger form, done — `1307ac7`