Skip to content

feat(validation): add opt-in strict project data validation - #380

Open
han4wluc wants to merge 1 commit into
mainfrom
feat/strict-validation-reland
Open

han4wluc wants to merge 1 commit into
mainfrom
feat/strict-validation-reland

Conversation

@han4wluc

Copy link
Copy Markdown
Contributor

Reland of #378. #378 was merged by mistake before review and reverted in #379. This branch reverts the revert on top of current main (which now includes #377), so the diff is byte-for-byte the same as #378. It still needs the review that #378 never got.

Re-run on this branch: bun run test (101 files, 2,488 passed), bun run lint, bun run check:test-markers, bun run format:changed, and node esbuild.js.


Problem

The engine accepts invalid project data and either falls back silently or fails later, when the affected line runs:

  • A missing screen renders at 1920x1080 (createBackgroundColorElement and the background centering in src/stores/constructRenderState.js). Zero, negative, fractional, NaN and string sizes are accepted, as are unknown top-level and screen keys.
  • An unknown initialLineId puts the read pointer on a line that does not exist (createInitialState in src/stores/system.store.js). An unknown initialSceneId or initialSectionId throws an uncoded TypeError, and a line without an id is accepted.
  • Unknown action names in a story line are skipped without an error when the line runs. dispatchStoreAction looks up the action with a truthy member lookup (if (!_systemStore[actionType]) return;), so authored keys reach store members that are not actions. toString and selectRenderState are invoked, appendPendingEffect queues an effect, and __proto__ throws "is not a function", but only when the line is entered. null and string action payloads are accepted.
  • Unpaired UTF-16 surrogates are accepted in any string or key. They do not survive UTF-8 transport (encoders replace them with U+FFFD), so persisted and exported data can no longer match the project.
  • Reserved keys are accepted anywhere. A __proto__ keyboard binding (valid JSON) is dropped: toRenderStateKeyboard assigns renderStateKeyboard[key] = ..., which sets the object's prototype instead of adding a binding, and when another binding is present, the render-state keyboard object ends up with a non-standard prototype. In the hotkeys-js version route-graphics ships, a constructor binding resolves to an inherited member of the key-code tables, so it can never fire. The engine already treats __proto__, constructor and prototype as reserved elsewhere: getValueByPath (src/util.js) and assertSafeMapId (src/l10n.js) refuse all three, and validateComputedVariableConfigs (src/util.js) refuses a __proto__ variable ID.

Tools that need one answer to "is this project valid?" cannot get it today: authoring-tool exports, CI checks of project files, and other runtimes that must agree with this engine.

Design

  • New option createRouteEngine({ handlePendingEffects, validation: "strict" }). Leaving it out, or passing "default", keeps today's behavior. Any other value throws a TypeError at creation, so a typo cannot silently turn strict mode off. The option sits beside randomSource because it applies to every init and updateProjectData of the engine.
  • In strict mode, init validates the canonical initialState.projectData (before L10n resolution) and scans initialState.l10nData. updateProjectData validates payload.projectData in the same way. Both checks run on the structuredClone the engine already makes, and before any state changes, so a failed strict init or update leaves the running generation untouched, as other failed initializations already do.
  • A failure throws an Error with two fields that hosts can rely on: code and path. path is an RFC 6901 JSON Pointer into initialState or into the updateProjectData payload, for example /projectData/story/scenes/s/sections/a/lines/1/actions/teleport. This follows the existing routing_cycle convention: identify the failure by code, not by message text.
  • Codes:
    • lone_surrogate: an unpaired surrogate in a string value or an object key.
    • reserved_key: an object key __proto__, constructor or prototype, anywhere in the document.
    • unknown_field: a top-level key other than config, screen, resources and story, or a screen key other than width, height and backgroundColor.
    • missing_field: a required envelope or story field is absent, such as screen, its size, the story structure, a line id, or the initial scene's initialSectionId.
    • invalid_value: a value has the wrong type or range, such as a screen size that is not a positive integer, a non-object resources, scene, section, line, actions map or action payload, an empty lines array, or a cyclic reference.
    • unknown_reference: initialSceneId, initialSectionId or initialLineId names nothing.
    • unknown_action: an action name outside the authored vocabulary.
  • The authored action vocabulary is the 83 properties of presentationActions.yaml and systemActions.yaml, which are exactly the line action properties of story.yaml. It also includes addViewedLine, addViewedResource and updateProjectData, which docs/RouteEngine.md documents as public but the schemas do not list. Internal store actions and selectors are rejected. A test keeps the list in sync with the schemas.
  • Action names are checked in every story line actions map, including nested conditional branches and weighted random outcomes. Templated action maps in resources are out of scope for this PR (see Follow-ups).
  • Checks run in three passes, and the first failure is thrown:
    1. One walk of the document, in key order, for surrogates and reserved keys. JSON Pointers are built only when a check fails.
    2. The envelope.
    3. The story.
  • Strict mode does not normalize, coerce or repair data. Data it accepts runs exactly as in the default mode.

