Make the sandbox a first-class citizen with a dedicated Sandbox class - #4854
Make the sandbox a first-class citizen with a dedicated Sandbox class#4854fabpot wants to merge 6 commits into
Conversation
|
📋 PR Summary This incremental change moves the Changes
|
![upsun-dispatch[bot]](https://avatars.githubusercontent.com/in/4153575?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Upsun Dispatch Review: full · 26 files reviewed · no issues found
Review details
Commit: Commit ac46f0a
Model: claude-opus-4-8
Panel: security · correctness · robustness · design
ac46f0a to
34a32bb
Compare
34a32bb to
bf97d98
Compare
| private Environment $env, | ||
| SecurityPolicyInterface $policy, | ||
| ) { | ||
| if ($policy instanceof SecurityPolicy && !$policy->isStrict()) { |
There was a problem hiding this comment.
🔵 Info — Post-construction mutation quietly defeats the documented strict guarantee.
The strict-policy invariant is only checked at construction and only for SecurityPolicy instances. Because the policy object is shared with the caller (and SecurityChecker::setSecurityPolicy() can replace it), calling $policy->setStrict(false) after new Sandbox(...) leaves the sandbox running with non-strict semantics without any error, so it silently diverges between Twig 3.x and 4.0 behavior.
| */ | ||
| public function stream(string $name, array $context = []): iterable | ||
| { | ||
| yield from $this->env->load($name)->stream($context); |
There was a problem hiding this comment.
🔵 Info — Deferred vs eager error timing across parallel methods can surprise callers.
stream() and streamBlock() use yield from, making the whole method a generator, so $this->env->load($name) (and any LoaderError for a missing template) is deferred until the returned iterable is first iterated. render()/display() delegate directly and throw eagerly, so stream('missing') that is never iterated raises nothing while the eager methods raise immediately — a behavioral divergence across the sibling render paths.
| } | ||
|
|
||
| if ((new \ReflectionMethod($this->policy, 'checkSecurity'))->getNumberOfParameters() >= 4) { | ||
| $this->policy->checkSecurity($tags, $filters, $functions, $tests); |
There was a problem hiding this comment.
shouldn't this catch the SecurityNotAllowed*Error exceptions (probably catching SecurityError) to set the source context in them ?
da4cbba to
8aa73b6
Compare
There was a problem hiding this comment.
📋 Upsun Dispatch Review: full · 32 files reviewed · 🟡 2 warnings · 🔵 1 info · 2 still open
🟡 Warning (P1 — real issues) — outside diff range
general· processThis PR does not merge cleanly onto the base branch: CHANGELOG, doc/sandbox.rst, and tests/Extension/CoreTest.php conflict. The branch must be rebased or the conflicts resolved before it can be merged.
Review details
Commit: Commit da4cbba
Model: claude-opus-4-8
Panel: security · correctness · robustness · design
Outstanding from earlier reviews:
- #3546865692 — src/Sandbox/Sandbox.php:36: Post-construction mutation quietly defeats the documented strict guarantee.
- #3546865697 — src/Sandbox/Sandbox.php:59: Deferred vs eager error timing across parallel methods can surprise callers.
There was a problem hiding this comment.
📋 Upsun Dispatch Review: full · 32 files reviewed · 🟡 1 warning · 3 still open
🟡 Warning (P1 — real issues) — outside diff range
general· merge-conflictThe branch does not merge cleanly onto the base branch: CHANGELOG, doc/sandbox.rst, and tests/Extension/CoreTest.php have merge conflicts. The merge is blocked until the branch is rebased and the conflicts are resolved.
Review details
Commit: Commit 8aa73b6
Model: claude-opus-4-8
Panel: security · correctness · robustness · design
Outstanding from earlier reviews:
- #3546865692 — src/Sandbox/Sandbox.php:36: Post-construction mutation quietly defeats the documented strict guarantee.
- #3546865697 — src/Sandbox/Sandbox.php:59: Deferred vs eager error timing across parallel methods can surprise callers.
- #3668201835 — src/Sandbox/SandboxInterface.php:53: A documented method is missing from the contract meant for DI, forcing a concrete-class dependency.
8aa73b6 to
f4cf0c3
Compare
There was a problem hiding this comment.
📋 Upsun Dispatch Review: full · 31 files reviewed · 🟡 1 warning · 3 still open
Outstanding from earlier reviews:
- #3546865692 — src/Sandbox/Sandbox.php:36: Post-construction mutation quietly defeats the documented strict guarantee.
- #3546865697 — src/Sandbox/Sandbox.php:59: Deferred vs eager error timing across parallel methods can surprise callers.
- #3668201835 — src/Sandbox/SandboxInterface.php:53: A documented method is missing from the contract meant for DI, forcing a concrete-class dependency.
There was a problem hiding this comment.
📋 Upsun Dispatch Review: incremental · 31 files reviewed · no new issues · 3 still open
Outstanding from earlier reviews:
- #3546865692 — src/Sandbox/Sandbox.php:36: Post-construction mutation quietly defeats the documented strict guarantee.
- #3546865697 — src/Sandbox/Sandbox.php:59: Deferred vs eager error timing across parallel methods can surprise callers.
- #3668201830 — src/Node/Expression/FunctionNode/RenderSandboxedFunction.php:66: The documented positional render_sandboxed usage is silently ignored and its safety/validation guards never fire.
2 participants
I've been thinking about making the sabdbox feature as a first class citizen for years. With all the work that has been done recently on security issues, I spent some time on it again. Here is the result.
The main ideas:
SandboxExtensionis registered on the environmen directly, so it instruments all compiled template, and adds runtime checks to all renders, trusted or not. As recommended in the docs, you should have a dedicated environment for sandboxes, different from the main one, but it's not really "enforced" nor natural to do.enableSandbox()/disableSandbox()with try/finally patterns scattered across the codebase to support rendering sandboxed and non-sandboxed templates from a environment.includeanything the loader can load, sees every application global, and inherits all extensions, this is a footgun (again, already not recommended in the docs).enableSandbox(),{% include(..., sandboxed: true) %}, and{% sandbox %}.The new
Twig\Sandbox\Sandboxclass renders untrusted templates through a dedicated, always-sandboxed environment crafted by the developer. Taht way, there is no state to toggle and nothing leaks between the main environment and the sandbox, in either direction.