diff --git a/README.md b/README.md index d4e2fc0..5dad97c 100644 --- a/README.md +++ b/README.md @@ -69,7 +69,7 @@ anwesen version | `--bind ` | `ANWESEN_BIND` | `127.0.0.1:8080` | Listen address for `serve`. | | `--log-level ` | `ANWESEN_LOG_LEVEL` | `info` | `error`, `warn`, `info`, `debug`, or `trace`. | | `--query ` | -- | _required for `merge`_ | A `/query` query string: frontmatter predicates plus `__anw-` controls. | -| `--uptrace-dsn ` | `ANWESEN_UPTRACE_DSN` | unset | uptrace DSN (`https://@api.uptrace.dev`). Excludes `--otlp-endpoint`. | +| `--uptrace-dsn ` | `ANWESEN_UPTRACE_DSN` | unset | uptrace DSN (`https://@host[:port]`). No path or query. Excludes `--otlp-endpoint`. | | `--otlp-endpoint ` | `ANWESEN_OTLP_ENDPOINT` | unset | OTLP/HTTP base URL. Excludes `--uptrace-dsn`. | | `--otlp-header ` | `ANWESEN_OTLP_HEADERS` | none | Extra export header, repeatable. The env var takes a comma-separated list. | | `--otlp-slow-request-ms` | `ANWESEN_OTLP_SLOW_REQUEST_MS` | `500` | Requests at or over this duration, or answering 5xx, also export a span. | diff --git a/src/telemetry.rs b/src/telemetry.rs index a685b2d..288b54d 100644 --- a/src/telemetry.rs +++ b/src/telemetry.rs @@ -131,6 +131,12 @@ fn parse_headers(specs: &[String]) -> anyhow::Result> { /// Parse an uptrace DSN (`https://@host[:port]`) into the OTLP /// endpoint base URL and the `uptrace-dsn` header uptrace expects. The full /// DSN is echoed as the header value per uptrace's ingest contract. +/// +/// The part after the credentials must be exactly `host[:port]`. A path, +/// query, or fragment tail is rejected: it would survive into the endpoint +/// base, and the per-signal path appended in [`TelemetryInner::new`] would +/// land inside it, giving a URL that no collector answers +/// ([ANW-42](https://crvrs.youtrack.cloud/issue/ANW-42)). fn parse_uptrace_dsn(dsn: &str) -> anyhow::Result<(String, (String, String))> { let dsn = dsn.trim(); let (scheme, rest) = dsn @@ -144,10 +150,57 @@ fn parse_uptrace_dsn(dsn: &str) -> anyhow::Result<(String, (String, String))> { if host.is_empty() { bail!("uptrace DSN has no host"); } + validate_dsn_authority(host)?; let endpoint = format!("{scheme}://{host}"); Ok((endpoint, ("uptrace-dsn".to_string(), dsn.to_string()))) } +/// Check that a DSN's post-credentials part is `host[:port]` and nothing +/// else. `host` is already non-empty and free of a trailing slash. +fn validate_dsn_authority(host: &str) -> anyhow::Result<()> { + if let Some(bad) = host.find(['/', '?', '#']) { + // Name the offending character: the operator pasted a URL where a + // DSN was wanted, and the tail is what has to go. + let tail = &host[bad..]; + bail!( + "uptrace DSN host {host:?} has a trailing {tail:?} \ + (expected https://@host[:port], with no path or query)" + ); + } + + // An IPv6 literal is bracketed, so only a colon past the closing bracket + // separates the port. + let (name, port) = if host.starts_with('[') { + let close = host + .find(']') + .with_context(|| format!("uptrace DSN host {host:?} has an unclosed IPv6 bracket"))?; + let (name, after) = host.split_at(close + 1); + if after.is_empty() { + (name, None) + } else { + let port = after + .strip_prefix(':') + .with_context(|| format!("uptrace DSN host {host:?} is not host[:port]"))?; + (name, Some(port)) + } + } else { + match host.rsplit_once(':') { + Some((name, port)) => (name, Some(port)), + None => (host, None), + } + }; + + if name.is_empty() || name == "[]" { + bail!("uptrace DSN has no host"); + } + if let Some(port) = port + && (port.is_empty() || !port.bytes().all(|b| b.is_ascii_digit())) + { + bail!("uptrace DSN host {host:?} has a non-numeric port (expected host[:port])"); + } + Ok(()) +} + /// Semantic route bucket for the `http.route` label. Coarser than the axum /// template on purpose: `/notes/{*path}` serves both a note fetch and a /// folder listing, and "304 share of note fetches" needs the two apart. The @@ -578,6 +631,76 @@ mod tests { assert!(err.to_string().contains("scheme")); } + /// Every shape that would otherwise leave a tail in the endpoint base, + /// so the `/v1/metrics` appended later lands inside it (ANW-42). + #[test] + fn resolve_rejects_dsn_with_a_tail() { + for dsn in [ + "https://tok@api.uptrace.dev?grpc=4317", + "https://tok@api.uptrace.dev/v1", + "https://tok@api.uptrace.dev#frag", + "https://tok@api.uptrace.dev:4318?grpc=4317", + "https://tok@api.uptrace.dev/?grpc=4317", + ] { + let err = TelemetryConfig::resolve(RawTelemetryArgs { + uptrace_dsn: Some(dsn.into()), + ..raw() + }) + .unwrap_err(); + assert!(err.to_string().contains("trailing"), "{dsn}: {err}"); + } + } + + #[test] + fn resolve_rejects_dsn_with_a_bad_port() { + for dsn in [ + "https://tok@api.uptrace.dev:grpc", + "https://tok@api.uptrace.dev:", + "https://tok@[::1]:grpc", + ] { + let err = TelemetryConfig::resolve(RawTelemetryArgs { + uptrace_dsn: Some(dsn.into()), + ..raw() + }) + .unwrap_err(); + assert!(err.to_string().contains("port"), "{dsn}: {err}"); + } + } + + #[test] + fn resolve_rejects_dsn_with_no_host_before_the_port() { + let err = TelemetryConfig::resolve(RawTelemetryArgs { + uptrace_dsn: Some("https://tok@:4318".into()), + ..raw() + }) + .unwrap_err(); + assert!(err.to_string().contains("no host"), "{err}"); + } + + /// The shapes a real DSN takes still resolve, port and IPv6 included. + #[test] + fn resolve_accepts_plain_host_forms() { + for (dsn, endpoint) in [ + ("https://tok@api.uptrace.dev", "https://api.uptrace.dev"), + ("https://tok@api.uptrace.dev/", "https://api.uptrace.dev"), + ( + "https://tok@collector.internal:4318", + "https://collector.internal:4318", + ), + ("http://tok@[::1]:4318", "http://[::1]:4318"), + ("http://tok@[::1]", "http://[::1]"), + ("https://api.uptrace.dev", "https://api.uptrace.dev"), + ] { + let cfg = TelemetryConfig::resolve(RawTelemetryArgs { + uptrace_dsn: Some(dsn.into()), + ..raw() + }) + .unwrap_or_else(|e| panic!("{dsn}: {e}")) + .expect("telemetry on"); + assert_eq!(cfg.endpoint, endpoint, "{dsn}"); + } + } + #[test] fn classify_route_buckets() { assert_eq!(classify_route("/health"), "health");