Reject oversized bid dimensions instead of truncating them

[dhruv8sh]

Summary

• Bid width and height in the mock ad-server mediation path were cast from u64 to u32 with as, which silently wraps values above u32::MAX into small, plausible-looking dimensions that pass the zero-check.
• Use u32::try_from and skip the bid (log at debug) when a dimension doesn't fit, consistent with the existing zero-dimension handling.
• Chose skip over clamp: it matches the existing zero-dimension behavior and doesn't invent a dimension the bidder never sent.

Changes

File	Change
crates/trusted-server-core/src/integrations/adserver_mock.rs	Replace truncating as u32 casts on bid w/h with u32::try_from, skipping the bid on overflow; add tests for oversized and zero/missing dimensions

Closes

Closes #420

Test plan

• cargo test-fastly && cargo test-axum
• cargo clippy-fastly && cargo clippy-axum
• cargo fmt --all -- --check
• JS tests: cd crates/trusted-server-js/lib && npx vitest run
• JS format: cd crates/trusted-server-js/lib && npm run format
• Docs format: cd docs && npm run format
• WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
• Manual testing via fastly compute serve
• Other:

Checklist

• Changes follow AGENTS.md conventions
• No unwrap() in production code — use expect("should ...")
• Uses tracing macros (not println!)
• New code has tests
• No secrets or credentials committed

[ChristianPavilonis]

Summary

The checked conversion correctly prevents oversized mock ad-server dimensions from entering auction consumers. One medium-severity test gap remains: the oversized fixture wraps to zero under the old implementation, so it does not reproduce the original nonzero-truncation defect.

[prk-Jr]

Summary

Reviewed commit 6f95f1d5096240b80fe7c44bb25ddd4c07edb3ea.

The checked conversion correctly rejects overflowing dimensions and preserves ordinary, zero, and missing-dimension behavior. One non-blocking test improvement remains.

Non-blocking

• 🤔 [P2] Exercise a nonzero wrapped value in the overflow regression — see inline at crates/trusted-server-core/src/integrations/adserver_mock.rs:1343.

Validation

All 18 mock-adserver tests pass at the reviewed head. Mutation checks confirm that the original cast passes the current fixture, fails with a u32::MAX + 101 fixture, and the checked conversion passes with that stronger fixture. Scratch source was restored byte-for-byte. These checks establish the test gap; they are not a full release-gate validation of a proposed patch.

CI Status

• browser integration tests: PASS
• integration tests: PASS
• integration tests (Fastly EC lifecycle): PASS
• CodeQL: PASS
• cargo test (ts CLI, native): PASS
• vitest: PASS
• Analyze (actions): PASS
• Analyze (javascript-typescript): PASS
• cargo fmt: PASS (required)
• cargo test (axum native): PASS
• format-typescript: PASS (required)
• cargo check (cloudflare native + wasm32-unknown-unknown): PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• cargo test (cross-adapter parity): PASS
• cargo test: PASS (required)
• CLAUDE.md symlink guard: PASS
• prepare integration artifacts: PASS
• Analyze (rust): PASS
• format-docs: PASS (required)
• Analyze (javascript-typescript): PASS

[aram356]

Summary

The production fix is correct and complete. I swept the repository for the JSON-accessor-to-narrowing-as idiom this PR removes and found zero remaining instances — adserver_mock.rs really was the last one, and Prebid, APS, and the generic OpenRTB path already use checked conversions. Skipping the bid rather than clamping is the right call, and the PR body justifies it well.

The blocking issue is in the test, not the fix: test_parse_mediation_response_skips_oversized_dimensions passes against the unfixed code, so it does not pin the regression it was written for.

1 of the inline comments below carries a one-click GitHub suggestion — use Commit suggestion to apply it as a commit on the PR branch. The remaining comments are prose because they describe follow-ups or trade-offs rather than a mechanical edit.

Blocking

wrench

• Oversized-dimension test passes against the unfixed code — see inline at crates/trusted-server-core/src/integrations/adserver_mock.rs:1343

Non-blocking

thinking / out of scope / nitpick

