Fix CEL policy YAML parser to use codepoint positions instead of byte positions.

PiperOrigin-RevId: 955506466

Author: maskri17

policy/BUILD

@@ -84,6 +84,7 @@ cc_library(
         ":cel_policy_parser",
         "//common:source",
         "//internal:status_macros",
+        "//internal:utf8",
         "//policy/internal:yaml_string_element_scanner",
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/status:statusor",

policy/cel_policy_parse_context.cc

@@ -14,12 +14,13 @@
 
 #include "policy/cel_policy_parse_context.h"
 
+#include <cstddef>
 #include <memory>
-#include <string>
 #include <string_view>
 #include <utility>
 
 #include "absl/log/absl_check.h"
+#include "common/source.h"
 #include "policy/cel_policy.h"
 #include "policy/cel_policy_parse_result.h"
 
@@ -43,7 +44,21 @@ CelPolicyParseResult CelPolicyParseContext::GetResult() {
 
 void CelPolicyParseContext::ReportError(CelPolicyElementId element_id,
                                         std::string_view message) {
-  issues_.push_back(CelPolicyIssue(element_id, std::string(message)));
+  issues_.push_back(CelPolicyIssue(element_id, message));
+}
+
+SourcePosition CelPolicyParseContext::GetCodepointPosition(
+    SourcePosition byte_offset) const {
+  if (byte_offset < 0) {
+    return -1;
+  }
+  if (byte_to_codepoint_mapping_.empty()) {
+    return byte_offset;
+  }
+  if (static_cast<size_t>(byte_offset) >= byte_to_codepoint_mapping_.size()) {
+    return byte_to_codepoint_mapping_.back();
+  }
+  return byte_to_codepoint_mapping_[byte_offset];
 }
 
 }  // namespace cel

policy/cel_policy_parse_context.h

@@ -20,6 +20,7 @@
 #include <vector>
 
 #include "absl/strings/string_view.h"
+#include "common/source.h"
 #include "policy/cel_policy.h"
 #include "policy/cel_policy_parse_result.h"
 
@@ -53,11 +54,18 @@ class CelPolicyParseContext {
 
   CelPolicyElementId next_element_id() { return next_element_id_++; }
 
+  void set_byte_to_codepoint_mapping(std::vector<SourcePosition> mapping) {
+    byte_to_codepoint_mapping_ = std::move(mapping);
+  }
+
+  SourcePosition GetCodepointPosition(SourcePosition byte_offset) const;
+
  private:
   std::shared_ptr<CelPolicySource> policy_source_;
   CelPolicyElementId next_element_id_ = 0;
   std::vector<CelPolicyIssue> issues_;
   std::unique_ptr<CelPolicy> policy_;
+  std::vector<SourcePosition> byte_to_codepoint_mapping_;
 };
 
 }  // namespace cel

policy/yaml_policy_parser.cc

@@ -14,35 +14,49 @@
 
 #include "policy/yaml_policy_parser.h"
 
+#include <cstddef>
 #include <memory>
 #include <optional>
 #include <string>
 #include <utility>
+#include <vector>
 
 #include "absl/status/status.h"
 #include "absl/status/statusor.h"
 #include "absl/strings/str_cat.h"
 #include "absl/strings/string_view.h"
 #include "common/source.h"
 #include "internal/status_macros.h"
+#include "internal/utf8.h"
 #include "policy/cel_policy.h"
 #include "policy/cel_policy_parse_context.h"
 #include "policy/cel_policy_parse_result.h"
 #include "policy/cel_policy_parser.h"
 #include "policy/internal/yaml_string_element_scanner.h"
 #include "yaml-cpp/exceptions.h"
