Bug 2085553 - Github logins are failing with JSON error after recent changes to Bugzilla

extensions/GitHubAuth/lib/Client.pm

@@ -12,6 +12,7 @@ use strict;
 use warnings;
 
 use JSON qw(decode_json);
+use LWP::UserAgent;
 use URI;
 use URI::QueryParam;
 use Digest;
@@ -66,15 +67,37 @@ sub get_email_key {
 
 sub _handle_response {
   my ($self, $response) = @_;
-  my $data = eval { decode_json($response->content); };
-  if ($@) {
-    ThrowCodeError("github_bad_response",
-      {message => "Unable to parse JSON response"});
-  }
 
+  # Check the status before parsing: error pages from GitHub, a proxy, or
+  # LWP itself (internal 500s for DNS/TLS failures) are usually not JSON.
   unless ($response->is_success) {
     ThrowCodeError("github_error", {response => $response});
   }
+
+  my $data = eval { decode_json($response->decoded_content(charset => 'none') // ''); };
+  if ($@) {
+    ThrowCodeError(
+      "github_bad_response",
+      {
+        reason       => "Unable to parse JSON response",
+        status       => $response->status_line,
+        content_type => scalar $response->header('Content-Type'),
+      }
+    );
+  }
+
+  # GitHub's token endpoint reports failures (e.g. bad_verification_code)
+  # as a 200 response with an "error" key.
+  if (ref $data eq 'HASH' && $data->{error}) {
+    ThrowCodeError(
+      "github_bad_response",
+      {
+        reason => $data->{error_description} || $data->{error},
+        status => $response->status_line,
+      }
+    );
+  }
+
   return $data;
 }
 

extensions/GitHubAuth/t/bmo/client-response.t

@@ -0,0 +1,137 @@
+#!/usr/bin/env perl
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+#
+# This Source Code Form is "Incompatible With Secondary Licenses", as
+# defined by the Mozilla Public License, v. 2.0.
+
+use strict;
+use warnings;
+use 5.10.1;
+use lib qw( . lib local/lib/perl5 );
+
+BEGIN {
+  $ENV{LOG4PERL_CONFIG_FILE}     = 'log4perl-t.conf';
+  $ENV{BUGZILLA_DISABLE_HOSTAGE} = 1;
+}
+
+use Test::More;
+use HTTP::Response;
+use Try::Tiny;
+
+use Bugzilla;
+BEGIN { Bugzilla->extensions }
+
+use_ok('Bugzilla::Extension::GitHubAuth::Client');
+use_ok('Bugzilla::Extension::GitHubAuth::Client::Error');
+
+# Fake user agent that returns a canned response for any request.
+package FakeUA {
+  sub new { my ($class, $response) = @_; bless {response => $response}, $class }
+  sub get  { $_[0]->{response} }
+  sub post { $_[0]->{response} }
+}
+
+$Bugzilla::Extension::GitHubAuth::Client::Error::USE_EXCEPTION_OBJECTS = 1;
+
+sub response {
+  my ($code, $content_type, $body, %headers) = @_;
+  my $res = HTTP::Response->new($code, undef,
+    ['Content-Type' => $content_type, %headers], $body);
+  return $res;
+}
+
+sub client_with {
+  my ($res) = @_;
+  my $client = Bugzilla::Extension::GitHubAuth::Client->new;
+  $client->{user_agent} = FakeUA->new($res);
+  return $client;
+}
+
+sub error_from {
+  my ($code) = @_;
+  return try { $code->(); undef } catch { $_ };
+}
+
+subtest 'successful JSON responses are returned' => sub {
+  my $client = client_with(response(
+    200, 'application/json', '{"access_token":"abc","token_type":"bearer"}'
+  ));
+  is($client->get_access_token('code'), 'abc', 'access token returned');
+
+  $client = client_with(response(
+    200, 'application/json', '[{"email":"a@example.com","verified":true}]'
+  ));
+  is_deeply(
+    $client->get_user_emails('abc'),
+    [{email => 'a@example.com', verified => 1}],
+    'email list returned'
+  );
+};
+
+subtest 'non-2xx HTML error page reports the HTTP status' => sub {
+  my $client = client_with(
+    response(403, 'text/html', '<html>Forbidden</html>'));
+  my $e = error_from(sub { $client->get_user_emails('abc') });
+  isa_ok($e, 'Bugzilla::Extension::GitHubAuth::Client::Error');
+  is($e->error, 'github_error', 'github_error, not github_bad_response');
+  is($e->vars->{response}->code, 403, 'response attached to error');
+};
+
+subtest 'LWP internal failure reports a connection problem' => sub {
+  my $client = client_with(response(
+    500, 'text/plain', "Can't connect to github.com:443",
+    'Client-Warning' => 'Internal response'
+  ));
+  my $e = error_from(sub { $client->get_access_token('code') });
+  is($e->error, 'github_error', 'github_error thrown');
+  is($e->vars->{response}->header('Client-Warning'),
+    'Internal response', 'internal response marker preserved');
+};
+
+subtest '200 with non-JSON body reports status and content type' => sub {
+  my $client = client_with(
+    response(200, 'text/html', '<html>not json</html>'));
+  my $e = error_from(sub { $client->get_access_token('code') });
+  is($e->error,           'github_bad_response', 'github_bad_response');
+  is($e->vars->{reason},  'Unable to parse JSON response', 'reason set');
+  like($e->vars->{status}, qr/^200\b/, 'status line included');
+  is($e->vars->{content_type}, 'text/html', 'content type included');
+  ok(!exists $e->vars->{message}, 'no "message" var to collide with header');
+};
+
+subtest 'OAuth error payload is surfaced' => sub {
+  my $client = client_with(response(
+    200, 'application/json',
+    '{"error":"bad_verification_code","error_description":"The code passed is incorrect or expired."}'
+  ));
+  my $e = error_from(sub { $client->get_access_token('code') });
+  is($e->error, 'github_bad_response', 'github_bad_response');
+  is(
+    $e->vars->{reason},
+    'The code passed is incorrect or expired.',
+    'GitHub error_description used as reason'
+  );
+};
+
+subtest 'error page renders the GitHub error, not the messages fallback' => sub {
+  my $template = Bugzilla->template;
+  my $output   = '';
+  $template->process(
+    'global/code-error.html.tmpl',
+    {
+      error        => 'github_bad_response',
+      reason       => 'Unable to parse JSON response',
+      status       => '200 OK',
+      content_type => 'text/html',
+    },
+    \$output
+  ) or diag($template->error);
+  like($output, qr/Unable to parse JSON response/, 'reason rendered');
+  like($output, qr/HTTP 200 OK, text\/html/,       'status rendered');
+  unlike($output, qr/messaging functions incorrectly/,
+    'header did not treat the error as a message tag');
+};
+
+done_testing;

extensions/GitHubAuth/template/en/default/hook/global/code-error-errors.html.tmpl

@@ -12,11 +12,18 @@
 
 [% ELSIF error == "github_bad_response" %]
   [% title = "Bad Response from GitHub" %]
-  Received unexpected response from GitHub: [% message FILTER html %]
+  Received unexpected response from GitHub: [% reason FILTER html %]
+  [% IF status %]
+    (HTTP [% status FILTER html %]
+    [%~ IF content_type %], [% content_type FILTER html %][% END %])
+  [% END %]
 
 [% ELSIF error == "github_error" %]
   [% title = "GitHub Error" %]
-  GitHub returned an error: [% response.message FILTER html %]
+  GitHub returned an error: [% response.status_line FILTER html %]
+  [% IF response.header('Client-Warning') == 'Internal response' %]
+    ([% terms.Bugzilla %] could not connect to GitHub)
+  [% END %]
 
 [% ELSIF error == "github_invalid_target" %]
   [% terms.Bugzilla %] cannot log you into an external site via GitHub.