• Skip-vs-unwrap_or(0) diverges from the cited prebid precedent — see inline at crates/trusted-server-core/src/integrations/adserver_mock.rs:295
• Rejects float-encoded dimensions the shared helper accepts — see inline at crates/trusted-server-core/src/integrations/adserver_mock.rs:301
• Skip logs drop the offending value — see inline at crates/trusted-server-core/src/integrations/adserver_mock.rs:297
• adserver_mock is the only provider not validating dimensions against requested slot formats — see cross-cutting below

Cross-cutting / body-level findings

• Out of scope — adserver_mock is the only bid parser that does not validate dimensions against the requested slot formats. Prebid and the generic OpenRTB path both call resolve_bid_dimensions (crates/trusted-server-core/src/auction/openrtb.rs:195), which rejects with DimensionMismatch; APS checks dimensions.contains(...) (crates/trusted-server-core/src/integrations/aps.rs:671) and compatible_dimensions (aps.rs:1375). adserver_mock.rs:307 only checks width == 0 || height == 0, so it still accepts an in-range but never-requested size such as 1x1 or 9999x9999.

  This is pre-existing and arguably acceptable for a mock provider, so it is not a blocker for this PR. Worth noting that it is the reason this path needed a bespoke dimension check at all: routing it through parse_optional_bid_dimension + resolve_bid_dimensions like the other three providers would delete the hand-rolled logic and make the truncation fix fall out for free. A reasonable follow-up issue.

• Completeness check (informational). Two sweeps over all crates — as_(u64|i64|f64)\(\).*\bas (u|i)(8|16|32)\b and unwrap_or\([0-9]+\) as — returned no hits after this change, so this PR closes the last instance of the pattern rather than one of several.

Verification performed

Scratch-verified in an isolated worktree at 6f95f1d50:

• The PR's two new tests were grafted onto the merge-base commit 6cae7f5da (the unfixed code). Both passed, confirming the oversized test does not detect the bug.
• Changing only the test input to u64::from(u32::MAX) + 101 makes that test fail on the base commit (left: 3, right: 1) and pass on this PR's head — a real regression test.
• With the suggested fix applied: cargo fmt --all -- --check, all six clippy aliases (fastly, axum, cloudflare, cloudflare-wasm, spin-native, spin-wasm), cargo test-axum -p trusted-server-core (2669 passed), cargo test-cloudflare, cargo test-spin, and the cross-adapter parity suite (13 passed) all pass.

CI Status

All 20 reported checks PASS.

• cargo test: PASS
• cargo test (axum native): PASS
• cargo test (cross-adapter parity): PASS
• cargo test (ts CLI, native): PASS
• cargo check (cloudflare native + wasm32-unknown-unknown): PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• cargo fmt: PASS
• vitest: PASS
• format-typescript: PASS
• format-docs: PASS
• CodeQL: PASS
• Analyze (rust): PASS
• Analyze (actions): PASS
• Analyze (javascript-typescript): PASS
• integration tests: PASS
• integration tests (Fastly EC lifecycle): PASS
• browser integration tests: PASS
• prepare integration artifacts: PASS
• CLAUDE.md symlink guard: PASS

[prk-Jr]

Summary

Tight, well-scoped fix that does exactly what #420 asked: the truncating as u32 casts on bid w/h in parse_mediation_response become checked u32::try_from conversions that skip the bid on overflow, so a dimension above u32::MAX can no longer wrap into a small, plausible-looking value that sails past the zero-check. The PR picks the safer of the two options the issue offered (skip rather than clamp), states that choice explicitly, and matches the existing zero-dimension behaviour and the u32::try_from(v).ok() precedent in prebid.rs. No blocking findings.

1 of the inline comments below carries a one-click GitHub suggestion — use Commit suggestion to apply it as a commit on the PR branch. The remaining comments are prose only.

Verification performed

Reviewed against head 855b49bf7d37b95a1c8d91b0aa0635811bfe8ed9 in an isolated worktree:

