diff --git a/apps/oxlint/src-js/plugins/load.ts b/apps/oxlint/src-js/plugins/load.ts index 6b64d11dd2b42..99a1271df8417 100644 --- a/apps/oxlint/src-js/plugins/load.ts +++ b/apps/oxlint/src-js/plugins/load.ts @@ -167,6 +167,9 @@ export function registerPlugin(plugin: Plugin, packageName: string | null): Plug if (!isArray(inputDefaultOptions)) { throw new TypeError("`rule.meta.defaultOptions` must be an array if provided"); } + // TODO: This isn't quite safe, as `defaultOptions` isn't from JSON, and `deepFreezeJsonArray` + // assumes it is. We should perform options merging on Rust side instead, and also validate + // `defaultOptions` against options schema. deepFreezeJsonArray(inputDefaultOptions); defaultOptions = inputDefaultOptions; } diff --git a/apps/oxlint/src-js/plugins/options.ts b/apps/oxlint/src-js/plugins/options.ts index 98912cf699c1f..3a7516eac74ec 100644 --- a/apps/oxlint/src-js/plugins/options.ts +++ b/apps/oxlint/src-js/plugins/options.ts @@ -174,9 +174,19 @@ function mergeValues(configValue: JsonValue, defaultValue: JsonValue): JsonValue const merged = { ...defaultValue, ...configValue }; // Symbol properties are not possible in JSON, so no need to handle them here. - // All properties are enumerable own properties, so can use simple `for..in` loop. - for (const key in configValue) { - if (key in defaultValue) { + // + // `defaultValue` is not from JSON, so we can't use a simple `for..in` loop over `defaultValue`. + // That would also pick up enumerable properties from prototype of `defaultValue`. + // `configValue` *is* from JSON, so simple `key in configValue` check is fine. + // + // A malicious plugin could potentially get up to mischief here (prototype pollution?) if `defaultValue` is a `Proxy`. + // But plugins are executable code, so they have far easier ways to do that. No point in defending against it here. + for (const key of Object.keys(defaultValue)) { + if (key in configValue) { + // `key` is an own property of both `configValue` and `defaultValue`, so must be an own property of `merged` too. + // Therefore, we don't need special handling for if `key` is `"__proto__"`. + // All the property reads and writes here will affect only the owned properties of these objects, + // (including if those properties are named `"__proto__"`). merged[key] = mergeValues(configValue[key], defaultValue[key]); } else { deepFreezeValue(configValue[key]);