Add proto validation for RuleType #4907

rdimitrov · 2024-11-07T17:00:36Z

Summary

Ref https://github.com/stacklok/minder-stories/issues/94

Change Type

Mark the type of change your PR introduces:

Bug fix (resolves an issue without affecting existing features)
Feature (adds new functionality without breaking changes)
Breaking change (may impact existing functionalities or require documentation updates)
Documentation (updates or additions to documentation)
Refactoring or test improvements (no bug fixes or new functionality)

Testing

Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.

Review Checklist:

Reviewed my own code for quality and clarity.
Added comments to complex or tricky code sections.
Updated any affected documentation.
Included tests that validate the fix or feature.
Checked that related changes are merged.

coveralls · 2024-11-07T17:12:24Z

coverage: 54.762% (+0.007%) from 54.755%
when pulling 9d2b131 on validate-ruletype
into 2683161 on main.

evankanderson

I only got partway through, this is a big one.

evankanderson · 2024-11-07T23:40:39Z

proto/minder/v1/minder.proto

-        string body = 2;
+        string body = 2 [
+            (buf.validate.field).string = {
+                pattern: "^\\s*\\{.*\\}\\s*$",


I'm not sure what this is trying to match. This may be a case where we can't qualify the contents or need to use CEL to do so.

Yes, it's supposed to be JSON there. so I added the following ^\\s*\\{[\\s\\S]*\\}\\s*$ which should check for opening/closing bracket and match any character including new lines

evankanderson · 2024-11-07T23:40:56Z

proto/minder/v1/minder.proto

+                max_len: 1000,
+            },
+            (buf.validate.field).ignore = IGNORE_IF_DEFAULT_VALUE
+        ];
    }

    // endpoint is the endpoint to fetch data from.
    // This can be a URL or the path on the API.bool


I wonder what .bool means here.

I think it's perhaps pool or a typo?

I suspect it's a typo. Want to fix it?

evankanderson · 2024-11-07T23:41:48Z

proto/minder/v1/minder.proto

