Skip to content

Restrict disallow on Draft 3 canonicalizer in more cases #875

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.

Sign up for GitHub

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

Merged
jviotti merged 3 commits into from
Jun 19, 2026
+189 −70
Merged

Restrict disallow on Draft 3 canonicalizer in more cases #875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1555e47
Restrict `disallow` on Draft 3 canonicalizer in more cases
jviotti Jun 19, 2026
fe49b69
Fix
jviotti Jun 19, 2026
1efde88
Fix
jviotti Jun 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 71 additions & 47 deletions schemas/canonical-draft3.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,56 @@
}
}
},
"schema": {
"extends": {
"x-lint-exclude": "simple_properties_identifiers",
"type": "object",
"allOf": [
{
"$ref": "#/$defs/metadata"
},
{
"$ref": "#/$defs/core"
}
],
"required": [ "extends" ],
"properties": {
"extends": {
"type": "array",
"minItems": 1,
"items": {
"$ref": "#/$defs/schema"
}
},
"required": {
"type": "boolean"
}
},
"unevaluatedProperties": false
},
"union": {
"x-lint-exclude": "simple_properties_identifiers",
"type": "object",
"allOf": [
{
"$ref": "#/$defs/metadata"
},
{
"$ref": "#/$defs/core"
}
],
"required": [ "type" ],
"properties": {
"type": {
"type": "array",
"minItems": 1,
"items": {
"$ref": "#/$defs/schema"
}
}
},
"unevaluatedProperties": false
},
"leaf": {
"anyOf": [
{
"const": {}
Expand Down Expand Up @@ -336,55 +385,19 @@
}
},
"unevaluatedProperties": false
}
]
},
"schema": {
"anyOf": [
{
"$ref": "#/$defs/leaf"
},
{
"x-lint-exclude": "simple_properties_identifiers",
"type": "object",
"allOf": [
{
"$ref": "#/$defs/metadata"
},
{
"$ref": "#/$defs/core"
}
],
"required": [ "type" ],
"properties": {
"type": {
"type": "array",
"minItems": 1,
"items": {
"$ref": "#/$defs/schema"
}
}
},
"unevaluatedProperties": false
"$ref": "#/$defs/union"
},
{
"x-lint-exclude": "simple_properties_identifiers",
"type": "object",
"allOf": [
{
"$ref": "#/$defs/metadata"
},
{
"$ref": "#/$defs/core"
}
],
"required": [ "extends" ],
"properties": {
"extends": {
"type": "array",
"minItems": 1,
"items": {
"$ref": "#/$defs/schema"
}
},
"required": {
"type": "boolean"
}
},
"unevaluatedProperties": false
"$ref": "#/$defs/extends"
},
{
"x-lint-exclude": "simple_properties_identifiers",
Expand All @@ -404,7 +417,18 @@
"maxItems": 1,
"minItems": 1,
"items": {
"$ref": "#/$defs/schema"
"$comment": "TODO: `disallow` should only ever wrap a single-kind leaf. We additionally permit `extends` and `type` unions here as an interim escape hatch: negating a conjunction or disjunction whose wrapper is targeted by a `$ref` cannot be pushed to the leaves, because doing so would dissolve the referenced node. The proper fix is to invert such references so the `disallow` wraps a `$ref` leaf instead, after which both can be dropped from this list",
"anyOf": [
{
"$ref": "#/$defs/leaf"
},
{
"$ref": "#/$defs/union"
},
{
"$ref": "#/$defs/extends"
}
]
}
}
},
Expand Down
35 changes: 33 additions & 2 deletions src/alterschema/canonicalizer/disallow_double_negation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,22 @@ class DisallowDoubleNegation final : public SchemaTransformRule {
ONLY_CONTINUE_IF(
wraps_single_constraint(schema, "disallow", walker, vocabularies));

ONLY_CONTINUE_IF(!frame.has_references_through(
location.pointer, WeakPointer::Token{std::cref(KEYWORD)}));
// Collapsing the chain dissolves every intermediate `disallow` wrapper, so
// a reference targeting any of them has no valid new home: bail. A
// reference into the surviving innermost schema relocates with it (handled
// by `rereference`)
auto wrapper{location.pointer};
const sourcemeta::core::JSON *node{&disallow->at(0)};
while (is_single_negation(*node)) {
Comment thread
jviotti marked this conversation as resolved.
wrapper.push_back(std::cref(KEYWORD));
wrapper.push_back(static_cast<std::size_t>(0));
if (frame.has_references_to(wrapper)) {
return false;
}

node = &node->at(KEYWORD).at(0);
}

return true;
}

Expand All @@ -55,6 +69,23 @@ class DisallowDoubleNegation final : public SchemaTransformRule {
}
}

