Skip to content

Commit 1d0a337

Browse files
Merge commit from fork
* openapi3filter: add regression test for nil-pointer panic in ConvertErrors (GHSA-mmfr-pmjx-hw9w) convertParseError dereferences e.Parameter.In in the "query" branch without a nil guard. For request-body errors e.Parameter is always nil, and only the multipart/form-data decoder produces the nested *ParseError shape that reaches that branch, so a malformed scalar part (e.g. a non-numeric value for an integer property) crashes any application that renders validation errors through ConvertErrors / ValidationErrorEncoder. This end-to-end test drives ValidateRequest -> ConvertErrors and asserts no panic plus a meaningful 400 for the multipart case, with application/json control cases. It fails (panics) on the current code and documents the expected post-fix behavior. Signed-off-by: Matías Insaurralde <matias@insaurral.de> * openapi3filter: fix nil-pointer panic in ConvertErrors on multipart body errors convertParseError dereferenced e.Parameter.In in the "query" branch without a nil guard. Body parse errors always have e.Parameter == nil (a RequestError carries either Parameter or RequestBody, never both), and the multipart/form-data decoder wraps a failed part's *ParseError inside another *ParseError, producing the nested shape that reaches that branch. A single malformed scalar part (e.g. a non-numeric value for an integer property) therefore crashed any app rendering errors via ConvertErrors / ValidationErrorEncoder -- an unauthenticated remote DoS. Add the missing `e.Parameter != nil` guard, mirroring the existing "path" branch. When the guard fails, control falls through to the pre-existing 400 body-error fallback, so query-param error rendering is unchanged. Also fall back to innerErr.Error() when innerErr.Reason is empty: the outer ParseError from the multipart decoder has no Reason, which would otherwise yield a 400 with an empty Title. The full error text (e.g. "path age: value notanumber: an invalid integer: invalid syntax") is more useful. Signed-off-by: Matías Insaurralde <matias@insaurral.de> --------- Signed-off-by: Matías Insaurralde <matias@insaurral.de>
1 parent 98d9564 commit 1d0a337

2 files changed

Lines changed: 149 additions & 2 deletions

File tree

