From 44bdf70ee5f39243bdf3737d97042e1e24bfe81d Mon Sep 17 00:00:00 2001
From: John Sirois <john.sirois@gmail.com>
Date: Sun, 30 Aug 2020 10:14:39 -0700
Subject: [PATCH] Snapshots of single files store normalized paths.

Previously we were not enforcing or normalizing relative paths. There
are two callers, `DownloadedFile::load_or_download` and the
`create_digest_to_digest` intrinsic. The former happens to always pass a
normalized relative path due to the parsing behavior of `Url` but the
latter did no validation or normalization taking a path string directly
via cpython. A normalized relative path is now enforced by demanding a
`RelativePath`.

[ci skip-build-wheels]
---
 src/rust/engine/fs/src/lib.rs       |  6 ++++++
 src/rust/engine/fs/store/src/lib.rs |  6 +++---
 src/rust/engine/src/intrinsics.rs   |  4 +++-
 src/rust/engine/src/nodes.rs        | 14 ++++++++------
 4 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/src/rust/engine/fs/src/lib.rs b/src/rust/engine/fs/src/lib.rs
index 13a833447fd..d90e1c719ba 100644
--- a/src/rust/engine/fs/src/lib.rs
+++ b/src/rust/engine/fs/src/lib.rs
@@ -139,6 +139,12 @@ impl AsRef<Path> for RelativePath {
   }
 }
 
+impl Into<PathBuf> for RelativePath {
+  fn into(self) -> PathBuf {
+    self.0
+  }
+}
+
 #[derive(Clone, Debug, Eq, Hash, PartialEq)]
 pub enum Stat {
   Link(Link),
diff --git a/src/rust/engine/fs/store/src/lib.rs b/src/rust/engine/fs/store/src/lib.rs
index e2357cfdc4b..343d214c988 100644
--- a/src/rust/engine/fs/store/src/lib.rs
+++ b/src/rust/engine/fs/store/src/lib.rs
@@ -303,7 +303,7 @@ impl Store {
   /// Store a digest under a given file path, returning a Snapshot
   pub async fn snapshot_of_one_file(
     &self,
-    name: PathBuf,
+    name: RelativePath,
     digest: hashing::Digest,
     is_executable: bool,
   ) -> Result<Snapshot, String> {
@@ -322,9 +322,9 @@ impl Store {
       self.clone(),
       Digester { digest },
       vec![fs::PathStat::File {
-        path: name.clone(),
+        path: name.clone().into(),
         stat: fs::File {
-          path: name,
+          path: name.into(),
           is_executable: is_executable,
         },
       }],
diff --git a/src/rust/engine/src/intrinsics.rs b/src/rust/engine/src/intrinsics.rs
index 112ca19c227..902996d8eee 100644
--- a/src/rust/engine/src/intrinsics.rs
+++ b/src/rust/engine/src/intrinsics.rs
@@ -6,6 +6,7 @@ use crate::nodes::{lift_digest, DownloadedFile, NodeResult, Snapshot};
 use crate::tasks::Intrinsic;
 use crate::types::Types;
 
+use fs::RelativePath;
 use futures::compat::Future01CompatExt;
 use futures::future::{self, BoxFuture, FutureExt, TryFutureExt};
 use indexmap::IndexMap;
@@ -311,12 +312,13 @@ fn create_digest_to_digest(
     .iter()
     .map(|file| {
       let filename = externs::project_str(&file, "path");
-      let path: PathBuf = filename.into();
       let bytes = bytes::Bytes::from(externs::project_bytes(&file, "content"));
       let is_executable = externs::project_bool(&file, "is_executable");
 
       let store = context.core.store();
       async move {
+        let path = RelativePath::new(PathBuf::from(filename))
+          .map_err(|e| format!("The file `path` must be relative: {:?}", e))?;
         let digest = store.store_file_bytes(bytes, true).await?;
         let snapshot = store
           .snapshot_of_one_file(path, digest, is_executable)
diff --git a/src/rust/engine/src/nodes.rs b/src/rust/engine/src/nodes.rs
index ee9f85f4c10..ba8b8ebcc88 100644
--- a/src/rust/engine/src/nodes.rs
+++ b/src/rust/engine/src/nodes.rs
@@ -629,15 +629,17 @@ impl DownloadedFile {
       .and_then(Iterator::last)
       .map(str::to_owned)
       .ok_or_else(|| format!("Error getting the file name from the parsed URL: {}", url))?;
-
+    let path = RelativePath::new(&file_name).map_err(|e| {
+      format!(
+        "The file name derived from {} was {} which is not relative: {:?}",
+        &url, &file_name, e
+      )
+    })?;
     let maybe_bytes = core.store().load_file_bytes_with(digest, |_| ()).await?;
     if maybe_bytes.is_none() {
-      DownloadedFile::download(core.clone(), url, file_name.clone(), digest).await?;
+      DownloadedFile::download(core.clone(), url, file_name, digest).await?;
     }
-    core
-      .store()
-      .snapshot_of_one_file(PathBuf::from(file_name), digest, true)
-      .await
+    core.store().snapshot_of_one_file(path, digest, true).await
   }
 
   async fn download(