Update controller.js - #19
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds input validation for the operand2 parameter in the controller's calculate function. The change appears to address security concerns by validating that operand2 is a properly formatted number.
- Adds validation logic to check if operand2 exists and matches a numeric pattern
- Throws an error with the operand2 value if validation fails
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| !req.query.operand2.match(/^(-)?[0-9\.]+(e(-)?[0-9]+)?$/) || | ||
| req.query.operand2.replace(/[-0-9e]/g, '').length > 1) { | ||
| throw new Error("Invalid operand2: " + req.query.operand2); | ||
| } |
There was a problem hiding this comment.
The validation code is placed outside the function scope. This code should be moved inside the calculate function before the operation call to properly validate the input.
| } |
| if (!req.query.operand2 || | ||
| !req.query.operand2.match(/^(-)?[0-9\.]+(e(-)?[0-9]+)?$/) || | ||
| req.query.operand2.replace(/[-0-9e]/g, '').length > 1) { | ||
| throw new Error("Invalid operand2: " + req.query.operand2); |
There was a problem hiding this comment.
The error message includes the raw user input (req.query.operand2), which could lead to information disclosure or log injection attacks. Consider using a generic error message without exposing the actual input value.
| throw new Error("Invalid operand2: " + req.query.operand2); | |
| throw new Error("Invalid operand2"); |
| //Make a change | ||
| if (!req.query.operand2 || | ||
| !req.query.operand2.match(/^(-)?[0-9\.]+(e(-)?[0-9]+)?$/) || | ||
| req.query.operand2.replace(/[-0-9e]/g, '').length > 1) { |
There was a problem hiding this comment.
The regex pattern allows multiple decimal points (e.g., '1.2.3' would pass). The pattern should use a more restrictive regex like /^-?\d+(.\d+)?(e-?\d+)?$/i to properly validate decimal numbers.
| req.query.operand2.replace(/[-0-9e]/g, '').length > 1) { | |
| !req.query.operand2.match(/^-?\d+(\.\d+)?(e-?\d+)?$/i)) { |
| //Make a change | ||
| if (!req.query.operand2 || | ||
| !req.query.operand2.match(/^(-)?[0-9\.]+(e(-)?[0-9]+)?$/) || | ||
| req.query.operand2.replace(/[-0-9e]/g, '').length > 1) { |
There was a problem hiding this comment.
This validation logic is complex and unclear. The replace operation checks for multiple decimal points but is difficult to understand. Consider using a clearer validation approach or adding a comment explaining the purpose.
| req.query.operand2.replace(/[-0-9e]/g, '').length > 1) { | |
| // Validate operand2: must be a valid floating-point number (optionally negative, with optional scientific notation) | |
| if (!req.query.operand2 || | |
| !req.query.operand2.match(/^[-+]?(?:\d+\.?\d*|\.\d+)(?:e[-+]?\d+)?$/i)) { |
I made changes not sure if secure.