• cargo test -p trusted-server-core --lib integrations::adserver_mock → 18 passed, 0 failed.
• Differential check: reverted only the production hunk back to the as u32 casts while keeping the new tests → test_parse_mediation_response_skips_oversized_dimensions FAILS (left: 3, right: 1). The regression test genuinely pins the fix.
• cargo fmt --all -- --check clean; cargo clippy-fastly exit 0.
• Behaviour for missing / negative / float-encoded w is unchanged (skip before, skip after) — only the debug log wording differs, so #420's "missing dimension keeps its current behavior" criterion holds.
• bid["w"] on a missing key yields Value::Null via serde_json's Index impl, so the {:?} in the new log::debug! cannot panic.

Non-blocking

⛏ nitpick

• oversized_width is also used as the h value — see inline at crates/trusted-server-core/src/integrations/adserver_mock.rs:1347-1368 (one-click suggestion)

📝 note

• No test pins the accepting side of the boundary (w == u32::MAX) — see inline at crates/trusted-server-core/src/integrations/adserver_mock.rs:1394

👍 praise

• Regression test was hardened to actually fail without the fix — see inline at crates/trusted-server-core/src/integrations/adserver_mock.rs:1342

Cross-cutting / body-level findings

• 🌱 trusted-server-core now has three divergent OpenRTB bid-dimension parsers — after this PR the crate parses a bid w/h three different ways:

  Site	Behaviour
  crates/trusted-server-core/src/auction/openrtb.rs:167 parse_optional_bid_dimension	u32::try_from, plus an f64 fallback for float-encoded dimensions, plus a > 0 filter, returning BidRejectionReason::InvalidBid. Already pub(crate) and consumed by prebid.rs.
  crates/trusted-server-core/src/integrations/prebid.rs:3039-3048	hand-rolled as_u64().and_then(u32::try_from).unwrap_or(0)
  crates/trusted-server-core/src/integrations/adserver_mock.rs:295-314 (this PR)	hand-rolled let-else guards + a separate zero-check

  parse_optional_bid_dimension already subsumes both the overflow guard and the zero-check this function now does in two steps, and it additionally accepts a float-encoded 728.0 that adserver_mock silently drops.

  Explicitly not asking for this in this PR. The return type is Result<Option<u32>, BidRejectionReason> while parse_mediation_response returns AuctionResponse, so the adaptation is non-mechanical, and AGENTS.md's "minimal changes" rule is on your side here. adserver_mock is a dev/mock integration, so the divergence is low-risk. Worth a follow-up issue rather than scope creep.

CI Status

• integration tests: PASS
• browser integration tests: PASS
• integration tests (Fastly EC lifecycle): PASS
• prepare integration artifacts: PASS
• cargo test: PASS
• cargo test (axum native): PASS
• cargo test (ts CLI, native): PASS
• cargo test (cross-adapter parity): PASS
• cargo check (cloudflare native + wasm32-unknown-unknown): PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• cargo fmt: PASS
• vitest: PASS
• format-typescript: PASS
• format-docs: PASS
• CLAUDE.md symlink guard: PASS
• CodeQL: PASS
• Analyze (rust): PASS
• Analyze (actions): PASS
• Analyze (javascript-typescript): PASS
• Analyze (javascript-typescript): PASS

All 20 reported checks pass; none pending, none failed.

[aram356]

Summary

Both blocking findings from the previous pass are resolved, and I confirmed that by re-running the original repro against the new merge-base rather than taking the diff at face value: the strengthened test now fails on the unfixed parser (left: 3, right: 1) and passes on the fix. It is a genuine regression test. The two merges from main are mechanical and touch nothing related to this code.

I then mutation-tested each production guard independently:

Mutation	Result
Revert the width guard to as u32	caught (left: 2)
Revert the height guard to as u32	caught (left: 2)
Delete || height == 0 from the zero-check	not caught - all 2763 core tests still pass

That last row is the one new finding. The rest are non-blocking observations and follow-ups.

1 of the inline comments below carries a one-click GitHub suggestion - use Commit suggestion to apply it as a commit on the PR branch. The others are prose because they propose follow-ups rather than a mechanical edit.

Blocking

wrench

• skips_zero_dimensions does not cover zero height - see inline at crates/trusted-server-core/src/integrations/adserver_mock.rs:1417

Non-blocking

thinking / seedling

