Repository navigation
Allow explicit per-provider HTTP auction endpoints - #1525
ChristianPavilonis wants to merge 4 commits into
Conversation
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
APPROVE — no actionable findings at cd8977cad39d8388d99f6ff3f93083ba880deb3e, reviewed against 6fbe64aa056a17d32b272d67df3dd89b21ce895f. Both revisions remained unchanged at the final check.
The HTTP exception stays local to the configured provider. HTTPS remains the default; credentials, fragments, and unsupported schemes remain rejected. Configuration loading preserves the flag, and dispatch preserves the validated scheme across all four adapters.
Evidence and limits:
- All 29 current-head hosted checks pass.
- Hosted logs confirm all 27 auction-plan tests passed, including the three new regressions.
- Local
git diff --checkpassed. - No local build or live HTTP probe was run. Large unrelated, unchanged test bodies were sampled.
CI Status
- browser integration tests: PASS
- trace runtime and browser acceptance: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- cargo test: PASS (required)
- cargo test (cross-adapter parity): PASS
- vitest: PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- ts dev lint domains (changed lines): PASS
- Analyze (actions): PASS
- CodeQL: PASS
- CI Node pin guard: PASS
- CLAUDE.md symlink guard: PASS
- Analyze (javascript-typescript): PASS
- format-docs: PASS (required)
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- HTML EOF runtime (Fastly): PASS
- cargo test (axum native): PASS
- docs-proposal scripts: PASS
- Analyze (rust): PASS
- worker-deadlines (cloudflare): PASS
- format-typescript: PASS (required)
- prepare integration artifacts: PASS
- worker-deadlines (spin): PASS
- cargo fmt: PASS (required)
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
The validation change in canonicalize_endpoint is correct and stays scoped to one provider: an opted-in provider's flag doesn't leak to others, and credentials, fragments and other schemes stay rejected. I confirmed it works end to end on Axum and on Fastly (Viceroy). What's missing is around the change. Operators get no signal that a provider is sending in plaintext, the tests don't cover the transport, and the CHANGELOG, configuration reference and spec provenance don't match the change.
5 of the inline comments below carry a one-click GitHub
suggestion. Use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. The remaining comments describe the fix in prose because the change touches lines outside the diff or other files.
Blocking
🔧 wrench
- Nothing tells the operator a provider is running in plaintext: see inline at
crates/trusted-server-core/src/auction/plan.rs:326 - Tests don't check how an opted-in provider is dialed, or that HTTPS isn't downgraded: see inline at
crates/trusted-server-core/src/auction/plan.rs:1156 - Spec provenance cites commits that don't exist in this repository: see inline at
docs/superpowers/specs/2026-11-10-http-auction-provider-opt-in-design.md:7 - No CHANGELOG entry, and pushing the flag breaks rollback to older binaries: see body-level below
- Configuration reference and three other docs still say HTTPS-only: see body-level below
Non-blocking
♻️ refactor / ⛏ nitpick
- Field doc comment: summary/details split and rollback rationale: see inline at
crates/trusted-server-core/src/auction/plan.rs:139 - Guide table understates plaintext exposure: see inline at
docs/guide/auction-orchestration.md:786 is_falseduplicatessettings::is_false: see inline atcrates/trusted-server-core/src/auction/plan.rs:611- Scheme error message says HTTPS for opted-in providers: see inline at
crates/trusted-server-core/src/auction/plan.rs:585
Cross-cutting / body-level findings
-
🔧 No CHANGELOG entry, and pushing the flag breaks rollback to older binaries.
ProviderConfighas#[serde(deny_unknown_fields)], so a pushed blob withallow_insecure_http = truewon't load on any binary that predates the field. That needs the same upgrade/rollback ordering the CHANGELOG givessanitize_creatives. The Unreleased breaking config-first entry also still says "Provider endpoints must be absolute HTTPS URLs." Proposed### Addedentry:- Added per-provider `allow_insecure_http` (default `false`) under `[auction.providers.<id>]`, which lets an explicitly approved demo or test provider use a plain `http://` endpoint. Other providers still require HTTPS, and credentials, fragments and other schemes remain rejected. Requests to an opted-in provider carry client IP, User-Agent, geo, consent signals and identifiers in cleartext, and its responses can be altered in transit. Upgrading: binaries that predate the field reject a blob that sets it, so upgrade the binary first, then push the config. Rollback: remove the flag, push the default-compatible blob, then roll back.
Also amend the breaking entry's sentence to "Provider endpoints must be absolute HTTPS URLs unless the provider sets
allow_insecure_http." -
🔧 Configuration reference and three other docs still say HTTPS-only. The PR updates only
docs/guide/auction-orchestration.md. Operators configuring providers read these too:-
docs/guide/configuration.md:2444-2452: the provider-field table has noallow_insecure_httprow, and itsendpointrow says "Absolute HTTPS URL with host and no credentials or fragment". Proposed rows (then runnpm run format:writeindocs/):| `endpoint` | Yes | None | Absolute HTTPS URL with host and no credentials or fragment; plain HTTP only with `allow_insecure_http` | | `allow_insecure_http` | No | `false` | Accept a plain `http://` endpoint for this provider only; approved demo/test endpoints only |
-
docs/guide/configuration.md:2386: "All provider endpoints must be absolute HTTPS URLs." Suggest: "All provider endpoints must be absolute HTTPS URLs, except a provider that setsallow_insecure_httpfor an approved demo or test endpoint." -
crates/trusted-server-core/src/auction/README.md:160: same sentence. Suggest: "Provider endpoints must be absolute HTTPS URLs unless the provider setsallow_insecure_httpfor an approved demo or test endpoint." -
docs/guide/error-reference.md:92: the solution for this error. Add: "Plain HTTP is accepted only when that provider setsallow_insecure_http = true, for an approved demo or test endpoint."
-
-
📌 The CLI drops core warnings.
trusted-server-clidepends onlogbut installs no logger, so anything core logs duringts config validate/ts config pushis discarded. That includes the existingpublisher.public_originHTTP warning and the warning proposed atplan.rs:326. With the realtsbinary, validate and push printed only their success lines for an opted-in HTTP provider. Worth a follow-up issue: either install a stderr logger atwarn, or return warnings from validation. -
🤔 The flag accepts any host, including public ones, so plaintext auction traffic can cross the internet. The spec deliberately leaves allowlists out of scope. If demos only need a local or private-network PBS, restricting
httpto loopback or private addresses would bound the exposure without losing that use case. Thepublic_originwarning already distinguishes loopback hosts the same way. -
📝 PR description. It has typos ("All29", "main6fbe64aa") and internal process notes (relay ancestry, retained dirty worktrees, excluded PR identities) that a reviewer can't act on. A plain summary of the behavior change, the default and the rollback note would serve readers better.
Verification
- Real binaries at
cd8977cad:ts config validaterejects anhttp://provider endpoint without the flag and accepts it with the flag, andts config pushkeeps"allow_insecure_http":truein the blob. A livePOST /auctionon Axum and on Fastly (Viceroy) reached a stub bidder over plaintext and returned its bid. Fastly registered a non-TLS dynamic backend (backend_http_127_0_0_1_…_p_pbs-main_…). - Mutation testing: 9 mutants of
plan.rsagainst the fulltrusted-server-corelib suite. 7 were killed and 2 survived (see inline atplan.rs:1156). With that suggestion applied, both are killed. - Each suggestion was verified alone (fmt,
clippy-fastly, core lib tests, prettier, domain lint) and all together through every clippy and test alias plus the parity suite.
CI Status
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
- Analyze (rust): PASS
- CI Node pin guard: PASS
- CLAUDE.md symlink guard: PASS
- CodeQL: PASS
- HTML EOF runtime (Fastly): PASS
- browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS (required)
- cargo test: PASS (required)
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- docs-proposal scripts: PASS
- format-docs: PASS (required)
- format-typescript: PASS (required)
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- trace runtime and browser acceptance: PASS
- ts dev lint domains (changed lines): PASS
- vitest: PASS
- worker-deadlines (cloudflare): PASS
- worker-deadlines (spin): PASS
| let endpoint = canonicalize_endpoint( | ||
| &id, | ||
| registration.id, | ||
| &raw.endpoint, | ||
| raw.allow_insecure_http, | ||
| )?; |
There was a problem hiding this comment.
🔧 wrench: Nothing tells the operator that a provider is running in plaintext.
An opted-in provider's requests carry the client IP (device.ip, plus X-Forwarded-For for the prebid-server profile), geo (lat/lon for PBS), User-Agent, the TCF string and consent cookies, and the EC ID and EIDs when present. Its responses (price, adm, nurl/burl) can be rewritten in transit, and sanitize_creatives defaults to off. I ran opted-in configs end to end on Axum and on Fastly under Viceroy. ts config validate, ts config push and the adapter runtime all stayed silent while the stub bidder received a plaintext POST /openrtb2/auction carrying those fields.
The repo already warns for the equivalent publisher-origin case (publisher_origin_diagnostics, config.rs:89). Logging at plan compile covers deploy validation and every adapter's startup from one place. On Fastly it fires once per instance, like the public_origin warning. CodeQL may flag the new log site as rust/cleartext-logging (Settings taint); the comment follows the convention at the orchestrator's provider-ID log sites. The CLI installs no logger, so this reaches adapter logs only (see the 📌 in the review body).
| let endpoint = canonicalize_endpoint( | |
| &id, | |
| registration.id, | |
| &raw.endpoint, | |
| raw.allow_insecure_http, | |
| )?; | |
| let endpoint = canonicalize_endpoint( | |
| &id, | |
| registration.id, | |
| &raw.endpoint, | |
| raw.allow_insecure_http, | |
| )?; | |
| if endpoint.url().scheme() == "http" { | |
| // This logs a configured provider identifier, not request data or secrets. | |
| log::warn!( | |
| "provider `{id}` uses unencrypted HTTP; its requests carry client IP, geo, consent signals and identifiers in cleartext, and its responses can be altered in transit" | |
| ); | |
| } |
| assert_eq!( | ||
| plan.providers()[0].endpoint.as_str(), | ||
| "http://pbs.example.com:8000/openrtb2/auction", | ||
| "should normalize the opted-in Prebid Server endpoint" | ||
| ); |
There was a problem hiding this comment.
🔧 wrench: The tests don't check how an opted-in provider is dialed, or that the flag leaves HTTPS alone.
I ran 9 mutants of plan.rs against the full core lib suite. The scope, scheme, credential, fragment and serde mutants are all killed, but two survive:
backend_spec_with_transport_timeouthard-codingscheme: "https". Every opted-in HTTP provider would then dial TLS (Fastly registers anhttps_…backend withenable_ssl), and CI stays green.canonicalize_endpointcallingset_scheme("http")when the flag is set. The spec's "HTTPS is not downgraded when the flag is true" has no test behind it.
With this suggestion applied, both mutants fail this test (should dial the opted-in provider without TLS on its configured port and should not downgrade HTTPS when the provider opts in to HTTP).
| assert_eq!( | |
| plan.providers()[0].endpoint.as_str(), | |
| "http://pbs.example.com:8000/openrtb2/auction", | |
| "should normalize the opted-in Prebid Server endpoint" | |
| ); | |
| assert_eq!( | |
| plan.providers()[0].endpoint.as_str(), | |
| "http://pbs.example.com:8000/openrtb2/auction", | |
| "should normalize the opted-in Prebid Server endpoint" | |
| ); | |
| let spec = plan.providers()[0].backend_spec(); | |
| assert_eq!( | |
| (spec.scheme.as_str(), spec.port), | |
| ("http", Some(8000)), | |
| "should dial the opted-in provider without TLS on its configured port" | |
| ); | |
| plan.validate_for_target(AuctionTargetId::Fastly) | |
| .expect("should predict a Fastly backend for the opted-in HTTP provider"); | |
| let mut secure = demo.clone(); | |
| secure.endpoint = "https://pbs.example.com".to_string(); | |
| let secure_plan = AuctionPlan::compile(config(BTreeMap::from([(id("demo"), secure)]))) | |
| .expect("should compile an opted-in HTTPS provider"); | |
| assert_eq!( | |
| secure_plan.providers()[0].endpoint.as_str(), | |
| "https://pbs.example.com/openrtb2/auction", | |
| "should not downgrade HTTPS when the provider opts in to HTTP" | |
| ); |
|
|
||
| Permit an explicitly configured auction provider to use unencrypted HTTP for an approved demo or test endpoint. Keep HTTPS as the default. Do not turn one provider's exception into permission for another provider. | ||
|
|
||
| This documents the existing reconstructed feature at `2c79ea59`. Its formal design reviews occur before publication readiness, not before the earlier implementation. It does not authorize deployment or production use of unencrypted auction traffic. |
There was a problem hiding this comment.
🔧 wrench: The provenance cites commits that don't exist in this repository.
2c79ea59 here, and dd2ca144..2c79ea59 in the review log (line 65), return 422 from the GitHub commits API and aren't on any ref. The "Contract SHA256" hashes a document that isn't published either. A reader can't check what the two reviews examined. Either push those commits to a ref in this repository, or describe the reviewed inputs without SHAs. The review-log rows and the closing paragraph at line 68 ("relay ancestry", "replays") need the same treatment; that spans the table, so it can't be a single suggestion. For line 7:
| This documents the existing reconstructed feature at `2c79ea59`. Its formal design reviews occur before publication readiness, not before the earlier implementation. It does not authorize deployment or production use of unencrypted auction traffic. | |
| This documents the provider-local HTTP opt-in added by this change. It does not authorize deployment or production use of unencrypted auction traffic. |
| /// Permit unencrypted HTTP for this provider only. Defaults to false. | ||
| /// Use only for explicitly approved demo/test endpoints. |
There was a problem hiding this comment.
♻️ refactor: Split the summary from the details per AGENTS.md, and say why the field is skipped while false. The sibling fields that use the same pattern (Publisher::allow_async_html_finish and DebugConfig::sandbox_metrics_enabled in settings.rs) document the rollback consequence. It applies here too, because ProviderConfig denies unknown fields.
| /// Permit unencrypted HTTP for this provider only. Defaults to false. | |
| /// Use only for explicitly approved demo/test endpoints. | |
| /// Permit an unencrypted `http` endpoint for this provider only. | |
| /// | |
| /// Defaults to false. Use only for explicitly approved demo/test endpoints: | |
| /// requests to this provider carry client IP, geo, consent signals and | |
| /// identifiers in cleartext, and its responses can be altered in transit. | |
| /// | |
| /// Skipped from serialization while false: [`ProviderConfig`] denies | |
| /// unknown fields, so a default blob stays readable by a binary built | |
| /// before this field existed. A blob with it enabled requires restoring a | |
| /// compatible blob before rolling back. |
| | `endpoint` | Required | Fixed absolute HTTPS endpoint | | ||
| | `allow_insecure_http` | `false` | Explicit per-provider HTTP opt-in for approved demos/tests; traffic is unencrypted | | ||
| | `timeout_ms` | Profile default | PBS 1000 ms, APS 800 ms, standard inherits auction timeout | | ||
| | `routing` | `explicit` | `explicit`, or `all_eligible` for non-PBS profiles | | ||
| | `profile_config` | `{}` | Profile-owned typed settings | | ||
| | `notifications` | No suppression | Common `nurl`/`burl` suppression by all bids or returned seats | | ||
|
|
There was a problem hiding this comment.
♻️ refactor: The endpoint row still reads as HTTPS-only, and "traffic is unencrypted" understates what an operator is opting into: client IP, geo, consent and identifiers leave in cleartext, and responses can be altered. A short warning block keeps the table cell readable. The cell widths are sized so prettier leaves the other rows alone.
| | `endpoint` | Required | Fixed absolute HTTPS endpoint | | |
| | `allow_insecure_http` | `false` | Explicit per-provider HTTP opt-in for approved demos/tests; traffic is unencrypted | | |
| | `timeout_ms` | Profile default | PBS 1000 ms, APS 800 ms, standard inherits auction timeout | | |
| | `routing` | `explicit` | `explicit`, or `all_eligible` for non-PBS profiles | | |
| | `profile_config` | `{}` | Profile-owned typed settings | | |
| | `notifications` | No suppression | Common `nurl`/`burl` suppression by all bids or returned seats | | |
| | `endpoint` | Required | Fixed absolute HTTPS endpoint; plain HTTP only with `allow_insecure_http` | | |
| | `allow_insecure_http` | `false` | Per-provider plain HTTP opt-in for approved demo/test endpoints; see warning below | | |
| | `timeout_ms` | Profile default | PBS 1000 ms, APS 800 ms, standard inherits auction timeout | | |
| | `routing` | `explicit` | `explicit`, or `all_eligible` for non-PBS profiles | | |
| | `profile_config` | `{}` | Profile-owned typed settings | | |
| | `notifications` | No suppression | Common `nurl`/`burl` suppression by all bids or returned seats | | |
| ::: warning Plaintext provider traffic | |
| `allow_insecure_http = true` sends that provider's requests without TLS. Their | |
| headers and OpenRTB bodies, including client IP, User-Agent, geo, consent | |
| signals and identifiers such as the EC ID and EIDs, are readable on the | |
| network, and bid prices, creatives and notification URLs in its responses can | |
| be altered in transit. The flag does not restrict which hosts are reachable. | |
| ::: | |
| Ok(CanonicalProviderEndpoint(endpoint)) | ||
| } | ||
|
|
||
| fn is_false(value: &bool) -> bool { |
There was a problem hiding this comment.
♻️ refactor: settings.rs:2822 already defines the same is_false serde predicate, with a doc comment. Make that one pub(crate) and point this field at it, so the two can't drift:
// settings.rs
/// Serde predicate for omitting `false` flags from serialized config blobs.
pub(crate) fn is_false(value: &bool) -> bool {
!*value
}// plan.rs
#[serde(default, skip_serializing_if = "crate::settings::is_false")]
pub allow_insecure_http: bool,Then delete this copy. Apply manually: the fix touches settings.rs, which is outside this diff.
| )) | ||
| })?; | ||
| if endpoint.scheme() != "https" | ||
| if !(endpoint.scheme() == "https" || (allow_insecure_http && endpoint.scheme() == "http")) |
There was a problem hiding this comment.
⛏ nitpick: For an opted-in provider, a rejected http:// endpoint (for example http://user:pass@pbs.example.com/bid) still gets endpoint must be absolute HTTPS with a host and no credentials or fragment from line 592. That reads as if the scheme were the problem. Consider wording that covers both schemes, for example:
"provider `{provider_id}` endpoint must be an absolute HTTPS URL, or HTTP when allow_insecure_http is set, with a host and no credentials or fragment"docs/guide/error-reference.md documents this error family, so update it alongside. Apply manually: line 592 is outside the diff hunk.
Summary
Ready for feature review. All29 current-head hosted checks passed at cd8977c. This is not deployment or production readiness.
Stack and exact input
Head:
cd8977cad39d8388d99f6ff3f93083ba880deb3e. Base:main.Retain the original demo RC #1424 and its dirty worktrees. No source from excluded #1499 or the independent #1506 implementation is imported or coordinated. Relay/privacy work remains separate. No deployment, live input change, main merge, old-RC retirement or recovery deletion is part of this PR.
Changes
Closes
Closes #1521.
Spec
HTTP auction opt-in contract and completed review log. The two reviews examined the original immutable delta, after implementation and before readiness; they do not assert execution of this replayed head.
Test plan
The inherited HTTP fixture now uses
pbs.example.cominstead of a TEST-NET IP rejected by the repository domain linter. All27 focused auction-plan tests and strict Core Clippy passed on that test-only correction; no allowlist/suppression, behavior or assertion was relaxed. Other unchanged-input results below remain bounded to their recorded source.Existing focused plan/adapter receipts and full source reviews belong to their recorded earlier inputs. The feature was cleanly replayed onto current main6fbe64aa. All29 current-head hosted checks passed. These results establish this isolated feature's review eligibility, not the later native combination or deployment qualification.
Checklist