Bug in the fix to 624 issue

[orensolo]

Hi,

There is a bug in the fix to #624 issue.
Using a special character in the parameter causes unmarshalling error.

Running the following:

package main

import (
	"context"
	"fmt"
	"net/http"

	"github.com/getkin/kin-openapi/openapi3"
	"github.com/getkin/kin-openapi/openapi3filter"
	"github.com/getkin/kin-openapi/routers/gorillamux"
)

func main() {
	ctx := context.Background()
	loader := &openapi3.Loader{Context: ctx, IsExternalRefsAllowed: true}
	schema :=
		`
openapi: 3.0.0
info:
 version: 1.0.0
 title: Sample API
paths:
 /items:
  get:
   parameters:
    - description: "test non object"
      explode: true
      style: form
      in: query
      name: test
      required: false
      content:
       application/json:
         schema:
          anyOf:
           - type: string
           - type: integer
   description: Returns a list of stuff              
   responses:
    '200':
     description: Successful response`

	doc, err := loader.LoadFromData([]byte(schema))

	if err != nil {
		fmt.Println("failed to load schema. " + err.Error())
		return
	}
	// Validate document

	if err = doc.Validate(ctx); err != nil {
		fmt.Println("failed to validate schema. " + err.Error())
		return
	}

	router, _ := gorillamux.NewRouter(doc)
	httpReq, _ := http.NewRequest(http.MethodGet, `/items?test=test[1`, nil)  // <== note the input parameter contains [

	// Find route
	route, pathParams, _ := router.FindRoute(httpReq)

	// Validate request
	requestValidationInput := &openapi3filter.RequestValidationInput{
		Request:    httpReq,
		PathParams: pathParams,
		Route:      route,
	}
	err = openapi3filter.ValidateRequest(ctx, requestValidationInput)
	if err != nil {
		fmt.Println("failed to validate request. " + err.Error())
		return
	}

}

Returns:

failed to validate request. parameter "test" in query has an error: error unmarshaling parameter "test"

The fix assumes that there cannot be special characters in the input parameters - which is a wrong assumption.
The correct fix is to use the decodeStyledParameter instead of decodeContentParameter. (with some relevant changes)

Please change the fix to the following fix:

In validate_request.go:

func ValidateParameter(ctx context.Context, input *RequestValidationInput, parameter *openapi3.Parameter) error {
	if parameter.Schema == nil && parameter.Content == nil {
		// We have no schema for the parameter. Assume that everything passes
		// a schema-less check, but this could also be an error. The OpenAPI
		// validation allows this to happen.
		return nil
	}

	options := input.Options
	if options == nil {
		options = DefaultOptions
	}

	var value interface{}
	var err error
	var found bool
	var schemaRef *openapi3.SchemaRef

	// Validation will ensure that we either have content or schema.
	if parameter.Content != nil {
		// We only know how to decode a parameter if it has one content, application/json
		if len(parameter.Content) != 1 {
			err = fmt.Errorf("multiple content types for parameter %q", parameter.Name)
			return &RequestError{Input: input, Parameter: parameter, Err: err}
		}

		mt := parameter.Content.Get("application/json")
		if mt == nil {
			err = fmt.Errorf("parameter %q has no content schema", parameter.Name)
			return &RequestError{Input: input, Parameter: parameter, Err: err}
		}
		schemaRef = mt.Schema

	} else {
		schemaRef = parameter.Schema
	}

	if value, found, err = decodeStyledParameter(parameter, input, schemaRef); err != nil {
		return &RequestError{Input: input, Parameter: parameter, Err: err}
	}

	schema := schemaRef.Value
	// Set default value if needed
	if value == nil && schema != nil && schema.Default != nil {
		value = schema.Default
		req := input.Request
		switch parameter.In {
		case openapi3.ParameterInPath:
			// Path parameters are required.
			// Next check `parameter.Required && !found` will catch this.
		case openapi3.ParameterInQuery:
			q := req.URL.Query()
			q.Add(parameter.Name, fmt.Sprintf("%v", value))
			req.URL.RawQuery = q.Encode()
		case openapi3.ParameterInHeader:
			req.Header.Add(parameter.Name, fmt.Sprintf("%v", value))
		case openapi3.ParameterInCookie:
			req.AddCookie(&http.Cookie{
				Name:  parameter.Name,
				Value: fmt.Sprintf("%v", value),
			})
		}
	}

	// Validate a parameter's value and presence.
	if parameter.Required && !found {
		return &RequestError{Input: input, Parameter: parameter, Reason: ErrInvalidRequired.Error(), Err: ErrInvalidRequired}
	}

	if isNilValue(value) {
		if !parameter.AllowEmptyValue && found {
			return &RequestError{Input: input, Parameter: parameter, Reason: ErrInvalidEmptyValue.Error(), Err: ErrInvalidEmptyValue}
		}
		return nil
	}
	if schema == nil {
		// A parameter's schema is not defined so skip validation of a parameter's value.
		return nil
	}

	var opts []openapi3.SchemaValidationOption
	if options.MultiError {
		opts = make([]openapi3.SchemaValidationOption, 0, 1)
		opts = append(opts, openapi3.MultiErrors())
	}
	if err = schema.VisitJSON(value, opts...); err != nil {
		return &RequestError{Input: input, Parameter: parameter, Err: err}
	}
	return nil
}

