Disperser api audit #1170
Disperser api audit #1170
Conversation
| @@ -32,6 +33,11 @@ func (s *DispersalServerV2) DisperseBlob(ctx context.Context, req *pb.DisperseBl | |||
| return nil, err | |||
| } | |||
|
|
|||
| // Check against payment meter to make sure there is quota remaining | |||
| if err := s.checkPaymentMeter(ctx, req); err != nil { | |||
There was a problem hiding this comment.
Separate out rate limiting checking and place it after the param validation, because this will make external RPC calls
| @@ -40,7 +46,7 @@ func (s *DispersalServerV2) DisperseBlob(ctx context.Context, req *pb.DisperseBl | |||
| blob := req.GetBlob() | |||
| blobHeader, err := corev2.BlobHeaderFromProtobuf(req.GetBlobHeader()) | |||
| if err != nil { | |||
| return nil, api.NewErrorInternal(err.Error()) | |||
| return nil, api.NewErrorInvalidArg(fmt.Sprintf("failed to parse the blob header proto: %w", err)) | |||
There was a problem hiding this comment.
This isn't internal error
| @@ -20,12 +20,12 @@ func (s *DispersalServerV2) GetBlobStatus(ctx context.Context, req *pb.BlobStatu | |||
| }() | |||
|
|
|||
| if req.GetBlobKey() == nil || len(req.GetBlobKey()) != 32 { | |||
| return nil, api.NewErrorInvalidArg("invalid blob key") | |||
| return nil, api.NewErrorInvalidArg("blob key must be present and with 32 bytes") | |||
There was a problem hiding this comment.
Informative error messages here and below
| @@ -32,6 +33,11 @@ func (s *DispersalServerV2) DisperseBlob(ctx context.Context, req *pb.DisperseBl | |||
| return nil, err | |||
| } | |||
|
|
|||
| // Check against payment meter to make sure there is quota remaining | |||
| if err := s.checkPaymentMeter(ctx, req); err != nil { | |||
| return nil, err | |||
There was a problem hiding this comment.
Shouldn't we be using api.NewErrorX for this return. Also noticed there are some other areas that aren't doing that.
There was a problem hiding this comment.
Isn't it already wrapped inside the function?
There was a problem hiding this comment.
Oh I see, seems like one level of indirection though. I was thinking the top level would handle that.
There was a problem hiding this comment.
yea the implementation here had chosen to let low level functions to classify the errors.
There was a problem hiding this comment.
actually like moving it up to have less redundant code. validateDispersalRequest should be homogeneous and all its errors are (and should be) invalid requests.
Why are these changes needed?
A pass of audit of the disperser/apiserver
Checks