diff --git a/README.md b/README.md index 9de2727..6df9b69 100644 --- a/README.md +++ b/README.md @@ -36,7 +36,7 @@ go get github.com/labbs/fiber-oapi/v3 > > Everything above is opt-in, except **three changes of behavior**: > 1. **[Authentication runs first.](#authentication-runs-first)** A protected route checks the credentials before reading its input: a request that fails authentication gets its 401 without its body being decoded, even if that body is malformed or invalid (it used to get a 400 or 422). -> 2. **Path, query and header values now win over the body.** The body used to be decoded after the path and query, and the JSON decoder matches keys case-insensitively: a field tagged `uri:"id"` without `json:"-"` could be overwritten by `{"id": "..."}` in the body. The body is now decoded first. +> 2. **Path, query and header values now win over the body.** The body used to be decoded after the path and query, and the JSON decoder matches keys case-insensitively: a field tagged `uri:"id"` without `json:"-"` could be overwritten by `{"id": "..."}` in the body. The body is now decoded first, and path, query and header fields are reset after it, so they never take a value from the body — not even when the parameter is absent from the request (fixed in v3.3.1). > 3. **`uri` fields no longer appear in the documented request body.** They were already documented as path parameters. > > Also: `SmartAuthMiddleware` and `MultiSchemeAuthMiddleware` now go through `Config.AuthErrorHandler` when one is set. @@ -136,7 +136,7 @@ fiberoapi.Put(router, path, handler, options) fiberoapi.Patch(router, path, handler, options) fiberoapi.Delete(router, path, handler, options) fiberoapi.Head(router, path, handler, options) -fiberoapi.Method(method, router, path, handler, options) // Custom HTTP method +fiberoapi.Method(router, method, path, handler, options) // Custom HTTP method ``` ## Parameter Types @@ -150,8 +150,11 @@ type MyInput struct { } ``` -The body is decoded first, then the path, query and header values are bound: -they always win over a body key with the same name. +Path, query and header fields never take their value from the body, even when +the parameter is absent from the request: the body is decoded first, those +fields are reset, then bound from the request. A field that also declares a +body tag (e.g. `query:"sort" json:"sort"`) accepts either, and the request +parameter wins when present. Special tags: - `body:"raw"` — Receive the request body as-is in a `[]byte` field (see [Raw request body](#raw-request-body-binary)) @@ -524,8 +527,8 @@ AuthErrorHandler: func(c fiber.Ctx, err *fiberoapi.AuthError) error { Any error value — a `TError`, an entry of `OpenAPIOptions.Errors` — can choose its `Content-Type` by implementing `ContentTyper`; `Problem` answers -`application/problem+json`. A handler can therefore return a `*Problem` as its -`TError`: +`application/problem+json`. A handler can therefore return a `Problem` or a +`*Problem` as its `TError`: ```go type ContentTyper interface{ ContentType() string } @@ -791,6 +794,9 @@ oapi := fiberoapi.New(app, fiberoapi.Config{ answers 500 (`ErrResourceAuthorizationNotConfigured`), never an access granted by default. +A validator that returns `(nil, nil)` — no error, but no `AuthContext` either — +is treated as a failed authentication (401), whatever the scheme. + A service that needs the request to validate a Bearer token (client IP, request context...) can implement `ContextTokenValidator`; it is then used instead of `ValidateToken`: diff --git a/auth_schemes.go b/auth_schemes.go index 110bf66..dc0c13a 100644 --- a/auth_schemes.go +++ b/auth_schemes.go @@ -98,7 +98,7 @@ func validateBasicAuth(c fiber.Ctx, authService AuthorizationService) (*AuthCont return nil, fmt.Errorf("invalid Basic auth format: expected username:password") } - return basicValidator.ValidateBasicAuth(parts[0], parts[1]) + return requireAuthContext(basicValidator.ValidateBasicAuth(parts[0], parts[1])) } // validateAPIKey validates an API key from header, query, or cookie. @@ -124,7 +124,7 @@ func validateAPIKey(c fiber.Ctx, scheme SecurityScheme, authService Authorizatio return nil, fmt.Errorf("API key not found in %s parameter '%s'", scheme.In, scheme.Name) } - return apiKeyValidator.ValidateAPIKey(key, scheme.In, scheme.Name) + return requireAuthContext(apiKeyValidator.ValidateAPIKey(key, scheme.In, scheme.Name)) } // validateAWSSigV4 validates an AWS Signature V4 Authorization header. @@ -161,7 +161,7 @@ func validateAWSSigV4(c fiber.Ctx, authService AuthorizationService) (*AuthConte params.Headers[headerName] = c.Get(headerName) } - return awsValidator.ValidateAWSSignature(params) + return requireAuthContext(awsValidator.ValidateAWSSignature(params)) } // parseAWSSigV4Header parses an AWS SigV4 Authorization header into its components. diff --git a/binding_order_test.go b/binding_order_test.go index 6371115..94ef945 100644 --- a/binding_order_test.go +++ b/binding_order_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "io" "net/http/httptest" + "reflect" "strings" "testing" @@ -68,3 +69,64 @@ func TestBindingOrder_BodySchemaExcludesPathParameters(t *testing.T) { assert.Contains(t, props, "name") assert.NotContains(t, schema, "required", "the only required field is the path parameter") } + +// proxyInput mimics a handler that trusts identity headers set by a proxy. +type proxyInput struct { + ID string `uri:"id"` + UserID string `header:"X-User-Id"` + Filter string `query:"filter"` + Name string `json:"name"` + // Sort declares both: it is accepted from the query or from the body. + Sort string `query:"sort" json:"sort"` +} + +func TestBindingOrder_AbsentParametersAreNotFilledFromTheBody(t *testing.T) { + var got proxyInput + app := fiber.New() + oapi := New(app) + Put(oapi, "/items/:id", func(c fiber.Ctx, in proxyInput) (fiber.Map, struct{}) { + got = in + return fiber.Map{}, struct{}{} + }, OpenAPIOptions{}) + + send := func(contentType, body, query string) { + t.Helper() + got = proxyInput{} + req := httptest.NewRequest("PUT", "/items/from-path"+query, strings.NewReader(body)) + req.Header.Set("Content-Type", contentType) + resp, err := app.Test(req) + require.NoError(t, err) + require.Equal(t, 200, resp.StatusCode) + } + + send("application/json", `{"id":"evil","userId":"evil","x-user-id":"evil","filter":"evil","name":"widget","sort":"from-body"}`, "") + assert.Equal(t, "from-path", got.ID) + assert.Empty(t, got.UserID, "a header absent from the request must not come from the body") + assert.Empty(t, got.Filter, "a query parameter absent from the request must not come from the body") + assert.Equal(t, "widget", got.Name) + assert.Equal(t, "from-body", got.Sort, "a field that also declares a json tag accepts the body") + + send("application/json", `{"sort":"from-body"}`, "?sort=from-query") + assert.Equal(t, "from-query", got.Sort, "the query still wins when present") + + send("application/x-www-form-urlencoded", "UserID=evil&Filter=evil&ID=evil", "") + assert.Equal(t, "from-path", got.ID) + assert.Empty(t, got.UserID, "the same holds for form bodies") + assert.Empty(t, got.Filter) +} + +func TestParamOnlyFields(t *testing.T) { + type input struct { + Path string `uri:"id"` + Header string `header:"X-A"` + Hidden string `header:"X-B" json:"-"` + Both string `query:"q" json:"q,omitempty"` + FormBoth string `query:"f" form:"f"` + Body string `json:"body"` + private string `header:"X-C"` + } + assert.Equal(t, []int{0, 1, 2}, paramOnlyFields(reflect.TypeFor[input]())) + assert.Equal(t, []int{0, 1, 2}, paramOnlyFields(reflect.TypeFor[*input]())) + assert.Nil(t, paramOnlyFields(reflect.TypeFor[string]())) + assert.Nil(t, paramOnlyFields(nil)) +} diff --git a/common.go b/common.go index f4dd056..f242529 100644 --- a/common.go +++ b/common.go @@ -27,6 +27,9 @@ type inputShape struct { // rawBody is the index of the []byte field tagged body:"raw", which // receives the request body as-is, or -1. rawBody int + // paramOnly lists the fields bound from the path, the query or the + // headers that declare no body tag (see paramOnlyFields). + paramOnly []int } var shapeCache sync.Map // map[reflect.Type]*inputShape @@ -40,8 +43,9 @@ func shapeFor[TInput any]() *inputShape { } rawBody, _ := rawBodyField(t) // an invalid declaration panics in Method s := &inputShape{ - isStruct: t != nil && dereferenceType(t).Kind() == reflect.Struct, - rawBody: rawBody, + isStruct: t != nil && dereferenceType(t).Kind() == reflect.Struct, + rawBody: rawBody, + paramOnly: paramOnlyFields(t), } actual, _ := shapeCache.LoadOrStore(t, s) return actual.(*inputShape) @@ -86,6 +90,11 @@ func parseInput[TInput any](app *OApiApp, c fiber.Ctx, path string, options *Ope } } } + // The decoder may have filled a path, query or header field from a + // body key with the same name. Binding below only overwrites the + // parameters present in the request, so reset those fields first: a + // header absent from the request must not keep a value from the body. + resetFields(&input, shape.paramOnly) } if shape.isStruct { @@ -158,6 +167,62 @@ func rawBodyField(t reflect.Type) (int, error) { var bytesType = reflect.TypeFor[[]byte]() +// bodyTags are the struct tags through which a body decoder fills a field. +var bodyTags = []string{"json", "form", "xml", "msgpack", "cbor"} + +// paramOnlyFields returns the indexes of the fields of the input struct t +// that are bound from the path, the query or the headers and declare no body +// tag (a "-" tag counts as none). Body decoders still fill such a field when a +// key matches its name, so parseInput resets them after decoding the body. A +// field that declares both, e.g. query:"name" json:"name", accepts either. +func paramOnlyFields(t reflect.Type) []int { + if t == nil { + return nil + } + t = dereferenceType(t) + if t.Kind() != reflect.Struct { + return nil + } + var fields []int + for i := 0; i < t.NumField(); i++ { + field := t.Field(i) + if !field.IsExported() { + continue + } + if field.Tag.Get("uri") == "" && field.Tag.Get("query") == "" && field.Tag.Get("header") == "" { + continue + } + fromBody := false + for _, tag := range bodyTags { + if name, _, _ := strings.Cut(field.Tag.Get(tag), ","); name != "" && name != "-" { + fromBody = true + break + } + } + if !fromBody { + fields = append(fields, i) + } + } + return fields +} + +// resetFields sets the given fields of input to their zero value. +func resetFields[TInput any](input *TInput, fields []int) { + if len(fields) == 0 { + return + } + v := reflect.ValueOf(input).Elem() + if v.Kind() == reflect.Ptr { + if v.IsNil() { + return + } + v = v.Elem() + } + for _, i := range fields { + v.Field(i).SetZero() + } +} + // setRawBody stores body in the raw body field of input, allocating the // struct when the input is a nil pointer. func setRawBody[TInput any](input *TInput, field int, body []byte) { diff --git a/header_params_test.go b/header_params_test.go index 878545a..b1e0879 100644 --- a/header_params_test.go +++ b/header_params_test.go @@ -266,20 +266,17 @@ func TestHeaderNotInRequestBody(t *testing.T) { _, hasName := properties["name"] assert.True(t, hasName, "JSON body field should appear in request body schema") - // Verify that sending the header field in JSON body without the actual header - // does NOT satisfy the header requirement when using json:"-". - // Without json:"-", c.BodyParser can populate exported fields by Go name. - // Header parsing runs after body parsing, so real headers always take priority. + // Sending the header field in the JSON body without the actual header does + // NOT satisfy the header requirement: the JSON decoder fills RequestID by + // Go field name, but a header-only field (no json tag) is reset after the + // body is decoded, so validate:"required" fails. bodyReq := httptest.NewRequest(http.MethodPost, "/items", strings.NewReader(`{"RequestID":"from-body","name":"test"}`)) bodyReq.Header.Set("Content-Type", "application/json") - // No x-request-id header set — body sets RequestID via Go field name match - // but header parser doesn't overwrite (no header present), so body value persists. - // Use json:"-" on header fields to prevent body injection. bodyResp, err := app.Test(bodyReq) require.NoError(t, err) - assert.Equal(t, 200, bodyResp.StatusCode) + assert.Equal(t, 422, bodyResp.StatusCode) } func TestHeaderTakesPriorityOverBody(t *testing.T) { diff --git a/problem.go b/problem.go index 038c682..72d60d5 100644 --- a/problem.go +++ b/problem.go @@ -27,14 +27,17 @@ type Problem struct { Errors []ValidationErrorEntry `json:"errors,omitempty" yaml:"errors,omitempty"` } +// The methods of Problem have value receivers, so that a Problem works the +// same whether it is passed by value or by pointer. + // HTTPStatus returns the status of the problem. -func (p *Problem) HTTPStatus() int { return p.Status } +func (p Problem) HTTPStatus() int { return p.Status } // ContentType returns application/problem+json. -func (p *Problem) ContentType() string { return MIMEApplicationProblemJSON } +func (p Problem) ContentType() string { return MIMEApplicationProblemJSON } // Error returns the title of the problem. -func (p *Problem) Error() string { return p.Title } +func (p Problem) Error() string { return p.Title } // ContentTyper is an optional interface an error value (TError, an entry of // OpenAPIOptions.Errors) can implement to choose the Content-Type it is sent diff --git a/problem_test.go b/problem_test.go index a87231f..857a521 100644 --- a/problem_test.go +++ b/problem_test.go @@ -242,3 +242,27 @@ func operationResponses(oapi *OApiApp, path, method string) map[string]any { spec := oapi.GenerateOpenAPISpec() return spec["paths"].(map[string]any)[path].(map[string]any)[method].(map[string]any)["responses"].(map[string]any) } + +func TestProblem_ByValue(t *testing.T) { + app := fiber.New() + oapi := New(app) + Get(oapi, "/value", func(c fiber.Ctx, _ struct{}) (fiber.Map, Problem) { + return nil, Problem{Status: 409, Title: "Conflict", Code: "already_exists"} + }, OpenAPIOptions{}) + Get(oapi, "/declared", func(c fiber.Ctx, _ struct{}) (fiber.Map, error) { + return nil, Problem{Status: 409, Title: "Conflict"} + }, OpenAPIOptions{Errors: []any{Problem{Status: 409, Title: "Conflict"}}}) + + status, p := readProblem(t, app, httptest.NewRequest("GET", "/value", nil)) + assert.Equal(t, 409, status) + assert.Equal(t, "already_exists", p.Code) + status, _ = readProblem(t, app, httptest.NewRequest("GET", "/declared", nil)) + assert.Equal(t, 409, status) + + catchAll := operationResponses(oapi, "/value", "get")["4XX"].(map[string]any)["content"].(map[string]any) + assert.Contains(t, catchAll, MIMEApplicationProblemJSON) + declared := operationResponses(oapi, "/declared", "get") + require.Contains(t, declared, "409", "the status comes from HTTPStatus(), not from the Code string") + assert.Contains(t, declared["409"].(map[string]any)["content"], MIMEApplicationProblemJSON) + assert.NotContains(t, declared, "500") +} diff --git a/responder.go b/responder.go index 4548a1c..459f4fc 100644 --- a/responder.go +++ b/responder.go @@ -28,8 +28,9 @@ type RawResponse struct { Body []byte } -// Respond writes the status, the Content-Type and the body. -func (r *RawResponse) Respond(c fiber.Ctx) error { +// Respond writes the status, the Content-Type and the body. Its receiver is a +// value, so a RawResponse works by value as well as by pointer. +func (r RawResponse) Respond(c fiber.Ctx) error { if r.Status != 0 { c.Status(r.Status) } diff --git a/responder_test.go b/responder_test.go index 56f5baa..8984eae 100644 --- a/responder_test.go +++ b/responder_test.go @@ -162,3 +162,19 @@ func TestResponseContentType_JSONOutput(t *testing.T) { assert.Equal(t, "application/vnd.api+json", resp.Header.Get("Content-Type")) assert.Contains(t, operationResponses(oapi, "/doc", "get")["200"].(map[string]any)["content"], "application/vnd.api+json") } + +func TestRawResponse_ByValue(t *testing.T) { + app := fiber.New() + oapi := New(app) + Get(oapi, "/value", func(c fiber.Ctx, _ struct{}) (RawResponse, struct{}) { + return RawResponse{ContentType: "image/png", Body: []byte("png")}, struct{}{} + }, OpenAPIOptions{}) + + resp, err := app.Test(httptest.NewRequest("GET", "/value", nil)) + require.NoError(t, err) + raw, _ := io.ReadAll(resp.Body) + assert.Equal(t, 200, resp.StatusCode) + assert.Equal(t, "image/png", resp.Header.Get("Content-Type")) + assert.Equal(t, "png", string(raw), "a RawResponse by value is written as-is, not encoded as JSON") + assert.Contains(t, operationResponses(oapi, "/value", "get")["200"].(map[string]any)["content"], fiber.MIMEOctetStream) +} diff --git a/token_validator.go b/token_validator.go index ef29c49..f507573 100644 --- a/token_validator.go +++ b/token_validator.go @@ -53,7 +53,21 @@ type ContextTokenValidator interface { // service implements ContextTokenValidator. func validateToken(c fiber.Ctx, authService AuthorizationService, token string) (*AuthContext, error) { if ctxValidator, ok := authService.(ContextTokenValidator); ok { - return ctxValidator.ValidateTokenCtx(c, token) + return requireAuthContext(ctxValidator.ValidateTokenCtx(c, token)) } - return authService.ValidateToken(token) + return requireAuthContext(authService.ValidateToken(token)) +} + +// errNoAuthContext reports a validator that accepted the credentials without +// returning an AuthContext. The request is treated as unauthenticated (401). +var errNoAuthContext = errors.New("authentication failed: the validator returned no authentication context") + +// requireAuthContext turns a (nil, nil) result of a validator into an +// authentication failure, so that nothing downstream dereferences a nil +// AuthContext. +func requireAuthContext(authCtx *AuthContext, err error) (*AuthContext, error) { + if err == nil && authCtx == nil { + return nil, errNoAuthContext + } + return authCtx, err } diff --git a/token_validator_test.go b/token_validator_test.go index f05b329..fd9c1c6 100644 --- a/token_validator_test.go +++ b/token_validator_test.go @@ -2,10 +2,12 @@ package fiberoapi import ( "errors" + "strings" "testing" "github.com/gofiber/fiber/v3" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func newTokenValidatorApp(t *testing.T, svc AuthorizationService) (*fiber.App, *OApiApp) { @@ -128,3 +130,56 @@ func TestContextTokenValidator_IsPreferred(t *testing.T) { assert.NoError(t, err) assert.Equal(t, 200, resp.StatusCode, "BearerTokenMiddleware prefers ValidateTokenCtx too") } + +// nilContextService accepts every credential without returning a context. +type nilContextService struct{ causeAuthService } + +func (nilContextService) ValidateToken(string) (*AuthContext, error) { return nil, nil } +func (nilContextService) ValidateBasicAuth(string, string) (*AuthContext, error) { return nil, nil } + +func TestNilAuthContext_IsAnAuthenticationFailure(t *testing.T) { + for name, schemes := range map[string]map[string]SecurityScheme{ + "bearer only": nil, + "security schemes": {"bearerAuth": {Type: "http", Scheme: "bearer"}}, + } { + t.Run(name, func(t *testing.T) { + app, decodes := newSpyApp() + oapi := New(app, Config{ + EnableValidation: true, + EnableAuthorization: true, + AuthService: nilContextService{}, + SecuritySchemes: schemes, + }) + registerAuthFirstRoute(oapi) + + assert.Equal(t, 401, postJSON(t, app, "/items", "any", `{"name": "widget"}`)) + assert.Zero(t, decodes.Load()) + }) + } + + basic := SecurityScheme{Type: "http", Scheme: "basic"} + for name, mw := range map[string]fiber.Handler{ + "BearerTokenMiddleware": BearerTokenMiddleware(nilContextService{}), + "SmartAuthMiddleware": SmartAuthMiddleware(nilContextService{}, Config{ + SecuritySchemes: map[string]SecurityScheme{"bearerAuth": {Type: "http", Scheme: "bearer"}}, + }), + "BasicAuthMiddleware": BasicAuthMiddleware(nilContextService{}), + "MultiSchemeAuthMiddleware basic": MultiSchemeAuthMiddleware(nilContextService{}, Config{ + SecuritySchemes: map[string]SecurityScheme{"basicAuth": basic}, + }), + } { + t.Run(name, func(t *testing.T) { + app := fiber.New() + app.Use(mw) + app.Get("/me", func(c fiber.Ctx) error { return c.SendString("ok") }) + + req := newBearerRequest("GET", "/me", "any") + if strings.Contains(name, "Basic") || strings.Contains(name, "basic") { + req.SetBasicAuth("user", "pass") + } + resp, err := app.Test(req) + require.NoError(t, err) + assert.Equal(t, 401, resp.StatusCode) + }) + } +}