feat: replace key improvements#2077
Conversation
* feat: ReplaceKey 7 day window around regExp & new forceCheck param * feat: Enhance writeJsonFile to support appending data
…n ReplaceKey * Removed appending functionality from writeJsonFile for cleaner implementation. * Inverted condition for forceCheck to improve logic clarity.
…tions * Added logic to process the response from KeyInstaller's installKey method. * Implemented error handling and notification system based on installation success or failure. * Notifications include event, subject, and description for better user feedback.
* Added `ReplaceKey` initialization and check to `Registration.page`, `Downgrade.page`, `Update.page`, and `unraidcheck` script. * Ensures consistent handling of key replacement across relevant components.
WalkthroughThe changes introduce a new dependency on the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Page as "Plugin Page (Registration/Update/Downgrade/UnraidCheck)"
participant ReplaceKey
participant KeyInstaller
User->>Page: Request page load
Page->>ReplaceKey: Instantiate & invoke check(force?)
ReplaceKey->>KeyInstaller: Optionally trigger key installation
KeyInstaller-->>ReplaceKey: Return installation response
ReplaceKey-->>Page: Return key check result
Page->>User: Continue with subsequent logic
Poem
🪧 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
CodeRabbit Configuration File (
|
* Added `UnraidCheckExec` class to handle execution of the unraidcheck script and return JSON responses. * Updated the unraidcheck script to parse query parameters for better integration with web components. * This change maintains separation of concerns between UnraidCheck and ReplaceKey functionalities.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emhttp/plugins/dynamix.plugin.manager/include/UnraidCheckExec.php (1)
31-37: Consider adding error handling for script execution.The execute method correctly calls setupEnvironment() and runs the script, but it lacks error handling for potential script execution failures. Consider checking the return value of exec() and handling cases where the script might not execute successfully.
public function execute(): string { $this->setupEnvironment(); $output = []; - exec(self::SCRIPT_PATH, $output); + $return_var = 0; + exec(self::SCRIPT_PATH, $output, $return_var); + if ($return_var !== 0) { + // Log error or handle failure + $output[] = json_encode(['error' => 'Failed to execute unraidcheck script']); + } return implode("\n", $output); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
emhttp/plugins/dynamix.plugin.manager/include/UnraidCheckExec.php(1 hunks)emhttp/plugins/dynamix.plugin.manager/scripts/unraidcheck(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/dynamix.plugin.manager/scripts/unraidcheck
🔇 Additional comments (4)
emhttp/plugins/dynamix.plugin.manager/include/UnraidCheckExec.php (4)
1-11: Well-documented class purpose and integration context.The docblock clearly explains the purpose of this file for maintaining separation of concerns between UnraidCheck and ReplaceKey, which aligns with the PR objectives. It provides valuable context about how this file integrates with other components through the WebguiCheckForUpdate function.
12-14: Good encapsulation of the script path constant.The SCRIPT_PATH constant is properly defined as private since it's an implementation detail of the class.
40-42: Simple and effective implementation of the class.The instantiation and execution of the UnraidCheckExec class is straightforward and follows good practices.
16-29:Details
❓ Verification inconclusive
Verify the security implications of the altUrl parameter.
The method correctly sets the Content-Type header and handles the altUrl parameter by validating it as a URL. However, consider implementing additional validation beyond FILTER_VALIDATE_URL to mitigate potential security risks if the unraidcheck script uses this URL in sensitive operations.
🏁 Script executed:
#!/bin/bash # Check how the unraidcheck script uses the altUrl parameter cat "/usr/local/emhttp/plugins/dynamix.plugin.manager/scripts/unraidcheck" | grep -A 10 "altUrl"Length of output: 195
Action Required: Verify altUrl Usage Manually
The
altUrlparameter is validated withFILTER_VALIDATE_URLand then passed via the environment variableQUERY_STRING. However, our verification attempt to locate the associated unraidcheck script at/usr/local/emhttp/plugins/dynamix.plugin.manager/scripts/unraidcheckdid not find any such file. This makes it unclear how—and if—the URL is used in sensitive operations.
- The unraidcheck script is missing at the expected location.
- Without seeing its usage of
altUrl, we cannot confirm whether additional validation or sanitization is needed.Please manually verify how the
altUrlparameter is handled in the broader unraidcheck workflow. If the script exists in a different location or if its usage is confirmed to be secure, update this section accordingly.
2 participants
Reflects recent changes to the same files in the
apirepoReplaceKey7 day window aroundregExp& newforceCheckparamReplaceKeynotification for installing auto-extended license keyReplaceKeycheck to/Tools/Downgrade,/Tools/Update, &/Tools/Registrationas they're directly related to a license key's OS Update eligibility date.KeyInstallerclass success responsesSummary by CodeRabbit