download_attachment emits unpadded base64 while documenting standard alphabet, so strict decoders reject it #116

New issue

Closed

opened 2026-08-25 07:36:50 +00:00 by jlxq0 · 1 comment

jlxq0 commented

2026-08-25 07:36:50 +00:00

Owner

DownloadAttachmentResult.body_base64 is documented as "Base64-encoded
(standard alphabet, no line breaks) file contents". It is emitted without
padding, and standard-alphabet base64 is padded. A strict decoder rejects it.

Observed against v0.10.3 (sha256:81e26f67…), fetching a real 1273922-byte
PNG from an E2EE room:

b64 length: 1698563    len % 4 = 3
tail:       'ORK5CYII'

>>> base64.b64decode(b64)
binascii.Error: Incorrect padding

base64.b64decode(b64 + '=' * (-len(b64) % 4)) then decodes to exactly 1273922
bytes, matching size_bytes, with a valid PNG signature and an ae 42 60 82
IEND CRC at the tail. So the payload is correct and complete; only the padding
is missing.

Python's base64.b64decode is strict by default, Rust's base64 engine with
STANDARD is strict, and Go's base64.StdEncoding is strict. Any of them fails
on this. Node's Buffer.from(s, 'base64') and atob tolerate it, which is why
it can go unnoticed: the first client to try it may be one of the lenient ones.

The failure is also confusing rather than obvious. content_type, size_bytes
and filename all come back correct, so a caller sees a well-formed result and a
decoder error, and the natural conclusion is that the file is corrupt or still
encrypted — which is precisely the diagnosis this field exists to make possible.

Fix

Either pad the output, or change the doc comment to say unpadded and name the
alphabet precisely (STANDARD_NO_PAD). Padding it is the better of the two:
the field is already documented as standard, callers on strict decoders are the
majority, and the change is backwards-compatible because lenient decoders accept
padded input.

Acceptance

Not "it round-trips in our tests". Decode the field with a strict decoder and
assert the result equals size_bytes — base64::engine::general_purpose::STANDARD
in Rust, or base64.b64decode(s) with no padding fix-up. Break it by removing
one character and watch the assertion fail; a test that pads before decoding
cannot observe this bug at all, which is how it got here.

Provenance

Found while proving #111's acceptance end to end: fetched
$mr1GZEv2zJuHc87Xl-DMmP1VbgP9Jp5KV1ScA9R9-6E from
!nJqaJVNKzmgkUjjSLE:kampong.social through the /mcp mount on v0.10.3. The
decrypted image is correct and readable; the padding cost one round of
diagnosis.

`DownloadAttachmentResult.body_base64` is documented as "Base64-encoded (standard alphabet, no line breaks) file contents". It is emitted **without padding**, and standard-alphabet base64 is padded. A strict decoder rejects it. Observed against `v0.10.3` (`sha256:81e26f67…`), fetching a real 1273922-byte PNG from an E2EE room: b64 length: 1698563 len % 4 = 3 tail: 'ORK5CYII' >>> base64.b64decode(b64) binascii.Error: Incorrect padding `base64.b64decode(b64 + '=' * (-len(b64) % 4))` then decodes to exactly 1273922 bytes, matching `size_bytes`, with a valid PNG signature and an `ae 42 60 82` IEND CRC at the tail. So the payload is correct and complete; only the padding is missing. Python's `base64.b64decode` is strict by default, Rust's `base64` engine with `STANDARD` is strict, and Go's `base64.StdEncoding` is strict. Any of them fails on this. Node's `Buffer.from(s, 'base64')` and `atob` tolerate it, which is why it can go unnoticed: the first client to try it may be one of the lenient ones. The failure is also confusing rather than obvious. `content_type`, `size_bytes` and `filename` all come back correct, so a caller sees a well-formed result and a decoder error, and the natural conclusion is that the file is corrupt or still encrypted — which is precisely the diagnosis this field exists to make possible. ## Fix Either pad the output, or change the doc comment to say unpadded and name the alphabet precisely (`STANDARD_NO_PAD`). Padding it is the better of the two: the field is already documented as standard, callers on strict decoders are the majority, and the change is backwards-compatible because lenient decoders accept padded input. ## Acceptance Not "it round-trips in our tests". Decode the field with a **strict** decoder and assert the result equals `size_bytes` — `base64::engine::general_purpose::STANDARD` in Rust, or `base64.b64decode(s)` with no padding fix-up. Break it by removing one character and watch the assertion fail; a test that pads before decoding cannot observe this bug at all, which is how it got here. ## Provenance Found while proving #111's acceptance end to end: fetched `$mr1GZEv2zJuHc87Xl-DMmP1VbgP9Jp5KV1ScA9R9-6E` from `!nJqaJVNKzmgkUjjSLE:kampong.social` through the `/mcp` mount on `v0.10.3`. The decrypted image is correct and readable; the padding cost one round of diagnosis.

