fix(gmail): restore base64url padding before decoding message bodies - #200
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesGmail tools now use shared strict Base64URL decoding for email bodies, MIME parts, and attachments. Tests cover unpadded data, invalid Base64URL, non-UTF-8 content, and attachment sizes. Gmail Base64URL decoding
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cbe9dc5 to
e06a08b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/providers/google/gmail/test_tools.py`:
- Around line 198-220: Extend the regression tests around
test_success_with_unpadded_single_part_body and the gmail_get_attachment
coverage to exercise unpadded text/html decoding and attachment decoding. Use a
body whose Base64 representation ends with “==” so both padding characters are
removed, while preserving the existing successful result assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4063dd8a-c043-452d-a4f1-3498777e26ee
📒 Files selected for processing (2)
src/apron_tools/providers/google/gmail/tools.pytests/providers/google/gmail/test_tools.py
The Gmail API returns MessagePartBody.data as base64url that commonly omits the trailing '=', which base64.urlsafe_b64decode rejects with binascii.Error. _extract_body decoded that field directly at three sites with no padding normalization, and every one of them sits inside a try/except or contextlib.suppress -- so an unpadded body did not raise, it silently became "(Could not decode email body)" or "(No email body found)". gmail_get_attachment already restores the padding. This lifts that normalization into a shared _decode_base64url helper and routes the three _extract_body sites plus gmail_get_attachment through it, so the module handles the wire format consistently. Addresses the first half of mozilla-ai#169. Two tests, both verified failing without the change: the single-part path returned "(Could not decode email body)" and the multipart text/plain path returned "(No email body found)".
…e sites Review follow-up to the shared _decode_base64url helper. The four decode sites caught failure inconsistently: _extract_body used broad `except Exception` / `contextlib.suppress(Exception)`, while gmail_get_attachment caught `(binascii.Error, ValueError)`. Both binascii.Error (bad base64) and UnicodeDecodeError (bad UTF-8) subclass ValueError, so every site now catches ValueError -- narrower at the body sites (no longer swallowing unrelated bugs) and without the redundant tuple (binascii.Error subclasses ValueError), which lets the now-unused binascii import go. Document the helper's decode-failure contract, noting the decode is lenient and does not validate the payload, and add regression tests pinning both failure modes (invalid base64 length; valid base64 that decodes to non-UTF-8 bytes) at the narrowed catch sites.
23109b4 to
9db3ff6
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/apron_tools/providers/google/gmail/tools.py`:
- Around line 60-80: Update _decode_base64url to validate the padded Base64URL
input using base64.b64decode with altchars=b"-_" and validate=True, rather than
lenient urlsafe_b64decode; preserve padding restoration and allow valid padded
or unpadded input. Add regression coverage for an invalid alphabet character,
alongside the existing invalid-length case, and ensure both raise ValueError.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a709ce28-9a35-4cd8-b4f2-b9ca3462295d
📒 Files selected for processing (2)
src/apron_tools/providers/google/gmail/tools.pytests/providers/google/gmail/test_tools.py
There was a problem hiding this comment.
LGTM, thanks @shoemoney .. I added some fixups on top, but looks almost ready to go!
(Had to rebase just FYI)
Lenient urlsafe_b64decode silently discards characters outside the base64url alphabet, so corrupt Gmail data decoded to truncated bytes instead of raising. Decode with validate=True and altchars=b"-_" so those payloads surface as a decode failure, and cover the invalid- alphabet case alongside the existing invalid-length one.
|
Pushed in 20ec75d — thanks for the rebase and the fixups.
Added 66 passing locally. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/apron_tools/providers/google/gmail/tools.py (1)
66-80: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winEnforce the documented Base64URL alphabet.
altchars=b"-_"makes-and_alternatives, but+and/remain accepted._decode_base64url()documents that characters outside the base64url alphabet are rejected, so++==and//==contradict the helper contract.Reject
+and/before callingbase64.b64decode, or update the helper contract to accept standard Base64 characters.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/apron_tools/providers/google/gmail/tools.py` around lines 66 - 80, Update _decode_base64url() to explicitly reject standard Base64 characters '+' and '/' before calling base64.b64decode, preserving the documented Base64URL-only alphabet and existing validation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/providers/google/gmail/test_tools.py`:
- Around line 294-316: Update the gmail body decoding test in
test_single_part_body_with_invalid_base64_alphabet_returns_placeholder to use an
invalid Base64 character that the older lenient decoder would still accept after
trimming, rather than an input like ab*d that fails for padding reasons. Keep
the same gmail_read_email assertion path and replace the payload body data with
a complete Base64 block plus an invalid suffix so the strict decoder in the test
fixture is exercised correctly.
---
Outside diff comments:
In `@src/apron_tools/providers/google/gmail/tools.py`:
- Around line 66-80: Update _decode_base64url() to explicitly reject standard
Base64 characters '+' and '/' before calling base64.b64decode, preserving the
documented Base64URL-only alphabet and existing validation behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: accfe652-f566-404c-8343-22d0a6378d01
📒 Files selected for processing (2)
src/apron_tools/providers/google/gmail/tools.pytests/providers/google/gmail/test_tools.py
…test The regression added in 20ec75d used `ab*d`, which the lenient decoder also rejects -- it discards `*`, leaving `abd`, which then fails the padding check -- so the test passed even without the validate=True change. Use `YWJj*`: a complete valid block plus a stray non-alphabet character that the lenient decoder silently accepts (decoding "abc") but strict validation rejects, so the test now fails without the fix.
2 participants
Addresses the first half of #169.
The bug
The Gmail API returns
MessagePartBody.dataas base64url that commonly omits the trailing=, whichbase64.urlsafe_b64decoderejects withbinascii.Error: Incorrect padding._extract_bodydecodes that field at three sites with no padding restoration, and every one of them sits inside atry/exceptorcontextlib.suppress:So an unpadded body never raises. It silently becomes
"(Could not decode email body)"on the single-part path, or falls through to"(No email body found)"on the multipart path — a perfectly readable email arriving at the caller as placeholder text. Padding is only present when the decoded length is a multiple of 3, so this fires on roughly two thirds of message lengths.The fix
gmail_get_attachmentalready restores the padding, with a comment explaining why. This lifts that normalization into a shared_decode_base64urlhelper and routes the three_extract_bodysites andgmail_get_attachmentthrough it, so the module handles the wire format the same way everywhere — the shared helper #169 asks for.The restoration is a no-op on already-padded input, so nothing that worked before changes.
Verification
Both new tests fail on
mainwithout the source change:They are modeled on the existing
test_success_with_unpadded_data, which already covers this class of bug for the attachment path.With the change:
Deliberately not fixed here
The second half of #169 — the ~10 unguarded
resp.json()sites across the module — is a separate concern and is left for its own PR. Happy to follow up with it if you'd like it in the same pass.Summary by CodeRabbit
Bug Fixes
Tests