fix: validate scene base against parcels - #60
Conversation
|
Coordinated scene-integrity hardening set:
Please review merge and deployment ordering across this set before rollout. |
decentraland-bot
left a comment
There was a problem hiding this comment.
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-testfails before build/tests because@dcl/schemas@27.0.0is still inside the repositoryminimumReleaseAgewindow. 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/schemasruntime/type dependency upgrade (.changeset/secure-scene-base-parcel.md:2,libs/content-validator/package.json:34):@dcl/content-validatoris a published package and code search showsdecentraland/catalystconsumes it. Moving from@dcl/schemas ^26.2.0to^27.0.0changes 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 reachessceneValidateFn. 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
|
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
left a comment
There was a problem hiding this comment.
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
Summary
Verification