replaced shell quote with catfile, added more tests

Dockerfile

@@ -1,7 +1,7 @@
 from alpine:latest
 
 run apk add wget gnupg sqlite unzip build-base perl perl-dev perl-app-cpanminus
-run cpanm --notest IPC::Run DBD::SQLite Net::DNS::Resolver Crypt::Eksblowfish::Bcrypt JSON URI::Escape HTML::Entities String::ShellQuote Net::Server HTTP::Server::Simple Crypt::Random
+run cpanm --notest IPC::Run DBD::SQLite Net::DNS::Resolver Crypt::Eksblowfish::Bcrypt JSON URI::Escape HTML::Entities Net::Server HTTP::Server::Simple Crypt::Random
 
 run mkdir -p /opt/data/plans
 copy schema.sql /opt/data

server.pl

@@ -14,8 +14,8 @@ my $pid_file = $ENV{'PID_FILE'} || './data/dotplan.pid';
 my $log_file = $ENV{'LOG_FILE'} || './data/dotplan.log';
 my $database = $ENV{'DATABASE'} || './data/users.db';
 my $plan_dir = $ENV{'PLAN_DIR'} || './data/plans';
-my $sendmail = $ENV{'SENDMAIL'} || '/usr/bin/true';
+my $sendmail = $ENV{'SENDMAIL'} || '/bin/true';
-my @sendmail_args = split(/,/, $ENV{'SENDMAIL_ARGS'});
+my @sendmail_args = defined $ENV{'SENDMAIL_ARGS'} ? split(/,/, $ENV{'SENDMAIL_ARGS'}) : ();
 
 my $pw_token_expiration_minutes = $ENV{'PW_TOKEN_EXPIRATION_MINUTES'} || 10;
 my $auth_token_default_expiration_minutes = $ENV{'AUTH_TOKEN_DEFAULT_EXPIRATION_MINUTES'} || 5;
