-
Notifications
You must be signed in to change notification settings - Fork 10
fix(config): deep merge agent overrides with reusable deepMerge utility
#27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
c84fa96
d943995
c3e9404
f29478a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| const DANGEROUS_KEYS = new Set(["__proto__", "constructor", "prototype"]); | ||
| const MAX_DEPTH = 50; | ||
|
|
||
| function isPlainObject(value: unknown): value is Record<string, unknown> { | ||
| return ( | ||
| typeof value === "object" && | ||
| value !== null && | ||
| !Array.isArray(value) && | ||
| Object.prototype.toString.call(value) === "[object Object]" | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Deep merges two objects, with override values taking precedence. | ||
| * - Objects are recursively merged | ||
| * - Arrays are replaced (not concatenated) | ||
| * - undefined values in override do not overwrite base values | ||
| * | ||
| * @example | ||
| * deepMerge({ a: 1, b: { c: 2, d: 3 } }, { b: { c: 10 }, e: 5 }) | ||
| * // => { a: 1, b: { c: 10, d: 3 }, e: 5 } | ||
| */ | ||
| export function deepMerge<T extends Record<string, unknown>>(base: T, override: Partial<T>, depth?: number): T; | ||
| export function deepMerge<T extends Record<string, unknown>>(base: T | undefined, override: T | undefined, depth?: number): T | undefined; | ||
| export function deepMerge<T extends Record<string, unknown>>( | ||
| base: T | undefined, | ||
| override: T | undefined, | ||
| depth = 0 | ||
| ): T | undefined { | ||
| if (!base && !override) return undefined; | ||
| if (!base) return override; | ||
| if (!override) return base; | ||
| if (depth > MAX_DEPTH) return override ?? base; | ||
|
|
||
| const result = { ...base } as Record<string, unknown>; | ||
|
|
||
| for (const key of Object.keys(override)) { | ||
| if (DANGEROUS_KEYS.has(key)) continue; | ||
|
|
||
| const baseValue = base[key]; | ||
| const overrideValue = override[key]; | ||
|
|
||
| if (overrideValue === undefined) continue; | ||
|
|
||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Recommended: Stack Overflow ProtectionRecursive calls without depth limit could cause stack overflow with deeply nested malicious configs. Suggested improvement: export function deepMerge<T extends Record<string, unknown>>(
base: T | undefined,
override: T | undefined,
depth = 0 // Add depth tracking
): T | undefined {
if (depth > 50) {
// Reasonable limit for config objects
return override ?? base;
}
// ...
result[key] = deepMerge(baseValue, overrideValue, depth + 1);
}This is optional but recommended for defense-in-depth.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in c3e9404 ✅ Added |
||
| if (isPlainObject(baseValue) && isPlainObject(overrideValue)) { | ||
| result[key] = deepMerge(baseValue, overrideValue, depth + 1); | ||
| } else { | ||
| result[key] = overrideValue; | ||
| } | ||
| } | ||
|
|
||
| return result as T; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Critical: Prototype Pollution Vulnerability
This loop doesn't filter dangerous keys. If a malicious project config contains
__proto__,constructor, orprototypekeys, it can polluteObject.prototypeand affect all objects in the runtime.Attack scenario:
Suggested fix:
This is a 1-line addition that prevents a critical security vulnerability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in c3e9404 ✅
Added
DANGEROUS_KEYSset to filter__proto__,constructor,prototypekeys.