mirage / ocaml-rpc

Light library to deal with RPCs in OCaml
ISC License
96 stars 31 forks source link

Rfc2045 decode #171

Closed tbrk closed 2 years ago

tbrk commented 2 years ago

Some XML-RPC servers break Base64 encoded data into lines of 76 characters long (see, e.g., the Python xmlrpc library). In these cases the Base64.decode_exn function raises an exception Invalid_argument "Malformed input".

I propose, in this pull request, to decode using the Base64_rfc2045 module which handles newlines. This module also decodes base64 strings without line breaks (in "dangerous" mode) and thus supports existing behavior.

mseri commented 2 years ago

I think this should be made configurable, maybe it is better to see first how this is going to be addressed in ocaml-base64

tbrk commented 2 years ago

What would you think about allowing the caller to provide a decode function (defaulting to Base64.decode), either by threading an argument down to the point where decoding happens or using a module-level reference to set the decoding function?

tbrk commented 2 years ago

As it stands it's not possible to use ocaml-rpc to interact with a server written in Python (using the xmlrpc library) that returns a base64-encoded value.

mseri commented 2 years ago

I understand the reason behind the PR. As per the solution, I think it would be fine to provide an optional decoder parameter, which uses the current one by default but allows you to pass in anything else. We could also expose the current Base64_rfc2045 one, so that there is already the alternative available

tbrk commented 2 years ago

OK. I agree with you that some kind of parameterization is necesary to handle all the different cases (no line breaks, line breaks every 76 characters with \r\n, line breaks every 76 characters with \n, etc.). I'll draft a solution and include the suggested comment and documentation. Thanks @mseri. My current suggestion is a nasty hack.

mseri commented 2 years ago

Thanks for the contribution

tbrk commented 2 years ago

This commit allows callers of Xmlrpc to override the function used to decode base64 values. It has the advantage of externalizing the decoding strategy. I guess the correct place for base64 decoding functions is in ocaml-base64. I did not update Rpc and Rpcmarshal, but I can do this if you would like. I'm not sure how useful the override option is in these cases.

tbrk commented 2 years ago

I've updated my proof of concept to use the proposed interface.

mseri commented 2 years ago

Thanks. I like this.

I think it makes sense to leave it only on the xmlrpc module right now. I would copy your little implementation of the base 64 decoder and maybe add a comment in the Readme or the documentation showing it and why it can be useful when communicating with python. But I can do that myself before releasing, after this is merged.

thanks a lot for the contributions

tbrk commented 2 years ago

Thanks for your suggestions and maintainership! I think the final result we arrived at is reasonable and makes the library more useful.

mseri commented 2 years ago

Thanks a lot for your patience and help!

mseri commented 2 years ago

I will prepare a new release by the end of the week