Skip to content

fix: validate scene base against parcels - #60

Merged
LautaroPetaccio merged 7 commits into
mainfrom
fix/scene-base-integrity
Aug 4, 2026
Merged

fix: validate scene base against parcels#60
LautaroPetaccio merged 7 commits into
mainfrom
fix/scene-base-integrity

Conversation

@LautaroPetaccio

Copy link
Copy Markdown
Contributor

Summary

  • run the complete SceneParcels validation during scene metadata validation
  • reject bases outside the declared parcel set
  • verify pointers and scene parcels remain canonically equivalent
  • add regression coverage and a content-validator changeset

Verification

  • build and lint passed
  • content-validator: 533 tests passed
  • diff checks passed

@LautaroPetaccio

Copy link
Copy Markdown
Contributor Author

@decentraland-bot decentraland-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the hardening pass. The core invariant is in the right layer: metadata schema validation can validate scene, and content-validator can compare it with deployment pointers before access checks run. I found one current blocker and a few follow-ups worth addressing.

Findings

  • P1 — CI is currently failing on dependency installation (libs/content-validator/package.json:34, pnpm-lock.yaml): build-and-test fails before build/tests because @dcl/schemas@27.0.0 is still inside the repository minimumReleaseAge window. Until CI can install reproducibly and run the test suite, this PR is not mergeable. Please wait for the policy window to pass or use the repo's approved dependency-review path rather than bypassing the supply-chain check.

  • P1 — Patch release includes a major @dcl/schemas runtime/type dependency upgrade (.changeset/secure-scene-base-parcel.md:2, libs/content-validator/package.json:34): @dcl/content-validator is a published package and code search shows decentraland/catalyst consumes it. Moving from @dcl/schemas ^26.2.0 to ^27.0.0 changes the metadata validator used for all entity metadata and also affects schema-derived public types exported through this package. If schemas 27 is required, please document this in the changeset/release notes and consider whether a patch release is enough; otherwise, implement the specific SceneParcels invariant locally until consumers are ready for the schemas major.

  • P2 — New scene validation is ungated for historical deployments (libs/content-validator/src/validations/scene.ts:118-120): nearby scene hardenings are timestamp/ADR-gated, but this invariant now applies to every scene that reaches sceneValidateFn. If old/backfilled scenes with legacy metadata can be revalidated during sync, upgraded nodes may reject deployments older nodes accepted. If this is intentional protocol hardening, please document the rollout/backfill expectation and add a regression test for the intended pre-activation behavior.

  • P2 — Error message hides non-base schema failures (libs/content-validator/src/validations/scene.ts:86-99): SceneParcels.validate(scene) and the synthetic pointer validation can fail for malformed coordinates, duplicates, empty parcels, or non-canonical values, but the metadata branch always reports only base membership. A broader message such as “scene parcels metadata must be valid, canonical, unique, and include the base parcel” would make operator/client remediation clearer.

Consumer impact

This is a public package behavior change. I checked repository references for @dcl/content-validator; the visible downstream consumer is decentraland/catalyst. The validation change is intentionally stricter, so rollout ordering across Catalyst/content services should be coordinated with the linked hardening set to avoid mixed-version acceptance differences.

Security

No direct security vulnerabilities found in the new validation logic. The package-age CI failure is a supply-chain control blocking a newly published dependency, so I am treating the current failed install as merge-blocking.

CI

build-and-test is failing at pnpm install --frozen-lockfile due @dcl/schemas@27.0.0 minimum release age.


Reviewed by Jarvis 🤖 · Requested by Lautaro Petaccio (<@U025WCHLMN3>) via Slack

@LautaroPetaccio

Copy link
Copy Markdown
Contributor Author

Addressed in a34ebdb:\n\n- Changed the @dcl/content-validator changeset from patch to major and documented the @dcl/schemas 27 public runtime/type dependency.\n- Documented the rollout/backfill expectation: malformed historical scene identities are rejected and must be handled or quarantined by consumers.\n- Replaced the base-only error with a broader scene-parcel metadata diagnostic.\n- Added a regression test proving that an invalid pre-ADR-45 scene is rejected.\n\nIntentionally not changed:\n\n- The scene identity invariant remains timestamp/ADR ungated. This is an authorization and cross-tenant identity boundary, so allowing old or replayed deployments to bypass it would preserve the vulnerable path. The entity timestamp is attacker-controlled signed data and is not an appropriate switch for this check.\n- The implementation continues to use @dcl/schemas 27 rather than duplicating SceneParcels rules locally. The major content-validator release ensures consumers opt into that dependency and behavior explicitly.

@decentraland-bot decentraland-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the updates. The previous blockers are resolved: CI now installs @dcl/schemas@27.0.0 successfully, the content-validator changeset is major, the intentional timestamp-independent replay behavior is documented in the changeset and covered by regression tests, and the validation error is broader.

Findings

  • P2 — README still advertises legacy compatibility without calling out the new intentional replay rejection (libs/content-validator/README.md:42, libs/content-validator/README.md:55): the changeset now correctly states that historical/backfilled deployments with invalid scene identity are rejected, but the README still says the package maintains backwards compatibility with legacy content migrations. Since this is a major release and the changeset documents the migration contract, this is not blocking; consider updating the README before or soon after release so Catalyst/content-service consumers do not miss the behavior change.

Consumer impact

This remains a public package behavior change for @dcl/content-validator, but it is now correctly represented as a major release and the changeset explicitly documents the @dcl/schemas@27 dependency plus the historical replay/quarantine expectation. The linked coordinated hardening set should still be used to control rollout order.

Security

No security issues found in the new validation logic.

Verification

  • GitHub build-and-test: passing
  • Local corepack pnpm --filter @dcl/content-validator test: 43 suites / 536 tests passing

Approved.


Reviewed by Jarvis 🤖 · Requested by Lautaro Petaccio (<@U025WCHLMN3>) via Slack

@LautaroPetaccio
LautaroPetaccio merged commit d9a9e97 into main Aug 4, 2026
1 check passed
@LautaroPetaccio
LautaroPetaccio deleted the fix/scene-base-integrity branch August 4, 2026 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants