Skip to content

fix: stop disclosing the seller's floor price in chat and on every counter-offer - #97

Merged
Sirajmx merged 2 commits into
mainfrom
fix/chat-and-counter-floor-leak
Oct 1, 2026
Merged

Sirajmx merged 2 commits into
mainfrom
fix/chat-and-counter-floor-leak

Conversation

@atc964

@atc964 atc964 commented Sep 17, 2026 •

Copy link
Copy Markdown
Collaborator

Security-relevant — please do not merge casually

This is an information-disclosure fix on a public reference implementation, and it removes a field from a response body on the normal negotiation path. Please review the wire change deliberately rather than rubber-stamping.

This is the second of at least two PRs in this class. #95 fixed the same defect class on GET /proposals/{proposal_id}/negotiation. That PR's own sweep found more surfaces, and this PR fixes the two that are worst by reachability. Others remain — see "Still open" below. #95 and this PR are independent and can merge in either order.

The defect

Two surfaces disclosed the seller's floor price, and neither is an obscure route.

1. The chat interface said it out loud. interfaces/chat/main.py:544, on a negotiation REJECT, emitted the literal sentence:

Our floor for this inventory is $33.11 CPM. Would you like to explore other packages that might fit your budget?

formatted from history.floor_price. The seller told its counterparty its own floor, in prose, mid-negotiation.

2. Every counter-offer carried it. flows/proposal_handling_flow.py:821 put "floor_price": product.floor_cpm into state.counter_terms. counter_terms is returned to the buyer on every counter-offer (it is where the buyer reads proposed_price and negotiation_id), so this crossed the wire on the normal negotiation path — not on a read someone had to go looking for. Observed payload before the fix:

{'proposed_price': 33.11, 'floor_price': 33.11, 'max_impressions': 1000000,
 'reason': '...', 'negotiation_id': 'neg-2602e115', 'round_number': 1, 'action': 'counter'}

A counterparty who knows the floor concedes nothing above it, so either leak makes the seller's negotiating position structurally worthless. floor_price, base_price, strategy and NegotiationLimits (max_rounds) are the four fields the shared Negotiation primitive excludes deliberately — from RECONCILIATION.md in iab-agentic-primitives: "floor_price, base_price, strategy, and NegotiationLimits are deliberately excluded — they are the seller's internal guardrails and must never cross the wire."

Because this is donated reference code, the pattern propagates to every implementer who copies it.

What changed

1. The chat REJECT is now a walk-away with no number in it. House voice kept, consistent with the neighbouring FINAL_OFFER branch, and it still offers the buyer a next step.

It also no longer interpolates the engine's REJECT rationale. That rationale has a max-rounds variant reading "Maximum 5 rounds reached", which publishes the remaining concession budget in prose. Dropping the interpolation is local to the branch being rewritten and removes a second guardrail disclosure from the same message.

2. floor_price is removed from counter_terms. Nothing else in the dict changed.

3. Both sites carry a comment saying why, naming the four fields, so the next person to extend either payload sees the constraint at the point of edit.

4. The engine now emits a second, buyer-facing rationale, and every outbound surface sends only that one — see "The prose leak is closed here too" below. This closes the known gap the first commit recorded instead of shipping it.

Cross-repo check before removing the field — done first, not after

counter_terms is read by the buyer agent, so removing a key from it could have broken the buyer. Checked against IABTechLab/buyer-agent at origin/main before touching it:

  • ad_buyer/orchestration/multi_seller.py:1422-1434 reads exactly two keys: counter_terms["proposed_price"] and counter_terms["negotiation_id"]. Both untouched.
  • No buyer code reads counter_terms["floor_price"]. No hits anywhere in src/.
  • The buyer's only floor_price consumer is ad_buyer/media_kit/client.py:110, in _parse_package_detail, which parses GET /media-kit/packages/{id}. That is a different seller surface (AuthenticatedPackageView.floor_price) and is deliberately not touched here — removing it would break the buyer's media-kit client, and it needs the public-versus-private ruling described below.

So this is safe for the buyer as it stands. A test in the new file pins the two keys the buyer does read, so a later tidy-up of counter_terms cannot quietly remove them.

Tests

tests/unit/test_counter_and_chat_floor_disclosure.py — 25 tests in four sections.

Sections 1 and 2 pin the two sites: the walk-away contains neither the floor value, nor the base price, nor the word "floor", nor the round budget, and is still a usable message; counter_terms has no floor_price key and still carries what the buyer reads.

Section 3 is the part that matters for the next leak: a by-field-name sweep. It recurses to any depth and asserts that neither path carries floor_price, base_price, strategy or max_rounds as a field name — over counter_terms, over the whole buyer-facing proposal response, and over the chat negotiation envelope. A per-site test catches today's two leaks; this catches the next guardrail someone adds to either payload without anyone having to remember to write a new test. One test deliberately asserts that the internal NegotiationHistory does contain all four, so the sweep is proven non-vacuous rather than passing because it is looking at nothing.

