Return 400/500 for some client/server errors.

9 files changed, 43 insertions, 25 deletions

diff --git a/perllib/FixMyStreet/App/Controller/Auth.pm b/perllib/FixMyStreet/App/Controller/Auth.pm
index 40cd163cf..c448f8749 100644
--- a/perllib/FixMyStreet/App/Controller/Auth.pm
+++ b/perllib/FixMyStreet/App/Controller/Auth.pm
@@ -271,9 +271,8 @@ sub facebook_callback: Path('/auth/Facebook') : Args(0) {
         $access_token = $fb->get_access_token(code => $c->get_param('code'));
     };
     if ($@) {
-        ($c->stash->{message} = $@) =~ s/at [^ ]*Auth.pm.*//;
-        $c->stash->{template} = 'errors/generic.html';
-        $c->detach;
+        (my $message = $@) =~ s/at [^ ]*Auth.pm.*//;
+        $c->detach('/page_error_500_internal_error', [ $message ]);
     }
 
     # save this token in session
@@ -339,9 +338,8 @@ sub twitter_callback: Path('/auth/Twitter') : Args(0) {
         $twitter->request_access_token(verifier => $verifier);
     };
     if ($@) {
-        ($c->stash->{message} = $@) =~ s/at [^ ]*Auth.pm.*//;
-        $c->stash->{template} = 'errors/generic.html';
-        $c->detach;
+        (my $message = $@) =~ s/at [^ ]*Auth.pm.*//;
+        $c->detach('/page_error_500_internal_error', [ $message ]);
     }
 
     my $info = $twitter->verify_credentials();
@@ -527,8 +525,7 @@ sub check_csrf_token : Private {
 
 sub no_csrf_token : Private {
     my ($self, $c) = @_;
-    $c->stash->{message} = _('Unknown error');
-    $c->stash->{template} = 'errors/generic.html';
+    $c->detach('/page_error_400_bad_request', []);
 }
 
 =head2 sign_out
diff --git a/perllib/FixMyStreet/App/Controller/Dashboard.pm b/perllib/FixMyStreet/App/Controller/Dashboard.pm
index 9189b28e5..fbe5a2dc9 100644
--- a/perllib/FixMyStreet/App/Controller/Dashboard.pm
+++ b/perllib/FixMyStreet/App/Controller/Dashboard.pm
@@ -57,9 +57,9 @@ sub example : Local : Args(0) {
         }
     };
     if ($@) {
-        $c->stash->{message} = _("There was a problem showing this page. Please try again later.") . ' ' .
+        my $message = _("There was a problem showing this page. Please try again later.") . ' ' .
             sprintf(_('The error was: %s'), $@);
-        $c->stash->{template} = 'errors/generic.html';
+        $c->detach('/page_error_500_internal_error', [ $message ]);
     }
 }
 
