Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

35 changes: 21 additions & 14 deletions crates/trusted-server-adapter-fastly/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,9 @@ use trusted_server_core::settings::{CollectTiming, ProxyAssetRoute, Settings};
use trusted_server_core::settings_data::{DEFAULT_CONFIG_STORE_ID, get_settings_from_config_store};
use trusted_server_core::tester_cookie::{handle_clear_tester, handle_set_tester};
use trusted_server_core::trace::{TraceMetadata, TracePreDispatchHook};
use trusted_server_identity_providers::{build_coordinator_with, partner_registry};
use trusted_server_identity_providers::{
build_coordinator_with, partner_registry, pull_sync_providers,
};

use crate::middleware::{AuthMiddleware, FinalizeResponseMiddleware};
use crate::platform::{
Expand Down Expand Up @@ -361,7 +363,6 @@ pub(crate) struct EcFinalizeState {
pub(crate) use_finalize_kv: bool,
pub(crate) eids_cookie: Option<String>,
pub(crate) sharedid_cookie: Option<String>,
pub(crate) is_real_browser: bool,
/// Per-request services carried to the entry point so the pull-sync
/// dispatcher can reuse the same platform HTTP client.
pub(crate) services: RuntimeServices,
Expand Down Expand Up @@ -435,8 +436,9 @@ impl<'a> ProviderRequestSignals<'a> {
/// Captures the request signals for after-response identity lookups on a
/// permitted publisher navigation.
///
/// Returns `None` when no after-response provider is enabled, the request has
/// no EC, or the identity graph is unavailable.
/// Returns `None` when no after-response provider (including a pull-sync
/// partner) is enabled, the request has no EC, or the identity graph is
/// unavailable.
fn prepare_after_response_lookups(
state: &AppState,
req: &Request,
Expand All @@ -447,7 +449,8 @@ fn prepare_after_response_lookups(
.ec
.identity_providers
.iter()
.any(|config| config.enabled && config.collect == CollectTiming::AfterResponse);
.any(|config| config.enabled && config.collect == CollectTiming::AfterResponse)
|| pull_sync_providers(&state.settings.ec).next().is_some();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔧 Skip the after-response row read when nothing is due

This line sends every pull-sync deployment through run_after_response_lookups (main.rs:730). That function calls graph.load_snapshot (main.rs:761) before it dispatches. Finalize issues the follow-up permit when the page-load write "succeeded or had nothing to change" (finalize.rs:221-224), so a steady-state returning visitor reaches that read on every navigation.

The legacy dispatcher avoided this read on purpose. It filtered partners against the request snapshot first ("a dispatch with nothing to pull must not add a KV operation", base ec/pull_sync.rs:158-160), so a row that held every pull UID cost no KV operation. Now a returning visitor whose pull sources are all held costs two reads and no partner request: this one, plus the commit_after_response_lookups re-read from the open thread on pull_sync.rs:60.

Proposed fix (apply manually: touches coordinator/mod.rs and main.rs, outside this hunk; not compiled). Check the request's snapshot, which finalize leaves holding the committed row, and return before the live read when no after-response provider has a due source. A source held in that snapshot is still held in the live row unless consent was withdrawn, and a withdrawal blocks the lookup anyway.

// coordinator/mod.rs
/// Whether any provider has a due source in `entry`, ignoring consent and
/// rate limits, so a caller can skip a row read that cannot lead to a lookup.
#[must_use]
pub fn any_due(&self, entry: Option<&KvEntry>, now: UnixSeconds) -> bool {
    self.providers.iter().any(|registered| {
        !entry.is_some_and(|entry| entry.opted_out(&registered.id, now))
            && !Self::due_sources(registered, entry, now).0.is_empty()
    })
}
// main.rs, run_after_response_lookups, before `graph.load_snapshot`
let request_entry = ec_state.ec_context.kv_snapshot().entry_for(&lookups.ec_id);
if !coordinator.any_due(request_entry, UnixSeconds::new(current_timestamp())) {
    log::debug!("After-response lookups skipped: nothing due");
    return;
}

A NotRead or Failed request snapshot yields no entry, so every source counts as due and the live read still runs.

if !has_after_response_provider {
return None;
}
Expand All @@ -472,13 +475,15 @@ async fn dispatch_identity_lookups(
req: &Request,
ec: &mut EcRequestState,
) -> Option<IdentityLookups> {
if !state
// Pull-sync partners run after the response, so only page providers
// count here.
let any_provider = state
.settings
.ec
.identity_providers
.iter()
.any(|config| config.enabled)
{
.any(|config| config.enabled && config.collect == CollectTiming::Page);
if !any_provider {
return None;
}
let partners = match state.partner_registry() {
Expand Down Expand Up @@ -559,7 +564,6 @@ impl EcRequestState {
use_finalize_kv: self.finalize_kv_graph.is_some(),
eids_cookie: self.eids_cookie,
sharedid_cookie: self.sharedid_cookie,
is_real_browser: self.is_real_browser,
services: self.services,
}
}
Expand Down Expand Up @@ -935,8 +939,6 @@ async fn run_named_route(
/// graph + partner registry + rate limiter, with a default EC context for
/// response finalization.
fn run_batch_sync(state: &AppState, services: &RuntimeServices, req: Request) -> Response {
let device_signals = device_signals_for(&req);
let is_real_browser = device_signals.looks_like_browser();
let eids_cookie = crate::extract_cookie_value(&req, COOKIE_TS_EIDS);
let sharedid_cookie = crate::extract_cookie_value(&req, COOKIE_SHAREDID);

Expand All @@ -954,7 +956,6 @@ fn run_batch_sync(state: &AppState, services: &RuntimeServices, req: Request) ->
use_finalize_kv: false,
eids_cookie,
sharedid_cookie,
is_real_browser,
services: services.clone(),
});
response
Expand Down Expand Up @@ -3206,7 +3207,10 @@ mod tests {
.get::<super::EcFinalizeState>()
.expect("fallback response should carry EcFinalizeState");
assert!(
finalize.is_real_browser,
finalize
.ec_context
.device_signals()
.is_some_and(DeviceSignals::looks_like_browser),
"browser-shaped JA4/H2 signals from the request extension must mark the request as a real browser"
);
}
Expand All @@ -3225,7 +3229,10 @@ mod tests {
.get::<super::EcFinalizeState>()
.expect("fallback response should carry EcFinalizeState");
assert!(
!finalize.is_real_browser,
!finalize
.ec_context
.device_signals()
.is_some_and(DeviceSignals::looks_like_browser),
"a request without captured device signals should not be classified as a real browser"
);
}
Expand Down
71 changes: 0 additions & 71 deletions crates/trusted-server-adapter-fastly/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,6 @@ use trusted_server_core::ec::finalize::ec_finalize_response;
use trusted_server_core::ec::kv::KvIdentityGraph;
use trusted_server_core::ec::page_load::NavigationPermit;
use trusted_server_core::ec::provider::ClientSignals;
use trusted_server_core::ec::pull_sync::{
PullSyncContext, build_pull_sync_context, dispatch_pull_sync,
};
use trusted_server_core::ec::registry::PartnerRegistry;
use trusted_server_core::error::TrustedServerError;
use trusted_server_core::integrations::RequestFilterEffects;
Expand Down Expand Up @@ -576,7 +573,6 @@ fn edgezero_main(
request_filter_effects.as_ref(),
counters.as_ref(),
);
run_edgezero_pull_sync_after_send(settings, &partner_registry, &ec_state);
run_after_response_lookups(
settings,
&partner_registry,
Expand Down Expand Up @@ -607,11 +603,6 @@ fn edgezero_main(
request_filter_effects.as_ref(),
counters.as_ref(),
);
run_edgezero_pull_sync_after_send(
&settings,
&partner_registry,
&ec_state,
);
run_after_response_lookups(
&settings,
&partner_registry,
Expand Down Expand Up @@ -730,33 +721,6 @@ fn apply_edgezero_ec_finalize(
Ok(partner_registry)
}

fn run_edgezero_pull_sync_after_send(
settings: &Settings,
partner_registry: &PartnerRegistry,
ec_state: &EcFinalizeState,
) {
if !ec_state.is_real_browser {
return;
}

let prepared_context = build_pull_sync_context(&ec_state.ec_context, partner_registry);
let Some((context, kv)) =
prepare_pull_sync_after_send(prepared_context, || require_identity_graph(settings))
else {
return;
};

let limiter = FastlyRateLimiter::new(RATE_COUNTER_NAME);
dispatch_pull_sync(
settings,
&kv,
partner_registry,
&limiter,
&context,
&ec_state.services,
);
}

/// Runs after-response identity lookups and applies their follow-up write.
///
/// Called once the response has been sent, so provider latency never delays
Expand Down Expand Up @@ -831,24 +795,6 @@ fn run_after_response_lookups(
log::info!("After-response lookups: write outcome={outcome}");
}

fn prepare_pull_sync_after_send<F>(
context: Option<PullSyncContext>,
graph_factory: F,
) -> Option<(PullSyncContext, KvIdentityGraph)>
where
F: FnOnce() -> Result<KvIdentityGraph, Report<TrustedServerError>>,
{
let context = context?;
let kv = match graph_factory() {
Ok(kv) => kv,
Err(err) => {
log::debug!("Pull sync: identity graph unavailable, skipping: {err:?}");
return None;
}
};
Some((context, kv))
}

/// Sends a finalized `EdgeZero` response to the client.
///
/// Streaming `EdgeZero` bodies commit headers first, then pipe chunks to Fastly's
Expand Down Expand Up @@ -1291,23 +1237,6 @@ mod tests {
}
}

#[test]
fn pull_sync_noop_states_skip_post_send_graph_factory() {
let calls = std::cell::Cell::new(0);
let result = prepare_pull_sync_after_send(None, || {
calls.set(calls.get() + 1);
Err(Report::new(TrustedServerError::KvStore {
store_name: "unexpected".to_owned(),
message: "graph factory should not run".to_owned(),
}))
});
assert!(
result.is_none(),
"a skipped pull-sync plan should return none"
);
assert_eq!(calls.get(), 0, "should not invoke the graph factory");
}

#[test]
fn trace_path_classification_survives_core_request_conversion() {
// `main` skips client-IP sanitization when the native path is a trace
Expand Down
74 changes: 72 additions & 2 deletions crates/trusted-server-core/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use serde::{Deserialize, Deserializer, Serialize, Serializer};
use validator::{Validate, ValidationError, ValidationErrors};

use crate::ec::provider::validate_enabled_configs;
use crate::ec::registry::PartnerRegistry;
use crate::ec::registry::{PULL_SYNC_KIND, PartnerRegistry};
use crate::error::TrustedServerError;
use crate::integrations::{
adserver_mock::AdServerMockConfig,
Expand Down Expand Up @@ -352,6 +352,20 @@ fn validate_identity_providers(ec: &Ec) -> Result<(), Report<TrustedServerError>
if ec.identity_providers.is_empty() {
return Ok(());
}
// Pull-sync providers come only from partner config, which carries the
// pull token and rate limit; configuring the kind directly bypasses both.
if let Some(config) = ec
.identity_providers
.iter()
.find(|config| config.kind() == PULL_SYNC_KIND)
{
return Err(Report::new(TrustedServerError::Configuration {
message: format!(
"ec.identity_providers: provider `{}` uses kind `{PULL_SYNC_KIND}`; configure pull sync on its [[ec.partners]] entry instead",
config.id
),
}));
}
let partners = PartnerRegistry::from_ec(ec)?;
validate_enabled_configs(&ec.identity_providers, &partners).map_err(|report| {
let detail = report
Expand All @@ -363,7 +377,24 @@ fn validate_identity_providers(ec: &Ec) -> Result<(), Report<TrustedServerError>
report.change_context(TrustedServerError::Configuration {
message: format!("ec.identity_providers: {detail}"),
})
})
})?;
// Pull-sync partners run as providers too; a source claimed by both would
// fail the coordinator build on every request.
for config in ec.identity_providers.iter().filter(|config| config.enabled) {
if let Some(source) = config.sources.iter().find(|source| {
partners
.find_by_source_domain(source)
.is_some_and(|partner| partner.pull_sync_enabled && partner.pull_sync_url.is_some())
}) {
return Err(Report::new(TrustedServerError::Configuration {
message: format!(
"ec.identity_providers: source `{source}` of provider `{}` is also claimed by a pull-sync partner",
config.id
),
}));
}
}
Ok(())
}

fn validate_js_asset_proxy_config(settings: &Settings) -> Result<(), Report<TrustedServerError>> {
Expand Down Expand Up @@ -1399,6 +1430,45 @@ gam_network_id = "99999"
);
}

#[test]
fn runtime_validation_rejects_source_claimed_by_provider_and_pull_sync() {
let mut settings = settings_with_identity_provider(serde_json::json!({}));
settings.ec.partners = vec![
serde_json::from_value(serde_json::json!({
"name": "ids",
"source_domain": "ids.example.com",
"pull_sync_enabled": true,
"pull_sync_url": "https://sync.example.com/pull",
"pull_sync_allowed_domains": ["sync.example.com"],
"ts_pull_token": "example-pull-token-32-bytes-minimum",
}))
.expect("should build partner"),
];

let err = validate_settings_for_runtime(&settings)
.expect_err("should reject a source claimed twice");

assert!(
err.to_string().contains("pull-sync partner"),
"should explain the double claim: {err:?}"
);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔧 Cover the enabled = false bypass of the double-claim check

The new check in validate_identity_providers filters on config.enabled, but only the rejection is tested. The test below passes at this head, and it fails if a later change drops the filter.

Suggested change
}
}
#[test]
fn runtime_validation_accepts_disabled_provider_sharing_a_pull_sync_source() {
let mut settings = settings_with_identity_provider(serde_json::json!({ "enabled": false }));
settings.ec.partners = vec![
serde_json::from_value(serde_json::json!({
"name": "ids",
"source_domain": "ids.example.com",
"pull_sync_enabled": true,
"pull_sync_url": "https://sync.example.com/pull",
"pull_sync_allowed_domains": ["sync.example.com"],
"ts_pull_token": "example-pull-token-32-bytes-minimum",
}))
.expect("should build partner"),
];
validate_settings_for_runtime(&settings)
.expect("should accept a disabled provider sharing a pull-sync source");
}


#[test]
fn runtime_validation_rejects_a_directly_configured_pull_sync_provider() {
let settings = settings_with_identity_provider(serde_json::json!({
"kind": PULL_SYNC_KIND,
}));

let err = validate_settings_for_runtime(&settings)
.expect_err("should reject the pull-sync kind in identity providers");

assert!(
err.to_string().contains("[[ec.partners]]"),
"should point to partner config: {err:?}"
);
}

#[test]
fn runtime_validation_rejects_provider_source_without_partner() {
let settings = settings_with_identity_provider(serde_json::json!({
Expand Down
4 changes: 3 additions & 1 deletion crates/trusted-server-core/src/constants.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
use http::header::HeaderName;

pub const COOKIE_TS_EC: &str = "ts-ec";
/// Short-lived signed proof that the current EC row has every pull-partner UID.
/// Retired pull-sync completeness marker; leftover cookies are expired.
pub const COOKIE_TS_EC_PULL_COMPLETE: &str = "ts-ec-pull-complete";
/// Cookie written by the Trusted Server JS SDK containing a standard-base64-encoded
/// JSON array of Extended User IDs (`[{ source, uids }]`) from identity providers.
pub const COOKIE_TS_EIDS: &str = "ts-eids";
/// JS-readable mirror of provider-owned bidstream UIDs (spec §10).
pub const COOKIE_TS_EIDS_MIRROR: &str = "__Host-ts-eids-mirror";
pub const COOKIE_TS_TESTER: &str = "ts-tester";
pub const COOKIE_SHAREDID: &str = "sharedId";

Expand Down
17 changes: 14 additions & 3 deletions crates/trusted-server-core/src/cookies/template_cache_policy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

use std::collections::{HashMap, HashSet};

use crate::constants::{COOKIE_SHAREDID, COOKIE_TS_EC, COOKIE_TS_EIDS};
use crate::constants::{COOKIE_SHAREDID, COOKIE_TS_EC, COOKIE_TS_EIDS, COOKIE_TS_EIDS_MIRROR};
use crate::platform::TemplateCookieValue;

/// Whether the request can share a template and its explicit cookie dimensions.
Expand Down Expand Up @@ -32,7 +32,13 @@ pub(crate) fn validate_cookie_names(
return Err(format!("{field} contains invalid cookie name `{name}`"));
}
if field == "template_cache_key_cookies"
&& [COOKIE_TS_EC, COOKIE_TS_EIDS, COOKIE_SHAREDID].contains(&name.as_str())
&& [
COOKIE_TS_EC,
COOKIE_TS_EIDS,
COOKIE_TS_EIDS_MIRROR,
COOKIE_SHAREDID,
]
.contains(&name.as_str())
{
return Err(format!(
"{field} must not key on Trusted Server identity cookie `{name}`"
Expand Down Expand Up @@ -232,7 +238,12 @@ mod tests {

#[test]
fn template_cookie_policy_rejects_identity_keys_but_allows_bypass() {
for name in [COOKIE_TS_EC, COOKIE_TS_EIDS, COOKIE_SHAREDID] {
for name in [
COOKIE_TS_EC,
COOKIE_TS_EIDS,
COOKIE_TS_EIDS_MIRROR,
COOKIE_SHAREDID,
] {
let error = validate_cookie_names(&names(&[name]), &[])
.expect_err("should reject identity cookie keys");
assert!(
Expand Down
Loading
Loading