FE-523: Sanitize user-controlled keys in the simulation path - #9222
Draft
kube wants to merge 1 commit into
Draft
Conversation
Reject net identifiers that collide with Object.prototype member names at the file-import and simulation boundaries, build every record keyed by user-authored strings without a prototype, guard artifact and marking reads with own-property lookups, bind compiled-program parameters as frozen prototype-free copies, and give place visualizer code the same sandbox hardening as scenario code.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
| * `emit-buffer-js.ts`), so these sources mirror that shape directly. | ||
| */ | ||
| const lambdaSourceReading = (name: string): string => | ||
| `(f64, u64, u8, placeBases, indices) => __params[${JSON.stringify(name)}]`; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🌟 What is the purpose of this PR?
Closes the four CodeQL
js/remote-property-injectionalerts from FE-523 and the wider class they belong to. Every identity string in a net (place, transition, colour, differential-equation, metric and scenario ids, parameter variable names, colour element names) comes from an imported.petrinautfile, whose schema accepts any string. Used as plain-object keys, names like__proto__andconstructorcorrupt the records they touch: a__proto__write with an object value replaces the record's prototype instead of storing the entry, and a read of a missingconstructorkey returns an inherited function that defeats?? fallbackandif (!entry)guards.Two concrete failures before this change:
artifacts.lambdas["__proto__"] = {...}in the HIR compiler dropped the artifact while the fingerprint check still passed, so the simulation started with a transition whose lambda silently did not exist; a token field namedtoStringdefeated the?? 0default in the packed-token encoder and producedNaNtoken bytes.The fix is layered rather than the single
Object.create(null)the ticket suggests, because no single layer covers every path:findDangerousSdcpnKeyswalks a net and reports every identifier on theDANGEROUS_RECORD_KEYSlist (Object.prototypemember names plusprototype).parseSDCPNFilerejects such files with the offending id named;buildSimulationrepeats the check for nets supplied programmatically by embedders. The editor validators (variableNameSchema, colour element and scenario identifier schemas) now reject them at entry;constructorwas the one all-lowercase name they admitted.createUserKeyedRecord(Object.create(null)) is used at every site that builds a record keyed by these strings: HIR artifact records, engine frame snapshots, token records, parameter values, scenario accumulators, layout positions, experiment state. Rejection alone misses keys that never pass a schema: scenario code-mode keys come from whatever object the user's code returns.getOwn(own-property reads) at every lookup on records that crossedstructuredCloneor JSON, since both revive plain objects and a null prototype does not survive a worker hop. Containment alone does not survive serialization.Separately, the review found that place visualizer code (
place.visualizerCode) ran throughnew Functionwith no hardening at all: not strict mode, no shadowed globals, no constructor masking, reachable by opening an imported file and viewing a place. It now gets the same sandbox treatment as scenario code, at module evaluation and at each render. Dynamics, lambdas, kernels and metrics were already fine: they compile through the HIR, whose emitter quotes every embedded key withJSON.stringify, and the__paramsbinding is now a frozen prototype-free copy so a hostile parameter name cannot readObject.prototypemembers from inside compiled code.🔗 Related links
MutationProvider, which this PR's schema tightening feeds into🚫 Blocked by
Nothing.
🔍 What does this change?
validation/record-keys.tsin@hashintel/petrinaut-core:DANGEROUS_RECORD_KEYS,isDangerousRecordKey,createUserKeyedRecord,getOwn,findDangerousSdcpnKeys,describeDangerousSdcpnKeys. Exported from the package index.parseSDCPNFilerejects nets whose identifiers collide withObject.prototypemember names (versioned and legacy formats);buildSimulationthrows the same error for programmatic input.compileHirArtifactssub-records,buildSimulationplace/transition states,toSnapshot()ininternal-frame.ts,coerceTokenRecord/decodeTokenRecord/readTokenRecord,deriveDefaultParameterValues/mergeParameterValues(the FE-523 alert sites), scenario compiler accumulators,flattenComponentInstancesvalues, monte-carlolatestByMetricId, actual-mode markings, ELK layout positions, and four sites in thepetrinautUI package.getOwn) for HIR artifact lookups (build-simulation.ts,compiled-model.ts, experiments provider), initial marking values, and token-encoder defaults.instantiate.tsbinds__paramsas a frozen copy with no prototype. Metric evaluators keep their live rebinding contract; their record is prototype-free at construction instead (hir-metric.ts).id === "__proto__"throws increateEngineFrameLayoutgeneralise toisDangerousRecordKey.variableNameSchema,colorElementSchema.nameandscenarioParameterSchema.identifierreject reserved property names.compile-visualizer.tsruns the compiled module in strict mode withSHADOWED_GLOBALSshadowed and wraps both module evaluation and each render inrunSandboxed.core.simulation.engine, with a reject/contain/guard lanes diagram and a three-entry-paths sequence diagram; thecore.simulation.authoringuser-code page documents the visualizer sandbox; thecore.validationlayer page documentsrecord-keys.ts.petri-net-extensions.md.Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR:
constructorkeep simulating, since every record on that path now stores it as an own property.Object.create(null)as a sanitizer for the four existing alerts. If they do not auto-close, they can be dismissed pointing atrecord-keys.ts: the flagged writes now target prototype-free records and the keys are rejected at both boundaries.🐾 Next steps
MutationProviderand surfacing pre-existing invalid names in the Diagnostics tab.🛡 What tests cover this?
validation/record-keys.test.ts: the key list, prototype-free construction, own-property reads (including own__proto__keys revived byJSON.parse), and the walk across every entity kind and subnets.hir/instantiate.test.ts: a parameter namedconstructorreads its own value; a missingtoStringparameter readsundefined; compiled code cannot mutate the caller's record.parameter-values.test.ts: the FE-523 alert sites with hostile names, plus prototype-free results.file-format/parse-sdcpn-file.test.ts: hostile ids and element names rejected in versioned and legacy formats.simulation/engine/build-simulation.test.ts: the simulation boundary throws on a hostile transition id; own-key initial marking iteration.ui/lib/compile-visualizer.test.ts: globals shadowed for module and component bodies, constructor-chain escape blocked at compile and at render, strict mode enforced.optimization.test.ts: updated: a reserved-name scenario parameter is now rejected at the model boundary, before the binding check it previously exercised.❓ How to test this?
yarn devinlibs/@hashintel/petrinaut)..petrinautfile with a transition id ofconstructor(edit any exported file by hand). The import fails with an error naming the id.constructoras its variable name. The properties panel rejects it.export default Visualization(() => <div>{String(typeof fetch)}</div>)and view the place: it rendersundefined.📹 Demo
Not applicable: error paths and internal containment, covered by the tests above.