• First debug! in integrations/ to interpolate a raw upstream value - see inline at crates/trusted-server-core/src/integrations/adserver_mock.rs:297
• Untested boundaries: u32::MAX exactly, negatives, float-encoded dimensions - see inline at crates/trusted-server-core/src/integrations/adserver_mock.rs:1347
• Silent degrade to no_bid is a monitoring blind spot - see cross-cutting below

Cross-cutting / body-level findings

• Seedling - an all-rejected mediation response is indistinguishable from genuine no-demand. When every bid is skipped, parse_mediation_response returns AuctionResponse::no_bid(...) at adserver_mock.rs:352, which is byte-identical to the mediator having had no demand: same NoBid status, same empty bids, same empty metadata. The log::info! at adserver_mock.rs:396 reports the count after filtering, so it prints 0 in both cases, and the per-bid log::debug! lines are filtered out at the default Info level. A mediator that starts emitting oversized dimensions on every bid is therefore invisible in a default deployment.

  The crate already ships the tool for this: ResponseAdmissionDiagnostics and BidRejectionReason at crates/trusted-server-core/src/auction/openrtb.rs:35-87, wired up by Prebid at crates/trusted-server-core/src/integrations/prebid.rs:2259-2302, which attaches bounded rejection counts to response.metadata. Both types are pub(crate) in the same crate, so the change is roughly three diagnostics.record(BidRejectionReason::InvalidBid) calls at the continue sites plus one attach_to before returning.

  This is a pre-existing property rather than something this PR introduces, so it is not a blocker. Raising it because this PR widens the set of silently-dropped bids. One caveat on how much it would buy you: I found the metadata written by Prebid but could not locate a reader for metadata["response_admission"], so I cannot confirm it currently reaches a dashboard. Worth checking before investing in it.

Verification performed

Scratch-verified in an isolated worktree at 965449577 (merge-base a4e01eb55):

• Grafted the current tests onto the unfixed merge-base: skips_oversized_dimensions FAILED (left: 3, right: 1), skips_zero_dimensions passed. The strengthened test pins the fix.
• Mutation tests on the production guards, as tabulated above. Both u32::try_from guards are independently covered; the height == 0 half of the zero-check is not covered by any test in the repository.
• With the suggested fix applied, the zero-check mutation fails loudly with left: ["sidebar", "footer"], right: ["footer"].
• Full CI gate with the suggestion applied: cargo fmt --all -- --check (the suggestion is rustfmt-canonical), all eight clippy aliases including the newly added clippy-cli and clippy-codegen, cargo test-axum -p trusted-server-core (2763 passed), cargo test-cloudflare, cargo test-spin, and the cross-adapter parity suite (14 passed). Post-verify drift check: no drift.

CI Status

All 20 reported checks PASS. They were mid-flight when this pass started and were re-checked at the end.

• cargo test: PASS
• cargo test (axum native): PASS
• cargo test (cross-adapter parity): PASS
• cargo test (ts CLI, native): PASS
• cargo check (cloudflare native + wasm32-unknown-unknown): PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• cargo fmt: PASS
• vitest: PASS
• format-typescript: PASS
• format-docs: PASS
• CodeQL: PASS
• Analyze (rust): PASS
• Analyze (actions): PASS
• Analyze (javascript-typescript): PASS
• integration tests: PASS
• integration tests (Fastly EC lifecycle): PASS
• browser integration tests: PASS
• prepare integration artifacts: PASS
• CLAUDE.md symlink guard: PASS

[aram356]

Summary

All three findings from the previous pass are resolved, and I verified each by mutation testing rather than by reading the diff. The production fix is correct, every guard in the dimension-validation path is now pinned by a test that fails when the guard is removed, and the full CI-equivalent gate is green.

Two non-blocking items remain, both raised before and neither in scope for a change the issue scoped to one truncating cast. I have measured the impact of both rather than describing them, so you can judge the priority yourself.

Mutation battery

Every mutation below was applied to the production code in an isolated worktree, with the full adserver_mock test module run against each.

Mutation	Previous pass	This pass
Revert the width guard to as u32	caught	caught
Revert the height guard to as u32	caught	caught
Delete the width == 0 half of the zero-check	not tested	caught
Delete the height == 0 half of the zero-check	survived, 2763 tests green	caught
Delete the zero-check entirely	not tested	caught