jlxq0 commented

2026-08-25 07:39:37 +00:00

Author

Owner

Confirmed at source, and the cause I wrote in the body is wrong. Correcting it
here rather than quietly, because the wrong cause implies a different fix.

Verified on origin/main:

src/mcp.rs:5178 — base64::engine::general_purpose::STANDARD_NO_PAD.encode(&bytes)
src/mcp.rs:2333 — the field's doc, twelve lines up: "Base64-encoded
(standard alphabet, no line breaks)". It names the alphabet, names the line
breaks, and is silent on padding. A reader takes that as standard base64, and
standard base64 is padded. Code and doc disagree in the one respect that
decides whether a strict decoder accepts the output.
general_purpose::STANDARD appears nowhere in src/. STANDARD_NO_PAD is the
only standard-alphabet engine in the tree, so nothing internal contradicts the
choice, which is part of why it reads as deliberate.

What I claimed: a test that padded before decoding hid the defect.

Actual: there is no decode site anywhere in src/. git grep '\.decode('
over origin/main returns nothing, and body_base64 appears in exactly three
places — the field declaration at 2333, the encode at 5178, and the struct
initialiser at 5185. Nothing in this repository has ever decoded the encoder's
output. So it is not a check that could not observe the defect; it is the absence
of any check at all, on a field whose entire purpose is to be decoded by
somebody else.

Therefore the acceptance in the body is too weak, and this replaces it. The
test must decode real download_attachment output with Rust's
base64::engine::general_purpose::STANDARD and assert success, and it must fail
before the fix. STANDARD rejects unpadded input, so that test can fail today,
which is the only property that makes it evidence. A test using
STANDARD_NO_PAD passes in both worlds and is worse than no test, because it
looks like coverage of exactly this field.

Found by Clark reading the source after the defect was observed from outside.

Confirmed at source, and the cause I wrote in the body is wrong. Correcting it here rather than quietly, because the wrong cause implies a different fix. **Verified on `origin/main`:** - `src/mcp.rs:5178` — `base64::engine::general_purpose::STANDARD_NO_PAD.encode(&bytes)` - `src/mcp.rs:2333` — the field's doc, twelve lines up: "Base64-encoded (standard alphabet, no line breaks)". It names the alphabet, names the line breaks, and is silent on padding. A reader takes that as standard base64, and standard base64 is padded. Code and doc disagree in the one respect that decides whether a strict decoder accepts the output. - `general_purpose::STANDARD` appears nowhere in `src/`. `STANDARD_NO_PAD` is the only standard-alphabet engine in the tree, so nothing internal contradicts the choice, which is part of why it reads as deliberate. **What I claimed:** a test that padded before decoding hid the defect. **Actual:** there is no decode site anywhere in `src/`. `git grep '\.decode('` over `origin/main` returns nothing, and `body_base64` appears in exactly three places — the field declaration at 2333, the encode at 5178, and the struct initialiser at 5185. Nothing in this repository has ever decoded the encoder's output. So it is not a check that could not observe the defect; it is the absence of any check at all, on a field whose entire purpose is to be decoded by somebody else. **Therefore the acceptance in the body is too weak, and this replaces it.** The test must decode real `download_attachment` output with Rust's `base64::engine::general_purpose::STANDARD` and assert success, and it must fail before the fix. `STANDARD` rejects unpadded input, so that test can fail today, which is the only property that makes it evidence. A test using `STANDARD_NO_PAD` passes in both worlds and is worse than no test, because it looks like coverage of exactly this field. Found by Clark reading the source after the defect was observed from outside.

jlxq0 referenced this issue from a pull request that will close it,

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

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

jlxq0 closed this issue

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