From fe2405842e485ab743cba0ed5021a0ce4ac68609 Mon Sep 17 00:00:00 2001 From: John Chadwick Date: Mon, 7 Apr 2025 13:05:31 -0400 Subject: [PATCH 1/3] Fixes for hostname, port, and enum validation The previous PRs introduce unified validation for a couple of particularly challenging cases (IPv4/IPv6 addresses and URIs.) This PR is relatively light, since the existing validation functions in protovalidate-cc are already structured close to what they need to be. For enum validation, all that was wrong was the message. There are two more classes of conformance test failures: - Bugs in the CEL string.format helper. - A bug in v0.10.3 of the conformance test suite that set IGNORE_ALWAYS on some fields that shouldn't have it. They will be addressed in future PRs, which should allow us to empty out expected_failures.yaml again. --- .../conformance/expected_failures.yaml | 46 ------------------- buf/validate/internal/constraints.cc | 2 +- buf/validate/internal/extra_func.cc | 41 +++++++++-------- 3 files changed, 23 insertions(+), 66 deletions(-) diff --git a/buf/validate/conformance/expected_failures.yaml b/buf/validate/conformance/expected_failures.yaml index 7d8e30a..9ab50b2 100644 --- a/buf/validate/conformance/expected_failures.yaml +++ b/buf/validate/conformance/expected_failures.yaml @@ -1,49 +1,3 @@ -library/is_host_and_port: - - port_required/false/invalid/port_number_sign - # input: [type.googleapis.com/buf.validate.conformance.cases.IsHostAndPort]:{val:"example.com:+0"} - # want: validation error (1 violation) - # 1. constraint_id: "library.is_host_and_port" - # got: valid - - port_required/true/invalid/port_number_sign - # input: [type.googleapis.com/buf.validate.conformance.cases.IsHostAndPort]:{val:"example.com:+0" port_required:true} - # want: validation error (1 violation) - # 1. constraint_id: "library.is_host_and_port" - # got: valid -library/is_hostname: - - valid/label_interior_hyphen - # input: [type.googleapis.com/buf.validate.conformance.cases.IsHostname]:{val:"a-b.a--b"} - # want: valid - # got: validation error (1 violation) - # 1. constraint_id: "library.is_hostname" - # message: "" -standard_constraints/required: - - proto2/scalar/optional/unset - # input: [type.googleapis.com/buf.validate.conformance.cases.RequiredProto2ScalarOptional]:{} - # want: validation error (1 violation) - # 1. constraint_id: "required" - # field: "val" elements:{field_number:1 field_name:"val" field_type:TYPE_STRING} - # rule: "required" elements:{field_number:25 field_name:"required" field_type:TYPE_BOOL} - # got: valid - - proto2/scalar/optional_with_default/unset - # input: [type.googleapis.com/buf.validate.conformance.cases.RequiredProto2ScalarOptionalDefault]:{} - # want: validation error (1 violation) - # 1. constraint_id: "required" - # field: "val" elements:{field_number:1 field_name:"val" field_type:TYPE_STRING} - # rule: "required" elements:{field_number:25 field_name:"required" field_type:TYPE_BOOL} - # got: valid -standard_constraints/enum: - - defined_only/invalid/unknown - # input: [type.googleapis.com/buf.validate.conformance.cases.EnumDefined]:{val:2147483647} - # want: validation error (1 violation) - # 1. constraint_id: "enum.defined_only" - # message: "value must be one of the defined enum values" - # field: "val" elements:{field_number:1 field_name:"val" field_type:TYPE_ENUM} - # rule: "enum.defined_only" elements:{field_number:16 field_name:"enum" field_type:TYPE_MESSAGE} elements:{field_number:2 field_name:"defined_only" field_type:TYPE_BOOL} - # got: validation error (1 violation) - # 1. constraint_id: "enum.defined_only" - # message: "enum value must be defined" - # field: "val" elements:{field_number:1 field_name:"val" field_type:TYPE_ENUM} - # rule: "enum.defined_only" elements:{field_number:16 field_name:"enum" field_type:TYPE_MESSAGE} elements:{field_number:2 field_name:"defined_only" field_type:TYPE_BOOL} standard_constraints/well_known_types/duration: - gt_lt/exclusive/invalid/max # input: [type.googleapis.com/buf.validate.conformance.cases.DurationExLTGT]:{val:{seconds:1}} diff --git a/buf/validate/internal/constraints.cc b/buf/validate/internal/constraints.cc index 550d909..974a79d 100644 --- a/buf/validate/internal/constraints.cc +++ b/buf/validate/internal/constraints.cc @@ -231,7 +231,7 @@ absl::Status EnumConstraintRules::Validate( if (field_->enum_type()->FindValueByNumber(value) == nullptr) { Violation violation; *violation.mutable_constraint_id() = "enum.defined_only"; - *violation.mutable_message() = "enum value must be defined"; + *violation.mutable_message() = "value must be one of the defined enum values"; *violation.mutable_field()->mutable_elements()->Add() = fieldPathElement(field_); *violation.mutable_rule()->mutable_elements()->Add() = fieldPathElement(EnumRules::descriptor()->FindFieldByNumber(EnumRules::kDefinedOnlyFieldNumber)); *violation.mutable_rule()->mutable_elements()->Add() = fieldPathElement(FieldConstraints::descriptor()->FindFieldByNumber(FieldConstraints::kEnumFieldNumber)); diff --git a/buf/validate/internal/extra_func.cc b/buf/validate/internal/extra_func.cc index a405c27..d0fbb7c 100644 --- a/buf/validate/internal/extra_func.cc +++ b/buf/validate/internal/extra_func.cc @@ -138,31 +138,25 @@ cel::CelValue endsWith( return cel::CelValue::CreateBool(result); } -bool IsHostname(std::string_view to_validate) { - if (to_validate.length() > 253) { +bool IsHostname(std::string_view toValidate) { + if (toValidate.length() > 253) { return false; } - static const re2::RE2 component_regex("^[A-Za-z0-9]+(?:-[A-Za-z0-9]+)*$"); - static const re2::RE2 all_digits("^[0-9]*$"); - to_validate = absl::StripSuffix(to_validate, "."); - std::vector split = absl::StrSplit(to_validate, '.'); - if (split.size() < 2) { - return re2::RE2::FullMatch(to_validate, component_regex) && - !re2::RE2::FullMatch(to_validate, all_digits); - } - if (re2::RE2::FullMatch(split[split.size() - 1], all_digits)) { - return false; - } - for (size_t i = 0; i < split.size(); i++) { - const std::string_view& part = split[i]; - if (part.empty() || part.size() > 63) { + toValidate = absl::StripSuffix(toValidate, "."); + bool allDigits = false; + for (auto part : absl::StrSplit(toValidate, '.')) { + allDigits = true; + if (part.empty() || part.size() > 63 || part.starts_with('-') || part.ends_with('-')) { return false; } - if (!re2::RE2::FullMatch(part, component_regex)) { - return false; + for (auto ch : part) { + if ((ch < 'a' || ch > 'z') && (ch < 'A' || ch > 'Z') && (ch < '0' || ch > '9') && ch != '-') { + return false; + } + allDigits = allDigits && ch >= '0' && ch <= '9'; } } - return true; + return !allDigits; } cel::CelValue isHostname(google::protobuf::Arena* arena, cel::CelValue::StringHolder lhs) { @@ -209,6 +203,15 @@ cel::CelValue isIP(google::protobuf::Arena* arena, cel::CelValue::StringHolder l bool IsPort(const std::string_view str) { uint32_t port; + if (str.empty()) { + return false; + } + for (auto c : str) { + if ('0' <= c && c <= '9') { + continue; + } + return false; + } if (!absl::SimpleAtoi(str, &port)) { return false; } From 06d0cfa87fe1ae2c82527efb9f65a062c6799cf3 Mon Sep 17 00:00:00 2001 From: John Chadwick Date: Mon, 7 Apr 2025 13:08:30 -0400 Subject: [PATCH 2/3] Fix accidental C++20 dependency --- buf/validate/internal/extra_func.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/buf/validate/internal/extra_func.cc b/buf/validate/internal/extra_func.cc index d0fbb7c..806b142 100644 --- a/buf/validate/internal/extra_func.cc +++ b/buf/validate/internal/extra_func.cc @@ -146,7 +146,8 @@ bool IsHostname(std::string_view toValidate) { bool allDigits = false; for (auto part : absl::StrSplit(toValidate, '.')) { allDigits = true; - if (part.empty() || part.size() > 63 || part.starts_with('-') || part.ends_with('-')) { + if (part.empty() || part.size() > 63 || absl::StartsWith(part, "-") || + absl::EndsWith(part, "-")) { return false; } for (auto ch : part) { From 2da93e875b89d60c9c2f6200ed40edcc14373edd Mon Sep 17 00:00:00 2001 From: John Chadwick Date: Mon, 7 Apr 2025 13:09:34 -0400 Subject: [PATCH 3/3] Re-add failing required tests accidentally removed --- buf/validate/conformance/expected_failures.yaml | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/buf/validate/conformance/expected_failures.yaml b/buf/validate/conformance/expected_failures.yaml index 9ab50b2..1ed588b 100644 --- a/buf/validate/conformance/expected_failures.yaml +++ b/buf/validate/conformance/expected_failures.yaml @@ -1,3 +1,18 @@ +standard_constraints/required: + - proto2/scalar/optional/unset + # input: [type.googleapis.com/buf.validate.conformance.cases.RequiredProto2ScalarOptional]:{} + # want: validation error (1 violation) + # 1. constraint_id: "required" + # field: "val" elements:{field_number:1 field_name:"val" field_type:TYPE_STRING} + # rule: "required" elements:{field_number:25 field_name:"required" field_type:TYPE_BOOL} + # got: valid + - proto2/scalar/optional_with_default/unset + # input: [type.googleapis.com/buf.validate.conformance.cases.RequiredProto2ScalarOptionalDefault]:{} + # want: validation error (1 violation) + # 1. constraint_id: "required" + # field: "val" elements:{field_number:1 field_name:"val" field_type:TYPE_STRING} + # rule: "required" elements:{field_number:25 field_name:"required" field_type:TYPE_BOOL} + # got: valid standard_constraints/well_known_types/duration: - gt_lt/exclusive/invalid/max # input: [type.googleapis.com/buf.validate.conformance.cases.DurationExLTGT]:{val:{seconds:1}}