From 62b897b91a27c06c0960cc66da88a06b7f412504 Mon Sep 17 00:00:00 2001 From: Muhammad Askri Date: Tue, 28 Jul 2026 15:32:29 -0700 Subject: [PATCH] Fix CEL policy YAML parser to use codepoint positions instead of byte positions. PiperOrigin-RevId: 955506466 --- common/values/parsed_message_value.cc | 14 +--- conformance/BUILD | 1 - eval/public/structs/BUILD | 3 + eval/public/structs/cel_proto_wrap_util.cc | 47 ++++++++++- .../structs/cel_proto_wrap_util_test.cc | 3 + eval/public/structs/cel_proto_wrapper_test.cc | 4 +- policy/BUILD | 1 + policy/yaml_policy_parser.cc | 77 ++++++++++++++++++- policy/yaml_policy_parser_test.cc | 11 +++ 9 files changed, 142 insertions(+), 19 deletions(-) diff --git a/common/values/parsed_message_value.cc b/common/values/parsed_message_value.cc index 8a2b8030d..fdfc8d977 100644 --- a/common/values/parsed_message_value.cc +++ b/common/values/parsed_message_value.cc @@ -60,14 +60,6 @@ EmptyParsedMessageValue() { return &T::default_instance(); } -template -std::enable_if_t< - std::conjunction_v, - std::negation>>, - const google::protobuf::Message* absl_nonnull> -EmptyParsedMessageValue() { - return internal::GetEmptyDefaultInstance(); -} } // namespace @@ -114,12 +106,8 @@ absl::Status ParsedMessageValue::ConvertToJson( ABSL_DCHECK_EQ(json->GetDescriptor()->well_known_type(), google::protobuf::Descriptor::WELLKNOWNTYPE_VALUE); - ValueReflection value_reflection; - CEL_RETURN_IF_ERROR(value_reflection.Initialize(json->GetDescriptor())); - google::protobuf::Message* json_object = value_reflection.MutableStructValue(json); - return internal::MessageToJson(*value_, descriptor_pool, message_factory, - json_object); + json); } absl::Status ParsedMessageValue::ConvertToJsonObject( diff --git a/conformance/BUILD b/conformance/BUILD index eb129470f..3bde28cc6 100644 --- a/conformance/BUILD +++ b/conformance/BUILD @@ -181,7 +181,6 @@ _TESTS_TO_SKIP = [ "enums/legacy_proto2/select_big,select_neg", # Skip until fixed. - "wrappers/field_mask/to_json", "wrappers/empty/to_json", "fields/qualified_identifier_resolution/map_value_repeat_key_heterogeneous", "parse/receiver_function_names", diff --git a/eval/public/structs/BUILD b/eval/public/structs/BUILD index 4e4d5481c..771258248 100644 --- a/eval/public/structs/BUILD +++ b/eval/public/structs/BUILD @@ -81,6 +81,7 @@ cc_library( "@com_google_absl//absl/types:variant", "@com_google_protobuf//:any_cc_proto", "@com_google_protobuf//:duration_cc_proto", + "@com_google_protobuf//:json_util", "@com_google_protobuf//:protobuf", "@com_google_protobuf//:struct_cc_proto", "@com_google_protobuf//:timestamp_cc_proto", @@ -111,6 +112,7 @@ cc_test( "@com_google_absl//absl/status", "@com_google_absl//absl/strings", "@com_google_absl//absl/time", + "@com_google_absl//absl/types:span", "@com_google_protobuf//:any_cc_proto", "@com_google_protobuf//:duration_cc_proto", "@com_google_protobuf//:empty_cc_proto", @@ -218,6 +220,7 @@ cc_test( "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", "@com_google_absl//absl/time", + "@com_google_absl//absl/types:span", "@com_google_protobuf//:any_cc_proto", "@com_google_protobuf//:duration_cc_proto", "@com_google_protobuf//:empty_cc_proto", diff --git a/eval/public/structs/cel_proto_wrap_util.cc b/eval/public/structs/cel_proto_wrap_util.cc index 7bfe81fe6..efe9e9d56 100644 --- a/eval/public/structs/cel_proto_wrap_util.cc +++ b/eval/public/structs/cel_proto_wrap_util.cc @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -52,6 +53,7 @@ #include "google/protobuf/descriptor.h" #include "google/protobuf/message.h" #include "google/protobuf/message_lite.h" +#include "google/protobuf/util/json_util.h" namespace google::api::expr::runtime::internal { @@ -79,7 +81,10 @@ using google::protobuf::Descriptor; using google::protobuf::DescriptorPool; using google::protobuf::Message; using google::protobuf::MessageFactory; - +using google::protobuf::util::JsonParseOptions; +using google::protobuf::util::JsonPrintOptions; +using google::protobuf::util::JsonStringToMessage; +using google::protobuf::util::MessageToJsonString; // kMaxIntJSON is defined as the Number.MAX_SAFE_INTEGER value per EcmaScript 6. constexpr int64_t kMaxIntJSON = (1ll << 53) - 1; @@ -1079,6 +1084,26 @@ google::protobuf::Message* ValueFromValue(google::protobuf::Message* message, co return message; } } break; + case CelValue::Type::kMessage: { + const google::protobuf::Message* message_ptr = value.MessageOrDie(); + if (message_ptr->GetDescriptor()->full_name() == + "google.protobuf.FieldMask") { + JsonPrintOptions json_options; + std::string json_str; + auto status = + MessageToJsonString(*message_ptr, &json_str, json_options); + if (!status.ok()) { + return nullptr; + } + JsonParseOptions json_parse_options; + status = JsonStringToMessage(json_str, message, json_parse_options); + if (!status.ok()) { + return nullptr; + } + return message; + } + return nullptr; + } break; case CelValue::Type::kNullType: reflection.SetNullValue(message); return message; @@ -1229,6 +1254,26 @@ bool ValueFromValue(Value* json, const CelValue& value, google::protobuf::Arena* return ListFromValue(json->mutable_list_value(), value, arena); case CelValue::Type::kMap: return StructFromValue(json->mutable_struct_value(), value, arena); + case CelValue::Type::kMessage: { + const google::protobuf::Message* message_ptr = value.MessageOrDie(); + if (message_ptr->GetDescriptor()->full_name() == + "google.protobuf.FieldMask") { + JsonPrintOptions json_options; + std::string json_str; + auto status = + MessageToJsonString(*message_ptr, &json_str, json_options); + if (!status.ok()) { + return false; + } + JsonParseOptions json_parse_options; + status = JsonStringToMessage(json_str, json, json_parse_options); + if (!status.ok()) { + return false; + } + return true; + } + return false; + } case CelValue::Type::kNullType: json->set_null_value(protobuf::NULL_VALUE); return true; diff --git a/eval/public/structs/cel_proto_wrap_util_test.cc b/eval/public/structs/cel_proto_wrap_util_test.cc index 59597fe8f..2b49a05cc 100644 --- a/eval/public/structs/cel_proto_wrap_util_test.cc +++ b/eval/public/structs/cel_proto_wrap_util_test.cc @@ -15,6 +15,7 @@ #include "eval/public/structs/cel_proto_wrap_util.h" #include +#include #include #include #include @@ -30,6 +31,7 @@ #include "absl/status/status.h" #include "absl/strings/str_cat.h" #include "absl/time/time.h" +#include "absl/types/span.h" #include "eval/public/cel_value.h" #include "eval/public/containers/container_backed_list_impl.h" #include "eval/public/containers/container_backed_map_impl.h" @@ -41,6 +43,7 @@ #include "internal/status_macros.h" #include "internal/testing.h" #include "testutil/util.h" +#include "google/protobuf/arena.h" #include "google/protobuf/dynamic_message.h" #include "google/protobuf/message.h" diff --git a/eval/public/structs/cel_proto_wrapper_test.cc b/eval/public/structs/cel_proto_wrapper_test.cc index b9fcd6b51..3ec9c9ac7 100644 --- a/eval/public/structs/cel_proto_wrapper_test.cc +++ b/eval/public/structs/cel_proto_wrapper_test.cc @@ -1,6 +1,7 @@ #include "eval/public/structs/cel_proto_wrapper.h" #include +#include #include #include #include @@ -16,14 +17,15 @@ #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" #include "absl/time/time.h" +#include "absl/types/span.h" #include "eval/public/cel_value.h" #include "eval/public/containers/container_backed_list_impl.h" #include "eval/public/containers/container_backed_map_impl.h" #include "eval/testutil/test_message.pb.h" #include "internal/proto_time_encoding.h" -#include "internal/status_macros.h" #include "internal/testing.h" #include "testutil/util.h" +#include "google/protobuf/arena.h" #include "google/protobuf/dynamic_message.h" #include "google/protobuf/message.h" diff --git a/policy/BUILD b/policy/BUILD index cace76f5b..6a204d07c 100644 --- a/policy/BUILD +++ b/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", diff --git a/policy/yaml_policy_parser.cc b/policy/yaml_policy_parser.cc index 4b4a37cb9..29070f8c7 100644 --- a/policy/yaml_policy_parser.cc +++ b/policy/yaml_policy_parser.cc @@ -14,10 +14,12 @@ #include "policy/yaml_policy_parser.h" +#include #include #include #include #include +#include #include "absl/status/status.h" #include "absl/status/statusor.h" @@ -25,24 +27,88 @@ #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 { + +class ByteToCodepointMapper { + public: + explicit ByteToCodepointMapper(const Source* source) { + if (source == nullptr) return; + std::string text = source->content().ToString(); + 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; + } + } + + SourcePosition GetMarkCodepointPosition(const YAML::Mark& mark) const { + if (mark.is_null() || mark.pos < 0) return -1; + size_t byte_pos = static_cast(mark.pos); + if (mapping_.empty()) return static_cast(byte_pos); + if (byte_pos >= mapping_.size()) return mapping_.back(); + return mapping_[byte_pos]; + } + + private: + std::vector mapping_; +}; + +thread_local const ByteToCodepointMapper* current_mapper = nullptr; + +struct MapperScope { + explicit MapperScope(const ByteToCodepointMapper* mapper) + : prev(current_mapper) { + current_mapper = mapper; + } + ~MapperScope() { current_mapper = prev; } + const ByteToCodepointMapper* prev; +}; + +SourcePosition GetMarkCodepointPosition(const CelPolicyParseContext& ctx, + const YAML::Mark& mark) { + if (current_mapper != nullptr) { + return current_mapper->GetMarkCodepointPosition(mark); + } + if (mark.is_null() || mark.pos < 0) return -1; + return static_cast(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 +128,10 @@ std::optional 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()); ctx.policy_source().NoteSourcePosition(id, element.starting_position); @@ -87,6 +154,9 @@ absl::Status YamlPolicyParser::ParsePolicy(CelPolicyParseContext& ctx) const { return absl::OkStatus(); } + ByteToCodepointMapper mapper(source); + MapperScope scope(&mapper); + ctx.policy().set_description(ValueString(-1, source->description())); std::string text = source->content().ToString(); YAML::Node node; @@ -94,7 +164,8 @@ absl::Status YamlPolicyParser::ParsePolicy(CelPolicyParseContext& ctx) const { 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(); diff --git a/policy/yaml_policy_parser_test.cc b/policy/yaml_policy_parser_test.cc index 4e7dfc49c..90c368546 100644 --- a/policy/yaml_policy_parser_test.cc +++ b/policy/yaml_policy_parser_test.cc @@ -198,6 +198,17 @@ std::vector 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