fix(mcp): pad body_base64, which strict decoders were rejecting #117

Merged

jlxq0 merged 2 commits from base64-padding into main

2026-08-25 07:51:19 +00:00

jlxq0 commented

2026-08-25 07:44:09 +00:00

Owner

Closes #116

download_attachment encoded with STANDARD_NO_PAD while DownloadAttachmentResult.body_base64 documented the standard alphabet. Standard-alphabet base64 is padded, so Python's base64.b64decode, Rust's STANDARD engine and Go's StdEncoding all reject it. Node's Buffer.from and atob accept it, which is why whether the bug was visible at all depended on which client tried first.

Padded rather than the doc reworded, for the reason in the issue: adding padding is backwards-compatible because lenient decoders take padded input, and the failure mode of leaving it was a caller seeing correct content_type, size_bytes and filename alongside a decoder error, and concluding the file was corrupt or still encrypted — the diagnosis size_bytes exists to make possible.

Acceptance, as the issue specifies it

an_attachment_body_decodes_with_a_strict_decoder decodes with base64::engine::general_purpose::STANDARD — not STANDARD_NO_PAD — and asserts the decoded length equals what size_bytes would carry. No padding fix-up anywhere in the test.

Control	Result
`encode_attachment` back to `STANDARD_NO_PAD`	red at `len 1`
bug kept, fixture cut to 3n lengths (`0, 3, 1002`)	green

The second control is the one worth reading. A payload whose length is a multiple of three encodes identically padded and unpadded, so a fixture of that length is blind to this fault — which is how it reached production. The fixture covers 3n, 3n+1 and 3n+2; the real payload that surfaced it was 1273922 bytes, 3n+2.

What the test cannot see, stated in its own comment: the call site choosing a different engine inline instead of calling encode_attachment. There is one such site and it sits two lines from size_bytes.

Also

AGENTS.md gets the alphabet trap. This crate needs both engines — PKCE in setup.rs requires URL_SAFE_NO_PAD per RFC 7636, attachments require padded STANDARD — so "use the other one" is not a safe default, and the 3n blindness is written down next to it.
docs/api-reference.md now says padded, and says to compare size_bytes against the decoded length.

Gates

fmt, clippy -D warnings (0 errors), 210 tests, on rustc 1.98.0 — which is now what ci.yml pins, per #114.

Cross-engine review (Codex), and what it changed

Three specific questions, one real finding acted on in 7469439.

Confirmed, not taken on trust: encode_attachment is the only producer of body_base64; nothing in the repository consumes the field (the docs/api-reference.md example is illustrative and never decoded, and send_image_from_url takes a URL and uploads raw bytes, never body_base64); and the two other base64 sites are PKCE in setup.rs, where URL_SAFE_NO_PAD is required by RFC 7636 and correct. The media-upload paths hand raw bytes to the SDK and make no alphabet choice at all.

The finding: the test asserted the decoded length against a fixture length it chose itself, never against an emitted size_bytes. The two agreed on the real path only because the same local was used twice at the call site — a convention, not a structure. attachment_result now takes one bytes and derives both fields from it, so they cannot describe different data, and the pairing is assertable without a homeserver.

Control	Result
`size_bytes` from `bytes.len() + 1`	red
`body_base64` encoded inline with `STANDARD_NO_PAD` inside the helper	red

The second closes most of the blind spot the first test admitted to. What remains is narrower: bypassing attachment_result entirely at its single call site.

A control I had claimed rather than run, now run. With the padding-length assertion deleted and STANDARD_NO_PAD restored, the strict decode alone still fails with InvalidPadding. So STANDARD genuinely rejects unpadded input and the decode is load-bearing independently of the % 4 assertion — this is not the WAV test's situation, where two mechanisms enforced one property and either could be deleted unseen. The comment now says which of the two it is.

Gates re-run: fmt, clippy -D warnings (0 errors), 211 tests on 1.98.0.

