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: policy expr type errors due to X.0 floating points in JSON being treated as ints #451

Merged
merged 1 commit into from
Sep 26, 2024

Conversation

j-lanson
Copy link
Collaborator

@j-lanson j-lanson commented Sep 26, 2024

Resolves #449 .

Pernicious bug in policy expression code. JSON has only "Number" type, doesn't distinguish between float and int. When a float that ends in .0 gets serialized then injected into a policy expression, it is treated as an integer. Our policy expression evaluation logic for arithmetic/comparison requires operands be int + int or float + float, so when this bug occurs we get a type error because we do int + float.

This fix updates binary_primitive_op to detect when the operands are int+float or float+int and implicitly upcasts the int to a float. It also removes other type checking from binary_primitive_op, as it was bad design. For example, although we updated fn add to allow DateTime + Span, the type checking in binary_primitive_op did not allow this to occur and we had not tested to encounter this bug before.

Added tests to ensure int+float arithmetic/comparisons return floats, and ensure (add datetime span) does not error.

In a more correct AST design, we would do a first-pass eval of the AST and insert "cast" nodes around integer primitives to make them floats. This fix approach works but doesn't afford per-op control, highlighting the need for #387 .

mchernicoff
mchernicoff previously approved these changes Sep 26, 2024
@j-lanson j-lanson changed the title DRAFT: fix: policy expr type errors due to X.0 floating points in JSON being treated as ints fix: policy expr type errors due to X.0 floating points in JSON being treated as ints Sep 26, 2024
Copy link
Collaborator

@alilleybrinker alilleybrinker left a comment

Choose a reason for hiding this comment

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

Thanks Julian, and yeah I agree on the need for a refactor of this code to address stuff like this more preemptively.

@alilleybrinker alilleybrinker merged commit 25a59e1 into main Sep 26, 2024
9 checks passed
@alilleybrinker alilleybrinker deleted the jlanson/499-bugfix branch November 5, 2024 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Identity analysis in config file format has invalid policy expression
3 participants