This complements #95 rather than duplicating it: that file pins one route's response shape and its auth gate; this one pins the chat text and the counter payload.

Section 4 covers the two-rationale design (below): per-action engine assertions that the buyer-facing rationale carries no floor value, no floor/minimum/strategy/concession wording and no N/M round-budget pattern while the internal rationale still does (so the clean checks are proven non-vacuous), plus counter_terms["reason"] on a below-floor offer, the chat COUNTER/FINAL_OFFER prose, the REST counter response, and the terminal-round mapper's old-round fallback.

Negative control (first commit). With both structured leaks restored from origin/main, 6 of the original 11 fail, each reporting the actual leaked value — the floor 33.11 in the chat text, Maximum 5 for the round budget, and ['floor_price'] in the counter_terms payload. The 5 that still pass correctly should: the walk-away-usability test, the buyer-keys test, the non-vacuousness test, the chat envelope test (that envelope never leaked), and the name sweep over the walk-away text — prose says "our floor", not floor_price, which is exactly why sections 1 and 2 assert on the value and the wording rather than on the field name. The fix was then re-applied and the restored diff verified byte-identical to the pre-revert diff (matching sha256).

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

The prose leak is closed here too: two rationales, internal and buyer-facing

The first commit recorded a known gap: the engine's rationale strings still stated the floor, the strategy and the round budget in prose — "Buyer offer $25.00 is below our floor. Countering at the floor price $33.11 CPM — the minimum viable price for this inventory (round 1/5).", "Counter at $30.00 CPM (collaborative strategy, round 1/5).", "Final offer at our floor price $X CPM. We cannot go lower.", "...our maximum concession of 15%." — and that sentence shipped to the buyer as counter_terms["reason"], in the chat COUNTER/FINAL_OFFER text, and on the REST negotiation responses. A leak moved from a named field into a prose string is not fixed.

The second commit closes it structurally rather than by rewording. NegotiationRound (a seller-local model, not the shared contract) now carries two rationales:

  • rationale — internal, unchanged wording. Still names the floor, the strategy, the round budget and the concession cap, because it exists for logs, the stored negotiation history and audit. Nothing is lost on the seller's side.
  • buyer_rationale — buyer-facing, constructed deliberately. Explains the action without stating the seller's price in prose (the structured seller_price/proposed_price carries the number, unlabeled — on a below-floor round the counter price is the floor, so any price in the sentence would be the floor value), and without naming the floor, the strategy, the concession budget or the round limit. Example: "We cannot accept $25.00 CPM for this inventory. We have countered at a price that reflects its value."

Every outbound surface sends only the buyer-facing one: counter_terms["reason"], the chat COUNTER and FINAL_OFFER text, the REST counter response (both the legacy /proposals/{id}/counter dict and the canonical /api/v1/negotiations/messages mapping), and the terminal-round mapper — which falls back to empty, never to the internal rationale, for rounds persisted before the field existed. The flow's truthful volume note (available-impressions shortfall) is appended to both rationales, since it disclosed nothing.

The fix is structural: a future engine string cannot leak, because the buyer-facing sentence is authored as a separate value rather than filtered out of the internal one. Engine pricing arithmetic — concession math, floor clamping, gap-split, acceptance conditions — is byte-identical in behavior; only strings and the round model changed, and the full pre-existing suite passes untouched.

Still open on this class, deliberately out of scope

Each of these needs a deliberate public-versus-private ruling rather than a reflexive patch, and one of them (the package view) has a live buyer consumer that a naive removal would break:

  • GET /products and GET /products/{id} are unauthenticated and return Product.ext.floor_cpm (contract_mappers.py:504)
  • AuthenticatedPackageView.floor_price (models/media_kit.py:250) exposes a package floor to any authenticated buyer — and the buyer agent's media-kit client reads it
  • the MCP surface returns floor_cpm per product (mcp_server.py:388) and a default floor in setup status (:237, :314)
  • GET /api/v1/rate-card is unauthenticated while PUT on the same path is operator-gated, though RECONCILIATION.md marks rate cards as never public

Operator impact

Operators running an earlier build should assume their product floors were readable by any buyer who received a counter-offer, and by any buyer who was rejected in chat. Both are ordinary negotiation outcomes, so the realistic exposure is "every buyer who negotiated", not "someone who probed an unusual endpoint". Called out in the CHANGELOG for that reason.

No version numbers changed.

Sirajmx
Sirajmx previously approved these changes Sep 23, 2026

@Sirajmx Sirajmx left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reproduced both leaks live, independently, on current main: the chat REJECT branch genuinely
outputs "Our floor for this inventory is $33.11 CPM... Maximum 5 rounds reached..." — floor
value and round budget both present, built from a hand-constructed history/round, not the PR's
own fixture. Verified the fix live on a clean merge onto current main: the same inputs through
the fixed formatter now return "...we've gone as far as we can on price here..." — no floor
value, no word "floor" anywhere. New test file: 25 passed. Full suite: 1592 passed. Ruff clean.