Compatibility

  • The default mode does not change. Without the option, the only new code that runs is normalizeValidationMode(undefined) and two branches that are never taken.
    • All 2,366 existing tests pass unmodified.
    • A differential run compared v1.48.0 and this branch with validation omitted. It recorded the effects, system state and render state after init and two to four follow-up actions. The results were identical for all 231 VT spec projects and for 9 invalid projects built from the strict-mode cases.
  • Strict mode changes nothing for valid projects. The same harness with validation: "strict" matched v1.48.0 on 230 of the 231 VT projects. The remaining project is one that strict mode rejects (see Tests). The project built from the RouteVN Creator default template initializes in both modes.
  • Passing a validation value other than "default" or "strict" now throws, where it used to be ignored. Older engine versions ignore validation: "strict" entirely, so hosts that depend on strict checks should require the release that adds it.
  • Cost in strict mode: one extra walk per init and updateProjectData. On a synthetic 7.5 MB project with 50,000 lines, that is about 150 ms on Node 24, roughly the cost of the structuredClone the engine already performs. The minified bundle grows by 6.3 KB (1.9 KB gzip).

Tests

New file spec/RouteEngine.strictValidation.test.js (80 tests, synthetic projects only):

  • Option handling: omitted, "default" and "strict" are accepted; "Strict", "lenient", true, null and 1 throw a TypeError.

  • 30 invalid inputs. Each one is rejected in strict mode with the exact code and path, and each one is still accepted by init in the default mode. The inputs:

    • lone high and lone low surrogates, a reversed pair, a surrogate in a key, a key that needs ~0/~1 escaping, and a surrogate in L10n data;
    • __proto__ and constructor keyboard bindings, a prototype key in layout data, and __proto__ as an action name;
    • an unknown top-level key, a missing screen, zero, fractional, string and NaN sizes, and an unknown screen field;
    • non-object resources, an unknown initialLineId, a line without an id, and a section without lines;
    • unknown, inherited (toString), selector and internal-store action names, and unknown names inside a conditional branch and a weighted random outcome;
    • null and string payloads, and a null actions map.
  • The default-mode fallbacks that strict mode replaces:

    • a 1920x1080 background for a missing screen;
    • a pointer to a missing initial line;
    • an unknown action skipped when its line is entered.

    An unknown initial scene and a cyclic reference already throw without a code in the default mode; strict mode reports unknown_reference and invalid_value with a path.

  • Strict mode accepts well-formed surrogate pairs, look-alike keys (Constructor, toString) and shared, non-cyclic references.

  • A failed strict re-initialization keeps the previous generation. A failed strict updateProjectData leaves the state unchanged, and the default mode accepts the same updates.

  • Fixtures:

    • The authored action list equals the schema actions plus the three documented actions, and story.yaml lists exactly the schema actions.
    • All 231 VT spec projects are run through default and strict init, as vt/static/main.js loads them. Every one passes in the default mode, and all but one pass in strict mode. The exception is vt/specs/complete/one.yaml, which still authors nextConfig, an action removed in the v0.1.0 rewrite (8ab56fd) that the default mode skips without an error. The test asserts this rejection exactly.
    • The integration-harness project shape (no scene or section names, a line without actions) is accepted.

