[atc964]

⚠️ Security-relevant — please do not merge casually

Information disclosure on a public reference implementation. This adds an auth requirement to three routes, scopes their results to the caller, changes the response shape of all five, and removes a field from a request body. Please review the wire change and the auth decisions deliberately rather than rubber-stamping.

Fixes the unauthenticated change-request disclosure (P0, tracked internally).

The defect

GET /api/v1/change-requests took optional auth (_get_optional_api_key_record, which returns None for anonymous callers), required no filter (order_id and status are both Optional), and declared no response_model. With no query params the service built an empty filter and asked storage for everything.

Each record carries rollback_snapshot, written at order_service.py:365 as order.copy() — the full order: OrderStateMachine.to_dict() (order_id, status, and the audit_log with every transition's actor and reason) plus deal_id, quote_id, created_at and metadata.

So one anonymous unfiltered GET returned every tenant's order audit trail. Here is the actual body, from the negative control below:

{"change_requests":[{... "rollback_snapshot":{"order_id":"ORD-VICTIM","status":"booked",
"deal_id":"DEMO-VICTIM","quote_id":"QT-VICTIM-SECRET","metadata":{"campaign":"victim-spring-2026"},
"audit_log":{"order_id":"ORD-VICTIM","transitions":[{"from_status":"draft","to_status":"booked",
"actor":"human:victim-trader@agency.example",
"reason":"renegotiated after Q3 shortfall call with CFO"}]}}}],"count":1}

POST /api/v1/change-requests was anonymous for the same reason and took requested_by straight off the wire (schemas.py:364, requested_by: str = "system"), writing it into the persisted record verbatim. Nothing downstream decides on requested_by — approval routing is severity-only and review/apply are operator-gated — so this is not privilege escalation. It is worse in the place that matters: requested_by is exactly what a human reviewer looks at when deciding whether to approve, so the practical attack is social-engineering the approval ("requested_by": "human:cfo@publisher.example"), and the audit trail that would catch it was the thing being disclosed.

Note the contrast on the same router: review and apply already used _require_operator_api_key_record. Only create and the two reads were open.

What changed

1. Authentication. Create, list and read-by-id now take require_api_key_record (anonymous → 401) — the same dependency the approvals router uses. Review/apply keep their stricter operator gate. A buyer key is sufficient for the buyer-facing three; this is not a promotion to operator-only.

2. Ownership scoping — because authentication alone is not enough. An authenticated buyer would still read every other buyer's trail. Each record is stamped at creation with an actor derived from the credential, and a buyer caller sees only records carrying its own actor. An operator caller is unscoped: reviewing the queue is the seller's job and needs all of it.

The actor comes from a new auth.dependencies.actor_from_api_key. It deliberately differs from the existing principal_from_api_key: that one anchors to the immutable key_id because an approval audit entry must name the exact credential that decided, whereas this is an ownership key and must survive key rotation, or a buyer would lose sight of records it filed under an earlier key. So it anchors to the strongest stable buyer identifier (agent:<advertiser|agency|seat>), falls back to agent:key:<key_id> only for a credential asserting no buyer identity at all, and gives operators their own operator: namespace so a buyer identifier can never collide with a seller-side actor.

Another actor's record reads 404, not 403. A distinguishable "exists but not yours" would turn the by-id route into an existence oracle over every tenant's change request ids.

3. The wire shape is pinned. All five handlers declare a response_model (ChangeRequestResponse, ChangeRequestListResponse, ChangeRequestApplyResponse). rollback_snapshot is absent from those models by construction, so it cannot reach the wire even if a future consumer serialises the record wholesale.

4. The actor is stamped, not asserted. requested_by is removed from CreateChangeRequestModel and derived from the credential in the router. A body that still sends it is accepted and the value ignored (pydantic drops unknown fields), so this is not a hard break for existing clients — but the value they send no longer does anything.

5. Idempotency keys are namespaced per actor. The key was idempotency:change-request:{order_id}:{idempotency_key}. Without the actor in it, one buyer replaying another's (order_id, idempotency_key) pair would be handed back the other buyer's change request through the replay path, straight past the new scoping. Pinned by a test.

6. The list scope is applied in the service, in Python — not in the storage filter dict. StorageBackend.list_change_requests understands only order_id and status and silently ignores any other key (storage/base.py:260-274), so a requested_by pushed in that way would fail open and return every tenant's records. There is a test asserting the scope is not passed that way.

Deliberately NOT done here

rollback_snapshot is still written. It is genuinely write-only — a repo-wide grep over src/ finds exactly two references, the declaration and the one write, and apply_change_request never reads it. Deleting it is the right call and is the cheapest part of the fix, but it is a persisted-storage-shape change (existing records carry it; both storage backends should be confirmed to deserialize them) and that is beyond the disclosure. This PR closes the wire completely — rollback_snapshot appears in no response on this router — and the deletion is tracked internally as a follow-up.

Tests

tests/unit/test_change_request_disclosure.py (16 tests), in four groups: the auth gate, the ownership scoping, the declared wire shape, and the server-stamped actor. Notable ones:

• the anonymous listing is 401 and the response text contains none of rollback_snapshot, the order id, the secret quote id or the audit-log reason — asserted against raw text, so the values cannot reappear under another key
• an authenticated stranger gets an empty list and a 404 by id, while the owner still gets 200 and its own record (the gate is scoping, not a blanket denial)
• an operator still sees the whole queue
• replaying another buyer's idempotency key does not return that buyer's record
• all five handlers are driven end to end and none emits rollback_snapshot, the audit log, or the quote id — plus a check that the snapshot is still in storage, so the test pins the wire rather than the storage shape
• every route object on the router has a non-None response_model
• a body requested_by of human:cfo@publisher.example does not appear in the response or in the persisted record
• the actor is stable across key rotation, operator actors cannot collide with a buyer identifier, and two identity-less buyer keys do not collapse onto one shared actor
• the service-level scope is not pushed into the storage filter dict

Negative control: with src/ reverted to origin/main (and a local shim standing in for the new actor_from_api_key, which is new code and cannot exist on the old tree), 11 of the 16 fail, reporting the actual leaked values shown above. The 5 that pass are the 3 pure helper unit tests plus "owner still sees its own records" and "operator sees the whole queue" — both of which correctly still pass, since the unguarded route answered everyone. src/ was then restored and the reapplied diff verified byte-identical to the pre-revert diff (matching sha256).

Full unit suite: 1537 passed. ruff check src/ and ruff format --check src/ tests/ both clean.

One note on the local run: tests/unit/test_trust_tier_verification.py::TestNegotiationMessagesCeiling::test_self_asserted_advertiser_identity_is_floored fails when the suite runs against a reused sqlite file, because a stale idempotency row short-circuits the endpoint. It fails identically on a pristine origin/main checkout, so it is pre-existing and unrelated — it is a known local test-environment issue (worktree .env inheritance), tracked internally. With an isolated DATABASE_URL, 1537/1537 pass.

---

The remaining unauthenticated routes

PR #95 listed the other unauthenticated routes and asked for a deliberate
public/private ruling. That ruling was made against one principle: a route is
legitimately public only if what it returns is identical for every caller and
is content the seller intends to publish; an id is not a credential; a state
change is not a read. The per-route classification and the follow-up hardening
work are tracked in the project's internal tracker and are in progress. None of
it blocks this PR, which is complete for the change-request surface on its own.

[Sirajmx]

The most serious of this batch (P0, seller-928). Reproduced the defect live and independently —
not via the PR's own fixture, a hand-built order + change request through the real service layer
— and the pre-fix listing genuinely returns the full rollback_snapshot, near-exact match to the
PR's own example: the victim's quote_id, campaign metadata, and audit-log actor/reason all
present, unauthenticated.

Fix verified on a clean merge onto current main (one trivial CHANGELOG conflict from #94
landing since I last checked, resolved by concatenation): new disclosure test file 16/16 passed,
existing test_change_request.py + test_operator_auth.py 103 passed (the scoping changes don't
break owner/operator behavior), full suite 1583 passed, ruff clean.

Checked the design choices, not just read them: 404-not-403 for another actor's record is right
(a 403 would let status codes enumerate valid ids); scoping applied in Python rather than pushed
into the storage filter dict is the safe choice — confirmed StorageBackend.list_change_requests
silently ignores unknown filter keys, so the naive approach would fail open; the idempotency key
namespaced per actor closes a real replay hole (without it, one buyer replaying another's
(order_id, idempotency_key) pair would get handed the other buyer's record); and
actor_from_api_key correctly anchors to buyer identity rather than key_id, since an ownership
key must survive key rotation while an audit identity should not.

Cross-repo: zero impact, confirmed by grep — buyer-agent has no change-request client code at
all, no hits for change-request, requested_by, or rollback_snapshot anywhere in its src/.

rollback_snapshot itself is still written to storage, deliberately left for a follow-up since
deleting it is a persisted-storage-shape change, not a wire-disclosure fix — confirmed it's
genuinely write-only (exactly two references repo-wide) so that follow-up is low-risk whenever it
lands. Approving.