Fix SchemaLoader crashes when handling $refs #1385
Conversation
… 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
…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
|
Ok, I missed to run codecept tests - I only ran |
…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
|
@schmunk42, I fixed, at least locally, the codecept/references.html issue in e4e61f0 . Would you please trigger the test to run again? Thanks. |
|
LGTM. @germanbisurgi Please have a look! |
|
@schmunk42, thanks for running the tests and LGTM. |
|
LGTM too, a test would be a nice addition IMO |
|
@germanbisurgi, thanks for feedback and LGTM. Could you please clarify what you mean when talking about test addition? Both changes come with their own test, and also existing test in references.html covers changes in the second patch as well. |
|
Sorry i meant an e2e test but probably the .spec test would be enough, WDYT @schmunk42 ? |
|
Since the schemaloader is the most sensible and complex (for me at least) part of the library, i would feel better if we can have those test. I would gladly submit a PR for that. |
|
I submitted a pull request #1386 that includes end-to-end tests for addressing issues #1383 and #1384. After that, I merged it with navytux's pull request #1386 to evaluate the effectiveness of the fix. Issue 1383 appears to have been resolved successfully, but issue 1384 still breaks. I suspect that the root cause of the problem is related to the way the ref definition is stored in In both scenarios, the mentioned schemas include 'default' keywords, indicating that the editor will generate a new instance. This is what prompts the initiation of the new "ref lookup." 
|
|
@schmunk42, @germanbisurgi, thanks for feedback and for #1386. I agree it indeed makes sense to have this more thoroughly tested and, it seems, more thoroughly fixed. I will try to look into it in some time. Not right now, due to pressure on other topics, but hopefully not in distant future. Kirill |
|
@germanbisurgi Please merge, unless you think something is still needed. |
|
( I'm pausing this on my side since we use json-schema-ref-parser and the editor gets already dereferenced schema ) |
Hello up there,
Please find a couple of fixes for SchemaLoader:
$ref'edschema contains entries with null values.$refsin externally$ref'edschema are handled.Please see individual patches for details.
/cc @schmunk42, @germanbisurgi, @robocoder, @cybtachyon, @churel
Thanks beforehand,
Kirill