Closes #116 `download_attachment` encoded with `STANDARD_NO_PAD` while `DownloadAttachmentResult.body_base64` documented the standard alphabet. Standard-alphabet base64 is padded, so Python's `base64.b64decode`, Rust's `STANDARD` engine and Go's `StdEncoding` all reject it. Node's `Buffer.from` and `atob` accept it, which is why whether the bug was visible at all depended on which client tried first. Padded rather than the doc reworded, for the reason in the issue: adding padding is backwards-compatible because lenient decoders take padded input, and the failure mode of leaving it was a caller seeing correct `content_type`, `size_bytes` and `filename` alongside a decoder error, and concluding the file was corrupt or still encrypted — the diagnosis `size_bytes` exists to make possible. ## Acceptance, as the issue specifies it `an_attachment_body_decodes_with_a_strict_decoder` decodes with `base64::engine::general_purpose::STANDARD` — not `STANDARD_NO_PAD` — and asserts the decoded length equals what `size_bytes` would carry. No padding fix-up anywhere in the test. | Control | Result | |---|---| | `encode_attachment` back to `STANDARD_NO_PAD` | **red** at `len 1` | | bug kept, fixture cut to 3n lengths (`0, 3, 1002`) | **green** | The second control is the one worth reading. A payload whose length is a multiple of three encodes identically padded and unpadded, so a fixture of that length is blind to this fault — which is how it reached production. The fixture covers 3n, 3n+1 and 3n+2; the real payload that surfaced it was 1273922 bytes, 3n+2. What the test cannot see, stated in its own comment: the call site choosing a different engine inline instead of calling `encode_attachment`. There is one such site and it sits two lines from `size_bytes`. ## Also - `AGENTS.md` gets the alphabet trap. This crate needs *both* engines — PKCE in `setup.rs` requires `URL_SAFE_NO_PAD` per RFC 7636, attachments require padded `STANDARD` — so "use the other one" is not a safe default, and the 3n blindness is written down next to it. - `docs/api-reference.md` now says padded, and says to compare `size_bytes` against the decoded length. ## Gates fmt, clippy `-D warnings` (0 errors), 210 tests, on rustc 1.98.0 — which is now what `ci.yml` pins, per #114. --- ## Cross-engine review (Codex), and what it changed Three specific questions, one real finding acted on in `7469439`. **Confirmed, not taken on trust:** `encode_attachment` is the only producer of `body_base64`; nothing in the repository consumes the field (the `docs/api-reference.md` example is illustrative and never decoded, and `send_image_from_url` takes a URL and uploads raw bytes, never `body_base64`); and the two other base64 sites are PKCE in `setup.rs`, where `URL_SAFE_NO_PAD` is required by RFC 7636 and correct. The media-upload paths hand raw bytes to the SDK and make no alphabet choice at all. **The finding: the test asserted the decoded length against a fixture length it chose itself, never against an emitted `size_bytes`.** The two agreed on the real path only because the same local was used twice at the call site — a convention, not a structure. `attachment_result` now takes one `bytes` and derives both fields from it, so they cannot describe different data, and the pairing is assertable without a homeserver. | Control | Result | |---|---| | `size_bytes` from `bytes.len() + 1` | **red** | | `body_base64` encoded inline with `STANDARD_NO_PAD` inside the helper | **red** | The second closes most of the blind spot the first test admitted to. What remains is narrower: bypassing `attachment_result` entirely at its single call site. **A control I had claimed rather than run, now run.** With the padding-length assertion deleted *and* `STANDARD_NO_PAD` restored, the strict decode alone still fails with `InvalidPadding`. So `STANDARD` genuinely rejects unpadded input and the decode is load-bearing independently of the `% 4` assertion — this is not the WAV test's situation, where two mechanisms enforced one property and either could be deleted unseen. The comment now says which of the two it is. Gates re-run: fmt, clippy `-D warnings` (0 errors), 211 tests on 1.98.0.

jlxq0 added 1 commit

2026-08-25 07:44:09 +00:00

fix(mcp): pad body_base64, which strict decoders were rejecting

