feat: add tests for disallowed identifier detection in object coercion scenarios#55
Merged
frontegg-david merged 3 commits intorelease/2.11.xfrom Feb 22, 2026
Merged
feat: add tests for disallowed identifier detection in object coercion scenarios#55frontegg-david merged 3 commits intorelease/2.11.xfrom
frontegg-david merged 3 commits intorelease/2.11.xfrom
Conversation
…n scenarios (cherry picked from commit 759061e)
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughNew static coercion analysis utilities were added and integrated into DisallowedIdentifierRule and no-global-access-rule to resolve computed property keys; tests were expanded to cover many coercion and computed-key patterns. No public APIs changed. Changes
Sequence DiagramsequenceDiagram
participant AST as AST Parser
participant Rule as DisallowedIdentifierRule
participant Coercion as coercion-utils
participant Resolver as String Resolver
AST->>Rule: Emit MemberExpression node
Rule->>Rule: Is property present? is computed?
alt Non-computed Identifier
Rule->>Rule: Read Identifier name
Rule->>Rule: Check against disallowed set
Rule->>Rule: Report if matched
else Computed property
Rule->>Coercion: tryGetStaticComputedKeys(propertyNode)
Coercion->>Resolver: Analyze node kind (Literal, Template, Object, Array, Conditional, Sequence, Assignment, Logical, Nullish, etc.)
alt Literal / simple Template
Resolver->>Coercion: Return string(s)
else Object coercion (toString/valueOf/getter)
Resolver->>Coercion: Extract return literal from method/getter
Coercion->>Resolver: Return coerced string
else Array / nested coercion
Resolver->>Coercion: Resolve elements recursively to string
Coercion->>Resolver: Return coerced string
else Unresolvable
Resolver->>Coercion: Return empty / null
end
Coercion->>Rule: Return resolved keys[]
Rule->>Rule: Check keys against disallowed set
Rule->>Rule: Report DISALLOWED_IDENTIFIER for first match
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
libs/core/src/__tests__/enclave.proto-escape-via-array-coercion.spec.ts (1)
782-798: Remove unusedcodevariable with non-functional string replacements.The
codevariable (lines 784-791) is defined but never used. The string replacement operations also don't function as intended—they attempt to replace patterns that don't exist in the input string. OnlysafeCode(lines 793-794) is actually executed.♻️ Suggested cleanup
it('ATK-ARRCOERCE-39: template literal `__proto__` as computed key', async () => { const enclave = new Enclave({ timeout: 5000 }); - const code = ` - const proto = {}\`__proto__\`; - if (proto === null || proto === undefined) return 'blocked'; - return { type: typeof proto }; - ` - .replace('{}`', '[`') - .replace('`]', '`]') - .replace('{}`__proto__`', '[`__proto__`]'); - // Use string building to avoid template literal parsing issues in test - const safeCode = - 'const proto = {}[`__proto__`]; if (proto === null || proto === undefined) return "blocked"; return { type: typeof proto };'; - const result = await enclave.run(safeCode); + // Use string building to avoid template literal parsing issues in test + const code = + 'const proto = {}[`__proto__`]; if (proto === null || proto === undefined) return "blocked"; return { type: typeof proto };'; + const result = await enclave.run(code); assertNoEscape(result); enclave.dispose(); }, 15000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/core/src/__tests__/enclave.proto-escape-via-array-coercion.spec.ts` around lines 782 - 798, Remove the dead/unused intermediate string variable and its no-op replace calls: delete the declaration and replacement lines for the variable named code (the block that builds `const code = ... .replace(...).replace(...).replace(...);`) and keep only the working safeCode string used with Enclave.prototype.run (the variable safeCode) and the existing test assertions (assertNoEscape and enclave.dispose); this cleans up the test and avoids confusing non-functional replacements.
🤖 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 14-27: The function extractReturnLiteralString currently returns
the first string-literal return it finds and ignores additional returns; update
it to enforce the "exactly one return" contract by scanning block.body (as in
the existing loop) and counting ReturnStatement nodes: if there is exactly one
return and its argument is a Literal with a string value, return that string;
otherwise (zero returns, more than one return, or the return argument is not a
string literal) return null. Keep references to extractReturnLiteralString and
the checks for stmt.type === 'ReturnStatement' and stmt.argument.type ===
'Literal' to locate and change the logic.
- Around line 43-117: The function tryGetObjectCoercedString currently returns
the first matching property it finds, which can violate the ToPrimitive string
hint precedence; modify tryGetObjectCoercedString to first scan node.properties
and locate/store the Property nodes (or their values) for keys "toString" and
"valueOf" without returning immediately, then after the loop attempt to resolve
the stored toString property (using the same logic you already have:
ArrowFunctionExpression/FunctionExpression/getter handling and
extractReturnLiteralString) and only if that yields null attempt to resolve the
stored valueOf property; keep using the existing helper
extractReturnLiteralString and the same checks for ArrowFunctionExpression,
FunctionExpression and getter return types when resolving each stored property.
---
Nitpick comments:
In `@libs/core/src/__tests__/enclave.proto-escape-via-array-coercion.spec.ts`:
- Around line 782-798: Remove the dead/unused intermediate string variable and
its no-op replace calls: delete the declaration and replacement lines for the
variable named code (the block that builds `const code = ...
.replace(...).replace(...).replace(...);`) and keep only the working safeCode
string used with Enclave.prototype.run (the variable safeCode) and the existing
test assertions (assertNoEscape and enclave.dispose); this cleans up the test
and avoids confusing non-functional replacements.
…ring and valueOf methods
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/core/src/__tests__/enclave.proto-escape-via-array-coercion.spec.ts (1)
656-667: Ensure Enclave disposal runs even on assertion failures.If
assertNoEscapethrows,enclave.dispose()won’t run, which can leak resources and make later tests flaky. Consider guarding withtry/finally(apply across these new tests).♻️ Suggested pattern (apply to similar tests)
it('ATK-ARRCOERCE-31: {toString: () => "__proto__"} direct key access', async () => { const enclave = new Enclave({ timeout: 5000 }); const code = ` const proto = {}[{toString: () => '__proto__'}]; if (proto === null || proto === undefined) return 'blocked'; const ctor = proto[{toString: () => 'constructor'}]; return { type: typeof ctor }; `; - const result = await enclave.run(code); - assertNoEscape(result); - enclave.dispose(); + try { + const result = await enclave.run(code); + assertNoEscape(result); + } finally { + enclave.dispose(); + } }, 15000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/core/src/__tests__/enclave.proto-escape-via-array-coercion.spec.ts` around lines 656 - 667, The test creates an Enclave instance and calls assertNoEscape then enclave.dispose, but if assertNoEscape throws the enclave is not disposed; wrap the assertion and any code that can throw in a try/finally around the call to assertNoEscape (and the returns) so enclave.dispose() is always called; locate this pattern in the test case using the Enclave constructor/variable name "enclave", the assertion call "assertNoEscape", and the disposer "enclave.dispose" and apply the same try/finally pattern to the other similar tests in this file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@libs/core/src/__tests__/enclave.proto-escape-via-array-coercion.spec.ts`:
- Around line 656-667: The test creates an Enclave instance and calls
assertNoEscape then enclave.dispose, but if assertNoEscape throws the enclave is
not disposed; wrap the assertion and any code that can throw in a try/finally
around the call to assertNoEscape (and the returns) so enclave.dispose() is
always called; locate this pattern in the test case using the Enclave
constructor/variable name "enclave", the assertion call "assertNoEscape", and
the disposer "enclave.dispose" and apply the same try/finally pattern to the
other similar tests in this file.
…try-finally blocks
Contributor
Cherry-pick ConflictAutomatic cherry-pick to An issue has been created with manual instructions. Please resolve if this change should also be in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
(cherry picked from commit 759061e)
Summary by CodeRabbit
New Features
Tests