Commands, run one after another: bun run lint, bun run check:test-markers, Prettier on the changed files, and bun run test (101 files, 2,446 passed). node esbuild.js builds. VT screenshots were not re-run, because no VT spec and no default-mode behavior changed.

As a one-off check that was not committed, making strict the default left 2,358 of the 2,366 existing tests passing. All 8 failures feed deliberately invalid or internal-only input:

  • 2 tests replace the project with {} or null.
  • 1 test authors updateProjectData with a non-object payload to force a failing state action.
  • 1 test asserts the existing message for an unknown scene-replay initial line, which strict mode reports first as unknown_reference.
  • 4 integration tests author the internal appendPendingEffect in story lines.

Follow-ups (documented, not implemented here)

  1. Unknown fields and payload types everywhere. The schemas already say additionalProperties: false, but at runtime the generated standalone validators (src/generated/l10nPayloadValidators.js, from scripts/generate-l10n-payload-validators.js) only check L10n patches. Generating the projectData and system-action validators the same way and running them in strict mode would also cover numeric resource IDs, string volumes and unknown payload fields. This first needs the schema's required lists reconciled with the runtime defaults:

    • Today 170 of the 231 VT projects fail the full projectData.yaml. The largest groups are scene and section name (307 errors), text-style fontWeight and fontStyle (68 each), and variable scope (8).
    • The already-bundled presentation-action and resource validators alone reject 35 of the 231.
    • The same run found dead fields: dialogue.layoutId, sectionTransition.sceneId, a scenedId typo, endReplay, pushOverlay.resourceType, characters.*.sprites, and numeric dialogue text.
  2. Action names outside story lines. These maps are not yet checked:

    • control keyboard and keyup bindings (resources.yaml around keyboard:, actions:, payload:);
    • layout and control element event payloads;
    • choice item events (presentationActions.yaml, choice items.events);
    • form submitActions and cancelActions;
    • showConfirmDialog.confirmActions and cancelActions;
    • submitForm.actions and cancelForm.actions (systemActions.yaml).

    Some of these maps are templated, so load-time checks must allow template keys. The dispatch-time hole stays open: dispatchStoreAction's truthy lookup (src/RouteEngine.js, if (!_systemStore[actionType]) return;) could reject members that are not own action functions, with unknown_action, in strict mode.

  3. JSON-safe save records. saveSlot builds { formatVersion, slotId, savedAt, image: thumbnailImage, state } (src/stores/system.store.js, saveSlot), and createIndexedDbPersistence stores it by structured clone (store.put(nextRecord) in src/indexedDbPersistence.js). Own-undefined members therefore survive in IndexedDB but disappear in any JSON backend. Examples are image when there is no thumbnail, an unset BGM resource ID, and undefined members of authored objects in contexts. Proposal: specify the slot record as JSON data and, in strict mode, reject undefined and non-finite values when a slot is written, with a path into the slot.

  4. Persisted global state. initialState.global (variables, runtime, saveSlots and the account registries) is not scanned. Lone surrogates and reserved keys can still enter through it.

  5. Reference integrity at load. Section, line, resource and variable targets named by actions (for example jumpToLine, sectionTransition, and updateVariable variableId) are still only checked when the action runs.

  6. Schema gap. addViewedLine, addViewedResource and updateProjectData are documented public actions, and docs/MusicBox.md authors addViewedResource, but systemActions.yaml and story.yaml do not list them. Adding their schemas would let the strict action list come from the schemas alone.

  7. Dead fixture action. vt/specs/complete/one.yaml line 1663 still authors nextConfig. Migrating it to setNextLineConfig would change that VT reference, so this PR leaves it alone and only asserts its strict rejection.

  8. Possible later work.

    • A standalone validateProjectData(projectData, { validation: "strict" }) export for authoring tools.
    • A decision on whether a future major release makes strict the default.

Relands #378, which was merged by mistake and reverted in #379.

This reverts commit 9057a7b.

This branch has not been deployed

No deployments
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.

1 participant