tafia / quick-protobuf

A rust implementation of protobuf parser
MIT License
446 stars 82 forks source link

proto3 optionals ignored #219

Open q3k opened 2 years ago

q3k commented 2 years ago

Protobuf 3.15 stabilized 'optional' in proto3: https://github.com/protocolbuffers/protobuf/releases/tag/v3.15.0

These, however, behave entirely differently from proto2 optionals. Thus, the following behaviour (effectively ignoring the 'optional' marker) is invalid:

q3k@sizeableunit ~/lolproto $ cat test.proto 
syntax = "proto3";

message Lol {
    bool foo = 1;
    optional bool foo = 2;
}
q3k@sizeableunit ~/lolproto $ pb-rs test.proto 
Found 1 messages, and 0 enums
Writing message Lol
q3k@sizeableunit ~/lolproto $ grep -A 3 'struct Lol' test.rs 
pub struct Lol {
    pub foo: bool,
    pub foo: bool,
}

Instead, quick-protobuf should either a) error out on 'optional' not being supported fully for proto3 b) implement proto3 optionals fully by emitting Option struct members for fields marked as optional, and correctly (un)marshal them from/to the wire format.

q3k commented 2 years ago

There is one scenario where this is particularly scary:

service RoleManager {
    rpc EditUser(EditUserRequest) returns (EditUserResponse);
}

message EditUserRequest {
    string username = 1;
    // If set and true: add administrator role ; if set and false: remove administrator role ; if unset: ignore.
    optional bool administrator = 2;
    string real_name = 3;
}

A fully working proto3 client implementation attempting to send { username: "admin", real_name: "Admin", ...Self::default() } will send a wire representation with field 2 being absent, thereby not modifying the administrative role of the user. Quick-protobuf will instead default to administrator: false on the wire, thereby 'accidentally' removing the administrator role from the user.

lamafab commented 1 year ago

I'm confused about this, same issue here. According to the README this should work?