+#include "yaml-cpp/mark.h"
 #include "yaml-cpp/node/node.h"
 #include "yaml-cpp/node/parse.h"
 #include "yaml-cpp/null.h"
 #include "yaml-cpp/yaml.h"  // IWYU pragma: keep
 
 namespace cel {
+namespace {
+
+SourcePosition GetMarkCodepointPosition(const CelPolicyParseContext& ctx,
+                                        const YAML::Mark& mark) {
+  if (mark.is_null() || mark.pos < 0) return -1;
+  return ctx.GetCodepointPosition(mark.pos);
+}
+
+}  // namespace
 
 CelPolicyElementId YamlPolicyParser::CollectMetadata(
     CelPolicyParseContext& ctx, const YAML::Node& node) const {
   CelPolicyElementId element_id = ctx.next_element_id();
   if (!node.Mark().is_null()) {
-    ctx.policy_source().NoteSourcePosition(element_id, node.Mark().pos);
+    ctx.policy_source().NoteSourcePosition(
+        element_id, GetMarkCodepointPosition(ctx, node.Mark()));
   }
   return element_id;
 }
@@ -62,9 +76,10 @@ std::optional<ValueString> YamlPolicyParser::GetValueString(
   }
 
   if (!node.Mark().is_null() && ctx.policy_source().content() != nullptr) {
+    SourcePosition codepoint_pos = GetMarkCodepointPosition(ctx, node.Mark());
     policy_internal::YamlStringElement element =
         policy_internal::ScanYamlStringElement(
-            ctx.policy_source().content()->content(), node.Mark().pos,
+            ctx.policy_source().content()->content(), codepoint_pos,
             node.as<std::string>());
 
     ctx.policy_source().NoteSourcePosition(id, element.starting_position);
@@ -87,14 +102,40 @@ absl::Status YamlPolicyParser::ParsePolicy(CelPolicyParseContext& ctx) const {
     return absl::OkStatus();
   }
 
-  ctx.policy().set_description(ValueString(-1, source->description()));
+  // TODO(b/542282964): Fold this mapping into cel::Source decoding happens
+  // once.
   std::string text = source->content().ToString();
+  std::vector<SourcePosition> mapping;
+  mapping.resize(text.size() + 1, 0);
+  size_t byte_offset = 0;
+  SourcePosition codepoint = 0;
+  absl::string_view view = text;
+  while (!view.empty()) {
+    auto [code_point, code_units] = cel::internal::Utf8Decode(view);
+    if (code_units == 0) break;
+    for (size_t i = 0; i < code_units; ++i) {
+      if (byte_offset + i < mapping.size()) {
+        mapping[byte_offset + i] = codepoint;
+      }
+    }
+    byte_offset += code_units;
+    view.remove_prefix(code_units);
+    codepoint++;
+  }
+  for (size_t i = byte_offset; i < mapping.size(); ++i) {
+    mapping[i] = codepoint;
+  }
+
+  ctx.set_byte_to_codepoint_mapping(std::move(mapping));
+
+  ctx.policy().set_description(ValueString(-1, source->description()));
   YAML::Node node;
   try {
     node = YAML::Load(text);
   } catch (YAML::Exception& e) {
     if (!e.mark.is_null()) {
-      ctx.policy_source().NoteSourcePosition(0, e.mark.pos);
+      ctx.policy_source().NoteSourcePosition(
+          0, GetMarkCodepointPosition(ctx, e.mark));
     }
     ctx.ReportError(0, "Invalid CEL policy YAML syntax");
     return absl::OkStatus();

policy/yaml_policy_parser_test.cc

@@ -150,6 +150,12 @@ TEST_P(YamlPolicyParseErrorTest, YamlSyntaxError) {
 
 std::vector<ParseTestCase> GetParseTestCases() {
   return {
+      ParseTestCase{
+          .yaml = "name: \"unclosed",
+          .expected_error = "1:16: Invalid CEL policy YAML syntax\n"
+                            " | name: \"unclosed\n"
+                            " | ...............^",
+      },
       ParseTestCase{
           .yaml = R"yaml( ? [ John, Doe ]: age: 30 )yaml",
           .expected_error = "1:22: Invalid CEL policy YAML syntax\n"
@@ -198,6 +204,17 @@ std::vector<ParseTestCase> GetParseTestCases() {
                             " |                     - cel.expr.conformance\n"
                             " | ....................^",
       },
+      ParseTestCase{
+          .yaml = R"yaml(
+                  # Comment with multi-byte char: €
+                  imports:
+                  - name:
+                    - cel.expr.conformance
+                )yaml",
+          .expected_error = "5:21: Import name is not a string\n"
+                            " |                     - cel.expr.conformance\n"
+                            " | ....................^",
+      },
       ParseTestCase{
           .yaml = R"yaml(
                   rule: do something