The fourth row was the blocking finding from the last pass, and it is now closed.

I also probed the two new boundary tests adversarially, because a boundary test that pins nothing is worse than no test at all:

• accepts_u32_max_dimensions — I introduced an off-by-one by narrowing the guard to filter(|v| *v < u64::from(u32::MAX)), simulating a < where <= belongs. The test failed. It genuinely pins the accept boundary, and it asserts the retained values rather than only the count.
• skips_negative_dimensions — I swapped as_u64() for as_i64().map(|v| v as u32), the plausible refactor that would start admitting negatives. That test and the oversized test both failed.

Also worth noting: moving the missing-dimensions bid to its own skyscraper slot was a good call, and better than what I suggested. It keeps the three rejection reasons independently attributable instead of collapsing two of them onto sidebar.

Non-blocking

refactor

• Float-encoded dimensions are silently dropped, contradicting the shared helper — see inline at crates/trusted-server-core/src/integrations/adserver_mock.rs:297

Cross-cutting / body-level findings

• Seedling — an all-rejected mediation response is byte-identical to genuine no-demand. This cannot anchor to a line inside the diff (the return is at adserver_mock.rs:360, outside both hunks), so it lives here.

  I measured it rather than asserting it. Two responses through parse_mediation_response: (A) an empty seatbid, and (B) three bids priced $5.00, $9.00 and $7.00, all with corrupt dimensions.

  A genuine-no-demand: status=NoBid bids=0 metadata={}
  B all-3-rejected:    status=NoBid bids=0 metadata={}
  INDISTINGUISHABLE: true
  

  Same status, same empty bid vector, same empty metadata. The log::info! at adserver_mock.rs:404 reports the count after filtering, so it prints 0 in both cases, and the per-bid log::debug! lines are filtered out at the default Info level (adapter-fastly/src/logging.rs:47-50). A mediator that starts emitting corrupt dimensions on every bid produces no distinguishable signal.

  The crate already ships the tool: ResponseAdmissionDiagnostics and BidRejectionReason at crates/trusted-server-core/src/auction/openrtb.rs:35-87, wired up by Prebid at prebid.rs:2259-2302, which attaches bounded rejection counts to response.metadata. Both are pub(crate) in the same crate, so the change is three record() calls at the continue sites plus one attach_to before returning.

  Two reasons I am keeping this a seedling rather than asking for it here. First, adserver_mock is documented as the mediator "used for auction mediation in dev/testing" (trusted-server.example.toml:654) and defaults to enabled = false, so a blind spot here is not a production revenue risk. Second, I looked for a consumer of metadata["response_admission"] and could not find one — Prebid writes it but I could not locate a reader, so adopting it may be wiring up a producer with no consumer. Worth confirming that before investing. Filing it as a follow-up issue would be reasonable.

Verification performed

Scratch-verified in an isolated worktree at bf96305b9. The merge-base is unchanged at a4e01eb55 (zero new commits on main since the last pass), so the previous pass's merge analysis still holds.

• cargo fmt --all -- --check: pass
• All eight clippy aliases, including the recently added clippy-cli and clippy-codegen: pass
• cargo test-axum -p trusted-server-core: 2765 passed
• cargo test-cloudflare: 23 passed. cargo test-spin: 38 passed
• cargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test parity: 14 passed
• Mutation battery and boundary-test probes as tabulated above

CI Status

All 20 reported checks PASS.

• cargo test: PASS
• cargo test (axum native): PASS
• cargo test (cross-adapter parity): PASS
• cargo test (ts CLI, native): PASS
• cargo check (cloudflare native + wasm32-unknown-unknown): PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• cargo fmt: PASS
• vitest: PASS
• format-typescript: PASS
• format-docs: PASS
• CodeQL: PASS
• Analyze (rust): PASS
• Analyze (actions): PASS
• Analyze (javascript-typescript): PASS
• integration tests: PASS
• integration tests (Fastly EC lifecycle): PASS
• browser integration tests: PASS
• prepare integration artifacts: PASS
• CLAUDE.md symlink guard: PASS

[dhruv8sh]

Squash-merged into rc/202609 as d6cbe44