From 4e9fb01e76611a4cf3ef27ee20698e0939ed244d Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Sat, 24 Oct 2026 18:29:17 -0400 Subject: [PATCH 1/2] Bug 2085553 - Github logins are failing with JSON error after recent changes to Bugzilla --- extensions/GitHubAuth/lib/Client.pm | 33 ++++- extensions/GitHubAuth/t/bmo/client-response.t | 137 ++++++++++++++++++ .../hook/global/code-error-errors.html.tmpl | 11 +- 3 files changed, 174 insertions(+), 7 deletions(-) create mode 100644 extensions/GitHubAuth/t/bmo/client-response.t diff --git a/extensions/GitHubAuth/lib/Client.pm b/extensions/GitHubAuth/lib/Client.pm index a94960d7ab..dd302ff519 100644 --- a/extensions/GitHubAuth/lib/Client.pm +++ b/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; } diff --git a/extensions/GitHubAuth/t/bmo/client-response.t b/extensions/GitHubAuth/t/bmo/client-response.t new file mode 100644 index 0000000000..7f7817b6f4 --- /dev/null +++ b/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 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) = @_; + eval { $code->(); }; + return $@; +} + +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', 'Forbidden')); + 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', 'not json')); + 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; diff --git a/extensions/GitHubAuth/template/en/default/hook/global/code-error-errors.html.tmpl b/extensions/GitHubAuth/template/en/default/hook/global/code-error-errors.html.tmpl index c32dd3cd5b..8151622c71 100644 --- a/extensions/GitHubAuth/template/en/default/hook/global/code-error-errors.html.tmpl +++ b/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. From 1d7ca8ffe408d10199a58cd229955288c5300a74 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Sat, 24 Oct 2026 19:54:16 -0400 Subject: [PATCH 2/2] Copilot review fixes --- extensions/GitHubAuth/t/bmo/client-response.t | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/extensions/GitHubAuth/t/bmo/client-response.t b/extensions/GitHubAuth/t/bmo/client-response.t index 7f7817b6f4..f039c24717 100644 --- a/extensions/GitHubAuth/t/bmo/client-response.t +++ b/extensions/GitHubAuth/t/bmo/client-response.t @@ -18,6 +18,7 @@ BEGIN { use Test::More; use HTTP::Response; +use Try::Tiny; use Bugzilla; BEGIN { Bugzilla->extensions } @@ -50,8 +51,7 @@ sub client_with { sub error_from { my ($code) = @_; - eval { $code->(); }; - return $@; + return try { $code->(); undef } catch { $_ }; } subtest 'successful JSON responses are returned' => sub {