Number literals with leading zeros cause issues in JSON marshaling

[XuejiaoZhang]

Expected Behavior

Run OPA server, specifying the policies

$ opa run -s policies/

Then list all policies

$ curl localhost:8181/v1/policies  -H 'Content-Type: application/json'

Actual Behavior

Steps to Reproduce the Problem

Got the following compliant

$ curl localhost:8181/v1/policies  -H 'Content-Type: application/json'
{
  "code": "internal_error",
  "message": "json: error calling MarshalJSON for type ast.Body: json: error calling MarshalJSON for type *ast.Term: json: error calling MarshalJSON for type ast.Number: json: invalid number literal \"00\""
}

Additional Info

I am wondering whether there are any requirements/dependencies on the policy file or anything else. Thanks.

[srenatus]

Thanks for reporting this.

Can you share your policy, or the part of it that's troubling here, in case it's clearly isolated and known?

[XuejiaoZhang]

Thanks! @sedooe

I think I found the cause. The policy file was similar to the following.

package example.rule

allow = true {
    a = 0
    a == 00 # bug here, should be a == 0  
} else = false

It was a bug in the policy writing. but it's really hard to debug based on the error message.

Is it possible to have the compliant more readable? Thanks.

[srenatus]

With your example code in foo.rego, I've not been able to replicate this:

If the policy is added with API (put), it could be listed.

opa/issue-2947 % curl localhost:8181/v1/policies/foo
{
  "code": "resource_not_found",
  "message": "storage_not_found_error: policy id \"foo\""
}
opa/issue-2947 % curl localhost:8181/v1/policies/foo  -H 'Content-Type: text/plain' -XPUT --data-binary @foo.rego
{}%                                                                                                                           
opa/issue-2947 % curl localhost:8181/v1/policies/foo
{
  "code": "internal_error",
  "message": "json: error calling MarshalJSON for type ast.Body: json: error calling MarshalJSON for type *ast.Term: json: error calling MarshalJSON for type ast.Number: json: invalid number literal \"00\""
}

That output is the same I'd get when running opa run -s foo.rego and querying for the policy. So at least that seems consistent to me -- or am I missing something?

[XuejiaoZhang]

One more thing I forgot to mention is that the error only occurred when there were 2 layers' (may be more than 1 layer) folders, as following policies/abc/

$ ls policies/abc/example.rego 
policies/abc/example.rego
$  opa run -s policies
{"addrs":[":8181"],"diagnostic-addrs":[],"insecure_addr":"","level":"info","msg":"Initializing server.","time":"2020-11-25T21:00:05+08:00"}
{"client_addr":"127.0.0.1:57985","level":"info","msg":"Received request.","req_id":1,"req_method":"GET","req_path":"/v1/policies","time":"2020-11-25T21:00:09+08:00"}
{"client_addr":"127.0.0.1:57985","level":"info","msg":"Sent response.","req_id":1,"req_method":"GET","req_path":"/v1/policies","resp_bytes":239,"resp_duration":0.717608,"resp_status":500,"time":"2020-11-25T21:00:09+08:00"}

[srenatus]

At its core, https://play.golang.org/p/NufrdsAa1lg replicates what we're seeing.

The problem here is that a policy containing 00 is accepted at all (either way, CLI arg or via API call).

[XuejiaoZhang]

Do you mean, "is NOT accepted at all", right?
But the behaviors are different for different parent folder layers of rego file, that's strange.
Still, the error message is not that helpful.
Anyway, we could close the ticket.

Thanks @srenatus !

[srenatus]

No I think this is a valid problem. I could not replicate what you saw with PUTing the policy vs. loading it from the command line, but the fact that you can store a policy that cannot be marshalled to JSON is a bug. 🐛

[srenatus]

Underlying this is the fact that we're using math/big's Parse to verify that a number is a number; but that'll accept leading zeros. We store the number as-is in the AST then. On marshaling it to JSON, that fails because json.Number (the string) cannot have leading zeros.

I think this boils down to deciding what we want to support, and fixing the problem accordingly. I'm leaning towards not allowing leading zeros in number literals.

[XuejiaoZhang]

@srenatus Hm, so the policy code itself is kinda right, it would do what it suppose to, just the writing 0 is in a strange way 00.

We are in an agreement there is a bug there. Thanks you for digging into it.

[srenatus]

I took the liberty of renaming the issue to make it a little more focused, hope you don't mind. Thanks again for bringing this up!

[XuejiaoZhang]

Sure, thanks @srenatus.

[tsandall]

This looks like an issue in the scanner. It shouldn't be scanning number literals with leading zeros (that's not valid JSON). We don't support non-decimal numbers so this should be rejected immediately when the policy is tokenized.