Encode the file path in url authority so we don't crash on whitespaces (

#499)

* Encode the file path in url authority so we don't crash on whitespaces

* Add test verifying that this no longer returns a null pointer in case of valid path

bin_tests/tests/crashtracker_bin_test.rs

@@ -226,12 +226,13 @@ fn setup_test_fixtures<'a>(crates: &[&'a ArtifactsBuild]) -> TestFixtures<'a> {
     let artifacts = build_artifacts(crates).unwrap();
 
     let tmpdir = tempfile::TempDir::new().unwrap();
+    let dirpath = tmpdir.path();
     TestFixtures {
-        crash_profile_path: extend_path(tmpdir.path(), "crash"),
-        crash_telemetry_path: extend_path(tmpdir.path(), "crash.telemetry"),
-        stdout_path: extend_path(tmpdir.path(), "out.stdout"),
-        stderr_path: extend_path(tmpdir.path(), "out.stderr"),
-        unix_socket_path: extend_path(tmpdir.path(), "crashtracker.socket"),
+        crash_profile_path: extend_path(dirpath, "crash"),
+        crash_telemetry_path: extend_path(dirpath, "crash.telemetry"),
+        stdout_path: extend_path(dirpath, "out.stdout"),
+        stderr_path: extend_path(dirpath, "out.stderr"),
+        unix_socket_path: extend_path(dirpath, "crashtracker.socket"),
 
         artifacts,
         tmpdir,

crashtracker/src/crash_info.rs

@@ -9,6 +9,7 @@ use chrono::{DateTime, Utc};
 use ddcommon::tag::Tag;
 use serde::{Deserialize, Serialize};
 use std::io::BufRead;
+use std::path::Path;
 use std::{collections::HashMap, fs::File, io::BufReader};
 use uuid::Uuid;
 
@@ -241,24 +242,21 @@ impl CrashInfo {
     /// Emit the CrashInfo as structured json in file `path`.
     /// SIGNAL SAFETY:
     ///     I believe but have not verified this is signal safe.
-    pub fn to_file(&self, path: &str) -> anyhow::Result<()> {
-        let file = File::create(path).with_context(|| format!("Failed to create {path}"))?;
+    pub fn to_file(&self, path: &Path) -> anyhow::Result<()> {
+        let file =
+            File::create(path).with_context(|| format!("Failed to create {}", path.display()))?;
         serde_json::to_writer_pretty(file, self)
-            .with_context(|| format!("Failed to write json to {path}"))?;
+            .with_context(|| format!("Failed to write json to {}", path.display()))?;
         Ok(())
     }
 
     pub fn upload_to_endpoint(&self, config: &CrashtrackerConfiguration) -> anyhow::Result<()> {
         // If we're debugging to a file, dump the actual crashinfo into a json
         if let Some(endpoint) = &config.endpoint {
             if Some("file") == endpoint.url.scheme_str() {
-                self.to_file(
-                    endpoint
-                        .url
-                        .path_and_query()
-                        .ok_or_else(|| anyhow::format_err!("empty path for upload to file"))?
-                        .as_str(),
-                )?;
+                let path = ddcommon::decode_uri_path_in_authority(&endpoint.url)
+                    .context("crash output file was not correctly formatted")?;
+                self.to_file(&path)?;
             }
         }
         self.upload_to_telemetry(config)

crashtracker/src/telemetry.rs

@@ -6,7 +6,7 @@ use std::fmt::Write;
 use std::time::{self, SystemTime};
 
 use super::{CrashInfo, CrashtrackerConfiguration, CrashtrackerMetadata, StackFrame};
-use anyhow::Ok;
+use anyhow::{Context, Ok};
 use ddtelemetry::{
     build_host,
     data::{self, Application, LogLevel},
@@ -70,7 +70,9 @@ impl TelemetryCrashUploader {
 
             // ignore result because what are we going to do?
             let _ = if endpoint.url.scheme_str() == Some("file") {
-                cfg.set_host_from_url(&format!("file://{}.telemetry", endpoint.url.path()))
+                let path = ddcommon::decode_uri_path_in_authority(&endpoint.url)
+                    .context("file path is not valid")?;
+                cfg.set_host_from_url(&format!("file://{}.telemetry", path.display()))
             } else {
                 cfg.set_endpoint(endpoint.clone())
             };

ddcommon-ffi/src/endpoint.rs

@@ -51,3 +51,28 @@ pub extern "C" fn ddog_endpoint_from_api_key_and_site(
 
 #[no_mangle]
 pub extern "C" fn ddog_endpoint_drop(_: Box<Endpoint>) {}
+
+#[cfg(test)]
+mod tests {
+    use crate::CharSlice;
+
+    use super::ddog_endpoint_from_url;
+
+    #[test]
+    fn test_ddog_endpoint_from_url() {
+        let cases = [
+            ("", false),
+            ("http:// /hey", false),
+            ("file://", false),
+            ("http://localhost:8383/hello", true),
+            ("file:/// file / with/weird chars 🤡", true),
+            ("file://./", true),
+            ("unix://./", true),
+        ];
+
+        for (input, expected) in cases {
+            let actual = ddog_endpoint_from_url(CharSlice::from(input)).is_some();
+            assert_eq!(actual, expected);
+        }
+    }
+}

ddcommon/src/lib.rs

@@ -1,7 +1,7 @@
 // Copyright 2021-Present Datadog, Inc. https://www.datadoghq.com/
 // SPDX-License-Identifier: Apache-2.0
 
-use std::{borrow::Cow, ops::Deref, str::FromStr};
+use std::{borrow::Cow, ops::Deref, path::PathBuf, str::FromStr};
 
 use hyper::{
     header::HeaderValue,
@@ -95,17 +95,7 @@ pub fn parse_uri(uri: &str) -> anyhow::Result<hyper::Uri> {
     } else if let Some(path) = uri.strip_prefix("windows:") {
         encode_uri_path_in_authority("windows", path)
     } else if let Some(path) = uri.strip_prefix("file://") {
-        let mut parts = uri::Parts::default();
-        parts.scheme = uri::Scheme::from_str("file").ok();
-        parts.authority = Some(uri::Authority::from_static("localhost"));
-
-        // TODO: handle edge cases like improperly escaped url strings
-        //
-        // this is eventually user configurable field
-        // anything we can do to ensure invalid input becomes valid - will improve usability
-        parts.path_and_query = uri::PathAndQuery::from_str(path).ok();
-
-        Ok(hyper::Uri::from_parts(parts)?)
+        encode_uri_path_in_authority("file", path)
     } else {
         Ok(hyper::Uri::from_str(uri)?)
     }
@@ -122,6 +112,26 @@ fn encode_uri_path_in_authority(scheme: &str, path: &str) -> anyhow::Result<hype
     Ok(hyper::Uri::from_parts(parts)?)
 }
 
+pub fn decode_uri_path_in_authority(uri: &hyper::Uri) -> anyhow::Result<PathBuf> {
+    let path = hex::decode(
+        uri.authority()
+            .ok_or_else(|| anyhow::anyhow!("missing uri authority"))?
+            .as_str(),
+    )?;
+    #[cfg(unix)]
+    {
+        use std::os::unix::ffi::OsStringExt;
+        Ok(PathBuf::from(std::ffi::OsString::from_vec(path)))
+    }
+    #[cfg(not(unix))]
+    {
+        match String::from_utf8(path) {
+            Ok(s) => Ok(PathBuf::from(s.as_str())),
+            _ => Err(anyhow::anyhow!("file uri should be utf-8")),
+        }
+    }
+}
+
 impl Endpoint {
     /// Return a request builder with the following headers:
     /// - User agent

ddtelemetry/src/config.rs

@@ -267,6 +267,8 @@ impl Config {
 
 #[cfg(test)]
 mod tests {
+    use std::path::Path;
+
     #[cfg(unix)]
     use ddcommon::connector::uds;
 
@@ -388,6 +390,10 @@ mod tests {
             ("file:///absolute/path", "/absolute/path"),
             ("file://./relative/path", "./relative/path"),
             ("file://relative/path", "relative/path"),
+            (
+                "file://c://temp//with space\\foo.json",
+                "c://temp//with space\\foo.json",
+            ),
         ];
 
         for (input, expected) in cases {
@@ -405,15 +411,8 @@ mod tests {
                     .to_string()
             );
             assert_eq!(
-                expected,
-                cfg.clone()
-                    .endpoint
-                    .unwrap()
-                    .url
-                    .into_parts()
-                    .path_and_query
-                    .unwrap()
-                    .as_str()
+                Path::new(expected),
+                ddcommon::decode_uri_path_in_authority(&cfg.endpoint.unwrap().url).unwrap(),
             );
         }
     }

ddtelemetry/src/worker/http_client.rs

@@ -2,7 +2,6 @@
 // SPDX-License-Identifier: Apache-2.0
 
 use ddcommon::HttpRequestBuilder;
-use http::uri::Parts;
 use http::{Request, Response};
 use hyper::Body;
 use std::{
@@ -41,21 +40,18 @@ pub fn request_builder(c: &Config) -> anyhow::Result<HttpRequestBuilder> {
 }
 
 pub fn from_config(c: &Config) -> Box<dyn HttpClient + Sync + Send> {
-    if let Some(Parts {
-        scheme: Some(scheme),
-        path_and_query: Some(path),
-        ..
-    }) = c.endpoint.as_ref().map(|e| e.url.clone().into_parts())
-    {
-        if scheme.as_str() == "file" {
+    match &c.endpoint {
+        Some(e) if e.url.scheme_str() == Some("file") => {
+            let file_path = ddcommon::decode_uri_path_in_authority(&e.url)
+                .expect("file urls should always have been encoded in authority");
             return Box::new(MockClient {
                 file: Arc::new(Mutex::new(Box::new(
-                    File::create(path.path()).expect("Couldn't open mock client file"),
+                    File::create(file_path).expect("Couldn't open mock client file"),
                 ))),
             });
         }
-    }
-
+        Some(_) | None => {}
+    };
     Box::new(HyperClient {
         inner: hyper::Client::builder()
             .pool_idle_timeout(std::time::Duration::from_secs(30))