perf: codex-sandbox check#6851
Conversation
Coverage Report
File CoverageNo changed files found. |
|
✅ Docs Preview Deployed! 🔗 👀 Click here to visit preview |
There was a problem hiding this comment.
Pull request overview
This PR strengthens the code-sandbox runtime security model (AST-based dynamic import() detection, tighter global/runtime hardening, safer module access) and updates tests/docs to reflect the new sandbox constraints.
Changes:
- Harden JS worker runtime (lock
Function/eval, freeze intrinsics, readonly module exports, timer cleanup, AST-walk to block dynamicimport()). - Add internal-network/IP/metadata SSRF checking utility and wire it into sandbox HTTP requests.
- Expand security and API integration tests to cover the new hardening behaviors (dynamic import variants, constructor-chain bypasses, pollution/isolation checks).
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/code-sandbox/src/pool/worker.ts | Adds AST-based import() detection and significant runtime hardening (globals, timers, require cache, readonly module views). |
| projects/code-sandbox/src/utils/ipCheck.util.ts | New internal-address detection helper used by sandbox HTTP requests. |
| projects/code-sandbox/src/env.ts | Uses dotenv.config({ quiet: true }) to avoid stdout banner interfering with IPC. |
| projects/code-sandbox/test/unit/security.test.ts | Updates/extends sandbox escape + isolation tests and changes public HTTP endpoints used by tests. |
| projects/code-sandbox/test/unit/resource-limits.test.ts | Updates request-body limit tests to use a different external endpoint. |
| projects/code-sandbox/test/integration/functional.test.ts | Adds “dynamic import detection negative samples” matrix + updates network request targets. |
| projects/code-sandbox/test/integration/api.test.ts | Adds API-level tests for dynamic import variants, eval/constructor-chain bypass attempts, require surface restrictions. |
| projects/code-sandbox/package.json | Adds ipaddr.js, acorn, acorn-walk dependencies. |
| pnpm-workspace.yaml | Switches from projects/* glob to an explicit projects list. |
| pnpm-lock.yaml | Lockfile updates for added deps / workspace changes. |
| document/content/self-host/upgrading/4-15/4150.mdx | Release note entry for codex-sandbox AST/security strengthening. |
| document/data/doc-last-modified.json | Timestamp update for the modified doc page. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
projects/code-sandbox/test/integration/functional.test.ts:626
- Same issue for the POST case:
https://1.1.1.1/...is HTTPS to a literal IP and can fail TLS hostname verification, which would make this test fail even though the sandbox implementation is correct. Consider using a stable hostname endpoint with a valid certificate.
name: 'httpRequest POST JSON',
code: `async function main() {
const res = await httpRequest('https://1.1.1.1/cdn-cgi/trace', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: { message: 'hello' }
});
return { hasStatus: typeof res.status === 'number' };
}`,
expect: { success: true, codeReturnMatch: { hasStatus: true } }
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| code: `async function main() { | ||
| const bigBody = 'x'.repeat(${sizeMB} * 1024 * 1024 + 1); | ||
| try { | ||
| await httpRequest('https://example.com', { method: 'POST', body: bigBody }); | ||
| await httpRequest('https://1.1.1.1/cdn-cgi/trace', { method: 'POST', body: bigBody }); | ||
| return { blocked: false }; | ||
| } catch(e) { | ||
| return { blocked: true, msg: e.message }; |
There was a problem hiding this comment.
https://1.1.1.1/cdn-cgi/trace is an HTTPS request to a literal IP; TLS validation may fail (cert hostname mismatch), causing this resource-limit test to fail for reasons unrelated to request body size. Use a hostname with a valid cert (or http://) so failures reflect the intended behavior.
| name: 'httpRequest GET', | ||
| code: `async function main() { | ||
| const res = await httpRequest('https://www.baidu.com'); | ||
| const res = await httpRequest('https://1.1.1.1/cdn-cgi/trace'); | ||
| return { status: res.status, hasData: res.data.length > 0 }; | ||
| }`, | ||
| expect: { success: true, codeReturnMatch: { status: 200, hasData: true } } |
There was a problem hiding this comment.
These integration tests expect network access over HTTPS to https://1.1.1.1/.... TLS hostname verification against an IP is commonly invalid, so this can fail in CI even when networking is fine. Prefer a hostname endpoint with a valid certificate to keep assertions (status: 200) reliable.
| code: `async function main() { | ||
| const smallBody = JSON.stringify({ data: 'hello' }); | ||
| try { | ||
| await httpRequest('https://example.com', { method: 'POST', body: smallBody }); | ||
| await httpRequest('https://1.1.1.1/cdn-cgi/trace', { method: 'POST', body: smallBody }); | ||
| return { sizeOk: true }; | ||
| } catch(e) { | ||
| // 网络错误可以接受,但不应该是 body too large |
There was a problem hiding this comment.
Same concern as above: https://1.1.1.1/... over HTTPS to a literal IP can fail TLS hostname verification, making this test fail for unrelated reasons. Use a hostname endpoint with a valid certificate (or http://) so the test only validates the body-size behavior.
| it('http_request POST 带 body', async () => { | ||
| const result = await runner.execute({ | ||
| code: `import json\ndef main():\n res = system_helper.http_request('https://www.baidu.com', method='POST', body={'key': 'value'})\n return {'hasStatus': type(res['status']) == int}`, | ||
| code: `import json\ndef main():\n res = system_helper.http_request('https://1.1.1.1/cdn-cgi/trace', method='POST', body={'key': 'value'})\n return {'hasStatus': type(res['status']) == int}`, | ||
| variables: {} | ||
| }); | ||
| expect(result.success).toBe(true); |
There was a problem hiding this comment.
Same TLS reliability concern for this POST case: https://1.1.1.1/... is an HTTPS request to a bare IP and may fail certificate hostname checks. A hostname-based endpoint (or http://) will make the test outcome depend on the request-body limit logic rather than TLS behavior.
| const result = await runner.execute({ | ||
| code: `async function main() { const res = await SystemHelper.httpRequest('https://www.baidu.com'); return { status: res.status, hasData: res.data.length > 0 }; }`, | ||
| code: `async function main() { const res = await SystemHelper.httpRequest('https://1.1.1.1/cdn-cgi/trace'); return { status: res.status, hasData: res.data.length > 0 }; }`, | ||
| variables: {} | ||
| }); |
There was a problem hiding this comment.
Using an https:// URL with a literal IP (https://1.1.1.1/...) is likely to fail TLS certificate validation (no matching SAN for the IP), which will make this test flaky/failing across environments. Prefer a stable hostname endpoint with a valid cert (e.g. https://www.cloudflare.com/cdn-cgi/trace or https://example.com/) or switch to plain http:// if TLS is not required for the test purpose.
| it('全局函数 httpRequest 可用', async () => { | ||
| const result = await runner.execute({ | ||
| code: `async function main() { const res = await httpRequest('https://www.baidu.com'); return { status: res.status }; }`, | ||
| code: `async function main() { const res = await httpRequest('https://1.1.1.1/cdn-cgi/trace'); return { status: res.status }; }`, | ||
| variables: {} | ||
| }); |
There was a problem hiding this comment.
This test uses https://1.1.1.1/... via the global httpRequest. HTTPS to a literal IP often fails TLS hostname verification, which can break the test even if httpRequest is functioning correctly. Prefer a hostname endpoint with a valid certificate (or http://).
| it('http_request GET 公网地址正常', async () => { | ||
| const result = await runner.execute({ | ||
| code: `def main():\n res = system_helper.http_request('https://www.baidu.com')\n return {'status': res['status'], 'hasData': len(res['data']) > 0}`, | ||
| code: `def main():\n res = system_helper.http_request('https://1.1.1.1/cdn-cgi/trace')\n return {'status': res['status'], 'hasData': len(res['data']) > 0}`, | ||
| variables: {} | ||
| }); | ||
| expect(result.success).toBe(true); |
There was a problem hiding this comment.
The Python http_request tests now hit https://1.1.1.1/... (HTTPS to a literal IP). Depending on the HTTP client implementation, TLS hostname verification can fail and make this test flaky. Consider switching to a hostname endpoint with a valid TLS certificate (or http://) to keep the test focused on sandbox networking behavior.
| it('全局函数 http_request 可用', async () => { | ||
| const result = await runner.execute({ | ||
| code: `def main():\n res = http_request('https://www.baidu.com')\n return {'status': res['status']}`, | ||
| code: `def main():\n res = http_request('https://1.1.1.1/cdn-cgi/trace')\n return {'status': res['status']}`, | ||
| variables: {} | ||
| }); | ||
| expect(result.success).toBe(true); |
There was a problem hiding this comment.
This also uses https://1.1.1.1/... via the global http_request; HTTPS-to-IP can fail TLS hostname validation and cause flaky failures unrelated to the sandbox. Prefer a hostname endpoint with a valid cert (or http://).
| const hostDomain = normalizeDomain(parsedUrl.hostname); | ||
| const localHost = SERVICE_LOCAL_HOST.split(':')[0].toLowerCase(); | ||
|
|
||
| // 1. localhost / 本机 | ||
| if (LOCALHOST_HOSTNAMES.has(hostDomain) || hostDomain === localHost) { | ||
| return true; |
There was a problem hiding this comment.
SERVICE_LOCAL_HOST.split(':')[0] is incorrect when HOSTNAME is IPv6 and SERVICE_LOCAL_HOST is formatted like [::1]:3000 (it becomes '['). This can cause isInternalAddress() to miss self-host checks in IPv6 deployments. Consider storing the hostname separately (without port) or extracting the bracketed IPv6 host via URL parsing / regex before comparing.
| const result = await runner.execute({ | ||
| code: `async function main() { | ||
| const res = await SystemHelper.httpRequest('https://www.baidu.com', { method: 'POST', body: { key: 'value' } }); | ||
| const res = await SystemHelper.httpRequest('https://1.1.1.1/cdn-cgi/trace', { method: 'POST', body: { key: 'value' } }); | ||
| return { hasStatus: typeof res.status === 'number' }; | ||
| }`, |
There was a problem hiding this comment.
This test now posts to https://1.1.1.1/cdn-cgi/trace. HTTPS to a bare IP commonly fails TLS hostname verification, so the sandbox call may throw and make the test unstable. Use a hostname-based endpoint with a valid certificate (or http:// if acceptable) to keep CI deterministic.
|
✅ Build Successful - Preview code-sandbox Image for this PR: |
|
✅ Admin Preview Image Ready! |
|
✅ Build Successful - Preview fastgpt Image for this PR: |
|
✅ Build Successful - Preview mcp_server Image for this PR: |
0a8a643 to
92a1233
Compare
No description provided.