diff --git a/CHANGELOG.md b/CHANGELOG.md index 317c2331b..17e72edfb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,10 @@ - Added [`Dsn::org_id`](https://docs.rs/sentry-types/latest/sentry_types/struct.Dsn.html#method.org_id), which parses the Sentry SaaS organization ID from DSN hosts such as `o123.ingest.sentry.io` ([#1202](https://github.com/getsentry/sentry-rust/pull/1202)). - Added [`ClientOptions::org_id`](https://docs.rs/sentry-core/latest/sentry_core/struct.ClientOptions.html#method.org_id) and [`ClientOptions::strict_trace_continuation`](https://docs.rs/sentry-core/latest/sentry_core/struct.ClientOptions.html#method.strict_trace_continuation) ([#1203](https://github.com/getsentry/sentry-rust/pull/1203)). These options control how traces are continued and can help prevent traces from third-party services, which happen to be instrumented with Sentry, from being continued. +### Improvements + +- Improved trace continuation safety by rejecting incoming traces with incompatible `sentry-org_id` values when starting transactions, according to [strict trace continuation rules](https://develop.sentry.dev/sdk/foundations/trace-propagation/#continuing-traces) ([#1218](https://github.com/getsentry/sentry-rust/pull/1218)). + ## 0.48.4 ### New Features diff --git a/sentry-actix/src/lib.rs b/sentry-actix/src/lib.rs index 7e2c7e94e..6ea7ad306 100644 --- a/sentry-actix/src/lib.rs +++ b/sentry-actix/src/lib.rs @@ -482,6 +482,8 @@ mod tests { use futures::future::join_all; use sentry::Level; + use sentry_core::protocol::{Context, EnvelopeItem, TraceContext}; + use sentry_core::Envelope; use super::*; @@ -707,6 +709,83 @@ mod tests { assert_eq!(events.len(), 1); } + fn trace_context_from_single_transaction(envelopes: &[Envelope]) -> TraceContext { + let [envelope] = envelopes else { + panic!("Expected exactly one envelope"); + }; + + let mut items = envelope.items(); + let Some(EnvelopeItem::Transaction(transaction)) = items.next() else { + panic!("Expected a transaction envelope item"); + }; + assert!(items.next().is_none(), "expected only one envelope item"); + + match transaction.contexts.get("trace") { + Some(Context::Trace(trace)) => *trace.clone(), + unexpected => panic!("expected trace context, got {unexpected:#?}"), + } + } + + fn run_request_with_org_ids(incoming_org_id: &str, client_org_id: &str) -> Vec { + sentry::test::with_captured_envelopes_options( + || { + block_on(async { + let app = init_service( + App::new() + .wrap( + Sentry::builder() + .with_hub(Hub::current()) + .start_transaction(true) + .finish(), + ) + .service(web::resource("/test").to(HttpResponse::Ok)), + ) + .await; + + let req = TestRequest::get() + .uri("/test") + .insert_header(( + "sentry-trace", + "09e04486820349518ac7b5d2adbf6ba5-9cf635fa5b870b3a-1", + )) + .insert_header(("baggage", format!("sentry-org_id={incoming_org_id}"))) + .to_request(); + let res = call_service(&app, req).await; + assert!(res.status().is_success()); + }) + }, + sentry::ClientOptions::new() + .org_id(client_org_id.parse().unwrap()) + .strict_trace_continuation(true) + .traces_sample_rate(1.0), + ) + } + + #[actix_web::test] + async fn test_transaction_continues_matching_org_id() { + let envelopes = run_request_with_org_ids("42", "42"); + let trace = trace_context_from_single_transaction(&envelopes); + assert_eq!( + trace.trace_id.to_string(), + "09e04486820349518ac7b5d2adbf6ba5" + ); + assert_eq!( + trace.parent_span_id.map(|span_id| span_id.to_string()), + Some("9cf635fa5b870b3a".to_owned()) + ); + } + + #[actix_web::test] + async fn test_transaction_rejects_mismatched_org_id() { + let envelopes = run_request_with_org_ids("43", "42"); + let trace = trace_context_from_single_transaction(&envelopes); + assert_ne!( + trace.trace_id.to_string(), + "09e04486820349518ac7b5d2adbf6ba5" + ); + assert_eq!(trace.parent_span_id, None); + } + /// Ensures transaction name can be overridden in handler scope. #[actix_web::test] async fn test_override_transaction_name() { diff --git a/sentry-core/src/performance/headers.rs b/sentry-core/src/performance/headers.rs index ce6a12736..2ef82a0b1 100644 --- a/sentry-core/src/performance/headers.rs +++ b/sentry-core/src/performance/headers.rs @@ -3,6 +3,7 @@ use std::error::Error; use std::fmt::{Display, Formatter, Result as FmtResult}; +#[cfg(feature = "client")] use sentry_types::protocol::v7::OrganizationId; use crate::protocol::{SpanId, TraceId}; @@ -11,6 +12,7 @@ use crate::protocol::{SpanId, TraceId}; type Header<'h> = (&'h str, &'h str); /// The baggage key for the Sentry org ID. +#[cfg(feature = "client")] const SENTRY_ORG_ID: &str = "sentry-org_id"; /// The [trace propagation] context. @@ -31,6 +33,7 @@ pub struct TracePropagationContext { pub(crate) trace_id: TraceId, pub(crate) span_id: SpanId, pub(super) sampled: Option, + #[cfg(feature = "client")] pub(super) org_id: Option, } @@ -60,6 +63,7 @@ impl TracePropagationContext { trace_id, span_id, sampled: None, + #[cfg(feature = "client")] org_id: None, } } @@ -76,7 +80,8 @@ impl TracePropagationContext { trace_id, span_id, sampled, - org_id: _, + #[cfg(feature = "client")] + org_id: _, } = self; let sampled_suffix = sampled @@ -94,6 +99,7 @@ impl TracePropagationContext { I: IntoIterator>, { let mut context_result = Err(HeaderParseError::Missing); + #[cfg(feature = "client")] let mut baggage = SentryBaggage::default(); for (header, value) in headers { @@ -104,14 +110,20 @@ impl TracePropagationContext { .map_or(context_result, Ok) .map_err(|_| HeaderParseError::Invalid); } else if header.eq_ignore_ascii_case("baggage") { + #[cfg(feature = "client")] baggage.update_from_header(value); } } let context = context_result?; + #[cfg(feature = "client")] let SentryBaggage { org_id } = baggage; - Ok(TracePropagationContext { org_id, ..context }) + Ok(TracePropagationContext { + #[cfg(feature = "client")] + org_id, + ..context + }) } /// Attempts to construct a [`TracePropagationContext`] from the given Sentry trace header. @@ -133,6 +145,7 @@ impl TracePropagationContext { trace_id, span_id, sampled, + #[cfg(feature = "client")] org_id: None, }) } @@ -150,7 +163,8 @@ where trace_id, span_id, sampled, - org_id: _, + #[cfg(feature = "client")] + org_id: _, } = TracePropagationContext::try_from_headers(headers).ok()?; Some(SentryTrace { @@ -192,6 +206,7 @@ impl From for TracePropagationContext { trace_id: trace.trace_id, span_id: trace.span_id, sampled: trace.sampled, + #[cfg(feature = "client")] org_id: None, } } @@ -211,11 +226,13 @@ impl std::fmt::Display for SentryTrace { /// A struct containing known Sentry baggage values. /// /// For now, this only includes the `org_id`, but we can add more values as we support them. +#[cfg(feature = "client")] #[derive(Debug, Default)] struct SentryBaggage { org_id: Option, } +#[cfg(feature = "client")] impl SentryBaggage { /// Update `self` with the known Sentry baggage values in the provided [baggage header]. /// @@ -264,6 +281,7 @@ mod tests { trace_id, span_id: parent_trace_id, sampled: Some(false), + #[cfg(feature = "client")] org_id: None, } ); @@ -277,6 +295,7 @@ mod tests { assert_eq!(parsed, trace); } + #[cfg(feature = "client")] #[test] fn parses_baggage_org_id() { let trace = TracePropagationContext::try_from_headers([ @@ -291,6 +310,7 @@ mod tests { assert_eq!(trace.org_id, Some("123".parse().unwrap())); } + #[cfg(feature = "client")] #[test] fn parses_baggage_org_id_with_unrelated_fields() { let trace = TracePropagationContext::try_from_headers([ @@ -308,6 +328,7 @@ mod tests { assert_eq!(trace.org_id, Some("123".parse().unwrap())); } + #[cfg(feature = "client")] #[test] fn accepts_mixed_case_baggage_header_name() { let trace = TracePropagationContext::try_from_headers([ @@ -322,6 +343,7 @@ mod tests { assert_eq!(trace.org_id, Some("123".parse().unwrap())); } + #[cfg(feature = "client")] #[test] fn treats_malformed_baggage_org_id_as_absent() { let trace = TracePropagationContext::try_from_headers([ diff --git a/sentry-core/src/performance/mod.rs b/sentry-core/src/performance/mod.rs index 267bda1a4..1f1678c83 100644 --- a/sentry-core/src/performance/mod.rs +++ b/sentry-core/src/performance/mod.rs @@ -6,7 +6,9 @@ use std::time::SystemTime; #[cfg(feature = "client")] use sentry_types::protocol::v7::client_report::Reason as ClientReportReason; -use sentry_types::protocol::v7::{OrganizationId, SpanId}; +#[cfg(feature = "client")] +use sentry_types::protocol::v7::OrganizationId; +use sentry_types::protocol::v7::SpanId; #[cfg(feature = "client")] use crate::clientoptions::TracesSamplingStrategy; @@ -116,10 +118,7 @@ pub struct TransactionContext { parent_span_id: Option, span_id: protocol::SpanId, sampled: Option, - #[cfg_attr( - not(test), - expect(dead_code, reason = "used by future strict trace continuation") - )] + #[cfg(feature = "client")] incoming_org_id: Option, custom: Option, } @@ -156,6 +155,7 @@ impl TransactionContext { parent_span_id: None, span_id: Default::default(), sampled: None, + #[cfg(feature = "client")] incoming_org_id: None, custom: None, } @@ -203,6 +203,7 @@ impl TransactionContext { parent_span_id: None, span_id: Default::default(), sampled: None, + #[cfg(feature = "client")] incoming_org_id: None, custom: None, }) @@ -234,6 +235,7 @@ impl TransactionContext { trace_id, span_id: context_span_id, sampled, + #[cfg(feature = "client")] org_id, } = context; @@ -243,6 +245,7 @@ impl TransactionContext { trace_id, parent_span_id: Some(context_span_id), sampled, + #[cfg(feature = "client")] incoming_org_id: org_id, span_id: span_id.unwrap_or_default(), custom: None, @@ -283,6 +286,7 @@ impl TransactionContext { parent_span_id: Some(parent_span_id), span_id: protocol::SpanId::default(), sampled, + #[cfg(feature = "client")] incoming_org_id: None, custom: None, } @@ -369,6 +373,17 @@ impl TransactionContext { ctx: TransactionContext::new(name, op), } } + + /// Clears incoming trace state so the transaction starts a new trace. + #[cfg(feature = "client")] + fn reject_incoming_trace(&mut self) { + ( + self.trace_id, + self.parent_span_id, + self.sampled, + self.incoming_org_id, + ) = Default::default(); + } } /// A transaction context builder created by [`TransactionContext::builder`]. @@ -654,6 +669,19 @@ fn transaction_sample_rate( } } +#[cfg(feature = "client")] +fn should_continue_trace( + incoming: Option, + sdk: Option, + strict: bool, +) -> bool { + match (incoming, sdk) { + (Some(incoming), Some(sdk)) => incoming == sdk, + (Some(_), None) | (None, Some(_)) => !strict, + (None, None) => true, + } +} + /// Determine whether the new transaction should be sampled. #[cfg(feature = "client")] impl Client { @@ -720,15 +748,29 @@ impl<'a> TransactionData<'a> { impl Transaction { #[cfg(feature = "client")] - fn new(client: Option>, ctx: TransactionContext) -> Self { + fn new(client: Option>, mut ctx: TransactionContext) -> Self { let ((sampled, sample_rate), transaction) = match client.as_ref() { - Some(client) => ( - client.determine_sampling_decision(&ctx), - Some(protocol::Transaction { - name: Some(ctx.name), - ..Default::default() - }), - ), + Some(client) => { + let options = client.options(); + let incoming_org_id = ctx.incoming_org_id; + let sdk_org_id = options.org_id.or_else(|| options.dsn.as_ref()?.org_id()); + + if !should_continue_trace( + incoming_org_id, + sdk_org_id, + options.strict_trace_continuation, + ) { + ctx.reject_incoming_trace(); + } + + ( + client.determine_sampling_decision(&ctx), + Some(protocol::Transaction { + name: Some(ctx.name), + ..Default::default() + }), + ) + } None => ( ( ctx.sampled.unwrap_or(false), diff --git a/sentry-core/tests/trace_continuation.rs b/sentry-core/tests/trace_continuation.rs new file mode 100644 index 000000000..4acb64b98 --- /dev/null +++ b/sentry-core/tests/trace_continuation.rs @@ -0,0 +1,147 @@ +#![cfg(feature = "test")] + +use std::sync::Arc; + +use sentry_core::protocol::{SpanId, TraceId}; +use sentry_core::test::TestTransport; +use sentry_core::{Client, ClientOptions, Hub, Transaction}; + +/// Fixture for starting a transaction from incoming trace headers with a configured client. +struct TraceContinuationScenario { + incoming_trace_id: TraceId, + incoming_parent_span_id: SpanId, + transaction: Transaction, +} + +impl TraceContinuationScenario { + /// Starts a transaction with the given incoming and SDK organization IDs. + fn run( + incoming_org_id: Option<&str>, + sdk_org_id: Option<&str>, + strict_trace_continuation: bool, + ) -> Self { + let options = sdk_org_id + .map(|org_id| ClientOptions::new().org_id(org_id.parse().unwrap())) + .unwrap_or_default() + .strict_trace_continuation(strict_trace_continuation) + .traces_sample_rate(0.0); + + Self::run_with_options(incoming_org_id, options) + } + + /// Starts a transaction with custom client options, preserving generated incoming IDs. + /// + /// `sentry::init` lives in the outer crate, so core tests bind a configured hub directly + /// and then use the public `start_transaction` entry point. + fn run_with_options(incoming_org_id: Option<&str>, mut options: ClientOptions) -> Self { + options + .dsn + .get_or_insert_with(|| "https://public@sentry.invalid/1".parse().unwrap()); + options + .transport + .get_or_insert_with(|| Arc::new(TestTransport::new())); + + // Generate a random sampled sentry trace header + let incoming_trace_id = TraceId::default(); + let incoming_parent_span_id = SpanId::default(); + let sentry_trace = format!("{incoming_trace_id}-{incoming_parent_span_id}-1"); + + // Construct the transaction context from headers. + let baggage = incoming_org_id.map(|org_id| format!("sentry-org_id={org_id}")); + let headers = std::iter::once(("sentry-trace", sentry_trace.as_str())) + .chain(baggage.as_deref().map(|baggage| ("baggage", baggage))); + let ctx = sentry_core::TransactionContext::continue_from_headers("noop", "noop", headers); + + // Create the hub with options, then start the transaction. + let hub = Arc::new(Hub::new( + Some(Arc::new(Client::with_options(options))), + Arc::new(Default::default()), + )); + let transaction = Hub::run(hub, || sentry_core::start_transaction(ctx)); + + Self { + incoming_trace_id, + incoming_parent_span_id, + transaction, + } + } + + /// Asserts that the transaction continued the incoming trace and inherited parent sampling. + fn assert_continued(&self) { + let context = self.transaction.get_trace_context(); + assert_eq!(context.trace_id, self.incoming_trace_id); + assert_eq!(context.parent_span_id, Some(self.incoming_parent_span_id)); + assert!(self.transaction.is_sampled()); + } + + /// Asserts that the transaction rejected the incoming trace and parent sampling. + fn assert_rejected(&self) { + let context = self.transaction.get_trace_context(); + assert_ne!(context.trace_id, self.incoming_trace_id); + assert_eq!(context.parent_span_id, None); + assert!(!self.transaction.is_sampled()); + } +} + +#[test] +fn start_transaction_continues_when_no_org_ids_and_not_strict() { + TraceContinuationScenario::run(None, None, false).assert_continued(); +} + +#[test] +fn start_transaction_continues_when_no_org_ids_and_strict() { + TraceContinuationScenario::run(None, None, true).assert_continued(); +} + +#[test] +fn start_transaction_continues_when_only_incoming_org_id_and_not_strict() { + TraceContinuationScenario::run(Some("42"), None, false).assert_continued(); +} + +#[test] +fn start_transaction_rejects_when_only_incoming_org_id_and_strict() { + TraceContinuationScenario::run(Some("42"), None, true).assert_rejected(); +} + +#[test] +fn start_transaction_continues_when_only_sdk_org_id_and_not_strict() { + TraceContinuationScenario::run(None, Some("42"), false).assert_continued(); +} + +#[test] +fn start_transaction_rejects_when_only_sdk_org_id_and_strict() { + TraceContinuationScenario::run(None, Some("42"), true).assert_rejected(); +} + +#[test] +fn start_transaction_continues_when_org_ids_match_and_not_strict() { + TraceContinuationScenario::run(Some("42"), Some("42"), false).assert_continued(); +} + +#[test] +fn start_transaction_continues_when_org_ids_match_and_strict() { + TraceContinuationScenario::run(Some("42"), Some("42"), true).assert_continued(); +} + +#[test] +fn start_transaction_rejects_when_org_ids_mismatch_and_not_strict() { + TraceContinuationScenario::run(Some("43"), Some("42"), false).assert_rejected(); +} + +#[test] +fn start_transaction_rejects_when_org_ids_mismatch_and_strict() { + TraceContinuationScenario::run(Some("43"), Some("42"), true).assert_rejected(); +} + +#[test] +fn start_transaction_prefers_explicit_org_id_over_dsn_org_id() { + TraceContinuationScenario::run_with_options( + Some("42"), + ClientOptions::new() + .dsn("https://public@o43.ingest.sentry.io/1") + .org_id("42".parse().unwrap()) + .strict_trace_continuation(true) + .traces_sample_rate(0.0), + ) + .assert_continued(); +} diff --git a/sentry-tower/src/http.rs b/sentry-tower/src/http.rs index 5b7c2f4eb..adc86225f 100644 --- a/sentry-tower/src/http.rs +++ b/sentry-tower/src/http.rs @@ -260,3 +260,85 @@ fn get_url_from_request(request: &Request) -> Option { let uri = uri::Uri::from_parts(uri_parts).ok()?; uri.to_string().parse().ok() } + +#[cfg(test)] +mod tests { + use sentry_core::protocol::{Context, EnvelopeItem, TraceContext}; + use sentry_core::{ClientOptions, Envelope}; + + use super::*; + + fn trace_context_from_single_transaction(envelopes: &[Envelope]) -> TraceContext { + let [envelope] = envelopes else { + panic!("Expected exactly one envelope"); + }; + + let mut items = envelope.items(); + let Some(EnvelopeItem::Transaction(transaction)) = items.next() else { + panic!("Expected a transaction envelope item"); + }; + assert!(items.next().is_none(), "expected only one envelope item"); + + match transaction.contexts.get("trace") { + Some(Context::Trace(trace)) => *trace.clone(), + unexpected => panic!("expected trace context, got {unexpected:#?}"), + } + } + + fn run_request_with_org_ids(incoming_org_id: &str, client_org_id: &str) -> Vec { + sentry::test::with_captured_envelopes_options( + || { + tokio::runtime::Runtime::new().unwrap().block_on(async { + let mut service = + SentryHttpLayer::new() + .enable_transaction() + .layer(tower::service_fn(|_request| async { + Ok::<_, std::convert::Infallible>(Response::new(())) + })); + let request = Request::builder() + .uri("http://example.com/test") + .header( + "sentry-trace", + "09e04486820349518ac7b5d2adbf6ba5-9cf635fa5b870b3a-1", + ) + .header("baggage", format!("sentry-org_id={incoming_org_id}")) + .body(()) + .unwrap(); + + service.call(request).await.unwrap(); + }); + }, + ClientOptions::new() + .org_id(client_org_id.parse().unwrap()) + .strict_trace_continuation(true) + .traces_sample_rate(1.0), + ) + } + + #[test] + fn transaction_continues_matching_org_id() { + let envelopes = run_request_with_org_ids("42", "42"); + let trace = trace_context_from_single_transaction(&envelopes); + + assert_eq!( + trace.trace_id.to_string(), + "09e04486820349518ac7b5d2adbf6ba5" + ); + assert_eq!( + trace.parent_span_id.map(|span_id| span_id.to_string()), + Some("9cf635fa5b870b3a".to_owned()) + ); + } + + #[test] + fn transaction_rejects_mismatched_org_id() { + let envelopes = run_request_with_org_ids("43", "42"); + let trace = trace_context_from_single_transaction(&envelopes); + + assert_ne!( + trace.trace_id.to_string(), + "09e04486820349518ac7b5d2adbf6ba5" + ); + assert_eq!(trace.parent_span_id, None); + } +}