Executor: Check validity of action outputs in compatible mode

This ensures that any entries that the standard remote execution
protocol accepts but are invalid in justbuild, i.e., upwards
symlinks, are rejected.

For this purpose, do not fail in the action response instances,
just perform the check there, as all required information is
available, and set a flag that the executor can check as needed.

2 files changed, 72 insertions, 48 deletions

diff --git a/src/buildtool/execution_api/remote/bazel/bazel_response.cpp b/src/buildtool/execution_api/remote/bazel/bazel_response.cpp
index 81392813..e11a3b7f 100644
--- a/src/buildtool/execution_api/remote/bazel/bazel_response.cpp
+++ b/src/buildtool/execution_api/remote/bazel/bazel_response.cpp
@@ -14,6 +14,7 @@
 
 #include "src/buildtool/execution_api/remote/bazel/bazel_response.hpp"
 
+#include <algorithm>
 #include <cstddef>
 #include <exception>
 #include <filesystem>
@@ -40,18 +41,22 @@
 
 namespace {
 
+/// \brief Return
 auto ProcessDirectoryMessage(HashFunction hash_function,
                              bazel_re::Directory const& dir) noexcept
-    -> expected<ArtifactBlob, std::string> {
-    // in compatible mode: check validity of all symlinks
-    for (auto const& link : dir.symlinks()) {
-        if (not PathIsNonUpwards(link.target())) {
-            return unexpected{
-                fmt::format("found invalid symlink at {}", link.name())};
-        }
-    }
-    return ArtifactBlob::FromMemory(
+    -> expected<std::pair<ArtifactBlob, /*has_upwards_symlinks*/ bool>,
+                std::string> {
+    // in compatible mode: track upwards symlinks
+    bool has_upwards_symlinks = std::any_of(
+        dir.symlinks().begin(), dir.symlinks().end(), [](auto const& link) {
+            return not PathIsNonUpwards(link.target());
+        });
+    auto blob = ArtifactBlob::FromMemory(
         hash_function, ObjectType::File, dir.SerializeAsString());
+    if (not blob) {
+        return unexpected{std::move(blob).error()};
+    }
+    return std::make_pair(std::move(blob).value(), has_upwards_symlinks);
 }
 
 }  // namespace
@@ -92,6 +97,14 @@ auto BazelResponse::DirectorySymlinks() noexcept
         &dir_symlinks_);  // explicit type needed for expected
 }
 
