OPSLAB-245: Enhance error management in stormshield #334
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances error management in the Stormshield adapter by adding XML response error checking and refactoring the curl command execution flow.
Changes:
- Moved return code arrays to global scope for reuse between functions
- Added new
is_error_xml()function to check for errors in SimpleXMLElement responses - Refactored curl command handling to store the curl command as a class property and added XML error validation after API calls
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| adapters/stormshield/nsrpc.php | Moved return code validation arrays to file scope and added new is_error_xml() function for XML response error checking |
| adapters/stormshield/connect.php | Refactored curl command execution to validate XML responses and store curl command as instance property for better error reporting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
adapters/stormshield/nsrpc.php
Outdated
| $rc = (int)$serverd['ret']; | ||
| if (empty($ok_return_code[$rc]) && empty($not_a_return_code[$rc])) |
There was a problem hiding this comment.
Type inconsistency in return code lookup. Line 135 casts the return code to int, but then line 136 uses this integer to look up values in arrays that have string keys ("100", "103", etc.). In PHP, array access with integer 100 will not match string key "100" in a reliable way. Either remove the (int) cast on line 135, or change the global array keys to integers to ensure consistent type handling.
| $ret = is_error_xml($this->response); | ||
| if ($ret !== false) | ||
| { | ||
| throw new SmsException("Response to API {$this->curl_cmd} Failed: \n$ret", ERR_SD_CMDFAILED, $origin); |
There was a problem hiding this comment.
Potential null reference error. The function is_error_xml() is called with $this->response which may be null or unset. If the HTTP response is empty and the status code is 204, the response object is never initialized (see lines 229-231), but the error checking at line 157 will still execute. Add a check to ensure $this->response is set before calling is_error_xml(), or handle null/unset values within the is_error_xml() function.
| $ret = is_error_xml($this->response); | |
| if ($ret !== false) | |
| { | |
| throw new SmsException("Response to API {$this->curl_cmd} Failed: \n$ret", ERR_SD_CMDFAILED, $origin); | |
| if ($this->response !== null && $this->response !== '') { | |
| $ret = is_error_xml($this->response); | |
| if ($ret !== false) | |
| { | |
| throw new SmsException("Response to API {$this->curl_cmd} Failed: \n$ret", ERR_SD_CMDFAILED, $origin); | |
| } |
| throw new SmsException ("$origin: Response to API {$this->curl_cmd} Failed, expected json received empty response, header $header", ERR_SD_CMDFAILED); | ||
| } | ||
| } | ||
| debug_dump($this->response, "HTTP RESPONSE:\n"); |
There was a problem hiding this comment.
Debug output moved to after potential exception. The debug_dump for "HTTP RESPONSE" has been moved from before the error checking (original position) to after the curl execution at line 233. This means that if an error occurs at lines 212-215 or 229-230, the response debug output will not be shown. Consider whether this is the intended behavior, as it may make debugging failed requests more difficult.
adapters/stormshield/nsrpc.php
Outdated
| } | ||
|
|
||
| /* | ||
| * Get return codes of SimpleXMLElement response from the ME |
There was a problem hiding this comment.
Incomplete or unclear documentation. The comment states the function gets return codes "from the ME", but "ME" is not defined or explained. Consider clarifying what "ME" stands for (e.g., "Managed Entity" or the appropriate term) to improve code documentation.
| * Get return codes of SimpleXMLElement response from the ME | |
| * Get return codes of a SimpleXMLElement response from the Management Engine (ME) |
adapters/stormshield/nsrpc.php
Outdated
| "111" => true, | ||
| ); | ||
|
|
||
| // intermediate return code, it is normaly followed by another return code |
There was a problem hiding this comment.
Typo in comment: 'normaly' should be 'normally'.
| // intermediate return code, it is normaly followed by another return code | |
| // intermediate return code, it is normally followed by another return code |
No description provided.