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

bug: Panic when using a JSON schema with recursive elements #5166

Closed
liamg opened this issue Sep 21, 2022 · 2 comments · Fixed by #5167
Closed

bug: Panic when using a JSON schema with recursive elements #5166

liamg opened this issue Sep 21, 2022 · 2 comments · Fixed by #5167
Labels

Comments

@liamg
Copy link
Contributor

liamg commented Sep 21, 2022

Short description

I'm using the Go API to apply some policies to a variety of different inputs. I'm using a JSON schema for each input type to validate rules and have come across a panic when using a schema with recursive elements. Minimal example below.

  • OPA version: v0.44.0

The example below is simplified for readability but we need this feature to parse tree structures etc.

Steps To Reproduce

  1. Use the following schema (schema.json):
{
  "$schema": "https://json-schema.org/draft/2020-12/schema",
  "$id": "https://github.com/open-policy-agent/opa/issues/5166",
  "type": "object",
  "properties": {
    "Something": {
      "$ref": "#/$defs/X"
    }
  },
  "$defs": {
    "X": {
      "type": "object",
      "properties": {
        "Y": {
          "$ref": "#/$defs/Y"
        }
      }
    },
    "Y": {
      "type": "object",
      "properties": {
        "X": {
          "$ref": "#/$defs/X"
        }
      }
    }
  }
}
  1. Run opa check -s schema.json input.json (where input.rego is pretty much anything)
  2. KABOOM

Expected behavior

The input.rego should be validated against the input schema.

Happy to have a go at fixing this if I can manage it 🤞

@liamg liamg added the bug label Sep 21, 2022
@boranx
Copy link
Member

boranx commented Sep 22, 2022

fwiw, yeah feel free to fix it :) I was debugging the issue and saw it goes into a loop as the child objects are continuously x->y->x->y then at some point the stack overflows

https://github.com/open-policy-agent/opa/blob/main/ast/compile.go#L1132-L1136

or the following might help for debugging

git diff
diff --git a/ast/compile.go b/ast/compile.go
index 6e863585..5eaa4312 100644
--- a/ast/compile.go
+++ b/ast/compile.go
@@ -1133,6 +1133,7 @@ func parseSchema(schema interface{}) (types.Type, error) {
                        if len(subSchema.PropertiesChildren) > 0 {
                                staticProps := make([]*types.StaticProperty, 0, len(subSchema.PropertiesChildren))
                                for _, pSchema := range subSchema.PropertiesChildren {
+                                       fmt.Println("pSchema:", pSchema.Ref.String())

@liamg
Copy link
Contributor Author

liamg commented Sep 22, 2022

@boranx Thanks, I raised a PR :)

liamg added a commit to liamg/opa that referenced this issue Sep 23, 2022
srenatus pushed a commit that referenced this issue Sep 23, 2022
This adds support for recursive json schemas, such as:

{
  "$schema": "https://json-schema.org/draft/2020-12/schema",
  "$id": "#5166",
  "type": "object",
  "properties": {
    "Something": {
      "$ref": "#/$defs/X"
    }
  },
  "$defs": {
    "X": {
      "type": "object",
      "properties": {
        "Y": {
          "$ref": "#/$defs/Y"
        }
      }
    },
    "Y": {
      "type": "object",
      "properties": {
        "X": {
          "$ref": "#/$defs/X"
        }
      }
    }
  }
}

The parseSchema method now hangs from a schemaParser struct. This allows the use of a
cache that can be populated with definitions as they are seen, to prevent them being
reparsed later. They are added to cache before they are completely parsed, otherwise the
recursive crash would still occur. Thus they are added to the cache with all properties
without values, then the values are added in by reference after the subsequent child tree parse.

I've added a couple of tests that cover my use case, as well as a schema parsing test (this
already passed before my changes - the compile time analysis of the schema is the problem)
for good measure.

Fixes #5166.

Signed-off-by: Liam Galvin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants