diff --git a/src/commands/api.rs b/src/commands/api.rs index a90b423c..8a09dc85 100644 --- a/src/commands/api.rs +++ b/src/commands/api.rs @@ -110,6 +110,8 @@ pub async fn run( } else { format!("{}{}", cfg.api_base_url(), normalize_path(endpoint)) }; + let parsed_url = reqwest::Url::parse(&url) + .map_err(|e| anyhow::anyhow!("invalid API endpoint {endpoint:?}: {e}"))?; // Only relative paths and absolute URLs that point at the configured Datadog // host may carry Datadog credentials. An absolute URL to any other host is an @@ -118,17 +120,15 @@ pub async fn run( // and leak the long-lived API keys to an arbitrary host. let credentials_allowed = !is_absolute || targets_configured_host(&url, cfg); - // Path used for per-endpoint auth routing. For relative endpoints this is the - // normalized API path; for absolute URLs on the Datadog host we use the URL's - // path component so the OAuth-exclusion table (raw_client::requires_api_key_fallback) - // still applies. - let auth_path = if is_absolute { - reqwest::Url::parse(&url) - .map(|u| u.path().to_string()) - .unwrap_or_else(|_| normalize_path(endpoint)) + // Route authentication by URL path so query strings and fragments cannot + // bypass endpoint-specific requirements. + let auth_path = parsed_url.path().to_string(); + + if credentials_allowed && crate::raw_client::requires_api_key_only(&method_upper, &auth_path) { + cfg.validate_api_key_only()?; } else { - normalize_path(endpoint) - }; + cfg.validate_auth()?; + } // POST, PUT, PATCH carry a body; GET/HEAD/DELETE use query params. let is_body_method = matches!(method_upper.as_str(), "POST" | "PUT" | "PATCH"); @@ -268,6 +268,22 @@ mod tests { use super::*; + async fn run_events_post(cfg: &Config, endpoint: &str) -> Result<()> { + super::run( + cfg, + endpoint, + "POST", + &[], + &["title=test".to_string(), "text=test".to_string()], + &[], + None, + false, + true, + false, + ) + .await + } + #[test] fn test_normalize_path_v2_prefix() { assert_eq!(normalize_path("v2/monitors"), "/api/v2/monitors"); @@ -470,6 +486,92 @@ mod tests { cleanup_env(); } + #[tokio::test] + async fn test_api_events_post_accepts_api_key_without_app_key() { + let _lock = lock_env().await; + let mut server = mockito::Server::new_async().await; + let mut cfg = test_config(&server.url()); + cfg.app_key = None; + let _mock = server + .mock("POST", "/api/v1/events") + .match_header("DD-API-KEY", "test-api-key") + .match_header("DD-APPLICATION-KEY", mockito::Matcher::Missing) + .match_header("Authorization", mockito::Matcher::Missing) + .with_status(202) + .with_header("content-type", "application/json") + .with_body(r#"{"status":"ok"}"#) + .create_async() + .await; + + let result = run_events_post(&cfg, "v1/events").await; + + assert!( + result.is_ok(), + "API-key-only event post failed: {:?}", + result.err() + ); + cleanup_env(); + } + + #[tokio::test] + async fn test_api_events_post_auth_ignores_query_and_fragment() { + let _lock = lock_env().await; + let mut server = mockito::Server::new_async().await; + let mut cfg = test_config(&server.url()); + cfg.app_key = None; + let mock = server + .mock("POST", "/api/v1/events") + .match_query(mockito::Matcher::UrlEncoded("source".into(), "test".into())) + .match_header("DD-API-KEY", "test-api-key") + .match_header("DD-APPLICATION-KEY", mockito::Matcher::Missing) + .match_header("Authorization", mockito::Matcher::Missing) + .with_status(202) + .with_header("content-type", "application/json") + .with_body(r#"{"status":"ok"}"#) + .create_async() + .await; + + let result = run_events_post(&cfg, "/v1/events?source=test#ignored").await; + + assert!( + result.is_ok(), + "decorated event endpoint bypassed API-key-only auth: {:?}", + result.err() + ); + mock.assert_async().await; + cleanup_env(); + } + + #[tokio::test] + async fn test_api_events_post_rejects_bearer_only() { + let _lock = lock_env().await; + let server = mockito::Server::new_async().await; + let mut cfg = test_config(&server.url()); + cfg.api_key = None; + cfg.app_key = None; + cfg.access_token = Some("test-token".into()); + + let result = run_events_post(&cfg, "v1/events#ignored").await; + + let err = result.expect_err("bearer-only event post should fail"); + assert!(err.to_string().contains("DD_API_KEY")); + cleanup_env(); + } + + #[tokio::test] + async fn test_api_events_post_rejects_missing_api_key() { + let _lock = lock_env().await; + let server = mockito::Server::new_async().await; + let mut cfg = test_config(&server.url()); + cfg.api_key = None; + + let result = run_events_post(&cfg, "v1/events").await; + + let err = result.expect_err("event post without API key should fail"); + assert!(err.to_string().contains("DD_API_KEY")); + cleanup_env(); + } + #[tokio::test] async fn test_api_raw_error_response() { let _lock = lock_env().await; diff --git a/src/commands/events.rs b/src/commands/events.rs index 5670824a..4bdc4653 100644 --- a/src/commands/events.rs +++ b/src/commands/events.rs @@ -18,12 +18,20 @@ use crate::config::Config; use crate::formatter; use crate::util_ext; +const MAX_AGGREGATION_KEY_CHARS: usize = 100; +const MAX_EVENT_TEXT_CHARS: usize = 4000; +const MAX_EVENT_AGE_SECONDS: i64 = 18 * 60 * 60; + #[derive(Clone, Debug, clap::ValueEnum)] pub(crate) enum EventAlertTypeArg { Error, Warning, Info, Success, + #[value(name = "user_update")] + UserUpdate, + Recommendation, + Snapshot, } impl From for EventAlertType { @@ -33,6 +41,9 @@ impl From for EventAlertType { EventAlertTypeArg::Warning => Self::WARNING, EventAlertTypeArg::Info => Self::INFO, EventAlertTypeArg::Success => Self::SUCCESS, + EventAlertTypeArg::UserUpdate => Self::USER_UPDATE, + EventAlertTypeArg::Recommendation => Self::RECOMMENDATION, + EventAlertTypeArg::Snapshot => Self::SNAPSHOT, } } } @@ -69,9 +80,8 @@ pub(crate) struct PostOptions { pub async fn post(cfg: &Config, mut options: PostOptions) -> Result<()> { // The V1 events intake endpoint (POST /api/v1/events) authenticates with the - // API key and does not accept OAuth2 bearer tokens, so require API+APP keys up - // front and skip bearer auth on the request. - cfg.validate_api_and_app_keys()?; + // API key alone and does not accept OAuth2 bearer tokens. + cfg.validate_api_key_only()?; if options.title.trim().is_empty() { anyhow::bail!("event title is empty"); } @@ -81,6 +91,7 @@ pub async fn post(cfg: &Config, mut options: PostOptions) -> Result<()> { std::io::stdin().is_terminal(), std::io::stdin().lock(), )?; + validate_post_request(&options, &message, chrono::Utc::now().timestamp())?; let body = build_post_request(options, message); let resp = api .create_event(body) @@ -154,8 +165,7 @@ fn resolve_message( ); } let buf = util_ext::read_to_string(reader, "failed to read event message from stdin")?; - // Drop the trailing newline shells add to piped input so stdin and - // argument messages produce the same event text. + // Match argument input by dropping trailing whitespace added by shell pipelines. buf.trim_end().to_owned() } }; @@ -166,6 +176,25 @@ fn resolve_message( Ok(message) } +fn validate_post_request(options: &PostOptions, message: &str, now: i64) -> Result<()> { + if let Some(aggregation_key) = &options.aggregation_key { + if aggregation_key.chars().count() > MAX_AGGREGATION_KEY_CHARS { + anyhow::bail!( + "event aggregation key must be at most {MAX_AGGREGATION_KEY_CHARS} characters" + ); + } + } + if message.chars().count() > MAX_EVENT_TEXT_CHARS { + anyhow::bail!("event text must be at most {MAX_EVENT_TEXT_CHARS} characters"); + } + if let Some(date_happened) = options.date_happened { + if date_happened < now - MAX_EVENT_AGE_SECONDS { + anyhow::bail!("event date_happened cannot be more than 18 hours in the past"); + } + } + Ok(()) +} + fn build_post_request(options: PostOptions, message: String) -> EventCreateRequest { let mut body = EventCreateRequest::new(message, options.title).priority(Some(options.priority.into())); @@ -284,7 +313,7 @@ mod tests { super::PostOptions { title: "Test title".into(), message: Some("Test message".into()), - date_happened: Some(1_700_000_000), + date_happened: None, handle: Some("test-user".into()), priority: super::EventPriorityArg::Low, related_event_id: Some(42), @@ -297,17 +326,42 @@ mod tests { } } + fn auth_config( + api_key: Option<&str>, + app_key: Option<&str>, + access_token: Option<&str>, + ) -> Config { + Config { + api_key: api_key.map(String::from), + app_key: app_key.map(String::from), + access_token: access_token.map(String::from), + site: "datadoghq.com".into(), + site_explicit: false, + org: None, + output_format: OutputFormat::Json, + auto_approve: false, + agent_mode: false, + read_only: false, + jq: None, + } + } + #[tokio::test] - async fn test_events_post_sends_dogshell_fields_without_host() { + async fn test_events_post_sends_only_api_key_and_post_fields() { let _lock = lock_env().await; let mut server = mockito::Server::new_async().await; - let cfg = test_config(&server.url()); + std::env::set_var("PUP_MOCK_SERVER", server.url()); + let cfg = auth_config(Some("test-api-key"), None, None); + let date_happened = chrono::Utc::now().timestamp(); let _mock = server .mock("POST", "/api/v1/events") + .match_header("DD-API-KEY", "test-api-key") + .match_header("DD-APPLICATION-KEY", mockito::Matcher::Missing) + .match_header("Authorization", mockito::Matcher::Missing) .match_body(mockito::Matcher::Json(serde_json::json!({ "aggregation_key": "test-group", "alert_type": "warning", - "date_happened": 1_700_000_000, + "date_happened": date_happened, "device_name": "test-device", "handle": "test-user", "priority": "low", @@ -323,7 +377,9 @@ mod tests { .create_async() .await; - let result = super::post(&cfg, post_options()).await; + let mut options = post_options(); + options.date_happened = Some(date_happened); + let result = super::post(&cfg, options).await; assert!(result.is_ok(), "events post failed: {:?}", result.err()); cleanup_env(); } @@ -376,53 +432,37 @@ mod tests { } #[tokio::test] - async fn test_events_post_requires_api_keys() { + async fn test_events_post_rejects_bearer_only() { // The V1 events intake endpoint rejects OAuth2 bearer tokens, so posting - // must fail fast when only an access token is configured. This is caught - // before any request, so no mock server or env setup is needed. - let cfg = Config { - api_key: None, - app_key: None, - access_token: Some("token".into()), - site: "datadoghq.com".into(), - site_explicit: false, - org: None, - output_format: OutputFormat::Json, - auto_approve: false, - agent_mode: false, - read_only: false, - jq: None, - }; + // must fail fast when only an access token is configured. + let cfg = auth_config(None, None, Some("token")); let result = super::post(&cfg, post_options()).await; - assert!(result.is_err(), "events post should require API keys"); + let err = result.expect_err("events post should reject bearer-only auth"); + assert!(err.to_string().contains("DD_API_KEY")); } #[tokio::test] - async fn test_events_post_rejects_empty_title() { - // Title emptiness is checked before any request, so no server is needed. - let cfg = Config { - api_key: Some("test-api-key".into()), - app_key: Some("test-app-key".into()), - access_token: None, - site: "datadoghq.com".into(), - site_explicit: false, - org: None, - output_format: OutputFormat::Json, - auto_approve: false, - agent_mode: false, - read_only: false, - jq: None, - }; + async fn test_events_post_rejects_missing_api_key() { + let cfg = auth_config(None, Some("test-app-key"), None); + let result = super::post(&cfg, post_options()).await; + let err = result.expect_err("events post should require an API key"); + assert!(err.to_string().contains("DD_API_KEY")); + } + + #[tokio::test] + async fn test_events_post_rejects_empty_title() { + let cfg = auth_config(Some("test-api-key"), None, None); let mut options = post_options(); options.title = " ".into(); + let result = super::post(&cfg, options).await; assert!(result.is_err(), "empty title should be rejected"); } #[test] - fn test_events_post_accepts_dogshell_flag_names() { + fn test_events_post_accepts_legacy_flag_aliases() { let result = crate::Cli::try_parse_from([ "pup", "events", @@ -451,22 +491,75 @@ mod tests { "Test title", ]); - assert!(result.is_ok(), "dogshell flags should parse"); + assert!(result.is_ok(), "legacy flag aliases should parse"); + } + + #[test] + fn test_events_post_accepts_v1_field_flag_names() { + for (device_flag, source_type_flag) in [ + ("--device-name", "--source-type-name"), + ("--device_name", "--source_type_name"), + ] { + let result = crate::Cli::try_parse_from([ + "pup", + "events", + "post", + device_flag, + "test-device", + source_type_flag, + "test-source", + "Test title", + "Test message", + ]); + + assert!( + result.is_ok(), + "V1 field flags {device_flag} and {source_type_flag} should parse" + ); + } } #[test] - fn test_events_post_rejects_invalid_alert_type() { + fn test_events_post_accepts_openapi_alert_types() { + for alert_type in [ + "error", + "warning", + "info", + "success", + "user_update", + "recommendation", + "snapshot", + ] { + let result = crate::Cli::try_parse_from([ + "pup", + "events", + "post", + "--alert_type", + alert_type, + "Test title", + "Test message", + ]); + + assert!( + result.is_ok(), + "alert type {alert_type:?} should be accepted" + ); + } + } + + #[test] + fn test_events_post_rejects_non_openapi_alert_type() { let result = crate::Cli::try_parse_from([ "pup", "events", "post", "--alert_type", - "critical", + "custom_alert", "Test title", "Test message", ]); - assert!(result.is_err(), "invalid alert type should be rejected"); + assert!(result.is_err(), "non-OpenAPI alert type should be rejected"); } #[test] @@ -529,6 +622,64 @@ mod tests { assert_eq!(body.tags, Some(vec!["a".to_string(), "b".to_string()])); } + #[test] + fn test_events_post_serializes_openapi_alert_types() { + for (alert_type, expected) in [ + (super::EventAlertTypeArg::Error, "error"), + (super::EventAlertTypeArg::Warning, "warning"), + (super::EventAlertTypeArg::Info, "info"), + (super::EventAlertTypeArg::Success, "success"), + (super::EventAlertTypeArg::UserUpdate, "user_update"), + (super::EventAlertTypeArg::Recommendation, "recommendation"), + (super::EventAlertTypeArg::Snapshot, "snapshot"), + ] { + let mut options = post_options(); + options.alert_type = Some(alert_type); + let body = super::build_post_request(options, "msg".into()); + let value = serde_json::to_value(body).unwrap(); + assert_eq!(value["alert_type"], expected); + } + } + + #[test] + fn test_events_post_accepts_openapi_request_limits() { + let now = chrono::Utc::now().timestamp(); + let mut options = post_options(); + options.aggregation_key = Some("a".repeat(super::MAX_AGGREGATION_KEY_CHARS)); + options.date_happened = Some(now - super::MAX_EVENT_AGE_SECONDS); + let message = "m".repeat(super::MAX_EVENT_TEXT_CHARS); + + assert!(super::validate_post_request(&options, &message, now).is_ok()); + } + + #[test] + fn test_events_post_rejects_aggregation_key_over_openapi_limit() { + let mut options = post_options(); + options.aggregation_key = Some("a".repeat(super::MAX_AGGREGATION_KEY_CHARS + 1)); + + let err = super::validate_post_request(&options, "message", 0).unwrap_err(); + assert!(err.to_string().contains("aggregation key")); + } + + #[test] + fn test_events_post_rejects_text_over_openapi_limit() { + let options = post_options(); + let message = "m".repeat(super::MAX_EVENT_TEXT_CHARS + 1); + + let err = super::validate_post_request(&options, &message, 0).unwrap_err(); + assert!(err.to_string().contains("event text")); + } + + #[test] + fn test_events_post_rejects_date_older_than_openapi_limit() { + let now = chrono::Utc::now().timestamp(); + let mut options = post_options(); + options.date_happened = Some(now - super::MAX_EVENT_AGE_SECONDS - 1); + + let err = super::validate_post_request(&options, "message", now).unwrap_err(); + assert!(err.to_string().contains("18 hours")); + } + #[test] fn test_events_post_no_host_overrides_host() { assert_eq!( diff --git a/src/config.rs b/src/config.rs index eaeb11b4..0c9e766f 100644 --- a/src/config.rs +++ b/src/config.rs @@ -371,6 +371,17 @@ impl Config { Ok(()) } + /// Validate that DD_API_KEY is configured for API-key-only endpoints. + pub fn validate_api_key_only(&self) -> Result<()> { + if self.api_key.is_none() { + bail!( + "this command requires DD_API_KEY — \ + OAuth2 bearer tokens are not supported here" + ); + } + Ok(()) + } + pub fn has_api_keys(&self) -> bool { self.api_key.is_some() && self.app_key.is_some() } @@ -979,6 +990,24 @@ mod tests { assert!(cfg.validate_api_and_app_keys().is_err()); } + #[test] + fn test_validate_api_key_only_accepts_api_key_without_app_key() { + let cfg = make_cfg(Some("key"), None, None); + assert!(cfg.validate_api_key_only().is_ok()); + } + + #[test] + fn test_validate_api_key_only_rejects_bearer_only() { + let cfg = make_cfg(None, None, Some("token")); + assert!(cfg.validate_api_key_only().is_err()); + } + + #[test] + fn test_validate_api_key_only_rejects_missing_api_key() { + let cfg = make_cfg(None, Some("app"), None); + assert!(cfg.validate_api_key_only().is_err()); + } + #[test] fn test_validate_auth_api_keys() { let cfg = make_cfg(Some("key"), Some("app"), None); diff --git a/src/main.rs b/src/main.rs index 2b34bf72..f759335b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -4094,9 +4094,17 @@ enum EventActions { help = "Do not associate a host with the event; overrides --host" )] no_host: bool, - #[arg(long, help = "Device to associate with the event")] + #[arg( + long = "device-name", + visible_aliases = ["device_name", "device"], + help = "Device name to associate with the event" + )] device: Option, - #[arg(long = "type", help = "Event source type")] + #[arg( + long = "source-type-name", + visible_aliases = ["source_type_name", "type"], + help = "Event source type name" + )] event_type: Option, #[arg( long, @@ -12492,7 +12500,9 @@ async fn main_inner() -> anyhow::Result<()> { } // --- Events --- Commands::Events { action } => { - cfg.validate_auth()?; + if !matches!(&action, EventActions::Post { .. }) { + cfg.validate_auth()?; + } match action { EventActions::Post { title, @@ -15665,7 +15675,6 @@ async fn main_inner() -> anyhow::Result<()> { silent, verbose, } => { - cfg.validate_auth()?; commands::api::run( &cfg, &endpoint, diff --git a/src/raw_client.rs b/src/raw_client.rs index ce94cb10..eb78158d 100644 --- a/src/raw_client.rs +++ b/src/raw_client.rs @@ -87,6 +87,11 @@ pub fn requires_api_key_fallback(method: &str, path: &str) -> bool { find_endpoint_requirement(method, path).is_some() } +/// Returns true if the endpoint accepts an API key without an application key. +pub(crate) fn requires_api_key_only(method: &str, path: &str) -> bool { + method == "POST" && path == "/api/v1/events" +} + fn find_endpoint_requirement(method: &str, path: &str) -> Option<&'static EndpointRequirement> { OAUTH_EXCLUDED_ENDPOINTS.iter().find(|req| { if req.method != method { @@ -565,17 +570,29 @@ async fn raw_post_impl( /// Apply Datadog authentication headers to a request builder. /// -/// Chooses between OAuth bearer and API-key/App-key auth based on `cfg` and the -/// per-endpoint requirements in [`requires_api_key_fallback`]: endpoints that do -/// not accept OAuth (see `OAUTH_EXCLUDED_ENDPOINTS`) force API-key auth even when -/// a bearer token is present. Exposed so the generic `pup api` passthrough reuses -/// the same auth routing as the typed clients. +/// Chooses between OAuth bearer and API-key auth based on `cfg` and the +/// per-endpoint requirements in [`requires_api_key_fallback`]. OAuth-excluded +/// endpoints require both API and application keys except for the V1 event +/// intake endpoint, which requires only the API key. Exposed so the generic +/// `pup api` passthrough reuses the same auth routing as the typed clients. pub fn apply_auth( mut req: reqwest::RequestBuilder, cfg: &Config, method: &str, path: &str, ) -> anyhow::Result { + // Events post is also in the broader OAuth-excluded table, so handle its + // API-key-only requirement before the fallback branch that adds both keys. + if requires_api_key_only(method, path) { + if let Some(api_key) = &cfg.api_key { + return Ok(req.header("DD-API-KEY", api_key.as_str())); + } + + anyhow::bail!( + "{method} {path} requires DD_API_KEY; OAuth2 bearer tokens are not supported" + ); + } + if requires_api_key_fallback(method, path) { if let (Some(api_key), Some(app_key)) = (&cfg.api_key, &cfg.app_key) { req = req @@ -810,9 +827,13 @@ mod tests { #[test] fn test_fallback_for_events_post() { - // Posting an event (V1 intake) requires API keys; reading events does not. + // Posting an event (V1 intake) requires only the API key; reading events + // does not require API-key fallback. assert!(requires_api_key_fallback("POST", "/api/v1/events")); + assert!(requires_api_key_only("POST", "/api/v1/events")); assert!(!requires_api_key_fallback("GET", "/api/v1/events")); + assert!(!requires_api_key_only("GET", "/api/v1/events")); + assert!(!requires_api_key_only("POST", "/api/v1/events/12345")); } #[test] @@ -950,6 +971,53 @@ mod tests { ); } + #[tokio::test] + async fn test_raw_events_post_sends_only_api_key() { + let _lock = lock_env().await; + let mut server = mockito::Server::new_async().await; + let cfg = test_config(&server.url()); + let mock = server + .mock("POST", "/api/v1/events") + .match_header("DD-API-KEY", "test-api-key") + .match_header("DD-APPLICATION-KEY", mockito::Matcher::Missing) + .match_header("Authorization", mockito::Matcher::Missing) + .with_status(202) + .with_header("content-type", "application/json") + .with_body(r#"{"status":"ok"}"#) + .expect(1) + .create_async() + .await; + + let result = raw_request( + &cfg, + "POST", + "/api/v1/events", + &[], + Some(br#"{"title":"test","text":"test"}"#.to_vec()), + Some("application/json"), + "application/json", + &[], + ) + .await; + + assert!(result.is_ok(), "raw event post failed: {:?}", result.err()); + mock.assert_async().await; + cleanup_env(); + } + + #[test] + fn test_other_oauth_excluded_endpoints_still_require_both_keys() { + let mut cfg = test_cfg(); + cfg.app_key = None; + let req = reqwest::Client::new().post("https://api.datadoghq.com/api/v2/api_keys"); + + let err = match apply_auth(req, &cfg, "POST", "/api/v2/api_keys") { + Ok(_) => panic!("API key management should require both keys"), + Err(err) => err, + }; + assert!(err.to_string().contains("DD_API_KEY and DD_APP_KEY")); + } + #[test] fn test_requires_api_key_fallback_profiling() { // /profiling/api/v1/*