[[nodiscard]] auto rereference(const std::string_view, const Pointer &,
const Pointer &target,
const Pointer &current) const
-> Pointer override {
auto old_prefix{current.concat({"disallow", 0, "disallow", 0})};
while (
target.starts_with(old_prefix.concat({"disallow", 0, "disallow", 0}))) {
old_prefix = old_prefix.concat({"disallow", 0, "disallow", 0});
}

if (!target.starts_with(old_prefix)) {
return target;
}

return target.rebase(old_prefix, current);

@augmentcode augmentcode Bot Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

src/alterschema/canonicalizer/disallow_double_negation.h:67-78: rereference() rebases anything under current/disallow/0/disallow/0..., but if the transform collapses multiple pairs (e.g. quadruple negation), a $ref that targets an intermediate single-negation wrapper like .../disallow/0 would be rebased to current/disallow/0 (which may not exist / may change semantics). Consider whether such intermediate-wrapper destinations should instead make the rule bail (or otherwise be treated as an un-rereferenceable broken reference).

Severity: medium

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

}

private:
static auto is_single_negation(const sourcemeta::core::JSON &schema) -> bool {
return schema.is_object() && schema.size() == 1 &&
Expand Down
106 changes: 85 additions & 21 deletions test/alterschema/alterschema_canonicalize_draft3_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1456,7 +1456,7 @@ TEST_F(CanonicalizerDraft3Test, disallow_quadruple_negation) {
}

TEST_F(CanonicalizerDraft3Test,
disallow_double_negation_with_reference_into_subtree_not_eliminated) {
disallow_double_negation_with_reference_into_subtree_rereferenced) {
auto document = sourcemeta::core::parse_json(R"JSON({
"$schema": "http://json-schema.org/draft-03/schema#",
"type": "object",
Expand Down Expand Up @@ -1490,34 +1490,26 @@ TEST_F(CanonicalizerDraft3Test,
"a": {
"extends": [
{
"disallow": [
{
"disallow": [
"type": "object",
"properties": {
"x": {
"extends": [
{
"type": "object",
"properties": {
"x": {
"extends": [
{
"type": "string",
"minLength": 0
}
],
"required": false
}
},
"patternProperties": {},
"additionalProperties": {}
"type": "string",
"minLength": 0
}
]
],
"required": false
}
]
},
"patternProperties": {},
"additionalProperties": {}
}
],
"required": false
},
"b": {
"$ref": "#/properties/a/extends/0/disallow/0/disallow/0/properties/x"
"$ref": "#/properties/a/extends/0/properties/x"
}
},
"patternProperties": {},
Expand Down Expand Up @@ -1651,6 +1643,78 @@ TEST_F(CanonicalizerDraft3Test,
CANONICALIZE_AND_VALIDATE(document, expected, *compiled_meta_);
}

TEST_F(CanonicalizerDraft3Test,
disallow_double_negation_deep_with_reference_into_subtree_rereferenced) {
auto document = sourcemeta::core::parse_json(R"JSON({
"$schema": "http://json-schema.org/draft-03/schema#",
"type": "object",
"properties": {
"a": {
"disallow": [
{
"disallow": [
{
"disallow": [
{
"disallow": [
{
"type": "object",
"properties": {
"x": {
"type": "string"
}
}
}
]
}
]
}
]
}
]
},
"b": {
"$ref": "#/properties/a/disallow/0/disallow/0/disallow/0/disallow/0/properties/x"
}
}
})JSON");

const auto expected = sourcemeta::core::parse_json(R"JSON({
"$schema": "http://json-schema.org/draft-03/schema#",
"type": "object",
"properties": {
"a": {
"extends": [
{
"type": "object",
"properties": {
"x": {
"extends": [
{
"type": "string",
"minLength": 0
}
],
"required": false
}
},
"patternProperties": {},
"additionalProperties": {}
}
],
"required": false
},
"b": {
"$ref": "#/properties/a/extends/0/properties/x"
}
},
"patternProperties": {},
"additionalProperties": {}
})JSON");

CANONICALIZE_AND_VALIDATE(document, expected, *compiled_meta_);
}

TEST_F(CanonicalizerDraft3Test, disallow_double_negation_over_type_union) {
auto document = sourcemeta::core::parse_json(R"JSON({
"$schema": "http://json-schema.org/draft-03/schema#",
Expand Down
Loading