Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: returns error with illegal args with unknowns flag (#7127) #7149

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kd-labs
Copy link
Contributor

@kd-labs kd-labs commented Nov 2, 2024

Why the changes in this PR are needed?

What are the changes in this PR?

  • We are doing a regex match for the presence of array in the argument passed to unknowns flag. If it matches with array then we return error.

Notes to assist PR review:

Further comments:

Signed-off-by: kunal.das <[email protected]>
Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this! It's got the right shape, I'm just wondering if we could attempt to parse the argument, to have exactly the same guards as would happen later on 🤔 Would you be up for trying this approach?

Thanks again for contributing. 🎉

Comment on lines +141 to +148
//* check if illegal arguments is passed with unknowns flag
regexArr := regexp.MustCompile(`^\[.*\]$`)
for _, unknwn := range p.unknowns {
if regexArr.MatchString(unknwn) {
return errors.New(errIllegalUnknownsArg.Error())
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can attempt to parse it using ast.ParseTerm, like

t, err := ast.ParseTerm(unknwn)
if err != nil { ... }

and then check t.Value's type, like

switch t.Value.(type) {
case ast.Ref: // OK
default:
  return errors.New(errIllegalUnknownsArg.Error())
}

That way, we should be able to capture more illegal inputs than using the regexp. For example, proficient rego users could attempt to pass a set of things like {data.unknown, input.foobar} using {..} which isn't captured by the regular expression.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure...will look into it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

opa eval: panic when given bad input for --unknowns
2 participants