Skip to content
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

A collison between hertz customed type binding and 'required' tag check #1007

Closed
iksars opened this issue Nov 26, 2023 · 3 comments · May be fixed by #1058
Closed

A collison between hertz customed type binding and 'required' tag check #1007

iksars opened this issue Nov 26, 2023 · 3 comments · May be fixed by #1058
Assignees
Labels
question Further information is requested

Comments

@iksars
Copy link

iksars commented Nov 26, 2023

Describe the Question
I use hertz thrift code generation for my project and I meet a problem about binding param of customed Type. I mark the customed type with "required" tag and "api.header=' X-Authorization-TokenPayload'" tag in the thrift file. Then I inject the customed bindConfig into hertz server . When there is a filed named 'X-Authorization-TokenPayload' in request header, it works well. However , when there is not a filed named that, I hope that hertz can throw a bind error but actually it doesn't. It seems that "required" tag can't work because of customed bind func. Is it a bug? If not, how can I solve it? (hz version v0.6.5)

idl define:

struct CreateUploadTaskReq {
    1: required base.TokenPayload TokenPayload (api.header="X-Authorization-TokenPayload");   
    2: .....  
}

customed bindconfig:

func bindTokenPayloadConfig() *binding.BindConfig {
	bindConfig := &binding.BindConfig{}
	bindConfig.MustRegTypeUnmarshal(reflect.TypeOf(base.TokenPayload{}), func(req *protocol.Request, params param.Params, text string) (reflect.Value, error) {
		hlog.Info("tokenpayload", "text:", text)
		if text == "" {
			return reflect.ValueOf(base.TokenPayload{}), errors.New("tokenpayload is required but not found")
		}
		val := base.TokenPayload{}
		if err := json.Unmarshal([]byte(text), &val); err != nil {
			return reflect.ValueOf(base.TokenPayload{}), err
		}
		return reflect.ValueOf(val), nil
	})
	return bindConfig
}
@FGYFFFF
Copy link
Contributor

FGYFFFF commented Nov 27, 2023

The previous idea was that a custom binding would allow the user to do all the actions for that field, but it seems like Required shouldn't be done by the user. So I'll put the Required validation into hertz. I'll mention a pr.

@li-jin-gou li-jin-gou added the question Further information is requested label Dec 11, 2023
iksars added a commit to iksars/hertz-develop that referenced this issue Feb 1, 2024
…cated with customed type is invalid in previous version. This pr solves this problem by adding required check for customed type binding. What's more, I find that custom type should not only use json tag, because it's useless. This should be pointed out in the document. \n Closes cloudwego#1007
@archever
Copy link

type ExchangeOauth2CodeReq struct {
	// 标记
	State string `thrift:"State,1,required" validate:"required"`
	// code
	Code string `thrift:"Code,2,required" validate:"required"`
}

func Ping(c context.Context, ctx *app.RequestContext) {
	req := idl_plugin.ExchangeOauth2CodeReq{}
	err := ctx.BindAndValidate(&req)
	if err != nil {
		ResponseErr(ctx, fmt.Errorf("BindAndValidate error: %w", err))
		return
	}
	ctx.JSON(http.StatusOK, req)
}
curl "127.0.0.1:6789/ping" -d '{"State":"xxx", "Code":"xxx"}' -H "content-type: application/json" -H "State: abc"
// {"State":"abc","Code":"xxx"}

hertz 的 validate:"required" tag 会自动绑定 header 的同名字段, 并覆盖到 body 里面, 这个问题会影响接口安全, 导致body 内容被覆盖

@archever
Copy link

curl "127.0.0.1:6789/open/api/v1/ping?State=abc" -d '{"State":"xxx", "Code":"xxx"}' -H "content-type: application/json" 
// {"State":"abc","Code":"xxx"}

query 里面也会覆盖掉

@FGYFFFF FGYFFFF closed this as completed Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Development

Successfully merging a pull request may close this issue.

4 participants