Details field in Status struct is not decoded properly

[anelson]

Bug Report

A recent change (#345) added base64 encode/decode to the custom details member of Status. Unfortunately it was not implemented properly, and as a result the details sent by the server are not the details received by the client.

Version

tonic v0.2.1

Platform

Linux shannon.assur.io 5.6.11-300.fc32.x86_64 #1 SMP Wed May 6 19:12:19 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

Crates

tonic

Description

The test in #345 is inadequate, as it tests only with a custom details value of &[1], meaning a single byte with value 0x01.

The encoding logic uses the base64 crate with an explicit config:

tonic/tonic/src/status.rs

Line 457 in 372da52

base64::encode_config(&details[..], base64::STANDARD_NO_PAD).into()

That part is correct. The decode logic on the other hand is incorrect:

tonic/tonic/src/status.rs

Lines 348 to 349 in 372da52

	base64::decode(h.as_bytes())
	.expect("Invalid status header, expected base64 encoded value")

It should be using decode_config with base64::STANDARD_NO_PAD.

[anelson]

While I still suspect there's a problem with missing decode_config, that's actually not the cause of my test failure.

Before this PR #345, any bytes passed with with_details would be returned verbatim from details(). That is no longer the case. When creating a Status object with with_details(), the details are immediately base64 encoded, so Status::details() returns the base64 encoded details. However, Status deserialized from an HTTP response header decodes the base64 encoding, so that Status::details() returns the decoded details.

This broke some tests I had that relied on the previous behavior. Basically, I manually implemented base64 encoding/decode to work around the fact that tonic didn't do that.

[anelson]

Sorry for the mistaken bug report. The change to the behavior of details() I noted above in the latest relese 0.2.1 is what caused my tests to fail, not a problem with base64 encoding as I first thought.

FWIW, I proved this to myself with a unit test, since the existing test code around custom details values was pretty minimal:

https://github.com/assuriosw/tonic/blob/bd84aab4846b5d45792187f2aed59f09d0f7f699/tonic/src/status.rs#L750-L760