PROD-1420 adding whmcs module for chainstack saas v1#1
Open
redikultsevsilver wants to merge 1 commit into
Open
PROD-1420 adding whmcs module for chainstack saas v1#1redikultsevsilver wants to merge 1 commit into
redikultsevsilver wants to merge 1 commit into
Conversation
|
|
Warning Review limit reached
More reviews will be available in 42 minutes and 4 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThis PR adds a Chainstack WHMCS provisioning module with API client, provisioning flow, WHMCS lifecycle handlers, client/admin rendering, network sync scripts, and PHPUnit coverage. ChangesChainstack WHMCS Module
Sequence Diagram(s)sequenceDiagram
participant WHMCS
participant Provisioner
participant ChainstackClient
participant ChainstackAPI
WHMCS->>Provisioner: provision()
Provisioner->>ChainstackClient: getDeploymentOptions()
ChainstackClient->>ChainstackAPI: GET deployment options
ChainstackAPI-->>ChainstackClient: options
Provisioner->>ChainstackClient: createProject()
ChainstackClient->>ChainstackAPI: POST project
ChainstackAPI-->>ChainstackClient: project id
Provisioner->>ChainstackClient: createNode()
ChainstackClient->>ChainstackAPI: POST node
ChainstackAPI-->>ChainstackClient: node id + protocol + status
Provisioner-->>WHMCS: persisted project/node state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
.gitignore (1)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
composer.lockversioned for reproducible installs.Ignoring the lockfile makes
composer installresolve a different dependency tree over time. Please removecomposer.lockfrom.gitignore.♻️ Proposed fix
/vendor/ -composer.lock .DS_Store🤖 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 @.gitignore around lines 1 - 2, The `.gitignore` file currently includes `composer.lock` which prevents the composer lockfile from being tracked in version control. Remove the `composer.lock` line from `.gitignore` to ensure the lockfile is versioned. This allows other developers to install the exact same dependency versions by running composer install, ensuring reproducible builds across the team.
🤖 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 `@composer.json`:
- Around line 5-10: The package declares PHP >=8.1 as the minimum requirement,
but PHPUnit 11.0 requires PHP 8.2 or higher, creating a version mismatch. Update
the "php" constraint in the "require" section of composer.json from >=8.1 to
>=8.2 to align the package's minimum PHP version with the actual minimum PHP
version required by the phpunit/phpunit ^11.0 dev dependency.
In `@modules/servers/chainstack/chainstack.php`:
- Around line 62-63: The TestConnection function's error handling path logs the
raw $params array to logModuleCall, which exposes the serverpassword (Chainstack
API key) in WHMCS module logs. Before calling logModuleCall in the catch block,
create a sanitized copy of $params where you unset or redact the serverpassword
field, then pass this sanitized array to logModuleCall instead of the raw
$params. Reference the logModuleCall invocation on line 62 where __FUNCTION__ is
used, and follow the same credential redaction pattern used by other handlers in
the module.
In `@modules/servers/chainstack/lib/ChainstackClient.php`:
- Around line 98-136: The code lacks explicit error handling for JSON
serialization and deserialization operations. Add error checking after the
json_encode call when setting CURLOPT_POSTFIELDS to verify the encoding
succeeded before passing it to curl. Additionally, after the json_decode call
that processes the raw response, check if the decoding actually succeeded by
verifying the result is valid JSON (not just an empty array coercion) before
proceeding to access array keys in the error handling logic below. This will
ensure that API contract violations are properly surfaced rather than causing
misleading downstream failures due to missing expected fields.
In `@modules/servers/chainstack/lib/Helpers.php`:
- Around line 137-156: The propGet and propSet methods are silently catching and
ignoring all exceptions from serviceProperties get and save operations, which
masks failures and can cause state to appear persisted when it actually failed.
Remove the try-catch blocks or re-throw the caught exceptions in the catch
clauses of both propGet and propSet methods so that callers are properly
notified when serviceProperties operations fail, ensuring idempotency and
cleanup guarantees are maintained.
In `@modules/servers/chainstack/lib/Provisioner.php`:
- Around line 99-103: The deleteEndpoint() method calls client->deleteNode()
without handling the case where the node has already been deleted externally
(404 error). Wrap the deleteNode() call in a try-catch block to catch HTTP
exceptions representing 404 Not Found responses, and ensure that
Helpers::removeNodeId() is still called to clear the local state even when the
remote node no longer exists. This way, externally deleted nodes won't leave
stale local IDs and won't surface avoidable errors.
- Around line 175-178: In the foreach loop that iterates over $list, after the
strcasecmp condition matches the network slug, the code currently returns
[$opt['blockchain'], $opt['cloud']] without validating that both keys exist in
the $opt array. Add validation to check that both 'blockchain' and 'cloud' keys
are set in $opt before returning them. If either key is missing, either skip to
the next iteration or handle the error appropriately to prevent invalid data
from being returned to the provisioning process.
In `@modules/servers/chainstack/templates/clientarea.tpl`:
- Around line 7-63: Unescaped template variables in the clientarea.tpl template
create XSS vulnerabilities where malicious content from the Chainstack API
response could be injected into the HTML. Apply the |escape modifier to all
dynamic variables throughout the template: escape $error, $node.name, $node.id,
and $node.status in HTML contexts using the default escape filter; escape
$node.protocol and $iconBase in URL/path contexts using |escape:'url'; escape
$node.https, $node.wss, and $node.beacon in HTML attribute contexts; and escape
the imploded namespaces list. Follow the same escaping pattern already
implemented in chainstack.php's admin area to maintain consistency across the
module.
In `@tests/e2e_harness.php`:
- Around line 54-77: The code creates live resources with
chainstack_CreateAccount but the cleanup using chainstack_TerminateAccount only
executes if all preceding operations succeed. Wrap the entire block starting
from chainstack_CreateAccount through chainstack_UnsuspendAccount in a try
block, and move the chainstack_TerminateAccount call with its associated output
statements into a finally block to guarantee cleanup executes regardless of any
exceptions that occur during resource creation or operation testing.
- Around line 61-73: The code logs full endpoint URLs directly in echo
statements for both $urlBefore and $urlAfter which may contain sensitive access
tokens. Create a helper function to redact the URLs by masking any path tokens
or sensitive query parameters, then apply this redaction function to both the
echo statement that prints "ClientArea https (before) = $urlBefore" and the echo
statement that prints "ClientArea https (after) = $urlAfter". This ensures that
sensitive access-bearing tokens are not exposed in stdout or CI logs.
---
Nitpick comments:
In @.gitignore:
- Around line 1-2: The `.gitignore` file currently includes `composer.lock`
which prevents the composer lockfile from being tracked in version control.
Remove the `composer.lock` line from `.gitignore` to ensure the lockfile is
versioned. This allows other developers to install the exact same dependency
versions by running composer install, ensuring reproducible builds across the
team.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d6d71e6-9b03-49ee-a94c-b01c0889fb2d
📒 Files selected for processing (17)
.gitignoreREADME.mdcomposer.jsonmodules/servers/chainstack/README.mdmodules/servers/chainstack/chainstack.phpmodules/servers/chainstack/hooks.phpmodules/servers/chainstack/lib/ChainstackClient.phpmodules/servers/chainstack/lib/Helpers.phpmodules/servers/chainstack/lib/Provisioner.phpmodules/servers/chainstack/scripts/list_networks.phpmodules/servers/chainstack/scripts/setup_network_option.phpmodules/servers/chainstack/templates/clientarea.tplphpunit.xmltests/HelpersTest.phptests/ProvisionerTest.phptests/bootstrap.phptests/e2e_harness.php
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/e2e_harness.php (1)
61-72: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRedact endpoint URLs before logging.
Line 61 and Line 71 still emit full endpoint URLs to stdout. Those URLs can be access-bearing and should be masked in logs.
Proposed fix
+function redactEndpoint(?string $url): string +{ + if (!$url) { + return ''; + } + $parts = parse_url($url); + if (!is_array($parts) || empty($parts['scheme']) || empty($parts['host'])) { + return '[invalid-endpoint]'; + } + return $parts['scheme'] . '://' . $parts['host'] . '/...'; +} + $ca = chainstack_ClientArea($params); $urlBefore = $ca['vars']['nodes'][0]['https'] ?? null; -echo "ClientArea https (before) = $urlBefore\n$line\n"; +echo "ClientArea https (before) = " . redactEndpoint($urlBefore) . "\n$line\n"; @@ $caUns = chainstack_ClientArea($params); $urlAfter = $caUns['vars']['nodes'][0]['https'] ?? null; -echo " ClientArea https (after) = $urlAfter\n"; +echo " ClientArea https (after) = " . redactEndpoint($urlAfter) . "\n";🤖 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 `@tests/e2e_harness.php` around lines 61 - 72, The test harness logging in chainstack_ClientArea / chainstack_UnsuspendAccount output is printing full endpoint URLs to stdout, which can leak access-bearing data. Update the e2e_harness.php logging around the “ClientArea https” and “URL changed” checks to redact or mask the URL values before echoing them, while keeping the suspend/unsuspend flow and comparison behavior intact.
🤖 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.
Duplicate comments:
In `@tests/e2e_harness.php`:
- Around line 61-72: The test harness logging in chainstack_ClientArea /
chainstack_UnsuspendAccount output is printing full endpoint URLs to stdout,
which can leak access-bearing data. Update the e2e_harness.php logging around
the “ClientArea https” and “URL changed” checks to redact or mask the URL values
before echoing them, while keeping the suspend/unsuspend flow and comparison
behavior intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73907c9c-15a9-48d3-b5fc-4a1c715d25e9
📒 Files selected for processing (7)
composer.jsonmodules/servers/chainstack/chainstack.phpmodules/servers/chainstack/lib/ChainstackClient.phpmodules/servers/chainstack/lib/Helpers.phpmodules/servers/chainstack/lib/Provisioner.phpmodules/servers/chainstack/templates/clientarea.tpltests/e2e_harness.php
🚧 Files skipped from review as they are similar to previous changes (3)
- composer.json
- modules/servers/chainstack/lib/ChainstackClient.php
- modules/servers/chainstack/lib/Provisioner.php
redikultsevsilver
force-pushed
the
feature/PROD-1420-add-whmcs-module-v1
branch
from
375e813 to
004541e
Compare
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
1 participant
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.
Provisions and manages Chainstack RPC/WSS node endpoints from WHMCS.