1240752 - Attachment data submitted via REST API must always be base64 encoded

Status: RESOLVED FIXED

(Bugzilla :: Documentation, defect)

Description

[Christian Holler (:decoder)]

As discussed on IRC, attachments that are created using the REST API must always have their data base64 encoded, even if the data is marked as content_type plain/text.

The documentation wrongly specifies that the data can either be base64 or string instead.

Comment 1

[David Lawrence [:dkl]]

This is also the case in the upstream docs so we will fix it there and back port it down.

dkl

Comment 2

[Frédéric Buclin]

We shouldn't force plain text attachments to be encoded as base64 when uploading. If we do, then that's a bug, IMO.

Comment 3

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #2)
> We shouldn't force plain text attachments to be encoded as base64 when
> uploading. If we do, then that's a bug, IMO.

For JSONRPC and REST, we currently expect all data submitted to be base64 regardless if it is plain text or not.

Bugzilla/WebService/Bug.pm line 51:

use constant BASE64_FIELDS => {
    add_attachment => ['data'],
};

Bugzilla/WebService/Server/JSONRPC.pm line 373:

    my @base64_fields = @{ $pkg->BASE64_FIELDS->{$method} || [] };
    foreach my $field (@base64_fields) {
        if (defined $params->{$field}) {
            $params->{$field} = decode_base64($params->{$field});
        }                         
    }

Above is repeated in Bugzilla/API/1_0/Server.pm line 255.

And for XMLRPC, the XMLRPC::Deserializer module handles the base64 decoding for us.

So, we need to either
1) update the docs to state everything must always be base64 or 
2) we look at the provided content type and do not decode if $mimetype =~ /^text\// and trust the client provided the correct content type.

I will work on a patch that investigates 2) and attach here.

dkl:

Comment 4

[Christian Holler (:decoder)]

In my opinion, there's nothing wrong with forcing base64 always. In fact, I assume it makes code a little easier (I don't need to distinguish by case if I should encode or not on the client side) and also it makes things less error prone. It might easily happen that people try to submit "plaintext" things that are not actually ASCII but contain Unicode characters or whatever, leading to strange results that cause debugging time.

Comment 5

[David Lawrence [:dkl]]

(In reply to Christian Holler (:decoder) from comment #4)
> In my opinion, there's nothing wrong with forcing base64 always. In fact, I
> assume it makes code a little easier (I don't need to distinguish by case if
> I should encode or not on the client side) and also it makes things less
> error prone. It might easily happen that people try to submit "plaintext"
> things that are not actually ASCII but contain Unicode characters or
> whatever, leading to strange results that cause debugging time.

From an implementation standpoint, it would make it much easier. Would just need to update the docs. LpSolit, do you still feel that we should accept plain text as well or can we just settle on this policy?

What are your thoughts Dylan?

dkl

Comment 6

[Dylan Hardison [:dylan] (he/him)]

Base64 is best practice for binary data in JSON. Let's just fix the docs to match reality.

I don't believe the mime types we store include the charset info, so even text/plain is potentially "binary" data.
(Is UTF-8? UTF-16? Is there a BOM?).

Comment 7

[David Lawrence [:dkl]]

(In reply to Dylan William Hardison [:dylan] from comment #6)
> Base64 is best practice for binary data in JSON. Let's just fix the docs to
> match reality.
> 
> I don't believe the mime types we store include the charset info, so even
> text/plain is potentially "binary" data.
> (Is UTF-8? UTF-16? Is there a BOM?).

JSONRPC/REST already require base64. My main concern is REST as it is what is documented at bugzilla.readthedocs.org. I will just go with updating the docs and attach a patch.

dkl

Comment 8

[David Lawrence [:dkl]]

Attachment: 1240752_1.patch

Comment 9

[Gervase Markham [:gerv]]

Comment on attachment 8710679 [details] [diff] [review]
1240752_1.patch

Review of attachment 8710679 [details] [diff] [review]:
-----------------------------------------------------------------

r=gerv.

Gerv

Comment 10

[David Lawrence [:dkl]]

To ssh://[e-mail]/bugzilla/bugzilla.git
   c688265..39d8526  master -> master

To ssh://[e-mail]/bugzilla/bugzilla.git
   5de72e1..e5a69b5  5.0 -> 5.0