Skip to content

Harden ExecuteTask with per-arg escapeshellarg and allow-list#485

Merged
dereuromark merged 1 commit intomasterfrom
harden-execute-task-allowlist
May 2, 2026
Merged

Harden ExecuteTask with per-arg escapeshellarg and allow-list#485
dereuromark merged 1 commit intomasterfrom
harden-execute-task-allowlist

Conversation

@dereuromark
Copy link
Copy Markdown
Owner

Summary

Two-part hardening of Queue.Execute:

  • Switch from escapeshellcmd() to per-token escapeshellarg() for the command and each params entry. The previous primitive only neutralizes shell metacharacters and cannot defend against argument injection (e.g. a params entry like -r --some-flag /etc/passwd would still split into multiple shell tokens once exec() re-tokenized the line). With escapeshellarg() each entry is wrapped as one quoted token.
  • Introduce a Queue.executeAllowedCommands allow-list that is enforced whenever debug is disabled. With debug off and no allow-list configured, every Execute job is rejected before exec() is reached. This protects environments where an attacker gains DB write access to queued_jobs, or where upstream code unexpectedly pipes user input into createJob('Queue.Execute', ...).

Notes

  • Behavior change for callers that previously embedded multiple tokens inside a single params entry: they must now split such entries across the params array. The two existing tests that depended on the old behavior were updated to the supported shape.
  • New regression tests cover token quoting for an arg-injection payload and the allow-list deny/empty/allow paths.
  • Docs section docs/sections/tasks/execute.md updated to describe the new escaping primitive and the production allow-list requirement.

Verification

  • vendor/bin/phpunit — 176 passing
  • vendor/bin/phpstan analyze — no errors
  • vendor/bin/phpcs src/ — clean

The ExecuteTask previously concatenated `command` and each `params` entry
through `escapeshellcmd()`, which only neutralizes shell metacharacters
(`; & | $`, etc.) and does NOT defend against argument injection. A
payload like `params => ['-r --some-flag /etc/passwd']` would still split
across additional shell tokens once `exec()` re-tokenized the line.

Switch the escape primitive to per-token `escapeshellarg()` so each
argument is wrapped as a single shell token (`ExecuteTask.php:97-110`).
This is a behavior change for callers that previously embedded multiple
tokens inside a single `params` entry: they must now split such entries
across the `params` array.

Add a `Queue.executeAllowedCommands` allow-list that is enforced
whenever `debug` is disabled (`ExecuteTask.php:93-100`). With debug off
and no allow-list configured, every Execute job is rejected before
`exec()` is reached. This protects environments where an attacker
gains DB write access to `queued_jobs`, or where upstream code
unexpectedly pipes user input into `createJob('Queue.Execute', ...)`.

Update the existing tests to use the supported separated
command/params shape, and add four regression tests covering the
arg-injection token quoting and the allow-list deny/empty/allow paths
(`ExecuteTaskTest.php`). Refresh the docs section to describe the new
escaping primitive and the production allow-list requirement.
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 2, 2026

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 77.54%. Comparing base (34e8050) to head (a685e6e).

Files with missing lines Patch % Lines
src/Queue/Task/ExecuteTask.php 88.88% 1 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #485      +/-   ##
============================================
+ Coverage     77.38%   77.54%   +0.16%     
- Complexity      963      966       +3     
============================================
  Files            45       45              
  Lines          3232     3238       +6     
============================================
+ Hits           2501     2511      +10     
+ Misses          731      727       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dereuromark dereuromark merged commit 6443a03 into master May 2, 2026
16 checks passed
@dereuromark dereuromark deleted the harden-execute-task-allowlist branch May 2, 2026 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants