[Security]: Fix security vulnerability through __proto__#34929
[Security]: Fix security vulnerability through __proto__#34929benediktjohannes wants to merge 1 commit intoopenclaw:mainfrom
Conversation
|
cc @shakkernerd could you please review, thanks! 👍 |
Greptile SummaryThis PR makes a focused, one-line security hardening change in The fix is correct and well-motivated. The Key points:
Confidence Score: 5/5
Last reviewed commit: 7158c11 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return false; | ||
| } | ||
| return accountId in accounts; | ||
| return Object.hasOwn(accounts, accountId); |
There was a problem hiding this comment.
Consider adding a test for the __proto__ edge case
The rest of the codebase has thorough test coverage for prototype-pollution vectors (e.g. src/config/merge-patch.proto-pollution.test.ts, src/routing/account-id.test.ts, src/config/runtime-overrides.test.ts), but there is no audit-channel.test.ts file and this PR doesn't add one. A small unit test verifying hasExplicitProviderAccountConfig returns false when accountId is "__proto__" (on an object that doesn't genuinely contain that own key) would lock in this behaviour and prevent future regressions, consistent with how the rest of the codebase covers similar vectors.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/security/audit-channel.ts
Line: 111
Comment:
**Consider adding a test for the `__proto__` edge case**
The rest of the codebase has thorough test coverage for prototype-pollution vectors (e.g. `src/config/merge-patch.proto-pollution.test.ts`, `src/routing/account-id.test.ts`, `src/config/runtime-overrides.test.ts`), but there is no `audit-channel.test.ts` file and this PR doesn't add one. A small unit test verifying `hasExplicitProviderAccountConfig` returns `false` when `accountId` is `"__proto__"` (on an object that doesn't genuinely contain that own key) would lock in this behaviour and prevent future regressions, consistent with how the rest of the codebase covers similar vectors.
How can I resolve this? If you propose a fix, please make it concise.
|
Check failure seems to be unrelated |
benediktjohannes
changed the title
|
test |
|
Thanks for the work, @benediktjohannes. This issue has been addressed by maintainer-selected PR #34982 (tracking issue #34926).\n\nTo keep the queue clean, I’m closing this as a duplicate. |
2 participants
Summary
Problem: The helper function hasExplicitProviderAccountConfig used the in operator to check if an accountId exists in an accounts object. The in operator also traverses the prototype chain, so a specially crafted accountId like "proto" would return true even when no such account is actually configured.
Why it matters: This can cause the security audit to misinterpret the configuration, potentially skipping warnings for real accounts or treating non‑existent accounts as explicitly configured, which could weaken security guarantees.
What changed: Replaced accountId in accounts with Object.hasOwn(accounts, accountId) to ensure only the object’s own properties are considered.
What did NOT change: No behavior changes for normal account IDs.
Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
None.
Security Impact (required)
NoNoNoNoNoRisk without this change: The helper function hasExplicitProviderAccountConfig (in src/security/audit-channel.ts) uses the JavaScript in operator to check whether a given accountId exists in the accounts configuration object. The in operator also traverses the object's prototype chain, meaning that if an attacker can supply a specially crafted accountId (e.g.,
__proto__orconstructor), the check will erroneously return true even though no such account is actually configured.Repro + Verification
Because this is a proactive hardening fix and the exact exploit steps could be misused if published, we deliberately omit a full proof‑of‑concept.
Unforetunately, I don't have a local instance of OpenClaw installed, so I haven't tested this change, but I'm pretty confident from looking at the code that this is correct, but please correct me if I'm mistaken, thanks!
Environment