From 3d70a213cc3bd6d1cb821a7c6d217b88a1ed07a0 Mon Sep 17 00:00:00 2001 From: Patrick Donelan Date: Thu, 2 Apr 2009 01:53:58 +0000 Subject: [PATCH] Made Survey branch expressions eval in safe compartment --- .../Asset/Wobject/Survey/ResponseJSON.pm | 18 +++++++++++++----- t/Asset/Wobject/Survey/ResponseJSON.t | 18 ++++++++++++++---- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/lib/WebGUI/Asset/Wobject/Survey/ResponseJSON.pm b/lib/WebGUI/Asset/Wobject/Survey/ResponseJSON.pm index 4fa267905..b6d187c6b 100644 --- a/lib/WebGUI/Asset/Wobject/Survey/ResponseJSON.pm +++ b/lib/WebGUI/Asset/Wobject/Survey/ResponseJSON.pm @@ -98,6 +98,7 @@ use strict; use JSON; use Params::Validate qw(:all); use List::Util qw(shuffle); +use Safe; Params::Validate::validation_options( on_fail => sub { WebGUI::Error::InvalidParam->throw( error => shift ) } ); #------------------------------------------------------------------- @@ -660,7 +661,7 @@ The expression is a simple subset of the formula language used in spreadsheet pr such as Excel, OpenOffice, Google Docs etc.. Here is an example using section variables S1 and S2 as jump targets and question -variables Q1-3 in the expression. It jumps to S1 if the user's answer to Q1 has a value +variables Q1-3 in the expression. It jumps to S1 if the user's answer to Q1 has a value of 3, jumps to S2 if Q2 + Q3 < 10, and otherwise doesn't branch at all (the default). S1: Q1 = 3 S2: Q2 + Q3 < 10 @@ -714,9 +715,12 @@ sub processGotoExpression { my $processed = $self->parseGotoExpression($line, $responses); next if !$processed; + + # Eval expression in a safe compartment + # N.B. Expression does not need access to any variables + my $compartment = Safe->new(); + my $result = $compartment->reval($processed->{expression}); - # (ab)use perl's eval to evaluate the processed expression - my $result = eval "$processed->{expression}"; ## no critic $self->session->log->warn($@) if $@; ## no critic if ($result) { @@ -805,7 +809,7 @@ sub parseGotoExpression { $self->session->log->debug("Parsing gotoExpression: $expression"); # Valid gotoExpression tokens are.. - my $tokens = qr{\s|[-0-9=!<>+*/.()]}; + my $tokens = qr{\s|[-0-9=!<>+*/.()&|:?]}; my ( $target, $rest ) = $expression =~ /\s* ([^:]+?) \s* : \s* (.*)/x; @@ -820,7 +824,11 @@ sub parseGotoExpression { $self->session->log->warn('Expression undefined'); return; } - + + # convert 'and' and 'or' to '&&' and '||' + $rest =~ s/\band\b/&&/ig; + $rest =~ s/\bor\b/||/ig; + # Replace each questionName with its response value while ( my ( $questionName, $response ) = each %{$responses} ) { $rest =~ s/$questionName/$response/g; diff --git a/t/Asset/Wobject/Survey/ResponseJSON.t b/t/Asset/Wobject/Survey/ResponseJSON.t index 151dab56e..a9a25ebfe 100644 --- a/t/Asset/Wobject/Survey/ResponseJSON.t +++ b/t/Asset/Wobject/Survey/ResponseJSON.t @@ -22,7 +22,7 @@ my $session = WebGUI::Test->session; #---------------------------------------------------------------------------- # Tests -my $tests = 79; +my $tests = 87; plan tests => $tests + 1; #---------------------------------------------------------------------------- @@ -323,7 +323,7 @@ is($rJSON->lastResponse(), 5, 'goto: finds first if there are duplicates'); #################################################### # -# processGotoExpression +# parseGotoExpression # #################################################### throws_ok { $rJSON->parseGotoExpression() } 'WebGUI::Error::InvalidParam', 'processGotoExpression takes exception to empty arguments'; @@ -339,7 +339,7 @@ cmp_deeply($rJSON->parseGotoExpression('t1: 1'), { target => 't1', expression => '1'}, 'works for simple numeric expression'); cmp_deeply($rJSON->parseGotoExpression('t1: 1 - 23 + 456 * (78 / 9.0)'), { target => 't1', expression => '1 - 23 + 456 * (78 / 9.0)'}, 'works for expression using all algebraic tokens'); -is($rJSON->parseGotoExpression('t1: 1 + &'), undef, '.. but disallows expression containing non-whitelisted token'); +is($rJSON->parseGotoExpression('t1: 1 + $'), undef, '.. but disallows expression containing non-whitelisted token'); cmp_deeply($rJSON->parseGotoExpression('t1: 1 = 3'), { target => 't1', expression => '1 == 3'}, 'converts single = to =='); cmp_deeply($rJSON->parseGotoExpression('t1: 1 != 3 <= 4 >= 5'), @@ -350,6 +350,10 @@ cmp_deeply($rJSON->parseGotoExpression('t1: a silly var name * 10 + another var { target => 't1', expression => '345 * 10 + 456'}, '..it even works for vars with spaces in their names'); is($rJSON->parseGotoExpression('t1: qX + 3', { q1 => '7'}), undef, q{..but doesn't like invalid var names}); +cmp_deeply($rJSON->parseGotoExpression('t1: (A < 4) AND (B < 4)', { A => 2, B => 3}), + { target => 't1', expression => '(2 < 4) && (3 < 4)'}, 'Boolean AND'); +cmp_deeply($rJSON->parseGotoExpression('t1: (A < 4) OR (B < 4)', { A => 2, B => 3}), + { target => 't1', expression => '(2 < 4) || (3 < 4)'}, 'Boolean OR'); #################################################### # @@ -376,7 +380,13 @@ ok(!$rJSON->processGotoExpression('s0: s1q0 != 3'), '3 != 3 is false'); ok($rJSON->processGotoExpression('s0: s1q0 > 2'), '3 > 2 is true'); ok($rJSON->processGotoExpression('s0: s1q0 < 4'), '3 < 2 is true'); ok(!$rJSON->processGotoExpression('s0: s1q0 >= 4'), '3 >= 4 is false'); -ok(!$rJSON->processGotoExpression('s0: s1q0 <= 2'), '3 >= 4 is false'); +ok(!$rJSON->processGotoExpression('s0: s1q0 <= 2'), '3 <= 2 is false'); +ok(!$rJSON->processGotoExpression('s0: s1q0 < 2 or s1q0 < 1'), '3 < 2 || 3 < 1 is false'); +ok($rJSON->processGotoExpression('s0: s1q0 < 2 or s1q0 < 5'), '3 < 2 || 3 < 5 is true'); +ok(!$rJSON->processGotoExpression('s0: s1q0 = 4 and 1 = 1'), '3 == 4 && 1 == 1 is false'); +ok($rJSON->processGotoExpression('s0: s1q0 = 3 and 1 = 1'), '3 == 3 && 1 == 1 is true'); +ok(!$rJSON->processGotoExpression('s0: (s1q0 > 1 ? 10 : 11) = 11'), '(3 > 1 ? 10 : 11) == 11 is false'); +ok($rJSON->processGotoExpression('s0: (s1q0 > 1 ? 10 : 11) = 10'), '(3 > 1 ? 10 : 11) == 10 is true'); cmp_deeply($rJSON->processGotoExpression(<<"END_EXPRESSION"), {target => 's2', expression => '3 == 3'}, 'first true expression wins'); s0: s1q0 <= 2