From 60e1ae4a014ff158513922aff1597ec974945546 Mon Sep 17 00:00:00 2001 From: chavacava Date: Fri, 15 Jul 2022 20:15:55 +0200 Subject: [PATCH] adds checks for protobuf struct tags (#707) * adds checks for protobuf struct tags * use actual tag numbers as key instead of strings removes debug println --- rule/struct-tag.go | 76 ++++++++++++++++++++++++++++++++++++++---- testdata/struct-tag.go | 22 ++++++++++-- 2 files changed, 89 insertions(+), 9 deletions(-) diff --git a/rule/struct-tag.go b/rule/struct-tag.go index 6e3e00b5b..3accf58fb 100644 --- a/rule/struct-tag.go +++ b/rule/struct-tag.go @@ -35,7 +35,7 @@ func (*StructTagRule) Name() string { type lintStructTagRule struct { onFailure func(lint.Failure) - usedTagNbr map[string]bool // list of used tag numbers + usedTagNbr map[int]bool // list of used tag numbers usedTagName map[string]bool // list of used tag keys } @@ -45,7 +45,7 @@ func (w lintStructTagRule) Visit(node ast.Node) ast.Visitor { if n.Fields == nil || n.Fields.NumFields() < 1 { return nil // skip empty structs } - w.usedTagNbr = map[string]bool{} // init + w.usedTagNbr = map[int]bool{} // init w.usedTagName = map[string]bool{} // init for _, f := range n.Fields.List { if f.Tag != nil { @@ -74,6 +74,10 @@ func (w lintStructTagRule) checkTagNameIfNeed(tag *structtag.Tag) (string, bool) } tagName := w.getTagName(tag) + if tagName == "" { + return "", true // No tag name found + } + // We concat the key and name as the mapping key here // to allow the same tag name in different tag type. key := tag.Key + ":" + tagName @@ -94,7 +98,7 @@ func (lintStructTagRule) getTagName(tag *structtag.Tag) string { return strings.TrimLeft(option, "name=") } } - return "protobuf tag lacks name" + return "" //protobuf tag lacks 'name' option default: return tag.Name } @@ -139,7 +143,10 @@ func (w lintStructTagRule) checkTaggedField(f *ast.Field) { w.addFailure(f.Tag, msg) } case "protobuf": - // Not implemented yet + msg, ok := w.checkProtobufTag(tag) + if !ok { + w.addFailure(f.Tag, msg) + } case "required": if tag.Name != "true" && tag.Name != "false" { w.addFailure(f.Tag, "required should be 'true' or 'false'") @@ -170,10 +177,14 @@ func (w lintStructTagRule) checkASN1Tag(t ast.Expr, tag *structtag.Tag) (string, if strings.HasPrefix(opt, "tag:") { parts := strings.Split(opt, ":") tagNumber := parts[1] - if w.usedTagNbr[tagNumber] { - return fmt.Sprintf("duplicated tag number %s", tagNumber), false + number, err := strconv.Atoi(tagNumber) + if err != nil { + return fmt.Sprintf("ASN1 tag must be a number, got '%s'", tagNumber), false + } + if w.usedTagNbr[number] { + return fmt.Sprintf("duplicated tag number %v", number), false } - w.usedTagNbr[tagNumber] = true + w.usedTagNbr[number] = true continue } @@ -275,6 +286,57 @@ func (lintStructTagRule) typeValueMatch(t ast.Expr, val string) bool { return typeMatches } +func (w lintStructTagRule) checkProtobufTag(tag *structtag.Tag) (string, bool) { + // check name + switch tag.Name { + case "bytes", "fixed32", "fixed64", "group", "varint", "zigzag32", "zigzag64": + // do nothing + default: + return fmt.Sprintf("invalid protobuf tag name '%s'", tag.Name), false + } + + // check options + seenOptions := map[string]bool{} + for _, opt := range tag.Options { + if number, err := strconv.Atoi(opt); err == nil { + _, alreadySeen := w.usedTagNbr[number] + if alreadySeen { + return fmt.Sprintf("duplicated tag number %v", number), false + } + w.usedTagNbr[number] = true + continue // option is an integer + } + + switch { + case opt == "opt" || opt == "proto3" || opt == "rep" || opt == "req": + // do nothing + case strings.Contains(opt, "="): + o := strings.Split(opt, "=")[0] + _, alreadySeen := seenOptions[o] + if alreadySeen { + return fmt.Sprintf("protobuf tag has duplicated option '%s'", o), false + } + seenOptions[o] = true + continue + } + } + _, hasName := seenOptions["name"] + if !hasName { + return "protobuf tag lacks mandatory option 'name'", false + } + + for k := range seenOptions { + switch k { + case "name", "json": + // do nothing + default: + return fmt.Sprintf("unknown option '%s' in protobuf tag", k), false + } + } + + return "", true +} + func (w lintStructTagRule) addFailure(n ast.Node, msg string) { w.onFailure(lint.Failure{ Node: n, diff --git a/testdata/struct-tag.go b/testdata/struct-tag.go index 600895a23..21e73aaf6 100644 --- a/testdata/struct-tag.go +++ b/testdata/struct-tag.go @@ -46,8 +46,9 @@ type TestContextSpecificTags2 struct { B int `asn1:"tag:2"` S string `asn1:"tag:0,utf8"` Ints []int `asn1:"set"` - Version int `asn1:"optional,explicit,default:0,tag:0"` // MATCH /duplicated tag number 0/ - Time time.Time `asn1:"explicit,tag:4,other"` // MATCH /unknown option 'other' in ASN1 tag/ + Version int `asn1:"optional,explicit,default:0,tag:000"` // MATCH /duplicated tag number 0/ + Time time.Time `asn1:"explicit,tag:4,other"` // MATCH /unknown option 'other' in ASN1 tag/ + X int `asn1:"explicit,tag:invalid"` // MATCH /ASN1 tag must be a number, got 'invalid'/ } type VirtualMachineRelocateSpecDiskLocator struct { @@ -102,3 +103,20 @@ type HelmChartArgs struct { ReleaseNamespace string `json:"releaseNamespace,omitempty" yaml:"releaseNamespace,omitempty"` ExtraArgs []string `json:"extraArgs,omitempty" yaml:"extraArgs,omitempty"` } + +// Test message for holding primitive types. +type Simple struct { + OBool *bool `protobuf:"varint,1,req,json=oBool"` // MATCH /protobuf tag lacks mandatory option 'name'/ + OInt32 *int32 `protobuf:"varint,2,opt,name=o_int32,jsonx=oInt32"` // MATCH /unknown option 'jsonx' in protobuf tag/ + OInt32Str *int32 `protobuf:"varint,3,rep,name=o_int32_str,name=oInt32Str"` // MATCH /protobuf tag has duplicated option 'name'/ + OInt64 *int64 `protobuf:"varint,4,opt,json=oInt64,name=o_int64,json=oInt64"` // MATCH /protobuf tag has duplicated option 'json'/ + OSint32Str *int32 `protobuf:"zigzag32,11,opt,name=o_sint32_str,json=oSint32Str"` + OSint64Str *int64 `protobuf:"zigzag64,13,opt,name=o_sint32_str,json=oSint64Str"` // MATCH /duplicate tag name: 'o_sint32_str'/ + OFloat *float32 `protobuf:"fixed32,14,opt,name=o_float,json=oFloat"` + ODouble *float64 `protobuf:"fixed64,014,opt,name=o_double,json=oDouble"` // MATCH /duplicated tag number 14/ + ODoubleStr *float64 `protobuf:"fixed6,17,opt,name=o_double_str,json=oDoubleStr"` // MATCH /invalid protobuf tag name 'fixed6'/ + OString *string `protobuf:"bytes,18,opt,name=o_string,json=oString"` + XXX_NoUnkeyedLiteral struct{} `json:"-"` + XXX_unrecognized []byte `json:"-"` + XXX_sizecache int32 `json:"-"` +}