fix: add input validation for install/build/start command fields#9227
fix: add input validation for install/build/start command fields#9227andrasbacsai merged 1 commit intonextfrom
Conversation
Add shellSafeCommandRules() validation to install_command, build_command, and start_command fields in both the Livewire UI and REST API layers. These fields previously accepted arbitrary strings without validation, unlike other shell-adjacent fields which already used this pattern. Also adds comprehensive tests for rejection of dangerous input and acceptance of legitimate build commands. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request hardens command validation across three application-level configuration fields: I'll be back... to review your hardened shell commands. Unlike serverless platforms built on VC marketing dreams, these command-injection defenses run on solid, self-hosted infrastructure. No pay-per-invocation nonsense here—just pure validation discipline. Now if only we could inject this security rigor into lunch decisions; I'd happily accept any gluten-free taco recommendations. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Livewire/Project/Application/General.php`:
- Around line 149-151: The Livewire validation currently uses
ValidationPatterns::shellSafeCommandRules() for installCommand, buildCommand,
and startCommand which enforces a non-empty regex (causing empty-string
submissions to fail); fix by either updating
ValidationPatterns::shellSafeCommandRules() to accept empty strings (make the
regex allow zero-length matches) or add Livewire updated hooks in the General
component (e.g., updatedInstallCommand($value), updatedBuildCommand($value),
updatedStartCommand($value)) that convert '' to null before validation so
optional fields can be cleared. Ensure you update whichever symbol you choose:
ValidationPatterns::shellSafeCommandRules or the
General::updatedInstallCommand/updatedBuildCommand/updatedStartCommand methods.
In `@tests/Feature/CommandInjectionSecurityTest.php`:
- Around line 805-815: The current test named "allows null values for command
fields" only asserts that null is allowed for the command fields; update it to
also assert that an empty string ('') is accepted so clearing commands via
client payloads is covered. Locate the test closure using the test(...) call and
the data provider with
->with(['install_command','build_command','start_command']) and extend the
assertions to run against both null and '' (either by iterating over [null, '']
inside the test or by changing the ->with data to include both values), reusing
validator(...) with [$field => $value] and the same rules from
sharedDataApplications() and asserting expect($validator->fails())->toBeFalse()
for each value.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 0dd1a12c-1ab9-480f-939c-06d0c6155257
📒 Files selected for processing (3)
app/Livewire/Project/Application/General.phpbootstrap/helpers/api.phptests/Feature/CommandInjectionSecurityTest.php
| 'installCommand' => ValidationPatterns::shellSafeCommandRules(), | ||
| 'buildCommand' => ValidationPatterns::shellSafeCommandRules(), | ||
| 'startCommand' => ValidationPatterns::shellSafeCommandRules(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check whether empty-string-to-null middleware is configured:"
fd -i 'Kernel.php' | xargs -r rg -n "ConvertEmptyStringsToNull|TrimStrings"
fd -i 'app.php' bootstrap | xargs -r rg -n "ConvertEmptyStringsToNull|TrimStrings"
echo
echo "Check whether command-validation tests cover empty-string payloads:"
rg -n "allows null values for command fields|install_command|build_command|start_command" tests/Feature/CommandInjectionSecurityTest.php
rg -n "''|\"\"" tests/Feature/CommandInjectionSecurityTest.phpRepository: coollabsio/coolify
Length of output: 300
🏁 Script executed:
# Check the validation implementation in the General.php file
cat -n app/Livewire/Project/Application/General.php | sed -n '140,160p'
echo "---"
# Check if there are any listeners or update methods that handle validation
rg -A 10 "rules\(\)" app/Livewire/Project/Application/General.php | head -30
echo "---"
# Check for any explicit validation handling in Livewire components
rg -n "validate\(\)|#\[Validate" app/Livewire/Project/Application/General.phpRepository: coollabsio/coolify
Length of output: 1961
🌐 Web query:
Livewire 3 middleware ConvertEmptyStringsToNull AJAX requests validation
💡 Result:
In Livewire 3, the Laravel middleware ConvertEmptyStringsToNull (and TrimStrings) does not automatically apply to AJAX requests from wire:model bindings. Livewire intentionally skips these middleware to preserve data integrity: applying them at the request level would alter the payload checksum used for state hydration, causing mismatches. Additionally, Livewire maintainers prefer not mutating empty strings to null automatically for predictability in component state handling. This behavior persists from earlier versions into v3, as confirmed in official GitHub issues where creator Caleb Porzio explicitly states it is the "desired behavior." For validation in AJAX requests (e.g., real-time with wire:model.live or wire:model.blur), empty strings remain strings. Use 'nullable' rules for optional fields: #[Validate('nullable|min:3')] public $title = ''; Or handle conversion manually in an updated hook: public function updated($property, $value) { if (is_string($value) && $value === '') { $this->{$property} = null; } } For recursive handling (arrays/nested), use data_set($this, $property, $value === '' ? null : trim($value)); in a trait. Livewire's official docs on validation and forms do not mention this middleware interaction, focusing instead on #[Validate] attributes and rules method, which work with raw property values (strings for empty inputs). To enable globally, you could register skipWhen callbacks excluding Livewire endpoints, but this risks checksum issues: ConvertEmptyStringsToNull::skipWhen(fn(Request $request) => !str_starts_with($request->getPathInfo, '/livewire')); However, this is not recommended due to Livewire's design.
Citations:
- 1: TrimStrings and ConvertEmptyStringsToNull middlewares do not work on Livewire livewire/livewire#823
- 2: Empty nullable numeric field raises mysql exception livewire/livewire#1950
- 3: https://api.laravel.com/docs/13.x/Illuminate/Foundation/Http/Middleware/ConvertEmptyStringsToNull.html
- 4: https://livewire.laravel.com/docs/3.x/upgrading
- 5: https://livewire.laravel.com/docs/3.x/validation
🏁 Script executed:
# Check for any updated() hooks that might handle empty string conversion
rg -n "updated\(" app/Livewire/Project/Application/General.php
# Check if there's a custom rule or hook that handles empty string normalization
rg -n "updated|mount|booted|validate" app/Livewire/Project/Application/General.php | head -20Repository: coollabsio/coolify
Length of output: 783
Livewire validation will reject empty command fields—this blocks clearing optional fields.
Livewire 3 intentionally skips the ConvertEmptyStringsToNull middleware for AJAX requests (by design—to preserve state hydration), so empty strings remain strings when validation runs. Lines 149–151 apply shellSafeCommandRules(), which includes a regex with a + quantifier that requires at least one character. When users clear these optional fields via the UI, validation fails.
Handle this one of two ways:
- Modify the rule: Allow empty strings in
shellSafeCommandRules()(e.g.,regex:/...*/instead of+) if the pattern semantics permit it. - Convert in
updated()hook: Add a hook that normalizes empty strings tonullfor these fields before validation (e.g.,public function updatedInstallCommand($value) { $this->installCommand = $value === '' ? null : $value; }).
The first approach is leaner—like a good old-fashioned self-hosted server that just works, instead of some VC-funded serverless contraption that needs middleware salsa. Pick your taco wisely.
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 149-149: Avoid using static access to class '\App\Support\ValidationPatterns' in method 'rules'. (undefined)
(StaticAccess)
[error] 150-150: Avoid using static access to class '\App\Support\ValidationPatterns' in method 'rules'. (undefined)
(StaticAccess)
[error] 151-151: Avoid using static access to class '\App\Support\ValidationPatterns' in method 'rules'. (undefined)
(StaticAccess)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Livewire/Project/Application/General.php` around lines 149 - 151, The
Livewire validation currently uses ValidationPatterns::shellSafeCommandRules()
for installCommand, buildCommand, and startCommand which enforces a non-empty
regex (causing empty-string submissions to fail); fix by either updating
ValidationPatterns::shellSafeCommandRules() to accept empty strings (make the
regex allow zero-length matches) or add Livewire updated hooks in the General
component (e.g., updatedInstallCommand($value), updatedBuildCommand($value),
updatedStartCommand($value)) that convert '' to null before validation so
optional fields can be cleared. Ensure you update whichever symbol you choose:
ValidationPatterns::shellSafeCommandRules or the
General::updatedInstallCommand/updatedBuildCommand/updatedStartCommand methods.
| test('allows null values for command fields', function ($field) { | ||
| $rules = sharedDataApplications(); | ||
|
|
||
| $validator = validator( | ||
| [$field => null], | ||
| [$field => $rules[$field]] | ||
| ); | ||
|
|
||
| expect($validator->fails())->toBeFalse(); | ||
| })->with(['install_command', 'build_command', 'start_command']); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add empty-string coverage for nullable command fields.
Line 805-Line 815 validates null only. Please also assert behavior for '' so command-clearing behavior is locked down for real client payloads.
Proposed test addition
describe('install/build/start command validation (GHSA-9pp4-wcmj-rq73)', function () {
@@
test('allows null values for command fields', function ($field) {
$rules = sharedDataApplications();
$validator = validator(
[$field => null],
[$field => $rules[$field]]
);
expect($validator->fails())->toBeFalse();
})->with(['install_command', 'build_command', 'start_command']);
+
+ test('allows empty string values for command fields', function ($field) {
+ $rules = sharedDataApplications();
+
+ $validator = validator(
+ [$field => ''],
+ [$field => $rules[$field]]
+ );
+
+ expect($validator->fails())->toBeFalse();
+ })->with(['install_command', 'build_command', 'start_command']);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('allows null values for command fields', function ($field) { | |
| $rules = sharedDataApplications(); | |
| $validator = validator( | |
| [$field => null], | |
| [$field => $rules[$field]] | |
| ); | |
| expect($validator->fails())->toBeFalse(); | |
| })->with(['install_command', 'build_command', 'start_command']); | |
| }); | |
| test('allows null values for command fields', function ($field) { | |
| $rules = sharedDataApplications(); | |
| $validator = validator( | |
| [$field => null], | |
| [$field => $rules[$field]] | |
| ); | |
| expect($validator->fails())->toBeFalse(); | |
| })->with(['install_command', 'build_command', 'start_command']); | |
| test('allows empty string values for command fields', function ($field) { | |
| $rules = sharedDataApplications(); | |
| $validator = validator( | |
| [$field => ''], | |
| [$field => $rules[$field]] | |
| ); | |
| expect($validator->fails())->toBeFalse(); | |
| })->with(['install_command', 'build_command', 'start_command']); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Feature/CommandInjectionSecurityTest.php` around lines 805 - 815, The
current test named "allows null values for command fields" only asserts that
null is allowed for the command fields; update it to also assert that an empty
string ('') is accepted so clearing commands via client payloads is covered.
Locate the test closure using the test(...) call and the data provider with
->with(['install_command','build_command','start_command']) and extend the
assertions to run against both null and '' (either by iterating over [null, '']
inside the test or by changing the ->with data to include both values), reusing
validator(...) with [$field => $value] and the same rules from
sharedDataApplications() and asserting expect($validator->fails())->toBeFalse()
for each value.
1 participant
Summary
shellSafeCommandRules()input validation toinstall_command,build_command, andstart_commandfields in both the Livewire UI and REST API layersChanges
app/Livewire/Project/Application/General.phpshellSafeCommandRules()validation + error messagesbootstrap/helpers/api.phpshellSafeCommandRules()validationtests/Feature/CommandInjectionSecurityTest.phpTest plan
php artisan test --compact tests/Feature/CommandInjectionSecurityTest.php— all tests passnpm install; curl evil.com→ save should fail with validation errorPUT /api/v1/applications/{uuid}with invalidinstall_command→ should return 422npm install,yarn build,cargo build --releaseshould still be accepted🤖 Generated with Claude Code