+auto BazelResponse::HasUpwardsSymlinks() noexcept
+    -> expected<bool, std::string> {
+    if (auto error_msg = Populate()) {
+        return unexpected{*std::move(error_msg)};
+    }
+    return has_upwards_symlinks_;
+}
+
 auto BazelResponse::Populate() noexcept -> std::optional<std::string> {
     // Initialized only once lazily
     if (populated_) {
@@ -140,13 +153,10 @@ auto BazelResponse::Populate() noexcept -> std::optional<std::string> {
     // collect all symlinks and store them
     for (auto const& link : action_result.output_file_symlinks()) {
         try {
-            // in compatible mode: check symlink validity
-            if (not ProtocolTraits::IsNative(
-                    network_->GetHashFunction().GetType()) and
-                not PathIsNonUpwards(link.target())) {
-                return fmt::format("BazelResponse: found invalid symlink at {}",
-                                   link.path());
-            }
+            // in compatible mode: track upwards symlinks
+            has_upwards_symlinks_ = has_upwards_symlinks_ or
+                                    (not ProtocolTraits::IsNative(hash_type) and
+                                     not PathIsNonUpwards(link.target()));
             artifacts.emplace(
                 link.path(),
                 Artifact::ObjectInfo{
@@ -164,13 +174,10 @@ auto BazelResponse::Populate() noexcept -> std::optional<std::string> {
     }
     for (auto const& link : action_result.output_directory_symlinks()) {
         try {
-            // in compatible mode: check symlink validity
-            if (not ProtocolTraits::IsNative(
-                    network_->GetHashFunction().GetType()) and
-                not PathIsNonUpwards(link.target())) {
-                return fmt::format("BazelResponse: found invalid symlink at {}",
-                                   link.path());
-            }
+            // in compatible mode: track upwards symlinks
+            has_upwards_symlinks_ = has_upwards_symlinks_ or
+                                    (not ProtocolTraits::IsNative(hash_type) and
+                                     not PathIsNonUpwards(link.target()));
             artifacts.emplace(
                 link.path(),
                 Artifact::ObjectInfo{
@@ -223,8 +230,8 @@ auto BazelResponse::Populate() noexcept -> std::optional<std::string> {
     tree_digests.reserve(
         gsl::narrow<std::size_t>(action_result.output_directories_size()));
     for (auto const& directory : action_result.output_directories()) {
-        auto digest = ArtifactDigestFactory::FromBazel(
-            network_->GetHashFunction().GetType(), directory.tree_digest());
+        auto digest = ArtifactDigestFactory::FromBazel(hash_type,
+                                                       directory.tree_digest());
         if (not digest) {
             return std::move(digest).error();
         }
@@ -251,16 +258,20 @@ auto BazelResponse::Populate() noexcept -> std::optional<std::string> {
             // has sent us as part of the Tree message. If we want to be
             // able to use the Directories as inputs for actions, we
             // have to upload them manually.
-            auto root_digest = UploadTreeMessageDirectories(*tree);
-            if (not root_digest) {
-                auto error =
-                    fmt::format("BazelResponse: {}", root_digest.error());
+            auto upload_result = UploadTreeMessageDirectories(*tree);
+            if (not upload_result) {
+                auto error = fmt::format("BazelResponse: {}",
+                                         std::move(upload_result).error());
                 Logger::Log(LogLevel::Trace, error);
                 return error;
             }
-            artifacts.emplace(action_result.output_directories(pos).path(),
-                              Artifact::ObjectInfo{.digest = *root_digest,
-                                                   .type = ObjectType::Tree});
+            has_upwards_symlinks_ =
+                has_upwards_symlinks_ or upload_result.value().second;
+            artifacts.emplace(
+                action_result.output_directories(pos).path(),
+                Artifact::ObjectInfo{
+                    .digest = std::move(upload_result).value().first,
+                    .type = ObjectType::Tree});
         } catch (std::exception const& ex) {
             return fmt::format(
                 "BazelResponse: unexpected failure gathering digest for "
@@ -276,8 +287,9 @@ auto BazelResponse::Populate() noexcept -> std::optional<std::string> {
     return std::nullopt;
 }
 
-auto BazelResponse::UploadTreeMessageDirectories(
-    bazel_re::Tree const& tree) const -> expected<ArtifactDigest, std::string> {
+auto BazelResponse::UploadTreeMessageDirectories(bazel_re::Tree const& tree)
+    const -> expected<std::pair<ArtifactDigest, /*has_upwards_symlinks*/ bool>,
+                      std::string> {
     auto const upload_callback =
         [&network =
              *network_](std::unordered_set<ArtifactBlob>&& blobs) -> bool {
@@ -285,22 +297,26 @@ auto BazelResponse::UploadTreeMessageDirectories(
     };
     auto const hash_function = network_->GetHashFunction();
 
-    auto rootdir_blob = ProcessDirectoryMessage(hash_function, tree.root());
-    if (not rootdir_blob) {
-        return unexpected{std::move(rootdir_blob).error()};
+    auto rootdir_result = ProcessDirectoryMessage(hash_function, tree.root());
+    if (not rootdir_result) {
+        return unexpected{std::move(rootdir_result).error()};
     }
-    auto const root_digest = rootdir_blob->GetDigest();
-    std::unordered_set<ArtifactBlob> dir_blobs{*std::move(rootdir_blob)};
+    auto rootdir_has_upwards_symlinks = rootdir_result.value().second;
+    auto const root_digest = rootdir_result.value().first.GetDigest();
+    std::unordered_set<ArtifactBlob> dir_blobs{
+        std::move(rootdir_result).value().first};
 
     for (auto const& subdir : tree.children()) {
         // store or upload blob, taking maximum transfer size into account
-        auto blob = ProcessDirectoryMessage(hash_function, subdir);
-        if (not blob) {
-            return unexpected{std::move(blob).error()};
+        auto subdir_result = ProcessDirectoryMessage(hash_function, subdir);
+        if (not subdir_result) {
+            return unexpected{std::move(subdir_result).error()};
         }
-        auto const blob_digest = blob->GetDigest();
+        rootdir_has_upwards_symlinks =
+            rootdir_has_upwards_symlinks or subdir_result.value().second;
+        auto const blob_digest = subdir_result.value().first.GetDigest();
         if (not UpdateContainerAndUpload(&dir_blobs,
-                                         *std::move(blob),
+                                         std::move(subdir_result).value().first,
                                          /*exception_is_fatal=*/false,
                                          upload_callback)) {
             return unexpected{
@@ -315,5 +331,5 @@ auto BazelResponse::UploadTreeMessageDirectories(
             fmt::format("failed to upload blobs for Tree with root digest {}",
                         root_digest.hash())};
     }
-    return root_digest;
+    return std::make_pair(root_digest, rootdir_has_upwards_symlinks);
 }
diff --git a/src/buildtool/execution_api/remote/bazel/bazel_response.hpp b/src/buildtool/execution_api/remote/bazel/bazel_response.hpp
index df652141..55f45236 100644
--- a/src/buildtool/execution_api/remote/bazel/bazel_response.hpp
+++ b/src/buildtool/execution_api/remote/bazel/bazel_response.hpp
@@ -19,7 +19,7 @@
 #include <memory>
 #include <optional>
 #include <string>
-#include <utility>  // std::move
+#include <utility>  // std::move, std:pair
 
 #include <grpcpp/support/status.h>
 
@@ -91,6 +91,7 @@ class BazelResponse final : public IExecutionResponse {
         -> expected<gsl::not_null<ArtifactInfos const*>, std::string> final;
     auto DirectorySymlinks() noexcept
         -> expected<gsl::not_null<DirSymlinks const*>, std::string> final;
+    auto HasUpwardsSymlinks() noexcept -> expected<bool, std::string> final;
 
   private:
     std::string action_id_;
@@ -98,6 +99,7 @@ class BazelResponse final : public IExecutionResponse {
     BazelExecutionClient::ExecutionOutput output_{};
     ArtifactInfos artifacts_;
     DirSymlinks dir_symlinks_;
+    bool has_upwards_symlinks_ = false;  // only tracked in compatible mode
     bool populated_ = false;
 
     explicit BazelResponse(std::string action_id,
@@ -119,8 +121,14 @@ class BazelResponse final : public IExecutionResponse {
     /// \returns Error message on failure, nullopt on success.
     [[nodiscard]] auto Populate() noexcept -> std::optional<std::string>;
 
-    [[nodiscard]] auto UploadTreeMessageDirectories(bazel_re::Tree const& tree)
-        const -> expected<ArtifactDigest, std::string>;
+    /// \brief Tries to upload the tree rot and subdirectories. Performs also a
+    /// symlinks check.
+    /// \returns Pair of ArtifactDigest of root tree and flag signaling the
+    /// presence of any upwards symlinks on success, error message on failure.
+    [[nodiscard]] auto UploadTreeMessageDirectories(
+        bazel_re::Tree const& tree) const
+        -> expected<std::pair<ArtifactDigest, /*has_upwards_symlinks*/ bool>,
+                    std::string>;
 };
 
 #endif  // INCLUDED_SRC_BUILDTOOL_EXECUTION_API_REMOTE_BAZEL_BAZEL_RESPONSE_HPP