feat: UnraidCheckExec for Check OS Updates via UPC dropdown #1265
Conversation
- Added `UnraidCheckExec.php` to separate concerns between UnraidCheck and ReplaceKey, allowing for JSON responses. - Updated `unraidcheck` script to parse query strings for compatibility with the new class. - Modified `webgui.ts` to call `UnraidCheckExec.php` instead of `UnraidCheck.php` for update checks.
- Updated `UnraidCheckExec.php` to handle `altUrl` and `json` parameters for improved flexibility in OS update checks. - Modified `webgui.ts` to utilize the new `WebguiUnraidCheckExecPayload` interface, ensuring compatibility with the updated parameters.
… restore and preserve files - Added `UnraidCheckExec.php` to the list of files restored during plugin updates. - Updated preservation rules to prevent downgrade for `UnraidCheckExec.php`.
WalkthroughThe changes update the Unraid plugin by adding a new file reference for restoration and preservation. A new PHP class, Changes
Sequence Diagram(s)sequenceDiagram
participant WG as Web GUI
participant PHP as UnraidCheckExec
participant SC as unraidcheck Script
WG->>PHP: Call WebguiCheckForUpdate (new endpoint)
PHP->>PHP: Execute setupEnvironment() (set headers & prepare QUERY_STRING)
PHP->>SC: exec() call to run external script
SC-->>PHP: Return script output
PHP-->>WG: Return concatenated result to Web UI
Possibly related PRs
Suggested reviewers
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 (
|
elibosley
left a comment
elibosley
left a comment
There was a problem hiding this comment.
looks good but please ensure the script cannot be called with any sort of CLI injection!
...namix.unraid.net/usr/local/emhttp/plugins/dynamix.plugin.manager/include/UnraidCheckExec.php
Show resolved
Hide resolved
…tion - Added security headers to the setupEnvironment method for improved security. - Enhanced altUrl validation to ensure only allowed domains are accepted. - Updated script execution checks to include executable permission validation.
![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
♻️ Duplicate comments (1)
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.plugin.manager/include/UnraidCheckExec.php (1)
43-44:⚠️ Potential issuePotential command injection vulnerability
The unescaped use of http_build_query output in putenv could potentially lead to environment variable injection attacks if parameter values contain malicious characters.
Consider sanitizing or escaping the query string before setting it as an environment variable:
- putenv('QUERY_STRING=' . http_build_query($params)); + putenv('QUERY_STRING=' . escapeshellarg(http_build_query($params)));
🧹 Nitpick comments (2)
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.plugin.manager/include/UnraidCheckExec.php (2)
62-63: Consider JSON encoding the responseSince the headers indicate JSON content and the 'json' parameter is set to true, consider JSON encoding the output if it's not already in JSON format from the script.
- return implode("\n", $output); + // Return JSON string directly if script already outputs JSON + $outputString = implode("\n", $output); + // Verify it's valid JSON before returning + if (json_decode($outputString) === null && json_last_error() !== JSON_ERROR_NONE) { + // If not valid JSON, encode it + return json_encode(['output' => $outputString]); + } + return $outputString;
66-69: Consider using namespaces and autoloadingFor better maintainability and to avoid potential class name conflicts, consider using PHP namespaces. Also, the direct instantiation and execution at the bottom of the file makes unit testing more difficult.
<?php + namespace Dynamix\PluginManager; /** * This file exists to maintain separation of concerns between UnraidCheck and ReplaceKey. ... } - // Usage - $checker = new UnraidCheckExec(); - echo $checker->execute(); + // Only execute if this file is being accessed directly + if (basename(__FILE__) == basename($_SERVER['SCRIPT_FILENAME'])) { + $checker = new UnraidCheckExec(); + echo $checker->execute(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.plugin.manager/include/UnraidCheckExec.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test API
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (6)
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.plugin.manager/include/UnraidCheckExec.php (6)
1-11: Well-documented purpose and interfaceThe file header clearly explains the purpose of separation of concerns and documents the expected interface parameters. Good practice to include this documentation.
12-16: Good use of constants for configurationThe class properly defines constants for script path and allowed domain, making these values easy to update if needed.
17-23: Comprehensive security headersThe security headers are well-implemented to protect against common web vulnerabilities:
- Content-Type header prevents MIME sniffing attacks
- X-Frame-Options prevents clickjacking
- Content-Security-Policy restricts resource loading
24-41: Thorough URL validation for preventing SSRFThe URL validation logic correctly:
- Verifies the URL format
- Ensures HTTPS protocol is used
- Restricts to allowed domain and its subdomains
This is a strong defense against Server-Side Request Forgery attacks.
46-53: Good script validation checksThe code properly validates that the script exists, is readable, and is executable before attempting to execute it. This prevents potential errors and provides clear error messages.
57-60: Proper command execution with escapingUsing escapeshellcmd on the script path is a good security practice to prevent command injection. The error handling for failed execution is also appropriate.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
|
since this was preapproved before cursor helped with security items I'm gonna merge this in so I can get these into webgui repo. Tested extensively after the security additions. Good call on having me add them |
🤖 I have created a release *beep* *boop* --- ## [4.4.0](v4.3.1...v4.4.0) (2025-03-25) ### Features * add ReplaceKey functionality to plugin ([#1264](#1264)) ([4aadcef](4aadcef)) * downgrade page replace key check ([#1263](#1263)) ([8d56d12](8d56d12)) * make log viewer component dynamic ([#1242](#1242)) ([e6ec110](e6ec110)) * ReplaceKey functionality in Registration and Update pages ([#1246](#1246)) ([04307c9](04307c9)) * UnraidCheckExec for Check OS Updates via UPC dropdown ([#1265](#1265)) ([5935a3b](5935a3b)) ### Bug Fixes * **deps:** update all non-major dependencies ([#1236](#1236)) ([7194f85](7194f85)) * **deps:** update all non-major dependencies ([#1247](#1247)) ([20b0aeb](20b0aeb)) * **deps:** update all non-major dependencies ([#1251](#1251)) ([33a1a1d](33a1a1d)) * **deps:** update all non-major dependencies ([#1253](#1253)) ([53fec0e](53fec0e)) * **deps:** update dependency @nestjs/passport to v11 ([#1244](#1244)) ([edc93a9](edc93a9)) * **deps:** update dependency graphql-subscriptions to v3 ([#1209](#1209)) ([c14c85f](c14c85f)) * **deps:** update dependency ini to v5 ([#1217](#1217)) ([f27660f](f27660f)) * **deps:** update dependency jose to v6 ([#1248](#1248)) ([42e3d59](42e3d59)) * **deps:** update dependency marked to v15 ([#1249](#1249)) ([2b6693f](2b6693f)) * **deps:** update dependency pino-pretty to v13 ([#1250](#1250)) ([85fb910](85fb910)) * **deps:** update dependency pm2 to v6 ([#1258](#1258)) ([04ad2bc](04ad2bc)) * **deps:** update dependency shadcn-vue to v1 ([#1259](#1259)) ([1a4fe8f](1a4fe8f)) * **deps:** update dependency vue-i18n to v11 ([#1261](#1261)) ([0063286](0063286)) * **deps:** update vueuse monorepo to v13 (major) ([#1262](#1262)) ([94caae3](94caae3)) * make scripts executable when building the plugin ([#1255](#1255)) ([e237f38](e237f38)) * node installation not persisting across reboots ([#1256](#1256)) ([0415cf1](0415cf1)) * update configValid state to ineligible in var.ini and adjust rel… ([#1268](#1268)) ([ef8c954](ef8c954)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
UnraidCheckExec.phpto separate concerns between UnraidCheck and ReplaceKey, allowing for JSON responses.unraidcheckscript to parse query strings for compatibility with the new class.webgui.tsto callUnraidCheckExec.phpinstead ofUnraidCheck.phpfor update checks.Summary by CodeRabbit
New Features
Refactor