One thing checked hard before approving, since it looked like a possible miss: the shared
NegotiationRound primitive (used by the canonical POST /api/v1/negotiations/messages route)
genuinely declares concession_pct/cumulative_concession_pct as real fields — confirmed via
NegotiationRound.model_fields and that extra: ignore doesn't drop them, so they do reach that
wire, the same base_price-reconstruction math #95 fixed elsewhere. Not a miss in #97, though —
#95's own body already names this exact gap and calls it "genuinely contract-sanctioned...
narrowing those is a contract discussion, not a fix here" (shared-library spec issue, correctly
out of scope for either PR). #97's actual job on this route, the rationale string content, is
correctly fixed — counter_proposal() now populates it from buyer_rationale, so the leaked
field name is a known/disclosed gap, but the prose itself is safe.

Cross-repo, re-verified independently rather than trusting the PR's own claim: multi_seller.py
reads exactly counter_terms.get("proposed_price") and .get("negotiation_id"), nothing else.
The other floor_price reads in buyer-agent are unrelated — a different connector's raw deal
object, and media_kit/client.py's genuinely separate AuthenticatedPackageView surface, which
is exactly why the PR correctly leaves that one untouched. Approving.

…unter-offer

Two information-disclosure leaks on paths a buyer actually reaches.

1. interfaces/chat/main.py stated the floor outright in prose on a
   negotiation REJECT -- "Our floor for this inventory is $X CPM",
   formatted from history.floor_price. The seller told its counterparty
   its own floor, mid-negotiation. The branch is now a walk-away that
   discloses no number. It also no longer interpolates the engine's
   REJECT rationale, whose max-rounds variant reads "Maximum N rounds
   reached" and publishes the remaining concession budget.

2. flows/proposal_handling_flow.py put "floor_price": product.floor_cpm
   into state.counter_terms, which is returned to the buyer on EVERY
   counter-offer -- so the floor crossed the wire on the normal
   negotiation path, not on an obscure read. The key is removed.

A counterparty who knows the floor concedes nothing above it. These are
two of the four fields the shared Negotiation primitive excludes
deliberately (floor_price, base_price, strategy, NegotiationLimits), per
RECONCILIATION.md.

Removing a key from counter_terms is a wire change, so the buyer was
checked first at origin/main: ad_buyer/orchestration/multi_seller.py
reads only counter_terms["proposed_price"] and
counter_terms["negotiation_id"], and no buyer code reads
counter_terms["floor_price"]. The buyer's only floor_price consumer is
media_kit/client.py, which parses GET /media-kit/packages/{id} -- a
different surface, untouched here.

tests/unit/test_counter_and_chat_floor_disclosure.py (11 tests) pins the
two sites and adds a by-field-name sweep: neither path may carry
floor_price, base_price, strategy or max_rounds as a field name at any
nesting depth, so the next guardrail added to either payload fails
without anyone remembering to write a test. Complements the
route-specific test in #95 rather than duplicating it.

Known remaining gap, recorded not half-built: the negotiation engine's
rationale strings state the floor and the strategy in prose (e.g.
"Countering at the floor price $X CPM ... (round 1/5)") and reach the
buyer via counter_terms["reason"]. Changing that is a change to the
engine's wording, on a different surface, and is not made here.
… strategy, or the round budget

The negotiation engine's rationale strings stated the seller's floor,
strategy and round budget in prose, and that sentence shipped to the
buyer as counter_terms["reason"], in the chat counter/final-offer text,
and on the REST negotiation responses — so the floor was still derivable
in prose on the normal counter path even with floor_price removed from
the structured payload (the known gap this PR recorded).

The engine now emits both: the internal rationale is byte-identical to
before and stays in logs and the stored negotiation history for audit,
and a deliberately constructed buyer_rationale explains the action
without stating the seller's price in prose (the structured seller_price
carries the number unlabeled) and without naming the floor, the
strategy, the concession budget or the round limit. Every outbound
surface — counter_terms["reason"], the chat COUNTER/FINAL_OFFER text,
the REST counter response, and the terminal-round mapper — sends only
the buyer-facing one; the mapper falls back to empty for rounds stored
before the field existed, never to the internal rationale.

Engine pricing arithmetic (concession math, floor clamping, gap-split,
acceptance conditions) is untouched; NegotiationRound is a seller-local
model, so no shared-contract change. 14 new tests extend the existing
disclosure file: per-action engine assertions that the buyer rationale
carries no guardrail while the internal one still does, the flow's
reason, the chat prose, the REST response, and the mapper fallback.
@Sirajmx
Sirajmx merged commit b405a61 into main Oct 1, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants