Skip to content

fix(hogvm): use own-property checks for nested access and async dispatch#59218

Merged
mariusandra merged 3 commits into
masterfrom
fix/hogvm-nested-property-access
May 21, 2026
Merged

fix(hogvm): use own-property checks for nested access and async dispatch#59218
mariusandra merged 3 commits into
masterfrom
fix/hogvm-nested-property-access

Conversation

@meikelmosby
Copy link
Copy Markdown
Collaborator

Problem

A few places in the HogVM TypeScript runtime treat inherited properties the same as own properties:

  • getNestedValue in common/hogvm/typescript/src/utils.ts does raw bracket access for non-numeric, non-Map keys, so a chain like ['__proto__'] walks the prototype chain instead of returning null.
  • The async dispatch in common/hogvm/typescript/src/execute.ts uses the in operator to look up asyncFunctionName in options.asyncFunctions and ASYNC_STL, which also matches inherited keys (toString, constructor, etc.). The sync dispatch path a few hundred lines down already uses Object.hasOwn(ASYNC_STL, ...), so the async paths were the odd ones out.

Both behaviors are surprising relative to what the VM is meant to expose. This PR tightens them up.

Changes

  • getNestedValue: explicitly reject __proto__, constructor, and prototype as keys when traversing plain objects. Map and numeric-index branches are unchanged.
  • execAsync: replace name in options.asyncFunctions and name in ASYNC_STL with Object.prototype.hasOwnProperty.call(...), matching the pattern already used by the sync dispatch site.

No behavior change for valid inputs — only inputs that previously resolved against inherited properties now fail cleanly.

How did you test this code?

I'm an agent — no manual testing was performed.

Ran the existing HogVM TypeScript test suites:

  • npx jest src/__tests__/utils.test.ts src/__tests__/execute.test.ts in common/hogvm/typescript/ — 47/47 passing.

Publish to changelog?

no

🤖 Agent context

Authored by Claude (Opus 4.7) at the user's direction. The user pointed at the three affected call sites and asked for the minimal recommended patch — no broader refactor of property-access utilities, STL conversion, or the surrounding execution flow. Considered defense-in-depth changes to setNestedValue and JSONExtractArrayRawFn but explicitly scoped them out for this PR.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 20, 2026

Security Review

  • Incomplete prototype blocklist in common/hogvm/typescript/src/utils.ts (getNestedValue): the three-name denylist (__proto__, constructor, prototype) does not cover other inherited Object.prototype properties such as toString, valueOf, and hasOwnProperty. These are still reachable via plain bracket access, contrary to the stated hardening goal.
  • The execAsync fix (Object.prototype.hasOwnProperty.call) correctly closes the in-operator gap for async function dispatch.
  • No new tests cover the prototype-access rejection, leaving the security contract untested.
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
common/hogvm/typescript/src/utils.ts:63-66
**Blocklist is incomplete — other inherited properties still leak**

The three-name blocklist prevents access to `__proto__`, `constructor`, and `prototype`, but every other property on `Object.prototype` (`toString`, `valueOf`, `hasOwnProperty`, `isPrototypeOf`, etc.) is still reachable via plain bracket access. For example, `getNestedValue({}, ['toString'])` returns the inherited `[Function: toString]` rather than `null`, which contradicts the stated goal of preventing prototype-chain traversal.

The more complete fix is to gate access on an own-property check and return `null` for anything not owned by the object: `obj = Object.prototype.hasOwnProperty.call(obj, key) ? (obj[key] ?? null) : null`. This eliminates the need for the blocklist entirely and is consistent with how the sync STL dispatch already uses `Object.hasOwn`.

### Issue 2 of 2
common/hogvm/typescript/src/utils.ts:63-65
**No tests for the new security-sensitive code paths**

Neither `utils.test.ts` nor `execute.test.ts` imports `getNestedValue` or covers the `__proto__`/`constructor`/`prototype` rejection, or the fixed `Object.prototype.hasOwnProperty.call` dispatch in `execAsync`. Given that these changes exist specifically to harden security boundaries, parametrised tests covering at least: a blocked key throwing, a normal string key still resolving correctly, and a non-own inherited key (e.g. `toString`) returning `null`, would be the minimum expected coverage.

Reviews (1): Last reviewed commit: "fix(hogvm): use own-property checks for ..." | Re-trigger Greptile

Comment thread common/hogvm/typescript/src/utils.ts Outdated
Comment thread common/hogvm/typescript/src/utils.ts Outdated
Address review feedback:
- Replace the three-name blocklist in getNestedValue with an own-property
  check, so all inherited keys (toString, valueOf, etc.) consistently
  return null rather than leaking prototype values.
- Use Object.prototype.hasOwnProperty.call for the declaredFunctions
  lookup in CALL_GLOBAL, matching the pattern already used elsewhere.
- Add parametrised tests for getNestedValue (own/missing/inherited keys,
  Map and array indexing) and for execAsync dispatch rejecting inherited
  function names.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@meikelmosby meikelmosby reopened this May 20, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 20, 2026

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
common/hogvm/typescript/src/__tests__/execute.test.ts:394-401
**`toString` missing from async dispatch test**

The test array covers `hasOwnProperty`, `constructor`, and `__proto__`, but omits `toString` — the one name explicitly guarded against in the adjacent `declaredFunctions` check (`name !== 'toString'`). Before this PR, `'toString' in ASYNC_STL` evaluated to `true` (inherited), so `toString` was the original motivating case. Adding it here would close the gap and keep the test in sync with the guarded name in the sync path.

Reviews (2): Last reviewed commit: "chore(hogvm): widen own-property checks ..." | Re-trigger Greptile

@mariusandra mariusandra enabled auto-merge (squash) May 21, 2026 10:21
@mariusandra mariusandra disabled auto-merge May 21, 2026 14:53
@mariusandra mariusandra merged commit 44fff20 into master May 21, 2026
215 of 216 checks passed
@mariusandra mariusandra deleted the fix/hogvm-nested-property-access branch May 21, 2026 14:53
@deployment-status-posthog
Copy link
Copy Markdown

deployment-status-posthog Bot commented May 21, 2026

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-05-21 15:29 UTC Run
prod-us ✅ Deployed 2026-05-21 15:41 UTC Run
prod-eu ✅ Deployed 2026-05-21 15:43 UTC Run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants