Support custom scripts to determine whether it is up/down based on the response JSON #1634
Conversation
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
Auto Pull Request Review from LlamaPReview
1. Overview
1.1 PR Summary
- Business value and requirements alignment: This PR introduces support for custom scripts to determine the status of HTTP(S) monitors based on the response JSON. This aligns with the business need for flexible and customizable monitoring solutions.
- Key components modified:
Server/db/models/Monitor.js: AddedtestScriptsproperty to store custom scripts.Server/service/networkService.js: ModifiedrequestHttpfunction to execute custom scripts.Server/utils/sandbox.js: Added for securely executing user scripts.Server/validation/joi.js: Updated validation schemas to includetestScripts.
- Impact assessment:
- Enhances the flexibility of the monitoring system by allowing users to define custom scripts for status determination.
- Introduces security considerations due to the execution of user-provided scripts.
- Potential performance impacts due to the overhead of script execution.
- System dependencies and integration impacts:
- Introduces a new dependency,
isolated-vm, for sandboxing script execution. - Modifies the core monitoring logic to integrate script execution.
- Introduces a new dependency,
1.2 Architecture Changes
- System design modifications:
- Addition of a sandboxing mechanism for secure script execution.
- Extension of the
Monitordata model to include custom scripts.
- Component interactions:
networkService.jsinteracts withsandbox.jsto execute custom scripts.Monitormodel stores the custom scripts.
- Integration points:
requestHttpfunction innetworkService.jsis modified to callrunInSandboxfromsandbox.js.
2. Detailed Technical Analysis
2.1 Code Logic Deep-Dive
Core Logic Changes
- Server/db/models/Monitor.js - testScripts
- Submitted PR Code:
],
+ },
+ testScripts: {
+ type: String,
},
url: {
type: String,- Analysis:
- The PR adds a
testScriptsfield of typeStringto theMonitormodel to store user-provided JavaScript code for custom status checks. - Validation and sanitization of this field are crucial to prevent injection attacks.
- Storing arbitrary strings without further validation could lead to unexpected behavior or security issues.
- The PR adds a
- LlamaPReview Suggested Improvements:
],
+ },
+ testScripts: {
+ type: String,
+ validate: {
+ validator: function(v) {
+ try {
+ // Attempt to parse the script to validate syntax. Simple check, not full AST parsing.
+ new Function(v);
+ return true;
+ } catch (e) {
+ return false;
+ }
+ },
+ message: 'testScripts must be valid JavaScript syntax.'
+ }
},
url: {
type: String,- Improvement rationale:
- Technical benefits: Adds a basic level of validation to ensure that the stored
testScriptsare at least syntactically valid JavaScript. This reduces the risk of runtime errors due to syntax mistakes in user scripts and improves data integrity. - Business value: Improves user experience by providing immediate feedback during monitor creation or update if the provided script has syntax errors.
- Risk assessment: Mitigates some risks associated with storing arbitrary strings as executable code by ensuring basic syntax correctness. This is a first step; more robust validation and security measures in the sandbox are still necessary.
- Technical benefits: Adds a basic level of validation to ensure that the stored
Core Logic Changes
- Server/service/networkService.js - requestHttp
- Submitted PR Code:
const testScripts = job.data.testScripts;
if (testScripts) {
const status = await runInSandbox(testScripts, response?.data);
httpResponse.status = !!status;
} else {
httpResponse.status = true;
}- Analysis:
- This code block executes the user-provided
testScriptswithin therunInSandboxfunction, passing the HTTP response data (response?.data) as an argument. - The implementation assumes the script can handle whatever
response?.datais, which might lead to runtime errors within the sandbox if the script expects a JSON object but receives something else. - The code simply uses
!!statusto convert the script's return value to a boolean forhttpResponse.status. It's not explicitly defined what kind of values are considered "truthy" or "falsy" by the sandbox and how this maps to the intended monitor status.
- This code block executes the user-provided
- LlamaPReview Suggested Improvements:
const testScripts = job.data.testScripts;
if (testScripts) {
let scriptResult;
try {
scriptResult = await runInSandbox(testScripts, response?.data);
} catch (sandboxError) {
this.logger.error({ message: 'User script execution error', error: sandboxError, monitorId: job.data._id, service: SERVICE_NAME, method: "requestHttp" });
httpResponse.status = false; // Default to down on script error
httpResponse.message = `User script error: ${sandboxError.message}`;
return httpResponse;
}
httpResponse.status = !!scriptResult; // Explicit boolean conversion
} else {
httpResponse.status = true;
}- Improvement rationale:
- Technical benefits: Improves error handling by adding a
try-catchblock around therunInSandboxcall. This allows the system to gracefully handle errors occurring during script execution (e.g., timeouts, runtime errors in user script). Logs the error for debugging and sets the monitor status tofalse(down) by default if a script error occurs, ensuring a safe fallback. - Business value: Enhances reliability by preventing script errors from crashing the monitoring process and providing informative error messages in the monitor status.
- Risk assessment: Reduces the risk of silent failures due to user script errors and improves the observability of the system by logging script execution errors.
- Technical benefits: Improves error handling by adding a
Core Logic Changes
- Server/utils/sandbox.js - runInSandbox
- Submitted PR Code:
+ import ivm from 'isolated-vm';
+
+ export class TimeoutError extends Error {
+ constructor(message) { super(message); }
+ }
+
+ export class UserScriptError extends Error {
+ constructor(message, row, col) {
+ super(message);
+ this.row = row;
+ this.col = col;
+ }
+ }
+
+ export default async function(code, param) {
+ // Create a new isolate limited to 128MB
+ const isolate = new ivm.Isolate({ memoryLimit: 128 });
+
+ // Create a new context within this isolate. Each context has its own copy of all the builtin Objects.
+ // So for instance if one context does Object.prototype.foo = 1 this would not affect any other contexts.
+ const context = isolate.createContextSync();
+
+ // Complete code to be executed by `evalClosure`.
+ // Add code to get the argument as the `res`, so that `res` can be used in the user code.
+ // Example:
+ // return res.code === 200;
+ // No spaces or line breaks, preserve the error location.
+ const script =
+ `const res = $0;
+ try {
+ ${code}
+ } catch (e) {
+ const lines = e.stack.split("\
");
+ const [, row, col] = lines[lines.length - 1].match(/\\sat\\s\\<isolated-vm\\>\\:(\\d)\\:(\\d)/);
+ throw new Error("UserScriptError:" + row + ":" + col + ":" + e.message);
+ }
+ `;
+
+ // Compiles and runs code as if it were inside a function, similar to the seldom-used new Function(code) constructor.
+ // The function will return a Promise while the work runs in a separate thread pool.
+ // After the timeout, the thread will be automatically terminated, and we do not need to handle it.
+ return context.evalClosure(script, [param], { timeout: 1000, arguments: { copy: true } }).catch(e => {
+ if (e.message.startsWith('UserScriptError')) {
+ // User script error, display error location in user script.
+ const [, row, col, ...messages] = e.message.split(':');
+ throw new UserScriptError("UserScriptError: " + messages.join(':'), row - 2, col);
+ } else if (e.message === 'Script execution timed out.') {
+ // Timeout
+ throw new TimeoutError(e.message);
+ } else {
+ // Other runtime error
+ throw e;
+ }
+ });
+ }- Analysis:
- The
sandbox.jsfile implements the script execution usingisolated-vm. The code sets amemoryLimit: 128for the isolate and atimeout: 1000ms for script execution, which are good starting points for resource control. - The code uses
evalClosureto execute the script. Whileisolated-vmprovides isolation,evalClosurestill executes arbitrary code. - The error handling within
sandbox.jsdistinguishes betweenUserScriptErrorandTimeoutError, which is good for providing specific error information. - The code injects the response data as
$0and accesses it asresin the script. This is a reasonable approach, but it's important to document this clearly for users writing scripts.
- The
- LlamaPReview Suggested Improvements: [No code change, documentation improvement suggested]
- Improvement rationale:
- Technical benefits: While no code change is suggested here, the analysis highlights the importance of documentation. Clear documentation about the scripting environment, available variables (
res), expected input data format, limitations (timeout, memory limit), and error handling will significantly improve usability and reduce potential misconfigurations or unexpected behavior. - Business value: Better documentation reduces support requests and improves user satisfaction by enabling users to effectively utilize the custom scripting feature.
- Risk assessment: Good documentation is crucial for security when dealing with user-provided code. It helps users understand the boundaries and limitations of the sandbox environment and write scripts that are less likely to cause issues or expose vulnerabilities (even within the sandbox). It also clarifies the security responsibilities of the user and the system.
- Technical benefits: While no code change is suggested here, the analysis highlights the importance of documentation. Clear documentation about the scripting environment, available variables (
2.2 Implementation Quality
- Code organization and structure: The PR is well-organized, with clear separation of concerns. The sandboxing logic is isolated in
sandbox.js, and the monitoring logic is modified innetworkService.js. - Design patterns usage: The use of
isolated-vmfor sandboxing is a good design choice for securely executing user-provided scripts. - Error handling approach: The PR includes robust error handling, especially in
sandbox.js, where different types of errors are distinguished and handled appropriately. - Resource management: The PR sets memory and timeout limits for script execution, which is crucial for resource management.
3. Critical Findings
3.1 Potential Issues
-
🔴 Critical Issues
- Security vulnerabilities: The execution of user-provided scripts introduces potential security risks. Ensuring the security of the sandbox and preventing any sandbox escape or unauthorized access is critical.
- Impact: Compromised security could lead to unauthorized access or data breaches.
- Recommendation: Conduct thorough security testing to verify sandbox isolation and prevent potential exploits.
- Performance degradation: The overhead of script execution, especially for frequent monitoring, could lead to performance degradation.
- Impact: Potential slowdown of the monitoring system, affecting overall performance.
- Recommendation: Evaluate the performance impact of script execution on monitor checks, especially under high load and with complex scripts. Measure response time overhead and resource consumption.
- Error handling complexity: User-provided scripts can introduce various error scenarios, including timeouts, syntax errors, and unexpected behavior.
- Impact: Complex error handling could lead to system instability.
- Recommendation: Test various error scenarios in user scripts (syntax errors, runtime errors, timeouts) and verify graceful error handling and informative error reporting.
- Security vulnerabilities: The execution of user-provided scripts introduces potential security risks. Ensuring the security of the sandbox and preventing any sandbox escape or unauthorized access is critical.
-
🟡 Warnings
- Potential risks: Resource exhaustion if user scripts are computationally intensive or contain infinite loops, impacting overall system stability.
- Suggested improvements: Implement additional safeguards to prevent resource exhaustion, such as more aggressive timeout and memory limits.
- Potential risks: Resource exhaustion if user scripts are computationally intensive or contain infinite loops, impacting overall system stability.
3.2 Code Quality Concerns
- Maintainability aspects: The PR introduces new complexity with the addition of user-provided scripts. Ensuring maintainability will be crucial.
- Readability issues: The code is generally readable, but clear documentation will be essential for users to understand how to write effective scripts.
- Performance bottlenecks: The performance impact of script execution needs to be carefully monitored and mitigated.
4. Security Assessment
- Authentication/Authorization impacts: No direct impacts on authentication or authorization.
- Data handling concerns: User-provided scripts handle response data, which needs to be secured.
- Input validation: Ensure robust validation and sanitization of user-provided scripts to prevent injection attacks.
- Security best practices: Follow best practices for sandboxing and isolating user-provided code.
- Potential security risks: The
sandbox.jscomponent and the usage ofisolated-vmare security-sensitive. Ensure robust isolation and prevent any sandbox escape or unauthorized access. - Mitigation strategies: Conduct thorough security testing to verify sandbox isolation and prevent potential exploits.
- Security testing requirements: Implement security tests to validate the sandboxing mechanism and ensure it prevents unauthorized access or data breaches.
5. Testing Strategy
5.1 Test Coverage
- Unit test analysis: Write unit tests to specifically target the sandbox functionality, focusing on isolation, security boundaries, and error handling.
- Integration test requirements: Create integration tests to verify the correct flow of data and control between
requestHttpandsandbox.js, ensuring proper status determination based on script output. - Edge cases coverage: Test various edge cases, including different types of HTTP responses and script execution scenarios.
5.2 Test Recommendations
Suggested Test Cases
// Example unit test for sandbox.js
const { runInSandbox, TimeoutError, UserScriptError } = require('./sandbox');
test('should execute script successfully', async () => {
const script = 'return res.code === 200;';
const param = { code: 200 };
const result = await runInSandbox(script, param);
expect(result).toBe(true);
});
test('should handle script timeout', async () => {
const script = 'while(true){}';
const param = {};
await expect(runInSandbox(script, param)).rejects.toThrow(TimeoutError);
});
test('should handle script syntax error', async () => {
const script = 'return res.code ===';
const param = { code: 200 };
await expect(runInSandbox(script, param)).rejects.toThrow(UserScriptError);
});- Coverage improvements: Ensure comprehensive test coverage for all potential script execution scenarios.
- Performance testing needs: Conduct performance testing to evaluate the impact of script execution on system performance.
6. Documentation & Maintenance
- Documentation updates needed: Update the documentation to include detailed information about the scripting environment, available variables (
res), expected input data format, limitations (timeout, memory limit), and error handling. - Long-term maintenance considerations: Monitor the performance and security of the scripting feature over time. Implement additional safeguards as needed to prevent resource exhaustion or security vulnerabilities.
- Technical debt and monitoring requirements: Regularly review and update the scripting feature to address any technical debt and ensure it remains secure and performant.
7. Deployment & Operations
- Deployment impact and strategy: The introduction of user-provided scripts requires careful deployment planning to ensure security and performance are maintained.
- Key operational considerations: Monitor the system for any performance degradation or security issues related to script execution.
8. Summary & Recommendations
8.1 Key Action Items
- Implement the suggested improvements for
testScriptsvalidation inMonitor.js. - Enhance error handling in
networkService.jsas suggested. - Conduct thorough security testing for the sandboxing mechanism.
- Update documentation to provide clear guidelines for users writing scripts.
8.2 Future Considerations
- Technical evolution path: Continuously improve the scripting feature based on user feedback and performance monitoring.
- Business capability evolution: Explore additional use cases for user-provided scripts to enhance monitoring capabilities.
- System integration impacts: Ensure the scripting feature integrates seamlessly with other system components and monitoring workflows.
💡 Help Shape LlamaPReview
How's this review format working for you? Vote in our Github Discussion Polls to help us improve your review experience!
YinDongFang
force-pushed
the
feature/http-json
branch
from
1dbebf0 to
a656b4b
Compare
YinDongFang
changed the title
|
Warning Rate limit exceeded@YinDongFang has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
WalkthroughThe pull request introduces changes to support the execution of test scripts within network monitoring jobs. A new Changes
Sequence DiagramsequenceDiagram
participant Client
participant NetworkService
participant Sandbox
Client->>NetworkService: Request HTTP Job
NetworkService->>NetworkService: Perform HTTP Request
alt Test Scripts Exist
NetworkService->>Sandbox: Run Test Script
Sandbox-->>NetworkService: Validation Result
NetworkService->>NetworkService: Set Response Status
else No Test Scripts
NetworkService->>NetworkService: Set Default Status
end
NetworkService-->>Client: Return Response
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Server/utils/sandbox.js (1)
3-5: Your constructor might be a bit too plain, slick.
Yo, with your palms sweaty, that constructor’s just callin’ the parent without added logic. You could safely drop it to keep your code lean and prevent faint knees.Here's a proposed diff to remove the no-op constructor:
-export class TimeoutError extends Error { - constructor(message) { super(message); } -} +export class TimeoutError extends Error {}🧰 Tools
🪛 Biome (1.9.4)
[error] 4-4: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
Server/db/models/Monitor.js (1)
41-43: Add constraints to avoid slipping up.
Storing lengthy scripts without limits might weigh you down like heavy arms. A max-length or separate handling for large scripts could keep the flow smooth.Server/validation/joi.js (1)
Line range hint
197-206: Consider stifling monstrous scripts early on.
Requiring minimal constraints (like.max(10000)) could keep your code from drowning in mom's spaghetti, especially if a user tries to stuff in a novel.
🛑 Comments failed to post (1)
Server/utils/sandbox.js (1)
34-35:
⚠️ Potential issueWatch out for multi-digit lines, champ.
If your script becomes bigger than 9 lines, this single\dcapturing group will drop spaghetti on your code. Increase it to\d+to capture multi-digit line numbers before mom's spaghetti hits the fan.Here's a suggested fix:
-const [, row, col] = lines[lines.length - 1].match(/\\sat\\s\\<isolated-vm\\>\\:(\\d)\\:(\\d)/); +const [, row, col] = lines[lines.length - 1].match(/\\sat\\s\\<isolated-vm\\>\\:(\\d+)\\:(\\d+)/);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const [, row, col] = lines[lines.length - 1].match(/\\sat\\s\\<isolated-vm\\>\\:(\\d+)\\:(\\d+)/); throw new Error("UserScriptError:" + row + ":" + col + ":" + e.message);
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
Server/utils/sandbox.js (1)
3-5: Yo dawg, let's simplify this error class!The constructor is unnecessary since it only calls super without adding any additional functionality.
-export class TimeoutError extends Error { - constructor(message) { super(message); } -} +export class TimeoutError extends Error {}🧰 Tools
🪛 Biome (1.9.4)
[error] 4-4: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
Server/service/networkService.js (1)
139-139: Keep that response code clean, no sauce stains!The
codeproperty should be set to a default value whenresponseis undefined.- code: response.status, + code: response?.status ?? this.NETWORK_ERROR,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
Server/db/models/Monitor.js(1 hunks)Server/package.json(1 hunks)Server/service/networkService.js(3 hunks)Server/utils/sandbox.js(1 hunks)Server/validation/joi.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Server/package.json
- Server/db/models/Monitor.js
- Server/validation/joi.js
🧰 Additional context used
🪛 Biome (1.9.4)
Server/utils/sandbox.js
[error] 4-4: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
Server/utils/sandbox.js
Outdated
| } | ||
| } | ||
|
|
||
| export default async function(code, param) { |
There was a problem hiding this comment.
Mom's spaghetti moment: Add input validation!
The function should validate the code parameter to ensure it's a non-empty string before execution.
export default async function(code, param) {
+ if (typeof code !== 'string' || !code.trim()) {
+ throw new Error('Code must be a non-empty string');
+ }
// Create a new isolate limited to 128MB
const isolate = new ivm.Isolate({ memoryLimit: 128 });Also applies to: 17-17
| const script = | ||
| `const res = $0; | ||
| try { | ||
| ${code} | ||
| } catch (e) { | ||
| const lines = e.stack.split("\\n"); | ||
| const [, row, col] = lines[lines.length - 1].match(/\\sat\\s\\<isolated-vm\\>\\:(\\d)\\:(\\d)/); | ||
| throw new Error("UserScriptError:" + row + ":" + col + ":" + e.message); | ||
| } | ||
| `; |
There was a problem hiding this comment.
Secure that script like your mom's secret recipe!
The script template should be hardened against potential injection attempts:
- Add strict mode to prevent unsafe operations
- Freeze the
resobject to prevent modification
const script =
- `const res = $0;
+ `'use strict';
+ const res = Object.freeze($0);
try {
${code}
} catch (e) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const script = | |
| `const res = $0; | |
| try { | |
| ${code} | |
| } catch (e) { | |
| const lines = e.stack.split("\\n"); | |
| const [, row, col] = lines[lines.length - 1].match(/\\sat\\s\\<isolated-vm\\>\\:(\\d)\\:(\\d)/); | |
| throw new Error("UserScriptError:" + row + ":" + col + ":" + e.message); | |
| } | |
| `; | |
| const script = | |
| `'use strict'; | |
| const res = Object.freeze($0); | |
| try { | |
| ${code} | |
| } catch (e) { | |
| const lines = e.stack.split("\\n"); | |
| const [, row, col] = lines[lines.length - 1].match(/\\sat\\s\\<isolated-vm\\>\\:(\\d)\\:(\\d)/); | |
| throw new Error("UserScriptError:" + row + ":" + col + ":" + e.message); | |
| } | |
| `; |
| const isolate = new ivm.Isolate({ memoryLimit: 128 }); | ||
|
|
||
| // Create a new context within this isolate. Each context has its own copy of all the builtin Objects. | ||
| // So for instance if one context does Object.prototype.foo = 1 this would not affect any other contexts. | ||
| const context = isolate.createContextSync(); |
There was a problem hiding this comment.
Cleanup after yourself, don't leave a mess!
The isolate should be disposed of after execution to prevent memory leaks.
const isolate = new ivm.Isolate({ memoryLimit: 128 });
+ try {
// Create a new context within this isolate. Each context has its own copy of all the builtin Objects.
// So for instance if one context does Object.prototype.foo = 1 this would not affect any other contexts.
const context = isolate.createContextSync();
+ } finally {
+ // Ensure isolate is disposed even if context creation fails
+ await isolate.dispose();
+ }Committable suggestion skipped: line range outside the PR's diff.
Server/service/networkService.js
Outdated
| const testScripts = job.data.testScripts; | ||
| if (testScripts) { | ||
| const status = await runInSandbox(testScripts, response?.data); | ||
| httpResponse.status = !!status; | ||
| } else { | ||
| httpResponse.status = true; | ||
| } |
There was a problem hiding this comment.
Don't let that spaghetti code slip through your fingers!
The sandbox execution needs proper error handling and type validation:
- Sandbox errors should be caught and handled gracefully
- The status should be explicitly validated as boolean
const testScripts = job.data.testScripts;
if (testScripts) {
- const status = await runInSandbox(testScripts, response?.data);
- httpResponse.status = !!status;
+ try {
+ const status = await runInSandbox(testScripts, response?.data);
+ if (typeof status !== 'boolean') {
+ throw new Error('Script must return a boolean value');
+ }
+ httpResponse.status = status;
+ } catch (error) {
+ httpResponse.status = false;
+ httpResponse.message = `Script execution failed: ${error.message}`;
+ }
} else {
httpResponse.status = true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const testScripts = job.data.testScripts; | |
| if (testScripts) { | |
| const status = await runInSandbox(testScripts, response?.data); | |
| httpResponse.status = !!status; | |
| } else { | |
| httpResponse.status = true; | |
| } | |
| const testScripts = job.data.testScripts; | |
| if (testScripts) { | |
| try { | |
| const status = await runInSandbox(testScripts, response?.data); | |
| if (typeof status !== 'boolean') { | |
| throw new Error('Script must return a boolean value'); | |
| } | |
| httpResponse.status = status; | |
| } catch (error) { | |
| httpResponse.status = false; | |
| httpResponse.message = `Script execution failed: ${error.message}`; | |
| } | |
| } else { | |
| httpResponse.status = true; | |
| } |
YinDongFang
force-pushed
the
feature/http-json
branch
from
a656b4b to
6c0c5e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Server/utils/sandbox.js (1)
3-5: Yo! Drop that constructor like it's hot!The constructor is unnecessary since it only calls super with no additional logic. Let the base Error class handle it!
-export class TimeoutError extends Error { - constructor(message) { super(message); } -} +export class TimeoutError extends Error {}🧰 Tools
🪛 Biome (1.9.4)
[error] 4-4: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
Server/db/models/Monitor.js(1 hunks)Server/package.json(1 hunks)Server/service/networkService.js(3 hunks)Server/utils/sandbox.js(1 hunks)Server/validation/joi.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Server/package.json
- Server/validation/joi.js
- Server/db/models/Monitor.js
🧰 Additional context used
🪛 Biome (1.9.4)
Server/utils/sandbox.js
[error] 4-4: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🔇 Additional comments (6)
Server/utils/sandbox.js (4)
7-13: Clean implementation, mom would be proud!The UserScriptError class properly extends Error and adds useful location information for debugging user scripts.
42-54:⚠️ Potential issueMom's spaghetti moment: Add input validation!
The function should validate the
codeparameter to ensure it's a non-empty string before execution.export default async function(code, param) { + if (typeof code !== 'string' || !code.trim()) { + throw new Error('Code must be a non-empty string'); + } // Create a new isolate limited to 128MBLikely invalid or redundant comment.
28-37: 🛠️ Refactor suggestionSecure that script like your mom's secret recipe!
The script template needs additional security measures:
- Add strict mode to prevent unsafe operations
- Freeze the
resobject to prevent modificationconst script = - `const res = $0; + `'use strict'; + const res = Object.freeze($0); try { ${code} } catch (e) {Likely invalid or redundant comment.
15-21:⚠️ Potential issueDon't let those resources slip through your fingers!
The isolate and context should be properly disposed of to prevent memory leaks.
export default async function (code, param) { // Create a new isolate limited to 128MB const isolate = new ivm.Isolate({ memoryLimit: 128 }); + try { // Create a new context within this isolate. Each context has its own copy of all the builtin Objects. // So for instance if one context does Object.prototype.foo = 1 this would not affect any other contexts. const context = isolate.createContextSync(); + } finally { + await isolate.dispose(); + }Likely invalid or redundant comment.
Server/service/networkService.js (2)
139-139: Clean as mom's kitchen!The addition of the response code is properly implemented and consistent with the error handling pattern.
150-178: 🛠️ Refactor suggestionDon't let that spaghetti code slip through your fingers!
The test scripts execution needs improvement in several areas:
- Validate that the script returns a boolean
- Provide more detailed error messages
- Avoid implicit boolean coercion
if (testScripts) { try { const status = await runInSandbox(testScripts, response?.data); - httpResponse.status = !!status; + if (typeof status !== 'boolean') { + throw new Error('Script must return a boolean value'); + } + httpResponse.status = status; httpResponse.message = this.http.STATUS_CODES[response.status]; } catch (error) { httpResponse.status = false; if (error instanceof TimeoutError) { - httpResponse.message = "User Scripts Timeout"; + httpResponse.message = "User script execution timed out after 1 second"; } else if (error instanceof UserScriptError) { - httpResponse.message = "User Scripts Runtime Error"; + httpResponse.message = `User script error at line ${error.row}, column ${error.col}`; } else { httpResponse.message = "System Error"; }Likely invalid or redundant comment.
YinDongFang
force-pushed
the
feature/http-json
branch
2 times, most recently
from
f5634ab to
27c931b
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Client/src/Pages/Uptime/CreateUptime/index.jsx (1)
112-112: Yo! Let's optimize that delete operation!Using the delete operator can impact performance. Consider using undefined assignment instead.
- if (!form.testScripts) delete form.testScripts; + if (!form.testScripts) form.testScripts = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 112-112: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Server/utils/sandbox.js (1)
3-5: Clean up that TimeoutError class, it's getting messy!The custom constructor is unnecessary since it's not adding any functionality beyond the parent class.
-export class TimeoutError extends Error { - constructor(message) { super(message); } -} +export class TimeoutError extends Error {}🧰 Tools
🪛 Biome (1.9.4)
[error] 4-4: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
Server/service/networkService.js (1)
150-178: Clean up that boolean coercion, it's getting messy!The double bang (!!) operator for boolean coercion could be more explicit.
- httpResponse.status = !!status; + if (typeof status !== 'boolean') { + throw new Error('Script must return a boolean value'); + } + httpResponse.status = status;Also, consider adding more descriptive error messages that include the actual error details:
- httpResponse.message = "User Scripts Runtime Error"; + httpResponse.message = `User Scripts Runtime Error: ${error.message}`;
🛑 Comments failed to post (1)
Client/src/Pages/Uptime/CreateUptime/index.jsx (1)
287-297:
⚠️ Potential issueMom's spaghetti moment: Fix those TextInput props!
The TextInput component for testScripts has several issues:
- Error handling is incorrectly using the 'name' field's errors
- Placeholder is using the name field's placeholder
<TextInput type="text" id="testScripts" label="testScripts" isOptional={true} - placeholder={monitorTypeMaps[monitor.type].namePlaceholder || ""} + placeholder="return res.status === 200;" value={monitor.testScripts} onChange={(event) => handleChange(event, "testScripts")} - error={errors["name"] ? true : false} - helperText={errors["name"]} + error={errors["testScripts"] ? true : false} + helperText={errors["testScripts"]} />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<TextInput type="text" id="testScripts" label="testScripts" isOptional={true} placeholder="return res.status === 200;" value={monitor.testScripts} onChange={(event) => handleChange(event, "testScripts")} error={errors["testScripts"] ? true : false} helperText={errors["testScripts"]} />🧰 Tools
🪛 Biome (1.9.4)
[error] 295-295: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
YinDongFang
force-pushed
the
feature/http-json
branch
from
27c931b to
e624988
Compare
There was a problem hiding this comment.
This is a potentially very dangerous pull request. Executing user provided is not something to be taken lightly.
I am umfailiar with the sandbox library used here, and thus I am not prepared to approve this PR.
If you have some resources that can show me that this library is absolutely bulletproof and that you've thoroughly tested this implementation we can look at it.
I appreciate the contribution very much, however this is probably the most dangerous thing you can do in a web based application so we must be thorough here 😂
|
Thank you for your feedback. From what I initially learned, the
Executing untrusted code is dangerous, and I cannot show you absolute safety (just like the vulnerability that was discovered in For more security, perhaps could use Alternatively, if there are any other suggestions/possible solutions, please feel free to share them, and I would be happy to do more research. If there are other implementation approaches for this issue (such as avoiding user code execution), Maybe we can discuss further in the issue. |
Hi @YinDongFang, Thanks for sharing some resources there, I will do some reading. As this project is mainly meant to be a self hosted solution I am concerned about users running untrusted code on their own machines. I'll dig into those resources as soon as I have a chance to do so and we can touch base again there and see if we agree that security is sufficient. Thanks for your contribution and I appreaciate your patience, we have to be careful as we really don't make a mistake with this one 👍 |
Test Cases （UI change is only for test in local, not included in PR）
(with error logs)
(with error logs)
Describe your changes
sandbox.jsfor securely executing user scripts.testScriptspoperty to Monitor to store custom scripts.requestHttpfunction to execute the script and determine the result status iftestScriptsis not empty.Issue number
#1573
Please ensure all items are checked off before requesting a review. "Checked off" means you need to add an "x" character between brackets so they turn into checkmarks.