Skip to content

Commit ab4deb0

Browse files
committed
fix(linter/plugins): improve safety of options merging (#16549)
Fix for an edge case. Code for merging options was assuming that `defaultOptions` is a plain object with standard prototype, but that assumption doesn't actually hold. Alter the merging logic to ignore properties on prototype of objects in `defaultOptions`.
1 parent 4d94438 commit ab4deb0

File tree

2 files changed

+16
-3
lines changed

2 files changed

+16
-3
lines changed

apps/oxlint/src-js/plugins/load.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,9 @@ export function registerPlugin(plugin: Plugin, packageName: string | null): Plug
167167
if (!isArray(inputDefaultOptions)) {
168168
throw new TypeError("`rule.meta.defaultOptions` must be an array if provided");
169169
}
170+
// TODO: This isn't quite safe, as `defaultOptions` isn't from JSON, and `deepFreezeJsonArray`
171+
// assumes it is. We should perform options merging on Rust side instead, and also validate
172+
// `defaultOptions` against options schema.
170173
deepFreezeJsonArray(inputDefaultOptions);
171174
defaultOptions = inputDefaultOptions;
172175
}

apps/oxlint/src-js/plugins/options.ts

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -174,9 +174,19 @@ function mergeValues(configValue: JsonValue, defaultValue: JsonValue): JsonValue
174174
const merged = { ...defaultValue, ...configValue };
175175

176176
// Symbol properties are not possible in JSON, so no need to handle them here.
177-
// All properties are enumerable own properties, so can use simple `for..in` loop.
178-
for (const key in configValue) {
179-
if (key in defaultValue) {
177+
//
178+
// `defaultValue` is not from JSON, so we can't use a simple `for..in` loop over `defaultValue`.
179+
// That would also pick up enumerable properties from prototype of `defaultValue`.
180+
// `configValue` *is* from JSON, so simple `key in configValue` check is fine.
181+
//
182+
// A malicious plugin could potentially get up to mischief here (prototype pollution?) if `defaultValue` is a `Proxy`.
183+
// But plugins are executable code, so they have far easier ways to do that. No point in defending against it here.
184+
for (const key of Object.keys(defaultValue)) {
185+
if (key in configValue) {
186+
// `key` is an own property of both `configValue` and `defaultValue`, so must be an own property of `merged` too.
187+
// Therefore, we don't need special handling for if `key` is `"__proto__"`.
188+
// All the property reads and writes here will affect only the owned properties of these objects,
189+
// (including if those properties are named `"__proto__"`).
180190
merged[key] = mergeValues(configValue[key], defaultValue[key]);
181191
} else {
182192
deepFreezeValue(configValue[key]);

0 commit comments

Comments
 (0)