-    string endpoint = 1;
+    string endpoint = 1 [
+        (buf.validate.field).string = {
+            pattern: "^(https:\\/\\/[a-zA-Z0-9.-]+)?(\\/[a-zA-Z0-9._-]+)*(\\/\\{\\{[^}]+\\}\\})*(\\/)?$",


Is this a "URI with Go template values embedded"? Again, CEL might be an option, or combining prefix with some other expressions.

Yes, I updated it to "^(https?:\\/\\/)?[a-zA-Z0-9._-]+(\\/[a-zA-Z0-9._{}\\/-]*)?$" which should allow for:

"https://api.something.com"

"https://api.something.com/path/to/resource"

"/repos/{{.Entity.Owner}}/{{.Entity.Name}}/branches/main"

"{{ $branch_param := index .Params "branch" }}/repos/{{.Entity.Owner}}/{{.Entity.Name}}"

evankanderson · 2024-11-07T23:42:05Z

proto/minder/v1/minder.proto

-    string method = 2;
+    string method = 2 [
+        (buf.validate.field).string = {
+            pattern: "^(GET|POST|PUT|PATCH|DELETE)$",


Prefer in

Is capitalization important?

Thanks, made it case insensitive 👍

in doesn't work though as I couldn't find an option to ignore casing in parallel 👍 The other way was to expand the list with upper and lower case values which seems odd

evankanderson · 2024-11-07T23:42:37Z

proto/minder/v1/minder.proto

+        (buf.validate.field).repeated = {
+            items: {
+                string: {
+                    pattern: "^[a-zA-Z0-9-]+: .+$",


You probably want to limit the RHS to avoid \n, among others.

Adressed 👍

evankanderson · 2024-11-07T23:44:22Z

proto/minder/v1/minder.proto

-    string branch = 2;
+    string branch = 2 [
+        (buf.validate.field).string = {
+            pattern: "^[a-zA-Z0-9._/-]+$",


^[[:word:]./-]+$?

evankanderson · 2024-11-07T23:45:22Z

proto/minder/v1/minder.proto

-    string version = 11;
+    string version = 11 [
+        (buf.validate.field).string = {
+            pattern: "^v\\d+(\\.\\d+)?(\\.\\d+)?(?:-[\\w\\.-]+)?$",


Again, I'd prefer just v1 than v1.1.7 here for now.

evankanderson · 2024-11-07T23:46:04Z

proto/minder/v1/minder.proto

-    string short_failure_message = 10;
+    string short_failure_message = 10 [
+        (buf.validate.field).string = {
+            pattern: "[A-Za-z][-/.!?,:;[:word:] *#\\[\\]()`{}]*",


Is this markdown, or plain-text?

It should be markdown

This may be a case where we want to simply say "this field is markdown", and then do validation in the code, rather than via annotation.

Removed the pattern check but left the length one 👍

evankanderson · 2024-11-07T23:46:42Z

proto/minder/v1/minder.proto

-            string type = 1;
+            string type = 1 [
+                (buf.validate.field).string = {
+                    pattern: "^[a-z]+(_[a-z]+)*$",


Should this use in, or are there more types?

I think there are, i.e. recently we added yq I believe

If this is still a small enum, I'd use those values, as it also provides documentation.

Refactored it to using in 👍

evankanderson · 2024-11-07T23:47:12Z

proto/minder/v1/minder.proto

-                    string def = 1;
+                    string def = 1 [
+                        (buf.validate.field).string = {
+                            pattern: "^\\.[a-z_]+(\\.[a-z_]+)*$",


Hey, this would have fixed my bad ruletype debugging experience! Thank you!

Do we need to accommodate " or array access ([])?

evankanderson

Okay, I got through the rest of this one.

At some point, we should look at creating predefined constraints for a lot of our use cases. For example:

rule_type_name, markdown_with_go_templates, profile_name, file_name etc.

I wouldn't do that here at this point, because it looks like it might be a lot of toolchaining, but we should leave a note somewhere for when someone is looking for a mid-sized cleanup project.

evankanderson · 2024-11-08T14:58:23Z

proto/minder/v1/minder.proto

-        string depfile = 2;
+        string depfile = 2 [
+            (buf.validate.field).string = {
+                pattern: "^(\\./)?([a-zA-Z0-9_]+\\/)*[a-zA-Z0-9_]+(\\.[a-zA-Z0-9]+)?$",


Suggested change

pattern: "^(\\./)?([a-zA-Z0-9_]+\\/)*[a-zA-Z0-9_]+(\\.[a-zA-Z0-9]+)?$",

pattern: "^(\\./)?([[:word:]]+\\/)*[[:word:]]+(\\.[[:alnum:]]+)?$",

Though, it seems like we should allow for . to show up in other file parts, so maybe:

Suggested change

pattern: "^(\\./)?([a-zA-Z0-9_]+\\/)*[a-zA-Z0-9_]+(\\.[a-zA-Z0-9]+)?$",

pattern: "^([[:word:].]+\\/)*[[:word:].]+$",

evankanderson · 2024-11-08T16:40:16Z

proto/minder/v1/minder.proto

-    string short_failure_message = 10;
+    string short_failure_message = 10 [
+        (buf.validate.field).string = {
+            pattern: "[A-Za-z][-/.!?,:;[:word:] *#\\[\\]()`{}]*",


This may be a case where we want to simply say "this field is markdown", and then do validation in the code, rather than via annotation.

evankanderson · 2024-11-08T16:40:38Z

proto/minder/v1/minder.proto

-            string type = 1;
+            string type = 1 [
+                (buf.validate.field).string = {
+                    pattern: "^[a-z]+(_[a-z]+)*$",


If this is still a small enum, I'd use those values, as it also provides documentation.

evankanderson · 2024-11-08T16:42:30Z

proto/minder/v1/minder.proto

-                    string def = 1;
+                    string def = 1 [
+                        (buf.validate.field).string = {
+                            pattern: "^\\.[a-z_]+(\\.[a-z_]+|\\[\\d+]|\\[\"[a-z_]+\"\\])*$",


Do we allow uppercase letters here? I know that GitHub prefers lower_with_underscores, but I think these could also be camelCase.

Updated it to allow for camelCase 👍

evankanderson · 2024-11-08T16:43:07Z

proto/minder/v1/minder.proto

-                string type = 1;
+                string type = 1 [
+                    (buf.validate.field).string = {
+                        pattern: "^[a-z]+(_[a-z]+)*$",


Can we use:

Suggested change

pattern: "^[a-z]+(_[a-z]+)*$",

in: ["deny-by-default", "constraints"]

evankanderson · 2024-11-08T17:01:34Z

proto/minder/v1/minder.proto

+                        pattern: "^minder(?:\\.[a-zA-Z0-9_]+)*$",
+                        max_len: 200,
+                    }
+                ];
                // params are unstructured parameters passed to the method. These are optional
                // and evaluated by the method.
                google.protobuf.Struct params = 6;


Hmm, this is tricky, because it's arbitrary content, but in a structured format that may be interpreted by the rest of our rules.

I don't think we can do much here now, but we'll get a fun bug at some point later.

yes 👍 I wonder if we should cover this by having custom validation functions or by updating our API to expect a better defined field for this

evankanderson · 2024-11-08T17:03:34Z

proto/minder/v1/minder.proto

-            string type = 1;
+            string type = 1 [
+                (buf.validate.field).string = {
+                    pattern: "^[a-z]+(_[a-z]+)*$",


Maybe:

Suggested change

pattern: "^[a-z]+(_[a-z]+)*$",

in: ["security_advisory"]

Applied 👍

evankanderson · 2024-11-08T17:06:44Z

proto/minder/v1/minder.proto

-                string severity = 1;
+                string severity = 1 [
+                    (buf.validate.field).string = {
+                        pattern: "^[a-z]+(_[a-z]+)*$",


This is a Severity enum as a string:

Suggested change

pattern: "^[a-z]+(_[a-z]+)*$",

in: ["unknown", "info", "low", "medium", "high", "critical"]

Applied 👍

evankanderson · 2024-11-08T17:07:08Z

proto/minder/v1/minder.proto

-    string description = 5;
+    string description = 5 [
+        (buf.validate.field).string = {
+            pattern: "[A-Za-z][-/.!?,:;[:word:] *#\\[\\]()`{}]*",


Again, this is markdown, so we can do this for now, but we may need to parse in Go code.

Removed the pattern validation 👍

evankanderson · 2024-11-08T17:07:41Z

proto/minder/v1/minder.proto

-    string guidance = 6;
+    string guidance = 6 [
+        (buf.validate.field).string = {
+            pattern: "[A-Za-z][-/.!?,:;[:word:] *#\\[\\]()`{}]*",


Again, I think this is markdown. We should add to the comment in all the markdown cases, saying:

This value should be rendered as markdown.

Removed the pattern validation 👍

evankanderson

Just a few more comments, but I'm not sure they are blocking, you could probably revisit them in a future PR.

evankanderson · 2024-11-11T14:49:38Z

proto/minder/v1/minder.proto

+                max_len: 1000,
+            },
+            (buf.validate.field).ignore = IGNORE_IF_DEFAULT_VALUE
+        ];
    }

    // endpoint is the endpoint to fetch data from.
    // This can be a URL or the path on the API.bool


I suspect it's a typo. Want to fix it?

evankanderson · 2024-11-11T14:50:13Z

proto/minder/v1/minder.proto

-        string body = 2;
+        string body = 2 [
+            (buf.validate.field).string = {
+                pattern: "^\\s*\\{[\\s\\S]*\\}\\s*$",


Is this trying to say "valid JSON"? This pattern doesn't seem meaningful.

evankanderson · 2024-11-11T14:57:23Z

proto/minder/v1/minder.proto

+        (buf.validate.field).repeated = {
+            items: {
+                string: {
+                    pattern: "^[a-zA-Z0-9-]+: [^\n\r]+$",


I'm going to let this pass for now, but we should prefer a positive expression. From https://www.rfc-editor.org/rfc/rfc7230#section-3.2 :

Suggested change

pattern: "^[a-zA-Z0-9-]+: [^\n\r]+$",

pattern: "^[a-zA-Z0-9-]+:[[:graph:][:blank:]]+$",

evankanderson · 2024-11-11T14:57:54Z

proto/minder/v1/minder.proto

-    optional string body = 4;
+    optional string body = 4 [
+        (buf.validate.field).string = {
+            pattern: "^\\s*\\{[\\s\\S]*\\}\\s*$",


Again, I think this is "is JSON", which I'd just leave as a comment here.

evankanderson · 2024-11-11T15:00:13Z

proto/minder/v1/minder.proto

-        string depfile = 2;
+        string depfile = 2 [
+            (buf.validate.field).string = {
+                pattern: "^(\\./)?([a-zA-Z0-9_\\-]+/)*[a-zA-Z0-9_\\-]+(\\.[a-zA-Z0-9]+)?$",


We don't want to allow . in the filename for the depfile path or multiple dots in the requirements file? (The python "standard" for development dependencies is requirements-dep.txt, but given how loose that standard is, I could see requirements.dep.txt slipping in.)

yeah, I wasn't sure about this too, but I'm okay to leave it for now because of the reasons you mentioned 👍

evankanderson · 2024-11-11T15:01:57Z

proto/minder/v1/minder.proto

                }

                // the title of the PR
-                string title = 1;
+                string title = 1 [
+                    (buf.validate.field).string = {


No pattern here? That could be okay, given that GitHub will validate this, but you should put a comment as to why no pattern.

evankanderson · 2024-11-11T15:03:20Z

proto/minder/v1/minder.proto

-    string description = 5;
+    string description = 5 [
+        (buf.validate.field).string = {
+            min_len: 1,


Again, add a comment that this is markdown?

evankanderson · 2024-11-11T15:04:12Z

proto/minder/v1/minder.proto


    // guidance are instructions we give the user in case a rule fails.
-    string guidance = 6;
+    string guidance = 6 [
+        (buf.validate.field).string = {


Ditto on the markdown comment.

Signed-off-by: Radoslav Dimitrov <[email protected]>

rdimitrov requested a review from a team as a code owner November 7, 2024 17:00

rdimitrov force-pushed the validate-ruletype branch from a0f1896 to 0de2694 Compare November 7, 2024 17:06

rdimitrov self-assigned this Nov 7, 2024

rdimitrov force-pushed the validate-ruletype branch from 0de2694 to 86c8194 Compare November 7, 2024 23:23

evankanderson reviewed Nov 7, 2024

View reviewed changes

rdimitrov force-pushed the validate-ruletype branch 4 times, most recently from 50b8d78 to 6892cd7 Compare November 8, 2024 16:26

evankanderson reviewed Nov 8, 2024

View reviewed changes

rdimitrov force-pushed the validate-ruletype branch 2 times, most recently from a25d1d5 to ab5a7f1 Compare November 11, 2024 12:21

evankanderson reviewed Nov 11, 2024

View reviewed changes

rdimitrov added 8 commits November 11, 2024 17:15

Add proto validation for RuleType

7ebe318

Signed-off-by: Radoslav Dimitrov <[email protected]>

Refactor the description, guidance and error message validations

27954ed

Signed-off-by: Radoslav Dimitrov <[email protected]>

Update validations

b1899e5

Signed-off-by: Radoslav Dimitrov <[email protected]>

Allow for the ' character in guidance and descriptions

a89d96a

Signed-off-by: Radoslav Dimitrov <[email protected]>

Update validations for rules and profiles

210e163

Signed-off-by: Radoslav Dimitrov <[email protected]>

Refactor the validations to use an exact match instead of a regex

b5e8e8a

Signed-off-by: Radoslav Dimitrov <[email protected]>

Refactor validation for description and guidance

fdd2fad

Signed-off-by: Radoslav Dimitrov <[email protected]>

Add comments and update proto validation

9d2b131

Signed-off-by: Radoslav Dimitrov <[email protected]>

rdimitrov force-pushed the validate-ruletype branch from fc8e7b5 to 9d2b131 Compare November 11, 2024 15:29

evankanderson approved these changes Nov 11, 2024

View reviewed changes

rdimitrov merged commit 5971728 into main Nov 11, 2024
26 checks passed

rdimitrov deleted the validate-ruletype branch November 11, 2024 16:52

	pattern: "^(\\./)?([a-zA-Z0-9_]+\\/)*[a-zA-Z0-9_]+(\\.[a-zA-Z0-9]+)?$",
	pattern: "^(\\./)?([[:word:]]+\\/)*[[:word:]]+(\\.[[:alnum:]]+)?$",

	pattern: "^(\\./)?([a-zA-Z0-9_]+\\/)*[a-zA-Z0-9_]+(\\.[a-zA-Z0-9]+)?$",
	pattern: "^([[:word:].]+\\/)*[[:word:].]+$",

	pattern: "^[a-z]+(_[a-z]+)*$",
	in: ["deny-by-default", "constraints"]

	pattern: "^[a-z]+(_[a-z]+)*$",
	in: ["unknown", "info", "low", "medium", "high", "critical"]

	pattern: "^[a-zA-Z0-9-]+: [^\n\r]+$",
	pattern: "^[a-zA-Z0-9-]+:[[:graph:][:blank:]]+$",

Add proto validation for RuleType #4907

Add proto validation for RuleType #4907

Conversation

rdimitrov commented Nov 7, 2024

Summary

Change Type

Testing

Review Checklist:

coveralls commented Nov 7, 2024 • edited Loading

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 7, 2024 •

edited

Loading