Skip to content

fix: avoid TypeError when a 403 error body is a JSON array - #2829

Open
kwy404 wants to merge 1 commit into
googleapis:mainfrom
kwy404:fix-403-list-error-reason
Open

kwy404 wants to merge 1 commit into
googleapis:mainfrom
kwy404:fix-403-list-error-reason

Conversation

@kwy404

@kwy404 kwy404 commented Sep 26, 2026

Copy link
Copy Markdown

Root cause

_should_retry_response handles two shapes of 403 error bodies. The dict branch reads the reason from data["error"]["errors"][0]["reason"], but the list branch (bodies like [{"error": {...}}], the format reported in #225) reads data[0]["error"]["errors"]["reason"]. errors is a list, so indexing it with "reason" raises TypeError. That exception is not in the caught tuple (UnicodeDecodeError, ValueError, KeyError), so it escapes HttpRequest.execute(): a list-wrapped rateLimitExceeded 403 is never retried, and any other list-wrapped 403 surfaces as TypeError: list indices must be integers or slices, not str instead of HttpError.

from googleapiclient.http import _should_retry_response
body = b'[{"error": {"code": 403, "errors": [{"reason": "rateLimitExceeded"}]}}]'
_should_retry_response(403, body)  # main: TypeError, this branch: True

The existing test_no_retry_403_list_fails did not catch this because its LIST_NOT_CONFIGURED_RESPONSE fixture is not valid JSON (the object is missing its braces), so json.loads fails before the list branch runs.

Fix

Index the first entry of errors, the same way the dict branch does. One line in googleapiclient/http.py. The fixture gets its missing braces so the existing test really exercises the list branch.

Test

  • New test_retry_403_list_rate_limit: a list-wrapped rateLimitExceeded 403 followed by a 200 should retry once and return {}.
  • Existing test_no_retry_403_list_fails, now with valid JSON: a list-wrapped accessNotConfigured 403 should raise HttpError without retrying.

Before the fix both fail with TypeError: list indices must be integers or slices, not str. After the fix both pass, and the rest of the suite gives the same results as on main.

Checklist

  • Make sure to open an issue as a bug/issue before writing your code (no separate issue; the bug and a reproduction are above)
  • Ensure the tests and linter pass (black and the CI flake8 selection are clean on the changed files)
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary): not needed

_should_retry_response read data[0]["error"]["errors"]["reason"] for
list-wrapped error bodies, but "errors" is a list, so any such 403
raised TypeError out of execute() instead of retrying on rate limits or
raising HttpError. Index the first entry, as the dict branch does.

Also make LIST_NOT_CONFIGURED_RESPONSE valid JSON so that
test_no_retry_403_list_fails actually reaches the list branch.
@kwy404
kwy404 requested a review from a team as a code owner September 26, 2026 09:03
@product-auto-label product-auto-label Bot added the size: s Pull request size is small. label Sep 26, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request resolves a bug where the error reason was incorrectly accessed from a dictionary instead of a list within the JSON response. It also updates the test suite with a valid mock response and adds a new test case for retrying on 403 rate limit errors. The reviewer recommends implementing defensive type and boundary checks when parsing the nested JSON response to prevent potential unhandled exceptions like TypeError or IndexError.

Comment thread googleapiclient/http.py
reason = reason["reason"]
else:
reason = data[0]["error"]["errors"]["reason"]
reason = data[0]["error"]["errors"][0]["reason"]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

While indexing data[0]["error"]["errors"][0]["reason"] fixes the immediate TypeError when errors is a list, this line is still highly vulnerable to other unhandled exceptions. Since the except block only catches (UnicodeDecodeError, ValueError, KeyError), any of the following scenarios will raise an unhandled TypeError, IndexError, or AttributeError and crash the request execution:

  1. data is an empty list [] (raises IndexError).
  2. data is not a list (e.g., None, int, bool, or a string) (raises TypeError).
  3. data[0] is not a dictionary (raises TypeError).
  4. data[0]["error"] is not a dictionary (raises TypeError or AttributeError if using .get()).
  5. data[0]["error"]["errors"] is an empty list or not a list (raises IndexError or TypeError).

To make this completely robust and prevent any unhandled exceptions from escaping _should_retry_response, we should perform defensive type and boundary checks before accessing these nested keys.

Suggested change
reason = data[0]["error"]["errors"][0]["reason"]
reason = None
if isinstance(data, list) and data and isinstance(data[0], dict):
error = data[0].get("error")
if isinstance(error, dict):
errors = error.get("errors")
if isinstance(errors, list) and errors and isinstance(errors[0], dict):
reason = errors[0].get("reason")

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: s Pull request size is small.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant