Security: Prototype pollution via untrusted YAML user-config keys

[qdzsh]

Summary

Security: Prototype pollution via untrusted YAML user-config keys

Problem

Severity: Medium | File: src/pkg/utils/yaml.ts:L12

parseUserConfig() copies YAML group names directly into a plain object (ret[groupKey] = groupValue). A malicious userscript can use special keys such as __proto__, constructor, or prototype to mutate object prototypes or interfere with later merges/lookups. Because this parser handles untrusted script metadata, this can become a cross-script integrity issue.

Solution

Create the result object with Object.create(null) and reject dangerous keys (__proto__, constructor, prototype) before assignment. Prefer a schema-validated structure instead of copying arbitrary keys into a plain object.

Changes

• src/pkg/utils/yaml.ts (modified)

Testing

• Existing tests pass
• Manual review completed
• No new warnings/errors introduced

[cyfung1031]

Then why not change it to Map?

Actually the ScriptCat coding was really poorly designed.
Using object without considering the special keys and key order... etc
Also the author did not set the rule for the key ... for example whether #xxxx is allowed.

---

But yeah, the proposed change is good to have.

[cyfung1031]

Suggested change

	if (["__proto__", "constructor", "prototype"].includes(groupKey)) {
	if (Reflect.has(Object.prototype, groupKey)) {

This should be better. This covers valueOf and toString, etc.
prototype is the inherited property for Function not Object, so it is okay. Since it is not a class function, adding the key prototype does not have any negative impact (pollution).

[cyfung1031]

Suggested change

	throw new Error(`UserConfig group "${groupKey}" is not a valid object.`);
	throw new Error(`UserConfig key "${groupKey}" is not valid.`);

The Error message here was wrong.

[qdzsh]

Good points, thanks for the careful review.

• Switched to Reflect.has(Object.prototype, groupKey) as you suggested — it covers valueOf, toString, etc. too, which the hardcoded list missed. And you're right that prototype is harmless here since this isn't a function.
• Fixed the error message — it was wrongly reusing the "is not a valid object" text.

On Map: I considered it, but UserConfig is consumed as a plain object in a few places downstream, so swapping the type would turn a small hardening fix into a wider refactor. I kept it as Object.create(null) to stay minimal and avoid touching unrelated code, but happy to follow up with a Map-based version in a separate PR if you'd prefer that direction.

[cyfung1031]

@CodFrm please final check and merge.

[CodFrm]

Thanks!