cedar-policy / cedar

Implementation of the Cedar Policy Language
https://www.cedarpolicy.com
Apache License 2.0
889 stars 80 forks source link

When parsing a `StaticPolicySet` from a text representation, only the first error gets recorded. #1293

Open AndreasKoestler opened 3 weeks ago

AndreasKoestler commented 3 weeks ago

Before opening, please confirm:

Bug Category

Cedar Parser

Describe the bug

When parsing a StaticPolicySet from a text representation, only the first error gets recorded. This makes handling the parsing results difficult. The use case at hand is the integration with a code editor - in general you want to handle/hint on all parsing errors for a given policy set.

Expected behavior

All errors in a provided text representation of a `StaticPolicySet should be returned to allow processing of the errors, e.g. displaying errors in a code editor.

Reproduction steps

Call check_parse_policy_set on the text representation of a policy set containing more than one error.

Code Snippet

let content = r#"permit(
  principal == U ser::"alice", 
  action    == Act ion::"view", 
  reso urce  == Photo::"VacationPhoto94.jpg"
); // We have three syntax errors here. 
"#;
let maybe_policies = PolicySet.from_str(content);
let res = match maybe_policies {
        Ok(policies) => check_parse_policy_set(policies), // This should report 3 errors but only reports the first. 
        Err(err) => todo!()
};

let err_count = match result {
        CheckParseAnswer::Success => 0,
        CheckParseAnswer::Failure { errors } => errors.len(),
};

assert_eq!(err_count, 3);

Log output

// Put your output below this line

Additional configuration

No response

Operating System

Linux

Additional information and screenshots

No response

john-h-kastner-aws commented 3 weeks ago

Hi, thanks for the question. Our parse error messages include a related field which contain some other associated error messages. If you look at that list of errors you should see that everything is reported. These FFI interfaces aren't the most well documented, but here's a link to it in the docs: https://docs.rs/cedar-policy/latest/cedar_policy/ffi/struct.DetailedError.html#structfield.related

Let me know if this doesn't work

AndreasKoestler commented 3 weeks ago

Indeed, related contains the remaining errors. For the above example, this yields:

Top level: "failed to parse policies from string: unexpected token `ser`"
related: Array(2)
0: 'unexpected token `ion`
1: 'unexpected token `to`'

However, this breaks down if I modify the example slightly:

permit(
  prin cipal == U ser::"alice", 
  action    == Act ion::"view", 
  resource  == Pho to::"VacationPhoto94.jpg"
)

Now I only get the top-level error and no related errors:

failed to parse policies from string: unexpected token `cipal`

Is this an issue with parsing and error reporting or am I not using the parsing interface correctly?

john-h-kastner-aws commented 3 weeks ago

It looks like you're using it correctly. Now you're running into a known limitation in how our parser handles error recovery. When there's a parse error in that position, it skips ahead to the start of the next policy, ignoring any other parse error which may exist in the current policy. For the other errors it is able to recover and continue parsing within the same policy. This could be improved, but likely won't be a priority for us to get to.

If you were interested in hacking on our parsers and contributing an improvement yourself, the place to start would be the relevant production in our grammar and the lalrpop documentation for error recovery.

You might also run into the related issue #1054 where a single parsing error will prevent us from reporting any higher level errors.