@@ -64,7 +64,7 @@ if (defined $ENV{'LOCAL_DOMAINS'}) {
   use JSON qw(encode_json decode_json);
   use URI::Escape qw(uri_escape);
   use HTML::Entities qw(encode_entities);
-  use String::ShellQuote qw(shell_quote);
+  use File::Spec::Functions qw(catfile);
 
   ###############
   # Common Errors
@@ -400,7 +400,7 @@ EOF
         print_json_response($cgi, 400, {error => "Pubkey exceeds maximum length of $maximum_pubkey_length."});
       } else {
         my (undef, $keyfile) = tempfile('tmpXXXXXX', SUFFIX => '.gpg', TMPDIR => 1, OPEN => 0);
-        my $basename = "$plan_dir/" . shell_quote($email);
+        my $basename = catfile($plan_dir, $email);
         IPC::Run::run ['gpg2', '--dearmor'], \$pubkey, '>', $keyfile, '2>>', '/dev/null' or die "gpg2 exited with $?";
         if(IPC::Run::run ['gpg2', '--no-default-keyring', '--keyring', "$keyfile", '--verify', "$basename.asc", "$basename.plan"], '>', '/dev/null', '2>>', '/dev/null') {
           $plan->{'verified'} = 1;
@@ -456,7 +456,7 @@ EOF
     my @arg = ($sendmail);
     push @arg, @sendmail_args;
     push @arg, $recipient;
-    IPC::Run::run \@arg, \$email, '>>', '/dev/null', '2>>', '/dev/null' or die "sendmail exited with $?";
+    IPC::Run::run \@arg, \$email or die "sendmail exited with $?";
   }
 
   # get mime type for response from querystring and accept header
@@ -553,7 +553,7 @@ EOF
   my $_plancache = {};
   sub util_save_plan {
     my ($email, $plan, $signature) = @_;
-    my $basename = "$plan_dir/" . shell_quote($email);
+    my $basename = catfile($plan_dir, $email);
 
     if (defined $plan) {
       open(my $plan_file, '>', "$basename.plan") or die $!;
@@ -581,7 +581,7 @@ EOF
   sub util_read_plan {
     my $email = shift;
     if (!defined $_plancache->{$email}) {
-      my $basename = "$plan_dir/" . shell_quote($email);
+      my $basename = catfile($plan_dir, $email);
 
       if (-f "$basename.plan") {
         my $details = {};

test/run.sh

@@ -55,7 +55,7 @@ assert_equal() {
   actual=$1;shift
   expected=$1;shift
   if [ "$actual" != "$expected" ]; then
-    printf "${RED}✗  CHECK${NC} ${BOLD}$check_name${NC}\n\n\"${YELLOW}$actual${NC}\" != \"${YELLOW}$expected${NC}\"\n\n"
+    printf "${RED}✗  CHECK${NC} ${BOLD}$check_name${NC}\n\n\"${YELLOW}"; echo -n "$actual"; printf "${NC}\" != \"${YELLOW}"; echo -n "$expected"; printf "${NC}\"\n\n"
     ((++FAILED))
     return 1
   fi
@@ -68,7 +68,7 @@ assert_equal_jq() {
   expected=$1;shift
   actual=$(echo "$TEST_CONTENT" | jq -r "$selector")
   if [ "$actual" != "$expected" ]; then
-    printf "${RED}✗  CHECK${NC} ${BOLD}$selector${NC}\n\n\"${YELLOW}$actual${NC}\" != \"${YELLOW}$expected${NC}\"\n\n"
+    printf "${RED}✗  CHECK${NC} ${BOLD}$selector${NC}\n\n\"${YELLOW}"; echo -n "$actual"; printf "${NC}\" != \"${YELLOW}"; echo -n "$expected"; printf "${NC}\"\n\n"
     ((++FAILED))
     return 1
   fi
@@ -81,7 +81,7 @@ assert_notequal_jq() {
   expected=$1;shift
   actual=$(echo "$TEST_CONTENT" | jq -r "$selector")
   if [ "$actual" == "$expected" ]; then
-    printf "${RED}✗  CHECK${NC} ${BOLD}$selector${NC} is null\n"
+    printf "${RED}✗  CHECK${NC} ${BOLD}$selector${NC}\n\n\"${YELLOW}"; echo -n "$selector"; printf "${NC}\" = \"${YELLOW}"; echo -n "$expected"; printf "${NC}\"\n\n"
     ((++FAILED))
     return 1
   fi
@@ -89,6 +89,32 @@ assert_notequal_jq() {
   return 0;
 }
 
+assert_exists() {
+  check_name=$1;shift
+  dir=$1;shift
+  file=$1;shift
+  if [ ! -e "$BASEDIR/$dir/$file" ]; then
+    printf "${RED}✗  CHECK${NC} ${BOLD}$check_name${NC}\n\n\"${YELLOW}"; echo -n "$BASEDIR/$dir/$file"; printf "${NC}\" does not exist\n\n"
+    ((++FAILED))
+    return 1
+  fi
+  printf "${GREEN}✓  CHECK${NC} ${BOLD}$check_name${NC}\n"
+  return 0;
+}
+
+assert_not_exists() {
+  check_name=$1;shift
+  dir=$1;shift
+  file=$1;shift
+  if [ -e "$BASEDIR/$dir/$file" ]; then
+    printf "${RED}✗  CHECK${NC} ${BOLD}$check_name${NC}\n\n\"${YELLOW}"; echo -n "$BASEDIR/$dir/$file"; printf "${NC}\" exists\n\n"
+    ((++FAILED))
+    return 1
+  fi
+  printf "${GREEN}✓  CHECK${NC} ${BOLD}$check_name${NC}\n"
+  return 0;
+}
+
 ############
 # Test Setup
 ############
@@ -107,7 +133,6 @@ if [ -z "$USE_DOCKER" ]; then
   LOG_FILE="$BASEDIR/data/test.log" \
   DATABASE="$BASEDIR/data/test.db" \
   PLAN_DIR="$BASEDIR/data/plans" \
-  SENDMAIL=/usr/bin/true \
   perl "$BASEDIR/../server.pl" -d >>/dev/null
 else
   docker build -t dotplan-online-test "$BASEDIR/.."
@@ -118,7 +143,6 @@ else
     -e LOG_FILE="/opt/data/test.log" \
     -e DATABASE="/opt/data/test.db" \
     -e PLAN_DIR="/opt/data/plans" \
-    -e SENDMAIL=/usr/bin/true \
     dotplan-online-test
 fi
 
@@ -248,6 +272,63 @@ curl_test 'Verify signed plan' 200 'application/json' -XPOST -d "$post_data" loc
   && assert_equal_jq '.plan' 'this is a plan
 that is signed'
 
+BADGUY='badguy@example.com%2F..%2Fgotya'
+curl_test 'Avoid directory traversal 1 account creation' 404 'application/json' -XPOST -d '{"password":"test1234"}' localhost:$PORT/users/$BADGUY
+
+BADGUY='badguy@example.com%252F..%252Fgotya'
+BADGUY_ESC='badguy@example.com%2F..%2Fgotya'
+curl_test 'Avoid directory traversal 2 account creation' 200 'application/json' -XPOST -d '{"password":"test1234"}' localhost:$PORT/users/$BADGUY \
+  && assert_notequal_jq '.email' 'null'
+
+pw_token=$(echo "select pw_token from users where email='$BADGUY_ESC'" | sqlite3 "$BASEDIR/data/test.db")
+
+curl_test 'Verify directory traversal address 2' 200 'application/json' -XPUT -d "{\"token\":\"$pw_token\"}" localhost:$PORT/users/$BADGUY \
+  && assert_equal_jq '.success' 1
+
+curl_test 'Get directory traversal 2 authentication token' 200 'application/json' -u "$BADGUY_ESC:test1234" localhost:$PORT/token
+
+token=$(echo "$TEST_CONTENT" | jq -r '.token')
+
+curl_test 'Create directory traversal 2 plan' 200 'application/json' -XPUT -d "{\"plan\":\"something\",\"auth\":\"$token\"}" localhost:$PORT/plan/$BADGUY \
+  && assert_equal_jq '.success' 1 \
+  && assert_not_exists 'malicious file' 'data' 'gotya' \
+  && assert_exists 'benign plan file' 'data/plans' "$BADGUY_ESC.plan"
+
+BADGUY="badguy@example.com\\..\\gotya"
+curl_test 'Avoid directory traversal 3 account creation' 200 'application/json' -XPOST -d '{"password":"test1234"}' localhost:$PORT/users/$BADGUY
+
+pw_token=$(echo "select pw_token from users where email='$BADGUY'" | sqlite3 "$BASEDIR/data/test.db")
+
+curl_test 'Verify directory traversal address 3' 200 'application/json' -XPUT -d "{\"token\":\"$pw_token\"}" localhost:$PORT/users/$BADGUY \
+  && assert_equal_jq '.success' 1
+
+curl_test 'Get directory traversal 3 authentication token' 200 'application/json' -u "$BADGUY:test1234" localhost:$PORT/token
+
+token=$(echo "$TEST_CONTENT" | jq -r '.token')
+
+curl_test 'Create directory traversal 3 plan' 200 'application/json' -XPUT -d "{\"plan\":\"something\",\"auth\":\"$token\"}" localhost:$PORT/plan/$BADGUY \
+  && assert_equal_jq '.success' 1 \
+  && assert_not_exists 'malicious file' 'data' 'gotya' \
+  && assert_exists 'benign plan file' 'data/plans' "$BADGUY.plan"
+
+BADGUY="badguy%40example.com%27%3Bdrop%20table%20users%3B"
+BADGUY_ESC="badguy@example.com';drop table users;"
+curl_test 'Avoid SQL injection account creation' 200 'application/json' -XPOST -d '{"password":"test1234"}' "localhost:$PORT/users/$BADGUY" \
+  && assert_equal_jq '.email' "$BADGUY_ESC"
+
+pw_token=$(echo "select pw_token from users where email='${BADGUY_ESC//\'/\'\'}'" | sqlite3 "$BASEDIR/data/test.db")
+
+curl_test 'Verify SQL injection address' 200 'application/json' -XPUT -d "{\"token\":\"$pw_token\"}" localhost:$PORT/users/$BADGUY \
+  && assert_equal_jq '.success' 1
+
+curl_test 'Get SQL injection authentication token' 200 'application/json' -u "$BADGUY_ESC:test1234" localhost:$PORT/token
+
+token=$(echo "$TEST_CONTENT" | jq -r '.token')
+
+curl_test 'Create SQL injection plan' 200 'application/json' -XPUT -d "{\"plan\":\"something\",\"auth\":\"$token\"}" localhost:$PORT/plan/$BADGUY \
+  && assert_equal_jq '.success' 1 \
+  && assert_exists 'benign plan file' 'data/plans' "$BADGUY_ESC.plan"
+
 ###############
 # Test Teardown
 ###############
@@ -264,5 +345,10 @@ else
   docker kill dotplan_online_test
 fi
 
-printf "Tests complete.\n"
+if [ $FAILED -gt 0 ]; then
+  printf "${RED}"
+else
+  printf "${GREEN}"
+fi
+printf "Tests complete. $FAILED failed.${NC}\n"
 exit $FAILED