Repository navigation
Allow explicit per-provider HTTP auction endpoints #1525
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
706469b
925aaed
dcac90e
cd8977c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -135,6 +135,10 @@ pub struct ProviderConfig { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub profile: String, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Fixed provider endpoint. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub endpoint: String, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Permit unencrypted HTTP for this provider only. Defaults to false. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Use only for explicitly approved demo/test endpoints. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[serde(default, skip_serializing_if = "is_false")] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 praise — |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub allow_insecure_http: bool, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Optional profile-default timeout override. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[serde(default)] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub timeout_ms: Option<u32>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -314,7 +318,12 @@ impl AuctionPlan { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "provider `{id}` profile_config must be an object" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ))); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let endpoint = canonicalize_endpoint(&id, registration.id, &raw.endpoint)?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let endpoint = canonicalize_endpoint( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| &id, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| registration.id, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| &raw.endpoint, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raw.allow_insecure_http, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| )?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+321
to
+326
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔧 wrench: Nothing tells the operator that a provider is running in plaintext. An opted-in provider's requests carry the client IP ( The repo already warns for the equivalent publisher-origin case (
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let timeout_ms = raw | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .timeout_ms | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .unwrap_or(match registration.default_timeout { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -566,13 +575,14 @@ fn canonicalize_endpoint( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| provider_id: &ProviderId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| profile_id: &str, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| value: &str, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| allow_insecure_http: bool, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> Result<CanonicalProviderEndpoint, Report<TrustedServerError>> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut endpoint = Url::parse(value).map_err(|error| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| configuration_error(format!( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "provider `{provider_id}` endpoint must be an absolute HTTPS URL: {error}" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⛏ nitpick — Both endpoint errors still say HTTPS is required, which misleads an operator who set
Suggested change
The second message at line 592 (outside the diff hunk, so no one-click suggestion) could become something like |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| )) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| })?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if endpoint.scheme() != "https" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if !(endpoint.scheme() == "https" || (allow_insecure_http && endpoint.scheme() == "http")) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⛏ nitpick: For an opted-in provider, a rejected "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"
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| || endpoint.host_str().is_none() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| || !endpoint.username().is_empty() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| || endpoint.password().is_some() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -598,6 +608,10 @@ fn canonicalize_endpoint( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Ok(CanonicalProviderEndpoint(endpoint)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn is_false(value: &bool) -> bool { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ♻️ refactor: // 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 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| !*value | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn normalize_prebid_server_endpoint(endpoint: &mut Url) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| match endpoint.path() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "" | "/" => endpoint.set_path("/openrtb2/auction"), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -644,6 +658,7 @@ mod tests { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn provider(profile: &str) -> ProviderConfig { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ProviderConfig { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| allow_insecure_http: false, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| protocol: "openrtb-2.6".to_string(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| profile: profile.to_string(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| endpoint: "https://bid.example/openrtb2/auction".to_string(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1123,6 +1138,82 @@ mod tests { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert!(AuctionPlan::compile(config(BTreeMap::from([(id("aps"), aps)]))).is_err()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn insecure_http_requires_an_explicit_per_provider_opt_in() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut demo = provider("prebid-server"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| demo.endpoint = "http://pbs.example.com:8000".to_string(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1152
to
+1156
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔧 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
With this suggestion applied, both mutants fail this test (
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut other = provider("standard"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| other.endpoint = "http://other.example.com/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" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn insecure_http_opt_in_does_not_allow_credentials_fragments_or_other_schemes() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for endpoint in [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "http://user:pass@pbs.example.com/bid", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "http://pbs.example.com/bid#fragment", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "ftp://pbs.example.com/bid", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "/relative", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ] { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut raw = provider("prebid-server"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raw.allow_insecure_http = true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raw.endpoint = endpoint.to_string(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert!( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AuctionPlan::compile(config(BTreeMap::from([(id("demo"), raw)]))).is_err(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "should reject {endpoint}" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn insecure_http_default_preserves_existing_provider_wire_shape() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let legacy = serde_json::to_value(provider("prebid-server")) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .expect("should serialize the default provider"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert!( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| legacy.get("allow_insecure_http").is_none(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "should omit the default HTTP opt-in field" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert!( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| !serde_json::from_value::<ProviderConfig>(legacy) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .expect("should deserialize the default provider") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .allow_insecure_http, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "should keep HTTP disabled 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 opted-in provider"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert_eq!( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| encoded["allow_insecure_http"], true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "should publish the explicit HTTP opt-in" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert!( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| serde_json::from_value::<ProviderConfig>(encoded) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .expect("should deserialize the opted-in provider") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .allow_insecure_http, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "should retain the explicit HTTP opt-in across serialization" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn compiler_normalizes_only_prebid_server_origin_and_canonical_paths() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (configured, expected) in [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -773,15 +773,16 @@ registered integration selected by `[auction].mediator`. | |||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| Common provider fields and defaults: | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| | Field | Default | Meaning | | ||||||||||||||||||||||||||||||||||||||||||||
| | ---------------- | --------------- | -------------------------------------------------------------- | | ||||||||||||||||||||||||||||||||||||||||||||
| | `protocol` | Required | `openrtb-2.6` | | ||||||||||||||||||||||||||||||||||||||||||||
| | `profile` | `standard` | Typed OpenRTB behavior | | ||||||||||||||||||||||||||||||||||||||||||||
| | `endpoint` | Required | Fixed absolute HTTPS endpoint | | ||||||||||||||||||||||||||||||||||||||||||||
| | `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 | | ||||||||||||||||||||||||||||||||||||||||||||
| | Field | Default | Meaning | | ||||||||||||||||||||||||||||||||||||||||||||
| | --------------------- | --------------- | ---------------------------------------------------------------------------------- | | ||||||||||||||||||||||||||||||||||||||||||||
| | `protocol` | Required | `openrtb-2.6` | | ||||||||||||||||||||||||||||||||||||||||||||
| | `profile` | `standard` | Typed OpenRTB behavior | | ||||||||||||||||||||||||||||||||||||||||||||
| | `endpoint` | Required | Fixed absolute HTTPS endpoint | | ||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⛏ nitpick — The
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||
| | `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 | | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+780
to
786
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ♻️ refactor: The
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||
| APS normally uses `all_eligible`, which sends every compatible banner slot but | ||||||||||||||||||||||||||||||||||||||||||||
| never another provider's bidder parameters. `explicit` providers receive only | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
♻️ 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_finishandDebugConfig::sandbox_metrics_enabledinsettings.rs) document the rollback consequence. It applies here too, becauseProviderConfigdenies unknown fields.