Lines changed: 139 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,139 @@
1+
package openapi3filter_test
2+
3+
import (
4+
"bytes"
5+
"mime/multipart"
6+
"net/http"
7+
"strings"
8+
"testing"
9+
10+
"github.com/stretchr/testify/require"
11+
12+
"github.com/getkin/kin-openapi/openapi3"
13+
"github.com/getkin/kin-openapi/openapi3filter"
14+
"github.com/getkin/kin-openapi/routers/gorillamux"
15+
)
16+
17+
// GHSA-mmfr-pmjx-hw9w: a nil-pointer dereference in convertParseError (reached
18+
// via ConvertErrors / ValidationErrorEncoder) lets an unauthenticated client
19+
// crash a server with a single multipart/form-data request.
20+
//
21+
// convertParseError dereferences e.Parameter.In in the "query" branch without a
22+
// nil guard. For request-body errors e.Parameter is always nil, and only the
23+
// multipart decoder produces the nested *ParseError shape that reaches that
24+
// branch, so a malformed scalar part (e.g. a non-numeric value for an integer
25+
// property) panics.
26+
//
27+
// Sending the crafted request through ValidateRequest and then ConvertErrors
28+
// must not panic; it must return a 400 with a meaningful message. The
29+
// application/json control cases confirm the JSON body path is unaffected.
30+
func TestGHSA_mmfr_pmjx_hw9w_ConvertErrors_NoPanic(t *testing.T) {
31+
spec := `
32+
openapi: '3.0.3'
33+
info:
34+
title: t
35+
version: '1.0.0'
36+
paths:
37+
/upload:
38+
post:
39+
requestBody:
40+
required: true
41+
content:
42+
multipart/form-data:
43+
schema:
44+
type: object
45+
properties:
46+
age: {type: integer}
47+
application/json:
48+
schema:
49+
type: object
50+
properties:
51+
age: {type: integer}
52+
responses:
53+
'200': {description: ok}
54+
`[1:]
55+
56+
loader := openapi3.NewLoader()
57+
ctx := loader.Context
58+
59+
doc, err := loader.LoadFromData([]byte(spec))
60+
require.NoError(t, err)
61+
require.NoError(t, doc.Validate(ctx))
62+
63+
router, err := gorillamux.NewRouter(doc)
64+
require.NoError(t, err)
65+
66+
multipartBody := func() (string, string) {
67+
var buf bytes.Buffer
68+
w := multipart.NewWriter(&buf)
69+
require.NoError(t, w.WriteField("age", "notanumber"))
70+
require.NoError(t, w.Close())
71+
return w.FormDataContentType(), buf.String()
72+
}
73+
74+
for _, tc := range []struct {
75+
name string
76+
contentType string
77+
body string
78+
wantStatus int
79+
}{
80+
{
81+
// The crash: multipart scalar part fails primitive parsing.
82+
// e.Parameter == nil and the cause is a nested *ParseError.
83+
name: "multipart malformed scalar (the crash)",
84+
body: "notanumber",
85+
wantStatus: http.StatusBadRequest,
86+
},
87+
{
88+
// Control: malformed JSON body. *ParseError whose cause is an
89+
// encoding/json error, so the type assertion fails and the safe
90+
// fallback branch is taken.
91+
name: "json malformed body",
92+
contentType: "application/json",
93+
body: `{"age": `,
94+
wantStatus: http.StatusBadRequest,
95+
},
96+
{
97+
// Control: wrong-type JSON body. Routed through convertSchemaError
98+
// (422), never reaching convertParseError.
99+
name: "json wrong-type body",
100+
contentType: "application/json",
101+
body: `{"age": "notanumber"}`,
102+
wantStatus: http.StatusUnprocessableEntity,
103+
},
104+
} {
105+
t.Run(tc.name, func(t *testing.T) {
106+
contentType, body := tc.contentType, tc.body
107+
if contentType == "" {
108+
contentType, body = multipartBody()
109+
}
110+
111+
req, err := http.NewRequest(http.MethodPost, "/upload", strings.NewReader(body))
112+
require.NoError(t, err)
113+
req.Header.Set("Content-Type", contentType)
114+
115+
route, pathParams, err := router.FindRoute(req)
116+
require.NoError(t, err)
117+
118+
reqErr := openapi3filter.ValidateRequest(ctx, &openapi3filter.RequestValidationInput{
119+
Request: req,
120+
PathParams: pathParams,
121+
Route: route,
122+
Options: &openapi3filter.Options{AuthenticationFunc: openapi3filter.NoopAuthenticationFunc},
123+
})
124+
require.Error(t, reqErr)
125+
126+
// This is what a typical error-rendering middleware calls; it must
127+
// not panic.
128+
var converted error
129+
require.NotPanics(t, func() {
130+
converted = openapi3filter.ConvertErrors(reqErr)
131+
})
132+
133+
var validationErr *openapi3filter.ValidationError
134+
require.ErrorAs(t, converted, &validationErr)
135+
require.Equal(t, tc.wantStatus, validationErr.Status)
136+
require.NotEmpty(t, validationErr.Title, "converted error should carry a meaningful message")
137+
})
138+
}
139+
}

openapi3filter/validation_error_encoder.go

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -117,16 +117,24 @@ func convertParseError(e *RequestError, innerErr *ParseError) *ValidationError {
117117
}
118118
} else if innerErr.RootCause() != nil {
119119
if rootErr, ok := innerErr.Cause.(*ParseError); ok &&
120-
rootErr.Kind == KindInvalidFormat && e.Parameter.In == "query" {
120+
rootErr.Kind == KindInvalidFormat && e.Parameter != nil && e.Parameter.In == "query" {
121121
return &ValidationError{
122122
Status: http.StatusBadRequest,
123123
Title: fmt.Sprintf("parameter %q in %s is invalid: %v is %s",
124124
e.Parameter.Name, e.Parameter.In, rootErr.Value, rootErr.Reason),
125125
}
126126
}
127+
// For body parse errors (e.Parameter == nil) the outer ParseError's
128+
// Reason is often empty, e.g. the multipart decoder wraps a part's
129+
// *ParseError without setting one. Fall back to the full error text so
130+
// the response still carries a meaningful message.
131+
title := innerErr.Reason
132+
if title == "" {
133+
title = innerErr.Error()
134+
}
127135
return &ValidationError{
128136
Status: http.StatusBadRequest,
129-
Title: innerErr.Reason,
137+
Title: title,
130138
}
131139
}
132140
return nil

0 commit comments

Comments
 (0)