From e8aa24c262c4cf90c2dbc59c51e90cb7fa5edaf8 Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Thu, 11 Jul 2024 10:31:14 +0200 Subject: [PATCH] format: Produce error when `--rego-v1` formatted module has rule name conflicting with keyword (#6867) Fixes: #6833 Signed-off-by: Johan Fylling --- ast/policy.go | 42 +++++++++++++- ast/rego_v1.go | 61 ++++++++++++++++---- cmd/fmt.go | 15 ++++- format/format.go | 7 ++- format/testfiles/rego_v1/keywords.rego | 9 +++ format/testfiles/rego_v1/keywords.rego.error | 5 ++ 6 files changed, 125 insertions(+), 14 deletions(-) create mode 100644 format/testfiles/rego_v1/keywords.rego create mode 100644 format/testfiles/rego_v1/keywords.rego.error diff --git a/ast/policy.go b/ast/policy.go index d8e6fa3bc4..ee6e14171f 100644 --- a/ast/policy.go +++ b/ast/policy.go @@ -100,7 +100,9 @@ var Wildcard = &Term{Value: Var("_")} var WildcardPrefix = "$" // Keywords contains strings that map to language keywords. -var Keywords = [...]string{ +var Keywords = KeywordsV0 + +var KeywordsV0 = [...]string{ "not", "package", "import", @@ -114,6 +116,24 @@ var Keywords = [...]string{ "some", } +var KeywordsV1 = [...]string{ + "not", + "package", + "import", + "as", + "default", + "else", + "with", + "null", + "true", + "false", + "some", + "if", + "contains", + "in", + "every", +} + // IsKeyword returns true if s is a language keyword. func IsKeyword(s string) bool { for _, x := range Keywords { @@ -124,6 +144,26 @@ func IsKeyword(s string) bool { return false } +// IsKeywordInRegoVersion returns true if s is a language keyword. +func IsKeywordInRegoVersion(s string, regoVersion RegoVersion) bool { + switch regoVersion { + case RegoV0: + for _, x := range KeywordsV0 { + if x == s { + return true + } + } + case RegoV1, RegoV0CompatV1: + for _, x := range KeywordsV1 { + if x == s { + return true + } + } + } + + return false +} + type ( // Node represents a node in an AST. Nodes may be statements in a policy module // or elements of an ad-hoc query, expression, etc. diff --git a/ast/rego_v1.go b/ast/rego_v1.go index ea3e907d70..9fa1c6f9b4 100644 --- a/ast/rego_v1.go +++ b/ast/rego_v1.go @@ -122,27 +122,65 @@ func checkDeprecatedBuiltinsForCurrentVersion(node interface{}) Errors { return checkDeprecatedBuiltins(deprecatedBuiltins, node) } +type RegoCheckOptions struct { + NoDuplicateImports bool + NoRootDocumentOverrides bool + NoDeprecatedBuiltins bool + NoKeywordsAsRuleNames bool + RequireIfKeyword bool + RequireContainsKeyword bool + RequireRuleBodyOrValue bool +} + +func NewRegoCheckOptions() RegoCheckOptions { + // all options are enabled by default + return RegoCheckOptions{ + NoDuplicateImports: true, + NoRootDocumentOverrides: true, + NoDeprecatedBuiltins: true, + NoKeywordsAsRuleNames: true, + RequireIfKeyword: true, + RequireContainsKeyword: true, + RequireRuleBodyOrValue: true, + } +} + // CheckRegoV1 checks the given module or rule for errors that are specific to Rego v1. // Passing something other than an *ast.Rule or *ast.Module is considered a programming error, and will cause a panic. func CheckRegoV1(x interface{}) Errors { + return CheckRegoV1WithOptions(x, NewRegoCheckOptions()) +} + +func CheckRegoV1WithOptions(x interface{}, opts RegoCheckOptions) Errors { switch x := x.(type) { case *Module: - return checkRegoV1Module(x) + return checkRegoV1Module(x, opts) case *Rule: - return checkRegoV1Rule(x) + return checkRegoV1Rule(x, opts) } panic(fmt.Sprintf("cannot check rego-v1 compatibility on type %T", x)) } -func checkRegoV1Module(module *Module) Errors { +func checkRegoV1Module(module *Module, opts RegoCheckOptions) Errors { var errors Errors - errors = append(errors, checkDuplicateImports([]*Module{module})...) - errors = append(errors, checkRootDocumentOverrides(module)...) - errors = append(errors, checkDeprecatedBuiltinsForCurrentVersion(module)...) + if opts.NoDuplicateImports { + errors = append(errors, checkDuplicateImports([]*Module{module})...) + } + if opts.NoRootDocumentOverrides { + errors = append(errors, checkRootDocumentOverrides(module)...) + } + if opts.NoDeprecatedBuiltins { + errors = append(errors, checkDeprecatedBuiltinsForCurrentVersion(module)...) + } + + for _, rule := range module.Rules { + errors = append(errors, checkRegoV1Rule(rule, opts)...) + } + return errors } -func checkRegoV1Rule(rule *Rule) Errors { +func checkRegoV1Rule(rule *Rule, opts RegoCheckOptions) Errors { t := "rule" if rule.isFunction() { t = "function" @@ -150,13 +188,16 @@ func checkRegoV1Rule(rule *Rule) Errors { var errs Errors - if rule.generatedBody && rule.Head.generatedValue { + if opts.NoKeywordsAsRuleNames && IsKeywordInRegoVersion(rule.Head.Name.String(), RegoV1) { + errs = append(errs, NewError(ParseErr, rule.Location, fmt.Sprintf("%s keyword cannot be used for rule name", rule.Head.Name.String()))) + } + if opts.RequireRuleBodyOrValue && rule.generatedBody && rule.Head.generatedValue { errs = append(errs, NewError(ParseErr, rule.Location, "%s must have value assignment and/or body declaration", t)) } - if rule.Body != nil && !rule.generatedBody && !ruleDeclarationHasKeyword(rule, tokens.If) && !rule.Default { + if opts.RequireIfKeyword && rule.Body != nil && !rule.generatedBody && !ruleDeclarationHasKeyword(rule, tokens.If) && !rule.Default { errs = append(errs, NewError(ParseErr, rule.Location, "`if` keyword is required before %s body", t)) } - if rule.Head.RuleKind() == MultiValue && !ruleDeclarationHasKeyword(rule, tokens.Contains) { + if opts.RequireContainsKeyword && rule.Head.RuleKind() == MultiValue && !ruleDeclarationHasKeyword(rule, tokens.Contains) { errs = append(errs, NewError(ParseErr, rule.Location, "`contains` keyword is required for partial set rules")) } diff --git a/cmd/fmt.go b/cmd/fmt.go index df0622aadb..bd464e84ea 100644 --- a/cmd/fmt.go +++ b/cmd/fmt.go @@ -27,6 +27,7 @@ type fmtCommandParams struct { fail bool regoV1 bool v1Compatible bool + checkResult bool } var fmtParams = fmtCommandParams{} @@ -126,13 +127,22 @@ func formatFile(params *fmtCommandParams, out io.Writer, filename string, info o return newError("failed to open file: %v", err) } - opts := format.Opts{} - opts.RegoVersion = params.regoVersion() + opts := format.Opts{ + RegoVersion: params.regoVersion(), + } formatted, err := format.SourceWithOpts(filename, contents, opts) if err != nil { return newError("failed to format Rego source file: %v", err) } + if params.checkResult { + popts := ast.ParserOptions{RegoVersion: params.regoVersion()} + _, err := ast.ParseModuleWithOpts("formatted", string(formatted), popts) + if err != nil { + return newError("%s was successfully formatted, but the result is invalid: %v\n\nTo inspect the formatted Rego, you can turn off this check with --check-result=false.", filename, err) + } + } + changed := !bytes.Equal(contents, formatted) if params.fail && !params.list && !params.diff { @@ -229,6 +239,7 @@ func init() { formatCommand.Flags().BoolVar(&fmtParams.fail, "fail", false, "non zero exit code on reformat") addRegoV1FlagWithDescription(formatCommand.Flags(), &fmtParams.regoV1, false, "format module(s) to be compatible with both Rego v1 and current OPA version)") addV1CompatibleFlag(formatCommand.Flags(), &fmtParams.v1Compatible, false) + formatCommand.Flags().BoolVar(&fmtParams.checkResult, "check-result", true, "assert that the formatted code is valid and can be successfully parsed (default true)") RootCommand.AddCommand(formatCommand) } diff --git a/format/format.go b/format/format.go index 02b6f73d82..4197a8cde5 100644 --- a/format/format.go +++ b/format/format.go @@ -56,7 +56,12 @@ func SourceWithOpts(filename string, src []byte, opts Opts) ([]byte, error) { } if opts.RegoVersion == ast.RegoV0CompatV1 || opts.RegoVersion == ast.RegoV1 { - errors := ast.CheckRegoV1(module) + checkOpts := ast.NewRegoCheckOptions() + // The module is parsed as v0, so we need to disable checks that will be automatically amended by the AstWithOpts call anyways. + checkOpts.RequireIfKeyword = false + checkOpts.RequireContainsKeyword = false + checkOpts.RequireRuleBodyOrValue = false + errors := ast.CheckRegoV1WithOptions(module, checkOpts) if len(errors) > 0 { return nil, errors } diff --git a/format/testfiles/rego_v1/keywords.rego b/format/testfiles/rego_v1/keywords.rego new file mode 100644 index 0000000000..b2b791436e --- /dev/null +++ b/format/testfiles/rego_v1/keywords.rego @@ -0,0 +1,9 @@ +package test + +if := 1 + +contains := 2 + +in := 3 + +every := 4 diff --git a/format/testfiles/rego_v1/keywords.rego.error b/format/testfiles/rego_v1/keywords.rego.error new file mode 100644 index 0000000000..fb9c3340be --- /dev/null +++ b/format/testfiles/rego_v1/keywords.rego.error @@ -0,0 +1,5 @@ +4 errors occurred: +testfiles/rego_v1/keywords.rego:3: rego_parse_error: if keyword cannot be used for rule name +testfiles/rego_v1/keywords.rego:5: rego_parse_error: contains keyword cannot be used for rule name +testfiles/rego_v1/keywords.rego:7: rego_parse_error: in keyword cannot be used for rule name +testfiles/rego_v1/keywords.rego:9: rego_parse_error: every keyword cannot be used for rule name \ No newline at end of file