Repository navigation
Retain provider-local HTTP opt-in in the demo baseline - #1533
ChristianPavilonis wants to merge 20 commits into
Conversation
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
The provider-local allow_insecure_http opt-in is correct and well covered: the flag gates only the http scheme, does not leak to other providers or schemes, is omitted from serialized output when false, and the scheme flows through to the backend spec so an opted-in endpoint is actually sent as plain HTTP. One blocking item: the current-head hosted check cargo test (axum native) fails from an inherited test fixture that this PR describes as corrected but which was corrected only in a sibling file.
1 of the inline comments below carries a one-click GitHub
suggestion. The blocking finding is body-level because its file is outside this PR's diff.
Blocking
🔧 wrench
startup.rsstill has the self-connection listener probe — see Cross-cutting below.
Non-blocking
⛏ nitpick
unwrap()in the new serialization test — see inline atcrates/trusted-server-core/src/auction/plan.rs:1186
Cross-cutting / body-level findings
- 🔧
startup.rsstill has the self-connection listener probe.crates/trusted-server-adapter-axum/tests/startup.rs:116(assert_failed_before_listen) treats any successfulTcpStream::connect_timeoutas "the server is listening". On Linux a connect can succeed with no listener when the ephemeral source port equals the target port. The PR description says this was corrected; the correction in70a894547addedconnected_to_listener(local address != peer address) tonative_acceptance.rsonly, andstartup.rskept the bareis_err()probe. At this head the Linux runner hit the case:missing_config_fails_before_listen_without_legacy_fallbackpanicked atstartup.rs:115("should not listen while required startup inputs are invalid"). The base branch passed with identical fixture content, so this is intermittent and inherited rather than introduced by the HTTP change. Minimal correction: apply the same local-vs-peer exclusion inassert_failed_before_listen(theconnected_to_listenerhelper fromnative_acceptance.rs). The file entered in the native unit and was last touched by the geo unit, so the fix belongs there and merges upward through the stack. Not a suggestion because the file is outside this diff. - 📝 Configuration guide now contradicts the new field.
docs/guide/configuration.md:2090still says all provider endpoints must be absolute HTTPS URLs, and the provider-field table at:2153has noallow_insecure_httprow, whileauction-orchestration.mddocuments the opt-in. Optional consistency fix. - 📝 Rollback note, no action needed.
ProviderConfighasdeny_unknown_fields, so a config blob withallow_insecure_httpset is rejected by any binary that predates this change; a demo config using it cannot roll back the binary alone. Default configs are unaffected because the field is omitted when false.
Verification notes
- Focused
auction::plansuite at head: 32 pass (host triple, native). - Mutation controls on the new guard: accepting
httpwithout the flag failsinsecure_http_requires_an_explicit_per_provider_opt_in; letting the flag accept any scheme failsinsecure_http_opt_in_does_not_allow_credentials_fragments_or_other_schemes. startupfixture on macOS: 3/3 pass (the Linux self-connection does not reproduce there).- The inline suggestion was applied in a scratch tree:
cargo fmt --check,cargo clippy -p trusted-server-core --lib --tests -D warnings, and the three focused tests pass.
CI Status
- cargo test (axum native): FAIL
- CLAUDE.md symlink guard: PASS
- browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip2): PASS
- cargo fmt: PASS
- cargo test: 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
- format-typescript: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- vitest: PASS
| let legacy = serde_json::to_value(provider("prebid-server")).unwrap(); | ||
| assert!(legacy.get("allow_insecure_http").is_none()); | ||
| assert!( | ||
| !serde_json::from_value::<ProviderConfig>(legacy) | ||
| .unwrap() | ||
| .allow_insecure_http | ||
| ); | ||
| let mut opted_in = provider("prebid-server"); | ||
| opted_in.allow_insecure_http = true; | ||
| let encoded = serde_json::to_value(opted_in).unwrap(); | ||
| assert_eq!(encoded["allow_insecure_http"], true); | ||
| assert!( | ||
| serde_json::from_value::<ProviderConfig>(encoded) | ||
| .unwrap() | ||
| .allow_insecure_http | ||
| ); |
There was a problem hiding this comment.
⛏ nitpick — These are the only unwrap() calls in this file (the base had none), and AGENTS.md asks for expect("should ...") in tests too. Scratch-verified: cargo fmt --check, clippy -D warnings, and the three focused tests pass with this applied.
| let legacy = serde_json::to_value(provider("prebid-server")).unwrap(); | |
| assert!(legacy.get("allow_insecure_http").is_none()); | |
| assert!( | |
| !serde_json::from_value::<ProviderConfig>(legacy) | |
| .unwrap() | |
| .allow_insecure_http | |
| ); | |
| let mut opted_in = provider("prebid-server"); | |
| opted_in.allow_insecure_http = true; | |
| let encoded = serde_json::to_value(opted_in).unwrap(); | |
| assert_eq!(encoded["allow_insecure_http"], true); | |
| assert!( | |
| serde_json::from_value::<ProviderConfig>(encoded) | |
| .unwrap() | |
| .allow_insecure_http | |
| ); | |
| let legacy = serde_json::to_value(provider("prebid-server")) | |
| .expect("should serialize the default provider"); | |
| assert!(legacy.get("allow_insecure_http").is_none()); | |
| assert!( | |
| !serde_json::from_value::<ProviderConfig>(legacy) | |
| .expect("should deserialize the legacy provider shape") | |
| .allow_insecure_http | |
| ); | |
| let mut opted_in = provider("prebid-server"); | |
| opted_in.allow_insecure_http = true; | |
| let encoded = | |
| serde_json::to_value(opted_in).expect("should serialize the opted-in provider"); | |
| assert_eq!(encoded["allow_insecure_http"], true); | |
| assert!( | |
| serde_json::from_value::<ProviderConfig>(encoded) | |
| .expect("should deserialize the opted-in provider") | |
| .allow_insecure_http | |
| ); |
Preserve the existing literal-loopback HTTP exception, explicit non-loopback provider opt-in, default-false wire shape and credential/fragment/scheme rejection. Retain the existing HTTP feature head and the direct EdgeZero cutover.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
The scoping in canonicalize_endpoint is right: one provider's flag doesn't leak to another, and credentials, fragments and other schemes stay rejected. I confirmed it end to end at this head on Axum and on Fastly (Viceroy). What's missing is around the change. Operators get no signal that a provider is sending in plaintext, two transport mutants survive the whole core suite, and the CHANGELOG and four other docs still say HTTPS-only. The production code is the same as #1525, so these match my #1525 r1 findings, re-checked against this base.
4 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 other inline comments describe the fix in prose because it touches lines outside the diff or another file.
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:1163 - Configuration reference, CHANGELOG and three other docs still say HTTPS-only: see body-level below
prepare integration artifactsfailed, so integration and browser jobs never ran: 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:771 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
-
🔧 Configuration reference, CHANGELOG and three other docs still say HTTPS-only. The PR updates only
docs/guide/auction-orchestration.md. This extends prk-Jr's 📝 onconfiguration.md: operators configuring providers read every one of these, and two of them give the wrong fix for the exact error an opted-in setup hits.-
CHANGELOG.md:15: the Unreleased config-first entry says "Provider endpoints must be absolute HTTPS URLs." Amend it to "... unless the provider setsallow_insecure_http", and add an### Addedentry with the rollback ordering.ProviderConfigdenies unknown fields, and the base branch's ownts config validate(built at0ae96ef5f) rejects an opted-in config withunknown field `allow_insecure_http` … in `auction.providers.pbs-main`, while the same config without the flag passes:- 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.
-
docs/guide/configuration.md:2149-2157: the provider-field table has noallow_insecure_httprow, and itsendpointrow says "Absolute HTTPS URL with host and no credentials or fragment". Proposed rows (thennpm 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:2089-2090: "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:151: 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-93: 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." -
docs/guide/integrations/aps.md:132: "The common providerendpoint... must be an absolute HTTPS URL". The flag applies toapsproviders too, so add the same exception or say the flag is for demo/test providers only.
-
-
🔧
prepare integration artifactsfailed at this head, so the integration and browser jobs never ran. The job built every artifact, thenactions/upload-artifactgot an HTML page back fromCreateArtifacton all 5 attempts (job log). That is a GitHub artifact-service failure, not a code failure, but it skippedintegration tests,integration tests (Fastly EC lifecycle)andbrowser integration tests, which the description lists as still required before readiness. Re-run the workflow. Separately,cargo test (axum native)passes at6501eede7, so thestartup.rsself-connection flake prk-Jr flagged at1cfba70a2didn't recur this run; it is still intermittent until that fixture gets the same local-vs-peer exclusion. -
📌 The CLI drops core warnings.
trusted-server-clidepends onlog(Cargo.toml:27) but installs no logger on this base either, so anything core logs duringts config validate/ts config pushis discarded, including the warning proposed atplan.rs:326. I raised the same point on #1525 and no issue exists yet. 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. No in-repo example in the stack (#1534 to #1541) sets the flag, so the demo's PBS host isn't visible from here. 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. If the demo PBS is public, the docs warning should say so explicitly. -
📝 PR description.
Head:still says1cfba70a2; the head is6501eede7after two EdgeZero-tag merges (tracker #1530 has the same stale value). Several identifiers have lost their separating space ("EdgeZero83d7a629", "PR1525/1526 ready and1527/1528 draft", "Nativeb1216763a", "UID/GID10001"). Most of the description covers base-branch corrections (docs build, startup probe, browser CI host) that are not in this PR's diff. A plain summary of the behavior change, the default, and the rollback note would serve reviewers better. -
📝 Keep this PR and #1525 in sync.
plan.rshere carries the same production change as #1525 (main), but the tests diverge: #1525 already has assertion messages andexpect()where this PR has bare asserts andunwrap(). The #1525 r1 findings (warning, transport tests, docs, CHANGELOG) apply here unchanged. Whichever fixes land on one should land on the other, or the two copies drift.
Verification
- Real binaries at
6501eede7:ts config validate --strictrejects anhttp://provider without the flag and accepts it with the flag, silently;ts config pushkeeps"allow_insecure_http":truein the blob. A livePOST /auctionon Axum and on Fastly (Viceroy) reached a plaintext stub bidder and returned its bid. Fastly registered a non-TLS dynamic backend (backend_http_127_0_0_1_…_p_pbs-main_…). The base branch'sts(at0ae96ef5f) rejects the same config withunknown field allow_insecure_http. - Mutation testing: 9 mutants of
plan.rsagainst the fulltrusted-server-corelib suite (3005 tests). 7 killed, 2 survived (see inline atplan.rs:1163). With that suggestion applied, both are killed by their named assertions. - Each suggestion verified alone (fmt,
clippy-fastly, core lib tests, or prettier and domain lint for the guide), and all four together through every adapter clippy and test alias plus the parity suite, with no drift between the approved and verified patches. One Axum test,managed_ingress_ignores_direct_spoof_and_isolates_two_gateway_clients, can't run on macOS (it binds127.0.0.2, whichlo0doesn't carry) and fails identically at the unmodified head; hostedcargo test (axum native)covers it on Linux.ts dev lint domains --changed-vson this PR's lines: clean.
CI Status
- browser integration tests: SKIPPED
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip2): PASS
- cargo fmt: PASS
- cargo test: PASS
- 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
- CLAUDE.md symlink guard: PASS
- docs-proposal scripts: PASS
- format-docs: PASS
- format-typescript: PASS
- integration tests: SKIPPED
- integration tests (Fastly EC lifecycle): SKIPPED
- prepare integration artifacts: FAIL
- vitest: 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.
I ran an opted-in prebid-server provider end to end at this head on Axum and on Fastly (Viceroy). ts config validate --strict and ts config push printed only their success lines, and neither adapter logged anything about it, while the stub bidder received a plaintext POST /openrtb2/auction (Fastly registered backend_http_127_0_0_1_…_p_pbs-main_…, no TLS) carrying the client IP (X-Forwarded-For and device.ip), the User-Agent, the TCF string, the raw euconsent-v2 cookie header, and on Fastly device.geo with lat/lon and city. Its bid came back and was rendered, so responses (price, adm, nurl/burl) can be rewritten in transit too.
Both branches already flag the equivalent TLS weakening on Fastly with an INSECURE: warning (warn_if_certificate_check_disabled at crates/trusted-server-adapter-fastly/src/app.rs:242, and backend.rs:251). Logging at plan compile covers every adapter from one place. It fires during runtime validation (config_payload.rs:140) and again in each adapter's build_state, so expect it twice per instance start. The CLI installs no logger, so it won't show at ts config validate (see the 📌 in the review body). Same finding as #1525 r1, with the INSECURE: prefix added to match.
| 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!( | |
| "INSECURE: 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!( | ||
| AuctionPlan::compile(config(BTreeMap::from([(id("demo"), demo.clone())]))).is_err() | ||
| ); | ||
| demo.allow_insecure_http = true; | ||
| let plan = AuctionPlan::compile(config(BTreeMap::from([(id("demo"), demo.clone())]))) | ||
| .expect("explicit demo HTTP endpoint should compile"); | ||
| assert_eq!( | ||
| plan.providers()[0].endpoint.as_str(), | ||
| "http://pbs.example.com:8000/openrtb2/auction" | ||
| ); | ||
| let mut other = provider("standard"); | ||
| other.endpoint = "http://other.example/bid".to_string(); | ||
| assert!( | ||
| AuctionPlan::compile(config(BTreeMap::from([ | ||
| (id("demo"), demo), | ||
| (id("other"), other) | ||
| ]))) | ||
| .is_err() | ||
| ); |
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 trusted-server-core lib suite (3005 tests) at this head. 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 would register anhttps_…backend withenable_sslinstead of thebackend_http_…one I saw under Viceroy), and CI stays green.canonicalize_endpointcallingset_scheme("http")when the flag is set, which silently downgrades an opted-in HTTPS endpoint. Nothing asserts that the flag leaves HTTPS alone.
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). It also gives the bare asserts the messages AGENTS.md asks for, and checks backend prediction on all four targets. prk-Jr's expect() suggestion on the wire-shape test below leaves its asserts without messages; worth adding them there too.
| assert!( | |
| AuctionPlan::compile(config(BTreeMap::from([(id("demo"), demo.clone())]))).is_err() | |
| ); | |
| demo.allow_insecure_http = true; | |
| let plan = AuctionPlan::compile(config(BTreeMap::from([(id("demo"), demo.clone())]))) | |
| .expect("explicit demo HTTP endpoint should compile"); | |
| assert_eq!( | |
| plan.providers()[0].endpoint.as_str(), | |
| "http://pbs.example.com:8000/openrtb2/auction" | |
| ); | |
| let mut other = provider("standard"); | |
| other.endpoint = "http://other.example/bid".to_string(); | |
| assert!( | |
| AuctionPlan::compile(config(BTreeMap::from([ | |
| (id("demo"), demo), | |
| (id("other"), other) | |
| ]))) | |
| .is_err() | |
| ); | |
| assert!( | |
| AuctionPlan::compile(config(BTreeMap::from([(id("demo"), demo.clone())]))).is_err(), | |
| "should reject HTTP without the provider's explicit opt-in" | |
| ); | |
| demo.allow_insecure_http = true; | |
| let plan = AuctionPlan::compile(config(BTreeMap::from([(id("demo"), demo.clone())]))) | |
| .expect("should compile an explicitly opted-in HTTP provider"); | |
| 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" | |
| ); | |
| for target in [ | |
| AuctionTargetId::Fastly, | |
| AuctionTargetId::Axum, | |
| AuctionTargetId::Cloudflare, | |
| AuctionTargetId::Spin, | |
| ] { | |
| plan.validate_for_target(target) | |
| .expect("should predict a 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" | |
| ); | |
| let mut other = provider("standard"); | |
| other.endpoint = "http://other.example/bid".to_string(); | |
| assert!( | |
| AuctionPlan::compile(config(BTreeMap::from([ | |
| (id("demo"), demo), | |
| (id("other"), other) | |
| ]))) | |
| .is_err(), | |
| "should not inherit another provider's HTTP opt-in" | |
| ); |
| /// 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 flag that uses the same skip_serializing_if = "is_false" pattern on this base, DebugConfig::sandbox_metrics_enabled (settings.rs:2619-2623), documents 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 two lines up still reads as HTTPS-only, and "traffic is unencrypted" understates what an operator opts into: client IP, geo, consent signals and identifiers leave in cleartext, and responses can be altered. A short warning block keeps the table cell readable. 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:2629 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/bid) still gets endpoint must be absolute HTTPS with a host and no credentials or fragment from line 592, which reads as if the scheme were the problem. A provider without the flag that sets http:// gets the same text, with no hint that an opt-in exists. 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:82-106 documents this error family, so update it alongside. Apply manually: line 592 is outside the diff hunk.
There was a problem hiding this comment.
Superseded by a follow-up review with inline comments: #1533 (review)
dhruv8sh
left a comment
dhruv8sh
left a comment
There was a problem hiding this comment.
Summary
Adds a default-false, per-provider allow_insecure_http opt-in to auction provider config, keeps the literal-loopback exception, and sets the field explicitly in every caller. The validation logic is correct: credentials, fragments, missing hosts and non-HTTP schemes stay rejected even with the opt-in, and the error messages now mention the opt-in. The remaining comments are non-blocking test and docs polish.
4 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 comment is prose only.
Non-blocking
♻️ refactor
- Assertion messages and rejected-provider check in opt-in test — see inline at
crates/trusted-server-core/src/auction/plan.rs:1065 - Assertion messages in wire-shape test — see inline at
crates/trusted-server-core/src/auction/plan.rs:1105
🤔 thinking
- Log when a provider enables insecure HTTP — see inline at
crates/trusted-server-core/src/auction/plan.rs:144
⛏ nitpick
endpointrow still says HTTPS only — see inline atdocs/guide/auction-orchestration.md:782- Redundant loopback sentence — see inline at
docs/guide/auction-orchestration.md:796
CI Status
- integration tests (Fastly EC lifecycle): PASS
- integration tests: PASS
- browser integration tests: PASS
- trace runtime and browser acceptance: PASS
- format-docs: PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- HTML EOF runtime (Fastly): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- ts dev lint domains (changed lines): PASS
- CI Node pin guard: PASS
- docs-proposal scripts: PASS
- prepare integration artifacts: PASS
- release scripts: PASS
- CLAUDE.md symlink guard: PASS
- format-typescript: PASS
- vitest: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- Fastly response diff script: PASS
- cargo test: PASS
- cargo test (axum native): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo test (cross-adapter parity): PASS
- cargo fmt: PASS
- worker-deadlines (spin): PASS
- worker-deadlines (cloudflare): PASS
| assert!( | ||
| AuctionPlan::compile(config(BTreeMap::from([(id("demo"), demo.clone())]))).is_err() | ||
| ); | ||
| demo.allow_insecure_http = true; | ||
| let plan = AuctionPlan::compile(config(BTreeMap::from([(id("demo"), demo.clone())]))) | ||
| .expect("should compile an explicitly approved demo HTTP endpoint"); | ||
| assert_eq!( | ||
| plan.providers()[0].endpoint.as_str(), | ||
| "http://pbs.example.com:8000/openrtb2/auction" | ||
| ); | ||
| let mut other = provider("standard"); | ||
| other.endpoint = "http://other.example/bid".to_string(); | ||
| assert!( | ||
| AuctionPlan::compile(config(BTreeMap::from([ | ||
| (id("demo"), demo), | ||
| (id("other"), other) | ||
| ]))) | ||
| .is_err() | ||
| ); |
There was a problem hiding this comment.
♻️ refactor — The new assertions have no messages, and the last is_err() would also pass if demo were the provider rejected. AGENTS.md asks for descriptive assertion messages; expect_err plus a check on the provider ID pins the rejection to the provider without the opt-in.
| assert!( | |
| AuctionPlan::compile(config(BTreeMap::from([(id("demo"), demo.clone())]))).is_err() | |
| ); | |
| demo.allow_insecure_http = true; | |
| let plan = AuctionPlan::compile(config(BTreeMap::from([(id("demo"), demo.clone())]))) | |
| .expect("should compile an explicitly approved demo HTTP endpoint"); | |
| assert_eq!( | |
| plan.providers()[0].endpoint.as_str(), | |
| "http://pbs.example.com:8000/openrtb2/auction" | |
| ); | |
| let mut other = provider("standard"); | |
| other.endpoint = "http://other.example/bid".to_string(); | |
| assert!( | |
| AuctionPlan::compile(config(BTreeMap::from([ | |
| (id("demo"), demo), | |
| (id("other"), other) | |
| ]))) | |
| .is_err() | |
| ); | |
| assert!( | |
| AuctionPlan::compile(config(BTreeMap::from([(id("demo"), demo.clone())]))).is_err(), | |
| "should reject non-loopback HTTP without the per-provider opt-in" | |
| ); | |
| demo.allow_insecure_http = true; | |
| let plan = AuctionPlan::compile(config(BTreeMap::from([(id("demo"), demo.clone())]))) | |
| .expect("should compile an explicitly approved demo HTTP endpoint"); | |
| assert_eq!( | |
| plan.providers()[0].endpoint.as_str(), | |
| "http://pbs.example.com:8000/openrtb2/auction", | |
| "should keep HTTP and normalize the Prebid Server path" | |
| ); | |
| let mut other = provider("standard"); | |
| other.endpoint = "http://other.example/bid".to_string(); | |
| let error = AuctionPlan::compile(config(BTreeMap::from([ | |
| (id("demo"), demo), | |
| (id("other"), other), | |
| ]))) | |
| .expect_err("should reject HTTP for a provider that did not opt in"); | |
| assert!( | |
| error.to_string().contains("provider `other`"), | |
| "should attribute the rejection to the provider without the opt-in: {error:?}" | |
| ); |
| assert!(legacy.get("allow_insecure_http").is_none()); | ||
| assert!( | ||
| !serde_json::from_value::<ProviderConfig>(legacy) | ||
| .expect("should deserialize an existing provider without the opt-in") | ||
| .allow_insecure_http | ||
| ); | ||
| let mut opted_in = provider("prebid-server"); | ||
| opted_in.allow_insecure_http = true; | ||
| let encoded = | ||
| serde_json::to_value(opted_in).expect("should serialize the explicit HTTP opt-in"); | ||
| assert_eq!(encoded["allow_insecure_http"], true); | ||
| assert!( | ||
| serde_json::from_value::<ProviderConfig>(encoded) | ||
| .expect("should deserialize the explicit HTTP opt-in") | ||
| .allow_insecure_http | ||
| ); |
There was a problem hiding this comment.
♻️ refactor — These assertions also have no messages (AGENTS.md asks for descriptive assertion messages). This adds one to each so a failure says which part of the wire-shape contract broke.
| assert!(legacy.get("allow_insecure_http").is_none()); | |
| assert!( | |
| !serde_json::from_value::<ProviderConfig>(legacy) | |
| .expect("should deserialize an existing provider without the opt-in") | |
| .allow_insecure_http | |
| ); | |
| let mut opted_in = provider("prebid-server"); | |
| opted_in.allow_insecure_http = true; | |
| let encoded = | |
| serde_json::to_value(opted_in).expect("should serialize the explicit HTTP opt-in"); | |
| assert_eq!(encoded["allow_insecure_http"], true); | |
| assert!( | |
| serde_json::from_value::<ProviderConfig>(encoded) | |
| .expect("should deserialize the explicit HTTP opt-in") | |
| .allow_insecure_http | |
| ); | |
| assert!( | |
| legacy.get("allow_insecure_http").is_none(), | |
| "should omit the default opt-in from the serialized wire shape" | |
| ); | |
| assert!( | |
| !serde_json::from_value::<ProviderConfig>(legacy) | |
| .expect("should deserialize an existing provider without the opt-in") | |
| .allow_insecure_http, | |
| "should default the opt-in to false when the field is absent" | |
| ); | |
| let mut opted_in = provider("prebid-server"); | |
| opted_in.allow_insecure_http = true; | |
| let encoded = | |
| serde_json::to_value(opted_in).expect("should serialize the explicit HTTP opt-in"); | |
| assert_eq!( | |
| encoded["allow_insecure_http"], true, | |
| "should serialize an enabled opt-in" | |
| ); | |
| assert!( | |
| serde_json::from_value::<ProviderConfig>(encoded) | |
| .expect("should deserialize the explicit HTTP opt-in") | |
| .allow_insecure_http, | |
| "should round-trip an enabled opt-in" | |
| ); |
| /// explicitly approved demo/test endpoints. | ||
| #[serde(default, skip_serializing_if = "is_false")] |
There was a problem hiding this comment.
🤔 thinking — Nothing logs when a provider compiles with allow_insecure_http = true. A one-time log::warn! during AuctionPlan::compile naming the provider would make an accidentally enabled opt-in in a non-demo config visible in startup logs. Not blocking; it would sit near the canonicalize_endpoint call (outside this hunk, so no suggestion).
| | --------------------- | --------------- | ---------------------------------------------------------------------------------- | | ||
| | `protocol` | Required | `openrtb-2.6` | | ||
| | `profile` | `standard` | Typed OpenRTB behavior | | ||
| | `endpoint` | Required | Fixed absolute HTTPS endpoint | |
There was a problem hiding this comment.
⛏ nitpick — This row still says the endpoint must be HTTPS, but the next row and the new paragraph add HTTP exceptions. A pointer avoids the contradiction.
| | `endpoint` | Required | Fixed absolute HTTPS endpoint | | |
| | `endpoint` | Required | Fixed absolute HTTPS endpoint; see the HTTP exceptions below | |
|
|
||
| A local test bidder's `endpoint` may instead be plain HTTP to `127.0.0.1` or | ||
| `[::1]`. |
There was a problem hiding this comment.
⛏ nitpick — The new paragraph above already covers the 127.0.0.1 / [::1] exception, so this older sentence repeats it. Suggest dropping it.
| A local test bidder's `endpoint` may instead be plain HTTP to `127.0.0.1` or | |
| `[::1]`. |
Per the user, every feature is built once and works on every adapter in platform order, with platform gaps sent to EdgeZero (DEC-16, SH-5). T21 moves the identity routes, EC lifecycle, providers and write-back into one router in core that each adapter mounts; A2, A10, T8, T9, T17 and T18 shrink to turning it on per adapter and passing the conformance suite, and T7, T14 and A11 fold into A2 and A10. EZI-2a adds EdgeZero's local store for Axum development and EZI-4 binds Axum's geo provider from the manifest. Restack the Axum RC units on #1506: #1531 carries A1 in place of #1528, #1533 and #1534 close as copies of #1525 and #1526, and the image and Terraform units sit directly on #1532 so A6 and A7 stop waiting on A2. Record #1566, refresh the provider stack's fixes at their current heads, and flag #1535 as unplanned.
Summary
Draft baseline review unit. Not a current-main merge candidate, deployment approval or completed current-head CI claim.
Baseline and sequence
Head:
1cfba70a2efecda9ff522187c69359eda1fa6692. Base:baseline/protected-geo-20261110.Historical main anchor:
2dd968aeebeb2cc0d5110115f64e0ad58e2e1d13. Original demo: TS2423a2ed plus separately preserved dirty inputs. Matching EdgeZero: draft431, exact revision83d7a6296d285e135c4ea70021b19e6c8dca7e9f.The user selected compatible-baseline cleanup. Current-main integration with EdgeZero462 and the authenticated-forwarder incompatibility are deferred. Preserve current-main PR1525/1526 ready and1527/1528 draft, with their existing bases. No excluded1499 or independent1506 implementation, relay/privacy experiment, new transport fallback, group/codec/phase design or operational deployment is included.
Changes
Closes
Refs #1530. This unit completes one part of the baseline extraction and does not close the overall tracker on its own.
Spec
Not a new feature. This is extraction of existing demo behavior and bounded build/caller corrections. Original native and EC storage design/review records remain in the baseline where applicable; they are not presented as approval of later combined source. No new forwarding, consent, timer or provider policy is designed here.
Test plan
Focused auction-plan tests and strict Core lint pass. Fixture uses pbs.example.com, without changing opt-in assertions. Current-main PR1525 remains separate and ready; its hosted results are not relabeled as this baseline head's results.
The b67ffc6 baseline checkpoint has passing documented local gates: Fastly/default/reuse/native/Cloudflare/Spin suites, all adapter strict lint lanes, 17 cross-adapter parity cases, 1252 JS cases, CLI suites, formatting, clock/digest/proposal checks and changed-lines domain lint. Passing checks at121ebcc36 are reused only for unchanged relevant inputs; the final follow-up covers the CLI test-import and documentation-only corrections. These results are not a substitute for this PR's current-head hosted checks.
Checklist
Operational Terraform remains in trusted-server-deployer. Local checks and prototype tests do not qualify cloud access, fleet durability or a live publisher.
Hosted docs-build correction
The first hosted format-docs job caught an additional site-build gate that the original local formatter checkpoint had not run: a relative link from the native EC guide to a Terraform example outside the site. Native EC correction8b11dbad3 replaces it with the plain repository path; no dead-link check is disabled. That docs-only correction is carried through the existing upper PR identities. Native EC and final integration8985b111 both pass docs ESLint and VitePress build; the final docs formatter also passes. Runtime, dependency, image-build-context and test inputs are unchanged from b67ffc6, so their prior exact-input proofs are reused. Hosted current-head checks still control readiness.
Startup negative-probe correction
A hosted native EC case failed the no-listener probe while its child logged the expected startup validation failure. The old log has no socket endpoints, so the failure's exact subtype is not claimed confirmed. A separate controlled Linux reproduction demonstrates a successful TCP self-connection with no listening server. Native correction8490c691d distinguishes equal local/peer endpoints from a real listener; actual socket controls prove the self-connection is excluded and a real listener is still detected. Nonzero exit, expected diagnostic, secret-redaction and the original startup deadline remain required. This changes only the acceptance fixture, not production admission, transport or trust.
The earliest native unit and final9356c557 default/all-feature acceptance checks and strict lint pass, including both controls and the original failure case. Ignores remain explicit; earlier separately executed celld proof is not relabeled as a new ignored-case run. Correction is merged upward through existing PR identities without force-pushing or retargeting. Current-head hosted checks must pass; the old failure is retained rather than rerun-only waived. Runtime/API/dependency inputs remain unchanged. A fresh exact9356 producer build and real runtime smoke now pass: image
sha256:65a735d38e9dbc0573d0f6068a8b261bd202c43aea05085c2b2d6bccc91ebdbd, linux/amd64 UID/GID10001, including startup rejection and in-flight SIGTERM drain. Prior b67 image receipts remain separate; no image was pushed. All local parent runners are complete.Browser CI host compatibility
Preserved older browser failures show Playwright1.58.2 rejecting ubuntu-latest's new Ubuntu26.04 image before browser installation; later missing-executable failures are consequences, not passing browser tests. Nativeb1216763a pins only the browser job to ubuntu-24.04, leaving dependency versions, tests and assertions unchanged. Direct Actionlint1.7.12 passes at the owning native and final integration points; the first shim-only attempt could not run because no global version was configured, and is not counted as lint proof. No global tooling was changed. The only9356 ->ee7 delta is this workflow host/comment; runtime, Cargo, docs, tests and Docker-context inputs are unchanged. Actual browser execution on the supported hosted runner remains pending and must pass before readiness. No waiver or browser skip.