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 SchemaLoader crashes when handling $refs #1385

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Commits on Sep 4, 2023

  1. schemaloader: Fix null dereference when loading $ref'ed schema with a…

    … null value
    
    If a $ref'ed schema contains a value with null inside,
    _manageRecursivePointer was crashing on that trying to access null.$ref
    as e.g in the following example:
    
        ---- 8< ---- (schema.json)
        {
          "$ref": "main.json"
        }
    
        ---- 8< ---- (main.json)
        {
          "properties": {
            "timeout": {
              "default": null,
              "type": ["number", "null"]
            }
          }
        }
    
        schemaloader.js:264 Uncaught (in promise) TypeError: Cannot read properties of null (reading '$ref')
            at eval (schemaloader.js:264:1)
            at Array.forEach (<anonymous>)
            at SchemaLoader._manageRecursivePointer (schemaloader.js:263:1)
            at SchemaLoader._getExternalRefs (schemaloader.js:279:1)
            at eval (schemaloader.js:309:1)
            at Array.forEach (<anonymous>)
            at SchemaLoader._getExternalRefs (schemaloader.js:298:1)
            at eval (schemaloader.js:309:1)
            at Array.forEach (<anonymous>)
            at SchemaLoader._getExternalRefs (schemaloader.js:298:1)
    
    -> Fix it by by checking entry's value is non-null before trying to
    further access its .$ref .
    
    Fixes: json-editor#1383
    navytux committed Sep 4, 2023
    Configuration menu
    Copy the full SHA
    3e33fbc View commit details
    Browse the repository at this point in the history
  2. schemaloader: Fix expandRefs not to crash if $ref'ed schema also has …

    …another $ref to self internally
    
    If a $ref'ed schema contains another $ref referring to somewhere inside
    of it, and if that second $ref lives in an array container, expandRefs
    was wrongly processing that second link, and, after emitting a warning
    was invoking expandSchema(schema=undefined) which was further crashing
    on trying to access undefined.type.
    
    The following example demonstrates this situation:
    
        ---- 8< ---- (schema.json)
        {
          "$ref": "main.json"
        }
    
        ---- 8< ---- (main.json)
        {
          "$schema": "http://json-schema.org/draft-07/schema#",
          "type": "object",
          "definitions": {
            "tcpv4port": {
              "type": "integer"
            }
          },
          "properties": {
            "port": {
              "anyOf": [
                {
                  "$ref": "#/definitions/tcpv4port"
                }
              ]
            }
          }
        }
    
    SchemaLoader.load initially handles this schema ok, but later
    SchemaLoader.expandSchema, which is used by JSONEditor during form
    building, is crashing when invoked with schema of "port":
    
        Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'type')
            at eval (schemaloader.js:219:1)
            at Array.forEach (<anonymous>)
            at SchemaLoader.expandSchema (schemaloader.js:218:1)
            at SchemaLoader.expandRefs (schemaloader.js:186:1)
            at SchemaLoader.expandSchema (schemaloader.js:232:1)
            at eval (schemaloader.js:68:1)
            at Array.forEach (<anonymous>)
            at SchemaLoader.anyOf (schemaloader.js:67:1)
            at eval (schemaloader.js:220:1)
            at Array.forEach (<anonymous>)
        eval	@	schemaloader.js:219
        expandSchema	@	schemaloader.js:218
        expandRefs	@	schemaloader.js:186
        expandSchema	@	schemaloader.js:232
        eval	@	schemaloader.js:68
        anyOf	@	schemaloader.js:67
        eval	@	schemaloader.js:220
        expandSchema	@	schemaloader.js:218
        expandSchema	@	core.js:67
        getEditorClass	@	core.js:211
        addObjectProperty	@	object.js:1014
        eval	@	object.js:423
        preBuild	@	object.js:422
        _callee$	@	core.js:81
        tryCatch	@	core.js:2
        eval	@	core.js:2
        eval	@	core.js:2
        asyncGeneratorStep	@	core.js:2
        _next	@	core.js:2
        Promise.then (async)
        asyncGeneratorStep	@	core.js:2
        _next	@	core.js:2
        eval	@	core.js:2
        eval	@	core.js:2
        load	@	core.js:102
        JSONEditor	@	core.js:61
        (anonymous)	@	k.html:98
    
    The problem here is that upon loading SchemaLoader.load returns schema
    with the following part for "port":
    
        { anyOf: [ { $ref: '#/counter/2' } ] }
    
    which expandRefs will need to further dereference. expandRefs, when run
    on that part, internally reconstructs ref for that $ref as
    
        ref = 'main.json#/definitions/tcpv4port'
    
    because after loading
    
        loader.refs['#/counter/2'] = { fetchUrl: 'main.json', $ref: '#/definitions/tcpv4port' }
    
    and further tries to lookup that ref in loader.refs registry.
    
    The first bug is that loader.refs registry needs to be looked up only
    with base part of the ref, because that's how loader.refs is
    initialized:
    
        https://github.com/json-editor/json-editor/blob/06f0117aa166f3fca095f4554fba22c83f1f38cb/src/schemaloader.js#L375-L378
        https://github.com/json-editor/json-editor/blob/06f0117aa166f3fca095f4554fba22c83f1f38cb/src/schemaloader.js#L283-L292
        https://github.com/json-editor/json-editor/blob/06f0117aa166f3fca095f4554fba22c83f1f38cb/src/schemaloader.js#L465
    
    The second bug is that the decision whether to dereference JSON Pointer
    part of the URI was being taken based only on the original form of the
    ref. In this case it was leading to main.json#/definitions/tcpv4port not
    being followed to /definitions/tcpv4port even after schema of main.json
    becomes correctly looked up with change of ref to refBase.
    
    -> Fix both bugs:
    
    1) lookup loader.refs only with base part of a ref;
    2) after retriving looked up schema, also dereference it via
       JSON-Pointer, if that part is present in looked up ref.
    
    Fixes: json-editor#1384
    navytux committed Sep 4, 2023
    Configuration menu
    Copy the full SHA
    3647b7b View commit details
    Browse the repository at this point in the history
  3. fixup! schemaloader: Fix expandRefs not to crash if $ref'ed schema al…

    …so has another $ref to self internally
    
    Testing via CodecepJS and reference.html revealed a bug in my second
    patch: if the ref is of the form
    
        schema.json#/json/pointer1#/json/pointer2
    
    we need to load schema.json, dereference pointer1 and then dereference
    pointer2 on the result.
    
    Original code was doing this second step, but my patch did not, and so
    it was breaking as e.g.:
    
    https://github.com/json-editor/json-editor/actions/runs/6074053135/job/16477372582#step:6:456
    navytux committed Sep 4, 2023
    Configuration menu
    Copy the full SHA
    e4e61f0 View commit details
    Browse the repository at this point in the history