Cherry-pick: feat: add tests for disallowed identifier detection in object coercion scenarios

[frontegg-david]

Summary by CodeRabbit

• Improvements
  • Enhanced detection of object-to-string coercion patterns, prioritizing toString over valueOf and supporting getter-returned functions.
• Tests
  • Added coverage for duplicate property resolution and multi-return getter scenarios.
  • Improved test reliability by ensuring deterministic resource cleanup in execution blocks.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

---

📝 Walkthrough

Walkthrough

This PR refactors object coercion string resolution in coercion-utils.ts (stricter single-return detection, new resolver helper, getter-returned-function support, toString-over-valueOf precedence) and updates multiple tests to use try/finally for deterministic Enclave disposal.

Changes

Cohort / File(s)	Summary
Coercion Detection Enhancement libs/ast/src/rules/coercion-utils.ts	Refactored literal extraction to require exactly one return, added resolveCoercionProperty helper to unify handling of toString/valueOf, extended support for getters that return functions, and enforced ToPrimitive precedence (toString then valueOf).
Test Resource Cleanup libs/core/src/__tests__/enclave.proto-escape-via-array-coercion.spec.ts	Replaced direct enclave.run/enclave.dispose patterns with try { ... } finally { enclave.dispose(); } across multiple tests to ensure proper deterministic teardown.
Tests: Rule Coverage libs/ast/src/__tests__/rules.spec.ts	Added tests for duplicate toString property precedence (last wins) and for getters with multiple returns that can yield a disallowed identifier.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

• Manual cherry-pick needed: feat: add tests for disallowed identifier detection in object coercion scenarios #56: Matches added/adjusted tests and coercion detection changes targeting disallowed identifier detection in object coercion.

Possibly related PRs

• feat: add tests for disallowed identifier detection in object coercion scenarios #55: Touches coercion-utils.ts and related coercion handling; strongly related to the refactor and tests added here.

Poem

🐇 I hop through returns and getter bends,

toString first, then valueOf mends.
Tests now tidy with finally in tow,
Clean disposals where failures go.
A rabbit cheers — the checks all flow!

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: adding tests for disallowed identifier detection in object coercion scenarios, which aligns with the PR's core additions of two new test cases in the DisallowedIdentifierRule suite.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch cherry-pick-pr-55

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

libs/ast/src/rules/coercion-utils.ts (1)

72-96: Getter handling doesn't check for multiple return paths.

The getter case iterates through statements and breaks after the first ReturnStatement (line 92), whereas extractReturnLiteralString enforces exactly one return. This could miss disallowed identifiers in alternative return paths:

get toString() {
  if (x) return () => 'safe';
  return () => '__proto__';  // not analyzed
}

For security-critical code, consider collecting all return statements in the getter and analyzing each returned function, similar to how collectStaticKeys handles branching expressions.

Potential approach

   // Getter: { get toString() { return () => 'x' } }
   // The getter returns a function; JS calls the getter then calls the returned function.
   if (prop.kind === 'get' && value.type === 'FunctionExpression') {
     if (value.body && value.body.type === 'BlockStatement') {
       for (const stmt of value.body.body) {
         if (stmt.type === 'ReturnStatement' && stmt.argument) {
           const ret = stmt.argument;
           if (ret.type === 'ArrowFunctionExpression') {
             if (ret.expression && ret.body?.type === 'Literal' && typeof ret.body.value === 'string') {
               return ret.body.value;
             }
             if (ret.body?.type === 'BlockStatement') {
               const inner = extractReturnLiteralString(ret.body);
               if (inner !== null) return inner;
             }
           }
           if (ret.type === 'FunctionExpression') {
             if (ret.body?.type === 'BlockStatement') {
               const inner = extractReturnLiteralString(ret.body);
               if (inner !== null) return inner;
             }
           }
-          break;
+          // Continue checking other returns for multi-path getters
         }
       }
     }
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@libs/ast/src/rules/coercion-utils.ts` around lines 72 - 96, The getter
handling currently stops after the first ReturnStatement (in the loop over
value.body.body) which can miss other return paths; update the logic in the
prop.kind === 'get' branch to collect and examine all ReturnStatement nodes
(instead of breaking on the first) and for each returned function
expression/arrow use extractReturnLiteralString to check for literal strings
(mirroring collectStaticKeys' branching handling) and return a disallowed string
if any return path yields one; ensure you still return null only if every return
path is safe.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@libs/ast/src/rules/coercion-utils.ts`:
- Around line 136-141: The current loop in coercion-utils.ts captures the first
occurrence of duplicate property names because of the guards `!toStringProp` and
`!valueOfProp`; change the logic so the loop always assigns `toStringProp =
prop` when `keyName === 'toString'` and `valueOfProp = prop` when `keyName ===
'valueOf'` (remove the `!toStringProp` and `!valueOfProp` guards) so the
variables `toStringProp` and `valueOfProp` hold the last occurrence (matching JS
runtime semantics for duplicate object literal properties).

---

Nitpick comments:
In `@libs/ast/src/rules/coercion-utils.ts`:
- Around line 72-96: The getter handling currently stops after the first
ReturnStatement (in the loop over value.body.body) which can miss other return
paths; update the logic in the prop.kind === 'get' branch to collect and examine
all ReturnStatement nodes (instead of breaking on the first) and for each
returned function expression/arrow use extractReturnLiteralString to check for
literal strings (mirroring collectStaticKeys' branching handling) and return a
disallowed string if any return path yields one; ensure you still return null
only if every return path is safe.