diff --git a/perllib/FixMyStreet/App/Controller/Questionnaire.pm b/perllib/FixMyStreet/App/Controller/Questionnaire.pm
index 017a552db..1b338732b 100755
--- a/perllib/FixMyStreet/App/Controller/Questionnaire.pm
+++ b/perllib/FixMyStreet/App/Controller/Questionnaire.pm
@@ -36,9 +36,8 @@ sub check_questionnaire : Private {
     if ( $questionnaire->whenanswered ) {
         my $problem_url = $c->cobrand->base_url_for_report( $problem ) . $problem->url;
         my $contact_url = $c->uri_for( "/contact" );
-        $c->stash->{message} = sprintf(_("You have already answered this questionnaire. If you have a question, please <a href='%s'>get in touch</a>, or <a href='%s'>view your problem</a>.\n"), $contact_url, $problem_url);
-        $c->stash->{template} = 'errors/generic.html';
-        $c->detach;
+        my $message = sprintf(_("You have already answered this questionnaire. If you have a question, please <a href='%s'>get in touch</a>, or <a href='%s'>view your problem</a>.\n"), $contact_url, $problem_url);
+        $c->detach('/page_error_400_bad_request', [ $message ]);
     }
 
     unless ( $problem->is_visible ) {
@@ -86,8 +85,8 @@ Display couldn't locate problem error message
 sub missing_problem : Private {
     my ( $self, $c ) = @_;
 
-    $c->stash->{message} = _("I'm afraid we couldn't locate your problem in the database.\n");
-    $c->stash->{template} = 'errors/generic.html';
+    my $message = _("I'm afraid we couldn't locate your problem in the database.\n");
+    $c->detach('/page_error_400_bad_request', [ $message ]);
 }
 
 sub submit_creator_fixed : Private {
diff --git a/perllib/FixMyStreet/App/Controller/Reports.pm b/perllib/FixMyStreet/App/Controller/Reports.pm
index 813c2052d..f2c43b5ee 100644
--- a/perllib/FixMyStreet/App/Controller/Reports.pm
+++ b/perllib/FixMyStreet/App/Controller/Reports.pm
@@ -76,13 +76,12 @@ sub index : Path : Args(0) {
         $c->stash->{open} = $j->{open};
     };
     if ($@) {
-        $c->stash->{message} = _("There was a problem showing the All Reports page. Please try again later.");
+        my $message = _("There was a problem showing the All Reports page. Please try again later.");
         if ($c->config->{STAGING_SITE}) {
-            $c->stash->{message} .= '</p><p>Perhaps the bin/update-all-reports script needs running. Use: bin/update-all-reports</p><p>'
+            $message .= '</p><p>Perhaps the bin/update-all-reports script needs running. Use: bin/update-all-reports</p><p>'
                 . sprintf(_('The error was: %s'), $@);
         }
-        $c->stash->{template} = 'errors/generic.html';
-        return;
+        $c->detach('/page_error_500_internal_error', [ $message ]);
     }
 
     # Down here so that error pages aren't cached.
diff --git a/perllib/FixMyStreet/App/Controller/Root.pm b/perllib/FixMyStreet/App/Controller/Root.pm
index 3d4c6a1ba..1df249999 100644
--- a/perllib/FixMyStreet/App/Controller/Root.pm
+++ b/perllib/FixMyStreet/App/Controller/Root.pm
@@ -103,9 +103,24 @@ sub page_error_410_gone : Private {
 
 sub page_error_403_access_denied : Private {
     my ( $self, $c, $error_msg ) = @_;
+    $c->detach('page_error', [ $error_msg || _("Sorry, you don't have permission to do that."), 403 ]);
+}
+
+sub page_error_400_bad_request : Private {
+    my ( $self, $c, $error_msg ) = @_;
+    $c->detach('page_error', [ $error_msg, 400 ]);
+}
+
+sub page_error_500_internal_error : Private {
+    my ( $self, $c, $error_msg ) = @_;
+    $c->detach('page_error', [ $error_msg, 500 ]);
+}
+
+sub page_error : Private {
+    my ($self, $c, $error_msg, $code) = @_;
     $c->stash->{template}  = 'errors/generic.html';
-    $c->stash->{message} = $error_msg || _("Sorry, you don't have permission to do that.");
-    $c->response->status(403);
+    $c->stash->{message} = $error_msg || _('Unknown error');
+    $c->response->status($code);
 }
 
 =head2 end
diff --git a/perllib/FixMyStreet/App/Controller/Tokens.pm b/perllib/FixMyStreet/App/Controller/Tokens.pm
index da017c57f..a1b0c57ba 100644
--- a/perllib/FixMyStreet/App/Controller/Tokens.pm
+++ b/perllib/FixMyStreet/App/Controller/Tokens.pm
@@ -348,6 +348,7 @@ sub token_too_old : Private {
     my ( $self, $c ) = @_;
     $c->stash->{token_not_found} = 1;
     $c->stash->{template} = 'auth/token.html';
+    $c->response->status(400);
 }
 
 __PACKAGE__->meta->make_immutable;
diff --git a/t/app/controller/moderate.t b/t/app/controller/moderate.t
index 0ccfcf2c2..3a3c7a708 100644
--- a/t/app/controller/moderate.t
+++ b/t/app/controller/moderate.t
@@ -67,7 +67,8 @@ subtest 'Auth' => sub {
         $mech->get_ok($REPORT_URL);
         $mech->content_lacks('Moderat');
 
-        $mech->get_ok('/contact?m=1&id=' . $report->id);
+        $mech->get('/contact?m=1&id=' . $report->id);
+        is $mech->res->code, 400;
         $mech->content_lacks('Good bad bad bad');
     };
 
diff --git a/t/app/controller/questionnaire.t b/t/app/controller/questionnaire.t
index b05f74225..f42908a3e 100644
--- a/t/app/controller/questionnaire.t
+++ b/t/app/controller/questionnaire.t
@@ -87,16 +87,19 @@ foreach my $test (
         desc => 'User goes to questionnaire URL with a bad token',
         token_extra => 'BAD',
         content => "Sorry, that wasn&rsquo;t a valid link",
+        code => 400,
     },
     {
         desc => 'User goes to questionnaire URL for a now-hidden problem',
         state => 'hidden',
         content => "we couldn't locate your problem",
+        code => 400,
     },
     {
         desc => 'User goes to questionnaire URL for an already answered questionnaire',
         answered => \'current_timestamp',
         content => 'already answered this questionnaire',
+        code => 400,
     },
 ) {
     subtest $test->{desc} => sub {
@@ -106,7 +109,8 @@ foreach my $test (
         $questionnaire->update;
         (my $token = $token->token);
         $token .= $test->{token_extra} if $test->{token_extra};
-        $mech->get_ok("/Q/$token");
+        $mech->get("/Q/$token");
+        is $mech->res->code, $test->{code}, "Right status received";
         $mech->content_contains( $test->{content} );
         # Reset, no matter what test did
         $report->state( 'confirmed' );
diff --git a/t/app/controller/report_updates.t b/t/app/controller/report_updates.t
index 5a88097fa..f7544f0a1 100644
--- a/t/app/controller/report_updates.t
+++ b/t/app/controller/report_updates.t
@@ -1829,7 +1829,8 @@ for my $test (
 subtest 'check have to be logged in for creator fixed questionnaire' => sub {
     $mech->log_out_ok();
 
-    $mech->get_ok( "/questionnaire/submit?problem=$report_id&reported=Yes" );
+    $mech->get( "/questionnaire/submit?problem=$report_id&reported=Yes" );
+    is $mech->res->code, 400, "got 400";
 
     $mech->content_contains( "I'm afraid we couldn't locate your problem in the database." )
 };
@@ -1838,7 +1839,8 @@ subtest 'check cannot answer other user\'s creator fixed questionnaire' => sub {
     $mech->log_out_ok();
     $mech->log_in_ok( $user2->email );
 
-    $mech->get_ok( "/questionnaire/submit?problem=$report_id&reported=Yes" );
+    $mech->get( "/questionnaire/submit?problem=$report_id&reported=Yes" );
+    is $mech->res->code, 400, "got 400";
 
     $mech->content_contains( "I'm afraid we couldn't locate your problem in the database." )
 };