And in req_resp_decoder.go, change decodeStyledParameter to the following:

func decodeStyledParameter(param *openapi3.Parameter, input *RequestValidationInput, schema *openapi3.SchemaRef) (interface{}, bool, error) {
	sm, err := param.SerializationMethod()
	if err != nil {
		return nil, false, err
	}

	var dec valueDecoder
	switch param.In {
	case openapi3.ParameterInPath:
		if len(input.PathParams) == 0 {
			return nil, false, nil
		}
		dec = &pathParamDecoder{pathParams: input.PathParams}
	case openapi3.ParameterInQuery:
		if len(input.GetQueryParams()) == 0 {
			return nil, false, nil
		}
		dec = &urlValuesDecoder{values: input.GetQueryParams()}
	case openapi3.ParameterInHeader:
		dec = &headerParamDecoder{header: input.Request.Header}
	case openapi3.ParameterInCookie:
		dec = &cookieParamDecoder{req: input.Request}
	default:
		return nil, false, fmt.Errorf("unsupported parameter's 'in': %s", param.In)
	}

	return decodeValue(dec, param.Name, sm, schema, param.Required)
}

Thanks,
Oren

[fenollp]

Hi @orensolo thanks for following up so quickly.
Do you mind opening a PR with your changes? I'd like to not loose history attribution. If that doesn't matter to you and you're short on time I can probably take care of it.

[orensolo]

Hi @fenollp
I also have a bug in my fix. I am working on this and update this issue with the correct fix.

Thanks,

[fenollp]

Any progress on your side? I have this draft PR in the works: #634

[orensolo]

My fix is not good. I am working to find another solution. I will update soon.

[orensolo]

Hi @fenollp

I suggest replacing defaultContentParameterDecoder in req_resp_decoder.go with the following code:

func defaultContentParameterDecoder(param *openapi3.Parameter, values []string) (
	outValue interface{}, outSchema *openapi3.Schema, err error) {
	// Only query parameters can have multiple values.
	if len(values) > 1 && param.In != openapi3.ParameterInQuery {
		err = fmt.Errorf("%s parameter %q cannot have multiple values", param.In, param.Name)
		return
	}

	content := param.Content
	if content == nil {
		err = fmt.Errorf("parameter %q expected to have content", param.Name)
		return
	}
	// We only know how to decode a parameter if it has one content, application/json
	if len(content) != 1 {
		err = fmt.Errorf("multiple content types for parameter %q", param.Name)
		return
	}

	mt := content.Get("application/json")
	if mt == nil {
		err = fmt.Errorf("parameter %q has no content schema", param.Name)
		return
	}
	outSchema = mt.Schema.Value

	unmarshal := func(encoded string, paramSchema *openapi3.SchemaRef) (decoded interface{}, err error) {
		if err = json.Unmarshal([]byte(encoded), &decoded); err != nil {
			if paramSchema != nil && paramSchema.Value.Type != "object" {
				decoded, err = encoded, nil
			}
		}
		return
	}

	if len(values) == 1 {
		if outValue, err = unmarshal(values[0], mt.Schema); err != nil {
			err = fmt.Errorf("error unmarshaling parameter %q", param.Name)
			return
		}
	} else {
		outArray := make([]interface{}, 0, len(values))
		for _, v := range values {
			var item interface{}
			if item, err = unmarshal(v, outSchema.Items); err != nil {
				err = fmt.Errorf("error unmarshaling parameter %q", param.Name)
				return
			}
			outArray = append(outArray, item)
		}
		outValue = outArray
	}
	return
}

This is similar to your fix. Just the condition to fallback the error is different (depends on the schema).

Thanks,
Oren