feat: enhance sandbox boundary checks to prevent access to internal constructors and ensure plain JSON serialization of tool schemas - #512
Merged
Conversation
![agentfront[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
… internal addresses by default and allow configuration for trusted environments (#511) Cherry-picked from #510 (merged to release/1.5.x) Original commit: 077201e Co-authored-by: agentfront[bot] <agentfront[bot]@users.noreply.github.com> Co-authored-by: frontegg-david <69419539+frontegg-david@users.noreply.github.com>
…onstructors and ensure plain JSON serialization of tool schemas
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR hardens CodeCall and enclave sandbox boundaries, sanitizes tool metadata and MCP results, validates package installation paths and names, adds security and behavior tests, updates documentation and public schema types, and upgrades enclave-related dependencies. ChangesCodeCall boundary hardening
Enclave output serialization
Package installation validation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR tightens sandbox boundary handling and tool introspection in the CodeCall plugin by ensuring only plain, inert data crosses into scripts (not live schema objects or other host-bound structures), and adds CLI install hardening against path/name injection.
Changes:
- Introduces a
toPlainJsonhelper and uses it to sanitize complex MCP tool results and tool schema metadata exposed to the sandbox. - Blocks CodeCall meta-tool introspection via
getTooland projects tool schemas via JSON Schema accessors (getInputJsonSchema/getOutputJsonSchema). - Adds sandbox bridge cloning for
callTool/getToolresults and hardensfrontmcp installwith plugin name + bundle path validation, with expanded unit/e2e coverage.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/plugin-codecall/src/utils/plain-json.ts | Adds JSON round-trip helper for producing plain JSON-safe data. |
| plugins/plugin-codecall/src/utils/mcp-result.ts | Sanitizes complex CallToolResult.content via toPlainJson. |
| plugins/plugin-codecall/src/utils/index.ts | Exposes toPlainJson from the utils barrel. |
| plugins/plugin-codecall/src/tools/execute.tool.ts | Prevents introspection of CodeCall tools and returns plain JSON tool schema projections. |
| plugins/plugin-codecall/src/README.md | Updates script-facing getTool typings for JSON Schema documents. |
| plugins/plugin-codecall/src/codecall.symbol.ts | Updates VM environment contract to expose JSON Schema objects (or null). |
| plugins/plugin-codecall/src/tests/plain-json.spec.ts | Adds unit tests for toPlainJson. |
| plugins/plugin-codecall/src/tests/mcp-result.spec.ts | Adds tests for multi-content isolation + plain-data projection. |
| plugins/plugin-codecall/src/tests/execute.tool.spec.ts | Updates tests for schema projection, null schemas, and self-reference blocking. |
| libs/sdk/src/job/enclave/job-enclave.bridge.ts | Clones callTool/getTool results before handing them to the sandbox. |
| libs/sdk/src/job/enclave/tests/job-enclave.bridge.spec.ts | Adds tests verifying sandbox receives copied/plain values and rejects non-serializable results. |
| libs/cli/src/commands/package/install.ts | Validates plugin name and bundle path containment during install; adds containment check to copies. |
| libs/cli/src/commands/package/tests/install.spec.ts | Adds install security regression tests for name/bundle path traversal. |
| apps/e2e/demo-e2e-codecall/e2e/codecall.e2e.spec.ts | Adds E2E coverage for schema inertness and constructor-escape blocking. |
Comments suppressed due to low confidence (1)
libs/cli/src/commands/package/install.ts:193
copyIfExistscan accept nested relative paths (sinceisPluginContainedPathallows them), but it doesn't create destination subdirectories. IfmanifestData.bundle(or any copied filename) contains a path segment likedist/bundle.js,fs.copyFileSyncwill throwENOENT.
const src = path.join(fromDir, filename);
if (fs.existsSync(src)) {
fs.copyFileSync(src, path.join(toDir, filename));
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/plugin-codecall/src/tools/execute.tool.ts (1)
156-176: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
getToolignores theallowedToolswhitelist.The new guard closes self-reference introspection, but
allowedToolSet(line 69) is still only consulted incallTool(line 88). A script constrained toallowedTools: ['users:list']can still enumerate the full name/description/schema of every registered tool viagetTool. If the whitelist is meant as a capability boundary and not just a call filter, apply it here too.🔒 Proposed alignment of introspection with the call whitelist
if (isBlockedSelfReference(name)) return undefined; const tools = this.scope.tools.getTools(true); const tool = tools.find((t) => t.name === name || t.fullName === name); if (!tool) return undefined; + if (allowedToolSet && !allowedToolSet.has(tool.name) && !allowedToolSet.has(tool.fullName)) { + return undefined; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/plugin-codecall/src/tools/execute.tool.ts` around lines 156 - 176, Update getTool to enforce the existing allowedToolSet whitelist before resolving and returning tool metadata, using the same tool-name matching semantics as callTool. Return undefined for tools outside the whitelist while preserving the self-reference guard and existing introspection behavior for allowed tools.
🧹 Nitpick comments (2)
apps/e2e/demo-e2e-codecall/e2e/codecall.e2e.spec.ts (1)
449-451: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winTighten these security assertions to exact values.
not.toBe('object')/not.toBe('function')pass for any unexpected type, so a regression that leaks internals as some other shape would go undetected. Since the projection is plain JSON, both should be exactly'undefined'.♻️ Proposed assertion tightening
- expect(execResult.result.internal).not.toBe('object'); - expect(execResult.result.parse).not.toBe('function'); + expect(execResult.result.internal).toBe('undefined'); + expect(execResult.result.parse).toBe('undefined');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/e2e/demo-e2e-codecall/e2e/codecall.e2e.spec.ts` around lines 449 - 451, Update the security assertions in the schema projection test to require exact undefined results: change the checks for execResult.result.internal and execResult.result.parse to assert the string value 'undefined', preserving the existing coverage of inaccessible internals and methods.libs/cli/src/commands/package/__tests__/install.spec.ts (1)
24-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
runCmdmock implementation leaks across tests.
runCmdis a single sharedjest.fn()(module-level mock).beforeEachusesclearAllMocks(), which clears call history but not implementations set viamockImplementation. ThemockImplementationset in "builds from frontmcp.config.js…" (Lines 169-171) therefore persists into later tests like "installs declared native addons" (Lines 196-213), silently writing an extra manifest into that test'spackageDir/distwhenrunCmdis invoked fornpm init/npm install. No current assertion fails, but this is a test-isolation smell that risks flaky/confusing failures as tests evolve.♻️ Proposed fix
-jest.clearAllMocks(); +jest.resetAllMocks();(Note:
resetAllMocks()removes implementations too, so mocks configured in module factories likerunQuestionnaire/fetchFromNpmwould need re-establishing per test, or scope the reset torunCmdonly, e.g.(runCmd as jest.Mock).mockReset();inbeforeEach.)Also applies to: 169-172, 196-213
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/cli/src/commands/package/__tests__/install.spec.ts` around lines 24 - 26, Reset the runCmd mock implementation between tests, not just its call history, so the mock configured in the “builds from frontmcp.config.js…” test cannot affect later tests such as “installs declared native addons.” Update the relevant beforeEach setup to reset only runCmd (for example via its mockReset behavior), while preserving the module-factory implementations for runQuestionnaire and fetchFromNpm.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/sdk/src/job/enclave/job-enclave.bridge.ts`:
- Line 118: In libs/sdk/src/job/enclave/job-enclave.bridge.ts at lines 118-118
and 127-127, capture context.callTool and context.getTool in local const
references before creating the sandbox closures, then invoke those captured
callbacks instead of re-reading the possibly mutated context properties; update
both affected call sites consistently.
In `@plugins/plugin-codecall/src/__tests__/execute.tool.spec.ts`:
- Around line 764-784: Rename the test case describing the legacy tool behavior
so its title reflects that getTool returns an object with null input and output
schemas, rather than saying it returns undefined. Keep the test setup and
assertions unchanged.
In `@plugins/plugin-codecall/src/__tests__/plain-json.spec.ts`:
- Around line 43-56: Rename the test case around toPlainJson to describe the
circular, unrepresentable pinned property scenario rather than flattening into
ordinary data. Keep the existing assertions and setup unchanged, since the
acyclic flattening behavior is covered separately.
In `@plugins/plugin-codecall/src/README.md`:
- Around line 673-678: Update the documented getTool declaration to allow an
undefined return value and make description optional, matching
CodeCallVmEnvironment’s schema contract. Review and update the mirrored
documentation under docs/frontmcp/plugins/** so all references consistently
describe the same behavior for meta-tools, unknown tools, and unprojectable
schemas.
---
Outside diff comments:
In `@plugins/plugin-codecall/src/tools/execute.tool.ts`:
- Around line 156-176: Update getTool to enforce the existing allowedToolSet
whitelist before resolving and returning tool metadata, using the same tool-name
matching semantics as callTool. Return undefined for tools outside the whitelist
while preserving the self-reference guard and existing introspection behavior
for allowed tools.
---
Nitpick comments:
In `@apps/e2e/demo-e2e-codecall/e2e/codecall.e2e.spec.ts`:
- Around line 449-451: Update the security assertions in the schema projection
test to require exact undefined results: change the checks for
execResult.result.internal and execResult.result.parse to assert the string
value 'undefined', preserving the existing coverage of inaccessible internals
and methods.
In `@libs/cli/src/commands/package/__tests__/install.spec.ts`:
- Around line 24-26: Reset the runCmd mock implementation between tests, not
just its call history, so the mock configured in the “builds from
frontmcp.config.js…” test cannot affect later tests such as “installs declared
native addons.” Update the relevant beforeEach setup to reset only runCmd (for
example via its mockReset behavior), while preserving the module-factory
implementations for runQuestionnaire and fetchFromNpm.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2f61786b-3ef1-478e-884b-5abccd34d12c
📒 Files selected for processing (14)
apps/e2e/demo-e2e-codecall/e2e/codecall.e2e.spec.tslibs/cli/src/commands/package/__tests__/install.spec.tslibs/cli/src/commands/package/install.tslibs/sdk/src/job/enclave/__tests__/job-enclave.bridge.spec.tslibs/sdk/src/job/enclave/job-enclave.bridge.tsplugins/plugin-codecall/src/README.mdplugins/plugin-codecall/src/__tests__/execute.tool.spec.tsplugins/plugin-codecall/src/__tests__/mcp-result.spec.tsplugins/plugin-codecall/src/__tests__/plain-json.spec.tsplugins/plugin-codecall/src/codecall.symbol.tsplugins/plugin-codecall/src/tools/execute.tool.tsplugins/plugin-codecall/src/utils/index.tsplugins/plugin-codecall/src/utils/mcp-result.tsplugins/plugin-codecall/src/utils/plain-json.ts
Contributor
Performance Test ResultsStatus: ✅ All tests passed Summary
Total: 92 tests across 18 projects 📊 View full report in workflow run Generated at: 2026-07-26T02:07:29.474Z |
…e the allowedTools whitelist and improve plain JSON schema handling
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/frontmcp/plugins/codecall/agentscript.mdx`:
- Around line 73-77: Update the getTool example so the returned meta value is
checked for undefined before accessing description or inputSchema. Follow the
existing documented guard pattern and ensure the example uses the library’s
public API surface while preserving the shown metadata output for a found tool.
In `@libs/testing/package.json`:
- Around line 95-97: Update the `@frontmcp/sdk` dependency entry in the package
manifest from 1.4.0 to the exact 1.5.2 release-line version, matching the
adjacent internal `@frontmcp` dependencies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c01abf78-c6de-45eb-9f0d-167248326989
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (34)
apps/e2e/demo-e2e-codecall/e2e/codecall.e2e.spec.tsdocs/frontmcp/plugins/codecall/agentscript.mdxlibs/adapters/package.jsonlibs/auth/package.jsonlibs/cli/package.jsonlibs/cli/src/commands/package/__tests__/install.spec.tslibs/di/package.jsonlibs/edge/package.jsonlibs/guard/package.jsonlibs/lazy-zod/package.jsonlibs/nx-plugin/package.jsonlibs/observability/package.jsonlibs/plugins/package.jsonlibs/protocol/package.jsonlibs/react/package.jsonlibs/sdk/src/job/enclave/job-enclave.bridge.tslibs/skills/package.jsonlibs/storage-sqlite/package.jsonlibs/testing/package.jsonlibs/ui/package.jsonlibs/uipack/package.jsonlibs/utils/package.jsonplugins/plugin-approval/package.jsonplugins/plugin-cache/package.jsonplugins/plugin-codecall/src/README.mdplugins/plugin-codecall/src/__tests__/execute.tool.spec.tsplugins/plugin-codecall/src/__tests__/plain-json.spec.tsplugins/plugin-codecall/src/codecall.symbol.tsplugins/plugin-codecall/src/tools/execute.tool.tsplugins/plugin-codecall/src/utils/plain-json.tsplugins/plugin-dashboard/package.jsonplugins/plugin-feature-flags/package.jsonplugins/plugin-remember/package.jsonplugins/plugin-skilled-openapi/package.json
🚧 Files skipped from review as they are similar to previous changes (9)
- plugins/plugin-codecall/src/utils/plain-json.ts
- plugins/plugin-codecall/src/codecall.symbol.ts
- libs/sdk/src/job/enclave/job-enclave.bridge.ts
- apps/e2e/demo-e2e-codecall/e2e/codecall.e2e.spec.ts
- plugins/plugin-codecall/src/tools/execute.tool.ts
- plugins/plugin-codecall/src/README.md
- libs/cli/src/commands/package/tests/install.spec.ts
- plugins/plugin-codecall/src/tests/execute.tool.spec.ts
- plugins/plugin-codecall/src/tests/plain-json.spec.ts
… improve schema handling
… improve schema handling
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 43 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
libs/cli/src/commands/package/install.ts:193
copyIfExists()now silently returns for unsafe relative paths and silently skips missing files. Since it’s used to copy required install artifacts (bundle, manifest, runner), this can lead to a “successful” install that registers an app with missing files. It also doesn’t create parent directories for nested bundle paths. Consider failing fast on unsafe/missing inputs and ensuring the destination directory exists.
function copyIfExists(fromDir: string, toDir: string, filename: string): void {
if (!isPluginContainedPath(toDir, filename)) return;
const src = path.join(fromDir, filename);
if (fs.existsSync(src)) {
fs.copyFileSync(src, path.join(toDir, filename));
}
}
alexmercerpo
approved these changes
Jul 26, 2026
… improve schema handling
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/cli/src/commands/package/install.ts`:
- Around line 192-213: Update resolveContainedFile and copyIfExists to use the
matching filesystem helpers exported by `@frontmcp/utils` instead of direct
node:fs calls, including realpath, stat, directory creation, and file copying;
preserve the existing containment checks and null/early-return behavior.
- Around line 205-213: Update copyIfExists and its installation-directory setup
so the destination parent is resolved and validated after recursive directory
creation, before fs.copyFileSync writes. Reject an existing symlinked installDir
or parent rather than traversing it, while preserving the existing
contained-path and source checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9acf51af-0918-46bd-af31-844e7ba8375b
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
libs/cli/src/commands/package/__tests__/install.spec.tslibs/cli/src/commands/package/install.tslibs/sdk/package.jsonlibs/sdk/src/job/enclave/__tests__/job-enclave.bridge.spec.tslibs/sdk/src/job/enclave/job-enclave.bridge.tslibs/utils/package.jsonplugins/plugin-codecall/package.jsonplugins/plugin-codecall/src/utils/mcp-result.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- libs/sdk/package.json
- plugins/plugin-codecall/package.json
- plugins/plugin-codecall/src/utils/mcp-result.ts
- libs/sdk/src/job/enclave/tests/job-enclave.bridge.spec.ts
… improve schema handling
… improve schema handling
2 tasks
Contributor
Cherry-pick ConflictAutomatic cherry-pick to An issue has been created with manual instructions. Please resolve if this change should also be in |
frontegg-david
added a commit
that referenced
this pull request
Jul 26, 2026
…onstructors and ensure plain JSON serialization of tool schemas (#512) (#514) * Cherry-pick: chore: update mcp-from-openapi to version 2.5.1 in package.json and yarn.lock (#509) Cherry-picked from #508 (merged to release/1.5.x) Original commit: 82f5351 * Cherry-pick: fix: enhance SSRF protection in OpenAPI polling to block internal addresses by default and allow configuration for trusted environments (#511) Cherry-picked from #510 (merged to release/1.5.x) Original commit: 077201e * feat: enhance sandbox boundary checks to prevent access to internal constructors and ensure plain JSON serialization of tool schemas * feat: enhance tool introspection to return undefined for tools outside the allowedTools whitelist and improve plain JSON schema handling * feat: update tool description retrieval to handle undefined cases and improve schema handling * feat: update tool description retrieval to handle undefined cases and improve schema handling * feat: update tool description retrieval to handle undefined cases and improve schema handling * feat: update tool description retrieval to handle undefined cases and improve schema handling --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: agentfront[bot] <agentfront[bot]@users.noreply.github.com>
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
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.
Summary by CodeRabbit
Security
Documentation
getTooldocs to reflect new schema shapes (inputSchema/outputSchemaas plain JSON Schema ornull) and clearerundefinedcases.Bug Fixes