fix(command): restrict allowed_classes in ClosureJob unserialize() to prevent RCE

[DeepDiver1975]

Summary

• ClosureJob::run() called bare unserialize() on DB-sourced data; same gadget-chain RCE risk as CommandJob
• The post-deserialization method_exists check doesn't help — __wakeup()/__destruct() already fired
• Fix adds allowed_classes restricted to the 6 laravel/serializable-closure classes that legitimately appear in serialized closure payloads

Security Impact

High (defense-in-depth) — requires prior DB write access to exploit

Test plan

• Legitimate closure still executes correctly after fix
• Malicious gadget payload's __wakeup() is NOT triggered (test would fail without fix)
• Run make test TEST_PHP_SUITE=lib/Command

🤖 Generated with Claude Code

[update-docs]

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.

[DeepDiver1975]

Code Review — fix(command): restrict allowed_classes in ClosureJob unserialize()

Overview: Adds allowed_classes restriction to unserialize() in ClosureJob::run(), limiting deserialization to the SerializableClosure class family. Prevents PHP Object Injection via gadget chains from attacker-controlled data in oc_jobs.

Correctness

The ALLOWED_CLASSES list is correct for SerializableClosure (the Laravel package used here). It includes:

• SerializableClosure and UnsignedSerializableClosure — the top-level wrappers
• Native and Signed — the underlying serializer implementations
• ClosureScope and SelfReference — support classes required for reconstruction

All six are necessary for the getClosure() call to succeed on a legitimately serialized closure. ✅

Test — excellent

The ClosureJobTest is particularly well-designed:

• testLegitimateClosureIsExecuted — confirms the happy path still works end-to-end using a real SerializableClosure and file-based flag (avoids $this binding issues in serialized closures)
• testMaliciousPayloadDoesNotTriggerGadget — the key security regression test: creates a real MaliciousGadget with __wakeup() and __destruct(), serializes it, passes it to run(), and asserts the gadget was never triggered. This directly proves the fix works.

The try/catch catching both \InvalidArgumentException and \Error covers both PHP 8 (method_exists on __PHP_Incomplete_Class throws Error) and PHP 7 behaviour. ✅

Summary

Aspect	Assessment
Security fix	✅ PHP Object Injection prevented
Allowed list	✅ Minimal and complete for SerializableClosure
Tests	✅ Both happy path and gadget-chain regression covered

Verdict: Ready to merge.

[DeepDiver1975]

Code Review

Overview: Security fix — restricts unserialize() in ClosureJob::run() to an explicit allowlist of SerializableClosure classes, preventing PHP Object Injection via arbitrary gadget chains.

The vulnerability: The original code called bare unserialize($serializedCallable) on data read from the oc_jobs database table. An attacker with database write access could inject a crafted PHP object payload to trigger __wakeup() or __destruct() methods on any class in the autoloader, potentially achieving RCE via gadget chains from bundled libraries (e.g. Symfony, Laravel).

The fix: ALLOWED_CLASSES constant enumerates exactly the six classes needed for SerializableClosure to reconstruct itself:

• SerializableClosure
• UnsignedSerializableClosure
• Laravel\SerializableClosure\Serializers\Native
• Laravel\SerializableClosure\Serializers\Signed
• Laravel\SerializableClosure\Support\ClosureScope
• Laravel\SerializableClosure\Support\SelfReference

Any object not in this list becomes __PHP_Incomplete_Class during deserialization — its __wakeup() and __destruct() are never called.

Test coverage in ClosureJobTest:

• testLegitimateClosureIsExecuted — verifies the happy path still works after the restriction
• testMaliciousPayloadDoesNotTriggerGadget — direct security regression test: serializes a MaliciousGadget object (with __wakeup() and __destruct() that set a static flag), runs it through the restricted unserialize, and asserts the flag was never set. This directly proves the gadget is blocked.

The use of a temp file for the legitimate closure test avoids $this binding issues across the serialize/unserialize boundary — good approach.

One observation: CommandJob (PR #41582, still at known SHA) applies the same fix to a parallel class. It's worth confirming the allowlist there is identical once that PR is also updated. The current #41582 SHA in state is unchanged, so it will be reviewed when it next updates.

Changelog: changelog/unreleased/41583 correctly categorises this as Security and accurately describes the RCE risk vector.

No concerns. Well-executed fix with direct security regression tests.