CI / cargo (pull_request) Successful in 1m21s

Details

CI / docker (pull_request) Has been skipped

Details

9e7f6daa39

`download_attachment` encoded with `STANDARD_NO_PAD` while the field
documented the standard alphabet, and standard-alphabet base64 is padded.
Python's `base64.b64decode`, Rust's `STANDARD` engine and Go's `StdEncoding`
all reject unpadded input. Node's `Buffer.from` and `atob` accept it, so
whether the bug was visible at all depended on which client tried first.

Found proving #111's acceptance end to end against v0.10.3: a real 1273922-byte
PNG came back as 1698563 characters, `len % 4 == 3`, and `b64decode` raised
`Incorrect padding`. The payload was complete — padding it by hand decoded to
exactly `size_bytes` with an intact PNG signature and IEND CRC.

The confusing part is why it is worth a fix rather than a doc change:
`content_type`, `size_bytes` and `filename` all come back correct, so the
caller sees a well-formed result and a decoder error, and concludes the file
is corrupt or still encrypted. That is precisely the diagnosis `size_bytes`
exists to make possible. Adding padding is also backwards-compatible; every
lenient decoder accepts padded input.

The test decodes with `STANDARD`, not `STANDARD_NO_PAD`, and asserts the
decoded length is `size_bytes`. Controls:

  - encode with `STANDARD_NO_PAD` again          -> red at len 1
  - keep the bug, cut the fixture to 3n lengths  -> green

The second is the reason this reached production. A payload whose length is a
multiple of three encodes identically padded and unpadded, so a fixture of
that length cannot see the fault. Both control results are in the test's own
comment, and the alphabet trap is in AGENTS.md — PKCE in setup.rs genuinely
needs the unpadded engine, so "the other one" is not a safe default here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0176noPigX2cwByDtRcafzyi

jlxq0 added 1 commit

2026-08-25 07:49:32 +00:00

test(mcp): pair size_bytes with body_base64 by construction, not convention

CI / cargo (pull_request) Successful in 1m15s

Details

CI / docker (pull_request) Has been skipped

Details

74694396c2

Codex, reviewing the padding fix, named a second blind spot: the test asserted
the decoded length against a fixture length it had chosen itself, never
against an emitted `size_bytes`. On the real path the two agreed, but only
because the same local was used twice at the call site — a convention, not a
structure, and the test would have stayed green if a later edit encoded a
different buffer or assigned `size_bytes` from somewhere else.

`attachment_result` now takes one `bytes` and derives both fields from it, so
they cannot describe different data, and the pairing is assertable without a
homeserver. Controls:

  - `size_bytes` computed from `bytes.len() + 1`     -> red
  - `body_base64` encoded inline with NO_PAD instead -> red

The second is the one worth having: the earlier test admitted it could not see
an engine chosen inline rather than through the helper, and this one can, as
long as the inline choice is inside `attachment_result`. What is left is
narrower — bypassing `attachment_result` at the single call site.

Also recorded a control I had claimed rather than run. Deleting the
padding-length assertion and restoring `STANDARD_NO_PAD` still fails, with
`InvalidPadding`, so `STANDARD` genuinely rejects unpadded input and the
strict decode is load-bearing on its own. That is not the WAV test's
situation, where two mechanisms enforced one property and either could be
deleted unseen, and the comment now says which of the two this is.

Codex also confirmed what I had not: `encode_attachment` is the only producer
of `body_base64`, nothing in the repository consumes it, and the two other
base64 sites are PKCE in setup.rs where `URL_SAFE_NO_PAD` is required by
RFC 7636 and correct. The media upload paths pass raw bytes to the SDK and
make no alphabet choice at all.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0176noPigX2cwByDtRcafzyi

jlxq0 merged commit e15a35059a into main

2026-08-25 07:51:19 +00:00

jlxq0 referenced this pull request from a commit

2026-08-25 07:51:20 +00:00

Merge pull request 'fix(mcp): pad body_base64, which strict decoders were rejecting' (#117) from base64-padding into main