From fc567ec7691bf10239e88721ac083aca91977b5a Mon Sep 17 00:00:00 2001 From: Rex Raphael Date: Thu, 30 Jul 2026 10:01:54 -0500 Subject: [PATCH] fix(http): enforce required nil pointers in bodies, not just parameters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A required pointer field in a request body was silently accepted when absent. The nil check was guarded by isParamField, so it only ran for query, path and header parameters — while IsFieldRequired returns true for a `required:"true"` pointer regardless, so the generated OpenAPI advertised the field as required. The published contract and the runtime disagreed, which is worse than either being wrong on its own: clients trust the spec. A pointer is how a caller says "I need to tell absent from zero". It is not a statement about whether the field may be omitted — that is what required/optional are for. Also collapses the two empty-string branches into one and confines the check to value types. For a non-pointer string, {"name":""} and {} are indistinguishable once decoded, so "" is the only available proxy for "not supplied" — but a non-nil pointer was demonstrably supplied, so "" behind one is a real value. Applying the proxy there would have made the pointer form pointless, and did: a required *string set to "" was rejected. Together these make the two forms mean distinct, useful things: Name string `required:"true"` // must be supplied and non-empty Name *string `required:"true"` // must be supplied; "" is allowed Tests cover both pointer cases. The absent case fails when the isParamField guard is restored. Not included: dropping omitempty as an optionality marker. It is a serialization directive doing double duty as a deserialization contract, and it is the reason optional:"true" goes unnoticed — but removing the inference flips every such field from optional to required. That is 699 fields in authsome alone, breaking 77 tests across 15 packages, and it would reject previously-valid client requests across every consumer. It needs a major version and a codemod, not a patch. --- http/binder_test.go | 38 ++++++++++++++++++++++++++++++++++++++ http/validator.go | 35 ++++++++++++++++++++++------------- 2 files changed, 60 insertions(+), 13 deletions(-) diff --git a/http/binder_test.go b/http/binder_test.go index a3187ff..645ac65 100644 --- a/http/binder_test.go +++ b/http/binder_test.go @@ -1269,3 +1269,41 @@ func TestBindRequest_JsonBodyEmptyStringBothRequired(t *testing.T) { require.True(t, ok) assert.True(t, valErrors.HasErrors()) } + +// bindJSON binds body into dst and returns the resulting error, if any. +func bindJSON(t *testing.T, body string, dst any) error { + t.Helper() + + req := httptest.NewRequest(http.MethodPost, "/test", bytes.NewBufferString(body)) + req.Header.Set("Content-Type", "application/json") + ctx := NewContext(httptest.NewRecorder(), req, nil).(*Ctx) + + return ctx.BindRequest(dst) +} + +// A pointer is how a caller expresses "must be supplied, but may be empty": +// nil is absent, "" is supplied-and-empty. Previously a nil required pointer +// was only reported for parameters, so a required *T in a body was silently +// accepted when absent while the generated OpenAPI still advertised it as +// required — the published contract and the runtime disagreed. +type RequiredPointerRequest struct { + Name *string `json:"name" required:"true"` +} + +func TestBindRequest_RequiredPointerRejectsAbsent(t *testing.T) { + var got RequiredPointerRequest + + require.Error(t, bindJSON(t, `{}`, &got), + "an absent required pointer must fail") +} + +// The presence of the pointer is what was checked; "" behind it is a real +// value, not a missing one, so the emptiness proxy must not apply here. +func TestBindRequest_RequiredPointerAcceptsEmpty(t *testing.T) { + var got RequiredPointerRequest + + require.NoError(t, bindJSON(t, `{"name": ""}`, &got), + "a supplied-but-empty required pointer is valid") + require.NotNil(t, got.Name) + assert.Equal(t, "", *got.Name) +} diff --git a/http/validator.go b/http/validator.go index 1f1dca6..e864994 100644 --- a/http/validator.go +++ b/http/validator.go @@ -193,8 +193,6 @@ func (c *Ctx) validateCustomTags(rv reflect.Value, rt reflect.Type, errors *val. field.Tag.Get("multipleOf") != "" || field.Tag.Get("enum") != "" - // Also check for required validation on parameter fields - isParamField := val.IsParameterField(field) fieldRequired := val.IsFieldRequired(field) // Skip if no validation needed: no custom tags AND (not required OR is optional) @@ -205,9 +203,17 @@ func (c *Ctx) validateCustomTags(rv reflect.Value, rt reflect.Type, errors *val. fieldName := val.GetFieldName(field) // Handle pointer fields + wasPointer := fieldValue.Kind() == reflect.Ptr if fieldValue.Kind() == reflect.Ptr { if fieldValue.IsNil() { - if fieldRequired && isParamField { + // Applies to body fields as well as parameters. Restricting + // this to parameters meant a required *T in a body was + // silently accepted when absent, while the generated OpenAPI + // still advertised it as required — the published contract + // and the runtime disagreed. A pointer is how a caller says + // "I need to tell absent from zero"; it is not a statement + // about whether the field may be omitted. + if fieldRequired { errors.AddWithCode(fieldName, "field is required", val.ErrCodeRequired, nil) } @@ -217,16 +223,19 @@ func (c *Ctx) validateCustomTags(rv reflect.Value, rt reflect.Type, errors *val. fieldValue = fieldValue.Elem() } - // Required validation for parameter string fields - if fieldRequired && isParamField && fieldValue.Kind() == reflect.String && fieldValue.String() == "" { - errors.AddWithCode(fieldName, "field is required", val.ErrCodeRequired, "") - - continue - } - - // Required validation for non-parameter (body) string fields - // go-playground/validator doesn't catch empty strings for required fields - if fieldRequired && !isParamField && fieldValue.Kind() == reflect.String && fieldValue.String() == "" { + // Value types only, and "" is a proxy for absence rather than a + // judgement about emptiness: once JSON is decoded into a non-pointer + // string, {"name":""} and {} are indistinguishable, so this is the + // only signal available at this layer. + // + // It must not extend to pointers. A non-nil pointer was demonstrably + // supplied — presence was already established above — so "" behind + // one is a real value. A field that must be present yet may + // legitimately be empty is therefore expressed as a pointer. + // + // The parameter and body cases were separate branches doing the same + // thing; they are one check now. + if fieldRequired && !wasPointer && fieldValue.Kind() == reflect.String && fieldValue.String() == "" { errors.AddWithCode(fieldName, "field is required", val.ErrCodeRequired, "") continue