fix(occ): prevent params command key from overriding validated allowlist command#41577
fix(occ): prevent params command key from overriding validated allowlist command#41577DeepDiver1975 wants to merge 2 commits into
Conversation
…ist command OccController::execute() validated the URL-path $command against the allowlist, then called array_merge(["command"=>$command], $params). PHP's array_merge with string keys lets later values overwrite earlier ones, so a "command" key in the POST body $params silently replaced the validated command, allowing execution of any occ command. Add unset($params["command"]) before the merge to strip any attacker-supplied command key, ensuring the validated value is always what reaches Symfony Console. Signed-off-by: Thomas Müller <thomas.mueller@owncloud.com> Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
DeepDiver1975
left a comment
There was a problem hiding this comment.
Code Review — fix(occ): prevent params command key from overriding validated allowlist command
Overview: OccController::execute() validated $command (from the URL path) against an allowlist, then built the ArrayInput via array_merge(['command' => $command], $params). PHP's array_merge with string keys causes later values to overwrite earlier ones, so a command key in the POST body replaced the validated command entirely. Fix: unset($params['command']) before the merge.
Correctness
The fix is correct and minimal:
unset($params['command']);
$inputArray = \array_merge(['command' => $command], $params);After the unset, array_merge can never find a second command key in $params, so the validated value always wins. ✅
Why not use array_merge($params, ['command' => $command]) instead? Reversing the argument order would also work but would make the intent less obvious to a reader, and would still require reasoning about key collision. The explicit unset is clearer. ✅
Test
testParamsCommandKeyCannotOverrideValidatedCommand:
- Sends
'command' => 'user:resetpassword'in params while URL command is'status' - Captures the
ArrayInputactually passed toconsole->run()viawillReturnCallback - Asserts
$capturedInput->getFirstArgument() === 'status'✅ - Asserts
exitCode === 0(the allowed command ran successfully) ✅
This is a strong regression test — it directly asserts that the console receives the validated command, not the attacker-supplied one. Without the fix, getFirstArgument() would return 'user:resetpassword' and the test would fail. ✅
Summary
| Aspect | Assessment |
|---|---|
| Allowlist bypass | ✅ Closed — unset before merge |
| Fix scope | ✅ Minimal — one line |
| Test | ✅ Captures actual console input, asserts correct command |
| Changelog | ✅ Accurate |
Verdict: Ready to merge. Critical security fix.
Summary
OccController::execute()validated the URL-path$commandagainst the allowlist, then merged it with user-supplied$paramsviaarray_merge(['command' => $command], $params)array_mergewith string keys causes later duplicates to overwrite earlier ones — acommandkey in the POST body bypassed the allowlist entirelyunset($params['command'])before the merge; one-line change, complete protectionSecurity Impact
Critical — authenticated updater callers with
updater.secret+ localhost access can execute anyocccommand (user:resetpassword, config:system:set, encryption:decrypt-all, etc.) regardless of the allowlistTest plan
testParamsCommandKeyCannotOverrideValidatedCommandsendsuser:resetpasswordin params while URL saysstatus; asserts console receivesstatus— fails without fixmake test TEST_PHP_SUITE=core/Controller🤖 Generated with Claude Code