-
Notifications
You must be signed in to change notification settings - Fork 282
chore: move multi error handler to shared #2333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| package auth | ||
|
|
||
| import ( | ||
| "errors" | ||
| "fmt" | ||
| "strings" | ||
|
|
||
| "github.com/getkin/kin-openapi/openapi3" | ||
| "github.com/getkin/kin-openapi/openapi3filter" | ||
| ) | ||
|
|
||
| const ( | ||
| SecurityErrPrefix = "error in openapi3filter.SecurityRequirementsError: security requirements failed: " | ||
| ForbiddenErrPrefix = "team forbidden: " | ||
| BlockedErrPrefix = "team blocked: " | ||
| ) | ||
|
|
||
| // MultiErrorHandler handles wrapped SecurityRequirementsError, so there are no multiple errors returned to the user. | ||
| func MultiErrorHandler(me openapi3.MultiError) error { | ||
| if len(me) == 0 { | ||
| return nil | ||
| } | ||
| err := me[0] | ||
|
|
||
| // Recreate logic from oapi-codegen/gin-middleware to handle the error | ||
| // Source: https://github.com/oapi-codegen/gin-middleware/blob/main/oapi_validate.go | ||
| switch e := err.(type) { //nolint:errorlint // we copied this and don't want it to change | ||
| case *openapi3filter.RequestError: | ||
| // We've got a bad request | ||
| // Split up the verbose error by lines and return the first one | ||
| // openapi errors seem to be multi-line with a decent message on the first | ||
| errorLines := strings.Split(e.Error(), "\n") | ||
|
|
||
| return fmt.Errorf("error in openapi3filter.RequestError: %s", errorLines[0]) | ||
| case *openapi3filter.SecurityRequirementsError: | ||
| return processCustomErrors(e) // custom implementation | ||
| default: | ||
| // This should never happen today, but if our upstream code changes, | ||
| // we don't want to crash the server, so handle the unexpected error. | ||
| return fmt.Errorf("error validating request: %w", err) | ||
| } | ||
| } | ||
|
|
||
| func processCustomErrors(e *openapi3filter.SecurityRequirementsError) error { | ||
| // Return only one security requirement error (there may be multiple securitySchemes) | ||
| unwrapped := e.Errors | ||
| err := unwrapped[0] | ||
|
|
||
| var teamForbidden *TeamForbiddenError | ||
| var teamBlocked *TeamBlockedError | ||
| // Return only the first non-missing authorization header error (if possible) | ||
| for _, errW := range unwrapped { | ||
| if errors.Is(errW, ErrNoAuthHeader) { | ||
| continue | ||
| } | ||
|
|
||
| if errors.As(errW, &teamForbidden) { | ||
| return fmt.Errorf("%s%s", ForbiddenErrPrefix, err.Error()) | ||
| } | ||
|
|
||
| if errors.As(errW, &teamBlocked) { | ||
| return fmt.Errorf("%s%s", BlockedErrPrefix, err.Error()) | ||
| } | ||
|
|
||
| err = errW | ||
|
|
||
| break | ||
| } | ||
|
|
||
| return fmt.Errorf("%s%s", SecurityErrPrefix, err.Error()) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,13 +9,11 @@ import ( | |
| "os" | ||
| "os/signal" | ||
| "strconv" | ||
| "strings" | ||
| "sync" | ||
| "sync/atomic" | ||
| "syscall" | ||
| "time" | ||
|
|
||
| "github.com/getkin/kin-openapi/openapi3" | ||
| "github.com/getkin/kin-openapi/openapi3filter" | ||
| "github.com/gin-contrib/cors" | ||
| "github.com/gin-gonic/gin" | ||
|
|
@@ -214,14 +212,7 @@ func run() int { | |
| "message": message, | ||
| }) | ||
| }, | ||
| MultiErrorHandler: func(me openapi3.MultiError) error { | ||
| msgs := make([]string, 0, len(me)) | ||
| for _, e := range me { | ||
| msgs = append(msgs, e.Error()) | ||
| } | ||
|
|
||
| return fmt.Errorf("%s", strings.Join(msgs, "; ")) | ||
| }, | ||
| MultiErrorHandler: sharedauth.MultiErrorHandler, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Behavioral regression: the previous handler joined all errors from openapi3.MultiError; this replacement only processes me[0] and returns errors prefixed with SecurityErrPrefix for security failures. Unlike packages/api, the dashboard-api error handler does not strip these prefixes. Auth/security validation failures would now expose the raw internal error string in API responses. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Behavioral regression: the previous handler joined all errors from Unlike |
||
| Options: openapi3filter.Options{ | ||
| AuthenticationFunc: authenticationFunc, | ||
| }, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential panic:
unwrapped[0]is accessed without a bounds check. This risk already existed inpackages/api, but it is now also reachable fromdashboard-api, which previously used a simple join handler that never hit this code path. If aSecurityRequirementsErroris ever produced with an emptyErrorsslice this will panic.