Skip to content

Commit

Permalink
refactor: move verb export check to before req body validation (#2995)
Browse files Browse the repository at this point in the history
# Summary
This smol PR includes a micro optimization by ensuring that a verb is
callable (a.k.a exported OR a call within the same module) _before_
validating the request body.


# Rationale
This change is not necessary by any means. Just something that crossed
my mindwhile looking at controller logic with @wesbillman.

# Changes
* moves `!verb.IsExported` check to before validating request body
* use the current caller to see if the call is intra-module instead of
looping over all callers.

> [!NOTE]
> there may have been a reason why all callers are being looped over
instead of checking the current caller, but i couldn't think of a good
one. if someone knows definitely let me know!
  • Loading branch information
mistermoe authored Oct 4, 2024
1 parent b91f56c commit 96e9f11
Showing 1 changed file with 18 additions and 16 deletions.
34 changes: 18 additions & 16 deletions backend/controller/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -1014,34 +1014,36 @@ func (s *Service) callWithRequest(
return nil, err
}

err := ingress.ValidateCallBody(req.Msg.Body, verb, sch)
callers, err := headers.GetCallers(req.Header())
if err != nil {
observability.Calls.Request(ctx, req.Msg.Verb, start, optional.Some("invalid request: invalid call body"))
observability.Calls.Request(ctx, req.Msg.Verb, start, optional.Some("failed to get callers"))
return nil, err
}

var currentCaller *schema.Ref // might be nil but that's fine. just means that it's not a cal from another verb
if len(callers) > 0 {
currentCaller = callers[len(callers)-1]
}

module := verbRef.Module
route, ok := sstate.routes[module]
if !ok {
observability.Calls.Request(ctx, req.Msg.Verb, start, optional.Some("no routes for module"))
return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("no routes for module %q", module))

if currentCaller != nil && currentCaller.Module != module && !verb.IsExported() {
observability.Calls.Request(ctx, req.Msg.Verb, start, optional.Some("invalid request: verb not exported"))
return nil, connect.NewError(connect.CodePermissionDenied, fmt.Errorf("verb %q is not exported", verbRef))
}
client := s.clientsForEndpoint(route.Endpoint)

callers, err := headers.GetCallers(req.Header())
err = ingress.ValidateCallBody(req.Msg.Body, verb, sch)
if err != nil {
observability.Calls.Request(ctx, req.Msg.Verb, start, optional.Some("failed to get callers"))
observability.Calls.Request(ctx, req.Msg.Verb, start, optional.Some("invalid request: invalid call body"))
return nil, err
}

if !verb.IsExported() {
for _, caller := range callers {
if caller.Module != module {
observability.Calls.Request(ctx, req.Msg.Verb, start, optional.Some("invalid request: verb not exported"))
return nil, connect.NewError(connect.CodePermissionDenied, fmt.Errorf("verb %q is not exported", verbRef))
}
}
route, ok := sstate.routes[module]
if !ok {
observability.Calls.Request(ctx, req.Msg.Verb, start, optional.Some("no routes for module"))
return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("no routes for module %q", module))
}
client := s.clientsForEndpoint(route.Endpoint)

var requestKey model.RequestKey
isNewRequestKey := false
Expand Down

0 comments on commit 96e9f11

Please sign in to comment.