640756 - Make the documentation clearer that attachments created with Bug.add_attachment must be of type 'base64'

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect)

Description

[Kim B. Nielsen]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_6; da-dk) AppleWebKit/533.19.4 (KHTML, like Gecko) Version/5.0.3 Safari/533.19.4
Build Identifier: 4.0

When an attachment is made, it is required to send base64 encoded data to the server (XMLRPC). However, that data does not get decoded before it is put into the database.

Attached is patch that fixes this issue.

Reproducible: Always

Steps to Reproduce:
1. Add an attachment through web services (Fx. a png)
2. Try to download the png through the web interface. It won't work, since it is still base64 encoded.
3.
Actual Results:  
Attachments through the webservices are invalid.

Expected Results:  
Attachments through the web services are valid.

Patch attached

Comment 1

[Kim B. Nielsen]

Attachment: Patch that fixes the base64 decoding issue.

Comment 2

[Frédéric Buclin]

Seems valid to me. From what I can see, JSONRPC is working fine, but XMLRPC isn't.

Comment 3

[Max Kanat-Alexander]

Comment on attachment 518505 [details] [diff] [review]
Patch that fixes the base64 decoding issue.

You are sending your data as a <string> instead of as a <base64>.

Comment 4

[Max Kanat-Alexander]

You must send your data with the appropriate XML-RPC datatype.

Comment 5

[Frédéric Buclin]

(In reply to comment #4)
> You must send your data with the appropriate XML-RPC datatype.

In that case, webservice_bug_add_attachment.t is wrong too, because the encoded data is inserted into the DB. Attachments are unreadable. So either there is a real bug, or the QA script must be fixed, in which case we can morphe this bug summary.

Comment 6

[Kim B. Nielsen]

Well, it worked by changing the datatype to base64 - I mistakenly just encoded it as base64, so I guess it was a misinterpretation on my part. Sorry for the false alarm.

But yes, I discovered it by noticing that the base64 data was inserted into the database, and thus unreadable for the client. I think some input validation would be a good idea, so that bugzilla-webservices-newbees like me get an error instead of grey hairs :)

Comment 7

[Max Kanat-Alexander]

(In reply to comment #5)
> In that case, webservice_bug_add_attachment.t is wrong too, because the encoded
> data is inserted into the DB. Attachments are unreadable.

  Are you sure? That test inserts random characters that would look much like base64. If it really is doing that (which I don't think it is, since it validates the returned data in both XML-RPC and JSON-RPC) then file a separate bug about it.

(In reply to comment #6)
> I think some input validation would be a good idea, so that bugzilla-
> webservices-newbees like me get an error instead of grey hairs :)

  Yeah, I thought about that too, but I think the option to send a <string> is actually sort of a feature and not a bug--that is, if you just want to insert text into the DB, that does make it simpler (no need to base64-encode).

Comment 8

[Frédéric Buclin]

(In reply to comment #7)
>   Are you sure?

Yes, I'm sure.

Comment 11

[Dave Miller [:justdave]]

(In reply to Max Kanat-Alexander from comment #7)
>   Yeah, I thought about that too, but I think the option to send a <string>
> is actually sort of a feature and not a bug--that is, if you just want to
> insert text into the DB, that does make it simpler (no need to
> base64-encode).

Then we should document it that way.

Comment 12

[Dave Miller [:justdave]]

Attachment: Patch v2

This patch updates the POD docs to illustrate the ability to send as a string type, and point out that base64 is required when encoding non-ascii data

Comment 13

[Frédéric Buclin]

Comment on attachment 683138 [details] [diff] [review]
Patch v2

>+B<Required> C<base64> or C<string> The content of the attachment.
>+If the content of the attachment is not ASCII text, you must send
>+it using the C<base64> type.

r=LpSolit, though I suspect users will still mix "encoded using base64 + passing it as string" and "encoded using base64 + declaring the encoded string as base64". Maybe you could make this even clearer: "encoding your attachment as base64 is not enough. You must declare it as being of type base64 too" or something like that.

Comment 14

[Dave Miller [:justdave]]

looks good to me.

Comment 15

[Dave Miller [:justdave]]

Attachment: Patch v3

Updated wording to be more explicit.

This patch applies cleanly on all branches 4.0 and newer.  Any objection to committing it on all of 4.0, 4.2, 4.4, and trunk?

Comment 16

[Frédéric Buclin]

Comment on attachment 683472 [details] [diff] [review]
Patch v3

r=LpSolit

Comment 17

[Frédéric Buclin]

It's fine to fix doc on all supported branches, so no problem to take it for 4.0 and newer (this method doesn't exist in 3.6).

Comment 18

[Dave Miller [:justdave]]

Bug 640756 - Make the documentation clearer that attachments created with Bug.add_attachment must by of type 'base64' when non-ASCII
.
r=LpSolit, a=LpSolit

Committing to: /home/dave/Source/bugzilla/
modified Bugzilla/WebService/Bug.pm
Committed revision 8481.
Pushed up to revision 8481.

Committing to: /home/dave/Source/bugzilla-4.4/
modified Bugzilla/WebService/Bug.pm
Committed revision 8465.
Pushed up to revision 8465.

Committing to: /home/dave/Source/bugzilla-4.2/
modified Bugzilla/WebService/Bug.pm
Committed revision 8173.
Pushed up to revision 8173.

Committing to: /home/dave/Source/bugzilla-4.0/
modified Bugzilla/WebService/Bug.pm
Committed revision 7737.
Pushed up to revision 7737.