fix: block eval of string BLOCK via template.new() (GH #245)

[toddr-bot]

What

Prevent arbitrary Perl code execution via template.new({BLOCK => '...'}) when EVAL_PERL is disabled.

Why

Fixes GH #245. Template::Document::new() calls eval $block on string BLOCK arguments — this runs regardless of the EVAL_PERL config. Since template and component are exposed as Document objects in the stash, a template author can call template.new({BLOCK => 'malicious code'}) to execute arbitrary Perl, bypassing the EVAL_PERL security control entirely.

The side effects run even though new() ultimately fails (at bless), because the eval on line 80 fires before the bless on line 94.

How

When new() is called as an instance method (detected via ref $class), string BLOCK and DEFBLOCKS arguments are rejected with an error before any eval occurs. This blocks the template-side attack vector while preserving class method calls — the only legitimate compilation path.

Testing

• 11 new tests in t/eval_perl_bypass.t:
  • Class method with string BLOCK succeeds (compilation path)
  • Instance method with string BLOCK is blocked (no side effects)
  • Instance method with string DEFBLOCKS is blocked
  • End-to-end exploit via template.new() blocked
  • End-to-end exploit via component.new() blocked
• Full test suite passes

---

Quality Report

Changes: 2 files changed, 122 insertions(+), 1 deletion(-)

Code scan: clean

Tests: passed (OK)

Branch hygiene: clean

Generated by Kōan

[toddr-bot]

Recreated from #352 (auto-closed when the toddr-bot fork was removed). Original branch is now hosted on this repo.

[minusdavid]

What do we need to do to help this one move along? Really looking forward to this change.

[atoomic]

@Koan-Bot review

[Koan-Bot]

@minusdavid: What do we need to do to help this one move along? Really looking forward to this change.

Fix logic sound. Two blockers: test payloads use die (fails under eval anyway — tests pass without the fix), and DEFBLOCKS string values still get eval'd on instance calls. Fix those, merge-ready.

[Koan-Bot]

Previous review — superseded by a newer review below.

[Koan-Bot]

Blocking issues found — see the review comment above.

[minusdavid]

Cool I see the new commit is up which should satisfy the "1 requested change" from "Koan-Bot"?

What's next?

[atoomic]

@Koan-Bot review

[Koan-Bot]

@minusdavid: Cool I see the new commit is up which should satisfy the "1 requested change" from "Koan-Bot"?

Yes. New commit resolves both blocking findings. Tests now use real eval-succeeding payloads. DEFBLOCKS bypass closed with second guard. Verified sole compile caller (Provider.pm:925) uses class method — fix won't break it. Two minor suggestions remain, non-blocking. Merge-ready.

[Koan-Bot]

PR Review — fix: block eval of string BLOCK via template.new() (GH #245)

Merge-ready. The revised commit resolves every blocking finding from the prior review; only two minor portability/clarity nits remain.

What's solid:

• The ref $class guard is surgical — it rejects only string BLOCK/DEFBLOCKS on instance calls, leaving legitimate class-method compilation (verified: Provider.pm:925 calls $DOCUMENT->new as a class method) and coderef-BLOCK instance calls untouched.

• The DEFBLOCKS bypass flagged last round is now closed by a second guard inside the map, before the eval.

• Tests now use payloads that succeed under eval (a file-creating open() + sub {"pwned"}), so they genuinely distinguish fixed-vs-unfixed code — the earlier die-based false-pass problem is gone. Side effects are asserted via a marker file, and both template.new() and component.new() are covered end-to-end.

• Suggestion: DEFBLOCKS guard reuses the "BLOCK" error string (misleading debug text only).

• Suggestion: test hardcodes /tmp, which weakens the side-effect assertion on non-Unix hosts.

---

🟢 Suggestions

1. DEFBLOCKS guard reuses the BLOCK error message
lib/Template/Document.pm:101

The guard inside the DEFBLOCKS map reports "cannot eval BLOCK in instance method call", but it fires for a string DEFBLOCKS value, not the main BLOCK.

Why it matters: if this path ever trips in real usage, the error text points a debugger at the wrong argument. Functionally the guard is correct — this is purely a clarity nit.

Consider a distinct message, e.g. "cannot eval DEFBLOCKS in instance method call".

return $class->error("cannot eval BLOCK in instance method call")
    if ref $class;

2. Hardcoded /tmp path reduces portability
t/eval_perl_bypass.t:15

The marker file is hardcoded to /tmp, which is not writable (or may not exist) on Windows and some CI sandboxes. If /tmp is unwritable, the exploit open() would silently fail even without the fix, weakening the negative assertion ok(!-e $marker).

Using File::Temp or File::Spec->tmpdir for the directory would make the test portable and keep the side-effect assertion meaningful everywhere:

use File::Temp qw( tempdir );
my $dir = tempdir( CLEANUP => 1 );
my $marker = "$dir/tt-eval-bypass-$$";

Not blocking — the test is correct on the Unix hosts TT primarily targets.

my $marker = "/tmp/tt-eval-bypass-test-$$";

---

Checklist

• No hardcoded secrets
• No unsafe eval/exec (BLOCK + DEFBLOCKS vectors closed)
• Input validation at security boundary
• Test coverage validates the fix (real eval payloads)
• Tests verify behavior, not source inspection
• Cross-platform test portability — suggestion ＃2

---

Automated review by Kōan (Claude · model opus) HEAD=df4d86b 3 min 19s

[Koan-Bot]

No blocking issues found.